fix(models): make group-equality order-insensitive everywhere

sameGroupClaim (device_projection.go, used to validate a member's
claim agrees with the master's) compared roles via a device-ID-keyed
map, making it order-insensitive. webtypes.replaceGroup's change
detection used reflect.DeepEqual on the whole *Group, which is
order-sensitive for Roles.Roles. Both the polled /getGroup response
and the pushed groupUpdated event populate Roles.Roles directly from
XML unmarshaling in wire order, so nothing guarantees a pair's roles
list in the same order across two reads -- DeepEqual could then report
a spurious "changed" for a pair that didn't actually change.

Extracted the order-insensitive comparison into models.SameGroup as
the single shared implementation (also handles the nil/nil case
correctly, unlike the old sameGroupClaim, which mattered for
replaceGroup's existing "no prior group" path). Both call sites now
use it; the duplicate sameGroupClaim is gone.

Added TestApplyGroupEventIgnoresRoleOrder, verified to fail against
the prior DeepEqual-based logic and pass with this fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tobias Gesellchen
2026-08-30 22:27:56 +02:00
co-authored by Claude Sonnet 5
parent 9629d3e057
commit 522c6b8cb6
4 changed files with 76 additions and 24 deletions
+41 -1
View File
@@ -1,6 +1,9 @@
package models
import "encoding/xml"
import (
"encoding/xml"
"strings"
)
// Group represents a stereo pair of two ST10 SoundTouch speakers.
type Group struct {
@@ -32,3 +35,40 @@ type GroupRole struct {
Role string `xml:"role"`
IPAddress string `xml:"ipAddress,omitempty"`
}
// 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
// WebSocket event both populate Roles.Roles directly from XML unmarshaling
// in wire order, so a polled read and a pushed event for the identical pair
// are not guaranteed to list roles in the same order -- comparing with
// 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.
func SameGroup(left, right *Group) bool {
if left == nil && right == nil {
return true
}
if left == nil || right == nil {
return false
}
if left.ID != right.ID || left.MasterDeviceID != right.MasterDeviceID ||
len(left.Roles.Roles) != len(right.Roles.Roles) {
return false
}
rightRoles := make(map[string]string, len(right.Roles.Roles))
for _, role := range right.Roles.Roles {
rightRoles[strings.TrimSpace(role.DeviceID)] = strings.ToUpper(strings.TrimSpace(role.Role))
}
for _, role := range left.Roles.Roles {
if rightRoles[strings.TrimSpace(role.DeviceID)] != strings.ToUpper(strings.TrimSpace(role.Role)) {
return false
}
}
return true
}
+1 -21
View File
@@ -204,27 +204,7 @@ func registeredMembersAgree(group *models.Group, byDeviceID map[string][]deviceP
continue
}
if entries[0].Status == nil || !sameGroupClaim(group, entries[0].Status.Group) {
return false
}
}
return true
}
func sameGroupClaim(left, right *models.Group) bool {
if left == nil || right == nil || left.ID != right.ID || left.MasterDeviceID != right.MasterDeviceID ||
len(left.Roles.Roles) != len(right.Roles.Roles) {
return false
}
rightRoles := make(map[string]string, len(right.Roles.Roles))
for _, role := range right.Roles.Roles {
rightRoles[strings.TrimSpace(role.DeviceID)] = strings.ToUpper(strings.TrimSpace(role.Role))
}
for _, role := range left.Roles.Roles {
if rightRoles[strings.TrimSpace(role.DeviceID)] != strings.ToUpper(strings.TrimSpace(role.Role)) {
if entries[0].Status == nil || !models.SameGroup(group, entries[0].Status.Group) {
return false
}
}
@@ -176,6 +176,39 @@ func TestEmptyGroupClearsCurrentClaim(t *testing.T) {
}
}
// TestApplyGroupEventIgnoresRoleOrder guards replaceGroup's change-detection
// against a spurious "changed" report when the same pair's roles simply
// arrive in a different order -- a polled /getGroup response and a pushed
// groupUpdated event both populate Roles.Roles straight from XML unmarshal
// in wire order, so nothing guarantees they list LEFT/RIGHT the same way
// every time for the identical pair.
func TestApplyGroupEventIgnoresRoleOrder(t *testing.T) {
conn := NewDeviceConnection(nil, &models.DeviceInfo{Name: "test"})
leftFirst := &models.Group{
ID: "pair-1",
MasterDeviceID: "master",
Roles: models.GroupRoles{Roles: []models.GroupRole{
{DeviceID: "master", Role: "LEFT"},
{DeviceID: "member", Role: "RIGHT"},
}},
}
conn.SetStatus(&DeviceStatus{Group: leftFirst})
rightFirst := &models.Group{
ID: "pair-1",
MasterDeviceID: "master",
Roles: models.GroupRoles{Roles: []models.GroupRole{
{DeviceID: "member", Role: "RIGHT"},
{DeviceID: "master", Role: "LEFT"},
}},
}
if conn.ApplyGroupEvent(rightFirst, time.Now()) {
t.Fatal("reordered roles for the same pair must not report a change")
}
}
func TestStatusSnapshotIsolation(t *testing.T) {
// A snapshot returned by Status() must NOT change when a later
// UpdateStatus replaces a pointer field. This proves the atomic
+1 -2
View File
@@ -2,7 +2,6 @@
package webtypes
import (
"reflect"
"sync"
"sync/atomic"
"time"
@@ -190,7 +189,7 @@ func (c *DeviceConnection) ApplyGroupEvent(group *models.Group, activity time.Ti
}
func (c *DeviceConnection) replaceGroup(group *models.Group, activity time.Time) bool {
changed := !reflect.DeepEqual(c.Status().Group, group)
changed := !models.SameGroup(c.Status().Group, group)
c.UpdateStatus(func(s *DeviceStatus) {
s.Group = group
if !activity.IsZero() {