diff --git a/crdt/crdt.go b/crdt/crdt.go index 36041a9..f275f9f 100644 --- a/crdt/crdt.go +++ b/crdt/crdt.go @@ -24,10 +24,13 @@ package crdt import ( "encoding/json" "log/slog" + "reflect" + "strings" "sync" deep "github.com/brunoga/deep/v5" "github.com/brunoga/deep/v5/crdt/hlc" + icore "github.com/brunoga/deep/v5/internal/core" ) // LWW represents a Last-Write-Wins register for type T. @@ -192,9 +195,42 @@ func (c *CRDT[T]) ApplyDelta(delta Delta[T]) bool { return deep.Apply(&c.value, deep.Patch[T]{Operations: filtered}) == nil } +// textType is the reflect.Type of crdt.Text, used to identify Text fields +// during merge without a full value-tree walk. +var textType = reflect.TypeOf(Text{}) + +// textAncestorPath walks up from opPath toward the root, resolving each prefix +// against root, and returns the path of the nearest ancestor whose value is of +// type Text together with true. It returns ("", false) if the op does not +// belong to a Text field. Walking stops as soon as a valid non-Text ancestor is +// found, keeping traversal to O(depth) Resolve calls per op. +func textAncestorPath(root reflect.Value, opPath string) (string, bool) { + path := opPath + for { + idx := strings.LastIndexByte(path, '/') + if idx <= 0 { + break + } + path = path[:idx] + val, err := icore.DeepPath(path).Resolve(root) + if err != nil || !val.IsValid() { + // Unresolvable prefix (e.g. map key not present locally) — keep walking up. + continue + } + if val.Type() == textType { + return path, true + } + // A valid non-Text ancestor was found at this level, but it might itself + // be nested inside a Text (e.g. a TextRun struct whose parent is Text). + // Keep walking up rather than breaking. + } + return "", false +} + // Merge performs a full state-based merge with another CRDT node. // For each changed field the node with the strictly newer effective timestamp -// (max of write clock and tombstone) wins. +// (max of write clock and tombstone) wins. Text fields are always merged +// convergently via MergeTextRuns, bypassing LWW. func (c *CRDT[T]) Merge(other *CRDT[T]) bool { c.mu.Lock() defer c.mu.Unlock() @@ -206,7 +242,7 @@ func (c *CRDT[T]) Merge(other *CRDT[T]) bool { c.clock.Update(h) } - // Text has its own convergent merge that doesn't rely on per-field clocks. + // Fast path: T itself is a Text. if v, ok := any(c.value).(Text); ok { otherV := any(other.value).(Text) c.value = any(MergeTextRuns(v, otherV)).(T) @@ -220,10 +256,22 @@ func (c *CRDT[T]) Merge(other *CRDT[T]) bool { return false } - // State-based LWW: apply each op only if the remote effective time is - // strictly newer than the local effective time for that path. + localRoot := reflect.ValueOf(&c.value).Elem() + otherRoot := reflect.ValueOf(&other.value).Elem() + + // Separate Text-field ops from LWW-eligible ops. Text is convergent, so + // we collect affected Text paths and apply MergeTextRuns after the LWW + // apply — no full tree walk required. + textPaths := make(map[string]struct{}) var filtered []deep.Operation for _, op := range patch.Operations { + if textPath, ok := textAncestorPath(localRoot, op.Path); ok { + textPaths[textPath] = struct{}{} + continue + } + + // State-based LWW: apply each op only if the remote effective time is + // strictly newer than the local effective time for that path. rClock, hasRC := other.clocks[op.Path] rTomb, hasRT := other.tombstones[op.Path] @@ -260,11 +308,33 @@ func (c *CRDT[T]) Merge(other *CRDT[T]) bool { c.mergeMeta(other) - if len(filtered) == 0 { - return false + changed := len(filtered) > 0 + if changed { + _ = deep.Apply(&c.value, deep.Patch[T]{Operations: filtered}) + // Refresh localRoot: Apply may have updated c.value in place. + localRoot = reflect.ValueOf(&c.value).Elem() } - _ = deep.Apply(&c.value, deep.Patch[T]{Operations: filtered}) - return true + + // Convergently merge each Text field by path. Both values are resolved + // fresh from the (already-updated) local root and the remote root. + for textPath := range textPaths { + localVal, err := icore.DeepPath(textPath).Resolve(localRoot) + if err != nil || !localVal.IsValid() { + continue + } + remoteVal, err := icore.DeepPath(textPath).Resolve(otherRoot) + if err != nil || !remoteVal.IsValid() { + continue + } + merged := MergeTextRuns(localVal.Interface().(Text), remoteVal.Interface().(Text)) + if err := icore.DeepPath(textPath).Set(localRoot, reflect.ValueOf(merged)); err != nil { + slog.Default().Error("crdt: Merge text set failed", "path", textPath, "err", err) + continue + } + changed = true + } + + return changed } func (c *CRDT[T]) mergeMeta(other *CRDT[T]) { diff --git a/crdt/crdt_test.go b/crdt/crdt_test.go index 53dde3b..74303ba 100644 --- a/crdt/crdt_test.go +++ b/crdt/crdt_test.go @@ -2,6 +2,7 @@ package crdt import ( "reflect" + "strings" "testing" "time" ) @@ -98,6 +99,189 @@ func TestCRDT_Conflict(t *testing.T) { } } +// --- Fix: Text fields in structs are merged convergently, not via LWW --- + +type docWithText struct { + Title string + Body Text +} + +// Both nodes concurrently insert into the same Text field. After a bidirectional +// Merge both must hold the same text containing both insertions. +func TestMerge_TextFieldConvergent(t *testing.T) { + nodeA := NewCRDT(docWithText{Title: "doc"}, "node-a") + nodeB := NewCRDT(docWithText{Title: "doc"}, "node-b") + + nodeA.Edit(func(d *docWithText) { + d.Body = d.Body.Insert(0, "hello", nodeA.Clock()) + }) + nodeB.Edit(func(d *docWithText) { + d.Body = d.Body.Insert(0, "world", nodeB.Clock()) + }) + + nodeA.Merge(nodeB) + nodeB.Merge(nodeA) + + viewA := nodeA.View() + viewB := nodeB.View() + + if viewA.Body.String() != viewB.Body.String() { + t.Errorf("Text diverged after merge: A=%q B=%q", viewA.Body.String(), viewB.Body.String()) + } + combined := viewA.Body.String() + if !strings.Contains(combined, "hello") || !strings.Contains(combined, "world") { + t.Errorf("Text merge dropped content: got %q", combined) + } +} + +// Even when nodeA's edit has an older HLC timestamp, its text must still +// appear in the merged result — Text bypasses LWW entirely. +func TestMerge_TextNotLWWFiltered(t *testing.T) { + nodeA := NewCRDT(docWithText{}, "node-a") + nodeB := NewCRDT(docWithText{}, "node-b") + + // nodeA edits first (older wall-clock time) + nodeA.Edit(func(d *docWithText) { + d.Body = d.Body.Insert(0, "older", nodeA.Clock()) + }) + time.Sleep(2 * time.Millisecond) + // nodeB edits later (newer wall-clock time) + nodeB.Edit(func(d *docWithText) { + d.Body = d.Body.Insert(0, "newer", nodeB.Clock()) + }) + + nodeA.Merge(nodeB) + + combined := nodeA.View().Body.String() + if !strings.Contains(combined, "older") || !strings.Contains(combined, "newer") { + t.Errorf("LWW incorrectly filtered Text: got %q, want both 'older' and 'newer'", combined) + } +} + +// Fix: TextRun sub-field ops (e.g. /Body//Deleted) must still be +// detected as Text-field ops and handled via MergeTextRuns, not LWW. +func TestMerge_TextSubFieldOp(t *testing.T) { + nodeA := NewCRDT(docWithText{}, "node-a") + nodeB := NewCRDT(docWithText{}, "node-b") + + // nodeA inserts a run; nodeB gets the same initial state then deletes it. + nodeA.Edit(func(d *docWithText) { + d.Body = d.Body.Insert(0, "shared", nodeA.Clock()) + }) + // Sync nodeB to the same starting state. + nodeB.Merge(nodeA) + + // nodeB soft-deletes the run (produces a /Body//Deleted sub-field op on next diff). + nodeB.Edit(func(d *docWithText) { + d.Body = d.Body.Delete(0, len("shared")) + }) + // nodeA inserts more text concurrently. + nodeA.Edit(func(d *docWithText) { + d.Body = d.Body.Insert(len("shared"), " appended", nodeA.Clock()) + }) + + nodeA.Merge(nodeB) + nodeB.Merge(nodeA) + + viewA := nodeA.View() + viewB := nodeB.View() + + if viewA.Body.String() != viewB.Body.String() { + t.Errorf("Sub-field op: Text diverged: A=%q B=%q", viewA.Body.String(), viewB.Body.String()) + } +} + +// Fix: Text field nested inside a nested struct must be detected and merged. +func TestMerge_TextInNestedStruct(t *testing.T) { + type inner struct{ Notes Text } + type outer struct{ Meta inner } + + nodeA := NewCRDT(outer{}, "node-a") + nodeB := NewCRDT(outer{}, "node-b") + + nodeA.Edit(func(o *outer) { + o.Meta.Notes = o.Meta.Notes.Insert(0, "note-a", nodeA.Clock()) + }) + nodeB.Edit(func(o *outer) { + o.Meta.Notes = o.Meta.Notes.Insert(0, "note-b", nodeB.Clock()) + }) + + nodeA.Merge(nodeB) + nodeB.Merge(nodeA) + + viewA := nodeA.View() + viewB := nodeB.View() + + if viewA.Meta.Notes.String() != viewB.Meta.Notes.String() { + t.Errorf("Nested Text diverged: A=%q B=%q", + viewA.Meta.Notes.String(), viewB.Meta.Notes.String()) + } + combined := viewA.Meta.Notes.String() + if !strings.Contains(combined, "note-a") || !strings.Contains(combined, "note-b") { + t.Errorf("Nested Text merge dropped content: got %q", combined) + } +} + +// Fix: Text field inside a keyed-slice element must be detected by key, not +// by positional index. The old tree-walk would misalign elements if the two +// nodes had different slice orderings. +func TestMerge_TextInKeyedSliceElement(t *testing.T) { + type section struct { + ID int `deep:"key"` + Content Text + } + type article struct{ Sections []section } + + initial := article{Sections: []section{ + {ID: 1}, + {ID: 2}, + }} + + nodeA := NewCRDT(initial, "node-a") + nodeB := NewCRDT(initial, "node-b") + + // Both nodes write to section 2's Content concurrently. + nodeA.Edit(func(a *article) { + a.Sections[1].Content = a.Sections[1].Content.Insert(0, "from-a", nodeA.Clock()) + }) + nodeB.Edit(func(a *article) { + a.Sections[1].Content = a.Sections[1].Content.Insert(0, "from-b", nodeB.Clock()) + }) + // NodeA also prepends a new section, shifting positional indices. + nodeA.Edit(func(a *article) { + a.Sections = append([]section{{ID: 0}}, a.Sections...) + }) + + nodeA.Merge(nodeB) + nodeB.Merge(nodeA) + + findSection := func(secs []section, id int) *section { + for i := range secs { + if secs[i].ID == id { + return &secs[i] + } + } + return nil + } + + viewA := nodeA.View() + viewB := nodeB.View() + + s2A := findSection(viewA.Sections, 2) + s2B := findSection(viewB.Sections, 2) + if s2A == nil || s2B == nil { + t.Fatal("section 2 missing after merge") + } + if s2A.Content.String() != s2B.Content.String() { + t.Errorf("Section 2 Text diverged: A=%q B=%q", + s2A.Content.String(), s2B.Content.String()) + } + combined := s2A.Content.String() + if !strings.Contains(combined, "from-a") || !strings.Contains(combined, "from-b") { + t.Errorf("Section 2 Text missing content: got %q", combined) + } +} + func TestCRDT_JSON(t *testing.T) { node := NewCRDT(TestUser{ID: 1, Name: "Initial"}, "node1") node.Edit(func(u *TestUser) { diff --git a/internal/core/path.go b/internal/core/path.go index 4e5f1f1..0580573 100644 --- a/internal/core/path.go +++ b/internal/core/path.go @@ -68,20 +68,34 @@ func (p DeepPath) Navigate(v reflect.Value, parts []PathPart) (reflect.Value, Pa return reflect.Value{}, PathPart{}, fmt.Errorf("path traversal failed: nil value at intermediate step") } - if part.IsIndex && (current.Kind() == reflect.Slice || current.Kind() == reflect.Array) { - // Check whether the element type uses a keyed-collection tag. - // If so, treat the numeric segment as a key value, not an array index. - if keyIdx, found := sliceKeyField(current.Type()); found { - keyStr := part.Key - if keyStr == "" { - keyStr = strconv.Itoa(part.Index) + if current.Kind() == reflect.Slice || current.Kind() == reflect.Array { + if current.Kind() == reflect.Slice { + // Check for keyed-collection tag first, regardless of whether the + // path segment is numeric. Keys like "todo" or "in-progress" are + // non-numeric but still valid keyed-slice selectors. + if keyIdx, found := sliceKeyField(current.Type()); found { + keyStr := part.Key + if keyStr == "" && part.IsIndex { + keyStr = strconv.Itoa(part.Index) + } + elem, ok := findSliceElemByKey(current, keyIdx, keyStr) + if !ok { + return reflect.Value{}, PathPart{}, fmt.Errorf("element with key %s not found", keyStr) + } + current = elem + } else if part.IsIndex { + if part.Index < 0 || part.Index >= current.Len() { + return reflect.Value{}, PathPart{}, fmt.Errorf("index out of bounds: %d", part.Index) + } + current = current.Index(part.Index) + } else { + return reflect.Value{}, PathPart{}, fmt.Errorf("non-numeric index %q for non-keyed slice", part.Key) } - elem, ok := findSliceElemByKey(current, keyIdx, keyStr) - if !ok { - return reflect.Value{}, PathPart{}, fmt.Errorf("element with key %s not found", keyStr) - } - current = elem } else { + // Array: always numeric. + if !part.IsIndex { + return reflect.Value{}, PathPart{}, fmt.Errorf("non-numeric index %q for array", part.Key) + } if part.Index < 0 || part.Index >= current.Len() { return reflect.Value{}, PathPart{}, fmt.Errorf("index out of bounds: %d", part.Index) } @@ -150,179 +164,289 @@ func (p DeepPath) Navigate(v reflect.Value, parts []PathPart) (reflect.Value, Pa } func (p DeepPath) Set(v reflect.Value, val reflect.Value) error { - parent, lastPart, err := p.ResolveParent(v) - if err != nil { - if string(p) == "" || string(p) == "/" { - if !v.CanSet() { - return fmt.Errorf("cannot set root value") - } - v.Set(val) - return nil + if string(p) == "" || string(p) == "/" { + if !v.CanSet() { + return fmt.Errorf("cannot set root value") } + SetValue(v, val) + return nil + } + return setAtPath(v, ParsePath(string(p)), val) +} + +// setAtPath recursively walks parts and sets val at the target location. +// It handles map boundaries with copy-modify-put-back so that values nested +// inside maps remain correct even though map elements are not addressable. +func setAtPath(v reflect.Value, parts []PathPart, val reflect.Value) error { + v, err := Dereference(v) + if err != nil { return err } - switch parent.Kind() { + if len(parts) == 0 { + if !v.CanSet() { + unsafe.DisableRO(&v) + } + SetValue(v, val) + return nil + } + + part := parts[0] + rest := parts[1:] + + switch v.Kind() { case reflect.Map: - keyType := parent.Type().Key() - var keyVal reflect.Value - key := lastPart.Key - if key == "" && lastPart.IsIndex { - key = strconv.Itoa(lastPart.Index) - } - if keyType.Kind() == reflect.String { - keyVal = reflect.ValueOf(key) - } else if keyType.Kind() == reflect.Int { - i, err := strconv.Atoi(key) - if err != nil { - return fmt.Errorf("invalid int key: %s", key) - } - keyVal = reflect.ValueOf(i) + keyVal, err := makeMapKey(v.Type().Key(), part) + if err != nil { + return err + } + if len(rest) == 0 { + v.SetMapIndex(keyVal, ConvertValue(val, v.Type().Elem())) + return nil + } + // Deeper path: copy the map element, recurse, put it back. + elem := v.MapIndex(keyVal) + if !elem.IsValid() { + return fmt.Errorf("map key %v not found", part.Key) + } + newElem := reflect.New(elem.Type()).Elem() + newElem.Set(elem) + if err := setAtPath(newElem, rest, val); err != nil { + return err } - parent.SetMapIndex(keyVal, ConvertValue(val, parent.Type().Elem())) + v.SetMapIndex(keyVal, newElem) return nil + case reflect.Slice: - // For keyed slices, find by key and update or append; for plain slices, use positional index. - if keyIdx, found := sliceKeyField(parent.Type()); found { - key := lastPart.Key - if key == "" && lastPart.IsIndex { - key = strconv.Itoa(lastPart.Index) + if keyIdx, found := sliceKeyField(v.Type()); found { + // Keyed slice: key lookup works for both numeric and non-numeric keys. + keyStr := part.Key + if keyStr == "" && part.IsIndex { + keyStr = strconv.Itoa(part.Index) } - converted := ConvertValue(val, parent.Type().Elem()) - for i := 0; i < parent.Len(); i++ { - elem := parent.Index(i) - if keyFieldStr(elem, keyIdx) == key { - elem.Set(converted) - return nil + converted := ConvertValue(val, v.Type().Elem()) + if len(rest) == 0 { + for i := 0; i < v.Len(); i++ { + if keyFieldStr(v.Index(i), keyIdx) == keyStr { + v.Index(i).Set(converted) + return nil + } + } + // Key not found: append. + if !v.CanSet() { + return fmt.Errorf("cannot append to non-settable keyed slice at key %s", keyStr) } + v.Set(reflect.Append(v, converted)) + return nil } - // Key not found: append the new element. - parent.Set(reflect.Append(parent, converted)) - return nil + // Deeper: recurse into the keyed element (slice elements are addressable). + for i := 0; i < v.Len(); i++ { + if keyFieldStr(v.Index(i), keyIdx) == keyStr { + return setAtPath(v.Index(i), rest, val) + } + } + return fmt.Errorf("element with key %s not found", keyStr) } - idx := lastPart.Index - if !lastPart.IsIndex { - var err error - idx, err = strconv.Atoi(lastPart.Key) + // Plain slice: positional index. + idx := part.Index + if !part.IsIndex { + idx, err = strconv.Atoi(part.Key) if err != nil { - return fmt.Errorf("invalid slice index: %s", lastPart.Key) + return fmt.Errorf("invalid slice index: %s", part.Key) } } - if idx < 0 || idx > parent.Len() { + if idx < 0 || idx > v.Len() { return fmt.Errorf("index out of bounds: %d", idx) } - if idx == parent.Len() { - parent.Set(reflect.Append(parent, ConvertValue(val, parent.Type().Elem()))) - } else { - parent.Index(idx).Set(ConvertValue(val, parent.Type().Elem())) + if len(rest) == 0 { + if idx == v.Len() { + if !v.CanSet() { + return fmt.Errorf("cannot append to non-settable slice at index %d", idx) + } + v.Set(reflect.Append(v, ConvertValue(val, v.Type().Elem()))) + } else { + v.Index(idx).Set(ConvertValue(val, v.Type().Elem())) + } + return nil } - return nil + if idx >= v.Len() { + return fmt.Errorf("index out of bounds: %d", idx) + } + return setAtPath(v.Index(idx), rest, val) + case reflect.Struct: - key := lastPart.Key - if key == "" && lastPart.IsIndex { - key = strconv.Itoa(lastPart.Index) + key := part.Key + if key == "" && part.IsIndex { + key = strconv.Itoa(part.Index) } - info := GetTypeInfo(parent.Type()) - var fieldIdx = -1 + info := GetTypeInfo(v.Type()) for _, fInfo := range info.Fields { if fInfo.Name == key || (fInfo.JSONTag != "" && fInfo.JSONTag == key) { - fieldIdx = fInfo.Index - break + f := v.Field(fInfo.Index) + if !f.CanInterface() { + unsafe.DisableRO(&f) + } + if len(rest) == 0 { + if !f.CanSet() { + unsafe.DisableRO(&f) + } + f.Set(ConvertValue(val, f.Type())) + return nil + } + return setAtPath(f, rest, val) } } - if fieldIdx == -1 { - return fmt.Errorf("field %s not found", key) + return fmt.Errorf("field %s not found", key) + + default: + return fmt.Errorf("cannot navigate into %v", v.Kind()) + } +} + +// makeMapKey converts a PathPart into a reflect.Value suitable as a map key. +func makeMapKey(keyType reflect.Type, part PathPart) (reflect.Value, error) { + key := part.Key + if key == "" && part.IsIndex { + key = strconv.Itoa(part.Index) + } + switch keyType.Kind() { + case reflect.String: + return reflect.ValueOf(key).Convert(keyType), nil + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + i, err := strconv.ParseInt(key, 10, 64) + if err != nil { + return reflect.Value{}, fmt.Errorf("invalid int key: %s", key) } - f := parent.Field(fieldIdx) - if !f.CanSet() { - unsafe.DisableRO(&f) + return reflect.ValueOf(i).Convert(keyType), nil + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + u, err := strconv.ParseUint(key, 10, 64) + if err != nil { + return reflect.Value{}, fmt.Errorf("invalid uint key: %s", key) } - f.Set(ConvertValue(val, f.Type())) - return nil + return reflect.ValueOf(u).Convert(keyType), nil default: - return fmt.Errorf("cannot set value in %v", parent.Kind()) + return reflect.Value{}, fmt.Errorf("unsupported map key type for path: %v", keyType) } } func (p DeepPath) Delete(v reflect.Value) error { - parent, lastPart, err := p.ResolveParent(v) + return deleteAtPath(v, ParsePath(string(p))) +} + +// deleteAtPath recursively walks parts and removes the value at the target location. +// Like setAtPath it uses copy-modify-put-back at map boundaries so that values +// nested inside maps can be deleted without hitting addressability panics. +func deleteAtPath(v reflect.Value, parts []PathPart) error { + v, err := Dereference(v) if err != nil { return err } - switch parent.Kind() { + if len(parts) == 0 { + return fmt.Errorf("cannot delete: empty path") + } + + part := parts[0] + rest := parts[1:] + + switch v.Kind() { case reflect.Map: - keyType := parent.Type().Key() - var keyVal reflect.Value - key := lastPart.Key - if key == "" && lastPart.IsIndex { - key = strconv.Itoa(lastPart.Index) - } - if keyType.Kind() == reflect.String { - keyVal = reflect.ValueOf(key) - } else if keyType.Kind() == reflect.Int { - i, err := strconv.Atoi(key) - if err != nil { - return fmt.Errorf("invalid int key: %s", key) - } - keyVal = reflect.ValueOf(i) + keyVal, err := makeMapKey(v.Type().Key(), part) + if err != nil { + return err } - parent.SetMapIndex(keyVal, reflect.Value{}) + if len(rest) == 0 { + v.SetMapIndex(keyVal, reflect.Value{}) + return nil + } + // Deeper: copy-modify-put-back. + elem := v.MapIndex(keyVal) + if !elem.IsValid() { + return fmt.Errorf("map key %v not found", part.Key) + } + newElem := reflect.New(elem.Type()).Elem() + newElem.Set(elem) + if err := deleteAtPath(newElem, rest); err != nil { + return err + } + v.SetMapIndex(keyVal, newElem) return nil + case reflect.Slice: - // For keyed slices, find by key and remove; for plain slices, use positional index. - if keyIdx, found := sliceKeyField(parent.Type()); found { - key := lastPart.Key - if key == "" && lastPart.IsIndex { - key = strconv.Itoa(lastPart.Index) + if keyIdx, found := sliceKeyField(v.Type()); found { + // Keyed slice. + keyStr := part.Key + if keyStr == "" && part.IsIndex { + keyStr = strconv.Itoa(part.Index) } - for i := 0; i < parent.Len(); i++ { - if keyFieldStr(parent.Index(i), keyIdx) == key { - newSlice := reflect.AppendSlice(parent.Slice(0, i), parent.Slice(i+1, parent.Len())) - parent.Set(newSlice) - return nil + if len(rest) == 0 { + for i := 0; i < v.Len(); i++ { + if keyFieldStr(v.Index(i), keyIdx) == keyStr { + newSlice := reflect.AppendSlice(v.Slice(0, i), v.Slice(i+1, v.Len())) + if !v.CanSet() { + return fmt.Errorf("cannot delete from non-settable keyed slice at key %s", keyStr) + } + v.Set(newSlice) + return nil + } } + return fmt.Errorf("element with key %s not found", keyStr) } - return fmt.Errorf("element with key %s not found", key) + // Deeper: recurse into element (slice elements are addressable). + for i := 0; i < v.Len(); i++ { + if keyFieldStr(v.Index(i), keyIdx) == keyStr { + return deleteAtPath(v.Index(i), rest) + } + } + return fmt.Errorf("element with key %s not found", keyStr) } - idx := lastPart.Index - if !lastPart.IsIndex { - var err error - idx, err = strconv.Atoi(lastPart.Key) + // Plain slice: positional index. + idx := part.Index + if !part.IsIndex { + idx, err = strconv.Atoi(part.Key) if err != nil { - return fmt.Errorf("invalid slice index: %s", lastPart.Key) + return fmt.Errorf("invalid slice index: %s", part.Key) } } - if idx < 0 || idx >= parent.Len() { + if idx < 0 || idx >= v.Len() { return fmt.Errorf("index out of bounds: %d", idx) } - newSlice := reflect.AppendSlice(parent.Slice(0, idx), parent.Slice(idx+1, parent.Len())) - parent.Set(newSlice) - return nil + if len(rest) == 0 { + newSlice := reflect.AppendSlice(v.Slice(0, idx), v.Slice(idx+1, v.Len())) + if !v.CanSet() { + return fmt.Errorf("cannot delete from non-settable slice at index %d", idx) + } + v.Set(newSlice) + return nil + } + return deleteAtPath(v.Index(idx), rest) + case reflect.Struct: - key := lastPart.Key - if key == "" && lastPart.IsIndex { - key = strconv.Itoa(lastPart.Index) + key := part.Key + if key == "" && part.IsIndex { + key = strconv.Itoa(part.Index) } - info := GetTypeInfo(parent.Type()) - var fieldIdx = -1 + info := GetTypeInfo(v.Type()) for _, fInfo := range info.Fields { if fInfo.Name == key || (fInfo.JSONTag != "" && fInfo.JSONTag == key) { - fieldIdx = fInfo.Index - break + f := v.Field(fInfo.Index) + if !f.CanInterface() { + unsafe.DisableRO(&f) + } + if len(rest) == 0 { + if !f.CanSet() { + unsafe.DisableRO(&f) + } + f.Set(reflect.Zero(f.Type())) + return nil + } + return deleteAtPath(f, rest) } } - if fieldIdx == -1 { - return fmt.Errorf("field %s not found", key) - } - f := parent.Field(fieldIdx) - if !f.CanSet() { - unsafe.DisableRO(&f) - } - f.Set(reflect.Zero(f.Type())) - return nil + return fmt.Errorf("field %s not found", key) + default: - return fmt.Errorf("cannot delete from %v", parent.Kind()) + return fmt.Errorf("cannot delete from %v", v.Kind()) } } diff --git a/internal/core/path_test.go b/internal/core/path_test.go index 1cac008..82027f9 100644 --- a/internal/core/path_test.go +++ b/internal/core/path_test.go @@ -5,6 +5,113 @@ import ( "testing" ) +// --- Fix: Navigate handles non-numeric keys in keyed slices --- + +type keyedItem struct { + Name string `deep:"key"` + Value int +} + +func TestNavigate_KeyedSlice_NonNumericKey(t *testing.T) { + items := []keyedItem{ + {Name: "todo", Value: 1}, + {Name: "in-progress", Value: 2}, + {Name: "done", Value: 3}, + } + v := reflect.ValueOf(items) + + for _, tc := range []struct { + key string + want int + }{ + {"todo", 1}, + {"in-progress", 2}, + {"done", 3}, + } { + val, err := DeepPath("/" + tc.key).Resolve(v) + if err != nil { + t.Errorf("Resolve /%s: unexpected error: %v", tc.key, err) + continue + } + got := int(val.FieldByName("Value").Int()) + if got != tc.want { + t.Errorf("Resolve /%s: got Value=%d, want %d", tc.key, got, tc.want) + } + } +} + +// --- Fix: Set/Delete on values nested inside maps --- + +type mapInner struct{ X int } +type mapOuter struct{ M map[string]mapInner } + +func TestSet_MapNestedValue(t *testing.T) { + o := mapOuter{M: map[string]mapInner{"k": {X: 1}}} + v := reflect.ValueOf(&o).Elem() + + if err := DeepPath("/M/k/X").Set(v, reflect.ValueOf(42)); err != nil { + t.Fatalf("Set /M/k/X: %v", err) + } + if got := o.M["k"].X; got != 42 { + t.Errorf("after Set /M/k/X: got %d, want 42", got) + } +} + +func TestDelete_MapNestedValue(t *testing.T) { + o := mapOuter{M: map[string]mapInner{"k": {X: 99}}} + v := reflect.ValueOf(&o).Elem() + + if err := DeepPath("/M/k/X").Delete(v); err != nil { + t.Fatalf("Delete /M/k/X: %v", err) + } + if got := o.M["k"].X; got != 0 { + t.Errorf("after Delete /M/k/X: got %d, want 0 (zero value)", got) + } +} + +func TestSet_MapNestedValue_DeepPath(t *testing.T) { + // Three levels deep: struct → map → struct → field. + type level2 struct{ Z string } + type level1 struct{ Inner map[string]level2 } + type root struct{ Outer level1 } + + r := root{Outer: level1{Inner: map[string]level2{"key": {Z: "old"}}}} + v := reflect.ValueOf(&r).Elem() + + if err := DeepPath("/Outer/Inner/key/Z").Set(v, reflect.ValueOf("new")); err != nil { + t.Fatalf("Set deep map path: %v", err) + } + if got := r.Outer.Inner["key"].Z; got != "new" { + t.Errorf("got %q, want %q", got, "new") + } +} + +// --- Fix: makeMapKey handles uint map key types --- + +func TestSet_UintMapKey(t *testing.T) { + m := map[uint]string{1: "one"} + v := reflect.ValueOf(&m).Elem() + + if err := DeepPath("/2").Set(v, reflect.ValueOf("two")); err != nil { + t.Fatalf("Set uint map key: %v", err) + } + if got := m[2]; got != "two" { + t.Errorf("got %q, want %q", got, "two") + } +} + +func TestDelete_UintMapKey(t *testing.T) { + m := map[uint]string{7: "seven"} + v := reflect.ValueOf(&m).Elem() + + if err := DeepPath("/7").Delete(v); err != nil { + t.Fatalf("Delete uint map key: %v", err) + } + if _, ok := m[7]; ok { + t.Error("key 7 still present after Delete") + } +} + func TestDeepPath_Resolve(t *testing.T) { type S struct { A int