From 119c8bca4aafc613e120d77e8cee13614c352eaf Mon Sep 17 00:00:00 2001 From: quobix Date: Sat, 27 Jun 2026 11:02:10 -0400 Subject: [PATCH] deduplicating and centralizing cloning code. noticed some sloppery accumulating, time to wash it off. --- bundler/extension_refs.go | 39 +--- bundler/extension_refs_test.go | 129 ------------ datamodel/high/base/schema_proxy.go | 36 +--- datamodel/high/base/schema_proxy_test.go | 4 +- datamodel/high/node_builder.go | 19 +- datamodel/high/node_builder_test.go | 1 - datamodel/high/v3/components.go | 19 +- datamodel/high/v3/components_test.go | 19 -- datamodel/low/base/property_merger.go | 38 +--- datamodel/low/base/property_merger_test.go | 8 - datamodel/low/base/sibling_ref_transformer.go | 34 +--- .../low/base/sibling_ref_transformer_test.go | 53 ----- overlay/engine.go | 33 +--- overlay/engine_test.go | 19 -- utils/clone_node.go | 63 ++++++ utils/clone_node_bench_test.go | 73 +++++++ utils/clone_node_test.go | 185 ++++++++++++++++++ what-changed/model/comparison_functions.go | 29 +-- .../model/comparison_functions_test.go | 10 - 19 files changed, 343 insertions(+), 468 deletions(-) create mode 100644 utils/clone_node.go create mode 100644 utils/clone_node_bench_test.go create mode 100644 utils/clone_node_test.go diff --git a/bundler/extension_refs.go b/bundler/extension_refs.go index 715266e4d..ef8def282 100644 --- a/bundler/extension_refs.go +++ b/bundler/extension_refs.go @@ -67,49 +67,12 @@ func resolveExtensionRefContent(ctx context.Context, ref *index.Reference, _ *in foundRef := ref.Index.FindComponent(ctx, ref.FullDefinition) if foundRef != nil && foundRef.Node != nil { // Deep copy to avoid mutating original component - return deepCopyNode(foundRef.Node) + return utils.CloneYAMLNodeWithFlags(foundRef.Node, utils.YAMLNodeCloneUnwrapDocument) } } return nil } -// deepCopyNode creates a deep copy of a yaml.Node tree. -func deepCopyNode(node *yaml.Node) *yaml.Node { - if node == nil { - return nil - } - - // unwrap document nodes - if node.Kind == yaml.DocumentNode && len(node.Content) > 0 { - node = node.Content[0] - } - - // create copy - nodeCopy := &yaml.Node{ - Kind: node.Kind, - Style: node.Style, - Tag: node.Tag, - Value: node.Value, - Anchor: node.Anchor, - Alias: node.Alias, - HeadComment: node.HeadComment, - LineComment: node.LineComment, - FootComment: node.FootComment, - Line: node.Line, - Column: node.Column, - } - - // deep copy children - if len(node.Content) > 0 { - nodeCopy.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - nodeCopy.Content[i] = deepCopyNode(child) - } - } - - return nodeCopy -} - func replaceRefNodeWithContent(refNode, content *yaml.Node) { if refNode == nil || content == nil { return diff --git a/bundler/extension_refs_test.go b/bundler/extension_refs_test.go index 228a7823f..e8c7f625a 100644 --- a/bundler/extension_refs_test.go +++ b/bundler/extension_refs_test.go @@ -305,135 +305,6 @@ func TestResolveExtensionRefContent_ComponentNotFound(t *testing.T) { } } -func TestDeepCopyNode_Nil(t *testing.T) { - result := deepCopyNode(nil) - if result != nil { - t.Error("Expected nil result for nil input") - } -} - -func TestDeepCopyNode_DocumentNode(t *testing.T) { - // Test unwrapping of DocumentNode - innerNode := &yaml.Node{ - Kind: yaml.ScalarNode, - Value: "test", - } - docNode := &yaml.Node{ - Kind: yaml.DocumentNode, - Content: []*yaml.Node{innerNode}, - } - - result := deepCopyNode(docNode) - if result == nil { - t.Fatal("Expected non-nil result") - } - if result.Kind != yaml.ScalarNode { - t.Errorf("Expected ScalarNode after unwrap, got %v", result.Kind) - } - if result.Value != "test" { - t.Errorf("Expected value 'test', got '%s'", result.Value) - } -} - -func TestDeepCopyNode_WithContent(t *testing.T) { - // Test deep copy with children - child1 := &yaml.Node{Kind: yaml.ScalarNode, Value: "key"} - child2 := &yaml.Node{Kind: yaml.ScalarNode, Value: "value"} - parent := &yaml.Node{ - Kind: yaml.MappingNode, - Content: []*yaml.Node{child1, child2}, - } - - result := deepCopyNode(parent) - if result == nil { - t.Fatal("Expected non-nil result") - } - if len(result.Content) != 2 { - t.Fatalf("Expected 2 children, got %d", len(result.Content)) - } - // Verify it's a deep copy (different pointers) - if result.Content[0] == child1 { - t.Error("Expected deep copy, got same pointer for child1") - } - if result.Content[1] == child2 { - t.Error("Expected deep copy, got same pointer for child2") - } - // Verify values are copied - if result.Content[0].Value != "key" { - t.Errorf("Expected 'key', got '%s'", result.Content[0].Value) - } - if result.Content[1].Value != "value" { - t.Errorf("Expected 'value', got '%s'", result.Content[1].Value) - } -} - -func TestDeepCopyNode_NoContent(t *testing.T) { - // Test node with no children - node := &yaml.Node{ - Kind: yaml.ScalarNode, - Value: "scalar", - } - - result := deepCopyNode(node) - if result == nil { - t.Fatal("Expected non-nil result") - } - if result.Value != "scalar" { - t.Errorf("Expected 'scalar', got '%s'", result.Value) - } - if result.Content != nil { - t.Error("Expected nil Content for scalar node") - } -} - -func TestDeepCopyNode_AllFields(t *testing.T) { - // Test that all fields are copied - node := &yaml.Node{ - Kind: yaml.ScalarNode, - Style: yaml.DoubleQuotedStyle, - Tag: "!!str", - Value: "test", - Anchor: "anchor1", - HeadComment: "head", - LineComment: "line", - FootComment: "foot", - Line: 10, - Column: 5, - } - - result := deepCopyNode(node) - if result.Kind != yaml.ScalarNode { - t.Errorf("Kind mismatch") - } - if result.Style != yaml.DoubleQuotedStyle { - t.Errorf("Style mismatch") - } - if result.Tag != "!!str" { - t.Errorf("Tag mismatch") - } - if result.Value != "test" { - t.Errorf("Value mismatch") - } - if result.Anchor != "anchor1" { - t.Errorf("Anchor mismatch") - } - if result.HeadComment != "head" { - t.Errorf("HeadComment mismatch") - } - if result.LineComment != "line" { - t.Errorf("LineComment mismatch") - } - if result.FootComment != "foot" { - t.Errorf("FootComment mismatch") - } - if result.Line != 10 { - t.Errorf("Line mismatch") - } - if result.Column != 5 { - t.Errorf("Column mismatch") - } -} - func TestReplaceRefNodeWithContent_NilRefNode(t *testing.T) { content := &yaml.Node{Kind: yaml.ScalarNode, Value: "test"} // Should not panic with nil refNode diff --git a/datamodel/high/base/schema_proxy.go b/datamodel/high/base/schema_proxy.go index 53ecc1f5c..94642b945 100644 --- a/datamodel/high/base/schema_proxy.go +++ b/datamodel/high/base/schema_proxy.go @@ -540,8 +540,8 @@ func (sp *SchemaProxy) renderTransformedRefWithSiblings(s *Schema) (*yaml.Node, continue } if keyNode.Value == "$ref" { - refKey := cloneYAMLNode(keyNode) - refValue := cloneYAMLNode(valueNode) + refKey := utils.CloneYAMLNode(keyNode) + refValue := utils.CloneYAMLNode(valueNode) if refValue == nil { refValue = utils.CreateStringNode(ref) } @@ -550,8 +550,8 @@ func (sp *SchemaProxy) renderTransformedRefWithSiblings(s *Schema) (*yaml.Node, continue } if _, siblingValue := findYAMLPair(siblingNode, keyNode.Value); siblingValue != nil { - renderKey := cloneYAMLNode(keyNode) - result.Content = append(result.Content, renderKey, cloneYAMLNode(siblingValue)) + renderKey := utils.CloneYAMLNode(keyNode) + result.Content = append(result.Content, renderKey, utils.CloneYAMLNode(siblingValue)) consumed[keyNode.Value] = struct{}{} } } @@ -562,7 +562,7 @@ func (sp *SchemaProxy) renderTransformedRefWithSiblings(s *Schema) (*yaml.Node, if _, ok := consumed[keyNode.Value]; ok { continue } - result.Content = append(result.Content, cloneYAMLNode(keyNode), cloneYAMLNode(valueNode)) + result.Content = append(result.Content, utils.CloneYAMLNode(keyNode), utils.CloneYAMLNode(valueNode)) } return result, true, nil @@ -609,32 +609,6 @@ func findYAMLPair(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) { return nil, nil } -func cloneYAMLNode(node *yaml.Node) *yaml.Node { - if node == nil { - return nil - } - clone := &yaml.Node{ - Kind: node.Kind, - Style: node.Style, - Tag: node.Tag, - Value: node.Value, - Anchor: node.Anchor, - Alias: node.Alias, - Line: node.Line, - Column: node.Column, - HeadComment: node.HeadComment, - LineComment: node.LineComment, - FootComment: node.FootComment, - } - if len(node.Content) > 0 { - clone.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - clone.Content[i] = cloneYAMLNode(child) - } - } - return clone -} - // Render will return a YAML representation of the Schema object as a byte slice. func (sp *SchemaProxy) Render() ([]byte, error) { return yaml.Marshal(sp) diff --git a/datamodel/high/base/schema_proxy_test.go b/datamodel/high/base/schema_proxy_test.go index a346c75b7..244699a25 100644 --- a/datamodel/high/base/schema_proxy_test.go +++ b/datamodel/high/base/schema_proxy_test.go @@ -2194,8 +2194,8 @@ func TestTransformedRefRenderHelpers(t *testing.T) { assert.Nil(t, key) assert.Nil(t, value) - assert.Nil(t, cloneYAMLNode(nil)) - cloned := cloneYAMLNode(mapping) + assert.Nil(t, utils.CloneYAMLNode(nil)) + cloned := utils.CloneYAMLNode(mapping) require.NotNil(t, cloned) assert.Equal(t, mapping.Content[0].Value, cloned.Content[0].Value) cloned.Content[0].Value = "changed" diff --git a/datamodel/high/node_builder.go b/datamodel/high/node_builder.go index 378eac817..b282dc079 100644 --- a/datamodel/high/node_builder.go +++ b/datamodel/high/node_builder.go @@ -356,28 +356,11 @@ func (n *NodeBuilder) Render() *yaml.Node { // the same node. Encoding a copy keeps shared nodes immutable. func encodeSafeValue(value any) any { if vn, ok := value.(*yaml.Node); ok { - return deepCopyYAMLNode(vn) + return utils.CloneYAMLNode(vn) } return value } -func deepCopyYAMLNode(n *yaml.Node) *yaml.Node { - if n == nil { - return nil - } - c := *n - if n.Alias != nil { - c.Alias = deepCopyYAMLNode(n.Alias) - } - if n.Content != nil { - c.Content = make([]*yaml.Node, len(n.Content)) - for i, child := range n.Content { - c.Content[i] = deepCopyYAMLNode(child) - } - } - return &c -} - // AddYAMLNode will add a new *yaml.Node to the parent node, using the tag, key and value provided. // If the value is nil, then the node will not be added. This method is recursive, so it will dig down // into any non-scalar types. diff --git a/datamodel/high/node_builder_test.go b/datamodel/high/node_builder_test.go index 47dd6e2fd..2c16d226d 100644 --- a/datamodel/high/node_builder_test.go +++ b/datamodel/high/node_builder_test.go @@ -1707,7 +1707,6 @@ func TestEncodeSafeValue_DeepCopiesYAMLNodes(t *testing.T) { assert.Equal(t, "hello", encodeSafeValue("hello")) // nil nodes return nil rather than panicking. - assert.Nil(t, deepCopyYAMLNode(nil)) assert.Nil(t, encodeSafeValue((*yaml.Node)(nil))) // a *yaml.Node is deep-copied: every node is a distinct pointer, including diff --git a/datamodel/high/v3/components.go b/datamodel/high/v3/components.go index 2548088c3..846f867e2 100644 --- a/datamodel/high/v3/components.go +++ b/datamodel/high/v3/components.go @@ -315,7 +315,7 @@ func preserveComponentRefEntries[T any]( sectionNode, ) } - upsertMapNodeEntry(sectionNode, cloneYAMLNode(keyNode), cloneYAMLNode(valueNode)) + upsertMapNodeEntry(sectionNode, utils.CloneYAMLNode(keyNode), utils.CloneYAMLNode(valueNode)) } } @@ -346,20 +346,3 @@ func upsertMapNodeEntry(m *yaml.Node, keyNode, valueNode *yaml.Node) { } m.Content = append(m.Content, keyNode, valueNode) } - -// cloneYAMLNode deep-copies a YAML node tree so preserved low-level nodes can be spliced into -// rendered output without mutating the original parsed model. -func cloneYAMLNode(node *yaml.Node) *yaml.Node { - if node == nil { - return nil - } - - cloned := *node - if len(node.Content) > 0 { - cloned.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - cloned.Content[i] = cloneYAMLNode(child) - } - } - return &cloned -} diff --git a/datamodel/high/v3/components_test.go b/datamodel/high/v3/components_test.go index 5b56f8022..767d89513 100644 --- a/datamodel/high/v3/components_test.go +++ b/datamodel/high/v3/components_test.go @@ -478,25 +478,6 @@ func TestFindMapValueNodeAndUpsertMapNodeEntry(t *testing.T) { assert.Equal(t, "./updated.yaml", rendered.Content[1].Value) } -func TestCloneYAMLNode_ClonesRecursively(t *testing.T) { - assert.Nil(t, cloneYAMLNode(nil)) - - original := utils.CreateEmptyMapNode() - original.Content = append( - original.Content, - utils.CreateStringNode("child"), - utils.CreateStringNode("value"), - ) - - cloned := cloneYAMLNode(original) - require.NotNil(t, cloned) - require.NotSame(t, original, cloned) - require.Len(t, cloned.Content, 2) - assert.NotSame(t, original.Content[0], cloned.Content[0]) - assert.Equal(t, "child", cloned.Content[0].Value) - assert.Equal(t, "value", cloned.Content[1].Value) -} - func setLowComponentsIndex(comp *v3.Components, idx *index.SpecIndex) { field := reflect.ValueOf(comp).Elem().FieldByName("index") reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem().Set(reflect.ValueOf(idx)) diff --git a/datamodel/low/base/property_merger.go b/datamodel/low/base/property_merger.go index 1ec44a708..416587f28 100644 --- a/datamodel/low/base/property_merger.go +++ b/datamodel/low/base/property_merger.go @@ -31,10 +31,10 @@ func (pm *PropertyMerger) MergeProperties(localNode, referencedNode *yaml.Node) return nil, nil } if localNode == nil { - return pm.copyNode(referencedNode), nil + return utils.CloneYAMLNode(referencedNode), nil } if referencedNode == nil { - return pm.copyNode(localNode), nil + return utils.CloneYAMLNode(localNode), nil } // extract properties from both nodes @@ -42,7 +42,7 @@ func (pm *PropertyMerger) MergeProperties(localNode, referencedNode *yaml.Node) referencedProps := pm.extractProperties(referencedNode) // create merged node starting with referenced content - merged := pm.copyNode(referencedNode) + merged := utils.CloneYAMLNode(referencedNode) mergedProps := pm.extractProperties(merged) // apply merge strategy for each local property @@ -101,42 +101,12 @@ func (pm *PropertyMerger) rebuildNodeFromProperties(baseNode *yaml.Node, props m // rebuild content from properties for key, value := range props { keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key} - result.Content = append(result.Content, keyNode, pm.copyNode(value)) + result.Content = append(result.Content, keyNode, utils.CloneYAMLNode(value)) } return result } -// copyNode creates a deep copy of a yaml node -func (pm *PropertyMerger) copyNode(node *yaml.Node) *yaml.Node { - if node == nil { - return nil - } - - copied := &yaml.Node{ - Kind: node.Kind, - Style: node.Style, - Tag: node.Tag, - Value: node.Value, - Anchor: node.Anchor, - Alias: node.Alias, - Line: node.Line, - Column: node.Column, - HeadComment: node.HeadComment, - LineComment: node.LineComment, - FootComment: node.FootComment, - } - - if node.Content != nil { - copied.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - copied.Content[i] = pm.copyNode(child) - } - } - - return copied -} - // ShouldMergeProperties determines if property merging should be applied based on configuration func (pm *PropertyMerger) ShouldMergeProperties(localNode, referencedNode *yaml.Node, config *datamodel.DocumentConfiguration) bool { if config == nil || !config.MergeReferencedProperties { diff --git a/datamodel/low/base/property_merger_test.go b/datamodel/low/base/property_merger_test.go index 8d8c5f304..bd8955dfb 100644 --- a/datamodel/low/base/property_merger_test.go +++ b/datamodel/low/base/property_merger_test.go @@ -234,11 +234,3 @@ func TestPropertyMerger_ShouldMergeProperties(t *testing.T) { assert.False(t, should) }) } - -func TestPropertyMerger_copyNode_Nil(t *testing.T) { - // test that copyNode handles nil input correctly (lines 113-114) - merger := NewPropertyMerger(datamodel.PreserveLocal) - - result := merger.copyNode(nil) - assert.Nil(t, result) -} diff --git a/datamodel/low/base/sibling_ref_transformer.go b/datamodel/low/base/sibling_ref_transformer.go index 69c9cd48e..3cebab2fe 100644 --- a/datamodel/low/base/sibling_ref_transformer.go +++ b/datamodel/low/base/sibling_ref_transformer.go @@ -74,7 +74,7 @@ func (srt *SiblingRefTransformer) CreateAllOfStructure(refValue string, siblings for _, key := range keys { valueNode := siblings[key] keyNode := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: key} - copiedValueNode := srt.copyNode(valueNode) + copiedValueNode := utils.CloneYAMLNode(valueNode) siblingSchemaNode.Content = append(siblingSchemaNode.Content, keyNode, copiedValueNode) } } @@ -121,7 +121,7 @@ func (srt *SiblingRefTransformer) createSiblingSchemaNode(node *yaml.Node) *yaml if keyNode == nil || keyNode.Value == "$ref" { continue } - siblingNode.Content = append(siblingNode.Content, srt.copyNode(keyNode), srt.copyNode(valueNode)) + siblingNode.Content = append(siblingNode.Content, utils.CloneYAMLNode(keyNode), utils.CloneYAMLNode(valueNode)) } return siblingNode } @@ -171,33 +171,3 @@ func (srt *SiblingRefTransformer) ShouldTransform(node *yaml.Node) bool { siblings, refValue := srt.ExtractSiblingProperties(node) return len(siblings) > 0 && refValue != "" } - -// copyNode creates a deep copy of a yaml node to avoid modifying the original -func (srt *SiblingRefTransformer) copyNode(node *yaml.Node) *yaml.Node { - if node == nil { - return nil - } - - copied := &yaml.Node{ - Kind: node.Kind, - Style: node.Style, - Tag: node.Tag, - Value: node.Value, - Anchor: node.Anchor, - Alias: node.Alias, - Line: node.Line, - Column: node.Column, - HeadComment: node.HeadComment, - LineComment: node.LineComment, - FootComment: node.FootComment, - } - - if node.Content != nil { - copied.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - copied.Content[i] = srt.copyNode(child) - } - } - - return copied -} diff --git a/datamodel/low/base/sibling_ref_transformer_test.go b/datamodel/low/base/sibling_ref_transformer_test.go index 9f80c72b5..d4f82034d 100644 --- a/datamodel/low/base/sibling_ref_transformer_test.go +++ b/datamodel/low/base/sibling_ref_transformer_test.go @@ -449,59 +449,6 @@ $ref: "#/components/schemas/Base"` }) } -func TestSiblingRefTransformer_copyNode(t *testing.T) { - config := index.CreateOpenAPIIndexConfig() - var rootNode yaml.Node - idx := index.NewSpecIndexWithConfig(&rootNode, config) - transformer := NewSiblingRefTransformer(idx) - - t.Run("copy simple scalar node", func(t *testing.T) { - original := &yaml.Node{ - Kind: yaml.ScalarNode, - Value: "test value", - Line: 10, - Column: 5, - } - - copied := transformer.copyNode(original) - - assert.NotSame(t, original, copied) - assert.Equal(t, original.Kind, copied.Kind) - assert.Equal(t, original.Value, copied.Value) - assert.Equal(t, original.Line, copied.Line) - assert.Equal(t, original.Column, copied.Column) - }) - - t.Run("copy mapping node with content", func(t *testing.T) { - original := &yaml.Node{ - Kind: yaml.MappingNode, - Content: []*yaml.Node{ - {Kind: yaml.ScalarNode, Value: "key1"}, - {Kind: yaml.ScalarNode, Value: "value1"}, - {Kind: yaml.ScalarNode, Value: "key2"}, - {Kind: yaml.ScalarNode, Value: "value2"}, - }, - } - - copied := transformer.copyNode(original) - - assert.NotSame(t, original, copied) - assert.Equal(t, original.Kind, copied.Kind) - assert.Len(t, copied.Content, 4) - - // verify content is copied but not same references - for i, child := range copied.Content { - assert.NotSame(t, original.Content[i], child) - assert.Equal(t, original.Content[i].Value, child.Value) - } - }) - - t.Run("copy nil node", func(t *testing.T) { - copied := transformer.copyNode(nil) - assert.Nil(t, copied) - }) -} - func TestSiblingRefTransformer_ChecBreak(t *testing.T) { transformer := NewSiblingRefTransformer(nil) result, str := transformer.ExtractSiblingProperties(&yaml.Node{ diff --git a/overlay/engine.go b/overlay/engine.go index 091c7f607..c58ac5b40 100644 --- a/overlay/engine.go +++ b/overlay/engine.go @@ -7,6 +7,7 @@ import ( "github.com/pb33f/jsonpath/pkg/jsonpath" "github.com/pb33f/jsonpath/pkg/jsonpath/config" highoverlay "github.com/pb33f/libopenapi/datamodel/high/overlay" + "github.com/pb33f/libopenapi/utils" "go.yaml.in/yaml/v4" ) @@ -210,7 +211,7 @@ func removeNode(idx parentIndex, node *yaml.Node) { func mergeNode(node *yaml.Node, merge *yaml.Node) { if node.Kind != merge.Kind { - *node = *cloneNode(merge) + *node = *utils.CloneYAMLNode(merge) return } switch node.Kind { @@ -237,39 +238,13 @@ NextKey: } } - node.Content = append(node.Content, merge.Content[i], cloneNode(mergeValue)) + node.Content = append(node.Content, merge.Content[i], utils.CloneYAMLNode(mergeValue)) } } func mergeSequenceNode(node *yaml.Node, merge *yaml.Node) { // clone each child individually to avoid wasteful intermediate allocation for _, child := range merge.Content { - node.Content = append(node.Content, cloneNode(child)) + node.Content = append(node.Content, utils.CloneYAMLNode(child)) } } - -func cloneNode(node *yaml.Node) *yaml.Node { - if node == nil { - return nil - } - newNode := &yaml.Node{ - Kind: node.Kind, - Style: node.Style, - Tag: node.Tag, - Value: node.Value, - Anchor: node.Anchor, - HeadComment: node.HeadComment, - LineComment: node.LineComment, - FootComment: node.FootComment, - } - if node.Alias != nil { - newNode.Alias = cloneNode(node.Alias) - } - if node.Content != nil { - newNode.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - newNode.Content[i] = cloneNode(child) - } - } - return newNode -} diff --git a/overlay/engine_test.go b/overlay/engine_test.go index d0c0eacb8..4d9c0d2f0 100644 --- a/overlay/engine_test.go +++ b/overlay/engine_test.go @@ -558,25 +558,6 @@ info: assert.NotNil(t, result) } -func TestCloneNode_WithAlias(t *testing.T) { - // Create a node with an alias - alias := &yaml.Node{Kind: yaml.ScalarNode, Value: "aliased"} - node := &yaml.Node{ - Kind: yaml.ScalarNode, - Alias: alias, - } - - cloned := cloneNode(node) - assert.NotNil(t, cloned.Alias) - assert.Equal(t, "aliased", cloned.Alias.Value) -} - -func TestCloneNode_Nil(t *testing.T) { - // cloneNode should handle nil input gracefully - cloned := cloneNode(nil) - assert.Nil(t, cloned) -} - func TestApply_InvalidJSONPath(t *testing.T) { targetYAML := `openapi: 3.0.0 info: diff --git a/utils/clone_node.go b/utils/clone_node.go new file mode 100644 index 000000000..37a32ef9f --- /dev/null +++ b/utils/clone_node.go @@ -0,0 +1,63 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package utils + +import "go.yaml.in/yaml/v4" + +// YAMLNodeCloneFlags configures CloneYAMLNodeWithFlags behavior. +type YAMLNodeCloneFlags uint8 + +const ( + // YAMLNodeCloneStripAnchors removes anchor names from every cloned node. + YAMLNodeCloneStripAnchors YAMLNodeCloneFlags = 1 << iota + + // YAMLNodeCloneUnwrapDocument clones the document node's first child instead + // of cloning the document node itself. + YAMLNodeCloneUnwrapDocument +) + +// CloneYAMLNode returns a deep copy of a YAML node graph. +func CloneYAMLNode(node *yaml.Node) *yaml.Node { + return CloneYAMLNodeWithFlags(node, 0) +} + +// CloneYAMLNodeWithFlags returns a deep copy of a YAML node graph, applying the +// supplied flags while cloning. +func CloneYAMLNodeWithFlags(node *yaml.Node, flags YAMLNodeCloneFlags) *yaml.Node { + if node == nil { + return nil + } + if flags&YAMLNodeCloneUnwrapDocument != 0 && node.Kind == yaml.DocumentNode && len(node.Content) > 0 { + node = node.Content[0] + } + return cloneYAMLNode(node, flags, make(map[*yaml.Node]*yaml.Node, 8)) +} + +func cloneYAMLNode(node *yaml.Node, flags YAMLNodeCloneFlags, seen map[*yaml.Node]*yaml.Node) *yaml.Node { + if node == nil { + return nil + } + if cloned, ok := seen[node]; ok { + return cloned + } + + cloned := *node + if flags&YAMLNodeCloneStripAnchors != 0 { + cloned.Anchor = "" + } + cloned.Alias = nil + cloned.Content = nil + seen[node] = &cloned + + if node.Alias != nil { + cloned.Alias = cloneYAMLNode(node.Alias, flags, seen) + } + if len(node.Content) > 0 { + cloned.Content = make([]*yaml.Node, len(node.Content)) + for i, child := range node.Content { + cloned.Content[i] = cloneYAMLNode(child, flags, seen) + } + } + return &cloned +} diff --git a/utils/clone_node_bench_test.go b/utils/clone_node_bench_test.go new file mode 100644 index 000000000..e6eeb618a --- /dev/null +++ b/utils/clone_node_bench_test.go @@ -0,0 +1,73 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package utils + +import ( + "testing" + + "github.com/pb33f/testify/require" + "go.yaml.in/yaml/v4" +) + +func BenchmarkCloneYAMLNode_Scalar(b *testing.B) { + node := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "value"} + for b.Loop() { + _ = CloneYAMLNode(node) + } +} + +func BenchmarkCloneYAMLNode_Schema(b *testing.B) { + node := benchmarkCloneYAMLNodeSchema(b) + for b.Loop() { + _ = CloneYAMLNode(node) + } +} + +func BenchmarkCloneYAMLNode_Alias(b *testing.B) { + target := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "target", Anchor: "target"} + node := &yaml.Node{ + Kind: yaml.SequenceNode, + Tag: "!!seq", + Content: []*yaml.Node{ + {Kind: yaml.AliasNode, Alias: target}, + {Kind: yaml.AliasNode, Alias: target}, + }, + } + for b.Loop() { + _ = CloneYAMLNode(node) + } +} + +func benchmarkCloneYAMLNodeSchema(b *testing.B) *yaml.Node { + b.Helper() + var root yaml.Node + err := yaml.Unmarshal([]byte(` +type: object +required: + - id +properties: + id: + type: string + profile: + allOf: + - $ref: '#/$defs/Profile' + - type: object + properties: + active: + type: boolean +$defs: + Profile: + type: object + properties: + name: + type: string + tags: + type: array + items: + type: string +`), &root) + require.NoError(b, err) + require.Len(b, root.Content, 1) + return root.Content[0] +} diff --git a/utils/clone_node_test.go b/utils/clone_node_test.go new file mode 100644 index 000000000..6efd10a66 --- /dev/null +++ b/utils/clone_node_test.go @@ -0,0 +1,185 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package utils + +import ( + "testing" + + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" + "go.yaml.in/yaml/v4" +) + +func TestCloneYAMLNode_Nil(t *testing.T) { + assert.Nil(t, CloneYAMLNode(nil)) + assert.Nil(t, CloneYAMLNodeWithFlags(nil, YAMLNodeCloneStripAnchors)) +} + +func TestCloneYAMLNode_ClonesScalarMetadata(t *testing.T) { + original := &yaml.Node{ + Kind: yaml.ScalarNode, + Style: yaml.SingleQuotedStyle, + Tag: "!!str", + Value: "value", + Anchor: "anchor", + Line: 10, + Column: 3, + HeadComment: "head", + LineComment: "line", + FootComment: "foot", + } + + cloned := CloneYAMLNode(original) + + require.NotSame(t, original, cloned) + assert.Equal(t, original.Kind, cloned.Kind) + assert.Equal(t, original.Style, cloned.Style) + assert.Equal(t, original.Tag, cloned.Tag) + assert.Equal(t, original.Value, cloned.Value) + assert.Equal(t, original.Anchor, cloned.Anchor) + assert.Equal(t, original.Line, cloned.Line) + assert.Equal(t, original.Column, cloned.Column) + assert.Equal(t, original.HeadComment, cloned.HeadComment) + assert.Equal(t, original.LineComment, cloned.LineComment) + assert.Equal(t, original.FootComment, cloned.FootComment) +} + +func TestCloneYAMLNode_ClonesContentRecursively(t *testing.T) { + original := &yaml.Node{ + Kind: yaml.MappingNode, + Tag: "!!map", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "key"}, + { + Kind: yaml.SequenceNode, + Tag: "!!seq", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "value"}, + }, + }, + }, + } + + cloned := CloneYAMLNode(original) + + require.NotSame(t, original, cloned) + require.Len(t, cloned.Content, 2) + require.NotSame(t, original.Content[0], cloned.Content[0]) + require.NotSame(t, original.Content[1], cloned.Content[1]) + require.Len(t, cloned.Content[1].Content, 1) + require.NotSame(t, original.Content[1].Content[0], cloned.Content[1].Content[0]) + + cloned.Content[1].Content[0].Value = "changed" + assert.Equal(t, "value", original.Content[1].Content[0].Value) +} + +func TestCloneYAMLNode_PreservesNilContentChildren(t *testing.T) { + original := &yaml.Node{ + Kind: yaml.SequenceNode, + Tag: "!!seq", + Content: []*yaml.Node{nil}, + } + + cloned := CloneYAMLNode(original) + + require.NotSame(t, original, cloned) + require.Len(t, cloned.Content, 1) + assert.Nil(t, cloned.Content[0]) +} + +func TestCloneYAMLNode_ClonesAliasTargets(t *testing.T) { + target := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "target", Anchor: "target"} + original := &yaml.Node{ + Kind: yaml.SequenceNode, + Tag: "!!seq", + Content: []*yaml.Node{ + {Kind: yaml.AliasNode, Alias: target}, + {Kind: yaml.AliasNode, Alias: target}, + }, + } + + cloned := CloneYAMLNode(original) + + firstAlias := cloned.Content[0] + secondAlias := cloned.Content[1] + require.NotSame(t, original.Content[0], firstAlias) + require.NotSame(t, target, firstAlias.Alias) + require.Same(t, firstAlias.Alias, secondAlias.Alias) + + firstAlias.Alias.Value = "changed" + assert.Equal(t, "target", target.Value) + assert.Equal(t, "changed", secondAlias.Alias.Value) +} + +func TestCloneYAMLNode_ReusesSeenContentNodes(t *testing.T) { + target := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "target"} + original := &yaml.Node{ + Kind: yaml.SequenceNode, + Tag: "!!seq", + Content: []*yaml.Node{target, target}, + } + + cloned := CloneYAMLNode(original) + + require.NotSame(t, target, cloned.Content[0]) + require.Same(t, cloned.Content[0], cloned.Content[1]) +} + +func TestCloneYAMLNode_ClonesAliasCycles(t *testing.T) { + root := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map", Anchor: "root"} + root.Content = []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "self"}, + {Kind: yaml.AliasNode, Alias: root}, + } + + cloned := CloneYAMLNode(root) + + require.NotSame(t, root, cloned) + require.Len(t, cloned.Content, 2) + require.NotSame(t, root.Content[1], cloned.Content[1]) + require.Same(t, cloned, cloned.Content[1].Alias) +} + +func TestCloneYAMLNodeWithFlags_StripsAnchors(t *testing.T) { + original := &yaml.Node{ + Kind: yaml.MappingNode, + Tag: "!!map", + Anchor: "root", + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "key", Anchor: "key"}, + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "value", Anchor: "value"}, + }, + } + + cloned := CloneYAMLNodeWithFlags(original, YAMLNodeCloneStripAnchors) + + assert.Empty(t, cloned.Anchor) + assert.Empty(t, cloned.Content[0].Anchor) + assert.Empty(t, cloned.Content[1].Anchor) + assert.Equal(t, "root", original.Anchor) + assert.Equal(t, "key", original.Content[0].Anchor) + assert.Equal(t, "value", original.Content[1].Anchor) +} + +func TestCloneYAMLNodeWithFlags_UnwrapsDocument(t *testing.T) { + child := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + document := &yaml.Node{ + Kind: yaml.DocumentNode, + Content: []*yaml.Node{child}, + } + + cloned := CloneYAMLNodeWithFlags(document, YAMLNodeCloneUnwrapDocument) + + require.NotSame(t, child, cloned) + assert.Equal(t, yaml.MappingNode, cloned.Kind) +} + +func TestCloneYAMLNodeWithFlags_DoesNotUnwrapEmptyDocument(t *testing.T) { + document := &yaml.Node{Kind: yaml.DocumentNode} + + cloned := CloneYAMLNodeWithFlags(document, YAMLNodeCloneUnwrapDocument) + + require.NotSame(t, document, cloned) + assert.Equal(t, yaml.DocumentNode, cloned.Kind) +} diff --git a/what-changed/model/comparison_functions.go b/what-changed/model/comparison_functions.go index 30361154b..27c0558ec 100644 --- a/what-changed/model/comparison_functions.go +++ b/what-changed/model/comparison_functions.go @@ -487,8 +487,8 @@ func compareYAMLNodesForChanges(left, right *yaml.Node) bool { if low.CompareYAMLNodes(left, right) { return true } - leftClone := cloneYAMLNodeWithoutAnchors(left, nil) - rightClone := cloneYAMLNodeWithoutAnchors(right, nil) + leftClone := utils.CloneYAMLNodeWithFlags(left, utils.YAMLNodeCloneStripAnchors) + rightClone := utils.CloneYAMLNodeWithFlags(right, utils.YAMLNodeCloneStripAnchors) if low.CompareYAMLNodes(leftClone, rightClone) { return true } @@ -498,31 +498,6 @@ func compareYAMLNodesForChanges(left, right *yaml.Node) bool { return leftErr == nil && rightErr == nil && bytes.Equal(leftBytes, rightBytes) } -func cloneYAMLNodeWithoutAnchors(node *yaml.Node, seen map[*yaml.Node]*yaml.Node) *yaml.Node { - if node == nil { - return nil - } - if seen == nil { - seen = make(map[*yaml.Node]*yaml.Node) - } - if clone, ok := seen[node]; ok { - return clone - } - - clone := *node - clone.Anchor = "" - seen[node] = &clone - - if len(node.Content) > 0 { - clone.Content = make([]*yaml.Node, len(node.Content)) - for i, child := range node.Content { - clone.Content[i] = cloneYAMLNodeWithoutAnchors(child, seen) - } - } - clone.Alias = cloneYAMLNodeWithoutAnchors(node.Alias, seen) - return &clone -} - // CheckForModification will check left and right yaml.Node instances for changes. Anything that is found in both // sides, but vary in value is considered a modification. // diff --git a/what-changed/model/comparison_functions_test.go b/what-changed/model/comparison_functions_test.go index bf4362f4e..93fdb0ef4 100644 --- a/what-changed/model/comparison_functions_test.go +++ b/what-changed/model/comparison_functions_test.go @@ -696,16 +696,6 @@ func TestCheckForModificationWithEncoding_IgnoresAnchorOnlyDifference(t *testing assert.Empty(t, changes) } -func TestCloneYAMLNodeWithoutAnchors_ReusesSeenNode(t *testing.T) { - target := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "same", Anchor: "target"} - node := &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq", Content: []*yaml.Node{target, target}} - - clone := cloneYAMLNodeWithoutAnchors(node, nil) - - assert.Empty(t, clone.Content[0].Anchor) - assert.Same(t, clone.Content[0], clone.Content[1]) -} - // TestCheckMapForChangesWithComp tests the deprecated CheckMapForChangesWithComp function func TestCheckMapForChangesWithComp(t *testing.T) { t.Run("detects removal", func(t *testing.T) {