Bug: specify the port in a creds object

[wahani]

In order to specify the port of a database in a creds object, currently you would have to specify it as part of the host url; like so

CREDS = {
    'user': 'root',
    'password': 'root',
    'host': '0.0.0.0:3307',
    'db': 'test',
    'port': 3307 # this field is simply ignored
}

The port is simply ignored, because we construct the sqlalchemy url for the user like so:

self.db_url = '{}+{}://{}:{}@{}/{}'.format(
                    dialect, driver, user, password, host, db)

where it should be:

self.db_url = '{}+{}://{}:{}@{}:{}/{}'.format(
                    dialect, driver, user, password, host, port, db)

or even better:

self.db_url = '{dialect}+{driver}://{user}:{password}@{host}:{port}/{database}'.format(
        dialect=dialect,
        driver=driver, 
        user=user,
        password=password,
        host=host,
        port=port,
        database=db
)

This leads to seemingly redundant, but necessary snippets in our tests:

dbrequests/tests/conftest.py

creds = {
        'user': 'root',
        'password': 'root',
        'host': '127.0.0.1',
        'db': 'test',
        'port': 3307
    }
    ...
    url = ("mysql+pymysql://{}:{}@{}:{}/{}".format(creds['user'], creds['password'], creds['host'], creds['port'], creds['db']))

and in dbrequests/mysql/tests/conftest.py the weirdly looking:

CREDS = {
    'user': 'root',
    'password': 'root',
    'host': '0.0.0.0:3307',
    'db': 'test',
    'port': 3307
}

I would vote for changing this, so you can supply the port as part of the credentials object. This is of course a breaking change/fix.

Maybe to stir up the discussion:

• should we rethink the approach of having two arguments for the same thing?
• (1) Remove one option entirely
• (2) We could only use one argument (the first) and dispatch on the class:
  • if we have a dict, we have a credentials object, if we have a string we expect a url.
  • This would not make the code any more cumbersome, we already have to do the dispatch when we have None as arguments for either one of the two possible parameters the user may choose.
  • Currently the user can supply a url + credentials in which case the credentials are silently ignored. This would also vanish.

@phainom I am happy to prepare a PR, once we agree on something.

[phainom]

Okay, wow, this is a huge bug, how did it pass unnoticed for so long?

The idea behind having a creds.json was to mimick what we have in the R package and to avoid having to write additional boilerplate around the database creation (in form of coping with credentials and creating the url). I would vote for (2), but also have at least within the current version a separate handling if the port is added to the url, with a deprecation warning, since this is a major change.

[wahani]

Okay. Lets go with option (2) then + adding a deprecation warning + not breaking any code bases.