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

pyramid_tm broken on Python 2 with transaction 2.0 #49

Closed
jamadden opened this issue Nov 17, 2016 · 9 comments · Fixed by #50
Closed

pyramid_tm broken on Python 2 with transaction 2.0 #49

jamadden opened this issue Nov 17, 2016 · 9 comments · Fixed by #50

Comments

@jamadden
Copy link

Transaction 2.0 defined the transaction note, username and description to all be text (unicode on Python 2.0), but pyramid_tm passes native_ values (meaning str/bytes on Python 2). This leads directly to TypeError (prior to 2.0.3, it could lead to UnicodeError in some cases). (See zopefoundation/transaction#34 for a specific stack trace.)

I'm willing to offer a PR to address this.

@tseaver
Copy link
Member

tseaver commented Nov 17, 2016

@jamadden ISTM we are going to need to release pyramid_tm 1.0.2, pinning the dependency to transaction < 2.0dev, and then fix the master to pin it transaction >= 2.0.

@mmerickel
Copy link
Member

I agree with @tseaver that this will require 2 releases from 2 PRs to handle properly.

@mmerickel
Copy link
Member

I can take care of getting 1.0.2 out tonight at least. If you wanted to submit a PR for master against transaction 2.0 and confirm that it works for you that would be greatly appreciated.

@jamadden
Copy link
Author

jamadden commented Nov 17, 2016

Existing storages, when passed unicode under transaction < 2 (and ZODB < 5.0.?), would accept it and encode it as ASCII or UTF8. If we're willing to accept the risk of UnicodeDecodeErrors there, we could simply always pass in the unicode and work with all versions of transaction.

Alternately, we can detect the behaviour of transaction by checking hasattr(Transaction(), 'extension'). If it's false, we can send the native string. If it's true, we can simply pass through the unicode. That seems pretty easy.

if hasattr(Transaction(), 'extension'):
   def tx_string(s): return text(s)
else:
   tx_string = native_

@mmerickel
Copy link
Member

Alternately, we can detect the behaviour of transaction by checking hasattr(Transaction(), 'extension'). If it's false, we can send the native string. If it's true, we can simply pass through the unicode. That seems pretty easy.

I don't think I like this fix. The hasattr check is unrelated to the actual issue at hand and feels unreliable down the road if transaction 3.0 drops extension then it's unclear why .note stopped working.

@jamadden
Copy link
Author

Ok, how about simply passing in a byte string and seeing what happens?

try:
   Transaction().note(b'bytes')
except TypeError:
   def tx_string(s): return text(s)
else:
   def tx_string(s): return bytes(s) # whatever the compat function is called

@jamadden
Copy link
Author

jamadden commented Nov 17, 2016

I should note that passing in the native string type is a bug of its own on Python 3, because of the ASCII decoding that storages will do to store the text as bytes. So there are already UnicodeErrors lurking.

@mmerickel
Copy link
Member

Ok pyramid_tm 1.0.2 is pinned to transaction < 1.99 to avoid anyone automatically upgrading.

mmerickel added a commit to mmerickel/pyramid_tm that referenced this issue Nov 19, 2016
@mmerickel
Copy link
Member

@jamadden Could you please comment on #50 and whether it works for you or not?

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

Successfully merging a pull request may close this issue.

3 participants