Skip to content

Commit

Permalink
Fix GEOHASH / GEOPOS should return nil array instead of error for non…
Browse files Browse the repository at this point in the history
…-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.
  • Loading branch information
enjoy-binbin authored and git-hulk committed Aug 1, 2023
1 parent f4c85b5 commit 9ac2eba
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
15 changes: 13 additions & 2 deletions src/commands/cmd_geo.cc
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,14 @@ class CommandGeoHash : public Commander {
std::vector<std::string> 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();
}
Expand All @@ -188,11 +192,18 @@ class CommandGeoPos : public Commander {
std::map<std::string, GeoPoint> 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<std::string> 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()) {
Expand Down
10 changes: 10 additions & 0 deletions tests/gocase/unit/geo/geo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down

0 comments on commit 9ac2eba

Please sign in to comment.