From 842ea553dcf9fe637cd33aaa5be232b8f18fe0d2 Mon Sep 17 00:00:00 2001 From: Vladimir Mihailenco Date: Sat, 14 Nov 2015 16:36:21 +0200 Subject: [PATCH] Fix GeoRadius reply parsing. --- command.go | 40 +++++++++++++++--- commands.go | 35 +++------------- commands_test.go | 106 ++++++++++++++++++----------------------------- parser.go | 100 +++++++++++++++++++++++--------------------- 4 files changed, 131 insertions(+), 150 deletions(-) diff --git a/command.go b/command.go index 1fbadc0..25f9dc5 100644 --- a/command.go +++ b/command.go @@ -772,9 +772,9 @@ func (cmd *ClusterSlotCmd) readReply(cn *conn) error { // GeoLocation is used with GeoAdd to add geospatial location. type GeoLocation struct { - Name string - Longitude, Latitude, Distance float64 - GeoHash int64 + Name string + Longitude, Latitude, Dist float64 + GeoHash int64 } // GeoRadiusQuery is used with GeoRadius to query geospatial index. @@ -793,11 +793,39 @@ type GeoRadiusQuery struct { type GeoLocationCmd struct { baseCmd + q *GeoRadiusQuery locations []GeoLocation } -func NewGeoLocationCmd(args ...interface{}) *GeoLocationCmd { - return &GeoLocationCmd{baseCmd: baseCmd{_args: args, _clusterKeyPos: 1}} +func NewGeoLocationCmd(q *GeoRadiusQuery, args ...interface{}) *GeoLocationCmd { + args = append(args, q.Radius) + if q.Unit != "" { + args = append(args, q.Unit) + } else { + args = append(args, "km") + } + if q.WithCoord { + args = append(args, "WITHCOORD") + } + if q.WithDist { + args = append(args, "WITHDIST") + } + if q.WithGeoHash { + args = append(args, "WITHHASH") + } + if q.Count > 0 { + args = append(args, "COUNT", q.Count) + } + if q.Sort != "" { + args = append(args, q.Sort) + } + return &GeoLocationCmd{ + baseCmd: baseCmd{ + _args: args, + _clusterKeyPos: 1, + }, + q: q, + } } func (cmd *GeoLocationCmd) reset() { @@ -818,7 +846,7 @@ func (cmd *GeoLocationCmd) String() string { } func (cmd *GeoLocationCmd) readReply(cn *conn) error { - reply, err := readArrayReply(cn, geoLocationSliceParser) + reply, err := readArrayReply(cn, newGeoLocationSliceParser(cmd.q)) if err != nil { cmd.err = err return err diff --git a/commands.go b/commands.go index 694cf4e..2a7f792 100644 --- a/commands.go +++ b/commands.go @@ -1722,41 +1722,16 @@ func (c *commandable) GeoAdd(key string, geoLocation ...*GeoLocation) *IntCmd { return cmd } -func (c *commandable) geoRadius(args []interface{}, query *GeoRadiusQuery) *GeoLocationCmd { - args = append(args, query.Radius) - if query.Unit != "" { - args = append(args, query.Unit) - } else { - args = append(args, "km") - } - if query.WithCoord { - args = append(args, "WITHCOORD") - } - if query.WithDist { - args = append(args, "WITHDIST") - } - if query.WithGeoHash { - args = append(args, "WITHHASH") - } - if query.Count > 0 { - args = append(args, "COUNT", query.Count) - } - if query.Sort != "" { - args = append(args, query.Sort) - } - cmd := NewGeoLocationCmd(args...) +func (c *commandable) GeoRadius(key string, longitude, latitude float64, query *GeoRadiusQuery) *GeoLocationCmd { + cmd := NewGeoLocationCmd(query, "GEORADIUS", key, longitude, latitude) c.Process(cmd) return cmd } -func (c *commandable) GeoRadius(key string, longitude, latitude float64, query *GeoRadiusQuery) *GeoLocationCmd { - args := []interface{}{"GEORADIUS", key, longitude, latitude} - return c.geoRadius(args, query) -} - func (c *commandable) GeoRadiusByMember(key, member string, query *GeoRadiusQuery) *GeoLocationCmd { - args := []interface{}{"GEORADIUSBYMEMBER", key, member} - return c.geoRadius(args, query) + cmd := NewGeoLocationCmd(query, "GEORADIUSBYMEMBER", key, member) + c.Process(cmd) + return cmd } func (c *commandable) GeoDist(key string, member1, member2, unit string) *FloatCmd { diff --git a/commands_test.go b/commands_test.go index 3304a1e..b931d05 100644 --- a/commands_test.go +++ b/commands_test.go @@ -2553,17 +2553,7 @@ var _ = Describe("Commands", func() { }) Describe("Geo add and radius search", func() { - - It("should add one geo location", func() { - geoAdd := client.GeoAdd( - "Sicily", - &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - ) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(1))) - }) - - It("should add multiple geo locations", func() { + BeforeEach(func() { geoAdd := client.GeoAdd( "Sicily", &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, @@ -2573,16 +2563,19 @@ var _ = Describe("Commands", func() { Expect(geoAdd.Val()).To(Equal(int64(2))) }) + It("should not add same geo location", func() { + geoAdd := client.GeoAdd( + "Sicily", + &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, + ) + Expect(geoAdd.Err()).NotTo(HaveOccurred()) + Expect(geoAdd.Val()).To(Equal(int64(0))) + }) + It("should search geo radius", func() { - geoAdd := client.GeoAdd( - "Sicily", - &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - &redis.GeoLocation{Longitude: 15.087269, Latitude: 37.502669, Name: "Catania"}, - ) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(2))) - - res, err := client.GeoRadius("Sicily", 15, 37, &redis.GeoRadiusQuery{Radius: 200}).Result() + res, err := client.GeoRadius("Sicily", 15, 37, &redis.GeoRadiusQuery{ + Radius: 200, + }).Result() Expect(err).NotTo(HaveOccurred()) Expect(res).To(HaveLen(2)) Expect(res[0].Name).To(Equal("Palermo")) @@ -2590,14 +2583,6 @@ var _ = Describe("Commands", func() { }) It("should search geo radius with options", func() { - locations := []*redis.GeoLocation{ - &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - &redis.GeoLocation{Longitude: 15.087269, Latitude: 37.502669, Name: "Catania"}, - } - geoAdd := client.GeoAdd("Sicily", locations...) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(2))) - res, err := client.GeoRadius("Sicily", 15, 37, &redis.GeoRadiusQuery{ Radius: 200, Unit: "km", @@ -2610,27 +2595,41 @@ var _ = Describe("Commands", func() { Expect(err).NotTo(HaveOccurred()) Expect(res).To(HaveLen(2)) Expect(res[1].Name).To(Equal("Palermo")) - Expect(res[1].Distance).To(Equal(190.4424)) + Expect(res[1].Dist).To(Equal(190.4424)) Expect(res[1].GeoHash).To(Equal(int64(3479099956230698))) Expect(res[1].Longitude).To(Equal(13.361389338970184)) Expect(res[1].Latitude).To(Equal(38.115556395496299)) Expect(res[0].Name).To(Equal("Catania")) - Expect(res[0].Distance).To(Equal(56.4413)) + Expect(res[0].Dist).To(Equal(56.4413)) + Expect(res[0].GeoHash).To(Equal(int64(3479447370796909))) + Expect(res[0].Longitude).To(Equal(15.087267458438873)) + Expect(res[0].Latitude).To(Equal(37.50266842333162)) + }) + + It("should search geo radius with WithDist=false", func() { + res, err := client.GeoRadius("Sicily", 15, 37, &redis.GeoRadiusQuery{ + Radius: 200, + Unit: "km", + WithGeoHash: true, + WithCoord: true, + Count: 2, + Sort: "ASC", + }).Result() + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(HaveLen(2)) + Expect(res[1].Name).To(Equal("Palermo")) + Expect(res[1].Dist).To(Equal(float64(0))) + Expect(res[1].GeoHash).To(Equal(int64(3479099956230698))) + Expect(res[1].Longitude).To(Equal(13.361389338970184)) + Expect(res[1].Latitude).To(Equal(38.115556395496299)) + Expect(res[0].Name).To(Equal("Catania")) + Expect(res[0].Dist).To(Equal(float64(0))) Expect(res[0].GeoHash).To(Equal(int64(3479447370796909))) Expect(res[0].Longitude).To(Equal(15.087267458438873)) Expect(res[0].Latitude).To(Equal(37.50266842333162)) }) It("should search geo radius by member with options", func() { - locations := []*redis.GeoLocation{ - &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - &redis.GeoLocation{Longitude: 15.087269, Latitude: 37.502669, Name: "Catania"}, - } - - geoAdd := client.GeoAdd("Sicily", locations...) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(2))) - res, err := client.GeoRadiusByMember("Sicily", "Catania", &redis.GeoRadiusQuery{ Radius: 200, Unit: "km", @@ -2643,25 +2642,18 @@ var _ = Describe("Commands", func() { Expect(err).NotTo(HaveOccurred()) Expect(res).To(HaveLen(2)) Expect(res[0].Name).To(Equal("Catania")) - Expect(res[0].Distance).To(Equal(0.0)) + Expect(res[0].Dist).To(Equal(0.0)) Expect(res[0].GeoHash).To(Equal(int64(3479447370796909))) Expect(res[0].Longitude).To(Equal(15.087267458438873)) Expect(res[0].Latitude).To(Equal(37.50266842333162)) Expect(res[1].Name).To(Equal("Palermo")) - Expect(res[1].Distance).To(Equal(166.2742)) + Expect(res[1].Dist).To(Equal(166.2742)) Expect(res[1].GeoHash).To(Equal(int64(3479099956230698))) Expect(res[1].Longitude).To(Equal(13.361389338970184)) Expect(res[1].Latitude).To(Equal(38.115556395496299)) }) It("should search geo radius with no results", func() { - geoAdd := client.GeoAdd("Sicily", &redis.GeoLocation{ - Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - &redis.GeoLocation{Longitude: 15.087269, Latitude: 37.502669, Name: "Catania"}, - ) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(2))) - res, err := client.GeoRadius("Sicily", 99, 37, &redis.GeoRadiusQuery{ Radius: 200, Unit: "km", @@ -2682,15 +2674,6 @@ var _ = Describe("Commands", func() { // "166274.15156960033" // GEODIST Sicily Palermo Catania km // "166.27415156960032" - locations := []*redis.GeoLocation{ - &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - &redis.GeoLocation{Longitude: 15.087269, Latitude: 37.502669, Name: "Catania"}, - } - - geoAdd := client.GeoAdd("Sicily", locations...) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(2))) - geoDist := client.GeoDist("Sicily", "Palermo", "Catania", "km") Expect(geoDist.Err()).NotTo(HaveOccurred()) Expect(geoDist.Val()).To(Equal(166.27415156960032)) @@ -2701,20 +2684,11 @@ var _ = Describe("Commands", func() { }) It("should get geo hash in string representation", func() { - locations := []*redis.GeoLocation{ - &redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"}, - &redis.GeoLocation{Longitude: 15.087269, Latitude: 37.502669, Name: "Catania"}, - } - geoAdd := client.GeoAdd("Sicily", locations...) - Expect(geoAdd.Err()).NotTo(HaveOccurred()) - Expect(geoAdd.Val()).To(Equal(int64(2))) - res, err := client.GeoHash("Sicily", "Palermo", "Catania").Result() Expect(err).NotTo(HaveOccurred()) Expect(res[0]).To(Equal("sqc8b49rny0")) Expect(res[1]).To(Equal("sqdtr74hyu0")) }) - }) Describe("marshaling/unmarshaling", func() { diff --git a/parser.go b/parser.go index dd242ca..43274fc 100644 --- a/parser.go +++ b/parser.go @@ -629,65 +629,69 @@ func clusterSlotInfoSliceParser(cn *conn, n int64) (interface{}, error) { return infos, nil } -func geoLocationParser(cn *conn, n int64) (interface{}, error) { - loc := &GeoLocation{} +func newGeoLocationParser(q *GeoRadiusQuery) multiBulkParser { + return func(cn *conn, n int64) (interface{}, error) { + var loc GeoLocation - var err error - loc.Name, err = readStringReply(cn) - if err != nil { - return nil, err - } - if n >= 2 { - loc.Distance, err = readFloatReply(cn) + var err error + loc.Name, err = readStringReply(cn) if err != nil { return nil, err } - } - if n >= 3 { - loc.GeoHash, err = readIntReply(cn) - if err != nil { - return nil, err + if q.WithDist { + loc.Dist, err = readFloatReply(cn) + if err != nil { + return nil, err + } } - } - if n >= 4 { - n, err := readArrayHeader(cn) - if err != nil { - return nil, err + if q.WithGeoHash { + loc.GeoHash, err = readIntReply(cn) + if err != nil { + return nil, err + } } - if n != 2 { - return nil, fmt.Errorf("got %d coordinates, expected 2", n) + if q.WithCoord { + n, err := readArrayHeader(cn) + if err != nil { + return nil, err + } + if n != 2 { + return nil, fmt.Errorf("got %d coordinates, expected 2", n) + } + + loc.Longitude, err = readFloatReply(cn) + if err != nil { + return nil, err + } + loc.Latitude, err = readFloatReply(cn) + if err != nil { + return nil, err + } } - loc.Longitude, err = readFloatReply(cn) - if err != nil { - return nil, err - } - loc.Latitude, err = readFloatReply(cn) - if err != nil { - return nil, err - } + return &loc, nil } - - return loc, nil } -func geoLocationSliceParser(cn *conn, n int64) (interface{}, error) { - locs := make([]GeoLocation, 0, n) - for i := int64(0); i < n; i++ { - v, err := readReply(cn, geoLocationParser) - if err != nil { - return nil, err - } - switch vv := v.(type) { - case []byte: - locs = append(locs, GeoLocation{ - Name: string(vv), - }) - case *GeoLocation: - locs = append(locs, *vv) - default: - return nil, fmt.Errorf("got %T, expected string or *GeoLocation", v) +func newGeoLocationSliceParser(q *GeoRadiusQuery) multiBulkParser { + return func(cn *conn, n int64) (interface{}, error) { + locs := make([]GeoLocation, 0, n) + for i := int64(0); i < n; i++ { + v, err := readReply(cn, newGeoLocationParser(q)) + if err != nil { + return nil, err + } + switch vv := v.(type) { + case []byte: + locs = append(locs, GeoLocation{ + Name: string(vv), + }) + case *GeoLocation: + locs = append(locs, *vv) + default: + return nil, fmt.Errorf("got %T, expected string or *GeoLocation", v) + } } + return locs, nil } - return locs, nil }