Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dbname and database kwarg in psycopg2.connect() #91

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

IlyaSukhanov
Copy link
Contributor

@IlyaSukhanov IlyaSukhanov commented Sep 6, 2018

*Issue #90

*Description of changes:
psycopg2.connect() Supports 3 variants of operation:

  1. psycopg2.connect("dbname=test user=postgres password=secret")
  2. psycopg2.connect(dbname="test", user="postgres", password="secret")
  3. psycopg2.connect()

Previously we only supported 1 and 2. Now support variant 3 and mix of 2 and 3.

This change is not entierly backwards compatible previously the metadata used to
contain keys url and user. These are no longer exposed however the connection
parameters are available as connection_string. If this backward incompatibility
is not desirable then change can be made to extract values as they were before
using conn.dsn. However the URL will not be correct as password is mangled. It's
not clear to me if one option is clearly better than the other.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@IlyaSukhanov
Copy link
Contributor Author

Looks like the fields do matter. I'll adjust PR to rebuild them of conn.dsn()

@IlyaSukhanov
Copy link
Contributor Author

Made change backwards compatible.

@Tankanow
Copy link
Contributor

Tankanow commented Sep 7, 2018

👍 I like this improvement.

@haotianw465
Copy link
Contributor

Thank you for the contribution. This improvement looks good to me. I think it is a miss on unit test where some use cases are not covered https://github.com/aws/aws-xray-sdk-python/blob/master/tests/ext/psycopg2/test_psycopg2.py. Do you mind adding more common use cases in the test to prevent regression?

@haotianw465 haotianw465 added the bug label Sep 7, 2018
@IlyaSukhanov IlyaSukhanov changed the title Support argument-less invocation of psycopg2.connect() Support dbname and database kwarg in psycopg2.connect() Sep 10, 2018
@IlyaSukhanov
Copy link
Contributor Author

Turns out underlying problem was slightly different but fix remained the same. Test has been added and bug report has been updated. I've squashed commits of this PR to remove erroneous description of problem and make it clearer for anyone reading comments in future.

@haotianw465 haotianw465 merged commit 180de75 into aws:master Sep 11, 2018
@haotianw465
Copy link
Contributor

Thank you for your contribution. I have merged this PR and will prepare a release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants