Merge pull request #189 from go-redis/fix/geo-radius-parsing

Fix GeoRadius reply parsing.

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
+	Name                      string
-	Longitude, Latitude, Distance float64
+	Longitude, Latitude, Dist float64
-	GeoHash                       int64
+	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 {
+func NewGeoLocationCmd(q *GeoRadiusQuery, args ...interface{}) *GeoLocationCmd {
-	return &GeoLocationCmd{baseCmd: baseCmd{_args: args, _clusterKeyPos: 1}}
+	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

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 {
+func (c *commandable) GeoRadius(key string, longitude, latitude float64, query *GeoRadiusQuery) *GeoLocationCmd {
-	args = append(args, query.Radius)
+	cmd := NewGeoLocationCmd(query, "GEORADIUS", key, longitude, latitude)
-	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...)
 	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}
+	cmd := NewGeoLocationCmd(query, "GEORADIUSBYMEMBER", key, member)
-	return c.geoRadius(args, query)
+	c.Process(cmd)
+	return cmd
 }
 
 func (c *commandable) GeoDist(key string, member1, member2, unit string) *FloatCmd {

commands_test.go

@@ -2553,17 +2553,7 @@ var _ = Describe("Commands", func() {
 	})
 
 	Describe("Geo add and radius search", func() {
-
+		BeforeEach(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() {
 			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(
+			res, err := client.GeoRadius("Sicily", 15, 37, &redis.GeoRadiusQuery{
-				"Sicily",
+				Radius: 200,
-				&redis.GeoLocation{Longitude: 13.361389, Latitude: 38.115556, Name: "Palermo"},
+			}).Result()
-				&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()
 			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() {

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) {
+func newGeoLocationParser(q *GeoRadiusQuery) multiBulkParser {
-	loc := &GeoLocation{}
+	return func(cn *conn, n int64) (interface{}, error) {
+		var loc GeoLocation
 
-	var err error
+		var err error
-	loc.Name, err = readStringReply(cn)
+		loc.Name, err = readStringReply(cn)
-	if err != nil {
-		return nil, err
-	}
-	if n >= 2 {
-		loc.Distance, err = readFloatReply(cn)
 		if err != nil {
 			return nil, err
 		}
-	}
+		if q.WithDist {
-	if n >= 3 {
+			loc.Dist, err = readFloatReply(cn)
-		loc.GeoHash, err = readIntReply(cn)
+			if err != nil {
-		if err != nil {
+				return nil, err
-			return nil, err
+			}
 		}
-	}
+		if q.WithGeoHash {
-	if n >= 4 {
+			loc.GeoHash, err = readIntReply(cn)
-		n, err := readArrayHeader(cn)
+			if err != nil {
-		if err != nil {
+				return nil, err
-			return nil, err
+			}
 		}
-		if n != 2 {
+		if q.WithCoord {
-			return nil, fmt.Errorf("got %d coordinates, expected 2", n)
+			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)
+		return &loc, nil
-		if err != nil {
-			return nil, err
-		}
-		loc.Latitude, err = readFloatReply(cn)
-		if err != nil {
-			return nil, err
-		}
 	}
-
-	return loc, nil
 }
 
-func geoLocationSliceParser(cn *conn, n int64) (interface{}, error) {
+func newGeoLocationSliceParser(q *GeoRadiusQuery) multiBulkParser {
-	locs := make([]GeoLocation, 0, n)
+	return func(cn *conn, n int64) (interface{}, error) {
-	for i := int64(0); i < n; i++ {
+		locs := make([]GeoLocation, 0, n)
-		v, err := readReply(cn, geoLocationParser)
+		for i := int64(0); i < n; i++ {
-		if err != nil {
+			v, err := readReply(cn, newGeoLocationParser(q))
-			return nil, err
+			if err != nil {
-		}
+				return nil, err
-		switch vv := v.(type) {
+			}
-		case []byte:
+			switch vv := v.(type) {
-			locs = append(locs, GeoLocation{
+			case []byte:
-				Name: string(vv),
+				locs = append(locs, GeoLocation{
-			})
+					Name: string(vv),
-		case *GeoLocation:
+				})
-			locs = append(locs, *vv)
+			case *GeoLocation:
-		default:
+				locs = append(locs, *vv)
-			return nil, fmt.Errorf("got %T, expected string or *GeoLocation", v)
+			default:
+				return nil, fmt.Errorf("got %T, expected string or *GeoLocation", v)
+			}
 		}
+		return locs, nil
 	}
-	return locs, nil
 }