Refactor Strategic Merge Patch

[mengqiy]

Refactor Strategic Merge Patch

None

[k8s-reviewable]

This change is 

[mengqiy]

/unassign @dchen1107
/assign @pwittrock

[pwittrock]

@janetkuo Are you familiar with this code and able to review?

[pwittrock]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go, line 30 at r1 (raw file):

	ErrNoMergeKeyFmt   = "map: %v does not contain declared merge key: %s"
	ErrBadArgTypeFmt   = "expected a %s, but received a %s"

Make these functions with arguments.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 57 at r1 (raw file):

type DiffOptions struct {
	IgnoreChangesAndAdditions bool

Comment what these are

---

Comments from Reviewable

[pwittrock]

Review status: 0 of 3 files reviewed at latest revision, 20 unresolved discussions.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 161 at r1 (raw file):

		// Types are the same, so compare values
		switch originalValueTyped := originalValue.(type) {
		case map[string]interface{}:

Pass in the casted value for modifiedValue

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 198 at r1 (raw file):

// handleMapDiff diff between 2 maps `originalValueTyped` and `modifiedValue`,
// puts the diff in the `patch` associated with `key`
func handleMapDiff(key string, originalValueTyped map[string]interface{}, modifiedValue interface{},

Add comments for arguments

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 200 at r1 (raw file):

func handleMapDiff(key string, originalValueTyped map[string]interface{}, modifiedValue interface{},
	t reflect.Type, patch map[string]interface{}, diffOptions DiffOptions) error {
	modifiedValueTyped := modifiedValue.(map[string]interface{})

Cast this before passing in

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 211 at r1 (raw file):

		return err
	}
	if fieldPatchStrategy == replaceDirective {

Add a comment that the directive tells us to replace the entire object instead of diffing it

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 221 at r1 (raw file):

		return err
	}
	if len(patchValue) > 0 {

Add comment // Maps were not identical, use provided patch value

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 229 at r1 (raw file):

// handleSliceDiff diff between 2 slices `originalValueTyped` and `modifiedValue`,
// puts the diff in the `patch` associated with `key`
func handleSliceDiff(key string, originalValueTyped []interface{}, modifiedValue interface{},

drop "Typed" piece of var names

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 231 at r1 (raw file):

func handleSliceDiff(key string, originalValueTyped []interface{}, modifiedValue interface{},
	t reflect.Type, patch map[string]interface{}, diffOptions DiffOptions) error {
	modifiedValueTyped := modifiedValue.([]interface{})

pass in casted value

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 244 at r1 (raw file):

	switch fieldPatchStrategy {
	case mergeDirective:

// Merge the 2 slices using mergePatchKey

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 265 at r1 (raw file):

// updatePatchFieldOnNotDeepEqual updates the patch if original and modified are not deep equal if ignoreChangesAndAdditions is false.
func updatePatchFieldOnNotDeepEqual(key string, original, modified interface{}, patch map[string]interface{}, ignoreChangesAndAdditions bool) {

Pass in the full options.

replacePatchFieldIfNotEqual

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 266 at r1 (raw file):

// updatePatchFieldOnNotDeepEqual updates the patch if original and modified are not deep equal if ignoreChangesAndAdditions is false.
func updatePatchFieldOnNotDeepEqual(key string, original, modified interface{}, patch map[string]interface{}, ignoreChangesAndAdditions bool) {
	if !ignoreChangesAndAdditions {

if ignoreChangesAndAdditions {
// Ignoring changes - do nothing
return
}
if reflect.DeepEqual(original, modified) {
// Contents are equal - do nothing
return
}
// Create a patch to replace the old value with the new one
patch[key] = modified

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 276 at r1 (raw file):

// updatePatchOnMissing iterates over `original` when ignoreDeletions is false.
// Clear the field whose key is not present in `modified`.
func updatePatchOnMissing(original, modified, patch map[string]interface{}, ignoreDeletions bool) {

Add comment for original and modified

// original maybe either the live cluster object or the last applied configuration
// modified is always the users new config

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 277 at r1 (raw file):

// Clear the field whose key is not present in `modified`.
func updatePatchOnMissing(original, modified, patch map[string]interface{}, ignoreDeletions bool) {
	if !ignoreDeletions {

if ignoreDeletions {
// Ignoring changes - do nothing
return
}
...

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 280 at r1 (raw file):

		// Add nils for deleted values
		for key := range original {
			_, found := modified[key]

if _, found := modified[key]; !found {
patch[key] = nil
}

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 339 at r1 (raw file):

		}
		// we need to compare the string representation of the scalar,
		// because the scalar is an interface which doesn't support neither < nor <

support either < or >

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 364 at r1 (raw file):

// controlledAppendList appends the list1Index-th item in list1 to resultList
// when either only list1 is in bound or list1 has additional scalar. The behavior is gated by notAppendItem.
func controlledAppendList(list1InBound, list2InBound,

`
// If first return value is non-nil, list1 contains an element not present in list2
// If second return value is non-nil, list2 contains an element not present in list1
func compareListValuesAtIndex(list1Inbounds, list2Inbounds bool, list1Value, list2Value string) (interface{}, interface{}) {
switch {
case list1Inbounds && list2Inbounds:
if list1Value < list2Value...
else
case !list1Inbounds
return nil, list2Value
case !list2Inbounds
return list1Value, nil
}
}

`

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 371 at r1 (raw file):

	bothListsInBound := list1InBound && list2InBound
	// only list1 is in bound OR list1 has additional scalar
	if !list2InBound || bothListsInBound && list1String < list2String {

Boolean logic unclear (put parens)

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 407 at r1 (raw file):

		var originalMap, modifiedMap map[string]interface{}
		if originalInBounds {
			originalMap, originalValue, err = getMapAndMergeKeyValueByIndex(originalIndex, mergeKey, originalSorted)

originalElement, originalElementMergeKeyValue

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 438 at r1 (raw file):

			fallthrough
		// modified has additional map
		case bothInBounds && originalString > modifiedString:

help func

`
func ItemAddedToModifiedSlice(original, modified string) { return originalString > modifiedString }

func ItemRemovedFromModifiedSlice(original, modified string) { return originalString < modifiedString }

func ItemMatchesOriginalAndModifiedSlice(original, modified string) { return originalString == modifiedString }

func CreateDeleteDirective(mergeKey: originalValue, directiveMarker: deleteDirective) { }
`

---

Comments from Reviewable

[pwittrock]

Review status: 0 of 3 files reviewed at latest revision, 21 unresolved discussions.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 604 at r1 (raw file):

			return nil, err
		}
		if shouldContinue {

should continue is vague. Call this skipProcessing or something.

---

Comments from Reviewable

[mengqiy]

Review status: 0 of 3 files reviewed at latest revision, 21 unresolved discussions.

---

staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go, line 30 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Make these functions with arguments.

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 57 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Comment what these are

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 161 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Pass in the casted value for modifiedValue

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 198 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add comments for arguments

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 200 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Cast this before passing in

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 211 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add a comment that the directive tells us to replace the entire object instead of diffing it

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 221 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add comment // Maps were not identical, use provided patch value

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 229 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

drop "Typed" piece of var names

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 231 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

pass in casted value

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 244 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// Merge the 2 slices using mergePatchKey

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 265 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Pass in the full options.

replacePatchFieldIfNotEqual

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 266 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

if ignoreChangesAndAdditions {
// Ignoring changes - do nothing
return
}
if reflect.DeepEqual(original, modified) {
// Contents are equal - do nothing
return
}
// Create a patch to replace the old value with the new one
patch[key] = modified

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 276 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add comment for original and modified

// original maybe either the live cluster object or the last applied configuration
// modified is always the users new config

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 277 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

if ignoreDeletions {
// Ignoring changes - do nothing
return
}
...

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 280 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

if _, found := modified[key]; !found {
patch[key] = nil
}

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 339 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

support either < or >

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 364 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

`
// If first return value is non-nil, list1 contains an element not present in list2
// If second return value is non-nil, list2 contains an element not present in list1
func compareListValuesAtIndex(list1Inbounds, list2Inbounds bool, list1Value, list2Value string) (interface{}, interface{}) {
switch {
case list1Inbounds && list2Inbounds:
if list1Value < list2Value...
else
case !list1Inbounds
return nil, list2Value
case !list2Inbounds
return list1Value, nil
}
}

`

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 371 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Boolean logic unclear (put parens)

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 407 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

originalElement, originalElementMergeKeyValue

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 438 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

help func

`
func ItemAddedToModifiedSlice(original, modified string) { return originalString > modifiedString }

func ItemRemovedFromModifiedSlice(original, modified string) { return originalString < modifiedString }

func ItemMatchesOriginalAndModifiedSlice(original, modified string) { return originalString == modifiedString }

func CreateDeleteDirective(mergeKey: originalValue, directiveMarker: deleteDirective) { }
`

Done.

---

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 604 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

should continue is vague. Call this skipProcessing or something.

Done.

---

Comments from Reviewable

[apelisse]

The number of if conditions and interface{} used here is quite scary. I wonder if both problems could be vastly improved by using an interface with a merge function and separate the logic in different types.

[apelisse]

I wonder if it would simplify the code to take expected, actual interface{} and the print the type with %T (instead of using reflect.TypeOf(obj).Kind().String()).

[mengqiy]

%T gives a type that is too concrete for us. Return a very concrete type may confuse the users sometimes.
But we can use reflect control which level of type we want to return to the users.

x := []byte("foo")
fmt.Printf("%T\n", x)
fmt.Printf(reflect.TypeOf(x).String())
fmt.Printf(reflect.TypeOf(x).Kind().String())

[]uint8
[]uint8
slice

[apelisse]

Yeah, thanks for pointing this out. TIL what typeOf(obj).Kind() does :-)

[apelisse]

Same comment as above

[apelisse]

I have a bad feeling about this function:

• It takes a lot of parameters
• Given a specific condition, it does only one simple thing (ignoring many of the parameters, not a good sign). And this is also done in the function below (exactly the same)
• Given the opposite condition, many of the types must be casted to different types

[mengqiy]

Given a specific condition, it does only one simple thing (ignoring many of the parameters, not a good sign). And this is also done in the function below (exactly the same)

This problem still exists. But I don't have a better way to that. Any suggestions?

[apelisse]

fieldType is only used to get elemType. Should we pass elemType instead?

[mengqiy]

I want to keep the signature of mergeSliceHandler and mergeSliceHandler similar. So I prefer to use fieldType

[apelisse]

You're assuming that original[key] can be casted to map[string]interface{} (no error checking) because it's been done by the calling function, but that's not obvious when you read this function. I think that could be simplified.

[mengqiy]

Added type assertion safety check.

[apelisse]

It's already much better to read, thanks!

[apelisse]

LGTM for me

[pwittrock]

/approve

[pwittrock]

/comment

[pwittrock]

/lgtm

[caesarxuchao]

/approve

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, pwittrock, ymqytw

Needs approval from an approver in each of these OWNERS Files:

• staging/src/k8s.io/apimachinery/pkg/OWNERS [caesarxuchao]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 44097, 42772, 43880, 44031, 44066)