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

Decoding data is not nesessary on some Redis configurations #294

Open
alex-eri opened this issue Jun 11, 2018 · 15 comments · Fixed by alex-eri/aiohttp-session#1
Open

Decoding data is not nesessary on some Redis configurations #294

alex-eri opened this issue Jun 11, 2018 · 15 comments · Fixed by alex-eri/aiohttp-session#1

Comments

@alex-eri
Copy link
Contributor

What reason for this line here?

data = data.decode('utf-8')

I want to store some bytes in session. Changed encoder setup_session(app, RedisStorage(redis_pool, encoder=pickle.dumps, decoder=pickle.loads)). And I have error on getting session:


    data = data.decode('utf-8')
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x80 in position 0: invalid start byte

It also breakes msgpack

alex-eri added a commit to alex-eri/aiohttp-session that referenced this issue Jun 11, 2018
@alex-eri alex-eri reopened this Jun 11, 2018
@alex-eri
Copy link
Contributor Author

Its clear - python3.5 needs str, not bytes.

@asvetlov
Copy link
Member

encoder and decoder are object <-> str converters by design.
There is a possibility to extend the contract but it requires abstract serializers, not just dropping .decode() call.

@NotSqrt
Copy link

NotSqrt commented Jun 18, 2018

I hit this bug when plugging the RedisStorage in my project, but my current pool uses: pool = await aioredis.create_redis_pool(redis_url, encoding='utf-8').
The encoding parameter already apply .decode('utf-8'), no need to call .decode('utf-8') again on a str.

@asvetlov
Copy link
Member

@NotSqrt it's your configuration problem, not the library bug.

@NotSqrt
Copy link

NotSqrt commented Jun 19, 2018

So regroup decoding and unserializing ? decoder=lambda data: json.loads(data.decode('utf-8')), so that everyone can customize the decoder/encoder ?

@alex-eri
Copy link
Contributor Author

alex-eri commented Jun 19, 2018

@asvetlov redis eats bytes. Why we cant use bytes?
Python 3.6+ designed to use bytes too.

There is opton designed customizable, but that we cant customize it.

Simple if does not breaks anything. Why not?

Maybe param to init to provide decoder? Maybe split it into method?

alex-eri added a commit to alex-eri/aiohttp-session that referenced this issue Jun 19, 2018
@alex-eri
Copy link
Contributor Author

alex-eri commented Jun 19, 2018

@asvetlov

object <-> str

Here encoder designed as object -> bytes:

All *.dumps and *.loads i know supports loading bytes except python3.5`s json

@anyUesr
Copy link

anyUesr commented Jul 2, 2018

I also meet this, please fix it quickly

thanks so much : )

@anyUesr
Copy link

anyUesr commented Jul 2, 2018

Error like this:

File "/home/redis/.pyenv/versions/3.6.3/lib/python3.6/site-packages/aiohttp_session/__init__.py", line 113, in get_session
    session = await storage.load_session(request)
  File "/home/redis/.pyenv/versions/3.6.3/lib/python3.6/site-packages/aiohttp_session/redis_storage.py", line 53, in load_session
    data = data.decode('utf-8')
AttributeError: 'str' object has no attribute 'decode'

after I change it to

 53                 try:
 54                     data = data.decode('utf-8')
 55                 except AttributeError:
 56                     pass

it run OK

@anyUesr
Copy link

anyUesr commented Jul 2, 2018

@asvetlov I agree @NotSqrt, maybe except decode error when data.decode('utf-8') is better

@NotSqrt
Copy link

NotSqrt commented Jul 2, 2018

This issue is about multiple problems:

  • mine, where it's not necessary to re-decode data if the redis client is already configured to do the decoding
  • @alex-eri , who wants to use serializers other than json, and where data must be in bytes, not decoded.

Ignoring the AttributeError will "fix" my problem, but not for @alex-eri

This is why I propose the possibility of customizing decoding + deserializing with a function parameter.
I used a lambda in #294 (comment), but it could be a proper function.

@alex-eri
Copy link
Contributor Author

alex-eri commented Jul 4, 2018

I removed this line in local fork.)

@remort
Copy link

remort commented Aug 23, 2018

Didn't read the tread.
If you set encoding, i.e utf-8 when create redis_pool, then aiohttp_session using RedisStorage cant't work with that storage due to attempting to decode bytes from redis to str. But redis already decode its data to str since you set the decoder to utf-8. So aiohttp_session should somehow check type of data coming from redis if its str or bytes.
Now I can't use encoding='utf-8' for redis pool since aiohttp_session stops work in that case.

@alex-eri
Copy link
Contributor Author

I think decode must be droped from here because it conflicts backend and codecs.

Its trivial to fix backward compatability using wrapper for json codek in default value.

@Dreamsorcerer
Copy link
Member

If someone can create a test that reproduces this Redis issue in a PR, then I'm sure we can figure out a fix.

@Dreamsorcerer Dreamsorcerer changed the title Decoding data is non nesessary and breakes binary coders Decoding data is not nesessary on some Redis configurations Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants