From 9ac2eba19315174c848046f070ce67662df22b30 Mon Sep 17 00:00:00 2001 From: Binbin Date: Wed, 12 Jul 2023 12:12:50 +0800 Subject: [PATCH] Fix GEOHASH / GEOPOS should return nil array instead of error for non-existing key (#1573) The current code GETHASH returns an error for a key that doesn't exist: ``` 127.0.0.1:6666> geohash mykey a b c (error) ERR NotFound: ``` In Redis, this would return: ``` 127.0.0.1:6379> geohash mykey a b c 1) (nil) 2) (nil) 3) (nil) ``` GEOPOS also has the same issue, this PR fixes these inconsistencies in this case. --- src/commands/cmd_geo.cc | 15 +++++++++++++-- tests/gocase/unit/geo/geo_test.go | 10 ++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/commands/cmd_geo.cc b/src/commands/cmd_geo.cc index 49cc99ad783..62dab23c338 100644 --- a/src/commands/cmd_geo.cc +++ b/src/commands/cmd_geo.cc @@ -163,10 +163,14 @@ class CommandGeoHash : public Commander { std::vector hashes; redis::Geo geo_db(svr->storage, conn->GetNamespace()); auto s = geo_db.Hash(args_[1], members_, &hashes); - if (!s.ok()) { + if (!s.ok() && !s.IsNotFound()) { return {Status::RedisExecErr, s.ToString()}; } + if (s.IsNotFound()) { + hashes.resize(members_.size(), ""); + } + *output = redis::MultiBulkString(hashes); return Status::OK(); } @@ -188,11 +192,18 @@ class CommandGeoPos : public Commander { std::map geo_points; redis::Geo geo_db(svr->storage, conn->GetNamespace()); auto s = geo_db.Pos(args_[1], members_, &geo_points); - if (!s.ok()) { + if (!s.ok() && !s.IsNotFound()) { return {Status::RedisExecErr, s.ToString()}; } std::vector list; + + if (s.IsNotFound()) { + list.resize(members_.size(), ""); + *output = redis::MultiBulkString(list); + return Status::OK(); + } + for (const auto &member : members_) { auto iter = geo_points.find(member.ToString()); if (iter == geo_points.end()) { diff --git a/tests/gocase/unit/geo/geo_test.go b/tests/gocase/unit/geo/geo_test.go index 173ae67cc62..c34dee8621b 100644 --- a/tests/gocase/unit/geo/geo_test.go +++ b/tests/gocase/unit/geo/geo_test.go @@ -134,12 +134,22 @@ func TestGeo(t *testing.T) { require.EqualValues(t, []redis.GeoLocation([]redis.GeoLocation{{Name: "wtc one", Longitude: 0, Latitude: 0, Dist: 0, GeoHash: 0}, {Name: "union square", Longitude: 0, Latitude: 0, Dist: 0, GeoHash: 0}, {Name: "central park n/q/r", Longitude: 0, Latitude: 0, Dist: 0, GeoHash: 0}, {Name: "4545", Longitude: 0, Latitude: 0, Dist: 0, GeoHash: 0}, {Name: "lic market", Longitude: 0, Latitude: 0, Dist: 0, GeoHash: 0}}), rdb.GeoRadiusByMember(ctx, "nyc", "wtc one", &redis.GeoRadiusQuery{Radius: 7, Unit: "km"}).Val()) }) + t.Run("GEOHASH against non existing key", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "points").Err()) + require.EqualValues(t, []interface{}{nil, nil, nil}, rdb.Do(ctx, "GEOHASH", "points", "a", "b", "c").Val()) + }) + t.Run("GEOHASH is able to return geohash strings", func(t *testing.T) { require.NoError(t, rdb.Del(ctx, "points").Err()) require.NoError(t, rdb.GeoAdd(ctx, "points", &redis.GeoLocation{Name: "test", Longitude: -5.6, Latitude: 42.6}).Err()) require.EqualValues(t, []string([]string{"ezs42e44yx0"}), rdb.GeoHash(ctx, "points", "test").Val()) }) + t.Run("GEOPOS against non existing key", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "points").Err()) + require.EqualValues(t, []interface{}{nil, nil, nil}, rdb.Do(ctx, "GEOPOS", "points", "a", "b", "c").Val()) + }) + t.Run("GEOPOS simple", func(t *testing.T) { require.NoError(t, rdb.Del(ctx, "points").Err()) require.NoError(t, rdb.GeoAdd(ctx, "points", &redis.GeoLocation{Name: "a", Longitude: 10, Latitude: 20}, &redis.GeoLocation{Name: "b", Longitude: 30, Latitude: 40}).Err())