Psycopg psycopg

Comments and changes to this ticket

• 

  Daniele Varrazzo September 25th, 2012 @ 09:08 PM

  • State changed from “new” to “open”

  You are right. The behaviour is probably changed when we have started passing keyword arguments through to the libpq. You can either specify a conninfo string (first parameter) or use keyword arguments, but if the conninfo is specified the keywords (except connection_factory and async) are ignored altogether. We can probably make connect() more robust and raise an exception if the dsn is used together with any keyword: it would raise an exception as expected in your case.

  Thank you for the report.

• 

  Daniele Varrazzo September 25th, 2012 @ 09:28 PM

  Actually no: the comment in connect() says it was done on purpose: when we added the keyword passthrough we reproduced the same bugs.

  I want to clean up the pile of hacks in the connection function anyway (they are documented in psycopg2/__init__.py: I prefer to avoid doing it for 2.4.6 but we could do it for a following version 2.5.

• You flagged this item as spam.
  

  Adrian Klaver September 25th, 2012 @ 09:38 PM

  I thought I was following, now I am not sure:) Is it considered a bug that the connection_factory argument is wrong and the connection still can be made?

  FYI using 2.4.2 I get:

  con=psycopg2.connect(DSN, conection_factory=psycopg2.extras.RealDictConnection)
  ---------------------------------------------------------------------------
  TypeError                                 Traceback (most recent call last)
  /home/aklaver/<ipython-input-4-44ada10fd237> in <module>()
  ----> 1 con=psycopg2.connect(DSN, conection_factory=psycopg2.extras.RealDictConnection)
  
  TypeError: 'conection_factory' is an invalid keyword argument for this function
  

