cunla / fakeredis-py

Implementation of Redis in python without having a Redis server running. Fully compatible with using redis-py.
https://fakeredis.moransoftware.ca/
BSD 3-Clause "New" or "Revised" License
298 stars 48 forks source link

Incorrect error message XGROUP_GROUP_NOT_FOUND_MSG #210

Closed steve-mavens closed 1 year ago

steve-mavens commented 1 year ago

Describe the bug

In fakeredis/_msgs.py, the message XGROUP_GROUP_NOT_FOUND_MSG uses %s for substitution placeholders. But that's not the right syntax for .format, which uses curly braces. The %s don't get replaced:

redis.exceptions.ResponseError: NOGROUP No such consumer group '%s' for key name '%s'

I would say that the fix is XGROUP_GROUP_NOT_FOUND_MSG = "NOGROUP No such consumer group '{1}' for key name '{0}'", and do a pull request but with that change:

redis.exceptions.ResponseError: NOGROUP No such consumer group 'b'nonexistent_group'' for key name 'b'foo''

It's still not quite right because the objects passed to the formatter are bytes, not str.

Anyway you might not like my quick fix: maybe you want to pass the parameters in the order they appear in the message, so that it's {} {} rather than {1} {0}, or maybe you want to name them. There are no other messages in that file with substitution placeholders in "the wrong order" for me to copy the style.

To Reproduce

import asyncio
import fakeredis.aioredis

async def main():
    async with fakeredis.aioredis.FakeRedis(decode_responses=True, version=6) as client:
        await client.xadd(name='foo', fields={'message': 'bar'})
        await client.xreadgroup('nonexistent_group', 'some_consumer', {'foo': '>'}, count=1)

if __name__ == '__main__':
    asyncio.run(main())

Traceback probably isn't very relevant, but since the template asks for it...

Traceback (most recent call last):
  File "fake_bug.py", line 11, in <module>
    asyncio.run(main())
  File "Lib\asyncio\runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "Lib\asyncio\runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "Lib\asyncio\base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "fake_bug.py", line 8, in main
    await client.xreadgroup('nonexistent_group', 'some_consumer', {'foo': '>'}, count=1)
  File "Lib\site-packages\redis\asyncio\client.py", line 518, in execute_command
    return await conn.retry.call_with_retry(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "Lib\site-packages\redis\asyncio\retry.py", line 59, in call_with_retry
    return await do()
           ^^^^^^^^^^
  File "Lib\site-packages\redis\asyncio\client.py", line 492, in _send_command_parse_response
    return await self.parse_response(conn, command_name, **options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "Lib\site-packages\redis\asyncio\client.py", line 539, in parse_response
    response = await connection.read_response()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "Lib\site-packages\fakeredis\aioredis.py", line 156, in read_response
    raise response
redis.exceptions.ResponseError: NOGROUP No such consumer group '%s' for key name '%s'

Expected behavior

redis.exceptions.ResponseError: NOGROUP No such consumer group 'nonexistent_group' for key name 'foo'

or similar.

Desktop (please complete the following information):

cunla commented 1 year ago

Hi, would you want to create a PR with the fix and a test for this?

steve-mavens commented 1 year ago

Sure, I can do. It won't be today, though.

steve-mavens commented 1 year ago

Also the terms of my employer's copyright waiver for open source contributions are such that it's best if I use my personal laptop to make the change, and that in turn means I'll be logged in using a different github user name (this is my work account because I found the issue while working, and I don't use work accounts on personal devices). Sorry for any confusion, it's just technically if I use work resources to write the code, they own the copyright on 10 lines of your project. I don't even know the internal process I have to go through to release those lines under an open source license, so it's best if I own them.

cunla commented 1 year ago

All contributions and contributors are mentioned in the release notes, and anyone can track them through PRs/commits. With that said, any contribution to this package is under the BSD3 license - so you can't "own" lines of code.

I think your company should go through the process of identifying open-source packages it uses and whether it wants to support/contribute to open-source projects or not. It is ok if not, though it will not encourage me to prioritize this bug.

steve-mavens commented 1 year ago

They're not intentionally obstructing me, they just don't have a general policy. It makes sense for them not to authorise me to release their entire codebase under BSD, not so much for them not to authorise me to write and release one test. I agree that ideally the lawyers should do the work to produce a policy, but as things stand I'd have to get case-by-case approval for the company to contribute code to open source projects. It's hassle, so I'd rather just make absolutely sure of the difference between stuff they own that I can't give away, and stuff I own that I can.