From 46ddf76cc92b13ce19648dc4ceba339cbd83a3e2 Mon Sep 17 00:00:00 2001 From: Yingzi Zhang <164733623+LindseyZ1205@users.noreply.github.com> Date: Wed, 21 Oct 2026 07:47:28 -0400 Subject: [PATCH] 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 | 99 +++++++++++++++------------------------ commands_test.go | 30 ++++++++++++ geo_commands_unit_test.go | 48 +++++++++++++++++++ 3 files changed, 117 insertions(+), 60 deletions(-) create mode 100644 geo_commands_unit_test.go diff --git a/command.go b/command.go index 22d77478..5e0dd5e4 100644 --- a/command.go +++ b/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, } } diff --git a/commands_test.go b/commands_test.go index bd7bfcb6..7620eb29 100644 --- a/commands_test.go +++ b/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{ diff --git a/geo_commands_unit_test.go b/geo_commands_unit_test.go new file mode 100644 index 00000000..98d34b98 --- /dev/null +++ b/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) + } + }) +}