Skip to content

Commit 1bad841

Browse files
denikdenik
authored
structdiff: construct paths on based on json tag (#2954)
## Changes Switch structdiff to (lazily) compute jsontag-based path instead of Golang field names. Follow up to #2928 ## Why This is closer to what user is going to want to see in "bundle diff". This is also useful if we want to look up the path in dyn.Value world. ## Benchmark This PR: ``` ~/work/cli-main/libs/structdiff % go test -bench=. -benchmem -run ^x -benchtime=10s goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 315444 36290 ns/op 18040 B/op 540 allocs/op BenchmarkChanges-16 307059 38747 ns/op 20704 B/op 598 allocs/op BenchmarkZero-16 260900 45907 ns/op 30456 B/op 843 allocs/op BenchmarkNils-16 365086 33155 ns/op 24034 B/op 572 allocs/op ``` main: ``` ~/work/cli-main/libs/structdiff % go test -bench=. -benchmem -run ^x -benchtime=10s goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 311192 36944 ns/op 13368 B/op 540 allocs/op BenchmarkChanges-16 310232 39032 ns/op 16008 B/op 598 allocs/op BenchmarkZero-16 264555 45187 ns/op 26011 B/op 843 allocs/op BenchmarkNils-16 363981 32819 ns/op 20542 B/op 572 allocs/op ```
1 parent f156cac commit 1bad841

File tree

5 files changed

+222
-63
lines changed

5 files changed

+222
-63
lines changed

‎libs/structdiff/diff.go‎

Lines changed: 48 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ import (
66
"slices"
77
"sort"
88
"strconv"
9-
"strings"
9+
10+
"github.com/databricks/cli/libs/structdiff/jsontag"
1011
)
1112

1213
type Change struct {
@@ -16,25 +17,40 @@ type Change struct {
1617
}
1718

1819
type pathNode struct {
19-
Prev *pathNode
20-
Key string
21-
// If Index >= 0, the node specifies a slice/array index in Index.
22-
// If Index == -1, the node specifies a struct attribute in Key
23-
// If Index == -2, the node specifies a map key in Key
24-
Index int
20+
prev *pathNode
21+
jsonTag jsontag.JSONTag // For lazy JSON key resolution
22+
key string // Computed key (JSON key for structs, string key for maps, or Go field name for fallback)
23+
// If index >= 0, the node specifies a slice/array index in index.
24+
// If index == -1, the node specifies a struct attribute
25+
// If index == -2, the node specifies a map key in key
26+
// If index == -3, the node specifies an unresolved struct attribute
27+
index int
2528
}
2629

2730
func (p *pathNode) String() string {
2831
if p == nil {
2932
return ""
3033
}
31-
if p.Index >= 0 {
32-
return p.Prev.String() + "[" + strconv.Itoa(p.Index) + "]"
34+
35+
if p.index >= 0 {
36+
return p.prev.String() + "[" + strconv.Itoa(p.index) + "]"
3337
}
34-
if p.Index == -1 {
35-
return p.Prev.String() + "." + p.Key
38+
39+
if p.index == -3 {
40+
// Lazy resolve JSON key for struct fields
41+
jsonName := p.jsonTag.Name()
42+
if jsonName != "" {
43+
p.key = jsonName
44+
}
45+
// If jsonName is empty, key already contains the Go field name as fallback
46+
p.index = -1
3647
}
37-
return fmt.Sprintf("%s[%q]", p.Prev.String(), p.Key)
48+
49+
if p.index == -1 {
50+
return p.prev.String() + "." + p.key
51+
}
52+
53+
return fmt.Sprintf("%s[%q]", p.prev.String(), p.key)
3854
}
3955

4056
// GetStructDiff compares two Go structs and returns a list of Changes or an error.
@@ -112,7 +128,7 @@ func diffValues(path *pathNode, v1, v2 reflect.Value, changes *[]Change) {
112128
*changes = append(*changes, Change{Field: path.String(), Old: v1.Interface(), New: v2.Interface()})
113129
} else {
114130
for i := range v1.Len() {
115-
node := pathNode{Prev: path, Index: i}
131+
node := pathNode{prev: path, index: i}
116132
diffValues(&node, v1.Index(i), v2.Index(i), changes)
117133
}
118134
}
@@ -144,21 +160,27 @@ func diffStruct(path *pathNode, s1, s2 reflect.Value, changes *[]Change) {
144160
continue
145161
}
146162

147-
node := pathNode{Prev: path, Key: sf.Name, Index: -1}
163+
// Store JSONTag and Go field name for lazy JSON key resolution
164+
node := pathNode{prev: path, jsonTag: jsontag.JSONTag(sf.Tag.Get("json")), key: sf.Name, index: -3}
148165
v1Field := s1.Field(i)
149166
v2Field := s2.Field(i)
150167

151-
hasOmitEmpty := strings.Contains(sf.Tag.Get("json"), "omitempty")
168+
zero1 := v1Field.IsZero()
169+
zero2 := v2Field.IsZero()
152170

153-
if hasOmitEmpty {
154-
if v1Field.IsZero() {
155-
if !slices.Contains(forced1, sf.Name) {
156-
v1Field = reflect.ValueOf(nil)
171+
if zero1 || zero2 {
172+
hasOmitEmpty := node.jsonTag.OmitEmpty()
173+
174+
if hasOmitEmpty {
175+
if zero1 {
176+
if !slices.Contains(forced1, sf.Name) {
177+
v1Field = reflect.ValueOf(nil)
178+
}
157179
}
158-
}
159-
if v2Field.IsZero() {
160-
if !slices.Contains(forced2, sf.Name) {
161-
v2Field = reflect.ValueOf(nil)
180+
if zero2 {
181+
if !slices.Contains(forced2, sf.Name) {
182+
v2Field = reflect.ValueOf(nil)
183+
}
162184
}
163185
}
164186
}
@@ -190,9 +212,9 @@ func diffMapStringKey(path *pathNode, m1, m2 reflect.Value, changes *[]Change) {
190212
v1 := m1.MapIndex(k)
191213
v2 := m2.MapIndex(k)
192214
node := pathNode{
193-
Prev: path,
194-
Key: ks,
195-
Index: -2,
215+
prev: path,
216+
key: ks,
217+
index: -2,
196218
}
197219
diffValues(&node, v1, v2, changes)
198220
}

‎libs/structdiff/diff_test.go‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -79,112 +79,112 @@ func TestGetStructDiff(t *testing.T) {
7979
name: "simple field change - omitempty",
8080
a: A{X: 5},
8181
b: A{},
82-
want: []Change{{Field: ".X", Old: 5, New: nil}},
82+
want: []Change{{Field: ".x", Old: 5, New: nil}},
8383
},
8484
{
8585
name: "simple field change - required",
8686
a: A{XX: 5},
8787
b: A{},
88-
want: []Change{{Field: ".XX", Old: 5, New: 0}},
88+
want: []Change{{Field: ".xx", Old: 5, New: 0}},
8989
},
9090
{
9191
name: "nested struct field",
9292
a: A{B: B{S: "one"}},
9393
b: A{B: B{S: "two"}},
94-
want: []Change{{Field: ".B.S", Old: "one", New: "two"}},
94+
want: []Change{{Field: ".b.S", Old: "one", New: "two"}},
9595
},
9696
{
9797
name: "pointer nil vs value",
9898
a: A{P: b1},
9999
b: A{},
100-
want: []Change{{Field: ".P", Old: b1, New: nil}},
100+
want: []Change{{Field: ".p", Old: b1, New: nil}},
101101
},
102102
{
103103
name: "pointer nested value diff",
104104
a: A{P: b1},
105105
b: A{P: b2},
106-
want: []Change{{Field: ".P.S", Old: "one", New: "two"}},
106+
want: []Change{{Field: ".p.S", Old: "one", New: "two"}},
107107
},
108108
{
109109
name: "map diff",
110110
a: A{M: map[string]int{"a": 1}},
111111
b: A{M: map[string]int{"a": 2}},
112-
want: []Change{{Field: ".M[\"a\"]", Old: 1, New: 2}},
112+
want: []Change{{Field: ".m[\"a\"]", Old: 1, New: 2}},
113113
},
114114
{
115115
name: "slice diff",
116116
a: A{L: []string{"a"}},
117117
b: A{L: []string{"a", "b"}},
118-
want: []Change{{Field: ".L", Old: []string{"a"}, New: []string{"a", "b"}}},
118+
want: []Change{{Field: ".l", Old: []string{"a"}, New: []string{"a", "b"}}},
119119
},
120120

121121
// ForceSendFields with non-empty fields (omitempty)
122122
{
123123
name: "forcesend nonempty 1",
124124
a: C{Name: "Hello", ForceSendFields: []string{"Name"}},
125125
b: C{Name: "World"},
126-
want: []Change{{Field: ".Name", Old: "Hello", New: "World"}},
126+
want: []Change{{Field: ".name", Old: "Hello", New: "World"}},
127127
},
128128
{
129129
name: "forcesend noneempty 2",
130130
a: C{Name: "Hello", ForceSendFields: []string{"Name"}},
131131
b: C{Name: "World", ForceSendFields: []string{"Name"}},
132-
want: []Change{{Field: ".Name", Old: "Hello", New: "World"}},
132+
want: []Change{{Field: ".name", Old: "Hello", New: "World"}},
133133
},
134134
{
135135
name: "forcesend noneempty 3",
136136
a: C{Name: "Hello"},
137137
b: C{Name: "World", ForceSendFields: []string{"Name"}},
138-
want: []Change{{Field: ".Name", Old: "Hello", New: "World"}},
138+
want: []Change{{Field: ".name", Old: "Hello", New: "World"}},
139139
},
140140

141141
// ForceSendFields with non-empty fields (required)
142142
{
143143
name: "forcesend nonempty required 1",
144144
a: C{Title: "Hello", ForceSendFields: []string{"Title"}},
145145
b: C{Title: "World"},
146-
want: []Change{{Field: ".Title", Old: "Hello", New: "World"}},
146+
want: []Change{{Field: ".title", Old: "Hello", New: "World"}},
147147
},
148148
{
149149
name: "forcesend noneempty required 2",
150150
a: C{Title: "Hello", ForceSendFields: []string{"Title"}},
151151
b: C{Title: "World", ForceSendFields: []string{"Title"}},
152-
want: []Change{{Field: ".Title", Old: "Hello", New: "World"}},
152+
want: []Change{{Field: ".title", Old: "Hello", New: "World"}},
153153
},
154154
{
155155
name: "forcesend noneempty required 3",
156156
a: C{Title: "Hello"},
157157
b: C{Title: "World", ForceSendFields: []string{"Title"}},
158-
want: []Change{{Field: ".Title", Old: "Hello", New: "World"}},
158+
want: []Change{{Field: ".title", Old: "Hello", New: "World"}},
159159
},
160160

161161
// ForceSendFields with empty fields
162162
{
163163
name: "forcesend empty string diff",
164164
a: C{ForceSendFields: []string{"Name"}}, // Name == "" zero, but forced
165165
b: C{},
166-
want: []Change{{Field: ".Name", Old: "", New: nil}},
166+
want: []Change{{Field: ".name", Old: "", New: nil}},
167167
},
168168
{
169169
name: "forcesend empty int diff",
170170
a: C{ForceSendFields: []string{"Age"}},
171171
b: C{},
172-
want: []Change{{Field: ".Age", Old: 0, New: nil}},
172+
want: []Change{{Field: ".age", Old: 0, New: nil}},
173173
},
174174
{
175175
name: "forcesend empty bool diff",
176176
a: C{ForceSendFields: []string{"IsEnabled"}},
177177
b: C{},
178-
want: []Change{{Field: ".IsEnabled", Old: false, New: nil}},
178+
want: []Change{{Field: ".is_enabled", Old: false, New: nil}},
179179
},
180180
{
181181
name: "forcesend empty all",
182182
a: C{ForceSendFields: []string{"Name", "IsEnabled"}},
183183
b: C{ForceSendFields: []string{"Age"}},
184184
want: []Change{
185-
{Field: ".Name", Old: "", New: nil},
186-
{Field: ".Age", Old: nil, New: 0},
187-
{Field: ".IsEnabled", Old: false, New: nil},
185+
{Field: ".name", Old: "", New: nil},
186+
{Field: ".age", Old: nil, New: 0},
187+
{Field: ".is_enabled", Old: false, New: nil},
188188
},
189189
},
190190
{
@@ -211,7 +211,7 @@ func TestGetStructDiff(t *testing.T) {
211211
name: "slice of struct with empty string and ForceSendFields diff",
212212
a: []C{{Name: "", ForceSendFields: []string{"Name"}}},
213213
b: []C{{Name: ""}},
214-
want: []Change{{Field: "[0].Name", Old: "", New: nil}},
214+
want: []Change{{Field: "[0].name", Old: "", New: nil}},
215215
},
216216

217217
// ForceSendFields inside map value
@@ -220,9 +220,9 @@ func TestGetStructDiff(t *testing.T) {
220220
a: map[string]C{"key1": {Title: "title", ForceSendFields: []string{"Name", "IsEnabled", "Title"}}},
221221
b: map[string]C{"key1": {Title: "title", ForceSendFields: []string{"Age"}}},
222222
want: []Change{
223-
{Field: "[\"key1\"].Name", Old: "", New: nil},
224-
{Field: "[\"key1\"].Age", Old: nil, New: 0},
225-
{Field: "[\"key1\"].IsEnabled", Old: false, New: nil},
223+
{Field: "[\"key1\"].name", Old: "", New: nil},
224+
{Field: "[\"key1\"].age", Old: nil, New: 0},
225+
{Field: "[\"key1\"].is_enabled", Old: false, New: nil},
226226
},
227227
},
228228

@@ -232,9 +232,9 @@ func TestGetStructDiff(t *testing.T) {
232232
a: map[string]*C{"key1": {Title: "title", ForceSendFields: []string{"Name", "IsEnabled", "Title"}}},
233233
b: map[string]*C{"key1": {Title: "title", ForceSendFields: []string{"Age"}}},
234234
want: []Change{
235-
{Field: "[\"key1\"].Name", Old: "", New: nil},
236-
{Field: "[\"key1\"].Age", Old: nil, New: 0},
237-
{Field: "[\"key1\"].IsEnabled", Old: false, New: nil},
235+
{Field: "[\"key1\"].name", Old: "", New: nil},
236+
{Field: "[\"key1\"].age", Old: nil, New: 0},
237+
{Field: "[\"key1\"].is_enabled", Old: false, New: nil},
238238
},
239239
},
240240
}

‎libs/structdiff/jobsettings_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -474,32 +474,32 @@ func TestJobDiff(t *testing.T) {
474474
changes, err := GetStructDiff(job, zero)
475475
require.NoError(t, err)
476476
require.GreaterOrEqual(t, len(changes), 75)
477-
assert.Equal(t, Change{Field: ".BudgetPolicyId", Old: "550e8400-e29b-41d4-a716-446655440000", New: ""}, changes[0])
477+
assert.Equal(t, Change{Field: ".budget_policy_id", Old: "550e8400-e29b-41d4-a716-446655440000", New: ""}, changes[0])
478478
// Note: pause_status shows up as nil here because Continous does not have ForceSendFields field
479-
assert.Equal(t, Change{Field: ".Continuous.PauseStatus", Old: jobs.PauseStatus("UNPAUSED"), New: nil}, changes[1], "job vs zero: %#v %#v", job.Continuous, zero.Continuous)
480-
assert.Equal(t, Change{Field: ".Deployment.Kind", Old: jobs.JobDeploymentKind("BUNDLE"), New: jobs.JobDeploymentKind("")}, changes[2])
481-
assert.Equal(t, Change{Field: ".Deployment.MetadataFilePath", Old: "string", New: ""}, changes[3])
479+
assert.Equal(t, Change{Field: ".continuous.pause_status", Old: jobs.PauseStatus("UNPAUSED"), New: nil}, changes[1], "job vs zero: %#v %#v", job.Continuous, zero.Continuous)
480+
assert.Equal(t, Change{Field: ".deployment.kind", Old: jobs.JobDeploymentKind("BUNDLE"), New: jobs.JobDeploymentKind("")}, changes[2])
481+
assert.Equal(t, Change{Field: ".deployment.metadata_file_path", Old: "string", New: ""}, changes[3])
482482

483483
changes, err = GetStructDiff(job, nils)
484484
require.NoError(t, err)
485485
require.GreaterOrEqual(t, len(changes), 77)
486-
assert.Equal(t, Change{Field: ".BudgetPolicyId", Old: "550e8400-e29b-41d4-a716-446655440000", New: nil}, changes[0])
486+
assert.Equal(t, Change{Field: ".budget_policy_id", Old: "550e8400-e29b-41d4-a716-446655440000", New: nil}, changes[0])
487487

488488
// continous is completely deleted from jobExampleResponseNils
489-
assert.Equal(t, Change{Field: ".Continuous", Old: &jobs.Continuous{PauseStatus: "UNPAUSED"}, New: nil}, changes[1])
489+
assert.Equal(t, Change{Field: ".continuous", Old: &jobs.Continuous{PauseStatus: "UNPAUSED"}, New: nil}, changes[1])
490490

491491
// deployment.kind is not omitempty field, so it does not show up as nil here
492-
assert.Equal(t, Change{Field: ".Deployment.Kind", Old: jobs.JobDeploymentKind("BUNDLE"), New: jobs.JobDeploymentKind("")}, changes[2])
492+
assert.Equal(t, Change{Field: ".deployment.kind", Old: jobs.JobDeploymentKind("BUNDLE"), New: jobs.JobDeploymentKind("")}, changes[2])
493493

494-
assert.Equal(t, Change{Field: ".Deployment.MetadataFilePath", Old: "string", New: nil}, changes[3])
494+
assert.Equal(t, Change{Field: ".deployment.metadata_file_path", Old: "string", New: nil}, changes[3])
495495

496496
changes, err = GetStructDiff(zero, nils)
497497
require.NoError(t, err)
498498
assert.GreaterOrEqual(t, len(changes), 58)
499-
assert.Equal(t, Change{Field: ".BudgetPolicyId", Old: "", New: nil}, changes[0])
500-
assert.Equal(t, Change{Field: ".Continuous", Old: &jobs.Continuous{}, New: nil}, changes[1])
499+
assert.Equal(t, Change{Field: ".budget_policy_id", Old: "", New: nil}, changes[0])
500+
assert.Equal(t, Change{Field: ".continuous", Old: &jobs.Continuous{}, New: nil}, changes[1])
501501

502502
// deployment.kind is "" in both
503503

504-
assert.Equal(t, Change{Field: ".Deployment.MetadataFilePath", Old: "", New: nil}, changes[2])
504+
assert.Equal(t, Change{Field: ".deployment.metadata_file_path", Old: "", New: nil}, changes[2])
505505
}

‎libs/structdiff/jsontag/jsontag.go‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
package jsontag
2+
3+
import "strings"
4+
5+
// JSONTag represents a struct field's `json` tag as a string.
6+
// It provides methods to lazily extract information from the tag.
7+
type JSONTag string
8+
9+
// Name returns the field name that JSON should use.
10+
// Returns "" if no name is specified (meaning "keep Go field name"),
11+
// or "-" if the field should be skipped.
12+
func (tag JSONTag) Name() string {
13+
s := string(tag)
14+
if s == "" {
15+
return ""
16+
}
17+
18+
if idx := strings.IndexByte(s, ','); idx == -1 {
19+
// Whole tag is just the name
20+
return s
21+
} else {
22+
return s[:idx]
23+
}
24+
}
25+
26+
func (tag JSONTag) OmitEmpty() bool {
27+
return tag.hasOption("omitempty")
28+
}
29+
30+
func (tag JSONTag) OmitZero() bool {
31+
return tag.hasOption("omitzero")
32+
}
33+
34+
func (tag JSONTag) hasOption(option string) bool {
35+
s := string(tag)
36+
if s == "" {
37+
return false
38+
}
39+
40+
// Skip the name part
41+
if idx := strings.IndexByte(s, ','); idx == -1 {
42+
// No options, just name
43+
return false
44+
} else {
45+
s = s[idx+1:]
46+
}
47+
48+
// Walk the comma-separated options
49+
for len(s) > 0 {
50+
opt := s
51+
if i := strings.IndexByte(s, ','); i != -1 {
52+
opt, s = s[:i], s[i+1:]
53+
} else {
54+
s = ""
55+
}
56+
57+
if opt == option {
58+
return true
59+
}
60+
}
61+
return false
62+
}

0 commit comments

Comments
 (0)