Skip to content

Commit a82323e

Browse files
authored
Fix GetStructDiff to dereference non-nil pointers when the other side is omitted (#4766)
## Changes Fix GetStructDiff to dereference non-nil pointers when the other side is omitted ## Why When comparing a non-nil pointer field against a nil/omitted side (due to json omitempty), diffValues was storing the pointer value directly. But when both sides are non-nil, it recursively dereferences via the Pointer case. This inconsistency caused type mismatches (e.g., bool vs *bool) when comparing changes from two different diff calls. `localDiff` compares saved state vs current config. Both sides have `Started = *bool(false) (non-nil)`, so `diffValues` hits the case `reflect.Pointer` path and dereferences both: `diffValues(*bool(false), *bool(true)) → diffValues(bool(false), bool(true)) → Change{New: bool(true)}` `remoteDiff` compares remote state vs current config. The remote has no started field — in diffStruct, a nil *bool with omitempty gets replaced with `reflect.Value{}` (invalid). But the current config side is `*bool(true) (non-nil, non-zero)`, so it stays as a pointer. Then diffValues stores the pointer without dereferencing: `diffValues(invalid, *bool(true)) → Change{New: *bool(true)}` So localDiff produces `Change{New: bool(true)}` and remoteDiff produces `Change{New: *bool(true)}` for the same path. ## Tests <!-- How have you tested the changes? --> <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
1 parent 7b71d68 commit a82323e

File tree

6 files changed

+22
-17
lines changed

6 files changed

+22
-17
lines changed

libs/structs/structaccess/get.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,11 @@ func accessKey(v reflect.Value, key string, path *structpath.PathNode) (reflect.
141141
if fv.IsNil() {
142142
return reflect.Value{}, nil
143143
}
144-
// Non-nil pointer: check if the pointed-to value is empty for omitempty
145-
if isEmptyForOmitEmpty(fv.Elem()) {
146-
return reflect.Value{}, nil
147-
}
144+
// Non-nil pointer: return the dereferenced value.
145+
// JSON omitempty only omits nil pointers, not pointers to zero values.
146+
// Returning the dereferenced value is consistent with GetStructDiff,
147+
// which recursively dereferences non-nil pointers.
148+
return fv.Elem(), nil
148149
} else if isEmptyForOmitEmpty(fv) {
149150
return reflect.Value{}, nil
150151
}

libs/structs/structaccess/get_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -690,8 +690,9 @@ func TestGet_PointerToStructWithZeroValues(t *testing.T) {
690690
NestedNoOmit: &NestedStruct{ID: 0, Name: "", Count: 0},
691691
}
692692

693-
// The pointer was explicitly set, so it should return the struct even with zero values
694-
testGet(t, obj, "nested_omit", &NestedStruct{ID: 0, Name: "", Count: 0})
693+
// The pointer was explicitly set; it should return the dereferenced struct (not nil).
694+
// JSON omitempty only omits nil pointers, not pointers to zero values.
695+
testGet(t, obj, "nested_omit", NestedStruct{ID: 0, Name: "", Count: 0})
695696
testGet(t, obj, "nested_omit.id", int64(0))
696697
testGet(t, obj, "nested_omit.name", "")
697698
testGet(t, obj, "nested_omit.count", 0)
@@ -725,7 +726,7 @@ func TestGetJobSettings(t *testing.T) {
725726
},
726727
}
727728

728-
testGet(t, &jobSettings, "tasks[0].run_job_task", &jobs.RunJobTask{})
729+
testGet(t, &jobSettings, "tasks[0].run_job_task", jobs.RunJobTask{})
729730
testGet(t, &jobSettings, "tasks[0].run_job_task.job_id", int64(0))
730731
}
731732

libs/structs/structaccess/set_test.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,8 @@ func TestSet(t *testing.T) {
177177
expectedChanges: []structdiff.Change{
178178
{
179179
Path: structpath.MustParsePath("count"),
180-
Old: nil, // structdiff reports this as interface{}(nil)
181-
New: intPtr(42),
180+
Old: nil,
181+
New: 42, // diffValues dereferences non-nil pointers when the other side is nil/omitted
182182
},
183183
},
184184
},
@@ -494,10 +494,6 @@ func TestSet(t *testing.T) {
494494
}
495495
}
496496

497-
func intPtr(i int) *int {
498-
return &i
499-
}
500-
501497
// testSet sets a value and gets it back, asserting they're equal (roundtrip)
502498
func testSet(t *testing.T, obj any, path string, value any) {
503499
t.Helper()

libs/structs/structdiff/diff.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,11 +107,18 @@ func diffValues(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Valu
107107
if !v2.IsValid() {
108108
return nil
109109
}
110-
110+
// Dereference non-nil pointers for consistency with the both-non-nil case,
111+
// where pointers are recursively dereferenced via "case reflect.Pointer".
112+
for v2.Kind() == reflect.Pointer && !v2.IsNil() {
113+
v2 = v2.Elem()
114+
}
111115
*changes = append(*changes, Change{Path: path, Old: nil, New: v2.Interface()})
112116
return nil
113117
} else if !v2.IsValid() {
114118
// v1 is valid
119+
for v1.Kind() == reflect.Pointer && !v1.IsNil() {
120+
v1 = v1.Elem()
121+
}
115122
*changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: nil})
116123
return nil
117124
}

libs/structs/structdiff/diff_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func TestGetStructDiff(t *testing.T) {
136136
name: "pointer nil vs value",
137137
a: A{P: b1},
138138
b: A{},
139-
want: []ResolvedChange{{Field: "p", Old: b1, New: nil}},
139+
want: []ResolvedChange{{Field: "p", Old: B{S: "one"}, New: nil}},
140140
},
141141
{
142142
name: "pointer nested value diff",

libs/structs/structdiff/jobsettings_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,7 @@ func TestJobDiff(t *testing.T) {
497497

498498
// continous is completely deleted from jobExampleResponseNils
499499
assert.Equal(t, "continuous", changes[1].Path.String())
500-
assert.Equal(t, &jobs.Continuous{PauseStatus: "UNPAUSED"}, changes[1].Old)
500+
assert.Equal(t, jobs.Continuous{PauseStatus: "UNPAUSED"}, changes[1].Old)
501501
assert.Nil(t, changes[1].New)
502502

503503
// deployment.kind is not omitempty field, so it does not show up as nil here
@@ -516,7 +516,7 @@ func TestJobDiff(t *testing.T) {
516516
assert.Equal(t, "", changes[0].Old)
517517
assert.Nil(t, changes[0].New)
518518
assert.Equal(t, "continuous", changes[1].Path.String())
519-
assert.Equal(t, &jobs.Continuous{}, changes[1].Old)
519+
assert.Equal(t, jobs.Continuous{}, changes[1].Old)
520520
assert.Nil(t, changes[1].New)
521521

522522
// deployment.kind is "" in both

0 commit comments

Comments
 (0)