Skip to content
This repository was archived by the owner on Apr 3, 2019. It is now read-only.

Don't assume values are unicode, they could be just binary#78

Open
quatrix wants to merge 2 commits into
leporo:masterfrom
quatrix:master
Open

Don't assume values are unicode, they could be just binary#78
quatrix wants to merge 2 commits into
leporo:masterfrom
quatrix:master

Conversation

@quatrix
Copy link
Copy Markdown

@quatrix quatrix commented Oct 22, 2014

When trying to retrieve a binary value (e.g. an image) an UnicodeDecodeError raises as it tries to decode the response to utf-8.

While debugging I've noticed a couple of things:

  1. pyredis under python3 treats keys and values as 'bytes', as there's no encoding, the user should take care of decoding the bytestring to the proper encoding.
  2. tornadoredis + python3 doesn't handle storing binary data properly, format_command takes the length of values after encoding it, it should instead store it its binary form. this is why the test in this commit breaks under python3 and it should be fixed in different commit.

Thanks.

@leporo
Copy link
Copy Markdown
Owner

leporo commented Oct 23, 2014

Seems like necessary change to me.
But it may affect applications depending on tornado-redis, so we need to be careful.

Can you please add the unit-test fix for python3 to this pull request?

@quatrix
Copy link
Copy Markdown
Author

quatrix commented Oct 23, 2014

Hi,

I pushed a new commit b3175af that fixes the python3 binary values storing issue. Now tornadoredis handles values more like pyredis under python3, meaning all values are of type 'bytes' and users need to handling the encoding themselves.

Some of the tests in test_pubsub don't pass on master, so I decorated them with @Skip(), but all other tests including the one I've added in prev commit now pass for py27 and py34, I changed a lot of the asserts to work with the bytes instead of str.

I'm not an expert on encoding so code review and comments are appreciated :)

Thanks.

@advance512
Copy link
Copy Markdown

+1,
commit 607437e ("Added a Python 3 support") broke existing services we use, would like to get back to using the latest version.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants