GEO* STORE with empty src key delete the dest key and return 0, not empty array #9271
Conversation
With an empty src key, we need to deal with two situations: 1. non-store: We should return emptyarray. 2. store: Try to delete the store key and return 0.
|
Thanks. |
oranagra
left a comment
There was a problem hiding this comment.
please add tests for all these cases (both missing key and one with no results found), for all variants...
|
@redis/core-team this is very similar to the case of #9089, with one exception: the bugs in GEORADIUS were present in the original version (before v6.2), not added by the refactoring for GEOSEARCH. |
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
oranagra
left a comment
There was a problem hiding this comment.
LGTM..
please add tests for both cases (store and non-store), of all of these various cases and commands (including missing key and empty-search).
thanks!
|
Approving both fixes. |
|
@oranagra Please check my last two commits. |
oranagra
left a comment
There was a problem hiding this comment.
@enjoy-binbin it is a bit hard to write / read / maintain such a big pile of tests, i think we can refactor that to be much easier.
it is clear that we have a bunch of district tests for
- missing key
- wrong type key
- empty results
- etc
and we have the STORE and non-STORE variants to test on each of these.
we also have both the old (GEORADIUS) and new (GEOSEARCH) forms to test on each of these.
but first the above combinations (GEORADIUS vs GEOSEARCH) always expect the same, so one test can call both types of commands and match them to the same expected response, secondly maybe there's a better way to share code even between the STORE and non-STORE variants.
e.g. imagine this pseudo code:
proc verify_geo_edge_response {expected_response expected_store_response} {
assert_match $expected_response [r GEORADIUS ...]
assert_match $expected_response [r GEOSEARCH ...]
assert_match $expected_store_response [r GEORADIUS ... STORE]
# putting STORE arg first
assert_match $expected_store_response [r GEORADIUS STORE ...]
assert_match $expected_store_response [r GEOSEARCHSTORE ...]
# same can probably be done for BYLATLON and BYMEMBER
}
test {geo missing key} {
del src{t}
verify_geo_edge_response {} 0
}
test {geo wrong type} {
set src{t} asdf
verify_geo_edge_response "ERR*" "ERR*"
}
# and so on...
have a look at your various tests, i hope that all of them can fit into that pattern in some way, and if you find any that aren't we can still add dedicated tests for them.
oranagra
left a comment
There was a problem hiding this comment.
the last commit certainly shaved a lot of test lines and even though the new code is a bit more complicated to read, it still makes it much easier to see what's being done and what may be missing.
i didn't go though all the deleted code myself. do the last version indeed cover everything the old one did (except the withcoord / asc options etc that are not needed for our issue)?
…mpty array (redis#9271) With an empty src key, we need to deal with two situations: 1. non-STORE: We should return emptyarray. 2. STORE: Try to delete the store key and return 0. This applies to both GEOSEARCHSTORE (new to v6.2), and also GEORADIUS STORE (which was broken since forever) This pr try to fix redis#9261. i.e. both STORE variants would have behaved like the non-STORE variants when the source key was missing, returning an empty array and not deleting the destination key, instead of returning 0, and deleting the destination key. Also add more tests for some commands. - GEORADIUS: wrong type src key, non existing src key, empty search, store with non existing src key, store with empty search - GEORADIUSBYMEMBER: wrong type src key, non existing src key, non existing member, store with non existing src key - GEOSEARCH: wrong type src key, non existing src key, empty search, frommember with non existing member - GEOSEARCHSTORE: wrong type key, non existing src key, fromlonlat with empty search, frommember with non existing member Co-authored-by: Oran Agra <oran@redislabs.com>
…mpty array (#9271) With an empty src key, we need to deal with two situations: 1. non-STORE: We should return emptyarray. 2. STORE: Try to delete the store key and return 0. This applies to both GEOSEARCHSTORE (new to v6.2), and also GEORADIUS STORE (which was broken since forever) This pr try to fix #9261. i.e. both STORE variants would have behaved like the non-STORE variants when the source key was missing, returning an empty array and not deleting the destination key, instead of returning 0, and deleting the destination key. Also add more tests for some commands. - GEORADIUS: wrong type src key, non existing src key, empty search, store with non existing src key, store with empty search - GEORADIUSBYMEMBER: wrong type src key, non existing src key, non existing member, store with non existing src key - GEOSEARCH: wrong type src key, non existing src key, empty search, frommember with non existing member - GEOSEARCHSTORE: wrong type key, non existing src key, fromlonlat with empty search, frommember with non existing member Co-authored-by: Oran Agra <oran@redislabs.com> (cherry picked from commit 86555ae)
With an empty src key, we need to deal with two situations:
This applies to both GEOSEARCHSTORE (new to v6.2), and
also GEORADIUS STORE (which was broken since forever)
This pr try to fix #9261. i.e. both STORE variants would have behaved like the non-STORE variants when the source key was missing, returning an empty array and not deleting the destination key, instead of returning 0, and deleting the destination key.
Also add more tests for some commands.