Skip to content

Commit ee23a08

Browse files
subomiclaude
subomi
and
claude
committed
fix: correct array sync to preserve identity matching during subset operations
Fixed a bug in syncArraySlice where the target array was prematurely truncated by position before reorderArrayElements could match elements by RootNode identity. This caused elements at higher indices to be discarded even when they should have been matched and retained. The fix defers array resizing until after the sync loop completes, allowing reorderArrayElements to search all target elements for identity matches first. Added dereferenceToLastPtr utility for cleaner pointer chain handling. Added TestSync_ArraySubset_Debug to verify syncing a smaller high-level model (3 items) to a larger pre-existing core model (6 items) correctly matches by identity, removes unmatched items, and preserves the correct order. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 4215825 commit ee23a08

4 files changed

Lines changed: 165 additions & 24 deletions

File tree

‎marshaller/syncer.go‎

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -306,30 +306,6 @@ func syncArraySlice(ctx context.Context, source any, target any, valueNode *yaml
306306
targetVal.Set(reflect.MakeSlice(targetVal.Type(), 0, 0))
307307
}
308308

309-
if targetVal.Len() > sourceVal.Len() {
310-
// Shorten the slice
311-
tempVal := reflect.MakeSlice(targetVal.Type(), sourceVal.Len(), sourceVal.Len())
312-
for i := 0; i < sourceVal.Len(); i++ {
313-
tempVal.Index(i).Set(targetVal.Index(i))
314-
}
315-
316-
targetVal.Set(tempVal)
317-
}
318-
319-
if targetVal.Len() < sourceVal.Len() {
320-
// Equalize the slice
321-
tempVal := reflect.MakeSlice(targetVal.Type(), sourceVal.Len(), sourceVal.Len())
322-
323-
for i := 0; i < targetVal.Len(); i++ {
324-
tempVal.Index(i).Set(targetVal.Index(i))
325-
}
326-
for i := targetVal.Len(); i < sourceVal.Len(); i++ {
327-
tempVal.Index(i).Set(reflect.Zero(targetVal.Type().Elem()))
328-
}
329-
330-
targetVal.Set(tempVal)
331-
}
332-
333309
// When arrays are reordered at the high level (e.g., moving workflows around),
334310
// we need to match source elements with their corresponding target core models
335311
// by identity (RootNode) rather than by array position to preserve elements.
@@ -363,6 +339,24 @@ func syncArraySlice(ctx context.Context, source any, target any, valueNode *yaml
363339
elements[i] = currentElementNode
364340
}
365341

342+
// Update targetVal to contain only the synced elements in the correct order
343+
// This ensures the target slice reflects the reordering and removals
344+
tempVal := reflect.MakeSlice(targetVal.Type(), sourceVal.Len(), sourceVal.Len())
345+
for i := 0; i < sourceVal.Len(); i++ {
346+
// reorderedTargets[i] contains pointers from .Addr().Interface()
347+
// Use dereferenceToLastPtr to handle chains of pointers (e.g., **T -> *T)
348+
targetPtr := dereferenceToLastPtr(reflect.ValueOf(reorderedTargets[i]))
349+
350+
if targetVal.Type().Elem().Kind() == reflect.Ptr {
351+
// Target slice expects pointers (*T), set the pointer directly
352+
tempVal.Index(i).Set(targetPtr)
353+
} else {
354+
// Target slice expects values (T), dereference the pointer
355+
tempVal.Index(i).Set(targetPtr.Elem())
356+
}
357+
}
358+
targetVal.Set(tempVal)
359+
366360
return yml.CreateOrUpdateSliceNode(ctx, elements, valueNode), nil
367361
}
368362

