tornado-redis icon indicating copy to clipboard operation
tornado-redis copied to clipboard

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

Open quatrix opened this issue 10 years ago • 3 comments

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.

quatrix avatar Oct 22 '14 16:10 quatrix

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?

leporo avatar Oct 23 '14 06:10 leporo

Hi,

I pushed a new commit b3175af46227bfe58c643bca69334633585ebb51 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.

quatrix avatar Oct 23 '14 15:10 quatrix

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

advance512 avatar Oct 27 '14 16:10 advance512