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
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

quatrix
Copy link

@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
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
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

+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