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

Incorrect error message XGROUP_GROUP_NOT_FOUND_MSG #210

Closed
steve-mavens opened this issue Jul 11, 2023 · 5 comments · Fixed by #211
Closed

Incorrect error message XGROUP_GROUP_NOT_FOUND_MSG #210

steve-mavens opened this issue Jul 11, 2023 · 5 comments · Fixed by #211
Labels
bug Something isn't working

Comments

@steve-mavens
Copy link

steve-mavens commented Jul 11, 2023

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):

  • OS: Windows 10
  • python version: 3.11.4
  • redis-py version: 4.6.0
  • full requirements.txt? n/a
@steve-mavens steve-mavens added the bug Something isn't working label Jul 11, 2023
@cunla
Copy link
Owner

cunla commented Jul 11, 2023

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

@steve-mavens
Copy link
Author

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

@steve-mavens
Copy link
Author

steve-mavens commented Jul 11, 2023

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
Copy link
Owner

cunla commented Jul 11, 2023

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
Copy link
Author

steve-mavens commented Jul 12, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants