Fix accidental deletion of sinterstore command when we meet wrong type error. by enjoy-binbin · Pull Request #9032 · redis/redis

enjoy-binbin · 2021-06-02T07:02:22Z

In sunion command, if we type any keys wrong(like type error), there will be an error

127.0.0.1:6381> flushdb
OK
127.0.0.1:6381> sadd c 1 2 3 
(integer) 3
127.0.0.1:6381> smembers c
1) "1"
2) "2"
3) "3"
127.0.0.1:6381> sunionstore c a b
(integer) 0
127.0.0.1:6381> smembers c
(empty array)
127.0.0.1:6381> 
127.0.0.1:6381> sadd c 1 2 3 
(integer) 3
127.0.0.1:6381> set a 123  // `a` is a string
OK
127.0.0.1:6381> sunion a b
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6381> sunion b a
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6381> sunionstore c a b
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6381> sunionstore c b a
(error) WRONGTYPE Operation against a key holding the wrong kind of value

In sinter command, it will be different. In sinter command, i think the result will be fine(although unlike sunion)
But in sinterstore command, the different will cause totally different about the dstkey.

127.0.0.1:6381> flushdb
OK
127.0.0.1:6381> sinter a b
(empty array)
127.0.0.1:6381> sinter b a
(empty array)
127.0.0.1:6381> set a 123
OK
127.0.0.1:6381> sinter a b
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6381> sinter b a
(empty array)
127.0.0.1:6381> sadd c 1 2 3
(integer) 3
// in here, there will be an type error, and `c(dstkey)` is exist
127.0.0.1:6381> sinterstore c a b
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6381> exists c
(integer) 1
// in here, we will delete the c(dstkey), although `a` is a string
127.0.0.1:6381> sinterstore c b a
(integer) 0
127.0.0.1:6381> exists c
(integer) 0

So i update sinterGenericCommand, and make them behave uniformly. After output:

127.0.0.1:6379> flushdb
OK
127.0.0.1:6379> sinter a b
(empty array)
127.0.0.1:6379> sinter b a
(empty array)
127.0.0.1:6379> set a 123
OK
127.0.0.1:6379> sinter a b
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6379> sinter b a
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6379> 
127.0.0.1:6379> sadd c 1 2 3
(integer) 3
127.0.0.1:6379> sinterstore c a b
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6379> exists c
(integer) 1
127.0.0.1:6379> sinterstore c b a
(error) WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6379> exists c
(integer) 1

Also add more tests for some commands.
Mainly focus on

wrong type error:
expand test case (base on sinter bug) in non-store variant
add tests for store variant (although it exists in non-store variant, i think it would be better to have same tests)
the dstkey result when we meet non-exist key (empty set) in *store

sdiff:

improve test case about wrong type error (the one we found in sinter, although it is safe in sdiff)
add test about using non-exist key (treat it like an empty set)
sdiffstore:
according to sdiff test case, also add some tests about wrong type error and non-exist key
the different is that in sdiffstore, we will consider the dstkey result

sunion/sunionstore add more tests (same as above)

sinter/sinterstore also same as above ...

…e error.

oranagra · 2021-06-02T10:19:28Z

@enjoy-binbin good catch, thank you.
can you please add tests to cover these cases.

@redis/core-team this bug-fix is in some way a breaking change, please approve (for 7.0)

enjoy-binbin · 2021-06-02T10:40:43Z

@oranagra Sure. I will do that when i have times. I also make pr about other commands. #9018 #9015 (not urgent, small fix. Hope you can take a look when you have time, would love to hear your thoughts)

madolson

LGTM

oranagra

@enjoy-binbin that's a lot of new test code (i didn't expect that).
are you sure many of these are not already covered?
can you please make a list (or a table) of what you added, so it'll be easier to review.
i.e. if some tests existed for the non-store variant and you added them for the store variant, or some existed for UNION and you added them for INTER, and so on..

also, there there are many multi-key commands involved with this test, maybe we should hold merging it to after #9033 so that we can make sure proper tagging are added to support external sharded test targets.

enjoy-binbin · 2021-06-03T12:26:07Z

