From 17f18604e40bdc45e68c89c9a2d18c7ebdfef5a2 Mon Sep 17 00:00:00 2001
From: Phani Teja Marupaka <pmarupaka@google.com>
Date: Tue, 7 Sep 2021 16:58:06 -0700
Subject: [PATCH] Copy reference nodes before copying comments and syncing
 order

---
 kyaml/comments/comments.go      | 16 +++++++++-------
 kyaml/comments/comments_test.go | 33 +++++++++++++++++++++++++++++++++
 kyaml/order/syncorder.go        |  4 +++-
 kyaml/order/syncorder_test.go   | 10 ++++++++++
 4 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/kyaml/comments/comments.go b/kyaml/comments/comments.go
index 0dfc4d8de..97334d0f0 100644
--- a/kyaml/comments/comments.go
+++ b/kyaml/comments/comments.go
@@ -11,10 +11,12 @@ import (
 
 // CopyComments recursively copies the comments on fields in from to fields in to
 func CopyComments(from, to *yaml.RNode) error {
-	copy(from, to)
+	// from node should not be modified, it should be just used as a reference
+	fromCopy := from.Copy()
+	copyFieldComments(fromCopy, to)
 	// walk the fields copying comments
 	_, err := walk.Walker{
-		Sources:            []*yaml.RNode{from, to},
+		Sources:            []*yaml.RNode{fromCopy, to},
 		Visitor:            &copier{},
 		VisitKeysAsScalars: true}.Walk()
 	return err
@@ -25,7 +27,7 @@ func CopyComments(from, to *yaml.RNode) error {
 type copier struct{}
 
 func (c *copier) VisitMap(s walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) {
-	copy(s.Dest(), s.Origin())
+	copyFieldComments(s.Dest(), s.Origin())
 	return s.Dest(), nil
 }
 
@@ -39,13 +41,13 @@ func (c *copier) VisitScalar(s walk.Sources, _ *openapi.ResourceSchema) (*yaml.R
 		to.Document().Style = yaml.DoubleQuotedStyle
 	}
 
-	copy(s.Dest(), to)
+	copyFieldComments(s.Dest(), to)
 	return s.Dest(), nil
 }
 
 func (c *copier) VisitList(s walk.Sources, _ *openapi.ResourceSchema, _ walk.ListKind) (
 	*yaml.RNode, error) {
-	copy(s.Dest(), s.Origin())
+	copyFieldComments(s.Dest(), s.Origin())
 	destItems := s.Dest().Content()
 	originItems := s.Origin().Content()
 
@@ -64,8 +66,8 @@ func (c *copier) VisitList(s walk.Sources, _ *openapi.ResourceSchema, _ walk.Lis
 	return s.Dest(), nil
 }
 
-// copy copies the comment from one field to another
-func copy(from, to *yaml.RNode) {
+// copyFieldComments copies the comment from one field to another
+func copyFieldComments(from, to *yaml.RNode) {
 	if from == nil || to == nil {
 		return
 	}
diff --git a/kyaml/comments/comments_test.go b/kyaml/comments/comments_test.go
index e96047417..2509e8adb 100644
--- a/kyaml/comments/comments_test.go
+++ b/kyaml/comments/comments_test.go
@@ -351,6 +351,30 @@ apiVersion: v1
 kind: ConfigMap
 data:
   somekey: "012345678901234567890123456789012345678901234567890123456789012345678901234" # x
+`,
+		},
+		{
+			name: "sort fields with null value",
+			from: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: workspaces.app.terraform.io
+  creationTimestamp: null # this field is null
+  namespace: staging
+`,
+			to: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: workspaces.app.terraform.io
+  creationTimestamp: null
+  namespace: staging
+`,
+			expected: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: workspaces.app.terraform.io
+  creationTimestamp: null # this field is null
+  namespace: staging
 `,
 		},
 	}
@@ -378,9 +402,18 @@ data:
 				t.FailNow()
 			}
 
+			actualFrom, err := from.String()
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+
 			if !assert.Equal(t, strings.TrimSpace(tc.expected), strings.TrimSpace(actual)) {
 				t.FailNow()
 			}
+
+			if !assert.Equal(t, strings.TrimSpace(tc.from), strings.TrimSpace(actualFrom)) {
+				t.FailNow()
+			}
 		})
 	}
 }
diff --git a/kyaml/order/syncorder.go b/kyaml/order/syncorder.go
index 06d157a5d..9352fd8cc 100644
--- a/kyaml/order/syncorder.go
+++ b/kyaml/order/syncorder.go
@@ -13,7 +13,9 @@ import (
 // Field order might be altered due to round-tripping in arbitrary functions.
 // This functionality helps to retain the original order of fields to avoid unnecessary diffs.
 func SyncOrder(from, to *yaml.RNode) error {
-	if err := syncOrder(from, to); err != nil {
+	// from node should not be modified, it should be just used as a reference
+	fromCopy := from.Copy()
+	if err := syncOrder(fromCopy, to); err != nil {
 		return errors.Errorf("failed to sync field order: %q", err.Error())
 	}
 	rearrangeHeadCommentOfSeqNode(to.YNode())
diff --git a/kyaml/order/syncorder_test.go b/kyaml/order/syncorder_test.go
index bd5c75f15..6fa8fe021 100644
--- a/kyaml/order/syncorder_test.go
+++ b/kyaml/order/syncorder_test.go
@@ -5,6 +5,7 @@ package order
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -413,6 +414,15 @@ status:
 			if !assert.Equal(t, tc.expected, out.String()) {
 				t.FailNow()
 			}
+
+			actualFrom, err := from.String()
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+
+			if !assert.Equal(t, strings.TrimSpace(tc.from), strings.TrimSpace(actualFrom)) {
+				t.FailNow()
+			}
 		})
 	}
 }