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

addressing ZODB/issues/12, encode request.path_infobefore passing it to transaction.note #25

Merged
merged 14 commits into from
Dec 13, 2014

Conversation

yentsun
Copy link
Contributor

@yentsun yentsun commented Dec 10, 2014

As discussed in zopefoundation/ZODB#12 I too have unicode urls in my project and this issue blocks me when I try to update a context object with unicode in path_info

File "/home/chao/projs/venvpython2/local/lib/python2.7/site-packages/ZODB-4.0.0-py2.7.egg/ZODB/FileStorage/format.py", line 287, in asString
    return b"".join(map(as_bytes, [s, self.user, self.descr, self.ext]))
  File "/home/chao/projs/venvpython2/local/lib/python2.7/site-packages/ZODB-4.0.0-py2.7.egg/ZODB/utils.py", line 75, in as_bytes
    return str(obj)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 32-34: ordinal not in range(128)

This update of mine fixes the problem, though I'm not sure if I didn't break anything else.

@yentsun yentsun changed the title addressing https://github.com/zopefoundation/ZODB/issues/12, encode request.path_infobefore passing it to transaction.note addressing ZODB/issues/12, encode request.path_infobefore passing it to transaction.note Dec 10, 2014
@yentsun
Copy link
Contributor Author

yentsun commented Dec 10, 2014

Sorry I haven't run the tests. I guess my fix breaks Python 3 compatibility...

@yentsun yentsun closed this Dec 10, 2014
t.note(request.path_info)
except UnicodeDecodeError:
t.note("Unable to decode path as unicode")
t.note(str(request.path_info))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs something like the pyramid.compat.text_ function.

@yentsun
Copy link
Contributor Author

yentsun commented Dec 11, 2014

Thanks for the pointing, Tres. I made it with pyramid.compat.bytes_ instead. I also added one test for the unicode path_info case.

@yentsun yentsun reopened this Dec 11, 2014
@yentsun
Copy link
Contributor Author

yentsun commented Dec 11, 2014

ok, I'll add if PY3 check...

@yentsun
Copy link
Contributor Author

yentsun commented Dec 12, 2014

Can someone point me if I need to improve the patch. Is it too naive? I really want to contribute!

@@ -3,7 +3,7 @@

from pyramid.util import DottedNameResolver
from pyramid.tweens import EXCVIEW
from pyramid_tm.compat import reraise
from pyramid_tm.compat import reraise, compat_str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just use pyramid.compat.bytes_? That function already handles the if PY3: case correctly, and its name makes it clear that you mean to be passing bytes to t.note().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was done initially in commit d902b85, but it failed tests in py3 environment. To be honest - I am not qualified enough in py3, so I moved the call under if check. But I got your point, I'll try to remove extra check and function call...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've looked at it: Transaction.note() requires a native string (not bytes as I first thought). Fortunately, the transaction package provides a helper for that:

from transaction._compat import native_
#...
    t.note(native_(request.path_info))

That would allow us to drop the changes to pyramid_tm/compat.py.

We do need to add a test which exercises this behavior: one each passing unicode and bytes to `tm_tween'.

@yentsun
Copy link
Contributor Author

yentsun commented Dec 13, 2014

Tres, please review. I didn't manage to avoid PY3 check in one of the tests I've added, but I hope its acceptable. Sorry for the commit spam - I don't have py3 env and relied on Travis output.

tseaver added a commit that referenced this pull request Dec 13, 2014
addressing ZODB/issues/12, encode `request.path_info`before passing it to `transaction.note`
@tseaver tseaver merged commit 0522de4 into Pylons:master Dec 13, 2014
@tseaver
Copy link
Member

tseaver commented Dec 13, 2014

Thank you!

tseaver added a commit that referenced this pull request Dec 13, 2014
@yentsun
Copy link
Contributor Author

yentsun commented Dec 14, 2014

Thank you for quick response!

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 this pull request may close these issues.

2 participants