Skip to content

Conversation

@leibale
Copy link
Contributor

@leibale leibale commented Jun 15, 2021

Using ZRANGESTORE and setting the src to an empty key will return an empty array and leave the dest key as-is:

ZADD dest 0 member
(integer) 1
ZRANGESTORE dest src 0 -1
(empty array)
ZCARD dest
(integer) 1

the expected result is:

ZADD dest 0 member
(integer) 1
ZRANGESTORE dest src 0 -1
(integer) 0
ZCARD dest
(integer) 0

@sundb
Copy link
Collaborator

sundb commented Jun 16, 2021

I'm not sure if this is a bug, because ZRANGESTORE command doesn't allow src to be null, and it would make sense to reply a error when src is null.

@leibale leibale changed the title fix ZRANGESTORE - should return 0 when src is null fix ZRANGESTORE - should return 0 when src empty key Jun 16, 2021
@leibale
Copy link
Contributor Author

leibale commented Jun 16, 2021

@sundb by "null" I mean an empty key (BTW, removing all the members from a sorted set will also reset the key value to null)

@leibale leibale changed the title fix ZRANGESTORE - should return 0 when src empty key fix ZRANGESTORE - should return 0 when src points to an empty key Jun 16, 2021
@sundb
Copy link
Collaborator

sundb commented Jun 16, 2021

@leibale All keys in redis will not be empty(except stream), if empty key will be deleted.
Where did you found that sorted set was empty?

@leibale
Copy link
Contributor Author

leibale commented Jun 16, 2021

@sundb yea, you're right, an empty sorted set will be deleted, but still: throwing an error in case the src points to null/empty/void/whatever you'll call it, will make this script throw an error

ZADD a 1 a
ZREM a a
ZRANGESTORE b a 0 -1

@sundb
Copy link
Collaborator

sundb commented Jun 16, 2021

@leibale I'm not sure I fully understand what you're saying.
The rely (empty array) doesn't mean that b becomes an empty key.
Your modification will cause b to be overwritten even if a does not exist, which should not make sense.

@leibale
Copy link
Contributor Author

leibale commented Jun 16, 2021

@sundb I feel like my short comments with my broken English will not get this forward.. let me try to explain myself again:

The "normal" behavior of ZRANGESTORE is - it'll override the dest key with a range from the src key, for example:

ZADD src 1 srcMember
(integer) 1
ZADD dest 1 destMember
(integer) 1
ZRANGESTORE dest src 0 -1
(integer) 1
ZSCORE dest srcMember
"1"
ZSCORE dest destMember
(nil)

but, when coping from a not existing src, the reply is (empty array), and the dest key is not being deleted:

ZADD dest 1 destMember
(integer) 1
ZRANGESTORE dest src 0 -1
(empty array)
ZSCORE dest destMember
(integer) 1

The problems with the current implementations are:

  1. the command documentation states that the command returns "Integer Reply" - and sometimes it's returning an array.
  2. the command normally overrides the dest key, not doing that when the src key does not exist is very not intuitive, and could create strange bugs when working with a set that was deleted after running ZREM.
  3. ZDIFFSTORE, ZUNIONSTORE and ZINTERSTORE are all behaving as I suggest we should do here.

@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten labels Jun 16, 2021
@sundb
Copy link
Collaborator

sundb commented Jun 16, 2021

@leibale Indeed, after looking at the code of the other commands, your modification is correct.

@oranagra
Copy link
Member

oranagra commented Jun 16, 2021

i see that SINTERSTORE (one of the oldest most mature commands that resembles ZRANGESTORE) returns 0 when the src key is missing, but it does still delete the key. (returning nil is certainly wrong too)
However, when the src key is a wrong type (a string key), it returns an error (WRONGTYPE) and in that case the dest key isn't deleted.

oranagra
oranagra previously approved these changes Jun 16, 2021
@oranagra
Copy link
Member

@redis/core-team this is a braking-change fix for a new command we introduced in 6.2.
please approve and state if you feel the fix should be backported to 6.2.

@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Jun 16, 2021
@oranagra
Copy link
Member

@leibale thank you for reporting and fixing.
i noticed the case of WRONGTYPE wasn't covered, so i added a test for it in this PR (hope i got it right this time)

@itamarhaber
Copy link
Member

Should be backported to 6.2.x imo.

@oranagra oranagra merged commit 95274f1 into redis:unstable Jun 29, 2021
@oranagra oranagra added the breaking-change This change can potentially break existing application label Jul 20, 2021
@oranagra oranagra mentioned this pull request Jul 21, 2021
oranagra added a commit that referenced this pull request Jul 21, 2021
)

mistakenly it used to return an empty array rather than 0.

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 95274f1)
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
…dis#9089)

mistakenly it used to return an empty array rather than 0.

Co-authored-by: Oran Agra <oran@redislabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

Development

Successfully merging this pull request may close these issues.

5 participants