Describe the bug

There is a recent regression where WRONGTYPE is returned when it should not.

To reproduce

$ sudo apt install redis-server python3-redis
$ python3 -c "import redis; \
               r = redis.StrictRedis('localhost', port=6379); \
               r.execute_command('set', b'\x00', b''); \
               r.execute_command('sinter', b'', b'\x00')"

Expected behavior

This command should pass, but it fails with:

Traceback (most recent call last):
   File "<string>", line 1, in <module>
   File "/usr/lib/python3/dist-packages/redis/client.py", line 901, in execute_command
     return self.parse_response(conn, command_name, **options)
   File "/usr/lib/python3/dist-packages/redis/client.py", line 915, in parse_response
     response = connection.read_response()
   File "/usr/lib/python3/dist-packages/redis/connection.py", line 756, in read_response
     raise response
redis.exceptions.ResponseError: WRONGTYPE Operation against a key holding the wrong kind of value

Additional information

  • It also fails if the last line is replaced with r.execute_command('sinterstore', b'', b'', b'\x00').
  • Reverting https://github.com/redis/redis/pull/9032 (specifically: https://github.com/redis/redis/commit/1655576e23c41ea9c12a42699651d207656a0e83) fixes the issue for me, strongly suggesting that this is a regression in this change.
  • This issue was detected via a regression in the Fakeredis testcase, but specifically through testcase regression detection in Debian: https://bugs.debian.org/991451 (Thanks to Paul Gevers and Jochen Sprickerhof)

Comment From: oranagra

@lamby i'm not sure why you're saying this is wrong behavior, as far as i can tell the old behavior was the wrong one. we can argue if that's a breaking change that should have been released in a major version rather than being backported, but that's not what you're arguing, right?

your script creates a key with the name \x00 (i.e. one character long), and then performs an SINTER on two keys, one named "" (0 chars), and the one that's one charter long. unless i'm missing something s simpler form of the same problem (easier to read) would be:

SET key1 val
SINTER key2 key1

i.e. the fact there are odd names for the keys doesn't matter. right?

in both cases, the old code would have seen that key2 (or one named "") is empty (non existing) and would return an empty set, and the new code first processes all the arguments (and finds a type error) before going to judge what should be the response.

imagine these two cases:

SET key1 val
SINTER key2 key1

vs

SET key1 val
SINTER key1 key2

i.e. the only difference between the above two cases is the order of arguments for SINTER. the old code would return en error for one and en empty set for the other, and the new code returns an error for both. are you arguing that's wrong to do?

Comment From: lamby

@oranagra Thanks for the reply. Let me take this back to the others — I'm somewhat playing Chinese whispers here. Your comment was v. useful though.

Comment From: oranagra

from a quick look at the bug report you linked, i'm guessing that it's just a matter of bad test in fakeredis. i.e. maybe they were attempting to be bug-compatible with redis, or maybe it was just a test that was relying on the bug, and now that we fixed that bug, the test fails. in which case fixing or just deleting that test could be the solution.