‎marshaller/syncing_test.go‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,3 +1143,109 @@ func TestSync_EmbeddedMapComparison_PointerVsValue_Success(t *testing.T) {
11431143
require.Equal(t, "shared_key: shared_value\n", string(ptrYAML))
11441144
})
11451145
}
1146+
1147+
func TestSync_ArraySubset_Debug(t *testing.T) {
1148+
t.Parallel()
1149+
1150+
// Create items for the high-level model (3 items - subset)
1151+
// Source: items one, four, and six
1152+
itemOne := &tests.TestItemHighModel{
1153+
Name: "one",
1154+
Description: "First item",
1155+
}
1156+
itemFour := &tests.TestItemHighModel{
1157+
Name: "four",
1158+
Description: "Fourth item",
1159+
}
1160+
itemSix := &tests.TestItemHighModel{
1161+
Name: "six",
1162+
Description: "Sixth item",
1163+
}
1164+
1165+
highModel := tests.TestArrayOfObjectsHighModel{
1166+
Items: []*tests.TestItemHighModel{itemOne, itemFour, itemSix},
1167+
}
1168+
1169+
// Set the root node for the high-level model by creating a YAML node
1170+
// This simulates the case where we have an existing YAML document with 6 items
1171+
// Target: items three, five, one, four, second, and six (in this order)
1172+
initialYAML := `items:
1173+
- name: three
1174+
description: Third item
1175+
- name: five
1176+
description: Fifth item
1177+
- name: one
1178+
description: First item
1179+
- name: four
1180+
description: Fourth item
1181+
- name: second
1182+
description: Second item
1183+
- name: six
1184+
description: Sixth item
1185+
`
1186+
var rootNode yaml.Node
1187+
err := yaml.Unmarshal([]byte(initialYAML), &rootNode)
1188+
require.NoError(t, err)
1189+
1190+
// Get the core model
1191+
coreModel := highModel.GetCore()
1192+
1193+
// Set the root node on the core model (accessed through GetCore())
1194+
coreModel.SetRootNode(rootNode.Content[0]) // Content[0] is the actual root mapping node
1195+
1196+
// Unmarshal the YAML into the core model to populate it with the 6 items
1197+
_, err = marshaller.UnmarshalModel(t.Context(), rootNode.Content[0], coreModel)
1198+
require.NoError(t, err)
1199+
1200+
// Use SetCore to link each high-level item to its corresponding core item
1201+
// This establishes the connection between high-level items and their core counterparts
1202+
for _, item := range coreModel.Items.Value {
1203+
switch item.Name.Value {
1204+
case "one":
1205+
itemOne.SetCore(item)
1206+
case "four":
1207+
itemFour.SetCore(item)
1208+
case "six":
1209+
itemSix.SetCore(item)
1210+
}
1211+
}
1212+
1213+
// Sync the high model (3 items) to the core model (currently 6 items)
1214+
// This tests what happens when syncing a subset
1215+
resultNode, err := marshaller.SyncValue(t.Context(), &highModel, highModel.GetCore(), highModel.GetRootNode(), false)
1216+
require.NoError(t, err)
1217+
require.NotNil(t, resultNode)
1218+
1219+
// Verify the synced array - should now match the high model's subset (3 items)
1220+
// After sync, items two, three, five, and second should be removed
1221+
items := coreModel.Items.Value
1222+
require.Len(t, items, 3, "After sync, core model should have 3 items matching high model")
1223+
1224+
// Debug: Print what we actually got
1225+
t.Logf("Item 0: %s - %s", items[0].Name.Value, items[0].Description.Value)
1226+
t.Logf("Item 1: %s - %s", items[1].Name.Value, items[1].Description.Value)
1227+
t.Logf("Item 2: %s - %s", items[2].Name.Value, items[2].Description.Value)
1228+
1229+
require.Equal(t, "one", items[0].Name.Value)
1230+
require.Equal(t, "First item", items[0].Description.Value)
1231+
1232+
require.Equal(t, "four", items[1].Name.Value)
1233+
require.Equal(t, "Fourth item", items[1].Description.Value)
1234+
1235+
require.Equal(t, "six", items[2].Name.Value)
1236+
require.Equal(t, "Sixth item", items[2].Description.Value)
1237+
1238+
// Verify the core model's RootNode contains the correct YAML
1239+
expectedYAML := `items:
1240+
- name: one
1241+
description: First item
1242+
- name: four
1243+
description: Fourth item
1244+
- name: six
1245+
description: Sixth item
1246+
`
1247+
1248+
actualYAML, err := yaml.Marshal(coreModel.GetRootNode())
1249+
require.NoError(t, err)
1250+
require.Equal(t, expectedYAML, string(actualYAML))
1251+
}

‎marshaller/tests/core/models.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,3 +183,25 @@ type TestTypeConversionCoreModel struct {
183183
HTTPMethodField marshaller.Node[*string] `key:"httpMethodField"`
184184
Extensions core.Extensions `key:"extensions"`
185185
}
186+
187+
// TestSimpleArrayModel is a minimal model with only an array field for testing array sync behavior
188+
type TestSimpleArrayModel struct {
189+
marshaller.CoreModel `model:"testSimpleArrayModel"`
190+
191+
ArrayField marshaller.Node[[]string] `key:"arrayField"`
192+
}
193+
194+
// TestItemModel represents a simple item with name and description
195+
type TestItemModel struct {
196+
marshaller.CoreModel `model:"testItemModel"`
197+
198+
Name marshaller.Node[string] `key:"name"`
199+
Description marshaller.Node[string] `key:"description"`
200+
}
201+
202+
// TestArrayOfObjectsModel is a minimal model with an array of objects for testing array sync behavior
203+
type TestArrayOfObjectsModel struct {
204+
marshaller.CoreModel `model:"testArrayOfObjectsModel"`
205+
206+
Items marshaller.Node[[]*TestItemModel] `key:"items"`
207+
}

‎marshaller/tests/models.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,3 +128,22 @@ type TestEmbeddedMapWithFieldsPointerHighModel struct {
128128
NameField string
129129
Extensions *extensions.Extensions
130130
}
131+
132+
// TestSimpleArrayHighModel is a minimal high-level model with only an array field for testing array sync behavior
133+
type TestSimpleArrayHighModel struct {
134+
marshaller.Model[core.TestSimpleArrayModel]
135+
ArrayField []string
136+
}
137+
138+
// TestItemHighModel represents a simple item with name and description
139+
type TestItemHighModel struct {
140+
marshaller.Model[core.TestItemModel]
141+
Name string
142+
Description string
143+
}
144+
145+
// TestArrayOfObjectsHighModel is a minimal high-level model with an array of objects for testing array sync behavior
146+
type TestArrayOfObjectsHighModel struct {
147+
marshaller.Model[core.TestArrayOfObjectsModel]
148+
Items []*TestItemHighModel
149+
}

0 commit comments

Comments
 (0)