From b534bc5615e58bfa1a9e4759c2fbb5535cb61236 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 5 Sep 2026 11:03:18 +0200 Subject: [PATCH] refactor(models): consolidate group-role topology equality pkg/stereopair's sameRoles and pkg/service/datastore's sameGroupGenerationTopology independently reimplemented the same Role-keyed topology comparison, but normalized IP addresses differently (net.ParseIP-only vs. plain string equality) -- exactly the class of disagreement models.SameGroup was already created to fix for order-sensitivity. Both now delegate their per-role comparison to a new models.SameGroupRoles, which treats equal-but-differently- formatted IPs as a match without regressing the common both-addresses-unset case either implementation relied on. models.SameGroup and datastore's sameStereoPair stay distinct (commented why): both are intentionally ID/IP-agnostic for reasons unrelated to this consolidation. Co-Authored-By: Claude Sonnet 5 --- pkg/models/group.go | 61 ++++++++++++++++++++ pkg/models/group_test.go | 90 ++++++++++++++++++++++++++++++ pkg/service/datastore/datastore.go | 41 +++++--------- pkg/stereopair/coordinator.go | 21 ++----- 4 files changed, 168 insertions(+), 45 deletions(-) create mode 100644 pkg/models/group_test.go diff --git a/pkg/models/group.go b/pkg/models/group.go index cc0074b6..aefbe5bc 100644 --- a/pkg/models/group.go +++ b/pkg/models/group.go @@ -2,6 +2,7 @@ package models import ( "encoding/xml" + "net" "strings" ) @@ -36,6 +37,61 @@ type GroupRole struct { IPAddress string `xml:"ipAddress,omitempty"` } +// SameGroupRoles reports whether two role slices describe the same stereo +// pair topology: matching length, no duplicate Role value on either side, and +// every role paired by Role with equal DeviceID and IPAddress. It tolerates +// any role count rather than assuming exactly LEFT/RIGHT. +// +// This is the shared core behind both pkg/stereopair's and +// pkg/service/datastore's topology-equality checks -- they used to compare +// IPAddress independently (one via net.ParseIP, one via plain string +// equality), which could disagree about whether two differently-formatted +// but equal addresses matched. See i655 code-review finding #10. +func SameGroupRoles(a, b []GroupRole) bool { + if len(a) != len(b) { + return false + } + + byRole := make(map[string]GroupRole, len(a)) + for _, role := range a { + if _, duplicate := byRole[role.Role]; duplicate { + return false + } + + byRole[role.Role] = role + } + + seen := make(map[string]struct{}, len(b)) + for _, role := range b { + if _, duplicate := seen[role.Role]; duplicate { + return false + } + + seen[role.Role] = struct{}{} + + other, ok := byRole[role.Role] + if !ok || other.DeviceID != role.DeviceID || !sameRoleIPAddress(other.IPAddress, role.IPAddress) { + return false + } + } + + return true +} + +// sameRoleIPAddress treats identical strings (including two empty/unset +// addresses) as equal, and otherwise falls back to parsed-IP equality so two +// differently-formatted representations of the same address still match. It +// never treats one populated and one empty/unparsable address as a match. +func sameRoleIPAddress(a, b string) bool { + if a == b { + return true + } + + parsedA, parsedB := net.ParseIP(a), net.ParseIP(b) + + return parsedA != nil && parsedB != nil && parsedA.Equal(parsedB) +} + // SameGroup reports whether left and right describe the same stereo-pair // configuration, comparing role assignments by device ID rather than by // slice order. The device's own /getGroup response and its groupUpdated @@ -45,6 +101,11 @@ type GroupRole struct { // reflect.DeepEqual (order-sensitive) would then report a spurious change // even though nothing about the pair actually changed. Two nil Groups are // equal; exactly one nil is not. +// +// SameGroup stays a distinct, IP-agnostic implementation from +// SameGroupRoles: its callers (event/status projection) need +// order-independence without caring about IP, and adding an IP check here +// would change that contract. func SameGroup(left, right *Group) bool { if left == nil && right == nil { return true diff --git a/pkg/models/group_test.go b/pkg/models/group_test.go new file mode 100644 index 00000000..7eb6928e --- /dev/null +++ b/pkg/models/group_test.go @@ -0,0 +1,90 @@ +package models + +import "testing" + +func TestSameGroupRoles(t *testing.T) { + base := []GroupRole{ + {DeviceID: "LEFT-ID", Role: "LEFT", IPAddress: "192.0.2.10"}, + {DeviceID: "RIGHT-ID", Role: "RIGHT", IPAddress: "192.0.2.11"}, + } + + t.Run("identical roles match", func(t *testing.T) { + if !SameGroupRoles(base, append([]GroupRole(nil), base...)) { + t.Fatal("identical roles reported as different") + } + }) + + t.Run("role order does not matter", func(t *testing.T) { + reordered := []GroupRole{base[1], base[0]} + if !SameGroupRoles(base, reordered) { + t.Fatal("reordered roles reported as different") + } + }) + + t.Run("differently formatted equal IP matches", func(t *testing.T) { + other := append([]GroupRole(nil), base...) + other[0].IPAddress = "::ffff:192.0.2.10" // IPv4-mapped IPv6 form of the same address + if !SameGroupRoles(base, other) { + t.Fatal("differently-formatted equal IP addresses reported as different") + } + }) + + t.Run("both empty IP addresses match", func(t *testing.T) { + noIP := []GroupRole{ + {DeviceID: "LEFT-ID", Role: "LEFT"}, + {DeviceID: "RIGHT-ID", Role: "RIGHT"}, + } + if !SameGroupRoles(noIP, append([]GroupRole(nil), noIP...)) { + t.Fatal("two roles with unset IP addresses reported as different") + } + }) + + t.Run("different IP does not match", func(t *testing.T) { + other := append([]GroupRole(nil), base...) + other[0].IPAddress = "198.51.100.10" + if SameGroupRoles(base, other) { + t.Fatal("different IP addresses reported as same") + } + }) + + t.Run("populated vs empty IP does not match", func(t *testing.T) { + other := append([]GroupRole(nil), base...) + other[0].IPAddress = "" + if SameGroupRoles(base, other) { + t.Fatal("populated vs empty IP address reported as same") + } + }) + + t.Run("different DeviceID does not match", func(t *testing.T) { + other := append([]GroupRole(nil), base...) + other[0].DeviceID = "OTHER-ID" + if SameGroupRoles(base, other) { + t.Fatal("different DeviceID reported as same") + } + }) + + t.Run("different Role does not match", func(t *testing.T) { + other := append([]GroupRole(nil), base...) + other[0].Role = "RIGHT" + if SameGroupRoles(base, other) { + t.Fatal("mismatched Role reported as same") + } + }) + + t.Run("different length does not match", func(t *testing.T) { + if SameGroupRoles(base, base[:1]) { + t.Fatal("different-length role slices reported as same") + } + }) + + t.Run("duplicate role on either side does not match", func(t *testing.T) { + duplicateA := []GroupRole{base[0], base[0]} + duplicateB := []GroupRole{base[1], base[1]} + if SameGroupRoles(duplicateA, base) { + t.Fatal("duplicate role in first argument reported as same") + } + if SameGroupRoles(base, duplicateB) { + t.Fatal("duplicate role in second argument reported as same") + } + }) +} diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index e6fdab94..057fbabf 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -3422,6 +3422,13 @@ func stereoRoleDevices(group *models.Group) (left, right string, ok bool) { return left, right, left != "" && right != "" } +// sameStereoPair stays a distinct, ID-agnostic and IP-agnostic +// implementation from sameGroupGenerationTopology: AddGroup's idempotency +// reuse check needs to recognize a retried create (which supplies no +// pre-existing group ID and may carry a since-changed IP) as "the same +// pair," not just an exact topology match. See i655 code-review finding #10 +// (and the separately-tracked finding #4 about this reuse potentially +// returning a stale IP). func sameStereoPair(a, b *models.Group) bool { if len(a.Roles.Roles) != 2 || len(b.Roles.Roles) != 2 || a.MasterDeviceID != b.MasterDeviceID { return false @@ -3433,38 +3440,16 @@ func sameStereoPair(a, b *models.Group) bool { return aOK && bOK && aLeft == bLeft && aRight == bRight } +// sameGroupGenerationTopology delegates its per-role comparison to +// models.SameGroupRoles, the shared topology-equality core also used by +// pkg/stereopair -- see i655 code-review finding #10 (three independent, +// subtly different implementations used to coexist). func sameGroupGenerationTopology(a, b *models.Group) bool { - if a == nil || b == nil || a.ID != b.ID || - a.MasterDeviceID != b.MasterDeviceID || len(a.Roles.Roles) != len(b.Roles.Roles) { + if a == nil || b == nil || a.ID != b.ID || a.MasterDeviceID != b.MasterDeviceID { return false } - roles := make(map[string]models.GroupRole, len(a.Roles.Roles)) - for i := range a.Roles.Roles { - role := a.Roles.Roles[i] - if _, duplicate := roles[role.Role]; duplicate { - return false - } - - roles[role.Role] = role - } - - seen := make(map[string]struct{}, len(b.Roles.Roles)) - for i := range b.Roles.Roles { - role := b.Roles.Roles[i] - if _, duplicate := seen[role.Role]; duplicate { - return false - } - - seen[role.Role] = struct{}{} - - other, found := roles[role.Role] - if !found || other.DeviceID != role.DeviceID || other.IPAddress != role.IPAddress { - return false - } - } - - return true + return models.SameGroupRoles(a.Roles.Roles, b.Roles.Roles) } func groupDeviceIDs(group *models.Group) map[string]struct{} { diff --git a/pkg/stereopair/coordinator.go b/pkg/stereopair/coordinator.go index d73c3561..915ad114 100644 --- a/pkg/stereopair/coordinator.go +++ b/pkg/stereopair/coordinator.go @@ -1502,24 +1502,11 @@ func sameGroupTopology(a, b *models.Group) bool { sameRoles(a.Roles.Roles, b.Roles.Roles) } +// sameRoles delegates to models.SameGroupRoles, the shared topology-equality +// core also used by pkg/service/datastore -- see i655 code-review finding +// #10 (three independent, subtly different implementations used to coexist). func sameRoles(a, b []models.GroupRole) bool { - if len(a) != len(b) { - return false - } - - byRole := make(map[string]models.GroupRole, len(a)) - for i := range a { - byRole[a[i].Role] = a[i] - } - - for i := range b { - other, ok := byRole[b[i].Role] - if !ok || other.DeviceID != b[i].DeviceID || !sameIP(other.IPAddress, b[i].IPAddress) { - return false - } - } - - return true + return models.SameGroupRoles(a, b) } func groupContainsDevice(group *models.Group, deviceID string) bool {