zhuyijun/read-redis
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

GEO* STORE with empty src key delete the dest key and return 0, not empty array (#9271)

Browse Source 
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 86555ae0f7cc45abac7f758d72bf456e90793b46)
This commit is contained in:
Binbin 2021-08-02 00:32:24 +08:00 committed by Oran Agra
parent dadc67a92e
commit 530c70b0a9
2 changed files with 105 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
src/geo.c
View File

@ -509,7 +509,7 @@ void geoaddCommand(client *c) {
* [COUNT count [ANY]] [STORE key] [STOREDIST key]
* GEORADIUSBYMEMBER key member radius unit ... options ...
* GEOSEARCH key [FROMMEMBER member] [FROMLONLAT long lat] [BYRADIUS radius unit]
* [BYBOX width height unit] [WITHCORD] [WITHDIST] [WITHASH] [COUNT count [ANY]] [ASC|DESC]
* [BYBOX width height unit] [WITHCOORD] [WITHDIST] [WITHASH] [COUNT count [ANY]] [ASC|DESC]
* GEOSEARCHSTORE dest_key src_key [FROMMEMBER member] [FROMLONLAT long lat] [BYRADIUS radius unit]
* [BYBOX width height unit] [WITHCORD] [WITHDIST] [WITHASH] [COUNT count [ANY]] [ASC|DESC] [STOREDIST]
* */
@ -518,21 +518,24 @@ void georadiusGeneric(client *c, int srcKeyIndex, int flags) {
int storedist = 0; /* 0 for STORE, 1 for STOREDIST. */
/* Look up the requested zset */
robj *zobj = NULL;
if ((zobj = lookupKeyReadOrReply(c, c->argv[srcKeyIndex], shared.emptyarray)) == NULL ||
checkType(c, zobj, OBJ_ZSET)) {
return;
}
robj *zobj = lookupKeyRead(c->db, c->argv[srcKeyIndex]);
if (checkType(c, zobj, OBJ_ZSET)) return;
/* Find long/lat to use for radius or box search based on inquiry type */
int base_args;
GeoShape shape = {0};
if (flags & RADIUS_COORDS) {
/* GEORADIUS or GEORADIUS_RO */
base_args = 6;
shape.type = CIRCULAR_TYPE;
if (extractLongLatOrReply(c, c->argv + 2, shape.xy) == C_ERR) return;
if (extractDistanceOrReply(c, c->argv+base_args-2, &shape.conversion, &shape.t.radius) != C_OK) return;
} else if ((flags & RADIUS_MEMBER) && !zobj) {
/* We don't have a source key, but we need to proceed with argument
* parsing, so we know which reply to use depending on the STORE flag. */
base_args = 5;
} else if (flags & RADIUS_MEMBER) {
/* GEORADIUSBYMEMBER or GEORADIUSBYMEMBER_RO */
base_args = 5;
shape.type = CIRCULAR_TYPE;
robj *member = c->argv[2];
@ -542,6 +545,7 @@ void georadiusGeneric(client *c, int srcKeyIndex, int flags) {
}
if (extractDistanceOrReply(c, c->argv+base_args-2, &shape.conversion, &shape.t.radius) != C_OK) return;
} else if (flags & GEOSEARCH) {
/* GEOSEARCH or GEOSEARCHSTORE */
base_args = 2;
if (flags & GEOSEARCHSTORE) {
base_args = 3;
@ -608,6 +612,13 @@ void georadiusGeneric(client *c, int srcKeyIndex, int flags) {
flags & GEOSEARCH &&
!fromloc)
{
/* No source key, proceed with argument parsing and return an error when done. */
if (zobj == NULL) {
frommember = 1;
i++;
continue;
}
if (longLatFromMember(zobj, c->argv[base_args+i+1], shape.xy) == C_ERR) {
addReplyError(c, "could not decode requested zset member");
return;
@ -676,6 +687,23 @@ void georadiusGeneric(client *c, int srcKeyIndex, int flags) {
return;
}
/* Return ASAP when src key does not exist. */
if (zobj == NULL) {
if (storekey) {
/* store key is not NULL, try to delete it and return 0. */
if (dbDelete(c->db, storekey)) {
signalModifiedKey(c, c->db, storekey);
notifyKeyspaceEvent(NOTIFY_GENERIC, "del", storekey, c->db->id);
server.dirty++;
}
addReply(c, shared.czero);
} else {
/* Otherwise we return an empty array. */
addReply(c, shared.emptyarray);
}
return;
}
/* COUNT without ordering does not make much sense (we need to
* sort in order to return the closest N entries),
* force ASC ordering if COUNT was specified but no sorting was

71
tests/unit/geo.tcl
View File

@ -71,6 +71,34 @@ proc pointInRectangle {width_km height_km lon lat search_lon search_lat error} {
return true
}
proc verify_geo_edge_response_bylonlat {expected_response expected_store_response} {
catch {r georadius src{t} 1 1 1 km} response
assert_match $expected_response $response
catch {r georadius src{t} 1 1 1 km store dest{t}} response
assert_match $expected_store_response $response
catch {r geosearch src{t} fromlonlat 0 0 byradius 1 km} response
assert_match $expected_response $response
catch {r geosearchstore dest{t} src{t} fromlonlat 0 0 byradius 1 km} response
assert_match $expected_store_response $response
}
proc verify_geo_edge_response_bymember {expected_response expected_store_response} {
catch {r georadiusbymember src{t} member 1 km} response
assert_match $expected_response $response
catch {r georadiusbymember src{t} member 1 km store dest{t}} response
assert_match $expected_store_response $response
catch {r geosearch src{t} frommember member bybox 1 1 km} response
assert_match $expected_response $response
catch {r geosearchstore dest{t} src{t} frommember member bybox 1 1 m} response
assert_match $expected_store_response $response
}
# The following list represents sets of random seed, search position
# and radius that caused bugs in the past. It is used by the randomized
# test later as a starting point. When the regression vectors are scanned
@ -95,6 +123,34 @@ set regression_vectors {
set rv_idx 0
start_server {tags {"geo"}} {
test {GEO with wrong type src key} {
r set src{t} wrong_type
verify_geo_edge_response_bylonlat "WRONGTYPE*" "WRONGTYPE*"
verify_geo_edge_response_bymember "WRONGTYPE*" "WRONGTYPE*"
}
test {GEO with non existing src key} {
r del src{t}
verify_geo_edge_response_bylonlat {} 0
verify_geo_edge_response_bymember {} 0
}
test {GEO BYLONLAT with empty search} {
r del src{t}
r geoadd src{t} 13.361389 38.115556 "Palermo" 15.087269 37.502669 "Catania"
verify_geo_edge_response_bylonlat {} 0
}
test {GEO BYMEMBER with non existing member} {
r del src{t}
r geoadd src{t} 13.361389 38.115556 "Palermo" 15.087269 37.502669 "Catania"
verify_geo_edge_response_bymember "ERR*" "ERR*"
}
test {GEOADD create} {
r geoadd nyc -73.9454966 40.747533 "lic market"
} {1}
@ -357,6 +413,21 @@ start_server {tags {"geo"}} {
assert_equal [r zrange points 0 -1] [r zrange points2 0 -1]
}
test {GEORADIUSBYMEMBER STORE/STOREDIST option: plain usage} {
r del points{t}
r geoadd points{t} 13.361389 38.115556 "Palermo" 15.087269 37.502669 "Catania"
r georadiusbymember points{t} Palermo 500 km store points2{t}
assert_equal {Palermo Catania} [r zrange points2{t} 0 -1]
r georadiusbymember points{t} Catania 500 km storedist points2{t}
assert_equal {Catania Palermo} [r zrange points2{t} 0 -1]
set res [r zrange points2{t} 0 -1 withscores]
assert {[lindex $res 1] < 1}
assert {[lindex $res 3] > 166}
}
test {GEOSEARCHSTORE STORE option: plain usage} {
r geosearchstore points2 points fromlonlat 13.361389 38.115556 byradius 500 km
assert_equal [r zrange points 0 -1] [r zrange points2 0 -1]