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

Add parse_dsn module function #321

Merged
merged 5 commits into from Oct 1, 2015

Conversation

Projects
None yet
2 participants
@a1exsh
Copy link
Contributor

a1exsh commented Jun 1, 2015

Calls PQconninfoParse to parse the dsn into a list of keyword and value
structs, then constructs a dictionary from that. Can be useful when one
needs to alter some part of the the connection string reliably, but
doesn't want to get into all the details of parsing a dsn string:
quoting, URL format, etc.

Add parse_dsn module function
Calls PQconninfoParse to parse the dsn into a list of keyword and value
structs, then constructs a dictionary from that.  Can be useful when one
needs to alter some part of the the connection string reliably, but
doesn't want to get into all the details of parsing a dsn string:
quoting, URL format, etc.
@@ -91,6 +92,17 @@ The module interface respects the standard defined in the |DBAPI|_.
The parameters *connection_factory* and *async* are Psycopg extensions
to the |DBAPI|.

.. function:: parse_dsn(dsn)

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

This function should be exposed by psycopg2.extensions, not the main module.


options = PQconninfoParse(dsn, &err);
if (!options) {
PyErr_Format(PyExc_RuntimeError, "PQconninfoParse: %s: %s", dsn, err);

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

This error is wrong. The dsn shouldn't be printed back as it may contain passwords that may end up in the logs. It shoud be a ProgrammingError if the dsn is wrong (with a human error message such as "error parsing the dsn: %s" % err) or an OperationalError if err is null as that's a memory allocation error according to the docs).

for (o = options; o->keyword != NULL; o++) {
if (o->val != NULL) {
value = PyString_FromString(o->val);
if (value == NULL || PyDict_SetItemString(res, o->keyword, value) != 0) {

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

This is terrible, separate the error check from PyString_FromString from the SetItemString!

It is also a reference leak: SetItemString doesn't steal the reference. value must be decref'd.

@@ -112,6 +112,44 @@ psyco_connect(PyObject *self, PyObject *args, PyObject *keywds)
return conn;
}

#define psyco_parse_dsn_doc \
"parse_dsn(dsn) -- Parse database connection string.\n\n"

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

better: parse_dsn(dsn) -> dict. Also I don't think you need the \n's. Even better put all the documentation here and use autodoc to get it in the docs.

res = PyDict_New();
for (o = options; o->keyword != NULL; o++) {
if (o->val != NULL) {
value = PyString_FromString(o->val);

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

Does it work ok on Python3? Probably you need a function like Text_FromUTF8 -- see psycopg/python.h and example of usage all around the lib.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Jun 1, 2015

Thank you for the contribution.

The patch needs tests, both failing and successful, and possibly with some tricky values (quotes, non-quotes, spaces etc).

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Jun 1, 2015

Excellent points, thank you! I will have another look on it and re-submit.

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Jun 1, 2015

I've addressed your feedback on this patch, please have a look when you got a minute.

@dvarrazzo

This comment has been minimized.

Copy link

dvarrazzo commented on tests/test_connection.py in 6a2f21a Jun 1, 2015

The url syntax was only added in PG 9.2. This test would fail if libpq is older. Unfortunately there's no way (yet) to query the libpq version.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Jun 1, 2015

Thank you, it seems much better.

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Jun 1, 2015

Good catch with libq version. I think it's just a matter of exposing PQlibVersion in python and having @skip_before_libpq decorator?

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Jun 1, 2015

Exposing the libpq version is another thing we should have done for a long time, see issue #35.

We sort of dragged it because at the time of opening the likelyhood of that function not being available was high, but now we are planning several features for psycopg 2.7 requiring a more modern libpq so I guess it's time to implement it.

@dvarrazzo dvarrazzo added this to the psycopg 2.7 milestone Jun 2, 2015

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Oct 1, 2015

Hey Daniele, could you please merge this? We could really use reliable DSN parsing in #322 in order to specify the replication=true|database correctly. Thanks!

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Oct 1, 2015

Ok, I'll give it a final review and merge it.

@dvarrazzo dvarrazzo merged commit d604127 into psycopg:master Oct 1, 2015

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Oct 1, 2015

I've made a few fixes and merged, thank you very much.

@a1exsh a1exsh deleted the zalando-stups:feature/parse-dsn branch Oct 1, 2015

@dvarrazzo dvarrazzo referenced this pull request Jun 1, 2016

Closed

conn.dsn #434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment