diff --git a/cmd/deep-gen/main.go b/cmd/deep-gen/main.go index d0367ab..ca5c2d2 100644 --- a/cmd/deep-gen/main.go +++ b/cmd/deep-gen/main.go @@ -545,7 +545,8 @@ var applyOpTmpl = template.Must(template.New("applyOp").Funcs(tmplFuncs).Parse( switch op.Path { case "/": if op.Strict && (op.Kind == {{.P}}OpReplace || op.Kind == {{.P}}OpRemove) { - if !{{.P}}Equal(*t, op.Old.({{.TypeName}})) { + old, ok := op.Old.({{.TypeName}}) + if !ok || !{{.P}}Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/condition/condition.go b/condition/condition.go index 78f98a8..312b64e 100644 --- a/condition/condition.go +++ b/condition/condition.go @@ -58,13 +58,14 @@ func Evaluate(root reflect.Value, c *Condition) (bool, error) { return false, nil } if c.Op == Not { - if len(c.Sub) > 0 { - ok, err := Evaluate(root, c.Sub[0]) - if err != nil { - return false, err - } - return !ok, nil + if len(c.Sub) == 0 { + return false, fmt.Errorf("malformed Not condition: missing sub-condition") + } + ok, err := Evaluate(root, c.Sub[0]) + if err != nil { + return false, err } + return !ok, nil } val, err := icore.DeepPath(c.Path).Resolve(root) diff --git a/condition/condition_test.go b/condition/condition_test.go index e17280b..b6b2185 100644 --- a/condition/condition_test.go +++ b/condition/condition_test.go @@ -76,3 +76,17 @@ func TestEvaluate(t *testing.T) { } } } + +// TestNotEmptySubReturnsError asserts that a malformed Not condition +// (no Sub-condition) yields an explicit error rather than silently +// falling through to the path-based comparison that would compare a +// nil-path value against c.Value. +func TestNotEmptySubReturnsError(t *testing.T) { + type S struct{ N int } + root := reflect.ValueOf(&S{N: 1}).Elem() + c := &Condition{Op: Not} + got, err := Evaluate(root, c) + if err == nil { + t.Errorf("Evaluate(Not{empty Sub}) = %v, nil; want error", got) + } +} diff --git a/crdt/crdt.go b/crdt/crdt.go index 9eea3b2..4360b7c 100644 --- a/crdt/crdt.go +++ b/crdt/crdt.go @@ -392,6 +392,6 @@ func (c *CRDT[T]) UnmarshalJSON(data []byte) error { c.tombstones = m.Tombstones c.nodeID = m.NodeID c.clock = hlc.NewClock(m.NodeID) - c.clock.Latest = m.Latest + c.clock.SetLatest(m.Latest) return nil } diff --git a/crdt/hlc/hlc.go b/crdt/hlc/hlc.go index 446c2c0..6223c10 100644 --- a/crdt/hlc/hlc.go +++ b/crdt/hlc/hlc.go @@ -15,6 +15,7 @@ package hlc import ( "fmt" + "math" "sync" "time" ) @@ -59,6 +60,11 @@ func (h HLC) String() string { } // Clock manages the local HLC state. +// +// Latest is exposed for serialisation but must not be mutated by callers +// after the clock is in use — direct writes bypass the internal mutex and +// race with concurrent Now/Update. Use [Clock.SetLatest] for explicit +// rehydration (e.g. from snapshots). type Clock struct { mu sync.Mutex Latest HLC @@ -77,13 +83,31 @@ func NewClock(nodeID string) *Clock { } } +// SetLatest rehydrates the clock from a previously observed timestamp under +// the clock's mutex, so it is safe to call alongside concurrent Now/Update. +// Subsequent Now/Update calls advance from at least h. +func (c *Clock) SetLatest(h HLC) { + c.mu.Lock() + defer c.mu.Unlock() + c.Latest = h +} + // Now returns the current HLC timestamp. func (c *Clock) Now() HLC { return c.Reserve(1) } // Reserve returns the current HLC timestamp and reserves n logical ticks. +// +// n must be non-negative and small enough that c.Latest.Logical + n fits in +// int32; otherwise Reserve panics. Practical text inserts and similar uses +// fall well under that bound; overflow would silently break causal ordering, +// so an explicit panic is preferred to a wraparound bug. func (c *Clock) Reserve(n int) HLC { + if n < 0 { + panic("hlc: Reserve called with negative n") + } + c.mu.Lock() defer c.mu.Unlock() @@ -94,6 +118,10 @@ func (c *Clock) Reserve(n int) HLC { c.Latest.Logical = 0 } + if int64(c.Latest.Logical)+int64(n) > int64(math.MaxInt32) { + panic("hlc: Reserve would overflow Logical (int32)") + } + start := c.Latest c.Latest.Logical += int32(n) return start diff --git a/crdt/hlc/hlc_test.go b/crdt/hlc/hlc_test.go index 64a427f..fd226b6 100644 --- a/crdt/hlc/hlc_test.go +++ b/crdt/hlc/hlc_test.go @@ -1,6 +1,7 @@ package hlc import ( + "math" "testing" "time" ) @@ -97,3 +98,48 @@ func TestClock_UpdateMore(t *testing.T) { t.Errorf("expected logical %d, got %d", remote2.Logical+1, c.Latest.Logical) } } + +// TestSetLatestSafeConcurrentWithNow stresses SetLatest against concurrent +// Now calls and checks that the race detector reports no data race on +// Clock.Latest. Direct field assignment would race here; SetLatest must +// take the clock mutex to be safe. +func TestSetLatestSafeConcurrentWithNow(t *testing.T) { + c := NewClock("n1") + done := make(chan struct{}) + go func() { + for i := 0; i < 1000; i++ { + _ = c.Now() + } + close(done) + }() + for i := 0; i < 1000; i++ { + c.SetLatest(HLC{WallTime: int64(i), Logical: 0, NodeID: "n1"}) + } + <-done +} + +// TestReserveOverflowPanics ensures Reserve panics rather than silently +// wrapping when the requested reservation would overflow Logical (int32). +func TestReserveOverflowPanics(t *testing.T) { + c := NewClock("n1") + // Use a far-future wall time so Reserve doesn't reset Logical to 0 before + // the overflow check fires. + c.SetLatest(HLC{WallTime: math.MaxInt64, Logical: math.MaxInt32 - 10, NodeID: "n1"}) + defer func() { + if r := recover(); r == nil { + t.Error("expected panic on int32 overflow, got nil") + } + }() + c.Reserve(100) +} + +// TestReserveNegativePanics rejects nonsensical reservations. +func TestReserveNegativePanics(t *testing.T) { + c := NewClock("n1") + defer func() { + if r := recover(); r == nil { + t.Error("expected panic on negative n") + } + }() + c.Reserve(-1) +} diff --git a/crdt/set.go b/crdt/set.go index 6e7204f..a289a13 100644 --- a/crdt/set.go +++ b/crdt/set.go @@ -37,6 +37,12 @@ func (s *Set[T]) NodeID() string { return s.inner.NodeID() } // Add appends a new uniquely-tagged entry for elem. // The tag is the current HLC timestamp serialised as a string map key. +// +// Two clock ticks occur per Add: one to mint the entry's tag and a second +// inside the underlying Edit for the resulting Delta's Timestamp. Both +// values are monotonic per the HLC mutex, so the extra tick is harmless; +// keeping the tag-mint outside Edit means the tag is fixed before the +// inner closure runs, which keeps the data-flow easy to follow. func (s *Set[T]) Add(elem T) { id := s.inner.Clock().Now() s.inner.Edit(func(si *setInner[T]) { @@ -86,8 +92,19 @@ func (s *Set[T]) Items() []T { } // Len returns the number of distinct live elements. +// +// Cost is O(n) in the number of entries (live + tombstoned) because OR-Set +// duplicates require dedup; the prior implementation built a full slice via +// Items just to take its length, which this avoids. func (s *Set[T]) Len() int { - return len(s.Items()) + state := s.inner.View() + seen := make(map[T]struct{}, len(state.Entries)) + for _, e := range state.Entries { + if !e.Deleted { + seen[e.Elem] = struct{}{} + } + } + return len(seen) } // Merge performs a full state-based OR-Set merge with another Set node. diff --git a/crdt/set_test.go b/crdt/set_test.go index 3793d50..eaee4cd 100644 --- a/crdt/set_test.go +++ b/crdt/set_test.go @@ -50,6 +50,27 @@ func TestSet_Items(t *testing.T) { } } +// TestSet_LenMatchesItemsAfterChurn asserts Len agrees with len(Items) after a +// mix of adds, duplicates, and removes. The previous Len implementation called +// Items() and took its length; the new direct dedup must produce the same +// distinct-live count without going through the allocation. +func TestSet_LenMatchesItemsAfterChurn(t *testing.T) { + s := NewSet[string]("node-a") + s.Add("a") + s.Add("b") + s.Add("a") // duplicate live add + s.Add("c") + s.Remove("b") // tombstone + + want := len(s.Items()) + if got := s.Len(); got != want { + t.Errorf("Len()=%d, len(Items())=%d", got, want) + } + if want != 2 { + t.Errorf("expected 2 distinct live items, got %d", want) + } +} + func TestSet_DuplicateAdd(t *testing.T) { s := NewSet[string]("node-a") s.Add("dup") diff --git a/diff.go b/diff.go index 4f2e029..816ff9a 100644 --- a/diff.go +++ b/diff.go @@ -35,12 +35,21 @@ func Diff[T any](a, b T) (Patch[T], error) { res := Patch[T]{} p.Walk(func(path string, op engine.OpKind, old, new any) error { - res.Operations = append(res.Operations, Operation{ + o := Operation{ Kind: op, Path: path, - Old: old, New: new, - }) + } + // Internal walk emits the source path in `old` for Move/Copy; lift it + // into the typed From field so Old stays free for prior values. + if op == engine.OpMove || op == engine.OpCopy { + if s, ok := old.(string); ok { + o.From = s + } + } else { + o.Old = old + } + res.Operations = append(res.Operations, o) return nil }) diff --git a/engine_test.go b/engine_test.go index b4ecb0b..6093613 100644 --- a/engine_test.go +++ b/engine_test.go @@ -108,6 +108,34 @@ func TestNilMapDiff(t *testing.T) { } } +// TestOpCopyDeepCopies asserts OpCopy on a reference-typed field gives the +// destination its own backing storage, so mutations to the source no longer +// leak into the destination. +func TestOpCopyDeepCopies(t *testing.T) { + type S struct { + A []int + B []int + M map[string]int + N map[string]int + } + s := &S{A: []int{1, 2, 3}, M: map[string]int{"k": 1}} + p := deep.Patch[S]{Operations: []deep.Operation{ + {Kind: deep.OpCopy, Path: "/B", From: "/A"}, + {Kind: deep.OpCopy, Path: "/N", From: "/M"}, + }} + if err := deep.Apply(s, p); err != nil { + t.Fatalf("Apply: %v", err) + } + s.A[0] = 99 + s.M["k"] = 99 + if s.B[0] == 99 { + t.Errorf("OpCopy on []int aliases source: B=%v", s.B) + } + if s.N["k"] == 99 { + t.Errorf("OpCopy on map aliases source: N=%v", s.N) + } +} + func TestReflectionEngineAdvanced(t *testing.T) { type Data struct { A int @@ -117,8 +145,8 @@ func TestReflectionEngineAdvanced(t *testing.T) { p := deep.Patch[Data]{} p.Operations = []deep.Operation{ - {Kind: deep.OpMove, Path: "/B", Old: "/A"}, - {Kind: deep.OpCopy, Path: "/A", Old: "/B"}, + {Kind: deep.OpMove, Path: "/B", From: "/A"}, + {Kind: deep.OpCopy, Path: "/A", From: "/B"}, {Kind: deep.OpRemove, Path: "/A"}, } @@ -127,18 +155,56 @@ func TestReflectionEngineAdvanced(t *testing.T) { } } +// TestStrictRootMismatchedOldType asserts that a strict OpReplace at root +// whose Old value carries the wrong concrete type returns an error rather +// than panicking on the type assertion. +func TestStrictRootMismatchedOldType(t *testing.T) { + u := &testmodels.User{Name: "alice"} + p := deep.Patch[testmodels.User]{ + Strict: true, + Operations: []deep.Operation{ + {Kind: deep.OpReplace, Path: "/", Old: "not-a-User", New: testmodels.User{Name: "bob"}}, + }, + } + defer func() { + if r := recover(); r != nil { + t.Errorf("Apply panicked on mismatched Old type: %v", r) + } + }() + if err := deep.Apply(u, p); err == nil { + t.Error("expected strict check error on mismatched Old type, got nil") + } +} + func TestEngineFailures(t *testing.T) { u := &testmodels.User{} - // Move from non-existent + // Move from non-existent path must surface an error rather than silently + // no-op (previously this test ignored the return value). p1 := deep.Patch[testmodels.User]{} - p1.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id", Old: "/nonexistent"}} - deep.Apply(u, p1) + p1.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id", From: "/nonexistent"}} + if err := deep.Apply(u, p1); err == nil { + t.Error("OpMove from non-existent source should return an error") + } - // Copy from non-existent + // Copy from non-existent path must also surface an error. p2 := deep.Patch[testmodels.User]{} - p2.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id", Old: "/nonexistent"}} - deep.Apply(u, p2) + p2.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id", From: "/nonexistent"}} + if err := deep.Apply(u, p2); err == nil { + t.Error("OpCopy from non-existent source should return an error") + } + + // Move/Copy with empty From must reject early with a clear error. + p3 := deep.Patch[testmodels.User]{} + p3.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id"}} + if err := deep.Apply(u, p3); err == nil { + t.Error("OpMove with empty From should return an error") + } + p4 := deep.Patch[testmodels.User]{} + p4.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id"}} + if err := deep.Apply(u, p4); err == nil { + t.Error("OpCopy with empty From should return an error") + } // Apply to nil if err := deep.Apply((*testmodels.User)(nil), p1); err == nil { diff --git a/examples/atomic_config/proxyconfig_deep.go b/examples/atomic_config/proxyconfig_deep.go index 851414f..04ff5b4 100644 --- a/examples/atomic_config/proxyconfig_deep.go +++ b/examples/atomic_config/proxyconfig_deep.go @@ -62,7 +62,8 @@ func (t *ProxyConfig) applyOperation(op deep.Operation, logger *slog.Logger) (bo switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(ProxyConfig)) { + old, ok := op.Old.(ProxyConfig) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -340,7 +341,8 @@ func (t *SystemMeta) applyOperation(op deep.Operation, logger *slog.Logger) (boo switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(SystemMeta)) { + old, ok := op.Old.(SystemMeta) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/audit_logging/user_deep.go b/examples/audit_logging/user_deep.go index e970ee4..ef8cafe 100644 --- a/examples/audit_logging/user_deep.go +++ b/examples/audit_logging/user_deep.go @@ -63,7 +63,8 @@ func (t *User) applyOperation(op deep.Operation, logger *slog.Logger) (bool, err switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(User)) { + old, ok := op.Old.(User) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/concurrent_updates/stock_deep.go b/examples/concurrent_updates/stock_deep.go index 44c6cf8..3255ea8 100644 --- a/examples/concurrent_updates/stock_deep.go +++ b/examples/concurrent_updates/stock_deep.go @@ -62,7 +62,8 @@ func (t *Stock) applyOperation(op deep.Operation, logger *slog.Logger) (bool, er switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Stock)) { + old, ok := op.Old.(Stock) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/config_manager/config_deep.go b/examples/config_manager/config_deep.go index 4fded8b..f184bcc 100644 --- a/examples/config_manager/config_deep.go +++ b/examples/config_manager/config_deep.go @@ -63,7 +63,8 @@ func (t *Config) applyOperation(op deep.Operation, logger *slog.Logger) (bool, e switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Config)) { + old, ok := op.Old.(Config) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/http_patch_api/resource_deep.go b/examples/http_patch_api/resource_deep.go index b8bee12..def6ec9 100644 --- a/examples/http_patch_api/resource_deep.go +++ b/examples/http_patch_api/resource_deep.go @@ -62,7 +62,8 @@ func (t *Resource) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Resource)) { + old, ok := op.Old.(Resource) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/json_interop/uistate_deep.go b/examples/json_interop/uistate_deep.go index b2751f6..3b7630f 100644 --- a/examples/json_interop/uistate_deep.go +++ b/examples/json_interop/uistate_deep.go @@ -62,7 +62,8 @@ func (t *UIState) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(UIState)) { + old, ok := op.Old.(UIState) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/keyed_inventory/inventory_deep.go b/examples/keyed_inventory/inventory_deep.go index dc0235a..43c8196 100644 --- a/examples/keyed_inventory/inventory_deep.go +++ b/examples/keyed_inventory/inventory_deep.go @@ -62,7 +62,8 @@ func (t *Item) applyOperation(op deep.Operation, logger *slog.Logger) (bool, err switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Item)) { + old, ok := op.Old.(Item) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -340,7 +341,8 @@ func (t *Inventory) applyOperation(op deep.Operation, logger *slog.Logger) (bool switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Inventory)) { + old, ok := op.Old.(Inventory) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/multi_error/strictuser_deep.go b/examples/multi_error/strictuser_deep.go index 6b1f594..7ecefbc 100644 --- a/examples/multi_error/strictuser_deep.go +++ b/examples/multi_error/strictuser_deep.go @@ -62,7 +62,8 @@ func (t *StrictUser) applyOperation(op deep.Operation, logger *slog.Logger) (boo switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(StrictUser)) { + old, ok := op.Old.(StrictUser) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/policy_engine/employee_deep.go b/examples/policy_engine/employee_deep.go index ce03ed8..0402e87 100644 --- a/examples/policy_engine/employee_deep.go +++ b/examples/policy_engine/employee_deep.go @@ -62,7 +62,8 @@ func (t *Employee) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Employee)) { + old, ok := op.Old.(Employee) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/state_management/docstate_deep.go b/examples/state_management/docstate_deep.go index 7e77670..2e4bf50 100644 --- a/examples/state_management/docstate_deep.go +++ b/examples/state_management/docstate_deep.go @@ -63,7 +63,8 @@ func (t *DocState) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(DocState)) { + old, ok := op.Old.(DocState) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/struct_map_keys/fleet_deep.go b/examples/struct_map_keys/fleet_deep.go index 1b84ff2..eeeda5d 100644 --- a/examples/struct_map_keys/fleet_deep.go +++ b/examples/struct_map_keys/fleet_deep.go @@ -61,7 +61,8 @@ func (t *Fleet) applyOperation(op deep.Operation, logger *slog.Logger) (bool, er switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Fleet)) { + old, ok := op.Old.(Fleet) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/three_way_merge/systemconfig_deep.go b/examples/three_way_merge/systemconfig_deep.go index 9436776..0f37e86 100644 --- a/examples/three_way_merge/systemconfig_deep.go +++ b/examples/three_way_merge/systemconfig_deep.go @@ -63,7 +63,8 @@ func (t *SystemConfig) applyOperation(op deep.Operation, logger *slog.Logger) (b switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(SystemConfig)) { + old, ok := op.Old.(SystemConfig) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/websocket_sync/gameworld_deep.go b/examples/websocket_sync/gameworld_deep.go index 87888ba..6a93595 100644 --- a/examples/websocket_sync/gameworld_deep.go +++ b/examples/websocket_sync/gameworld_deep.go @@ -63,7 +63,8 @@ func (t *GameWorld) applyOperation(op deep.Operation, logger *slog.Logger) (bool switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(GameWorld)) { + old, ok := op.Old.(GameWorld) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -345,7 +346,8 @@ func (t *Player) applyOperation(op deep.Operation, logger *slog.Logger) (bool, e switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Player)) { + old, ok := op.Old.(Player) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/internal/engine/apply_reflection.go b/internal/engine/apply_reflection.go index 18ac799..c6aa12a 100644 --- a/internal/engine/apply_reflection.go +++ b/internal/engine/apply_reflection.go @@ -21,6 +21,9 @@ func ApplyOpReflection[T any](target *T, op Operation, logger *slog.Logger) erro // ApplyOpReflectionValue applies op to the already-reflected value v. func ApplyOpReflectionValue(v reflect.Value, op Operation, logger *slog.Logger) error { + if logger == nil { + logger = slog.Default() + } // Strict check. if op.Strict && (op.Kind == OpReplace || op.Kind == OpRemove) { current, err := icore.DeepPath(op.Path).Resolve(v) @@ -71,22 +74,26 @@ func ApplyOpReflectionValue(v reflect.Value, op Operation, logger *slog.Logger) case OpRemove: err = icore.DeepPath(op.Path).Delete(v) case OpMove: - fromPath := op.Old.(string) + if op.From == "" { + return fmt.Errorf("move at %s: missing From source path", op.Path) + } var val reflect.Value - val, err = icore.DeepPath(fromPath).Resolve(v) + val, err = icore.DeepPath(op.From).Resolve(v) if err == nil { copied := reflect.New(val.Type()).Elem() copied.Set(val) - if err = icore.DeepPath(fromPath).Delete(v); err == nil { + if err = icore.DeepPath(op.From).Delete(v); err == nil { err = icore.DeepPath(op.Path).Set(v, copied) } } case OpCopy: - fromPath := op.Old.(string) + if op.From == "" { + return fmt.Errorf("copy at %s: missing From source path", op.Path) + } var val reflect.Value - val, err = icore.DeepPath(fromPath).Resolve(v) + val, err = icore.DeepPath(op.From).Resolve(v) if err == nil { - err = icore.DeepPath(op.Path).Set(v, val) + err = icore.DeepPath(op.Path).Set(v, icore.DeepCopyValue(val)) } case OpLog: logger.Info("deep log", "message", op.New, "path", op.Path) diff --git a/internal/engine/apply_reflection_test.go b/internal/engine/apply_reflection_test.go new file mode 100644 index 0000000..275101b --- /dev/null +++ b/internal/engine/apply_reflection_test.go @@ -0,0 +1,27 @@ +package engine + +import ( + "reflect" + "testing" +) + +// TestApplyOpReflectionValueNilLogger asserts the value-variant entry point +// doesn't panic when a caller passes a nil logger and an OpLog op forces a +// log call. The pointer variant has always nil-checked the logger; the value +// variant must match so external callers (including future generated code) +// don't NPE on logger.Info. +func TestApplyOpReflectionValueNilLogger(t *testing.T) { + type S struct{ A int } + s := S{A: 1} + v := reflect.ValueOf(&s).Elem() + + defer func() { + if r := recover(); r != nil { + t.Errorf("nil logger caused panic: %v", r) + } + }() + + if err := ApplyOpReflectionValue(v, Operation{Kind: OpLog, Path: "/", New: "msg"}, nil); err != nil { + t.Errorf("OpLog with nil logger should succeed silently, got err=%v", err) + } +} diff --git a/internal/engine/operation.go b/internal/engine/operation.go index 70f0ed0..1440fdd 100644 --- a/internal/engine/operation.go +++ b/internal/engine/operation.go @@ -3,9 +3,22 @@ package engine import "github.com/brunoga/deep/v5/condition" // Operation represents a single change within a Patch. +// +// Field semantics by Kind: +// - OpAdd: Path = target; New = added value. +// - OpRemove: Path = target; Old = removed value (prior). +// - OpReplace: Path = target; Old = prior value; New = replacement. +// - OpMove: Path = destination; From = source path; Old = displaced value at Path (optional). +// - OpCopy: Path = destination; From = source path; Old = displaced value at Path (optional). +// - OpLog: Path = scope; New = log message. +// +// Old for OpMove/OpCopy was previously the source-path string; that role now +// belongs to From, freeing Old to carry the prior destination value +// (necessary for full Reverse fidelity when the destination was non-empty). type Operation struct { Kind OpKind `json:"k"` Path string `json:"p"` + From string `json:"f,omitempty"` Old any `json:"o,omitempty"` New any `json:"n,omitempty"` If *condition.Condition `json:"if,omitempty"` diff --git a/internal/testmodels/user_deep.go b/internal/testmodels/user_deep.go index e4d1589..5db52d7 100644 --- a/internal/testmodels/user_deep.go +++ b/internal/testmodels/user_deep.go @@ -64,7 +64,8 @@ func (t *User) applyOperation(op deep.Operation, logger *slog.Logger) (bool, err switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(User)) { + old, ok := op.Old.(User) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -595,7 +596,8 @@ func (t *Detail) applyOperation(op deep.Operation, logger *slog.Logger) (bool, e switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Detail)) { + old, ok := op.Old.(Detail) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/patch.go b/patch.go index fd430bb..9e9ead9 100644 --- a/patch.go +++ b/patch.go @@ -105,9 +105,9 @@ func (p Patch[T]) String() string { case OpReplace: b.WriteString(fmt.Sprintf("Replace %s: %v -> %v", op.Path, op.Old, op.New)) case OpMove: - b.WriteString(fmt.Sprintf("Move %v to %s", op.Old, op.Path)) + b.WriteString(fmt.Sprintf("Move %s to %s", op.From, op.Path)) case OpCopy: - b.WriteString(fmt.Sprintf("Copy %v to %s", op.Old, op.Path)) + b.WriteString(fmt.Sprintf("Copy %s to %s", op.From, op.Path)) case OpLog: b.WriteString(fmt.Sprintf("Log %s: %v", op.Path, op.New)) } @@ -122,6 +122,11 @@ func (p Patch[T]) Reverse() Patch[T] { } for i := len(p.Operations) - 1; i >= 0; i-- { op := p.Operations[i] + // OpLog has no state effect; its inverse is itself a no-op. Skip rather + // than emit a zero-valued Operation (which would default to OpAdd). + if op.Kind == OpLog { + continue + } rev := Operation{ Path: op.Path, } @@ -138,14 +143,24 @@ func (p Patch[T]) Reverse() Patch[T] { rev.New = op.Old case OpMove: rev.Kind = OpMove - // op.Old for Move was the fromPath string. - // To reverse, we move back from current Path to op.Old Path. - rev.Path = fmt.Sprintf("%v", op.Old) - rev.Old = op.Path + rev.Path = op.From + rev.From = op.Path + // If the destination had a displaced value at apply-time, restore + // it via Old; reversing the move strands that value otherwise. + if op.Old != nil { + rev.New = op.Old + } case OpCopy: - // Undoing a copy means removing the copied value at the target path - rev.Kind = OpRemove - rev.Old = op.New + // If we know the prior destination value, restore it with Replace; + // otherwise the destination was empty pre-copy so Remove suffices. + if op.Old != nil { + rev.Kind = OpReplace + rev.Old = op.New + rev.New = op.Old + } else { + rev.Kind = OpRemove + rev.Old = op.New + } } res.Operations = append(res.Operations, rev) } @@ -177,7 +192,7 @@ func (p Patch[T]) ToJSONPatch() ([]byte, error) { case OpAdd, OpReplace: m["value"] = op.New case OpMove, OpCopy: - m["from"] = op.Old + m["from"] = op.From case OpLog: m["value"] = op.New // log message } @@ -197,22 +212,34 @@ func (p Patch[T]) ToJSONPatch() ([]byte, error) { // ParseJSONPatch parses a JSON Patch document (RFC 6902 plus deep extensions) // back into a Patch[T]. This is the inverse of Patch.ToJSONPatch(). +// +// Wire convention: a leading {"op":"test","path":"/","if":} entry +// is interpreted as the global Patch.Guard rather than a regular test op. +// This mirrors what ToJSONPatch emits; user-authored documents that happen +// to start with that exact triple will have it lifted into Guard. A test op +// on "/" without an "if" key is preserved as a regular operation (it has no +// special meaning in this format), and any subsequent test op is treated +// normally. To round-trip a regular test op at "/", attach it via Builder +// rather than serialising it as the document's first entry. func ParseJSONPatch[T any](data []byte) (Patch[T], error) { var ops []map[string]any if err := json.Unmarshal(data, &ops); err != nil { return Patch[T]{}, fmt.Errorf("ParseJSONPatch: %w", err) } res := Patch[T]{} - for _, m := range ops { + for i, m := range ops { opStr, _ := m["op"].(string) path, _ := m["path"].(string) - // Global condition is encoded as a test op on "/" with an "if" predicate. - if opStr == "test" && path == "/" { + // Global condition encoding: ONLY the leading entry (matched as + // op==test, path=="/", "if" present) is lifted into Guard. A later + // test op with the same shape is kept as a regular operation so a + // document with multiple "/" tests round-trips faithfully. + if i == 0 && opStr == "test" && path == "/" { if ifPred, ok := m["if"].(map[string]any); ok { res.Guard = condition.FromPredicate(ifPred) + continue } - continue } op := Operation{Path: path} @@ -236,10 +263,14 @@ func ParseJSONPatch[T any](data []byte) (Patch[T], error) { op.New = m["value"] case "move": op.Kind = OpMove - op.Old = m["from"] + if s, ok := m["from"].(string); ok { + op.From = s + } case "copy": op.Kind = OpCopy - op.Old = m["from"] + if s, ok := m["from"].(string); ok { + op.From = s + } case "log": op.Kind = OpLog op.New = m["value"] @@ -296,13 +327,13 @@ func Remove[T, V any](p Path[T, V]) Op { // Move returns a type-safe move operation that relocates the value at from to to. // Both paths must share the same value type V. func Move[T, V any](from, to Path[T, V]) Op { - return Op{op: Operation{Kind: OpMove, Path: to.String(), Old: from.String()}} + return Op{op: Operation{Kind: OpMove, Path: to.String(), From: from.String()}} } // Copy returns a type-safe copy operation that duplicates the value at from to to. // Both paths must share the same value type V. func Copy[T, V any](from, to Path[T, V]) Op { - return Op{op: Operation{Kind: OpCopy, Path: to.String(), Old: from.String()}} + return Op{op: Operation{Kind: OpCopy, Path: to.String(), From: from.String()}} } // Builder constructs a [Patch] via a fluent chain. diff --git a/patch_test.go b/patch_test.go index 4dca040..09d71cd 100644 --- a/patch_test.go +++ b/patch_test.go @@ -103,8 +103,8 @@ func TestPatchUtilities(t *testing.T) { {Kind: deep.OpAdd, Path: "/a", New: 1}, {Kind: deep.OpRemove, Path: "/b", Old: 2}, {Kind: deep.OpReplace, Path: "/c", Old: 3, New: 4}, - {Kind: deep.OpMove, Path: "/d", Old: "/e"}, - {Kind: deep.OpCopy, Path: "/f", Old: "/g"}, + {Kind: deep.OpMove, Path: "/d", From: "/e"}, + {Kind: deep.OpCopy, Path: "/f", From: "/g"}, {Kind: deep.OpLog, Path: "/h", New: "msg"}, } @@ -162,14 +162,82 @@ func TestPatchReverseExhaustive(t *testing.T) { {Kind: deep.OpAdd, Path: "/a", New: 1}, {Kind: deep.OpRemove, Path: "/b", Old: 2}, {Kind: deep.OpReplace, Path: "/c", Old: 3, New: 4}, - {Kind: deep.OpMove, Path: "/d", Old: "/e"}, - {Kind: deep.OpCopy, Path: "/f", Old: "/g"}, + {Kind: deep.OpMove, Path: "/d", From: "/e"}, + {Kind: deep.OpCopy, Path: "/f", From: "/g"}, {Kind: deep.OpLog, Path: "/h", New: "msg"}, } rev := p.Reverse() - if len(rev.Operations) != 6 { - t.Errorf("expected 6 reversed ops, got %d", len(rev.Operations)) + // OpLog has no state effect; Reverse skips it instead of emitting a + // malformed op whose Kind defaults to OpAdd. + if len(rev.Operations) != 5 { + t.Errorf("expected 5 reversed ops (OpLog skipped), got %d", len(rev.Operations)) + } + for _, op := range rev.Operations { + if op.Kind == deep.OpLog { + t.Errorf("Reverse should drop OpLog, got %+v", op) + } + // Reversing OpLog used to emit {Kind:OpAdd, Path:"/h", New:nil}; guard + // against that exact regression. + if op.Path == "/h" { + t.Errorf("Reverse leaked OpLog at /h as an OpAdd: %+v", op) + } + } +} + +// TestPatchReverseOpCopyWithPriorValue asserts that when an OpCopy carries the +// displaced destination value in Old, Reverse emits an OpReplace that restores +// that value rather than an OpRemove that strands it. +func TestPatchReverseOpCopyWithPriorValue(t *testing.T) { + p := deep.Patch[testmodels.User]{} + p.Operations = []deep.Operation{ + // Pre-copy /dst held "before"; copy overwrote it with "after". + {Kind: deep.OpCopy, Path: "/dst", From: "/src", Old: "before", New: "after"}, + } + rev := p.Reverse() + if len(rev.Operations) != 1 { + t.Fatalf("expected 1 reversed op, got %d", len(rev.Operations)) + } + got := rev.Operations[0] + if got.Kind != deep.OpReplace { + t.Errorf("reverse of OpCopy with prior value should be OpReplace, got %v", got.Kind) + } + if got.Path != "/dst" { + t.Errorf("reverse target path = %q, want /dst", got.Path) + } + if got.Old != "after" || got.New != "before" { + t.Errorf("reverse should restore prior value: got Old=%v New=%v, want Old=after New=before", got.Old, got.New) + } +} + +// TestPatchReverseOpMoveSymmetric asserts OpMove reverses by swapping From and +// Path, restoring the original location. +func TestPatchReverseOpMoveSymmetric(t *testing.T) { + p := deep.Patch[testmodels.User]{} + p.Operations = []deep.Operation{ + {Kind: deep.OpMove, Path: "/dst", From: "/src"}, + } + rev := p.Reverse() + if len(rev.Operations) != 1 { + t.Fatalf("expected 1 reversed op, got %d", len(rev.Operations)) + } + got := rev.Operations[0] + if got.Kind != deep.OpMove || got.Path != "/src" || got.From != "/dst" { + t.Errorf("reverse OpMove: got Path=%s From=%s, want Path=/src From=/dst", got.Path, got.From) + } +} + +// TestPatchReverseOpLogOnly asserts that a patch containing only OpLog ops +// reverses to an empty patch rather than a sequence of malformed OpAdds. +func TestPatchReverseOpLogOnly(t *testing.T) { + p := deep.Patch[testmodels.User]{} + p.Operations = []deep.Operation{ + {Kind: deep.OpLog, Path: "/", New: "first"}, + {Kind: deep.OpLog, Path: "/", New: "second"}, + } + rev := p.Reverse() + if len(rev.Operations) != 0 { + t.Errorf("expected empty reverse of OpLog-only patch, got %+v", rev.Operations) } } @@ -200,6 +268,35 @@ func TestPatchIsEmpty(t *testing.T) { } } +// TestParseJSONPatchGuardOnlyLeading asserts the Guard-extraction heuristic +// only fires on the leading entry. A later {"op":"test","path":"/","if":...} +// must not overwrite Guard or be re-interpreted as a second guard; deep does +// not model standalone test ops, so trailing tests are dropped as unknown. +func TestParseJSONPatchGuardOnlyLeading(t *testing.T) { + raw := []byte(`[ + {"op":"test","path":"/","if":{"op":"more","path":"/age","value":18}}, + {"op":"replace","path":"/name","value":"Alice"}, + {"op":"test","path":"/","if":{"op":"more","path":"/age","value":99}} + ]`) + type Doc struct { + Name string `json:"name"` + Age int `json:"age"` + } + p, err := deep.ParseJSONPatch[Doc](raw) + if err != nil { + t.Fatalf("ParseJSONPatch: %v", err) + } + if p.Guard == nil { + t.Fatal("expected leading test op to be lifted into Guard") + } + // The leading test specified Gt(/age, 18); the trailing one carries + // Gt(/age, 99). If the trailing entry were also lifted into Guard, the + // 99 value would clobber the 18. + if gv, ok := p.Guard.Value.(float64); !ok || gv != 18 { + t.Errorf("Guard value should remain 18 from leading test, got %v", p.Guard.Value) + } +} + func TestParseJSONPatchRoundTrip(t *testing.T) { type Doc struct { Name string `json:"name"` @@ -279,8 +376,8 @@ func TestBuilderMoveCopy(t *testing.T) { if len(p.Operations) != 1 || p.Operations[0].Kind != deep.OpMove { t.Error("Move not added correctly") } - if p.Operations[0].Old != aPath.String() || p.Operations[0].Path != bPath.String() { - t.Errorf("Move paths wrong: from=%v to=%v", p.Operations[0].Old, p.Operations[0].Path) + if p.Operations[0].From != aPath.String() || p.Operations[0].Path != bPath.String() { + t.Errorf("Move paths wrong: from=%v to=%v", p.Operations[0].From, p.Operations[0].Path) } p2 := deep.Edit(&S{}).With(deep.Copy(aPath, bPath)).Build() diff --git a/selector.go b/selector.go index b59fbee..269765f 100644 --- a/selector.go +++ b/selector.go @@ -5,6 +5,8 @@ import ( "reflect" "strings" "sync" + + icore "github.com/brunoga/deep/v5/internal/core" ) // selector is a function that retrieves a field from a struct of type T. @@ -40,8 +42,9 @@ func At[T any, S ~[]E, E any](p Path[T, S], i int) Path[T, E] { } // MapKey returns a type-safe path to the value at key k within a map field. +// Keys are RFC 6901-escaped so values containing '/' or '~' navigate correctly. func MapKey[T any, M ~map[K]V, K comparable, V any](p Path[T, M], k K) Path[T, V] { - return Path[T, V]{path: fmt.Sprintf("%s/%v", p.String(), k)} + return Path[T, V]{path: fmt.Sprintf("%s/%s", p.String(), icore.EscapeKey(fmt.Sprintf("%v", k)))} } // pathCache stores resolved paths keyed by selector function pointer. diff --git a/selector_test.go b/selector_test.go index beb9291..45c8a51 100644 --- a/selector_test.go +++ b/selector_test.go @@ -64,6 +64,46 @@ func TestSelectorNestedPointer(t *testing.T) { } } +// TestMapKeyEscapesJSONPointerSpecials verifies that map keys containing the +// JSON Pointer reserved characters '/' and '~' are RFC 6901-escaped so the +// resulting path navigates to the correct key. +func TestMapKeyEscapesJSONPointerSpecials(t *testing.T) { + type S struct { + M map[string]int `json:"m"` + } + + mPath := deep.Field(func(s *S) *map[string]int { return &s.M }) + + cases := []struct { + key string + wantPath string + }{ + {"a/b", "/m/a~1b"}, + {"c~d", "/m/c~0d"}, + {"~/", "/m/~0~1"}, + {"plain", "/m/plain"}, + } + for _, c := range cases { + got := deep.MapKey[S, map[string]int, string, int](mPath, c.key).String() + if got != c.wantPath { + t.Errorf("MapKey(%q) path = %q, want %q", c.key, got, c.wantPath) + } + } + + // End-to-end: a Set through MapKey on a slash-bearing key must hit the + // right entry. + s := &S{M: map[string]int{"a/b": 0, "other": 0}} + p := deep.Edit(s).With( + deep.Set(deep.MapKey[S, map[string]int, string, int](mPath, "a/b"), 42), + ).Build() + if err := deep.Apply(s, p); err != nil { + t.Fatalf("Apply: %v", err) + } + if s.M["a/b"] != 42 { + t.Errorf("expected M[\"a/b\"] == 42 after Apply, got %v", s.M) + } +} + // TestSelectorCircularType verifies that self-referential struct types do not // cause infinite recursion during path resolution. func TestSelectorCircularType(t *testing.T) {