@oranagra Yes, many of these are not covered in some tests case.
My mistake, i know it is quite difficult to review the test code.
These codes are very similar. I added some notes in the top comment.
(But I'm not sure I described it clearly... sadly. It was a little bit hard for me to describe it)

The reason i added extra tests for sdiff/sunion is that:
I think they are very similar in the first half of the code processing
Although there is no problem with the original code and has nothing to do with this fix
I think it might be better to add these test cases (in future?) Although it may be a bit long-winded

As for 9033. I am not very familiar with it. Will follow your suggestion
(ps: Sorry for the delay (saw it a long time age). It took me some time to block the language. Also busy with others things)

…e error. (#9032) SINTERSTORE would have deleted the dest key right away, even when later on it is bound to fail on an (WRONGTYPE) error. With this change it first picks up all the input keys, and only later delete the dest key if one is empty. Also add more tests for some commands. Mainly focus on - `wrong type error`: expand test case (base on sinter bug) in non-store variant add tests for store variant (although it exists in non-store variant, i think it would be better to have same tests) - the dstkey result when we meet `non-exist key (empty set)` in *store sdiff: - improve test case about wrong type error (the one we found in sinter, although it is safe in sdiff) - add test about using non-exist key (treat it like an empty set) sdiffstore: - according to sdiff test case, also add some tests about `wrong type error` and `non-exist key` - the different is that in sdiffstore, we will consider the `dstkey` result sunion/sunionstore add more tests (same as above) sinter/sinterstore also same as above ... (cherry picked from commit b8a5da8) (cherry picked from commit f4702b8)

…e error. (#9032) SINTERSTORE would have deleted the dest key right away, even when later on it is bound to fail on an (WRONGTYPE) error. With this change it first picks up all the input keys, and only later delete the dest key if one is empty. Also add more tests for some commands. Mainly focus on - `wrong type error`: expand test case (base on sinter bug) in non-store variant add tests for store variant (although it exists in non-store variant, i think it would be better to have same tests) - the dstkey result when we meet `non-exist key (empty set)` in *store sdiff: - improve test case about wrong type error (the one we found in sinter, although it is safe in sdiff) - add test about using non-exist key (treat it like an empty set) sdiffstore: - according to sdiff test case, also add some tests about `wrong type error` and `non-exist key` - the different is that in sdiffstore, we will consider the `dstkey` result sunion/sunionstore add more tests (same as above) sinter/sinterstore also same as above ... (cherry picked from commit b8a5da8)

…e error. (redis#9032) SINTERSTORE would have deleted the dest key right away, even when later on it is bound to fail on an (WRONGTYPE) error. With this change it first picks up all the input keys, and only later delete the dest key if one is empty. Also add more tests for some commands. Mainly focus on - `wrong type error`: expand test case (base on sinter bug) in non-store variant add tests for store variant (although it exists in non-store variant, i think it would be better to have same tests) - the dstkey result when we meet `non-exist key (empty set)` in *store sdiff: - improve test case about wrong type error (the one we found in sinter, although it is safe in sdiff) - add test about using non-exist key (treat it like an empty set) sdiffstore: - according to sdiff test case, also add some tests about `wrong type error` and `non-exist key` - the different is that in sdiffstore, we will consider the `dstkey` result sunion/sunionstore add more tests (same as above) sinter/sinterstore also same as above ...

oranagra · 2022-03-22T14:16:23Z

Late realization: this fix could cause data inconsistency between the master and the replica.
If the replica is upgraded and the master wasn't (the common case), then the master will delete the key, and the replica won't.
see #10419 (comment)

Fix accidental deletion of sinterstore command when we meet wrong typ…

4a7bf9c

…e error.

oranagra previously approved these changes Jun 2, 2021

View reviewed changes

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-test-code the PR is missing test code labels Jun 2, 2021

yossigo previously approved these changes Jun 2, 2021

View reviewed changes

madolson previously approved these changes Jun 2, 2021

View reviewed changes

Add more tests for sdiff/sunion/sinter commands.

8ba059d

enjoy-binbin dismissed stale reviews from madolson, yossigo, and oranagra via 8ba059d June 3, 2021 02:33

enjoy-binbin requested review from madolson, oranagra and yossigo June 3, 2021 02:41

oranagra reviewed Jun 3, 2021

View reviewed changes

enjoy-binbin added 2 commits June 9, 2021 21:24

Merge branch 'unstable' into fix_sinterstore

209681e

test

9c45a12

enjoy-binbin requested a review from oranagra June 9, 2021 13:41

Merge branch 'unstable' into fix_sinterstore

c19c91c

oranagra approved these changes Jun 13, 2021

View reviewed changes

oranagra merged commit b8a5da8 into redis:unstable Jun 13, 2021

enjoy-binbin deleted the fix_sinterstore branch June 13, 2021 07:59

This was referenced Jul 21, 2021

Release 6.2.5 #9264

Merged

Release 6.0.15 #9266

Merged

lamby mentioned this pull request Jul 25, 2021

[BUG] Regression in WRONGTYPE handling after deletion handling improvements #9273

Open

oranagra added the breaking-change This change can potentially break existing application label Jul 25, 2021

oranagra mentioned this pull request Mar 22, 2022

Getex extra options to avoid duplicate options #10419

Closed

soloestoy mentioned this pull request Nov 20, 2023

set: SINTER should satisfy commutative law on empty or wrong type #5791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix accidental deletion of sinterstore command when we meet wrong type error.#9032

Fix accidental deletion of sinterstore command when we meet wrong type error.#9032
oranagra merged 5 commits into
redis:unstablefrom
enjoy-binbin:fix_sinterstore

enjoy-binbin commented Jun 2, 2021 •

edited

Loading

Uh oh!

oranagra commented Jun 2, 2021

Uh oh!

enjoy-binbin commented Jun 2, 2021

Uh oh!

madolson left a comment

Uh oh!

oranagra left a comment

Uh oh!

enjoy-binbin commented Jun 3, 2021 •

edited

Loading

Uh oh!

oranagra commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

enjoy-binbin commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jun 2, 2021

Uh oh!

enjoy-binbin commented Jun 2, 2021

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

enjoy-binbin commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

enjoy-binbin commented Jun 2, 2021 •

edited

Loading

enjoy-binbin commented Jun 3, 2021 •

edited

Loading