From 0b3f8bea60ad28a6894d68029bfaab8ccc793212 Mon Sep 17 00:00:00 2001 From: Binbin Date: Wed, 13 Sep 2023 16:23:29 +0800 Subject: [PATCH] GEO FROMMEMBER returns error when member does not exist Redis will return a specific error if the member does not exist, but Kvrocks currently handles it as if the src key does not exist: ``` 127.0.0.1:6666> geoadd src 10 10 Shenzhen (integer) 1 127.0.0.1:6666> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen_2 BYBOX 88 88 m (integer) 0 127.0.0.1:6379> GEOADD src 10 10 Shenzhen (integer) 1 127.0.0.1:6379> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen_2 BYBOX 88 88 m (error) ERR could not decode requested zset member ``` Now we will return an error, the error message is the same as Redis: could not decode requested zset member ``` 127.0.0.1:6666> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen_2 BYBOX 88 88 m (error) ERR Invalid argument: could not decode requested zset member 127.0.0.1:6666> GEORADIUSBYMEMBER src Shenzhen_2 20 M STORE dst (error) ERR Invalid argument: could not decode requested zset member ``` --- src/types/redis_geo.cc | 2 +- tests/gocase/unit/geo/geo_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/types/redis_geo.cc b/src/types/redis_geo.cc index cc8173e6175..37adc35d374 100644 --- a/src/types/redis_geo.cc +++ b/src/types/redis_geo.cc @@ -188,7 +188,7 @@ rocksdb::Status Geo::Get(const Slice &user_key, const Slice &member, GeoPoint *g auto iter = geo_points.find(member.ToString()); if (iter == geo_points.end()) { - return rocksdb::Status::NotFound(); + return rocksdb::Status::InvalidArgument("could not decode requested zset member"); } *geo_point = iter->second; return rocksdb::Status::OK(); diff --git a/tests/gocase/unit/geo/geo_test.go b/tests/gocase/unit/geo/geo_test.go index fe7bb199695..6db8c222a41 100644 --- a/tests/gocase/unit/geo/geo_test.go +++ b/tests/gocase/unit/geo/geo_test.go @@ -136,6 +136,18 @@ func TestGeo(t *testing.T) { require.EqualValues(t, []interface{}([]interface{}{}), rdb.Do(ctx, "GEORADIUSBYMEMBER_RO", "points", "member", "1", "km").Val()) }) + t.Run("GEORADIUSBYMEMBER against non existing member", func(t *testing.T) { + require.NoError(t, rdb.Do(ctx, "DEL", "src", "dst").Err()) + + require.NoError(t, rdb.Do(ctx, "GEOADD", "src", "10", "10", "Shenzhen").Err()) + require.Error(t, rdb.Do(ctx, "GEORADIUSBYMEMBER", "src", "Shenzhen_2", 20, "M").Err()) + + require.NoError(t, rdb.Do(ctx, "GEOADD", "dst", "20", "20", "Guangzhou").Err()) + require.Error(t, rdb.Do(ctx, "GEORADIUSBYMEMBER", "src", "Shenzhen_2", 20, "M", "STORE", "dst").Err()) + // Verify command does not affect dst + require.EqualValues(t, 1, rdb.Exists(ctx, "dst").Val()) + }) + t.Run("GEORADIUSBYMEMBER store: remove the dst key when there is no result set", func(t *testing.T) { require.NoError(t, rdb.Do(ctx, "DEL", "src", "dst").Err()) @@ -244,6 +256,18 @@ func TestGeo(t *testing.T) { require.EqualValues(t, 0, rdb.Do(ctx, "GEOSEARCHSTORE", "dst", "src", "FROMMEMBER", "Shenzhen", "BYBOX", 88, 88, "m").Val()) }) + t.Run("GEOSEARCH / GEOSEARCHSTORE FROMMEMBER against non existing member", func(t *testing.T) { + require.NoError(t, rdb.Do(ctx, "DEL", "src", "dst").Err()) + + require.NoError(t, rdb.Do(ctx, "GEOADD", "src", "10", "10", "Shenzhen").Err()) + require.Error(t, rdb.Do(ctx, "GEOSEARCH", "src", "FROMMEMBER", "Shenzhen_2", "BYBOX", 88, 88, "M").Err()) + + require.NoError(t, rdb.Do(ctx, "GEOADD", "dst", "20", "20", "Guangzhou").Err()) + require.Error(t, rdb.Do(ctx, "GEOSEARCHSTORE", "dst", "src", "FROMMEMBER", "Shenzhen_2", "BYBOX", 88, 88, "M").Err()) + // Verify command does not affect dst + require.EqualValues(t, 1, rdb.Exists(ctx, "dst").Val()) + }) + t.Run("GEOSEARCHSTORE remove the dst key when there is no result set", func(t *testing.T) { require.NoError(t, rdb.Do(ctx, "del", "src", "dst").Err())