Skip to content
This repository was archived by the owner on Mar 9, 2026. It is now read-only.

Remove nil/null reply description from ZADD command#2614

Open
moznion wants to merge 1 commit intoredis:masterfrom
moznion:remove_nil_null_reply_from_zadd
Open

Remove nil/null reply description from ZADD command#2614
moznion wants to merge 1 commit intoredis:masterfrom
moznion:remove_nil_null_reply_from_zadd

Conversation

@moznion
Copy link
Copy Markdown
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 <moznion@mail.moznion.net>
@netlify
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants