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

[psycopg2] The quote_ident function requires psycopg2.extensions.connection. #474

Closed
eliwind opened this issue May 29, 2018 · 7 comments · Fixed by #477
Closed

[psycopg2] The quote_ident function requires psycopg2.extensions.connection. #474

eliwind opened this issue May 29, 2018 · 7 comments · Fixed by #477

Comments

@eliwind
Copy link

eliwind commented May 29, 2018

This is very related to #383.

To reproduce:

from ddtrace import patch_all
patch_all()

import psycopg2
from psycopg2.extensions import quote_ident

conn = psycopg2.connect(dbname="test")
quote_ident('foo', conn) # Fails with TypeError: argument 2 must be a connection or a cursor
@palazzem
Copy link

palazzem commented Jun 1, 2018

Hello @eliwind! A PR is available here #477 that should address the issue. I'll double check it with the other connected issue #383 and will reach you again.

In the meantime, do you want to temporarily test this branch in your development environment to validate the fix? Thank you very much!

@eliwind
Copy link
Author

eliwind commented Jun 1, 2018

Thanks @palazzem.

Unfortunately I tried it out, and no dice. I see your change in the version of the ddtrace code inside my venv, I can now see quote_ident in the list of things in _psycopg2_extensions. But when I do exactly what's in your test interactively, something isn't happy:

>>> from ddtrace import patch_all
>>> import psycopg2
>>> from psycopg2.extensions import quote_ident
>>> conn=psycopg2.connect(dbname='mydb')
>>> quote_ident('foo', conn)
'"foo"'
>>>
>>> patch_all()
>>> conn2=psycopg2.connect(dbname='mydb')
>>> quote_ident('foo', conn2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: argument 2 must be a connection or a cursor
>>>
>>> from ddtrace.contrib.psycopg.patch import _psycopg2_extensions
>>> _psycopg2_extensions
[(<built-in function register_type>, <module 'psycopg2.extensions' from '/Users/eli/.virtualenvs/myvenv-mNlG-o--/lib/python2.7/site-packages/psycopg2/extensions.pyc'>, 'register_type', <function _extensions_register_type at 0x10dea69b0>), (<built-in function register_type>, <module 'psycopg2._psycopg' from '/Users/eli/.virtualenvs/myvenv-mNlG-o--/lib/python2.7/site-packages/psycopg2/_psycopg.so'>, 'register_type', <function _extensions_register_type at 0x10dea69b0>), (<built-in function adapt>, <module 'psycopg2.extensions' from '/Users/eli/.virtualenvs/myvenv-mNlG-o--/lib/python2.7/site-packages/psycopg2/extensions.pyc'>, 'adapt', <function _extensions_adapt at 0x10dea6a28>), (<built-in function register_type>, <module 'psycopg2._json' from '/Users/eli/.virtualenvs/myvenv-mNlG-o--/lib/python2.7/site-packages/psycopg2/_json.pyc'>, 'register_type', <function _extensions_register_type at 0x10dea69b0>), (<built-in function quote_ident>, <module 'psycopg2.extensions' from '/Users/eli/.virtualenvs/myvenv-mNlG-o--/lib/python2.7/site-packages/psycopg2/extensions.pyc'>, 'quote_ident', <function _extensions_register_type at 0x10dea69b0>)]

@palazzem
Copy link

palazzem commented Jun 1, 2018

OK thank you! we'll continue the investigation then.

@Kyle-Verhoog
Copy link
Member

Hey @eliwind!

I've been digging into the issue and I can confirm that our test case was not properly testing the patching since quote_ident was probably patched prior to the test running.

If you try running:

>>> patch_all()
>>> import psycopg2
>>> from psycopg2.extensions import quote_ident
>>> conn=psycopg2.connect(dbname='mydb')
>>> quote_ident('foo', conn)

do you still get the error?

@eliwind
Copy link
Author

eliwind commented Jun 1, 2018

@Kyle-Verhoog, indeed, no error when I patch_all() at the beginning.

@Kyle-Verhoog
Copy link
Member

Great!

@eliwind are you able to test it in your application and confirm that it works? Then we can close out this issue. 😄

@palazzem
Copy link

We're going to merge the PR and prepare the release. If your application is still impacted by the issue after the 0.12.1 release, feel free to re-open the issue so we can continue the triage!

Thanks a lot for your feedback on that!

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

Successfully merging a pull request may close this issue.

3 participants