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

LMOVE, BLMOVE return incorrect response #1776

Closed
varunchopra opened this issue Dec 6, 2021 · 3 comments · Fixed by #1906
Closed

LMOVE, BLMOVE return incorrect response #1776

varunchopra opened this issue Dec 6, 2021 · 3 comments · Fixed by #1906
Assignees

Comments

@varunchopra
Copy link
Contributor

Version:
redis-py: 4.0.2
Redis server: 6.2.3 (enterprise)

Platform:
3.8.10, Ubuntu 20

Description:
According to the docs and the latest redis CLI, LMOVE and BLMOVE must return the value of the key that is being moved.

To repro:

> lmove x x LEFT RIGHT
"abc"

> blmove x x LEFT RIGHT 1
"abc"

However, these return bool on redis-py:

>>> r.lmove('x', 'x')
True

>>> r.blmove('x', 'x', timeout=1)
True

I believe these were erroneously added in RESPONSE_CALLBACKS in #1504 and the fix is to remove this.

@chayim Please let me know if I'm correct and I can create a PR as well.

@chayim
Copy link
Contributor

chayim commented Dec 8, 2021

@varunchopra Thank you! By all means, I'd love to see that :). At the same time, please notice that BLMOVE returns a Bulk string or a Null reply whereas LMOVE returns a bulk string.

Since you kindly offered, I assigned this to you.

@varunchopra
Copy link
Contributor Author

varunchopra commented Dec 11, 2021

At the same time, please notice that BLMOVE returns a Bulk string or a Null reply whereas LMOVE returns a bulk string.

I'm sorry I don't understand. Isn't the behavior already correct once the bool conversion is removed?

I've looked at similar implementations and it seems to be alright. Will send a PR.

@chayim
Copy link
Contributor

chayim commented Dec 12, 2021

@varunchopra Apologies, apparently I wasn't clear. You are correct. That removal ought solve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants