fix(cmd): don't modify the caller's geo queries (#4173)

* fix(cmd): don't modify the caller's geo queries

geoLocationArgs incremented withLen on the GeoRadiusQuery that the
caller passed, and GeoLocationCmd read it back to parse the reply. A
second GeoRadius call with the same query parsed the reply with a
doubled withLen and failed with "got 2 elements in the array, wanted
3", and sharing one query between goroutines was a data race. Count
the WITH flags when the reply is read, as GeoSearchLocationCmd does,
and drop the withLen field.

geoSearchArgs also wrote the default "km" unit back to the
GeoSearchQuery. Keep the default in a local variable instead.

* fix(cmd): copy the geo query into the command

GeoLocationCmd and GeoSearchLocationCmd kept the caller's query and read
its WITH flags when the reply arrived. With a pipeline or autopipelining,
a change to the query between queuing and reading parsed the reply with
flags that weren't sent. Keep a copy of the query in the command, so the
reply is parsed with the flags of the command, and make Clone a plain
copy.

command.go

@@ -5015,29 +5015,31 @@ type GeoRadiusQuery struct {
 	Sort      string
 	Store     string
 	StoreDist string
-
-	// WithCoord+WithDist+WithGeoHash
-	withLen int
 }
 
 type GeoLocationCmd struct {
 	baseCmd
 
-	q         *GeoRadiusQuery
+	// q is a copy of the caller's query, taken when the command is built, so
+	// the reply is parsed with the flags that were sent even if the caller
+	// changes or reuses its query before the reply arrives (pipelines,
+	// autopipelining).
+	q         GeoRadiusQuery
 	locations []GeoLocation
 }
 
 var _ Cmder = (*GeoLocationCmd)(nil)
 
 func NewGeoLocationCmd(ctx context.Context, q *GeoRadiusQuery, args ...interface{}) *GeoLocationCmd {
-	return &GeoLocationCmd{
+	cmd := &GeoLocationCmd{
 		baseCmd: baseCmd{
 			ctx:     ctx,
-			args:    geoLocationArgs(q, args...),
 			cmdType: CmdTypeGeoLocation,
 		},
-		q: q,
+		q: *q,
 	}
+	cmd.args = geoLocationArgs(&cmd.q, args...)
+	return cmd
 }
 
 func geoLocationArgs(q *GeoRadiusQuery, args ...interface{}) []interface{} {
@@ -5049,15 +5051,12 @@ func geoLocationArgs(q *GeoRadiusQuery, args ...interface{}) []interface{} {
 	}
 	if q.WithCoord {
 		args = append(args, "withcoord")
-		q.withLen++
 	}
 	if q.WithDist {
 		args = append(args, "withdist")
-		q.withLen++
 	}
 	if q.WithGeoHash {
 		args = append(args, "withhash")
-		q.withLen++
 	}
 	if q.Count > 0 {
 		args = append(args, "count", q.Count)
@@ -5102,9 +5101,21 @@ func (cmd *GeoLocationCmd) readReply(rd *proto.Reader) error {
 	}
 	cmd.locations = make([]GeoLocation, n)
 
+	// Count the WITH flags of the query this command was built with.
+	withLen := 0
+	if cmd.q.WithCoord {
+		withLen++
+	}
+	if cmd.q.WithDist {
+		withLen++
+	}
+	if cmd.q.WithGeoHash {
+		withLen++
+	}
+
 	for i := 0; i < len(cmd.locations); i++ {
 		// only name
-		if cmd.q.withLen == 0 {
+		if withLen == 0 {
 			if cmd.locations[i].Name, err = rd.ReadString(); err != nil {
 				return err
 			}
@@ -5112,7 +5123,7 @@ func (cmd *GeoLocationCmd) readReply(rd *proto.Reader) error {
 		}
 
 		// +name
-		if err = rd.ReadFixedArrayLen(cmd.q.withLen + 1); err != nil {
+		if err = rd.ReadFixedArrayLen(withLen + 1); err != nil {
 			return err
 		}
 
@@ -5146,21 +5157,6 @@ func (cmd *GeoLocationCmd) readReply(rd *proto.Reader) error {
 }
 
 func (cmd *GeoLocationCmd) Clone() Cmder {
-	var q *GeoRadiusQuery
-	if cmd.q != nil {
-		q = &GeoRadiusQuery{
-			Radius:      cmd.q.Radius,
-			Unit:        cmd.q.Unit,
-			WithCoord:   cmd.q.WithCoord,
-			WithDist:    cmd.q.WithDist,
-			WithGeoHash: cmd.q.WithGeoHash,
-			Count:       cmd.q.Count,
-			Sort:        cmd.q.Sort,
-			Store:       cmd.q.Store,
-			StoreDist:   cmd.q.StoreDist,
-			withLen:     cmd.q.withLen,
-		}
-	}
 	var locations []GeoLocation
 	if cmd.locations != nil {
 		locations = make([]GeoLocation, len(cmd.locations))
@@ -5168,7 +5164,7 @@ func (cmd *GeoLocationCmd) Clone() Cmder {
 	}
 	return &GeoLocationCmd{
 		baseCmd:   cmd.cloneBaseCmd(),
-		q:         q,
+		q:         cmd.q,
 		locations: locations,
 	}
 }
@@ -5241,15 +5237,17 @@ func geoSearchArgs(q *GeoSearchQuery, args []interface{}) []interface{} {
 	}
 
 	if q.Radius > 0 {
-		if q.RadiusUnit == "" {
-			q.RadiusUnit = "km"
+		unit := q.RadiusUnit
+		if unit == "" {
+			unit = "km"
 		}
-		args = append(args, "byradius", q.Radius, q.RadiusUnit)
+		args = append(args, "byradius", q.Radius, unit)
 	} else {
-		if q.BoxUnit == "" {
-			q.BoxUnit = "km"
+		unit := q.BoxUnit
+		if unit == "" {
+			unit = "km"
 		}
-		args = append(args, "bybox", q.BoxWidth, q.BoxHeight, q.BoxUnit)
+		args = append(args, "bybox", q.BoxWidth, q.BoxHeight, unit)
 	}
 
 	if q.Sort != "" {
@@ -5269,7 +5267,8 @@ func geoSearchArgs(q *GeoSearchQuery, args []interface{}) []interface{} {
 type GeoSearchLocationCmd struct {
 	baseCmd
 
-	opt *GeoSearchLocationQuery
+	// opt is a copy of the caller's query; see GeoLocationCmd.q.
+	opt GeoSearchLocationQuery
 	val []GeoLocation
 }
 
@@ -5278,14 +5277,15 @@ var _ Cmder = (*GeoSearchLocationCmd)(nil)
 func NewGeoSearchLocationCmd(
 	ctx context.Context, opt *GeoSearchLocationQuery, args ...interface{},
 ) *GeoSearchLocationCmd {
-	return &GeoSearchLocationCmd{
+	cmd := &GeoSearchLocationCmd{
 		baseCmd: baseCmd{
 			ctx:     ctx,
-			args:    geoSearchLocationArgs(opt, args),
 			cmdType: CmdTypeGeoSearchLocation,
 		},
-		opt: opt,
+		opt: *opt,
 	}
+	cmd.args = geoSearchLocationArgs(&cmd.opt, args)
+	return cmd
 }
 
 func (cmd *GeoSearchLocationCmd) SetVal(val []GeoLocation) {
@@ -5381,27 +5381,6 @@ func (cmd *GeoSearchLocationCmd) readReply(rd *proto.Reader) error {
 }
 
 func (cmd *GeoSearchLocationCmd) Clone() Cmder {
-	var opt *GeoSearchLocationQuery
-	if cmd.opt != nil {
-		opt = &GeoSearchLocationQuery{
-			GeoSearchQuery: GeoSearchQuery{
-				Member:     cmd.opt.Member,
-				Longitude:  cmd.opt.Longitude,
-				Latitude:   cmd.opt.Latitude,
-				Radius:     cmd.opt.Radius,
-				RadiusUnit: cmd.opt.RadiusUnit,
-				BoxWidth:   cmd.opt.BoxWidth,
-				BoxHeight:  cmd.opt.BoxHeight,
-				BoxUnit:    cmd.opt.BoxUnit,
-				Sort:       cmd.opt.Sort,
-				Count:      cmd.opt.Count,
-				CountAny:   cmd.opt.CountAny,
-			},
-			WithCoord: cmd.opt.WithCoord,
-			WithDist:  cmd.opt.WithDist,
-			WithHash:  cmd.opt.WithHash,
-		}
-	}
 	var val []GeoLocation
 	if cmd.val != nil {
 		val = make([]GeoLocation, len(cmd.val))
@@ -5409,7 +5388,7 @@ func (cmd *GeoSearchLocationCmd) Clone() Cmder {
 	}
 	return &GeoSearchLocationCmd{
 		baseCmd: cmd.cloneBaseCmd(),
-		opt:     opt,
+		opt:     cmd.opt,
 		val:     val,
 	}
 }

commands_test.go

@@ -9280,6 +9280,24 @@ var _ = Describe("Commands", func() {
 			Expect(res[0].Latitude).To(Equal(37.50266842333162))
 		})
 
+		It("should search geo radius twice with the same query", func() {
+			q := &redis.GeoRadiusQuery{
+				Radius:   200,
+				Unit:     "km",
+				WithDist: true,
+				Sort:     "ASC",
+			}
+			for i := 0; i < 2; i++ {
+				res, err := client.GeoRadius(ctx, "Sicily", 15, 37, q).Result()
+				Expect(err).NotTo(HaveOccurred())
+				Expect(res).To(HaveLen(2))
+				Expect(res[0].Name).To(Equal("Catania"))
+				Expect(res[0].Dist).To(Equal(56.4413))
+				Expect(res[1].Name).To(Equal("Palermo"))
+				Expect(res[1].Dist).To(Equal(190.4424))
+			}
+		})
+
 		It("should search geo radius by member with options", func() {
 			res, err := client.GeoRadiusByMember(ctx, "Sicily", "Catania", &redis.GeoRadiusQuery{
 				Radius:      200,
@@ -9462,6 +9480,18 @@ var _ = Describe("Commands", func() {
 			Expect(val).To(Equal([]string{"Palermo"}))
 		})
 
+		It("should geo search without changing the query", func() {
+			q := &redis.GeoSearchQuery{
+				Member: "Catania",
+				Radius: 200,
+				Sort:   "asc",
+			}
+			val, err := client.GeoSearch(ctx, "Sicily", q).Result()
+			Expect(err).NotTo(HaveOccurred())
+			Expect(val).To(Equal([]string{"Catania", "Palermo"}))
+			Expect(q.RadiusUnit).To(BeEmpty())
+		})
+
 		It("should geo search with options", func() {
 			q := &redis.GeoSearchLocationQuery{
 				GeoSearchQuery: redis.GeoSearchQuery{

geo_commands_unit_test.go

@@ -0,0 +1,48 @@
+package redis
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/redis/go-redis/v9/internal/proto"
+)
+
+// A pipelined or autopipelined command is built when it is queued and its
+// reply is read later. Changing the caller's query in between must not change
+// how the reply is parsed.
+func TestGeoCmdsParseReplyWithQueuedFlags(t *testing.T) {
+	// Two entries, each [name, dist]: the reply to a WITHDIST query.
+	const withDistReply = "*2\r\n" +
+		"*2\r\n$7\r\nCatania\r\n$7\r\n56.4413\r\n" +
+		"*2\r\n$7\r\nPalermo\r\n$8\r\n190.4424\r\n"
+
+	t.Run("GeoLocationCmd", func(t *testing.T) {
+		q := &GeoRadiusQuery{Radius: 200, Unit: "km", WithDist: true}
+		cmd := NewGeoLocationCmd(context.Background(), q, "georadius", "Sicily", 15, 37)
+		q.WithDist, q.WithCoord = false, true
+
+		if err := cmd.readReply(proto.NewReader(strings.NewReader(withDistReply))); err != nil {
+			t.Fatalf("readReply: %v", err)
+		}
+		if got := cmd.Val(); len(got) != 2 || got[0].Dist != 56.4413 || got[1].Dist != 190.4424 {
+			t.Fatalf("got %+v", got)
+		}
+	})
+
+	t.Run("GeoSearchLocationCmd", func(t *testing.T) {
+		q := &GeoSearchLocationQuery{
+			GeoSearchQuery: GeoSearchQuery{Member: "Catania", Radius: 200},
+			WithDist:       true,
+		}
+		cmd := NewGeoSearchLocationCmd(context.Background(), q, "geosearch", "Sicily")
+		q.WithDist, q.WithCoord = false, true
+
+		if err := cmd.readReply(proto.NewReader(strings.NewReader(withDistReply))); err != nil {
+			t.Fatalf("readReply: %v", err)
+		}
+		if got := cmd.Val(); len(got) != 2 || got[0].Dist != 56.4413 || got[1].Dist != 190.4424 {
+			t.Fatalf("got %+v", got)
+		}
+	})
+}