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

Remove nil/null reply description from ZADD command #2614

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

moznion
Copy link
Contributor

@moznion moznion commented Nov 30, 2023

This command doesn't return nil/null reply even if the NX/XX/LT/GT options are conflict; in that case, it returns the error reply like the following:

zadd z XX NX 1 foo
-ERR XX and NX options at the same time are not compatible
zadd z NX GT 1 foo
-ERR GT, LT, and/or NX options at the same time are not compatible
zadd z LT GT 1 foo
-ERR GT, LT, and/or NX options at the same time are not compatible

This command doesn't return nil/null reply even if the NX/XX/LT/GT
options are conflict; in that case, it returns the error reply like the
following:

```
zadd z XX NX 1 foo
-ERR XX and NX options at the same time are not compatible
zadd z NX GT 1 foo
-ERR GT, LT, and/or NX options at the same time are not compatible
zadd z LT GT 1 foo
-ERR GT, LT, and/or NX options at the same time are not compatible
```

Signed-off-by: moznion <[email protected]>
Copy link

netlify bot commented Nov 30, 2023

👷 Deploy request for redis-doc pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 48b7278

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Can you find the PR where this was changed in redis?

@zuiderkwast
Copy link
Contributor

You are right that an error is returned when the options conflict, but ZADD can still return null in some cases. See the test case "ZADD INCR LT/GT replies with nill if score not updated" in tests/unit/type/zset.tcl.

127.0.0.1:6379> zadd ztmp 28 x
(integer) 1
127.0.0.1:6379> zadd ztmp lt incr 1 x
(nil)

I think we should change the description to something like "... if the score was not updated when INCR was combined with LT/GT options" similar to what is used in the test case.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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 this pull request may close these issues.

3 participants