• 

  Daniele Varrazzo September 25th, 2012 @ 10:13 PM

  Yes, actually it is. I cannot remember exactly in which case the c implementation would have swallowed the keyword arguments, apparently not in all of them. Quoting the comment I put there implementing the python version of connect (i.e. one converting all the keywords into a conninfo string and just handing it to the C connection:

  # Note: reproducing the behaviour of the previous C implementation:
  # keyword are silently swallowed if a DSN is specified. I would have
  # raised an exception. File under "histerical raisins".
  

  Another quirk of the same function is that specifying a negative port it is swallowed as well.

  fog, what shall we do: shall we clean up this parameters handling? We have kept them this way for backward compatibility but honestly we have taken the risk order times to improve the library: I think these quirk should just go. After all swallowing the args and adopt the wrong behaviour (such as not going async because somebody has specified (DSN, asinc=True) is not the most helpful thing...

• You flagged this item as spam.
  

  Adrian Klaver September 25th, 2012 @ 10:32 PM

  Per my example from 2.4.2, in the past the code did choke on improper arguments. I am all for restoring the previous behavior. When someone explicitly calls for a behavior the program should honor that and fail if the information is wrong.

• 

  Daniele Varrazzo September 25th, 2012 @ 10:34 PM

  I propose the attached patch. Is that ok?

  • fix-connect-keywords.patch 2.7 KB

• You flagged this item as spam.
  

  Adrian Klaver September 25th, 2012 @ 10:58 PM

  I not quite sure what that changes. In both cases you end up with:

  return _connect(dsn, connection_factory=connection_factory, async=async)
  

  It would seem that there should be an exception in either case if connection_factory is not correctly specified. Seems to me the problem lies further on in the code where _connect() is processed.

• 

  Daniele Varrazzo September 25th, 2012 @ 11:22 PM

  In your case, either you have specified "conection_factory" together with the dns, which is forbidden, or you don't have a dns, in which case "conection_factory" will be passed as dns to PQconnect, that will raise an exception as the parameter is not supported.

  Oh, now I remember what the above quirk was about: the behaviour emulated was to silently discard valid keywords if the dns was specified, i.e. in:

  connect("dbname=foo", database="bar")
  

  "bar" is swallowed. With my patch it would raise an exception instead.

  The code invoked by connect(), i.e. _connect(), doesn't perform any check at all: it passes the conninfo string to the libpq and raises whatever exception the library complains about (different libpq versions support different arguments).

• You flagged this item as spam.
  

  Adrian Klaver September 25th, 2012 @ 11:30 PM

  How can it be forbidden? When:
  _connect(dsn, connection_factory=connection_factory, async=async)
  I see both a dsn and a connection_factory argument.

• 

  Daniele Varrazzo September 25th, 2012 @ 11:36 PM

  connection_factory and async are not handled by the libpq: they are handled separately as named parameters and don't end up into kwargs. If you spell them wrongly they end up in the kwargs and passed to the libpq, which will moan.

  Please, if you are not convinced, try to apply the patch and see if it works for you. It's pure Python so you don't even need to recompile psycopg. Note that by mistake I've posted a change in setup.cfg that was sitting in my working tree: you can ignore that.

• You flagged this item as spam.
  

  Adrian Klaver September 26th, 2012 @ 02:36 AM

  Patched and it works after a fashion. In particular the error message is confusing:
  InterfaceError: you cannot specify keyword arguments (conection_factory) together with a connection string

  You can specify a keyword argument with a connection string:

  con=psycopg2.connect(DSN, connection_factory=psycopg2.extras.RealDictConnection)

  The error message from 2.4.2 was clearer about was going on:
  'conection_factory' is an invalid keyword argument for this function

  The issue seems to be there are two classes of keyword arguments, those that can be passed to Postgres and those that are psycopg2 specific. In the function signature they are presented as equal, but in reality they are not.
  The patch forces an exception which is a good thing, it just needs some message clarification to reduce possible confusion.

• 

  Daniele Varrazzo September 26th, 2012 @ 03:38 AM

  The problem is that we don't know anymore what keyword argument is allowed and what not: all the keywords (except the two you correctly identify as psycopg-specific) are passed through to the libpq after values escaping.

  The message states the fact that "conection_factory" is not allowed as a keyword when DSN is specified: we don't know if it's a good keyword or not. If you try to call the function without the DSN param you will see it fail with a different error (because the keyword has made it to the libpq, which has rejected it).

  If you suggest a better wording that would be great, thank you and thank you for testing.

  Maybe "'foo' is an invalid keyword argument when the connection string is specified"?.

• 

  Daniele Varrazzo September 26th, 2012 @ 03:39 AM

  ...or "when a connection string is specified"?

• 

  Federico Di Gregorio September 26th, 2012 @ 07:15 AM

  Patch is fine, IMHO. Historically the swallowing was a bug and even if we break compatility on this the user already has a broken DSN (kwargs or not). So fixing this is OK.

• 

  Daniele Varrazzo September 26th, 2012 @ 11:00 AM

  Patches are pushed into connect-keywords branch. The final wording used for the error is: "'blah' is an invalid keyword argument when the dsn is specified" (the name of the param is now explicit).

  Just to pull out all the skeletons from the closets, the function has historically raised InterfaceError. The standard Python error should be TypeError instead. As I explain in the commit message:

  Raise TypeError instead of InterfaceError on bad params on connect()
  
  TypeError is the standard Python error raised in this case:
  
      $ python -c "(lambda a: None)(b=10)"
      TypeError: <lambda>() got an unexpected keyword argument 'b'
  
  We only used to raise InterfaceError when connect was used without
  any parameter at all, so it's hard to think a program depending on
  that design. Furthermore the function has always raised (and still
  does) OperationalError too, if the bad argument is detected by the
  libpq, and that cannot be changed because we can't tell the
  difference from a normal connection error.
  

  So with the last commit we change raising InterfaceError + OperationalError (the first only raised by calling a naked connect() to raising TypeError + OperationalError. Is that ok? If not we can avoid merging this commit into devel.

• 

  Federico Di Gregorio September 26th, 2012 @ 11:45 AM

  Seems fine to me.

• 

  Daniele Varrazzo September 26th, 2012 @ 12:23 PM

  • State changed from “open” to “resolved”

  Merged in my devel. Thanks Fog and Adrian.

• You flagged this item as spam.
  

  Adrian Klaver September 26th, 2012 @ 02:07 PM

  Works for me. Not sure I follow the InterfaceError/TypeError debate though. Per my example from 2.4.2 above, psycopg2 used to raise TypeError for invalid parameters.

• 

  Daniele Varrazzo September 26th, 2012 @ 02:18 PM

  My above remark was more directed to the error you get when you call connect(DSN, database='x'), with a keyword together the DSN.

  TypeError is definitely the exception to raise for the bad parameters handling you mention in this issue. Python used to do it for us before introducing **kwargs, now we have to do it ourselves.