From ee23a087d8f6d4c5b13525df4a0373fc58adb893 Mon Sep 17 00:00:00 2001 From: subomi Date: Mon, 17 Nov 2025 18:41:21 +0000 Subject: [PATCH 1/2] fix: correct array sync to preserve identity matching during subset operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- marshaller/syncer.go | 42 ++++++------- marshaller/syncing_test.go | 106 ++++++++++++++++++++++++++++++++ marshaller/tests/core/models.go | 22 +++++++ marshaller/tests/models.go | 19 ++++++ 4 files changed, 165 insertions(+), 24 deletions(-) diff --git a/marshaller/syncer.go b/marshaller/syncer.go index adfda1c..a9b622f 100644 --- a/marshaller/syncer.go +++ b/marshaller/syncer.go @@ -306,30 +306,6 @@ func syncArraySlice(ctx context.Context, source any, target any, valueNode *yaml targetVal.Set(reflect.MakeSlice(targetVal.Type(), 0, 0)) } - if targetVal.Len() > sourceVal.Len() { - // Shorten the slice - tempVal := reflect.MakeSlice(targetVal.Type(), sourceVal.Len(), sourceVal.Len()) - for i := 0; i < sourceVal.Len(); i++ { - tempVal.Index(i).Set(targetVal.Index(i)) - } - - targetVal.Set(tempVal) - } - - if targetVal.Len() < sourceVal.Len() { - // Equalize the slice - tempVal := reflect.MakeSlice(targetVal.Type(), sourceVal.Len(), sourceVal.Len()) - - for i := 0; i < targetVal.Len(); i++ { - tempVal.Index(i).Set(targetVal.Index(i)) - } - for i := targetVal.Len(); i < sourceVal.Len(); i++ { - tempVal.Index(i).Set(reflect.Zero(targetVal.Type().Elem())) - } - - targetVal.Set(tempVal) - } - // When arrays are reordered at the high level (e.g., moving workflows around), // we need to match source elements with their corresponding target core models // 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 elements[i] = currentElementNode } + // Update targetVal to contain only the synced elements in the correct order + // This ensures the target slice reflects the reordering and removals + tempVal := reflect.MakeSlice(targetVal.Type(), sourceVal.Len(), sourceVal.Len()) + for i := 0; i < sourceVal.Len(); i++ { + // reorderedTargets[i] contains pointers from .Addr().Interface() + // Use dereferenceToLastPtr to handle chains of pointers (e.g., **T -> *T) + targetPtr := dereferenceToLastPtr(reflect.ValueOf(reorderedTargets[i])) + + if targetVal.Type().Elem().Kind() == reflect.Ptr { + // Target slice expects pointers (*T), set the pointer directly + tempVal.Index(i).Set(targetPtr) + } else { + // Target slice expects values (T), dereference the pointer + tempVal.Index(i).Set(targetPtr.Elem()) + } + } + targetVal.Set(tempVal) + return yml.CreateOrUpdateSliceNode(ctx, elements, valueNode), nil } diff --git a/marshaller/syncing_test.go b/marshaller/syncing_test.go index aec2c99..5abc014 100644 --- a/marshaller/syncing_test.go +++ b/marshaller/syncing_test.go @@ -1143,3 +1143,109 @@ func TestSync_EmbeddedMapComparison_PointerVsValue_Success(t *testing.T) { require.Equal(t, "shared_key: shared_value\n", string(ptrYAML)) }) } + +func TestSync_ArraySubset_Debug(t *testing.T) { + t.Parallel() + + // Create items for the high-level model (3 items - subset) + // Source: items one, four, and six + itemOne := &tests.TestItemHighModel{ + Name: "one", + Description: "First item", + } + itemFour := &tests.TestItemHighModel{ + Name: "four", + Description: "Fourth item", + } + itemSix := &tests.TestItemHighModel{ + Name: "six", + Description: "Sixth item", + } + + highModel := tests.TestArrayOfObjectsHighModel{ + Items: []*tests.TestItemHighModel{itemOne, itemFour, itemSix}, + } + + // Set the root node for the high-level model by creating a YAML node + // This simulates the case where we have an existing YAML document with 6 items + // Target: items three, five, one, four, second, and six (in this order) + initialYAML := `items: + - name: three + description: Third item + - name: five + description: Fifth item + - name: one + description: First item + - name: four + description: Fourth item + - name: second + description: Second item + - name: six + description: Sixth item +` + var rootNode yaml.Node + err := yaml.Unmarshal([]byte(initialYAML), &rootNode) + require.NoError(t, err) + + // Get the core model + coreModel := highModel.GetCore() + + // Set the root node on the core model (accessed through GetCore()) + coreModel.SetRootNode(rootNode.Content[0]) // Content[0] is the actual root mapping node + + // Unmarshal the YAML into the core model to populate it with the 6 items + _, err = marshaller.UnmarshalModel(t.Context(), rootNode.Content[0], coreModel) + require.NoError(t, err) + + // Use SetCore to link each high-level item to its corresponding core item + // This establishes the connection between high-level items and their core counterparts + for _, item := range coreModel.Items.Value { + switch item.Name.Value { + case "one": + itemOne.SetCore(item) + case "four": + itemFour.SetCore(item) + case "six": + itemSix.SetCore(item) + } + } + + // Sync the high model (3 items) to the core model (currently 6 items) + // This tests what happens when syncing a subset + resultNode, err := marshaller.SyncValue(t.Context(), &highModel, highModel.GetCore(), highModel.GetRootNode(), false) + require.NoError(t, err) + require.NotNil(t, resultNode) + + // Verify the synced array - should now match the high model's subset (3 items) + // After sync, items two, three, five, and second should be removed + items := coreModel.Items.Value + require.Len(t, items, 3, "After sync, core model should have 3 items matching high model") + + // Debug: Print what we actually got + t.Logf("Item 0: %s - %s", items[0].Name.Value, items[0].Description.Value) + t.Logf("Item 1: %s - %s", items[1].Name.Value, items[1].Description.Value) + t.Logf("Item 2: %s - %s", items[2].Name.Value, items[2].Description.Value) + + require.Equal(t, "one", items[0].Name.Value) + require.Equal(t, "First item", items[0].Description.Value) + + require.Equal(t, "four", items[1].Name.Value) + require.Equal(t, "Fourth item", items[1].Description.Value) + + require.Equal(t, "six", items[2].Name.Value) + require.Equal(t, "Sixth item", items[2].Description.Value) + + // Verify the core model's RootNode contains the correct YAML + expectedYAML := `items: + - name: one + description: First item + - name: four + description: Fourth item + - name: six + description: Sixth item +` + + actualYAML, err := yaml.Marshal(coreModel.GetRootNode()) + require.NoError(t, err) + require.Equal(t, expectedYAML, string(actualYAML)) +} diff --git a/marshaller/tests/core/models.go b/marshaller/tests/core/models.go index c6bed8c..c867233 100644 --- a/marshaller/tests/core/models.go +++ b/marshaller/tests/core/models.go @@ -183,3 +183,25 @@ type TestTypeConversionCoreModel struct { HTTPMethodField marshaller.Node[*string] `key:"httpMethodField"` Extensions core.Extensions `key:"extensions"` } + +// TestSimpleArrayModel is a minimal model with only an array field for testing array sync behavior +type TestSimpleArrayModel struct { + marshaller.CoreModel `model:"testSimpleArrayModel"` + + ArrayField marshaller.Node[[]string] `key:"arrayField"` +} + +// TestItemModel represents a simple item with name and description +type TestItemModel struct { + marshaller.CoreModel `model:"testItemModel"` + + Name marshaller.Node[string] `key:"name"` + Description marshaller.Node[string] `key:"description"` +} + +// TestArrayOfObjectsModel is a minimal model with an array of objects for testing array sync behavior +type TestArrayOfObjectsModel struct { + marshaller.CoreModel `model:"testArrayOfObjectsModel"` + + Items marshaller.Node[[]*TestItemModel] `key:"items"` +} diff --git a/marshaller/tests/models.go b/marshaller/tests/models.go index c3ae50b..8d48422 100644 --- a/marshaller/tests/models.go +++ b/marshaller/tests/models.go @@ -128,3 +128,22 @@ type TestEmbeddedMapWithFieldsPointerHighModel struct { NameField string Extensions *extensions.Extensions } + +// TestSimpleArrayHighModel is a minimal high-level model with only an array field for testing array sync behavior +type TestSimpleArrayHighModel struct { + marshaller.Model[core.TestSimpleArrayModel] + ArrayField []string +} + +// TestItemHighModel represents a simple item with name and description +type TestItemHighModel struct { + marshaller.Model[core.TestItemModel] + Name string + Description string +} + +// TestArrayOfObjectsHighModel is a minimal high-level model with an array of objects for testing array sync behavior +type TestArrayOfObjectsHighModel struct { + marshaller.Model[core.TestArrayOfObjectsModel] + Items []*TestItemHighModel +} From 991c710f1733d6310d354c9f1f12f4042243f094 Mon Sep 17 00:00:00 2001 From: subomi Date: Mon, 17 Nov 2025 18:47:31 +0000 Subject: [PATCH 2/2] refactor: clean up array subset sync test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename TestSync_ArraySubset_Debug to TestSync_ArraySubset_Success - Remove debug logging statements - Simplify comments to be more concise and professional - Remove unused TestSimpleArrayModel and TestSimpleArrayHighModel - Update model comments to be clearer and less verbose 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- marshaller/syncing_test.go | 33 +++++++++------------------------ marshaller/tests/core/models.go | 11 ++--------- marshaller/tests/models.go | 10 ++-------- 3 files changed, 13 insertions(+), 41 deletions(-) diff --git a/marshaller/syncing_test.go b/marshaller/syncing_test.go index 5abc014..3ea0d24 100644 --- a/marshaller/syncing_test.go +++ b/marshaller/syncing_test.go @@ -1144,11 +1144,10 @@ func TestSync_EmbeddedMapComparison_PointerVsValue_Success(t *testing.T) { }) } -func TestSync_ArraySubset_Debug(t *testing.T) { +func TestSync_ArraySubset_Success(t *testing.T) { t.Parallel() - // Create items for the high-level model (3 items - subset) - // Source: items one, four, and six + // Create high-level model with 3 items itemOne := &tests.TestItemHighModel{ Name: "one", Description: "First item", @@ -1166,9 +1165,7 @@ func TestSync_ArraySubset_Debug(t *testing.T) { Items: []*tests.TestItemHighModel{itemOne, itemFour, itemSix}, } - // Set the root node for the high-level model by creating a YAML node - // This simulates the case where we have an existing YAML document with 6 items - // Target: items three, five, one, four, second, and six (in this order) + // Populate core model with 6 items from YAML initialYAML := `items: - name: three description: Third item @@ -1187,18 +1184,13 @@ func TestSync_ArraySubset_Debug(t *testing.T) { err := yaml.Unmarshal([]byte(initialYAML), &rootNode) require.NoError(t, err) - // Get the core model coreModel := highModel.GetCore() + coreModel.SetRootNode(rootNode.Content[0]) - // Set the root node on the core model (accessed through GetCore()) - coreModel.SetRootNode(rootNode.Content[0]) // Content[0] is the actual root mapping node - - // Unmarshal the YAML into the core model to populate it with the 6 items _, err = marshaller.UnmarshalModel(t.Context(), rootNode.Content[0], coreModel) require.NoError(t, err) - // Use SetCore to link each high-level item to its corresponding core item - // This establishes the connection between high-level items and their core counterparts + // Link high-level items to their corresponding core items for _, item := range coreModel.Items.Value { switch item.Name.Value { case "one": @@ -1210,21 +1202,14 @@ func TestSync_ArraySubset_Debug(t *testing.T) { } } - // Sync the high model (3 items) to the core model (currently 6 items) - // This tests what happens when syncing a subset + // Sync high model subset to core model resultNode, err := marshaller.SyncValue(t.Context(), &highModel, highModel.GetCore(), highModel.GetRootNode(), false) require.NoError(t, err) require.NotNil(t, resultNode) - // Verify the synced array - should now match the high model's subset (3 items) - // After sync, items two, three, five, and second should be removed + // Verify synced array matches high model subset items := coreModel.Items.Value - require.Len(t, items, 3, "After sync, core model should have 3 items matching high model") - - // Debug: Print what we actually got - t.Logf("Item 0: %s - %s", items[0].Name.Value, items[0].Description.Value) - t.Logf("Item 1: %s - %s", items[1].Name.Value, items[1].Description.Value) - t.Logf("Item 2: %s - %s", items[2].Name.Value, items[2].Description.Value) + require.Len(t, items, 3) require.Equal(t, "one", items[0].Name.Value) require.Equal(t, "First item", items[0].Description.Value) @@ -1235,7 +1220,7 @@ func TestSync_ArraySubset_Debug(t *testing.T) { require.Equal(t, "six", items[2].Name.Value) require.Equal(t, "Sixth item", items[2].Description.Value) - // Verify the core model's RootNode contains the correct YAML + // Verify YAML output expectedYAML := `items: - name: one description: First item diff --git a/marshaller/tests/core/models.go b/marshaller/tests/core/models.go index c867233..a624300 100644 --- a/marshaller/tests/core/models.go +++ b/marshaller/tests/core/models.go @@ -184,14 +184,7 @@ type TestTypeConversionCoreModel struct { Extensions core.Extensions `key:"extensions"` } -// TestSimpleArrayModel is a minimal model with only an array field for testing array sync behavior -type TestSimpleArrayModel struct { - marshaller.CoreModel `model:"testSimpleArrayModel"` - - ArrayField marshaller.Node[[]string] `key:"arrayField"` -} - -// TestItemModel represents a simple item with name and description +// TestItemModel represents an item with a name and description type TestItemModel struct { marshaller.CoreModel `model:"testItemModel"` @@ -199,7 +192,7 @@ type TestItemModel struct { Description marshaller.Node[string] `key:"description"` } -// TestArrayOfObjectsModel is a minimal model with an array of objects for testing array sync behavior +// TestArrayOfObjectsModel contains an array of items type TestArrayOfObjectsModel struct { marshaller.CoreModel `model:"testArrayOfObjectsModel"` diff --git a/marshaller/tests/models.go b/marshaller/tests/models.go index 8d48422..4c69921 100644 --- a/marshaller/tests/models.go +++ b/marshaller/tests/models.go @@ -129,20 +129,14 @@ type TestEmbeddedMapWithFieldsPointerHighModel struct { Extensions *extensions.Extensions } -// TestSimpleArrayHighModel is a minimal high-level model with only an array field for testing array sync behavior -type TestSimpleArrayHighModel struct { - marshaller.Model[core.TestSimpleArrayModel] - ArrayField []string -} - -// TestItemHighModel represents a simple item with name and description +// TestItemHighModel represents an item with a name and description type TestItemHighModel struct { marshaller.Model[core.TestItemModel] Name string Description string } -// TestArrayOfObjectsHighModel is a minimal high-level model with an array of objects for testing array sync behavior +// TestArrayOfObjectsHighModel contains an array of items type TestArrayOfObjectsHighModel struct { marshaller.Model[core.TestArrayOfObjectsModel] Items []*TestItemHighModel