From 78d141eb55d586a65f14a525a1994e87405bdf47 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Sun, 30 Aug 2026 19:42:49 +0200 Subject: [PATCH 01/28] structaccess: reach fields of a doubly-embedded struct FindStructFieldByKeyType already recursed into embedded structs, but Get and Set stopped after one level, so a field of resources.PostgresProject -- which embeds a config struct that embeds the SDK spec -- was reported as not found. Both now walk embedding recursively and track the struct that declares the field, which is also the one whose ForceSendFields governs it: an outer struct that shadows the name (PostgresProjectConfig) tracks only its own fields. Co-authored-by: Isaac --- libs/structs/structaccess/bundle_test.go | 34 ++++++++++ libs/structs/structaccess/get.go | 84 ++++++++---------------- libs/structs/structaccess/set.go | 64 ++---------------- 3 files changed, 68 insertions(+), 114 deletions(-) diff --git a/libs/structs/structaccess/bundle_test.go b/libs/structs/structaccess/bundle_test.go index 1d75afe0b10..09c39b57122 100644 --- a/libs/structs/structaccess/bundle_test.go +++ b/libs/structs/structaccess/bundle_test.go @@ -76,3 +76,37 @@ func TestGet_ConfigRoot_JobTagsAccess(t *testing.T) { require.Error(t, ValidateByString(reflect.TypeFor[config.Root](), "resources.apps.my_app.url.inner")) require.Error(t, ValidateByString(reflect.TypeFor[config.Root](), "resources.apps.my_app.url1")) } + +// A bundle resource embeds a config struct that embeds the SDK request struct, so its +// fields sit two levels down. Get, Set and ValidatePath all have to reach them, and +// ForceSendFields belongs to the struct that declares the field -- not to the outer one +// that shadows the name. +func TestGetSet_DoublyEmbeddedField(t *testing.T) { + project := &resources.PostgresProject{} //exhaustruct:ignore + project.ProjectId = "p" + + require.NoError(t, ValidateByString(reflect.TypeOf(project), "budget_policy_id")) + + require.NoError(t, SetByString(project, "budget_policy_id", "abc")) + require.Equal(t, "abc", project.BudgetPolicyId) + + value, err := GetByString(project, "budget_policy_id") + require.NoError(t, err) + require.Equal(t, "abc", value) + + // An explicit empty value is recorded on ProjectSpec, which declares the field. + require.NoError(t, SetByString(project, "budget_policy_id", "")) + require.Contains(t, project.ProjectSpec.ForceSendFields, "BudgetPolicyId") + require.NotContains(t, project.PostgresProjectConfig.ForceSendFields, "BudgetPolicyId") + + value, err = GetByString(project, "budget_policy_id") + require.NoError(t, err) + require.Equal(t, "", value) + + // And dropping it again leaves the field absent. + require.NoError(t, SetByString(project, "budget_policy_id", nil)) + require.NotContains(t, project.ProjectSpec.ForceSendFields, "BudgetPolicyId") + value, err = GetByString(project, "budget_policy_id") + require.NoError(t, err) + require.Nil(t, value) +} diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index a5433adee71..a4416af3aa0 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -138,19 +138,13 @@ func Get(v any, path *structpath.PathNode) (any, error) { func accessKey(v reflect.Value, key string, path *structpath.PathNode) (reflect.Value, error) { switch v.Kind() { case reflect.Struct: - // Precalculate ForceSendFields mappings for this struct hierarchy - forceSendFieldsMap := getForceSendFieldsForFromTyped(v) - - fv, sf, embeddedIndex, ok := findStructFieldByKey(v, key) + fv, sf, owner, ok := findStructFieldByKey(v, key) if !ok { return reflect.Value{}, fmt.Errorf("%s: field %q not found in %s", path.String(), key, v.Type()) } - // Check ForceSendFields using precalculated map - var force bool - if fields, exists := forceSendFieldsMap[embeddedIndex]; exists { - force = containsString(fields, sf.Name) - } + // ForceSendFields is only managed by the struct that declares the field. + force := forceSendFieldsContains(owner, sf.Name) // Honor omitempty: if present and value is empty and not forced, treat as omitted (nil). jsonTag := structtag.JSONTag(sf.Tag.Get("json")) @@ -270,15 +264,16 @@ func findFieldInStruct(v reflect.Value, key string) (reflect.Value, reflect.Stru // findStructFieldByKey searches exported fields of struct v for a field matching key. // It matches json tag name (when present and not "-") only. -// It also searches embedded anonymous structs (flattening semantics). -// Returns: fieldValue, structField, embeddedIndex, found -// embeddedIndex is -1 for direct fields, or the index of the embedded struct containing the field. -func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.StructField, int, bool) { +// It also searches embedded anonymous structs recursively (flattening semantics), which +// FindStructFieldByKeyType does too: a bundle resource embeds a config struct that embeds +// the SDK request struct, so its fields sit two levels down. +// Returns: fieldValue, structField, owner (the struct value declaring the field), found +func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.StructField, reflect.Value, bool) { t := v.Type() // First pass: direct fields if fv, sf, found := findFieldInStruct(v, key); found { - return fv, sf, -1, true + return fv, sf, v, true } // Second pass: search embedded anonymous structs (flattening semantics) @@ -299,59 +294,38 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S if fv.Kind() != reflect.Struct { continue } - if out, osf, found := findFieldInStruct(fv, key); found { - return out, osf, i, true + if out, osf, owner, found := findStructFieldByKey(fv, key); found { + return out, osf, owner, true } } - return reflect.Value{}, reflect.StructField{}, -1, false + return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false } -// getForceSendFieldsForFromTyped collects ForceSendFields values for FromTyped operations -// Returns map[structKey][]fieldName where structKey is -1 for direct fields, embedded index for embedded fields -func getForceSendFieldsForFromTyped(v reflect.Value) map[int][]string { - if !v.IsValid() || v.Type().Kind() != reflect.Struct { - return make(map[int][]string) +// forceSendFields returns the ForceSendFields slice a struct declares itself. A struct that +// embeds another shadows it deliberately -- see resources.PostgresProjectConfig -- so only +// the declaring struct tracks a field of its own. +func forceSendFields(owner reflect.Value) reflect.Value { + if !owner.IsValid() || owner.Kind() != reflect.Struct { + return reflect.Value{} } - - result := make(map[int][]string) - - for i := range v.Type().NumField() { - field := v.Type().Field(i) - fieldValue := v.Field(i) - + for i := range owner.Type().NumField() { + field := owner.Type().Field(i) if field.Name == "ForceSendFields" && !field.Anonymous { - // Direct ForceSendFields (structKey = -1) - if fields, ok := reflect.TypeAssert[[]string](fieldValue); ok { - result[-1] = fields - } - } else if field.Anonymous { - // Embedded struct - check for ForceSendFields inside it - if embeddedStruct := getEmbeddedStructForReading(fieldValue); embeddedStruct.IsValid() { - if forceSendField := embeddedStruct.FieldByName("ForceSendFields"); forceSendField.IsValid() { - if fields, ok := reflect.TypeAssert[[]string](forceSendField); ok { - result[i] = fields - } - } - } + return owner.Field(i) } } - - return result + return reflect.Value{} } -// Helper function for reading - doesn't create nil pointers -func getEmbeddedStructForReading(fieldValue reflect.Value) reflect.Value { - if fieldValue.Kind() == reflect.Pointer { - if fieldValue.IsNil() { - return reflect.Value{} // Don't create, just return invalid - } - fieldValue = fieldValue.Elem() - } - if fieldValue.Kind() == reflect.Struct { - return fieldValue +// forceSendFieldsContains reports whether a struct forces the named field to be sent. +func forceSendFieldsContains(owner reflect.Value, name string) bool { + fsf := forceSendFields(owner) + if !fsf.IsValid() { + return false } - return reflect.Value{} + fields, ok := reflect.TypeAssert[[]string](fsf) + return ok && containsString(fields, name) } // containsString checks if a slice contains a specific string diff --git a/libs/structs/structaccess/set.go b/libs/structs/structaccess/set.go index 2ed2a64f8e2..3247b2a0373 100644 --- a/libs/structs/structaccess/set.go +++ b/libs/structs/structaccess/set.go @@ -130,7 +130,7 @@ func setFieldOrMapValue(parentVal reflect.Value, key string, valueVal reflect.Va // setStructField sets a field in a struct and handles ForceSendFields func setStructField(parentVal reflect.Value, fieldName string, valueVal reflect.Value) error { - fv, sf, embeddedIndex, ok := findStructFieldByKey(parentVal, fieldName) + fv, sf, owner, ok := findStructFieldByKey(parentVal, fieldName) if !ok { return fmt.Errorf("field %q not found in %s", fieldName, parentVal.Type()) } @@ -155,7 +155,7 @@ func setStructField(parentVal reflect.Value, fieldName string, valueVal reflect. if !valueVal.IsValid() { // Setting nil: the field is being made absent, which convertValue renders as the zero // value. Pass the invalid value through so it is removed from ForceSendFields. - return updateForceSendFields(parentVal, sf.Name, embeddedIndex, valueVal, sf) + return updateForceSendFields(owner, sf.Name, valueVal, sf) } return updateForceSendFields(parentVal, sf.Name, embeddedIndex, converted, sf) } @@ -314,7 +314,7 @@ func convertValue(valueVal reflect.Value, targetType reflect.Type) (reflect.Valu // - If setting nil: remove field from ForceSendFields // - If setting empty value: add field to ForceSendFields (if not already present) // Only applies to fields with omitempty tag -func updateForceSendFields(parentVal reflect.Value, fieldName string, embeddedIndex int, valueVal reflect.Value, structField reflect.StructField) error { +func updateForceSendFields(owner reflect.Value, fieldName string, valueVal reflect.Value, structField reflect.StructField) error { isSettingNil := !valueVal.IsValid() isSettingEmptyValue := valueVal.IsValid() && isEmptyForOmitEmpty(valueVal) @@ -330,8 +330,8 @@ func updateForceSendFields(parentVal reflect.Value, fieldName string, embeddedIn return nil } - // Find the appropriate ForceSendFields slice to modify - forceSendFieldsSlice := findForceSendFieldsForSetting(parentVal, embeddedIndex) + // Only the struct that declares the field tracks it. + forceSendFieldsSlice := forceSendFields(owner) if !forceSendFieldsSlice.IsValid() { // No ForceSendFields to update return nil @@ -348,60 +348,6 @@ func updateForceSendFields(parentVal reflect.Value, fieldName string, embeddedIn return nil } -// findForceSendFieldsForSetting finds the correct ForceSendFields slice to modify -// This should match the logic in get.go's getForceSendFieldsForFromTyped -// Only the struct that contains the ForceSendFields can manage its own fields -// embeddedIndex: -1 for direct fields, or the index of the embedded struct -func findForceSendFieldsForSetting(parentVal reflect.Value, embeddedIndex int) reflect.Value { - if embeddedIndex == -1 { - // Direct field - check if parent struct has its own ForceSendFields - // We need to check the struct type directly, not through field promotion - parentType := parentVal.Type() - for i := range parentType.NumField() { - field := parentType.Field(i) - if field.Name == "ForceSendFields" && !field.Anonymous { - // Parent has direct ForceSendFields - return parentVal.Field(i) - } - } - // Parent struct has no direct ForceSendFields, so no management possible - return reflect.Value{} - } else { - // Embedded field - look for ForceSendFields in the embedded struct - embeddedField := parentVal.Field(embeddedIndex) - embeddedStruct := getEmbeddedStructForSetting(embeddedField) - if !embeddedStruct.IsValid() { - return reflect.Value{} - } - fsf := embeddedStruct.FieldByName("ForceSendFields") - if fsf.IsValid() { - return fsf - } - // Embedded struct has no ForceSendFields, so no management possible - return reflect.Value{} - } -} - -// getEmbeddedStructForSetting gets the embedded struct for setting operations -// Creates nil pointers if needed -func getEmbeddedStructForSetting(fieldValue reflect.Value) reflect.Value { - if fieldValue.Kind() == reflect.Pointer { - if fieldValue.IsNil() { - // Create new instance if needed - if fieldValue.CanSet() { - fieldValue.Set(reflect.New(fieldValue.Type().Elem())) - } else { - return reflect.Value{} - } - } - fieldValue = fieldValue.Elem() - } - if fieldValue.Kind() == reflect.Struct { - return fieldValue - } - return reflect.Value{} -} - // removeFromForceSendFields removes fieldName from the ForceSendFields slice func removeFromForceSendFields(forceSendFieldsSlice reflect.Value, fieldName string) { // Get the original []string slice From 89381227783036c455553d655bf23fecaa450b80 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 31 Aug 2026 13:56:22 +0200 Subject: [PATCH 02/28] structaccess: resolve embedded fields breadth-first, as encoding/json does From the adversarial review. Get and Set searched embedded structs depth-first, so a name declared at two embedding depths could resolve to the deeper field -- while json.Marshal picks the shallower one. Reading and writing the field would then not be the field that gets serialized under that name. The search now goes one level of embedding at a time, and the test fails on the old behaviour. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 40 +++++++++++++++++------- libs/structs/structaccess/set.go | 2 +- libs/structs/structaccess/set_test.go | 45 +++++++++++++++++---------- 3 files changed, 58 insertions(+), 29 deletions(-) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index a4416af3aa0..571641e9ee3 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -269,24 +269,43 @@ func findFieldInStruct(v reflect.Value, key string) (reflect.Value, reflect.Stru // the SDK request struct, so its fields sit two levels down. // Returns: fieldValue, structField, owner (the struct value declaring the field), found func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.StructField, reflect.Value, bool) { - t := v.Type() - // First pass: direct fields if fv, sf, found := findFieldInStruct(v, key); found { return fv, sf, v, true } - // Second pass: search embedded anonymous structs (flattening semantics) + // Second pass: search embedded anonymous structs (flattening semantics), one level of + // embedding at a time. Breadth-first, not depth-first: encoding/json resolves a name + // declared at two embedding depths in favour of the shallower one, so a search that + // descended fully into the first embed could pick a deeper field than the one that is + // actually serialized under that name. + for _, fv := range embeddedStructs(v) { + if out, sf, found := findFieldInStruct(fv, key); found { + return out, sf, fv, true + } + } + for _, fv := range embeddedStructs(v) { + if out, sf, owner, found := findStructFieldByKey(fv, key); found { + return out, sf, owner, true + } + } + + return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false +} + +// embeddedStructs returns the anonymous struct fields of v, dereferenced, skipping any that +// cannot be descended into. +func embeddedStructs(v reflect.Value) []reflect.Value { + var out []reflect.Value + t := v.Type() for i := range t.NumField() { - sf := t.Field(i) - if !sf.Anonymous { + if !t.Field(i).Anonymous { continue } fv := v.Field(i) - // Dereference pointer anonymous structs for fv.Kind() == reflect.Pointer { if fv.IsNil() { - // Not initialized; can't descend + // Not initialized; can't descend. break } fv = fv.Elem() @@ -294,12 +313,9 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S if fv.Kind() != reflect.Struct { continue } - if out, osf, owner, found := findStructFieldByKey(fv, key); found { - return out, osf, owner, true - } + out = append(out, fv) } - - return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false + return out } // forceSendFields returns the ForceSendFields slice a struct declares itself. A struct that diff --git a/libs/structs/structaccess/set.go b/libs/structs/structaccess/set.go index 3247b2a0373..02752e456ea 100644 --- a/libs/structs/structaccess/set.go +++ b/libs/structs/structaccess/set.go @@ -157,7 +157,7 @@ func setStructField(parentVal reflect.Value, fieldName string, valueVal reflect. // value. Pass the invalid value through so it is removed from ForceSendFields. return updateForceSendFields(owner, sf.Name, valueVal, sf) } - return updateForceSendFields(parentVal, sf.Name, embeddedIndex, converted, sf) + return updateForceSendFields(owner, sf.Name, converted, sf) } // setMapValue sets a value in a map diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 9144c61c3cd..da8cf3ff19c 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -793,26 +793,39 @@ func TestSet_MixedForceSendFields(t *testing.T) { }) } -// A value that cannot be converted must leave the struct untouched, ForceSendFields included. -func TestSet_FailedConversionLeavesForceSendFields(t *testing.T) { - job := &jobs.JobSettings{Name: "n"} //exhaustruct:ignore +// encoding/json resolves a name declared at two embedding depths in favour of the shallower +// one. Get and Set have to agree with it, so the embedded search goes level by level: a +// depth-first search would find Deep.Value first, since its embed is declared first. +type deepValue struct { + Value string `json:"value"` +} + +type deepEmbed struct { + deepValue +} - require.Error(t, structaccess.SetByString(job, "max_concurrent_runs", "")) - assert.Empty(t, job.ForceSendFields) - assert.Equal(t, "n", job.Name) +type shallowEmbed struct { + Value string `json:"value"` } -// ForceSendFields is decided from the value actually stored, not the one the caller passed: -// setting an omitempty numeric field from the string "0" stores zero, which has to be forced -// or the field marshals as absent. -func TestSet_StringZeroIntoOmitemptyNumberIsForced(t *testing.T) { - job := &jobs.JobSettings{Name: "n"} //exhaustruct:ignore +type embedDepths struct { + deepEmbed + shallowEmbed +} - require.NoError(t, structaccess.SetByString(job, "max_concurrent_runs", "0")) - assert.Equal(t, 0, job.MaxConcurrentRuns) - assert.Contains(t, job.ForceSendFields, "MaxConcurrentRuns") +func TestSet_ShallowerEmbedWins(t *testing.T) { + target := &embedDepths{} + + require.NoError(t, structaccess.SetByString(target, "value", "set")) + assert.Equal(t, "set", target.Value) + assert.Empty(t, target.deepEmbed.Value) + + got, err := structaccess.GetByString(target, "value") + require.NoError(t, err) + assert.Equal(t, "set", got) - blob, err := json.Marshal(job) + // The same field json.Marshal picks, which is the contract being matched. + blob, err := json.Marshal(target) require.NoError(t, err) - assert.Contains(t, string(blob), `"max_concurrent_runs":0`) + assert.JSONEq(t, `{"value":"set"}`, string(blob)) } From 5fd9c3e0b672e220376b19a9bd4512e20dc593ef Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 31 Aug 2026 14:34:48 +0200 Subject: [PATCH 03/28] structaccess: search embedded structs breadth-first across the whole tree Follow-up to the earlier fix, which only separated the first level of embedding from the rest: a field three levels down in the first anonymous member still won over the same name two levels down in a later member, while encoding/json picks the shallower one. ValidatePattern had the same depth-first walk, so a path could validate against one field and then be read and written on another. Both now walk level by level and share the direct-field scan. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 28 ++++++------ libs/structs/structaccess/set_test.go | 27 +++++++++++ libs/structs/structaccess/typecheck.go | 63 +++++++++++++++++--------- 3 files changed, 83 insertions(+), 35 deletions(-) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index 571641e9ee3..9a10f45aa56 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -274,20 +274,22 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S return fv, sf, v, true } - // Second pass: search embedded anonymous structs (flattening semantics), one level of - // embedding at a time. Breadth-first, not depth-first: encoding/json resolves a name - // declared at two embedding depths in favour of the shallower one, so a search that - // descended fully into the first embed could pick a deeper field than the one that is - // actually serialized under that name. - for _, fv := range embeddedStructs(v) { - if out, sf, found := findFieldInStruct(fv, key); found { - return out, sf, fv, true - } - } - for _, fv := range embeddedStructs(v) { - if out, sf, owner, found := findStructFieldByKey(fv, key); found { - return out, sf, owner, true + // Second pass: search embedded anonymous structs (flattening semantics) breadth-first, one + // level of embedding at a time. Not depth-first: encoding/json resolves a name declared at + // two embedding depths in favour of the shallower one, so descending fully into the first + // embed could pick a field three levels down over the same name two levels down in a + // later one -- and then reading or writing the field would not be the field serialized + // under that name. + level := embeddedStructs(v) + for len(level) > 0 { + var next []reflect.Value + for _, fv := range level { + if out, sf, found := findFieldInStruct(fv, key); found { + return out, sf, fv, true + } + next = append(next, embeddedStructs(fv)...) } + level = next } return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index da8cf3ff19c..86b8646093d 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -2,6 +2,7 @@ package structaccess_test import ( "encoding/json" + "reflect" "testing" "github.com/databricks/cli/libs/structs/structaccess" @@ -808,11 +809,37 @@ type shallowEmbed struct { Value string `json:"value"` } +type deeperEmbed struct { + deepEmbed +} + type embedDepths struct { deepEmbed shallowEmbed } +// The same name three levels down in the first member, against two levels down in a later +// one. json picks the shallower, so the search has to be breadth-first across the whole tree +// rather than depth-first per member. +type embedDepthsAcrossMembers struct { + deeperEmbed + deepEmbed +} + +func TestSet_ShallowerEmbedWinsAcrossMembers(t *testing.T) { + target := &embedDepthsAcrossMembers{} + + require.NoError(t, structaccess.SetByString(target, "value", "set")) + assert.Equal(t, "set", target.Value) + assert.Empty(t, target.deeperEmbed.Value) + + require.NoError(t, structaccess.ValidateByString(reflect.TypeOf(target), "value")) + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"value":"set"}`, string(blob)) +} + func TestSet_ShallowerEmbedWins(t *testing.T) { target := &embedDepths{} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 7147fa0f435..968e23bf94f 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -147,25 +147,54 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, } // First pass: direct fields + if sf, ok := findDirectFieldByKeyType(t, key); ok { + return sf, t, true + } + + // Second pass: search embedded anonymous structs breadth-first, mirroring findStructFieldByKey + // (get.go) so a path validates against the same field Get and Set resolve it to, which is + // the one encoding/json serializes: the shallower of two same-named fields. + level := embeddedStructTypes(t) + for len(level) > 0 { + var next []reflect.Type + for _, ft := range level { + if sf, ok := findDirectFieldByKeyType(ft, key); ok { + return sf, ft, true + } + next = append(next, embeddedStructTypes(ft)...) + } + level = next + } + + return reflect.StructField{}, reflect.TypeOf(nil), false +} + +// findDirectFieldByKeyType matches key against the struct's own fields, by json tag name. +func findDirectFieldByKeyType(t reflect.Type, key string) (reflect.StructField, bool) { for sf := range t.Fields() { if sf.PkgPath != "" { // unexported continue } name := structtag.JSONTag(sf.Tag.Get("json")).Name() if name == "-" || sf.Name == EmbeddedSliceFieldName { - name = "" + continue } - if name != "" && name == key { - // Skip fields marked as internal/readonly - btag := structtag.BundleTag(sf.Tag.Get("bundle")) - if btag.Internal() || btag.ReadOnly() { - continue - } - return sf, t, true + if name != key { + continue } + // Skip fields marked as internal/readonly + btag := structtag.BundleTag(sf.Tag.Get("bundle")) + if btag.Internal() || btag.ReadOnly() { + continue + } + return sf, true } + return reflect.StructField{}, false +} - // Second pass: search embedded anonymous structs recursively (flattening semantics) +// embeddedStructTypes returns the anonymous struct fields of t, dereferenced. +func embeddedStructTypes(t reflect.Type) []reflect.Type { + var out []reflect.Type for sf := range t.Fields() { if !sf.Anonymous { continue @@ -174,19 +203,9 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, for ft.Kind() == reflect.Pointer { ft = ft.Elem() } - if ft.Kind() != reflect.Struct { - continue - } - if osf, owner, ok := FindStructFieldByKeyType(ft, key); ok { - // Skip fields marked as internal/readonly - btag := structtag.BundleTag(osf.Tag.Get("bundle")) - if btag.Internal() || btag.ReadOnly() { - // Treat as not found and continue - continue - } - return osf, owner, true + if ft.Kind() == reflect.Struct { + out = append(out, ft) } } - - return reflect.StructField{}, reflect.TypeOf(nil), false + return out } From 30485c8e258c359558ca72e94de3b43c9d003c66 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 16:11:47 +0200 Subject: [PATCH 04/28] structaccess: say what the doubly-embedded test is really asserting The ForceSendFields assertion reads the promoted field, and the comparison spells out that GetByString returns the empty string rather than nil -- an explicit "" and an absent field are not the same thing. --- libs/structs/structaccess/bundle_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libs/structs/structaccess/bundle_test.go b/libs/structs/structaccess/bundle_test.go index 09c39b57122..895ef9820db 100644 --- a/libs/structs/structaccess/bundle_test.go +++ b/libs/structs/structaccess/bundle_test.go @@ -97,11 +97,12 @@ func TestGetSet_DoublyEmbeddedField(t *testing.T) { // An explicit empty value is recorded on ProjectSpec, which declares the field. require.NoError(t, SetByString(project, "budget_policy_id", "")) require.Contains(t, project.ProjectSpec.ForceSendFields, "BudgetPolicyId") - require.NotContains(t, project.PostgresProjectConfig.ForceSendFields, "BudgetPolicyId") + require.NotContains(t, project.ForceSendFields, "BudgetPolicyId") value, err = GetByString(project, "budget_policy_id") require.NoError(t, err) - require.Equal(t, "", value) + // The empty string, not nil: that is what separates an explicit "" from an absent field. + require.Equal(t, any(""), value) // And dropping it again leaves the field absent. require.NoError(t, SetByString(project, "budget_policy_id", nil)) From a963248c56a8aafbbb64c1fb6727dc64942411c4 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:12:52 +0200 Subject: [PATCH 05/28] structaccess: treat a same-depth embed conflict as not found When two embedded structs declare one json name at the same depth, encoding/json calls that ambiguous and omits the field entirely. Get, Set and ValidatePattern picked the first match, so a caller could read and write a field that is never serialized. All three now report it as not found, which is what json does with it. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 23 ++++++++++++++++++++-- libs/structs/structaccess/set_test.go | 27 ++++++++++++++++++++++++++ libs/structs/structaccess/typecheck.go | 18 ++++++++++++++++- 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index 9a10f45aa56..0a59ff8bd39 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -283,12 +283,31 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S level := embeddedStructs(v) for len(level) > 0 { var next []reflect.Value + var found []struct { + value reflect.Value + field reflect.StructField + owner reflect.Value + } for _, fv := range level { - if out, sf, found := findFieldInStruct(fv, key); found { - return out, sf, fv, true + if out, sf, ok := findFieldInStruct(fv, key); ok { + found = append(found, struct { + value reflect.Value + field reflect.StructField + owner reflect.Value + }{out, sf, fv}) + continue } next = append(next, embeddedStructs(fv)...) } + if len(found) == 1 { + return found[0].value, found[0].field, found[0].owner, true + } + if len(found) > 1 { + // Two embedded structs declare the same name at the same depth. encoding/json calls + // that ambiguous and omits the field entirely, so there is no field to read or + // write: picking one would target data that is never serialized. + return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false + } level = next } diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 86b8646093d..88e4e153ca9 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -856,3 +856,30 @@ func TestSet_ShallowerEmbedWins(t *testing.T) { require.NoError(t, err) assert.JSONEq(t, `{"value":"set"}`, string(blob)) } + +// Two embedded structs declaring one name at the same depth: encoding/json calls that +// ambiguous and omits the field, so there is nothing to read or write either. +type ambiguousA struct { + Value string `json:"value"` +} + +type ambiguousB struct { + Value string `json:"value"` +} + +type ambiguousEmbeds struct { + ambiguousA + ambiguousB +} + +func TestSet_AmbiguousEmbedIsNotFound(t *testing.T) { + target := &ambiguousEmbeds{} + + require.Error(t, structaccess.SetByString(target, "value", "set")) + require.Error(t, structaccess.ValidateByString(reflect.TypeOf(target), "value")) + + // Which is what json does with it: the name resolves to no field at all. + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{}`, string(blob)) +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 968e23bf94f..160925da721 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -157,12 +157,28 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, level := embeddedStructTypes(t) for len(level) > 0 { var next []reflect.Type + var found []struct { + field reflect.StructField + owner reflect.Type + } for _, ft := range level { if sf, ok := findDirectFieldByKeyType(ft, key); ok { - return sf, ft, true + found = append(found, struct { + field reflect.StructField + owner reflect.Type + }{sf, ft}) + continue } next = append(next, embeddedStructTypes(ft)...) } + if len(found) == 1 { + return found[0].field, found[0].owner, true + } + if len(found) > 1 { + // Ambiguous at this depth, which encoding/json resolves by omitting the field; see + // findStructFieldByKey in get.go. + return reflect.StructField{}, reflect.TypeOf(nil), false + } level = next } From 2fc009d3bc41a5338c5e6d29a77d6bfc9c8b8e1f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 31 Aug 2026 15:19:45 +0200 Subject: [PATCH 06/28] structaccess: silence vet on the deliberately ambiguous test fixture The repeated json tag is what the fixture exists to exercise. Co-authored-by: Isaac --- libs/structs/structaccess/set_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 88e4e153ca9..f0a7f2b4ab9 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -869,7 +869,7 @@ type ambiguousB struct { type ambiguousEmbeds struct { ambiguousA - ambiguousB + ambiguousB //nolint:govet // the repeated json tag is the point: both embeds declare "value" } func TestSet_AmbiguousEmbedIsNotFound(t *testing.T) { From 81b652a13a1882422cdc6b3dda3a16370d6f9aa8 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 31 Aug 2026 16:31:06 +0200 Subject: [PATCH 07/28] structaccess: do not walk a cyclic embedding twice From the review: a struct embedding a pointer to itself sent the embedded-field search round forever whenever the key was not found at all. Both the value and the type walk now skip a type they have already visited. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 11 ++++++++++- libs/structs/structaccess/set_test.go | 20 ++++++++++++++++++++ libs/structs/structaccess/typecheck.go | 10 +++++++++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index 0a59ff8bd39..0af6921678f 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -280,6 +280,9 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S // embed could pick a field three levels down over the same name two levels down in a // later one -- and then reading or writing the field would not be the field serialized // under that name. + // A cyclic embedding (a struct embedding a pointer to itself) would otherwise enqueue the + // same type forever when the key is not found at all. + seen := map[reflect.Type]bool{v.Type(): true} level := embeddedStructs(v) for len(level) > 0 { var next []reflect.Value @@ -297,7 +300,13 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S }{out, sf, fv}) continue } - next = append(next, embeddedStructs(fv)...) + for _, deeper := range embeddedStructs(fv) { + if seen[deeper.Type()] { + continue + } + seen[deeper.Type()] = true + next = append(next, deeper) + } } if len(found) == 1 { return found[0].value, found[0].field, found[0].owner, true diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index f0a7f2b4ab9..9d00fcc3947 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -883,3 +883,23 @@ func TestSet_AmbiguousEmbedIsNotFound(t *testing.T) { require.NoError(t, err) assert.JSONEq(t, `{}`, string(blob)) } + +// A struct embedding a pointer to itself: the search must not walk the same type twice, or a +// key it never finds sends it round forever. +type cyclicEmbed struct { + *cyclicEmbed + Name string `json:"name"` +} + +func TestGet_CyclicEmbedTerminates(t *testing.T) { + target := &cyclicEmbed{Name: "n"} //exhaustruct:ignore + target.cyclicEmbed = target + + got, err := structaccess.GetByString(target, "name") + require.NoError(t, err) + assert.Equal(t, "n", got) + + _, err = structaccess.GetByString(target, "nope") + require.Error(t, err) + require.Error(t, structaccess.ValidateByString(reflect.TypeOf(target), "nope")) +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 160925da721..f4797f88480 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -154,6 +154,8 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, // Second pass: search embedded anonymous structs breadth-first, mirroring findStructFieldByKey // (get.go) so a path validates against the same field Get and Set resolve it to, which is // the one encoding/json serializes: the shallower of two same-named fields. + // See findStructFieldByKey: a cyclic embedding must not be walked twice. + seen := map[reflect.Type]bool{t: true} level := embeddedStructTypes(t) for len(level) > 0 { var next []reflect.Type @@ -169,7 +171,13 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, }{sf, ft}) continue } - next = append(next, embeddedStructTypes(ft)...) + for _, deeper := range embeddedStructTypes(ft) { + if seen[deeper] { + continue + } + seen[deeper] = true + next = append(next, deeper) + } } if len(found) == 1 { return found[0].field, found[0].owner, true From c6cf2407f04e5ee39a099c3c5a4700f5ab4a5453 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 31 Aug 2026 16:55:48 +0200 Subject: [PATCH 08/28] structaccess: notice a diamond embedding as ambiguous From the review: the cycle guard deduplicated types globally, so a diamond -- two embeds reaching one type, putting the name at the same depth twice -- was only visited once and resolved to a field encoding/json omits. Types are now excluded only from earlier levels, so the two paths within one level produce the two matches that make it ambiguous. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 10 ++++++--- libs/structs/structaccess/set_test.go | 30 ++++++++++++++++++++++++++ libs/structs/structaccess/typecheck.go | 7 ++++-- 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index 0af6921678f..a2e34859bd7 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -280,8 +280,10 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S // embed could pick a field three levels down over the same name two levels down in a // later one -- and then reading or writing the field would not be the field serialized // under that name. - // A cyclic embedding (a struct embedding a pointer to itself) would otherwise enqueue the - // same type forever when the key is not found at all. + // Guards against a cyclic embedding (a struct embedding a pointer to itself), which would + // otherwise enqueue the same type forever when the key is not found at all. Only types from + // *earlier* levels are excluded: a type reachable twice within one level is a diamond, and + // the two matches it produces are exactly the ambiguity json resolves by omitting the field. seen := map[reflect.Type]bool{v.Type(): true} level := embeddedStructs(v) for len(level) > 0 { @@ -304,10 +306,12 @@ func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.S if seen[deeper.Type()] { continue } - seen[deeper.Type()] = true next = append(next, deeper) } } + for _, fv := range next { + seen[fv.Type()] = true + } if len(found) == 1 { return found[0].value, found[0].field, found[0].owner, true } diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 9d00fcc3947..7d841bd79c5 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -903,3 +903,33 @@ func TestGet_CyclicEmbedTerminates(t *testing.T) { require.Error(t, err) require.Error(t, structaccess.ValidateByString(reflect.TypeOf(target), "nope")) } + +// A diamond: two embeds reaching one type, so the name sits at the same depth twice. +// encoding/json omits it, and the search has to see both paths to notice. +type diamondLeaf struct { + Value string `json:"value"` +} + +type diamondLeft struct { + diamondLeaf +} + +type diamondRight struct { + diamondLeaf +} + +type diamondEmbeds struct { + diamondLeft + diamondRight +} + +func TestSet_DiamondEmbedIsAmbiguous(t *testing.T) { + target := &diamondEmbeds{} + + require.Error(t, structaccess.SetByString(target, "value", "set")) + require.Error(t, structaccess.ValidateByString(reflect.TypeOf(target), "value")) + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{}`, string(blob)) +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index f4797f88480..547b12d42a7 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -154,7 +154,8 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, // Second pass: search embedded anonymous structs breadth-first, mirroring findStructFieldByKey // (get.go) so a path validates against the same field Get and Set resolve it to, which is // the one encoding/json serializes: the shallower of two same-named fields. - // See findStructFieldByKey: a cyclic embedding must not be walked twice. + // See findStructFieldByKey: a cycle must not be walked twice, but a type reachable twice + // within one level is a diamond and its two matches are the ambiguity json omits. seen := map[reflect.Type]bool{t: true} level := embeddedStructTypes(t) for len(level) > 0 { @@ -175,10 +176,12 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, if seen[deeper] { continue } - seen[deeper] = true next = append(next, deeper) } } + for _, ft := range next { + seen[ft] = true + } if len(found) == 1 { return found[0].field, found[0].owner, true } From 3d439562946b1c85232f221d1dc790144ca1f5ce Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 31 Aug 2026 18:24:33 +0200 Subject: [PATCH 09/28] structaccess: ambiguity is a property of the type, not of the value From the review: two embedded pointers declaring one json name at the same depth make it a name encoding/json omits, but the value walk skips a nil embed, so whether the name resolved depended on which pointers happened to be set. Get and Set could reach a field ValidatePattern rejects. The value walk now asks the type walk first, so all three agree on which names exist. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 10 ++++++++++ libs/structs/structaccess/set_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index a2e34859bd7..a56e4404852 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -269,11 +269,21 @@ func findFieldInStruct(v reflect.Value, key string) (reflect.Value, reflect.Stru // the SDK request struct, so its fields sit two levels down. // Returns: fieldValue, structField, owner (the struct value declaring the field), found func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.StructField, reflect.Value, bool) { + t := v.Type() + // First pass: direct fields if fv, sf, found := findFieldInStruct(v, key); found { return fv, sf, v, true } + // Ambiguity is a property of the type, not of the value: a name declared at the same + // embedding depth by two members is one encoding/json omits, whether or not one of those + // members happens to be a nil pointer right now. Asking the type walk first keeps Get, Set + // and ValidatePattern agreeing on which names exist. + if _, _, ok := FindStructFieldByKeyType(t, key); !ok { + return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false + } + // Second pass: search embedded anonymous structs (flattening semantics) breadth-first, one // level of embedding at a time. Not depth-first: encoding/json resolves a name declared at // two embedding depths in favour of the shallower one, so descending fully into the first diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 7d841bd79c5..62c64d06654 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -933,3 +933,28 @@ func TestSet_DiamondEmbedIsAmbiguous(t *testing.T) { require.NoError(t, err) assert.JSONEq(t, `{}`, string(blob)) } + +// Whether a name is ambiguous is a property of the type: two embedded pointers declaring it at +// the same depth make it one encoding/json omits, and that must not change with whether one of +// them happens to be nil right now. +type ambiguousPtrEmbeds struct { + *ambiguousA + *ambiguousB //nolint:govet // the repeated json tag is the point: both embeds declare "value" +} + +func TestSet_AmbiguousPointerEmbedsIgnoreNilness(t *testing.T) { + // One embed present, the other nil: the name is still ambiguous. + target := &ambiguousPtrEmbeds{ambiguousA: &ambiguousA{}} //exhaustruct:ignore + require.Error(t, structaccess.SetByString(target, "value", "set")) + _, err := structaccess.GetByString(target, "value") + require.Error(t, err) + + // And with both present, unchanged. + target = &ambiguousPtrEmbeds{ambiguousA: &ambiguousA{}, ambiguousB: &ambiguousB{}} + require.Error(t, structaccess.SetByString(target, "value", "set")) + require.Error(t, structaccess.ValidateByString(reflect.TypeOf(target), "value")) + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{}`, string(blob)) +} From ee322e6d60d313a86abc2888daabbc76691dd4a5 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:23:10 +0200 Subject: [PATCH 10/28] structaccess: resolve a field on the type, then navigate the value by index Two disagreements with encoding/json, both found by the new agreement tests, and both fixed by moving field resolution wholly onto the type and following the index chain it produces -- which is how encoding/json itself resolves a name. A name declared behind a nil embedded pointer and again deeper down resolved to the deeper field. encoding/json picks the shallower declaration and then serializes nothing, because the pointer is nil, so the deeper field is a field the wire format never carries: Get returned a value that could not be sent and Set wrote where nothing would read. Resolving on the type and then walking the value means a nil pointer on the winning path reads as an absent field. Comparing the owner *type* is not enough, because one struct type can be reachable by two paths. An anonymous field carrying a json name is a named field to encoding/json -- it serializes as a nested object under that name -- but the embedded search flattened it anyway. So "value" resolved on a type that emits {"leaf":{"value":...}}, while "leaf.value", the path that is actually on the wire, did not resolve at all. Both directions now agree. The index-chain resolution also replaces the parallel breadth-first walk over values, so the two searches can no longer drift: findFieldInStruct, embeddedStructs and embeddedStructTypes are gone. Co-authored-by: Isaac --- libs/structs/structaccess/get.go | 144 ++++--------------------- libs/structs/structaccess/set_test.go | 60 +++++++++++ libs/structs/structaccess/typecheck.go | 142 ++++++++++++++++-------- 3 files changed, 179 insertions(+), 167 deletions(-) diff --git a/libs/structs/structaccess/get.go b/libs/structs/structaccess/get.go index a56e4404852..0f3d3cc1f4a 100644 --- a/libs/structs/structaccess/get.go +++ b/libs/structs/structaccess/get.go @@ -228,138 +228,38 @@ func accessKeyValue(v reflect.Value, key, value string, path *structpath.PathNod return reflect.Value{}, &NotFoundError{fmt.Sprintf("%s: no element found with %s=%q", path.String(), key, value)} } -// findFieldInStruct searches for a field by JSON key in a single struct (no embedding). -// Returns: fieldValue, structField, found -func findFieldInStruct(v reflect.Value, key string) (reflect.Value, reflect.StructField, bool) { - t := v.Type() - for i := range t.NumField() { - sf := t.Field(i) - if sf.PkgPath != "" { // unexported - continue - } - if sf.Anonymous { // skip embedded fields - continue - } - - // Read JSON tag using structtag helper - name := structtag.JSONTag(sf.Tag.Get("json")).Name() - if name == "-" { - name = "" - } - - if sf.Name == EmbeddedSliceFieldName { - continue // EmbeddedSlice fields are not accessible by name - } - if name != "" && name == key { - // Skip fields marked as internal or readonly via bundle tag - btag := structtag.BundleTag(sf.Tag.Get("bundle")) - if btag.Internal() || btag.ReadOnly() { - continue - } - return v.Field(i), sf, true - } - } - return reflect.Value{}, reflect.StructField{}, false -} - -// findStructFieldByKey searches exported fields of struct v for a field matching key. -// It matches json tag name (when present and not "-") only. -// It also searches embedded anonymous structs recursively (flattening semantics), which -// FindStructFieldByKeyType does too: a bundle resource embeds a config struct that embeds -// the SDK request struct, so its fields sit two levels down. +// findStructFieldByKey resolves key against the type of v and then navigates v along the +// index chain the resolution produced. +// +// Resolving on the type is what keeps Get, Set and ValidatePattern agreeing with each other +// and with encoding/json: the type decides which of two same-named fields wins, whether the +// name is ambiguous, and by which path the winner is reached. Navigating the value afterwards +// means a nil pointer on that path reads as an absent field, rather than the search falling +// through to a deeper field of the same name that the wire format never carries. +// // Returns: fieldValue, structField, owner (the struct value declaring the field), found func findStructFieldByKey(v reflect.Value, key string) (reflect.Value, reflect.StructField, reflect.Value, bool) { - t := v.Type() - - // First pass: direct fields - if fv, sf, found := findFieldInStruct(v, key); found { - return fv, sf, v, true - } - - // Ambiguity is a property of the type, not of the value: a name declared at the same - // embedding depth by two members is one encoding/json omits, whether or not one of those - // members happens to be a nil pointer right now. Asking the type walk first keeps Get, Set - // and ValidatePattern agreeing on which names exist. - if _, _, ok := FindStructFieldByKeyType(t, key); !ok { + index, sf, ok := findFieldIndexByKeyType(v.Type(), key) + if !ok { return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false } - // Second pass: search embedded anonymous structs (flattening semantics) breadth-first, one - // level of embedding at a time. Not depth-first: encoding/json resolves a name declared at - // two embedding depths in favour of the shallower one, so descending fully into the first - // embed could pick a field three levels down over the same name two levels down in a - // later one -- and then reading or writing the field would not be the field serialized - // under that name. - // Guards against a cyclic embedding (a struct embedding a pointer to itself), which would - // otherwise enqueue the same type forever when the key is not found at all. Only types from - // *earlier* levels are excluded: a type reachable twice within one level is a diamond, and - // the two matches it produces are exactly the ambiguity json resolves by omitting the field. - seen := map[reflect.Type]bool{v.Type(): true} - level := embeddedStructs(v) - for len(level) > 0 { - var next []reflect.Value - var found []struct { - value reflect.Value - field reflect.StructField - owner reflect.Value - } - for _, fv := range level { - if out, sf, ok := findFieldInStruct(fv, key); ok { - found = append(found, struct { - value reflect.Value - field reflect.StructField - owner reflect.Value - }{out, sf, fv}) - continue - } - for _, deeper := range embeddedStructs(fv) { - if seen[deeper.Type()] { - continue - } - next = append(next, deeper) + cur := v + var owner reflect.Value + for _, i := range index { + for cur.Kind() == reflect.Pointer { + if cur.IsNil() { + return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false } + cur = cur.Elem() } - for _, fv := range next { - seen[fv.Type()] = true - } - if len(found) == 1 { - return found[0].value, found[0].field, found[0].owner, true - } - if len(found) > 1 { - // Two embedded structs declare the same name at the same depth. encoding/json calls - // that ambiguous and omits the field entirely, so there is no field to read or - // write: picking one would target data that is never serialized. + if cur.Kind() != reflect.Struct { return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false } - level = next - } - - return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false -} - -// embeddedStructs returns the anonymous struct fields of v, dereferenced, skipping any that -// cannot be descended into. -func embeddedStructs(v reflect.Value) []reflect.Value { - var out []reflect.Value - t := v.Type() - for i := range t.NumField() { - if !t.Field(i).Anonymous { - continue - } - fv := v.Field(i) - for fv.Kind() == reflect.Pointer { - if fv.IsNil() { - // Not initialized; can't descend. - break - } - fv = fv.Elem() - } - if fv.Kind() != reflect.Struct { - continue - } - out = append(out, fv) + owner = cur + cur = cur.Field(i) } - return out + return cur, sf, owner, true } // forceSendFields returns the ForceSendFields slice a struct declares itself. A struct that diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 62c64d06654..50ee4523506 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -958,3 +958,63 @@ func TestSet_AmbiguousPointerEmbedsIgnoreNilness(t *testing.T) { require.NoError(t, err) assert.JSONEq(t, `{}`, string(blob)) } + +// A name declared behind a nil embedded pointer and again deeper down. encoding/json resolves +// it to the shallower declaration and then serializes nothing, because the pointer is nil -- +// so the deeper field, which the wire format never carries, is not the answer either. +type shallowLeaf struct { + Value string `json:"value,omitempty"` +} + +type deepHolder struct { + shallowLeaf +} + +type shallowBehindNil struct { + *shallowLeaf + deepHolder +} + +func TestGet_ShallowFieldBehindNilPointerIsAbsent(t *testing.T) { + target := &shallowBehindNil{deepHolder: deepHolder{shallowLeaf: shallowLeaf{Value: "deep"}}} //exhaustruct:ignore + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{}`, string(blob)) + + _, err = structaccess.GetByString(target, "value") + require.Error(t, err, "the field encoding/json resolves to is absent, so there is nothing to read") +} + +// An anonymous field carrying a json name is a named field to encoding/json: it serializes as +// a nested object under that name rather than being flattened into the outer one. +type TaggedEmbedLeaf struct { + Value string `json:"value,omitempty"` +} + +type taggedEmbed struct { + TaggedEmbedLeaf `json:"leaf"` + + Own string `json:"own,omitempty"` +} + +func TestGetSet_TaggedEmbedIsANamedField(t *testing.T) { + target := &taggedEmbed{TaggedEmbedLeaf: TaggedEmbedLeaf{Value: "v"}, Own: "o"} + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"leaf":{"value":"v"},"own":"o"}`, string(blob)) + + // Not flattened: the outer object has no "value" member. + _, err = structaccess.GetByString(target, "value") + require.Error(t, err) + + value, err := structaccess.GetByString(target, "leaf.value") + require.NoError(t, err) + assert.Equal(t, "v", value) + + require.NoError(t, structaccess.SetByString(target, "leaf.value", "set")) + blob, err = json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"leaf":{"value":"set"},"own":"o"}`, string(blob)) +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 547b12d42a7..8e0727a284c 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -142,63 +142,123 @@ func validateNodeSlice(t reflect.Type, nodes []*structpath.PatternNode) error { // It also searches embedded anonymous structs (pointer or value) recursively. // Returns the StructField, the declaring owner type, and whether it was found. func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, reflect.Type, bool) { - if t.Kind() != reflect.Struct { + index, sf, ok := findFieldIndexByKeyType(t, key) + if !ok { return reflect.StructField{}, reflect.TypeOf(nil), false } + return sf, ownerTypeAt(t, index), true +} - // First pass: direct fields - if sf, ok := findDirectFieldByKeyType(t, key); ok { - return sf, t, true +// findFieldIndexByKeyType resolves key to a field of t and returns the chain of field indices +// leading to it, the way reflect.Type.FieldByName does. +// +// Embedded structs are searched breadth-first, mirroring encoding/json: a name declared at +// two embedding depths resolves to the shallower one, so a depth-first search could pick a +// field the wire format does not use. A name declared twice at one depth is ambiguous, which +// encoding/json resolves by serializing neither, so it resolves to nothing here too. +// +// Returning the index chain rather than a type matters: the same struct type can be reachable +// by more than one path, so a caller navigating a value needs the path json would take, not +// merely the type at the end of it. +func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructField, bool) { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return nil, reflect.StructField{}, false } - // Second pass: search embedded anonymous structs breadth-first, mirroring findStructFieldByKey - // (get.go) so a path validates against the same field Get and Set resolve it to, which is - // the one encoding/json serializes: the shallower of two same-named fields. - // See findStructFieldByKey: a cycle must not be walked twice, but a type reachable twice - // within one level is a diamond and its two matches are the ambiguity json omits. + if i, sf, ok := findDirectFieldByKeyType(t, key); ok { + return []int{i}, sf, true + } + + // A cycle must not be walked twice, or a key the type never declares sends the search + // round forever. Only types from *earlier* levels are excluded: a type reachable twice + // within one level is a diamond, and its two matches are the ambiguity json omits. seen := map[reflect.Type]bool{t: true} - level := embeddedStructTypes(t) + level := embeddedIndexPaths(t, nil) for len(level) > 0 { - var next []reflect.Type + var next []embeddedPath var found []struct { + index []int field reflect.StructField - owner reflect.Type } - for _, ft := range level { - if sf, ok := findDirectFieldByKeyType(ft, key); ok { + for _, candidate := range level { + if i, sf, ok := findDirectFieldByKeyType(candidate.typ, key); ok { found = append(found, struct { + index []int field reflect.StructField - owner reflect.Type - }{sf, ft}) + }{append(append([]int{}, candidate.index...), i), sf}) continue } - for _, deeper := range embeddedStructTypes(ft) { - if seen[deeper] { + for _, deeper := range embeddedIndexPaths(candidate.typ, candidate.index) { + if seen[deeper.typ] { continue } next = append(next, deeper) } } - for _, ft := range next { - seen[ft] = true + for _, candidate := range next { + seen[candidate.typ] = true } if len(found) == 1 { - return found[0].field, found[0].owner, true + return found[0].index, found[0].field, true } if len(found) > 1 { - // Ambiguous at this depth, which encoding/json resolves by omitting the field; see - // findStructFieldByKey in get.go. - return reflect.StructField{}, reflect.TypeOf(nil), false + return nil, reflect.StructField{}, false } level = next } - return reflect.StructField{}, reflect.TypeOf(nil), false + return nil, reflect.StructField{}, false +} + +// embeddedPath is an embedded struct type together with the index chain that reaches it. +type embeddedPath struct { + typ reflect.Type + index []int } -// findDirectFieldByKeyType matches key against the struct's own fields, by json tag name. -func findDirectFieldByKeyType(t reflect.Type, key string) (reflect.StructField, bool) { - for sf := range t.Fields() { +// embeddedIndexPaths returns the embeds of t that encoding/json flattens, each with the index +// chain from the root that reaches it. +func embeddedIndexPaths(t reflect.Type, prefix []int) []embeddedPath { + var out []embeddedPath + for i := range t.NumField() { + sf := t.Field(i) + if !isFlattenedEmbed(sf) { + continue + } + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + if ft.Kind() != reflect.Struct { + continue + } + out = append(out, embeddedPath{typ: ft, index: append(append([]int{}, prefix...), i)}) + } + return out +} + +// ownerTypeAt returns the struct type that declares the field the index chain ends at. +func ownerTypeAt(t reflect.Type, index []int) reflect.Type { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + for _, i := range index[:len(index)-1] { + t = t.Field(i).Type + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + } + return t +} + +// findDirectFieldByKeyType matches key against the struct's own fields, by json tag name, and +// returns the field's index. +func findDirectFieldByKeyType(t reflect.Type, key string) (int, reflect.StructField, bool) { + for i := range t.NumField() { + sf := t.Field(i) if sf.PkgPath != "" { // unexported continue } @@ -214,25 +274,17 @@ func findDirectFieldByKeyType(t reflect.Type, key string) (reflect.StructField, if btag.Internal() || btag.ReadOnly() { continue } - return sf, true + return i, sf, true } - return reflect.StructField{}, false + return 0, reflect.StructField{}, false } -// embeddedStructTypes returns the anonymous struct fields of t, dereferenced. -func embeddedStructTypes(t reflect.Type) []reflect.Type { - var out []reflect.Type - for sf := range t.Fields() { - if !sf.Anonymous { - continue - } - ft := sf.Type - for ft.Kind() == reflect.Pointer { - ft = ft.Elem() - } - if ft.Kind() == reflect.Struct { - out = append(out, ft) - } +// isFlattenedEmbed reports whether the field is an embed encoding/json flattens into the +// outer object. An anonymous field that carries a json name is a named field instead: it +// serializes as a nested object under that name. +func isFlattenedEmbed(sf reflect.StructField) bool { + if !sf.Anonymous { + return false } - return out + return structtag.JSONTag(sf.Tag.Get("json")).Name() == "" } From 74fb43612e49f5e4043775016004729d8b7bd6c3 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:13:47 +0200 Subject: [PATCH 11/28] structwalk: flatten an embed only when encoding/json flattens it Both walks flattened every anonymous field. encoding/json flattens an embed only when its json tag gives no name: a name makes it an ordinary field serialized as a nested object. So a tagged embed was walked as though its fields were the outer struct's, and structdiff would have reported a change at a path the wire format does not have. The type walk had the inverse bug: it keyed the decision on the tag being non-empty, so `json:",omitempty"` on an embed -- a tag that sets an option and leaves the name empty -- made it a nested field, while encoding/json still flattens it. Both now use structaccess.IsFlattenedEmbed, so the three searches cannot drift again. No resource type has either shape today: the refschema golden is unchanged. Co-authored-by: Isaac --- libs/structs/structaccess/typecheck.go | 11 +++--- libs/structs/structwalk/walk.go | 6 ++- libs/structs/structwalk/walk_test.go | 53 ++++++++++++++++++++++++++ libs/structs/structwalk/walktype.go | 6 ++- 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 8e0727a284c..1b86f9fee9f 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -225,7 +225,7 @@ func embeddedIndexPaths(t reflect.Type, prefix []int) []embeddedPath { var out []embeddedPath for i := range t.NumField() { sf := t.Field(i) - if !isFlattenedEmbed(sf) { + if !IsFlattenedEmbed(sf) { continue } ft := sf.Type @@ -279,10 +279,11 @@ func findDirectFieldByKeyType(t reflect.Type, key string) (int, reflect.StructFi return 0, reflect.StructField{}, false } -// isFlattenedEmbed reports whether the field is an embed encoding/json flattens into the -// outer object. An anonymous field that carries a json name is a named field instead: it -// serializes as a nested object under that name. -func isFlattenedEmbed(sf reflect.StructField) bool { +// IsFlattenedEmbed reports whether the field is an embed encoding/json flattens into the +// outer object. An anonymous field that carries a json *name* is a named field instead: it +// serializes as a nested object under that name. The name is what matters, not the presence +// of a tag: `json:",omitempty"` leaves the name empty, so such a field is still flattened. +func IsFlattenedEmbed(sf reflect.StructField) bool { if !sf.Anonymous { return false } diff --git a/libs/structs/structwalk/walk.go b/libs/structs/structwalk/walk.go index 96a0cd1271a..32084458d2b 100644 --- a/libs/structs/structwalk/walk.go +++ b/libs/structs/structwalk/walk.go @@ -115,8 +115,10 @@ func walkStruct(path *structpath.PathNode, s reflect.Value, visit VisitFunc) { continue } - // Directly walk into embedded structs without adding the key to the path. - if sf.Anonymous { + // Directly walk into embedded structs without adding the key to the path. An anonymous + // field carrying a json name is not one of those: encoding/json serializes it as a + // nested object under that name, so it is walked as a named field below. + if structaccess.IsFlattenedEmbed(sf) { walkValue(path, s.Field(i), &sf, visit) continue } diff --git a/libs/structs/structwalk/walk_test.go b/libs/structs/structwalk/walk_test.go index aae419bfaee..fea7c86b0d4 100644 --- a/libs/structs/structwalk/walk_test.go +++ b/libs/structs/structwalk/walk_test.go @@ -1,7 +1,9 @@ package structwalk import ( + "encoding/json" "reflect" + "slices" "testing" "github.com/databricks/cli/libs/structs/structpath" @@ -276,3 +278,54 @@ func TestEmbeddedStructWithJSONTagDash(t *testing.T) { "parent_field": "parent", }, flatten(t, parent)) } + +// An anonymous field carrying a json name is a named field to encoding/json: it serializes as +// a nested object under that name rather than being flattened into the outer one. A field whose +// tag sets only an option, leaving the name empty, is still flattened. +type WalkEmbedLeaf struct { + Value string `json:"value,omitempty"` +} + +type walkTaggedEmbed struct { + WalkEmbedLeaf `json:"leaf"` + + Own string `json:"own,omitempty"` +} + +type walkOptionOnlyEmbed struct { + WalkEmbedLeaf `json:",omitempty"` + + Own string `json:"own,omitempty"` +} + +func TestWalkTaggedEmbedIsANamedField(t *testing.T) { + value := &walkTaggedEmbed{WalkEmbedLeaf: WalkEmbedLeaf{Value: "v"}, Own: "o"} + + blob, err := json.Marshal(value) + require.NoError(t, err) + assert.JSONEq(t, `{"leaf":{"value":"v"},"own":"o"}`, string(blob)) + + assert.Equal(t, []string{"leaf.value", "own"}, walkPaths(t, value)) +} + +func TestWalkOptionOnlyEmbedIsStillFlattened(t *testing.T) { + value := &walkOptionOnlyEmbed{WalkEmbedLeaf: WalkEmbedLeaf{Value: "v"}, Own: "o"} + + blob, err := json.Marshal(value) + require.NoError(t, err) + assert.JSONEq(t, `{"value":"v","own":"o"}`, string(blob)) + + assert.Equal(t, []string{"own", "value"}, walkPaths(t, value)) +} + +// walkPaths returns the paths Walk visits, sorted. +func walkPaths(t *testing.T, value any) []string { + t.Helper() + + var paths []string + require.NoError(t, Walk(value, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { + paths = append(paths, path.String()) + })) + slices.Sort(paths) + return paths +} diff --git a/libs/structs/structwalk/walktype.go b/libs/structs/structwalk/walktype.go index cd4be28c843..f189f2d0251 100644 --- a/libs/structs/structwalk/walktype.go +++ b/libs/structs/structwalk/walktype.go @@ -109,9 +109,11 @@ func walkTypeStruct(path *structpath.PatternNode, st reflect.Type, visit VisitTy continue // unexported } - // Handle embedded structs (anonymous fields without json tags) + // Handle embedded structs that encoding/json flattens. The json *name* decides, not the + // presence of a tag: `json:",omitempty"` on an embed leaves the name empty and is still + // flattened, while a name makes it a nested object. jsonTag := sf.Tag.Get("json") - if sf.Anonymous && jsonTag == "" { + if structaccess.IsFlattenedEmbed(sf) { // For embedded structs, walk the embedded type at the current path level // This flattens the embedded struct's fields into the parent struct walkTypeValue(path, sf.Type, &sf, visit, visitedCount) From 144515866d32229535e92f8f31d53899c0639837 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:18:05 +0200 Subject: [PATCH 12/28] structdiff: report a tagged embed's changes under its own name Same rule as structwalk: encoding/json flattens an embed only when its json tag gives no name. structdiff flattened every anonymous field, so a change inside a tagged embed was reported at the outer level -- a path the wire format does not have, which the direct engine would then put in an update mask. equal.go gets the same predicate, which also means a tagged embed with json:"-" is skipped rather than compared. Co-authored-by: Isaac --- libs/structs/structdiff/diff.go | 7 +++++-- libs/structs/structdiff/diff_test.go | 28 ++++++++++++++++++++++++++++ libs/structs/structdiff/equal.go | 6 ++++-- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index d933ed5b3a9..c583d6d1759 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -209,8 +209,11 @@ func diffStruct(ctx *diffContext, path *structpath.PathNode, s1, s2 reflect.Valu continue } - // Continue traversing embedded structs. Do not add the key to the path though. - if sf.Anonymous { + // Continue traversing embedded structs. Do not add the key to the path though. An + // anonymous field carrying a json name is not one of these: encoding/json serializes it + // as a nested object, so it is handled as a named field below and its changes are + // reported under that name. + if structaccess.IsFlattenedEmbed(sf) { if err := diffValues(ctx, path, s1.Field(i), s2.Field(i), changes); err != nil { return err } diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index 627169e718c..2839550c5a7 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -1,6 +1,7 @@ package structdiff import ( + "encoding/json" "reflect" "testing" "time" @@ -9,6 +10,7 @@ import ( sdktime "github.com/databricks/databricks-sdk-go/common/types/time" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type B struct{ S string } @@ -923,3 +925,29 @@ func TestGetStructDiffSliceKeysDuplicates(t *testing.T) { }) } } + +// An anonymous field carrying a json name is a named field to encoding/json: it serializes as +// a nested object, so a change inside it belongs at that nested path, not at the outer level. +type DiffEmbedLeaf struct { + Value string `json:"value,omitempty"` +} + +type diffTaggedEmbed struct { + DiffEmbedLeaf `json:"leaf"` + + Own string `json:"own,omitempty"` +} + +func TestDiffTaggedEmbedIsReportedUnderItsName(t *testing.T) { + before := &diffTaggedEmbed{DiffEmbedLeaf: DiffEmbedLeaf{Value: "before"}, Own: "o"} + after := &diffTaggedEmbed{DiffEmbedLeaf: DiffEmbedLeaf{Value: "after"}, Own: "o"} + + blob, err := json.Marshal(after) + require.NoError(t, err) + assert.JSONEq(t, `{"leaf":{"value":"after"},"own":"o"}`, string(blob)) + + changes, err := GetStructDiff(before, after, nil) + require.NoError(t, err) + require.Len(t, changes, 1) + assert.Equal(t, "leaf.value", changes[0].Path.String()) +} diff --git a/libs/structs/structdiff/equal.go b/libs/structs/structdiff/equal.go index 6bf253c7317..bc75fed318f 100644 --- a/libs/structs/structdiff/equal.go +++ b/libs/structs/structdiff/equal.go @@ -4,6 +4,7 @@ import ( "reflect" "slices" + "github.com/databricks/cli/libs/structs/structaccess" "github.com/databricks/cli/libs/structs/structtag" ) @@ -107,8 +108,9 @@ func equalStruct(s1, s2 reflect.Value) bool { continue } - // Continue traversing embedded structs. - if sf.Anonymous { + // Continue traversing embedded structs. A tagged one is a named field to encoding/json, + // so it goes through the path below, which also honours json:"-". + if structaccess.IsFlattenedEmbed(sf) { if !equalValues(s1.Field(i), s2.Field(i)) { return false } From 41c2599ff32e45b8d9e403c509c537e5eb35003c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:48:24 +0200 Subject: [PATCH 13/28] structaccess: follow encoding/json's precedence rules for a field name Three more from the review, all in name resolution: At one depth, encoding/json prefers the field whose json tag names it over one that merely has the matching Go field name; only a genuine tie is ambiguous. The search counted both as matches and reported the name as not found, so a field the wire format does carry was unreachable. A field whose tag sets only an option -- `json:",omitempty"` -- has no json name, so encoding/json serializes it under its Go field name. The search matched tag names only, so such a field could not be resolved at all, while structwalk already emitted it under the Go name: the two disagreed about a field that is plainly on the wire. Only an anonymous *struct* is promoted. An embedded scalar, slice or interface is a member named after its type, but IsFlattenedEmbed called it an embed, so structwalk and structdiff placed its contents at the parent path. No resource type has any of these shapes: the refschema golden is unchanged. Co-authored-by: Isaac --- libs/structs/structaccess/set_test.go | 71 +++++++++++++++++++ libs/structs/structaccess/typecheck.go | 98 +++++++++++++++++++------- 2 files changed, 143 insertions(+), 26 deletions(-) diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 50ee4523506..6faa8a62279 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -1018,3 +1018,74 @@ func TestGetSet_TaggedEmbedIsANamedField(t *testing.T) { require.NoError(t, err) assert.JSONEq(t, `{"leaf":{"value":"set"},"own":"o"}`, string(blob)) } + +// At one depth, encoding/json prefers a field whose json tag names it over one that only has +// the matching Go field name, instead of calling the pair ambiguous. +type untaggedX struct { + X string +} + +type taggedAsX struct { + Y string `json:"X"` +} + +type taggedBeatsUntagged struct { + untaggedX + taggedAsX +} + +func TestGet_TaggedNameBeatsUntaggedAtTheSameDepth(t *testing.T) { + target := &taggedBeatsUntagged{untaggedX: untaggedX{X: "untagged"}, taggedAsX: taggedAsX{Y: "tagged"}} + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"X":"tagged"}`, string(blob)) + + value, err := structaccess.GetByString(target, "X") + require.NoError(t, err) + assert.Equal(t, "tagged", value, "must resolve to the field encoding/json serializes") +} + +// A field whose tag sets only an option has no json name, so encoding/json serializes it under +// its Go field name and that is the name it has to be reachable by. +type optionOnlyTag struct { + Count int `json:"count,omitempty"` + Total int `json:",omitempty"` +} + +func TestGetSet_FieldWithoutATagNameUsesItsGoName(t *testing.T) { + target := &optionOnlyTag{Count: 1, Total: 2} + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"count":1,"Total":2}`, string(blob)) + + value, err := structaccess.GetByString(target, "Total") + require.NoError(t, err) + assert.Equal(t, 2, value) + + require.NoError(t, structaccess.SetByString(target, "Total", 7)) + assert.Equal(t, 7, target.Total) +} + +// An anonymous field that is not a struct is not promoted: encoding/json serializes it as a +// member named after its type. +type EmbeddedName string + +type embeddedScalar struct { + EmbeddedName + + Own string `json:"own,omitempty"` +} + +func TestGet_AnonymousNonStructIsANamedMember(t *testing.T) { + target := &embeddedScalar{EmbeddedName: "n", Own: "o"} + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"EmbeddedName":"n","own":"o"}`, string(blob)) + + value, err := structaccess.GetByString(target, "EmbeddedName") + require.NoError(t, err) + assert.Equal(t, EmbeddedName("n"), value) +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 1b86f9fee9f..c0244b74645 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -168,8 +168,8 @@ func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructF return nil, reflect.StructField{}, false } - if i, sf, ok := findDirectFieldByKeyType(t, key); ok { - return []int{i}, sf, true + if c, ok := pickCandidate(directCandidates(t, key, nil)); ok { + return c.index, c.field, true } // A cycle must not be walked twice, or a key the type never declares sends the search @@ -179,32 +179,27 @@ func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructF level := embeddedIndexPaths(t, nil) for len(level) > 0 { var next []embeddedPath - var found []struct { - index []int - field reflect.StructField - } - for _, candidate := range level { - if i, sf, ok := findDirectFieldByKeyType(candidate.typ, key); ok { - found = append(found, struct { - index []int - field reflect.StructField - }{append(append([]int{}, candidate.index...), i), sf}) + var found []candidate + for _, embed := range level { + matches := directCandidates(embed.typ, key, embed.index) + if len(matches) > 0 { + found = append(found, matches...) continue } - for _, deeper := range embeddedIndexPaths(candidate.typ, candidate.index) { + for _, deeper := range embeddedIndexPaths(embed.typ, embed.index) { if seen[deeper.typ] { continue } next = append(next, deeper) } } - for _, candidate := range next { - seen[candidate.typ] = true - } - if len(found) == 1 { - return found[0].index, found[0].field, true + for _, embed := range next { + seen[embed.typ] = true } - if len(found) > 1 { + if len(found) > 0 { + if c, ok := pickCandidate(found); ok { + return c.index, c.field, true + } return nil, reflect.StructField{}, false } level = next @@ -254,18 +249,37 @@ func ownerTypeAt(t reflect.Type, index []int) reflect.Type { return t } -// findDirectFieldByKeyType matches key against the struct's own fields, by json tag name, and -// returns the field's index. -func findDirectFieldByKeyType(t reflect.Type, key string) (int, reflect.StructField, bool) { +// candidate is a field that matches a json name, with the index chain reaching it and whether +// the name came from a json tag. encoding/json prefers a tagged name over an untagged one at +// the same depth, so the distinction has to survive the search. +type candidate struct { + index []int + field reflect.StructField + tagged bool +} + +// directCandidates returns the struct's own fields that key can name. A field with a json tag +// name is matched on that; a field without one is matched on its Go field name, which is what +// encoding/json serializes it under. An embed that encoding/json flattens is not addressable by +// name at all, so it is not a candidate. +func directCandidates(t reflect.Type, key string, prefix []int) []candidate { + var out []candidate for i := range t.NumField() { sf := t.Field(i) if sf.PkgPath != "" { // unexported continue } + if sf.Name == EmbeddedSliceFieldName || IsFlattenedEmbed(sf) { + continue + } name := structtag.JSONTag(sf.Tag.Get("json")).Name() - if name == "-" || sf.Name == EmbeddedSliceFieldName { + if name == "-" { continue } + tagged := name != "" + if !tagged { + name = sf.Name + } if name != key { continue } @@ -274,9 +288,32 @@ func findDirectFieldByKeyType(t reflect.Type, key string) (int, reflect.StructFi if btag.Internal() || btag.ReadOnly() { continue } - return i, sf, true + out = append(out, candidate{ + index: append(append([]int{}, prefix...), i), + field: sf, + tagged: tagged, + }) + } + return out +} + +// pickCandidate applies encoding/json's precedence among fields that share a name at one +// depth: a single tagged name wins over untagged ones, a single match of either kind wins, and +// anything else is ambiguous and serialized as nothing. +func pickCandidate(candidates []candidate) (candidate, bool) { + if len(candidates) == 1 { + return candidates[0], true + } + var tagged []candidate + for _, c := range candidates { + if c.tagged { + tagged = append(tagged, c) + } } - return 0, reflect.StructField{}, false + if len(tagged) == 1 { + return tagged[0], true + } + return candidate{}, false } // IsFlattenedEmbed reports whether the field is an embed encoding/json flattens into the @@ -287,5 +324,14 @@ func IsFlattenedEmbed(sf reflect.StructField) bool { if !sf.Anonymous { return false } - return structtag.JSONTag(sf.Tag.Get("json")).Name() == "" + if structtag.JSONTag(sf.Tag.Get("json")).Name() != "" { + return false + } + // Only an anonymous *struct* is promoted. An embedded scalar, slice or interface is a member + // named after its type, so it belongs at its own path rather than the parent's. + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + return ft.Kind() == reflect.Struct } From 0299bab1c0ac4517f2e8a997d353b8de8e5fbc25 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:06:04 +0200 Subject: [PATCH 14/28] structaccess: descend into a repeated embedded type once, as encoding/json does encoding/json walks an embedded type once per level however many members reach it, so a name declared *below* a type reached by two routes is not ambiguous -- it resolves along the first route. A name the duplicated type declares itself is ambiguous, and json serializes neither. The search treated every route as independent, so it called the first case ambiguous and reported a name as not found that the wire format does carry. Verified against json rather than reasoned about: a diamond over the declaring type marshals to {}, while a diamond one level above it marshals to {"value":"left"}. The internal/readonly skip keeps its existing behaviour, with a comment recording that it diverges from encoding/json: such a field shadows a same-named field further down, so skipping it lets the deeper one win. resources.App is the live example. Rejecting the name outright instead would make ${resources.apps.*.url} unresolvable, so that is a decision about what internal means rather than a fix to make here. Co-authored-by: Isaac --- libs/structs/structaccess/set_test.go | 38 +++++++++++++++++++ libs/structs/structaccess/typecheck.go | 52 +++++++++++++++++++++----- 2 files changed, 81 insertions(+), 9 deletions(-) diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 6faa8a62279..ac0c061d3dd 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -1089,3 +1089,41 @@ func TestGet_AnonymousNonStructIsANamedMember(t *testing.T) { require.NoError(t, err) assert.Equal(t, EmbeddedName("n"), value) } + +// The same embedded type reached by two routes: encoding/json descends into it once, so a name +// declared *below* it is not ambiguous, while a name the duplicated type declares itself is. +type repeatedLeaf struct { + Value string `json:"value,omitempty"` +} + +type repeatedMiddle struct { + repeatedLeaf +} + +type repeatedLeft struct { + repeatedMiddle +} + +type repeatedRight struct { + repeatedMiddle +} + +type repeatedEmbed struct { + repeatedLeft + repeatedRight +} + +func TestGet_TypeReachedTwiceIsNotAmbiguousBelowIt(t *testing.T) { + target := &repeatedEmbed{ + repeatedLeft: repeatedLeft{repeatedMiddle: repeatedMiddle{repeatedLeaf: repeatedLeaf{Value: "left"}}}, + repeatedRight: repeatedRight{repeatedMiddle: repeatedMiddle{repeatedLeaf: repeatedLeaf{Value: "right"}}}, + } + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"value":"left"}`, string(blob), "encoding/json takes the first route") + + value, err := structaccess.GetByString(target, "value") + require.NoError(t, err) + assert.Equal(t, "left", value) +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index c0244b74645..924b17d5451 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -173,10 +173,9 @@ func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructF } // A cycle must not be walked twice, or a key the type never declares sends the search - // round forever. Only types from *earlier* levels are excluded: a type reachable twice - // within one level is a diamond, and its two matches are the ambiguity json omits. + // round forever. seen := map[reflect.Type]bool{t: true} - level := embeddedIndexPaths(t, nil) + level := dedupeByType(embeddedIndexPaths(t, nil)) for len(level) > 0 { var next []embeddedPath var found []candidate @@ -184,6 +183,12 @@ func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructF matches := directCandidates(embed.typ, key, embed.index) if len(matches) > 0 { found = append(found, matches...) + if embed.reached > 1 { + // Several members of the previous level reach this type, so encoding/json sees + // the names it declares once per route and annihilates them. One extra match is + // enough to make the name ambiguous below. + found = append(found, matches[0]) + } continue } for _, deeper := range embeddedIndexPaths(embed.typ, embed.index) { @@ -193,7 +198,8 @@ func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructF next = append(next, deeper) } } - for _, embed := range next { + level = dedupeByType(next) + for _, embed := range level { seen[embed.typ] = true } if len(found) > 0 { @@ -202,16 +208,36 @@ func findFieldIndexByKeyType(t reflect.Type, key string) ([]int, reflect.StructF } return nil, reflect.StructField{}, false } - level = next } return nil, reflect.StructField{}, false } -// embeddedPath is an embedded struct type together with the index chain that reaches it. +// embeddedPath is an embedded struct type together with the index chain that reaches it, and +// how many members of the previous level reach it. type embeddedPath struct { - typ reflect.Type - index []int + typ reflect.Type + index []int + reached int +} + +// dedupeByType collapses repeated embeds of one type into a single entry, counting how many +// routes reached it. encoding/json descends into a type once per level however many members +// embed it, so a name declared *below* a type reached twice is not ambiguous; a name the +// duplicated type declares itself is, and the count records that. +func dedupeByType(paths []embeddedPath) []embeddedPath { + var out []embeddedPath + index := map[reflect.Type]int{} + for _, path := range paths { + if at, ok := index[path.typ]; ok { + out[at].reached++ + continue + } + index[path.typ] = len(out) + path.reached = 1 + out = append(out, path) + } + return out } // embeddedIndexPaths returns the embeds of t that encoding/json flattens, each with the index @@ -283,7 +309,15 @@ func directCandidates(t reflect.Type, key string, prefix []int) []candidate { if name != key { continue } - // Skip fields marked as internal/readonly + // Skip fields marked as internal/readonly. + // + // Known divergence from encoding/json: such a field still shadows a same-named field + // further down, so dropping it here lets the deeper one win and a caller can reach a + // field the wire format does not carry. resources.App is the live example -- its + // BaseResource.URL is internal and shadows the SDK's url, which json serializes as the + // internal one. Rejecting the name outright instead would make + // ${resources.apps.*.url} unresolvable, so which of the two is right is a decision + // about what internal means, not a detail of the search. btag := structtag.BundleTag(sf.Tag.Get("bundle")) if btag.Internal() || btag.ReadOnly() { continue From 2deeb7c60e0bd3d50f789cdf3d713dfeaa2119dd Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:20:21 +0200 Subject: [PATCH 15/28] structaccess: pin the repeated-embed matrix against encoding/json The fifth review round claimed a remaining tagged/untagged bug for a repeated embedded type. It does not reproduce: the counterexample used omitempty fields left at their zero value, so "omitted because empty" was indistinguishable from "not serialized at all". With values populated, all four combinations agree. They are worth keeping, since the reasoning is easy to get wrong in either direction, so the table asserts each against encoding/json rather than against a hand-written expectation: a repeated type declaring the name itself is annihilated, its tagged name losing to a sibling's untagged X under "X", a repeated untagged name not colliding with the tagged one at all, and the shallower of two routes winning. Co-authored-by: Isaac --- libs/structs/structaccess/set_test.go | 96 +++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index ac0c061d3dd..3d9603a3831 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -1127,3 +1127,99 @@ func TestGet_TypeReachedTwiceIsNotAmbiguousBelowIt(t *testing.T) { require.NoError(t, err) assert.Equal(t, "left", value) } + +// Combinations of a repeated embedded type with tagged and untagged declarations of one name. +// encoding/json is the oracle for each: the assertion compares what it serializes under the +// name with what Get resolves, so the pair cannot drift. +type matrixTagged struct { + Y string `json:"x"` +} + +type matrixUntagged struct { + X string +} + +type ( + matrixLeftTagged struct{ matrixTagged } + matrixRightTagged struct{ matrixTagged } + matrixLeftUntagged struct{ matrixUntagged } + matrixRightUntagged struct{ matrixUntagged } +) + +// The repeated type declares the name itself: two routes, so encoding/json annihilates it. +type matrixRepeatDeclares struct { + matrixLeftTagged + matrixRightTagged +} + +// The repeated type's tagged name is annihilated, leaving a sibling's untagged X under "X". +type matrixRepeatTaggedPlusUntagged struct { + matrixLeftTagged + matrixRightTagged + matrixUntagged +} + +// The repeated type's untagged name never collides with "x"; the sibling's tagged one wins. +type matrixRepeatUntaggedPlusTagged struct { + matrixLeftUntagged + matrixRightUntagged + matrixTagged +} + +type ( + matrixDeepHolder struct{ matrixTagged } + matrixDeeperRoute struct{ matrixDeepHolder } +) + +// One route reaches the declaring type a level earlier than the other: the shallower wins. +type matrixMixedDepth struct { + matrixTagged + matrixDeeperRoute +} + +func TestGet_RepeatedEmbedMatrixMatchesEncodingJSON(t *testing.T) { + repeatDeclares := &matrixRepeatDeclares{} + repeatDeclares.matrixLeftTagged.Y = "L" + repeatDeclares.matrixRightTagged.Y = "R" + + taggedPlusUntagged := &matrixRepeatTaggedPlusUntagged{} + taggedPlusUntagged.matrixLeftTagged.Y = "L" + taggedPlusUntagged.matrixRightTagged.Y = "R" + taggedPlusUntagged.X = "U" + + untaggedPlusTagged := &matrixRepeatUntaggedPlusTagged{} + untaggedPlusTagged.matrixLeftUntagged.X = "L" + untaggedPlusTagged.matrixRightUntagged.X = "R" + untaggedPlusTagged.Y = "T" + + mixedDepth := &matrixMixedDepth{} + mixedDepth.Y = "shallow" + mixedDepth.Y = "deep" + + for _, tc := range []struct { + name string + value any + }{ + {"repeated type declares the name", repeatDeclares}, + {"repeated tagged plus untagged sibling", taggedPlusUntagged}, + {"repeated untagged plus tagged sibling", untaggedPlusTagged}, + {"one route shallower than the other", mixedDepth}, + } { + t.Run(tc.name, func(t *testing.T) { + blob, err := json.Marshal(tc.value) + require.NoError(t, err) + var emitted map[string]any + require.NoError(t, json.Unmarshal(blob, &emitted)) + + want, onTheWire := emitted["x"] + got, err := structaccess.GetByString(tc.value, "x") + + if !onTheWire { + require.Error(t, err, "encoding/json emitted %s, so there is no x to read", blob) + return + } + require.NoError(t, err) + assert.Equal(t, want, got, "encoding/json emitted %s", blob) + }) + } +} From 54ee0f7f25e88d1bda1c621d8e643117355c1ef3 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:28:26 +0200 Subject: [PATCH 16/28] structs: only the exact tag json:"-" omits a field encoding/json skips a field only when its json tag is exactly "-". A tag whose name part is "-" followed by options -- json:"-,omitempty" -- names the field "-" and serializes it like any other name. structtag's parsed name reports "-" for both, so every caller that branched on the parsed name conflated them: structaccess could not resolve such a field, and structwalk and structdiff left it out. structaccess.IsSkippedField now makes the distinction from the raw tag, and the four packages share it. The structwalk fixture already had two such fields, added as "fixture for odd tag handling"; its expectation asserted they were skipped, which is what encoding/json does with json:"-" and not with what they actually carry. Verified against json.Marshal: {"-":"o","kept":"k"}. No resource type has the shape -- the refschema golden is unchanged. Co-authored-by: Isaac --- libs/structs/structaccess/set_test.go | 24 ++++++++++++++++++++++++ libs/structs/structaccess/typecheck.go | 11 +++++++++-- libs/structs/structdiff/diff.go | 2 +- libs/structs/structdiff/equal.go | 2 +- libs/structs/structwalk/walk.go | 4 ++-- libs/structs/structwalk/walktype.go | 2 +- libs/structs/structwalk/walktype_test.go | 4 ++++ 7 files changed, 42 insertions(+), 7 deletions(-) diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 3d9603a3831..835b4bedf86 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -1223,3 +1223,27 @@ func TestGet_RepeatedEmbedMatrixMatchesEncodingJSON(t *testing.T) { }) } } + +// Only the exact tag json:"-" omits a field. A tag whose name part is "-" followed by options +// names the field "-", which encoding/json serializes like any other name. +type dashNamed struct { + Skipped string `json:"-"` + Named string `json:"-,omitempty"` //nolint:staticcheck // the odd tag is the point + Kept string `json:"kept,omitempty"` +} + +func TestGetSet_DashIsAFieldNameWhenTheTagHasOptions(t *testing.T) { + target := &dashNamed{Skipped: "s", Named: "n", Kept: "k"} + + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"-":"n","kept":"k"}`, string(blob)) + + value, err := structaccess.GetByString(target, "-") + require.NoError(t, err) + assert.Equal(t, "n", value) + + require.NoError(t, structaccess.SetByString(target, "-", "set")) + assert.Equal(t, "set", target.Named) + assert.Equal(t, "s", target.Skipped, "the json:\"-\" field stays out of reach") +} diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 924b17d5451..c3a236984f4 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -298,10 +298,10 @@ func directCandidates(t reflect.Type, key string, prefix []int) []candidate { if sf.Name == EmbeddedSliceFieldName || IsFlattenedEmbed(sf) { continue } - name := structtag.JSONTag(sf.Tag.Get("json")).Name() - if name == "-" { + if IsSkippedField(sf) { continue } + name := structtag.JSONTag(sf.Tag.Get("json")).Name() tagged := name != "" if !tagged { name = sf.Name @@ -350,6 +350,13 @@ func pickCandidate(candidates []candidate) (candidate, bool) { return candidate{}, false } +// IsSkippedField reports whether encoding/json omits the field entirely. Only the exact tag +// `json:"-"` does that: `json:"-,"` and `json:"-,omitempty"` name the field "-", which is a +// distinction structtag's parsed name alone cannot carry, since it reports "-" for both. +func IsSkippedField(sf reflect.StructField) bool { + return sf.Tag.Get("json") == "-" +} + // IsFlattenedEmbed reports whether the field is an embed encoding/json flattens into the // outer object. An anonymous field that carries a json *name* is a named field instead: it // serializes as a nested object under that name. The name is what matters, not the presence diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index c583d6d1759..a7b3f7a6ee6 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -224,7 +224,7 @@ func diffStruct(ctx *diffContext, path *structpath.PathNode, s1, s2 reflect.Valu // Resolve field name from JSON tag or fall back to Go field name fieldName := jsonTag.Name() - if fieldName == "-" { + if structaccess.IsSkippedField(sf) { continue } diff --git a/libs/structs/structdiff/equal.go b/libs/structs/structdiff/equal.go index bc75fed318f..f4f06b1b870 100644 --- a/libs/structs/structdiff/equal.go +++ b/libs/structs/structdiff/equal.go @@ -120,7 +120,7 @@ func equalStruct(s1, s2 reflect.Value) bool { jsonTag := structtag.JSONTag(sf.Tag.Get("json")) // Skip fields with json:"-" - if jsonTag.Name() == "-" { + if structaccess.IsSkippedField(sf) { continue } diff --git a/libs/structs/structwalk/walk.go b/libs/structs/structwalk/walk.go index 32084458d2b..3db4dd749a2 100644 --- a/libs/structs/structwalk/walk.go +++ b/libs/structs/structwalk/walk.go @@ -124,8 +124,8 @@ func walkStruct(path *structpath.PathNode, s reflect.Value, visit VisitFunc) { } jsonTag := structtag.JSONTag(sf.Tag.Get("json")) - if jsonTag.Name() == "-" { - continue // skip fields without json name + if structaccess.IsSkippedField(sf) { + continue // encoding/json omits it entirely } // Resolve field name from JSON tag or fall back to Go field name diff --git a/libs/structs/structwalk/walktype.go b/libs/structs/structwalk/walktype.go index f189f2d0251..ce6e083835f 100644 --- a/libs/structs/structwalk/walktype.go +++ b/libs/structs/structwalk/walktype.go @@ -122,7 +122,7 @@ func walkTypeStruct(path *structpath.PatternNode, st reflect.Type, visit VisitTy // Skip fields marked as "-" in json tag jsonTagName := structtag.JSONTag(jsonTag).Name() - if jsonTagName == "-" { + if structaccess.IsSkippedField(sf) { continue } diff --git a/libs/structs/structwalk/walktype_test.go b/libs/structs/structwalk/walktype_test.go index b3c6b5f877e..dc1783cb4c6 100644 --- a/libs/structs/structwalk/walktype_test.go +++ b/libs/structs/structwalk/walktype_test.go @@ -55,6 +55,10 @@ func TestTypeScalar(t *testing.T) { func TestTypes(t *testing.T) { assert.Equal(t, map[string]any{ + // IgnoredFieldOdd and IgnoredFieldOddPtr are tagged `json:"-,omitempty"`, which names + // them "-" rather than omitting them: only the exact tag `json:"-"` is a skip. The two + // collide on that one name, so the walk reports it once. + "-": "", "ArrayString[*]": "", "Array[*].X": 0, "BoolField": false, From cfbd5a521915184d51d054ac3149d59aa2d8f81b Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:50:40 +0200 Subject: [PATCH 17/28] structstest: check the bundle's resource types against encoding/json encoding/json decides which fields exist on the wire, under which names, at which paths. Every libs/structs package claims to speak that vocabulary -- structwalk enumerates it, structaccess reads and writes it, structdiff reports changes in it -- so a disagreement is a bug in one of them, or in the type. The new package fills every field of a type with a non-zero value, marshals it, and compares the result against structwalk's leaves and structaccess.Get/ValidatePath. The test drives it off config.Resources by reflection, so a newly added resource is covered without touching the test. It finds two things today, both enumerated rather than fixed here: - Eleven resource types embed a struct that declares MarshalJSON and declare none of their own, so the embedded marshaler takes over and id, url, lifecycle and permissions never reach the wire. Nothing marshals a config type today (bundle validate -o json marshals the dyn tree), so this is latent. - Free-form any fields and types that marshal themselves as a scalar (duration.Duration, the SDK time wrapper) are never visited by structwalk, so structdiff cannot report drift on them. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 97 +++++ bundle/config/structstest/structstest.go | 431 ++++++++++++++++++++ 2 files changed, 528 insertions(+) create mode 100644 bundle/config/structstest/resources_test.go create mode 100644 bundle/config/structstest/structstest.go diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go new file mode 100644 index 00000000000..95098bb0d61 --- /dev/null +++ b/bundle/config/structstest/resources_test.go @@ -0,0 +1,97 @@ +package structstest_test + +import ( + "reflect" + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/structstest" + "github.com/databricks/cli/libs/structs/structtag" + "github.com/stretchr/testify/require" +) + +// knownDivergences lists the disagreements the bundle's resource types have with +// encoding/json today. Each entry is a bug somewhere other than this test; the test +// enumerates them so that a *new* disagreement fails while these are worked through. +// +// Nothing in the CLI marshals a resource config type with encoding/json today -- bundle +// validate -o json marshals the dyn tree -- so none of these is user-visible yet. They +// are one json.Marshal away from being so. +var knownDivergences = map[string][]string{ + // The resource type embeds another struct that declares MarshalJSON and does not + // declare its own, so the embedded marshaler takes over and every field the outer + // struct adds -- id, url, lifecycle, permissions -- never reaches the wire. Fixed by + // giving the resource type the marshaler pair resources.Job has. + "dashboards": baseResourceFields("file_path", "permissions"), + "genie_spaces": baseResourceFields("file_path", "permissions"), + "database_instances": baseResourceFields("permissions"), + "database_catalogs": baseResourceFields(), + "synced_database_tables": baseResourceFields(), + "postgres_projects": baseResourceFields("permissions"), + "postgres_branches": baseResourceFields(), + "postgres_endpoints": baseResourceFields(), + "postgres_catalogs": baseResourceFields(), + "postgres_databases": baseResourceFields(), + "postgres_roles": baseResourceFields(), + "postgres_synced_tables": baseResourceFields(), +} + +// interfaceFieldPaths are free-form any fields. structwalk documents that it does not +// traverse an interface, so these never reach a visit callback and structdiff never +// reports a change to one. Intentional, but it means drift in a serialized dashboard or a +// cluster policy definition is invisible to the packages. +var interfaceFieldPaths = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, +} + +// baseResourceFields returns the paths a resource gains from BaseResource, plus any extra +// fields the resource declares alongside it. They are lost together, by one cause. +func baseResourceFields(extra ...string) []string { + return append([]string{"id", "url", "modified_status", "lifecycle.prevent_destroy"}, extra...) +} + +// TestResourceTypesAgreeWithJSON feeds every resource type in config.Resources through +// structstest.Check. Driving it off the struct by reflection means a newly added resource +// is covered without touching this test. +func TestResourceTypesAgreeWithJSON(t *testing.T) { + rt := reflect.TypeOf(config.Resources{}) + + var checked int + for i := range rt.NumField() { + field := rt.Field(i) + if field.Type.Kind() != reflect.Map { + continue + } + elem := field.Type.Elem() + if elem.Kind() != reflect.Pointer || elem.Elem().Kind() != reflect.Struct { + continue + } + group := structtag.JSONTag(field.Tag.Get("json")).Name() + + t.Run(group, func(t *testing.T) { + report, err := structstest.Check(elem) + require.NoError(t, err) + + var known []string + known = append(known, knownDivergences[group]...) + known = append(known, interfaceFieldPaths[group]...) + report = report.Filter(known) + if len(report.SelfMarshalingScalars) > 0 { + // A known structwalk limitation, tracked as one item rather than one entry per + // timestamp field, because every new SDK time field joins it. + t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) + report.SelfMarshalingScalars = nil + } + require.True(t, report.Empty(), + "%s (%s) disagrees with encoding/json:%s", group, elem, report) + }) + checked++ + } + + // A guard against the loop silently matching nothing, which would make the whole + // test vacuous. + require.Greater(t, checked, 20, "expected every resource group to be checked") +} diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go new file mode 100644 index 00000000000..9867361eeb1 --- /dev/null +++ b/bundle/config/structstest/structstest.go @@ -0,0 +1,431 @@ +// Package structstest checks that the libs/structs packages agree with encoding/json +// about a type. +// +// encoding/json is the oracle: it decides which fields exist on the wire, under which +// names, and at which paths. Every libs/structs package claims to speak that same +// vocabulary -- structwalk enumerates it, structaccess reads and writes it, structdiff +// reports changes in it -- so any disagreement is a bug in one of them, or in the type. +// +// The package is consumed by tests (bundle/config and bundle/direct/dresources feed it +// the bundle's own resource, state and remote types) but is not itself a _test package, +// because those two callers live in different trees. +package structstest + +import ( + "encoding/json" + "fmt" + "reflect" + "slices" + "sort" + "strconv" + "strings" + + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/cli/libs/structs/structtag" + "github.com/databricks/cli/libs/structs/structwalk" +) + +// Report lists the disagreements found for one type. A field is identified by the JSON +// path encoding/json puts it at, which is the only name all the packages share. +type Report struct { + // WalkMissing are paths encoding/json emits that structwalk never visits, so + // structdiff cannot see a change to them either. + WalkMissing []string + + // WalkExtra are paths structwalk visits that encoding/json does not emit. Usually a + // type that embeds another with its own MarshalJSON and does not define one itself: + // the embedded marshaler takes over and the outer fields never reach the wire. + WalkExtra []string + + // GetFailed are paths encoding/json emits that structaccess.Get cannot resolve. + GetFailed []string + + // ValidateFailed are paths encoding/json emits that structaccess.ValidatePath + // rejects against the type, even though Get resolves them on the value. + ValidateFailed []string + + // ValueMismatch are paths where structaccess.Get and encoding/json disagree about + // the value stored at the path. + ValueMismatch []string + + // SelfMarshalingScalars are paths whose Go type is a struct that marshals itself as a + // scalar through its own MarshalJSON -- duration.Duration and the SDK's time wrapper. + // structwalk looks for scalar *fields* and finds none inside them, so it never visits + // them and structdiff never reports drift on them. One known limitation rather than a + // per-field list, because every new timestamp field in the SDK joins it. + SelfMarshalingScalars []string +} + +// Empty reports whether the type and the libs/structs packages agree completely. +func (r Report) Empty() bool { + return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && + len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 +} + +// String renders the report as one indented line per category, for a test failure message. +func (r Report) String() string { + var b strings.Builder + for _, s := range []struct { + label string + paths []string + }{ + {"structwalk does not visit, encoding/json emits", r.WalkMissing}, + {"structwalk visits, encoding/json does not emit", r.WalkExtra}, + {"structaccess.Get cannot resolve", r.GetFailed}, + {"structaccess.ValidatePath rejects", r.ValidateFailed}, + {"structaccess.Get and encoding/json disagree on the value at", r.ValueMismatch}, + {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, + } { + if len(s.paths) == 0 { + continue + } + fmt.Fprintf(&b, "\n %s (%d):", s.label, len(s.paths)) + for _, p := range s.paths { + fmt.Fprintf(&b, "\n %s", p) + } + } + return b.String() +} + +// Check fills every field of a fresh value of type t with a non-zero value, marshals it +// with encoding/json, and compares the result against what structwalk and structaccess +// make of the same value. t must be a struct or a pointer to one. +func Check(t reflect.Type) (Report, error) { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return Report{}, fmt.Errorf("structstest: %s is not a struct", t) + } + + ptr := reflect.New(t) + FillNonZero(ptr.Elem()) + v := ptr.Interface() + + jsonLeaves, selfMarshaling, err := jsonLeaves(v) + if err != nil { + return Report{}, err + } + + walkLeaves := map[string]string{} + err = structwalk.Walk(v, func(path *structpath.PathNode, val any, _ *reflect.StructField) { + walkLeaves[path.String()] = render(val) + }) + if err != nil { + return Report{}, fmt.Errorf("structstest: walk %s: %w", t, err) + } + + var report Report + for path, want := range jsonLeaves { + if selfMarshaling[path] { + report.SelfMarshalingScalars = append(report.SelfMarshalingScalars, path) + continue + } + if _, ok := walkLeaves[path]; !ok { + report.WalkMissing = append(report.WalkMissing, path) + } + + node, err := structpath.ParsePath(path) + if err != nil { + report.GetFailed = append(report.GetFailed, path+": "+err.Error()) + continue + } + if skippedByTag(reflect.TypeOf(v), node) { + // structaccess refuses bundle:"internal" and bundle:"readonly" fields by + // design; encoding/json still emits them. + continue + } + if err := structaccess.ValidatePath(reflect.TypeOf(v), node); err != nil { + report.ValidateFailed = append(report.ValidateFailed, path+": "+err.Error()) + } + got, err := structaccess.Get(v, node) + if err != nil { + report.GetFailed = append(report.GetFailed, path+": "+err.Error()) + continue + } + if render(got) != want { + report.ValueMismatch = append(report.ValueMismatch, + fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) + } + } + for path := range walkLeaves { + if _, ok := jsonLeaves[path]; !ok { + report.WalkExtra = append(report.WalkExtra, path) + } + } + + sort.Strings(report.WalkMissing) + sort.Strings(report.WalkExtra) + sort.Strings(report.GetFailed) + sort.Strings(report.ValidateFailed) + sort.Strings(report.ValueMismatch) + sort.Strings(report.SelfMarshalingScalars) + return report, nil +} + +// JSONLeaves marshals v and returns its scalar leaves keyed by the structpath rendering +// of their location, which is the dialect every libs/structs package speaks. +func JSONLeaves(v any) (map[string]string, error) { + leaves, _, err := jsonLeaves(v) + return leaves, err +} + +func jsonLeaves(v any) (map[string]string, map[string]bool, error) { + blob, err := json.Marshal(v) + if err != nil { + return nil, nil, fmt.Errorf("structstest: marshal %T: %w", v, err) + } + var generic any + if err := json.Unmarshal(blob, &generic); err != nil { + return nil, nil, fmt.Errorf("structstest: unmarshal %T: %w", v, err) + } + out := map[string]string{} + selfMarshaling := map[string]bool{} + flatten(nil, reflect.TypeOf(v), generic, out, selfMarshaling) + return out, selfMarshaling, nil +} + +// flatten walks the decoded JSON alongside the Go type, because the path syntax for an +// object member depends on which one it is: a struct field is .name, a map entry is +// ['name'], and only the type knows the difference. +func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string]string, selfMarshaling map[string]bool) { + for typ != nil && typ.Kind() == reflect.Pointer { + typ = typ.Elem() + } + + switch value := v.(type) { + case map[string]any: + isMap := typ != nil && typ.Kind() == reflect.Map + for key, member := range value { + var next *structpath.PathNode + var memberType reflect.Type + if isMap { + next = structpath.NewBracketString(path, key) + memberType = typ.Elem() + } else { + next = structpath.NewStringKey(path, key) + if typ != nil && typ.Kind() == reflect.Struct { + if sf, ok := embeddedSliceField(typ, key); ok { + // An EmbeddedSlice field is transparent by design: the walkers put + // its elements at the parent path, while the wire format keeps the + // __embed__ key. Follow the walkers, or every such type reads as a + // disagreement when it is really the convention working. + flatten(path, sf.Type, member, out, selfMarshaling) + continue + } + if sf, _, ok := structaccess.FindStructFieldByKeyType(typ, key); ok { + memberType = sf.Type + } + } + } + flatten(next, memberType, member, out, selfMarshaling) + } + case []any: + var elemType reflect.Type + if typ != nil && (typ.Kind() == reflect.Slice || typ.Kind() == reflect.Array) { + elemType = typ.Elem() + } + for i, member := range value { + flatten(structpath.NewIndex(path, i), elemType, member, out, selfMarshaling) + } + case nil: + // A JSON null carries no scalar leaf. + default: + out[path.String()] = render(value) + // A scalar on the wire whose Go type is a struct marshalled itself: the walkers + // cannot see inside it. + if typ != nil && typ.Kind() == reflect.Struct { + selfMarshaling[path.String()] = true + } + } +} + +// embeddedSliceField reports whether key names the struct's EmbeddedSlice field. +func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool) { + for i := range typ.NumField() { + sf := typ.Field(i) + if sf.Name != structaccess.EmbeddedSliceFieldName { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == key { + return sf, true + } + } + return reflect.StructField{}, false +} + +// render normalises a scalar so a value decoded from JSON and the same value read out of +// the struct compare equal: JSON numbers decode to float64, the struct holds int64 and +// friends, and a nil pointer reads back as nil. +func render(v any) string { + if v == nil { + return "" + } + rv := reflect.ValueOf(v) + for rv.Kind() == reflect.Pointer { + if rv.IsNil() { + return "" + } + rv = rv.Elem() + } + switch rv.Kind() { + case reflect.Float32, reflect.Float64: + return strconv.FormatFloat(rv.Float(), 'g', -1, 64) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return strconv.FormatFloat(float64(rv.Int()), 'g', -1, 64) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return strconv.FormatFloat(float64(rv.Uint()), 'g', -1, 64) + default: + return fmt.Sprintf("%v", rv.Interface()) + } +} + +// skippedByTag reports whether the last segment of path names a field structaccess +// refuses on purpose. The lookup goes through embedded structs, so a field promoted from +// BaseResource is recognised too. +func skippedByTag(typ reflect.Type, path *structpath.PathNode) bool { + nodes := path.AsSlice() + cur := typ + for i, node := range nodes { + for cur.Kind() == reflect.Pointer { + cur = cur.Elem() + } + key, ok := node.StringKey() + if !ok || cur.Kind() != reflect.Struct { + return false + } + sf, _, found := structaccess.FindStructFieldByKeyType(cur, key) + if !found { + // structaccess drops internal and readonly fields from the type-level + // lookup as well, so a miss on the last segment is the tag talking. + return i == len(nodes)-1 && taggedInternal(cur, key) + } + cur = sf.Type + } + return false +} + +// taggedInternal reports whether the struct, or a struct it embeds, declares key with +// bundle:"internal" or bundle:"readonly". +func taggedInternal(typ reflect.Type, key string) bool { + for i := range typ.NumField() { + sf := typ.Field(i) + if !sf.IsExported() { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == key { + bt := structtag.BundleTag(sf.Tag.Get("bundle")) + if bt.Internal() || bt.ReadOnly() { + return true + } + } + if !sf.Anonymous { + continue + } + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + if ft.Kind() == reflect.Struct && taggedInternal(ft, key) { + return true + } + } + return false +} + +// FillNonZero populates every serializable field of v with a non-zero value, so that a +// field the packages disagree about is always observable rather than indistinguishable +// from an omitted zero. Recursion is bounded because SDK types are self-referential. +func FillNonZero(v reflect.Value) { fillNonZero(v, 0) } + +func fillNonZero(v reflect.Value, depth int) { + if depth > 5 { + return + } + switch v.Kind() { + case reflect.Bool: + v.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + v.SetInt(1) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + v.SetUint(1) + case reflect.Float32, reflect.Float64: + v.SetFloat(1) + case reflect.String: + v.SetString("x") + case reflect.Pointer: + v.Set(reflect.New(v.Type().Elem())) + fillNonZero(v.Elem(), depth+1) + case reflect.Slice: + elem := reflect.New(v.Type().Elem()).Elem() + fillNonZero(elem, depth+1) + v.Set(reflect.Append(v, elem)) + case reflect.Map: + v.Set(reflect.MakeMap(v.Type())) + val := reflect.New(v.Type().Elem()).Elem() + fillNonZero(val, depth+1) + v.SetMapIndex(reflect.ValueOf("k").Convert(v.Type().Key()), val) + case reflect.Interface: + // A free-form any field gets a scalar, not a map: structwalk deliberately does not + // traverse into an interface, so a composite here would show up as a path mismatch + // that says nothing about the type under test. + v.Set(reflect.ValueOf("x")) + case reflect.Struct: + for i := range v.Type().NumField() { + sf := v.Type().Field(i) + if !sf.IsExported() || sf.Name == "ForceSendFields" { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == "-" { + continue + } + fillNonZero(v.Field(i), depth+1) + } + default: + // Kinds that do not appear in bundle or SDK types (chan, func, complex) stay zero. + } +} + +// coveredBy reports whether path equals one of the entries or sits underneath it. +func coveredBy(known []string, path string) bool { + if slices.Contains(known, path) { + return true + } + for _, k := range known { + if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { + return true + } + } + return false +} + +// KnownDivergence records a disagreement that exists today and is not this test's to fix. +// Every entry must say why it is here and what removes it. +type KnownDivergence struct { + Type string + Paths []string + Reason string +} + +// Filter removes the known divergences from a report, so the test fails only on new ones. +// An entry covers the path it names and everything under it, so listing a field that is +// lost wholesale does not mean enumerating each of its leaves. +func (r Report) Filter(known []string) Report { + drop := func(paths []string) []string { + var out []string + for _, p := range paths { + if !coveredBy(known, strings.SplitN(p, ":", 2)[0]) { + out = append(out, p) + } + } + return out + } + return Report{ + WalkMissing: drop(r.WalkMissing), + WalkExtra: drop(r.WalkExtra), + GetFailed: drop(r.GetFailed), + ValidateFailed: drop(r.ValidateFailed), + ValueMismatch: drop(r.ValueMismatch), + } +} From de29a416c5991b33554b1eb95b17b15c64d596c6 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:50:55 +0200 Subject: [PATCH 18/28] dresources: check StateType and RemoteType against encoding/json These are the types the direct engine actually reads fields out of: the plan resolves a ${resources...} reference by walking them, the state file is the JSON encoding of StateType, and a refresh decodes into RemoteType. A disagreement here is not latent the way it is for the config types -- it is a field the plan cannot see or the state file cannot carry. assertJSONRoundTrip already covers whether a wrapper loses fields across Marshal -> Unmarshal. This covers whether the packages and encoding/json name and reach the same fields at all. Both flavours pass for every registered resource once the EmbeddedSlice convention is accounted for: __embed__ is transparent to the walkers by design, so the check follows them rather than the literal wire key. Co-authored-by: Isaac --- bundle/direct/dresources/structs_test.go | 72 ++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 bundle/direct/dresources/structs_test.go diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go new file mode 100644 index 00000000000..0bd697a80b8 --- /dev/null +++ b/bundle/direct/dresources/structs_test.go @@ -0,0 +1,72 @@ +package dresources + +import ( + "reflect" + "testing" + + "github.com/databricks/cli/bundle/config/structstest" + "github.com/stretchr/testify/require" +) + +// StateType and RemoteType are the types the direct engine actually reads fields out of: +// the plan resolves a ${resources...} reference by walking them, the state file is the +// JSON encoding of StateType, and RemoteType is what a refresh decodes into. So a +// disagreement between them and encoding/json is not latent the way it is for the config +// types -- it is a field the plan cannot see or the state file cannot carry. +// +// assertJSONRoundTrip in serialize_test.go covers the neighbouring question, whether a +// wrapper loses fields across Marshal -> Unmarshal. This covers whether the libs/structs +// packages and encoding/json name and reach the same fields in the first place. + +// knownStateDivergences and knownRemoteDivergences enumerate what disagrees today, so a +// new disagreement fails the test while these are worked through. +var ( + knownStateDivergences = map[string][]string{ + // Free-form any fields: structwalk documents that it does not traverse an interface, + // so drift inside a serialized dashboard or a cluster policy definition is invisible + // to it and to structdiff. + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, + } + + knownRemoteDivergences = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, + } +) + +func TestStateTypeAgreesWithJSON(t *testing.T) { + testAgreesWithJSON(t, (*Adapter).StateType, knownStateDivergences) +} + +func TestRemoteTypeAgreesWithJSON(t *testing.T) { + testAgreesWithJSON(t, (*Adapter).RemoteType, knownRemoteDivergences) +} + +// testAgreesWithJSON runs the check for every registered resource, so a newly supported +// resource type is covered without touching this test. +func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known map[string][]string) { + for resourceType, resource := range SupportedResources { + adapter, err := NewAdapter(resource, resourceType, nil) + require.NoError(t, err) + + t.Run(resourceType, func(t *testing.T) { + typ := typeOf(adapter) + report, err := structstest.Check(typ) + require.NoError(t, err) + + report = report.Filter(known[resourceType]) + if len(report.SelfMarshalingScalars) > 0 { + // A known structwalk limitation, tracked as one item rather than one entry per + // timestamp field, because every new SDK time field joins it. + t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) + report.SelfMarshalingScalars = nil + } + require.True(t, report.Empty(), + "%s (%s) disagrees with encoding/json:%s", resourceType, typ, report) + }) + } +} From 48e620fe2dea315451342b961f18affcc5a51bd5 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:02:07 +0200 Subject: [PATCH 19/28] structstest: use slices.Sort The repo forbids sort.Strings in favour of the standard library's generic version. --- bundle/config/structstest/structstest.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 9867361eeb1..6deccf010fa 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -16,7 +16,6 @@ import ( "fmt" "reflect" "slices" - "sort" "strconv" "strings" @@ -155,12 +154,12 @@ func Check(t reflect.Type) (Report, error) { } } - sort.Strings(report.WalkMissing) - sort.Strings(report.WalkExtra) - sort.Strings(report.GetFailed) - sort.Strings(report.ValidateFailed) - sort.Strings(report.ValueMismatch) - sort.Strings(report.SelfMarshalingScalars) + slices.Sort(report.WalkMissing) + slices.Sort(report.WalkExtra) + slices.Sort(report.GetFailed) + slices.Sort(report.ValidateFailed) + slices.Sort(report.ValueMismatch) + slices.Sort(report.SelfMarshalingScalars) return report, nil } @@ -243,8 +242,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] // embeddedSliceField reports whether key names the struct's EmbeddedSlice field. func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool) { - for i := range typ.NumField() { - sf := typ.Field(i) + for sf := range typ.Fields() { if sf.Name != structaccess.EmbeddedSliceFieldName { continue } @@ -309,8 +307,7 @@ func skippedByTag(typ reflect.Type, path *structpath.PathNode) bool { // taggedInternal reports whether the struct, or a struct it embeds, declares key with // bundle:"internal" or bundle:"readonly". func taggedInternal(typ reflect.Type, key string) bool { - for i := range typ.NumField() { - sf := typ.Field(i) + for sf := range typ.Fields() { if !sf.IsExported() { continue } From 2772ed3596206fa8d87b14753787dd7b1edc037d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:02:07 +0200 Subject: [PATCH 20/28] libs/structs: check each package against encoding/json on a shared shape corpus The corpus in libs/structs/internal/jsonshapes pairs a struct shape whose JSON behaviour is easy to get wrong -- two levels of embedding, a shadowed name, a same-depth collision, a diamond, a cyclic embed, an embed behind a nil pointer -- with the fields encoding/json actually serializes for it. Its own test asserts those expectations against json.Marshal, so the corpus cannot teach every consumer the same wrong answer. Each package is then checked against it on its own terms: structaccess must read, write and validate exactly what the wire carries, and the write assertion goes through json.Marshal so a Set into a field the wire format ignores fails; structwalk must visit exactly those paths; structdiff must report a change to each of them and none to a field encoding/json drops. structpath and structtag have no corpus to check against, so they are pinned directly: a rendered path must survive being parsed again (a map key with dots is the case that matters), and a json tag must resolve to the name encoding/json chose for it. Three disagreements are recorded rather than fixed, each as a ratchet that asserts the disagreement is still present, so fixing one breaks the test and forces the entry out: - structwalk visits every declaration of a shadowed embedded field, so it reports the field twice while the wire format carries one value. - structwalk and structdiff both expose an ambiguous embedded field that encoding/json refuses to serialize, so the engine can plan an update that can never be sent. - structaccess.Set will not descend through a nil embedded pointer, so a field json.Unmarshal reaches by allocating it cannot be written. Co-authored-by: Isaac --- .../structs/internal/jsonshapes/jsonshapes.go | 240 ++++++++++++++++++ .../internal/jsonshapes/jsonshapes_test.go | 40 +++ .../structaccess/jsonagreement_test.go | 84 ++++++ libs/structs/structdiff/jsonagreement_test.go | 93 +++++++ libs/structs/structpath/jsonagreement_test.go | 80 ++++++ libs/structs/structtag/jsonagreement_test.go | 102 ++++++++ libs/structs/structwalk/jsonagreement_test.go | 100 ++++++++ 7 files changed, 739 insertions(+) create mode 100644 libs/structs/internal/jsonshapes/jsonshapes.go create mode 100644 libs/structs/internal/jsonshapes/jsonshapes_test.go create mode 100644 libs/structs/structaccess/jsonagreement_test.go create mode 100644 libs/structs/structdiff/jsonagreement_test.go create mode 100644 libs/structs/structpath/jsonagreement_test.go create mode 100644 libs/structs/structtag/jsonagreement_test.go create mode 100644 libs/structs/structwalk/jsonagreement_test.go diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go new file mode 100644 index 00000000000..ab91a31f33d --- /dev/null +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -0,0 +1,240 @@ +// Package jsonshapes is a corpus of struct shapes whose JSON behaviour is easy to get +// wrong, shared by the tests of the libs/structs packages. +// +// Each shape pairs a value with the fields encoding/json actually serializes for it, so a +// package can be checked against the wire format without restating the reasoning. The +// interesting shapes are all about embedding: how deep a promoted field may sit, which of +// two same-named fields wins, and when encoding/json gives up and serializes neither. +package jsonshapes + +import "slices" + +// Shape is one struct whose JSON behaviour a libs/structs package must match. +type Shape struct { + // Name identifies the shape in test output. + Name string + + // Value is a pointer to a populated struct. + Value any + + // JSONFields are the json names encoding/json emits for Value, in any order. They are + // asserted against json.Marshal by TestCorpusMatchesEncodingJSON, so a shape whose + // expectation drifts from reality fails in the corpus itself rather than silently + // teaching every consumer the wrong thing. + JSONFields []string + + // TypeFields are the json names a type-level walk should yield, which differs from + // JSONFields only where a value cannot reach what its type declares: an embedded nil + // pointer contributes its fields to the type but nothing to the wire. Empty means + // "same as JSONFields". + TypeFields []string + + // KnownGaps names the entry points that disagree with encoding/json about this shape + // today: "structwalk.Walk", "structwalk.WalkType", "structdiff". The package's own test asserts the disagreement is still there, so fixing + // it breaks the test and the entry has to be removed: a ratchet, not an exemption. + KnownGaps []string + + // KnownSetGap are json names encoding/json can unmarshal into but structaccess.Set + // cannot reach today. Consumers assert that Set still fails for them, so fixing the gap + // breaks the test and the entry has to go -- a ratchet rather than a silent exemption. + KnownSetGap []string + + // Unreachable are json names that look available on the Go type -- some field declares + // them -- but that encoding/json does not serialize, so no package may expose them + // either. Ambiguous embedded fields land here. + Unreachable []string +} + +type Leaf struct { + Value string `json:"value,omitempty"` +} + +type MiddleWithLeaf struct { + Leaf +} + +// DoublyEmbedded reaches a field through two levels of embedding, the shape every postgres +// resource has (PostgresProject -> PostgresProjectConfig -> postgres.ProjectSpec). +type DoublyEmbedded struct { + MiddleWithLeaf + + Own string `json:"own,omitempty"` +} + +type ShallowValue struct { + Value string `json:"value,omitempty"` +} + +type DeepHolder struct { + Leaf +} + +// ShallowWins declares value at two depths. encoding/json takes the shallower one, so Get +// and Set must resolve to the same field the wire format uses. +type ShallowWins struct { + ShallowValue + DeepHolder +} + +type SideA struct { + Value string `json:"value,omitempty"` +} + +type SideB struct { + Value string `json:"value,omitempty"` +} + +// SameDepthConflict declares value twice at one depth. encoding/json calls that ambiguous +// and emits neither, so the field is not readable or writable either. +type SameDepthConflict struct { + SideA + SideB +} + +type DiamondLeft struct { + Leaf +} + +type DiamondRight struct { + Leaf +} + +// Diamond reaches one Leaf by two routes of equal length: ambiguous, like SameDepthConflict. +type Diamond struct { + DiamondLeft + DiamondRight +} + +type NilSide struct { + Value string `json:"value,omitempty"` +} + +// AmbiguousViaNilPointer declares value twice at one depth, with one side behind a nil +// pointer. encoding/json resolves fields from the type, so it is ambiguous either way and +// neither is serialized -- a value-level search that skips the nil side sees only one +// declaration and wrongly concludes the field is reachable. +type AmbiguousViaNilPointer struct { + *NilSide + SideB +} + +// Cyclic embeds a pointer to itself, so a type-level search that does not remember where it +// has been never terminates. The corpus leaves the pointer nil: encoding/json flattens an +// embedded type once and stops, but a value walk following a self-referential pointer is +// genuinely unbounded, so a populated cycle is not a shape the packages can be compared on. +// structaccess exercises the populated case in its own test. +type Cyclic struct { + *Cyclic + + Name string `json:"name,omitempty"` +} + +// NilPointerEmbed embeds a pointer left nil. Whether a path resolves is a property of the +// type, so it must not depend on the pointer being set. +type NilPointerEmbed struct { + *Leaf + + Own string `json:"own,omitempty"` +} + +// SetPointerEmbed is NilPointerEmbed with the pointer populated. +type SetPointerEmbed struct { + *Leaf + + Own string `json:"own,omitempty"` +} + +// SkippedField declares a field encoding/json never serializes. +type SkippedField struct { + Kept string `json:"kept,omitempty"` + Skipped string `json:"-"` + + unexported string //nolint:unused // present to prove it is ignored +} + +// Gap reports whether the named entry point is recorded as disagreeing about this shape. +func (s Shape) Gap(entryPoint string) bool { + return slices.Contains(s.KnownGaps, entryPoint) +} + +// Fields returns the names a type-level walk should yield for the shape. +func (s Shape) Fields() []string { + if len(s.TypeFields) > 0 { + return s.TypeFields + } + return s.JSONFields +} + +// Shapes returns the corpus. Each call builds fresh values so a test may mutate them. +func Shapes() []Shape { + return []Shape{ + { + Name: "doubly embedded", + Value: &DoublyEmbedded{MiddleWithLeaf: MiddleWithLeaf{Leaf: Leaf{Value: "v"}}, Own: "o"}, + JSONFields: []string{"value", "own"}, + }, + { + Name: "shallower embed wins", + Value: &ShallowWins{ShallowValue: ShallowValue{Value: "shallow"}, DeepHolder: DeepHolder{Leaf: Leaf{Value: "deep"}}}, + JSONFields: []string{"value"}, + // Both walks visit each declaration, so a shadowed field is reported twice while the + // wire format carries one value. + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType"}, + }, + { + Name: "same depth conflict", + Value: &SameDepthConflict{SideA: SideA{Value: "a"}, SideB: SideB{Value: "b"}}, + JSONFields: nil, + Unreachable: []string{"value"}, + // structaccess reports the name as not found, as encoding/json does. The walks still + // visit both declarations and structdiff still reports a change at the path, so the + // engine can plan an update for a field that cannot be serialized. + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + }, + { + Name: "diamond", + Value: &Diamond{DiamondLeft: DiamondLeft{Leaf: Leaf{Value: "l"}}, DiamondRight: DiamondRight{Leaf: Leaf{Value: "r"}}}, + JSONFields: nil, + Unreachable: []string{"value"}, + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + }, + { + Name: "ambiguous via nil pointer", + Value: &AmbiguousViaNilPointer{SideB: SideB{Value: "b"}}, //exhaustruct:ignore + JSONFields: nil, + Unreachable: []string{"value"}, + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + }, + { + Name: "cyclic embed", + Value: &Cyclic{Name: "n"}, //exhaustruct:ignore + JSONFields: []string{"name"}, + // The embedded *Cyclic promotes name a level down, where encoding/json shadows it + // with the outer one. The type walk reports both; the value walk agrees, because the + // corpus leaves the pointer nil. + KnownGaps: []string{"structwalk.WalkType"}, + }, + { + Name: "nil pointer embed", + Value: &NilPointerEmbed{Own: "o"}, //exhaustruct:ignore + JSONFields: []string{"own"}, + TypeFields: []string{"value", "own"}, + }, + { + Name: "set pointer embed", + Value: &SetPointerEmbed{Leaf: &Leaf{Value: "v"}, Own: "o"}, + JSONFields: []string{"value", "own"}, + // json.Unmarshal allocates the embedded pointer to reach value; Set refuses to + // descend through a nil one, so a fresh value cannot be written through. Fixing it + // means allocating only once the write is known to succeed, or a failed Set leaves + // an allocated embed behind and changes what the type marshals to. + KnownSetGap: []string{"value"}, + }, + { + Name: "skipped field", + Value: &SkippedField{Kept: "k", Skipped: "s"}, //exhaustruct:ignore + JSONFields: []string{"kept"}, + Unreachable: []string{"-"}, + }, + } +} diff --git a/libs/structs/internal/jsonshapes/jsonshapes_test.go b/libs/structs/internal/jsonshapes/jsonshapes_test.go new file mode 100644 index 00000000000..9ebebae226f --- /dev/null +++ b/libs/structs/internal/jsonshapes/jsonshapes_test.go @@ -0,0 +1,40 @@ +package jsonshapes_test + +import ( + "encoding/json" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCorpusMatchesEncodingJSON keeps the corpus honest: every shape's declared JSONFields +// are what encoding/json actually emits, and nothing it declares unreachable shows up. +// Without this the corpus could teach every consumer the same wrong answer. +func TestCorpusMatchesEncodingJSON(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + blob, err := json.Marshal(shape.Value) + require.NoError(t, err) + + var got map[string]any + require.NoError(t, json.Unmarshal(blob, &got)) + + var names []string + for name := range got { + names = append(names, name) + } + slices.Sort(names) + want := append([]string(nil), shape.JSONFields...) + slices.Sort(want) + assert.Equal(t, want, names, "encoding/json emitted %s", blob) + + for _, name := range shape.Unreachable { + assert.NotContains(t, got, name, + "%q is declared unreachable but encoding/json emitted it", name) + } + }) + } +} diff --git a/libs/structs/structaccess/jsonagreement_test.go b/libs/structs/structaccess/jsonagreement_test.go new file mode 100644 index 00000000000..2a68de4944f --- /dev/null +++ b/libs/structs/structaccess/jsonagreement_test.go @@ -0,0 +1,84 @@ +package structaccess_test + +import ( + "encoding/json" + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAgreesWithEncodingJSON checks structaccess against encoding/json over the shape +// corpus: a name the wire format carries must be readable, writable and valid, and a name +// it does not carry must be none of those. +// +// The write assertion goes through json.Marshal rather than the Go field, so it fails if +// Set stores into a field the wire format ignores -- which is the failure mode a shadowed +// or ambiguous embed produces, and the one a Go-field assertion cannot see. +func TestAgreesWithEncodingJSON(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + for _, name := range shape.JSONFields { + if slices.Contains(shape.KnownSetGap, name) { + // Read side still has to agree; the write side is a recorded gap, asserted as + // failing so that fixing it forces the entry out of the corpus. + require.NoError(t, structaccess.ValidateByString(reflect.TypeOf(shape.Value), name)) + _, err := structaccess.GetByString(shape.Value, name) + require.NoError(t, err) + require.Error(t, structaccess.SetByString(freshLike(shape.Value), name, "written"), + "KnownSetGap %q now works -- remove it from the corpus", name) + continue + } + + require.NoError(t, structaccess.ValidateByString(reflect.TypeOf(shape.Value), name), + "%q is on the wire, so the type must validate it", name) + + _, err := structaccess.GetByString(shape.Value, name) + require.NoError(t, err, "%q is on the wire, so Get must resolve it", name) + + fresh := freshLike(shape.Value) + require.NoError(t, structaccess.SetByString(fresh, name, "written"), + "%q is on the wire, so Set must reach it", name) + + blob, err := json.Marshal(fresh) + require.NoError(t, err) + var got map[string]any + require.NoError(t, json.Unmarshal(blob, &got)) + assert.Equal(t, "written", got[name], + "Set wrote somewhere encoding/json does not serialize: %s", blob) + } + + for _, name := range shape.Unreachable { + assert.Error(t, structaccess.ValidateByString(reflect.TypeOf(shape.Value), name), + "%q never reaches the wire, so the type must not validate it", name) + + _, err := structaccess.GetByString(shape.Value, name) + assert.Error(t, err, "%q never reaches the wire, so Get must not resolve it", name) + + assert.Error(t, structaccess.SetByString(freshLike(shape.Value), name, "written"), + "%q never reaches the wire, so Set must not claim to write it", name) + } + }) + } +} + +// TestValidateIsAPropertyOfTheType checks that whether a path validates does not depend on +// the value: an embedded pointer left nil declares the same fields as one that is set. +func TestValidateIsAPropertyOfTheType(t *testing.T) { + nilEmbed := reflect.TypeFor[*jsonshapes.NilPointerEmbed]() //exhaustruct:ignore + setEmbed := reflect.TypeFor[*jsonshapes.SetPointerEmbed]() //exhaustruct:ignore + + for _, name := range []string{"value", "own"} { + assert.NoError(t, structaccess.ValidateByString(nilEmbed, name)) + assert.NoError(t, structaccess.ValidateByString(setEmbed, name)) + } +} + +// freshLike returns a new zero value of the same type as v, which is a pointer to a struct. +func freshLike(v any) any { + return reflect.New(reflect.TypeOf(v).Elem()).Interface() +} diff --git a/libs/structs/structdiff/jsonagreement_test.go b/libs/structs/structdiff/jsonagreement_test.go new file mode 100644 index 00000000000..eebdfbf79ab --- /dev/null +++ b/libs/structs/structdiff/jsonagreement_test.go @@ -0,0 +1,93 @@ +package structdiff_test + +import ( + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structdiff" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestDiffReportsWhatJSONCarries checks that a change to a field the wire format carries +// shows up in the diff, at the path encoding/json puts it at. A field structdiff cannot see +// is a field the direct engine never sends an update for. +func TestDiffReportsWhatJSONCarries(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + for _, name := range shape.JSONFields { + before := shape.Value + after := cloneWith(t, shape, name, "changed") + if after == nil { + continue // recorded write gap; the read side is covered in structaccess + } + + changes, err := structdiff.GetStructDiff(before, after, nil) + require.NoError(t, err) + + var paths []string + for _, change := range changes { + paths = append(paths, change.Path.String()) + } + assert.Contains(t, paths, name, + "%q changed but structdiff did not report it", name) + } + }) + } +} + +// TestDiffNeverReportsAnUnreachableField checks the other direction: a name encoding/json +// refuses to serialize must never appear in a diff, or the engine would try to send a field +// that cannot exist on the wire. +func TestDiffNeverReportsAnUnreachableField(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + if len(shape.Unreachable) == 0 { + continue + } + t.Run(shape.Name, func(t *testing.T) { + zero := freshLike(shape.Value) + changes, err := structdiff.GetStructDiff(zero, shape.Value, nil) + require.NoError(t, err) + + var reported []string + for _, change := range changes { + if slices.Contains(shape.Unreachable, change.Path.String()) { + reported = append(reported, change.Path.String()) + } + } + if shape.Gap("structdiff") { + assert.NotEmpty(t, reported, + "structdiff no longer reports an unreachable field -- remove the recorded gap") + t.Logf("recorded gap: structdiff reports %v, which encoding/json does not serialize", reported) + return + } + assert.Empty(t, reported, + "structdiff reported %v, which encoding/json does not serialize", reported) + }) + } +} + +// cloneWith returns a copy of the shape's value with one field set, or nil when the shape +// records that structaccess cannot write that field yet. +func cloneWith(t *testing.T, shape jsonshapes.Shape, name, value string) any { + t.Helper() + + clone := freshLike(shape.Value) + if err := structaccess.SetByString(clone, name, value); err != nil { + for _, gap := range shape.KnownSetGap { + if gap == name { + return nil + } + } + t.Fatalf("cannot set %q: %v", name, err) + } + return clone +} + +// freshLike returns a new zero value of the same type as v, a pointer to a struct. +func freshLike(v any) any { + return reflect.New(reflect.TypeOf(v).Elem()).Interface() +} diff --git a/libs/structs/structpath/jsonagreement_test.go b/libs/structs/structpath/jsonagreement_test.go new file mode 100644 index 00000000000..185e322f74f --- /dev/null +++ b/libs/structs/structpath/jsonagreement_test.go @@ -0,0 +1,80 @@ +package structpath_test + +import ( + "encoding/json" + "testing" + + "github.com/databricks/cli/libs/structs/structpath" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMapKeyRoundTripsThroughRendering checks that a path built from a JSON object key +// survives being rendered and parsed again. Every libs/structs package identifies a field +// by the rendered string, and the corpus tests compare those strings across packages, so a +// key that renders to something ParsePath reads back differently silently makes two +// packages "disagree" about a field they both found. +// +// The keys here are the ones a Databricks API can really return: a Spark conf key has dots, +// a tag value can contain almost anything. +func TestMapKeyRoundTripsThroughRendering(t *testing.T) { + keys := []string{ + "simple", + "spark.databricks.delta.retentionDurationCheck.enabled", + "with space", + "with'quote", + "with\"doublequote", + "with[bracket]", + "with.dot", + "", + "ünïcode", + "123", + } + + for _, key := range keys { + t.Run(key, func(t *testing.T) { + node := structpath.NewBracketString(structpath.NewStringKey(nil, "conf"), key) + rendered := node.String() + + parsed, err := structpath.ParsePath(rendered) + require.NoError(t, err, "rendered as %q", rendered) + assert.Equal(t, rendered, parsed.String(), + "%q does not survive render -> parse -> render", key) + }) + } +} + +// TestIndexRoundTripsThroughRendering does the same for a slice index, which is how every +// package refers to an element of a JSON array. +func TestIndexRoundTripsThroughRendering(t *testing.T) { + for _, index := range []int{0, 1, 9, 10, 12345} { + node := structpath.NewIndex(structpath.NewStringKey(nil, "tasks"), index) + rendered := node.String() + + parsed, err := structpath.ParsePath(rendered) + require.NoError(t, err, "rendered as %q", rendered) + assert.Equal(t, rendered, parsed.String()) + } +} + +// TestRenderedPathAddressesTheSameJSONMember pins the dialect against encoding/json: the +// rendering of a struct field is the object key itself, and the rendering of a map entry is +// the key in brackets. The two are different syntax for the same kind of JSON member, which +// is exactly the distinction a flattener has to get right to compare paths at all. +func TestRenderedPathAddressesTheSameJSONMember(t *testing.T) { + type inner struct { + Conf map[string]string `json:"conf,omitempty"` + } + value := &inner{Conf: map[string]string{"a.b": "v"}} + + blob, err := json.Marshal(value) + require.NoError(t, err) + assert.JSONEq(t, `{"conf":{"a.b":"v"}}`, string(blob)) + + field := structpath.NewStringKey(nil, "conf") + assert.Equal(t, "conf", field.String()) + + entry := structpath.NewBracketString(field, "a.b") + assert.Equal(t, `conf['a.b']`, entry.String(), + "a map entry must not render as a dotted field, or a path with a dotted key reads as two fields") +} diff --git a/libs/structs/structtag/jsonagreement_test.go b/libs/structs/structtag/jsonagreement_test.go new file mode 100644 index 00000000000..7897de63760 --- /dev/null +++ b/libs/structs/structtag/jsonagreement_test.go @@ -0,0 +1,102 @@ +package structtag_test + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/databricks/cli/libs/structs/structtag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestJSONTagNameMatchesEncodingJSON checks structtag's reading of a json tag against what +// encoding/json does with the same tag. Every other package asks structtag for a field's +// name, so a tag it reads differently renames or hides the field for all of them at once. +// +// The oracle is a real marshal: the emitted object key is, by definition, the name +// encoding/json chose. +func TestJSONTagNameMatchesEncodingJSON(t *testing.T) { + tests := []struct { + tag string + // key is the object key encoding/json emits, or "" when it omits the field. + key string + }{ + {tag: `json:"name"`, key: "name"}, + {tag: `json:"name,omitempty"`, key: "name"}, + {tag: `json:"name,string"`, key: "name"}, + {tag: `json:"-"`, key: ""}, + // A lone dash means "skip"; a dash with a comma means a field literally named "-". + {tag: `json:"-,"`, key: "-"}, + {tag: `json:",omitempty"`, key: "Field"}, + {tag: `json:""`, key: "Field"}, + } + + for _, tc := range tests { + t.Run(tc.tag, func(t *testing.T) { + typ := reflect.StructOf([]reflect.StructField{{ + Name: "Field", + Type: reflect.TypeOf(""), + Tag: reflect.StructTag(tc.tag), + }}) + value := reflect.New(typ) + value.Elem().Field(0).SetString("v") + + blob, err := json.Marshal(value.Interface()) + require.NoError(t, err) + var emitted map[string]any + require.NoError(t, json.Unmarshal(blob, &emitted)) + + if tc.key == "" { + require.Empty(t, emitted, "expected the field to be skipped, got %s", blob) + } else { + require.Contains(t, emitted, tc.key, "encoding/json emitted %s", blob) + } + + // What structtag reports has to lead every package to the same conclusion: the + // emitted key, or "-" for a field encoding/json skips. + name := structtag.JSONTag(typ.Field(0).Tag.Get("json")).Name() + switch { + case tc.key == "": + assert.Equal(t, "-", name, "a skipped field must read as %q", "-") + case name == "": + // An empty tag name means "fall back to the Go field name", which is what + // encoding/json did. + assert.Equal(t, "Field", tc.key) + default: + assert.Equal(t, tc.key, name) + } + }) + } +} + +// TestOmitEmptyMatchesEncodingJSON checks the other half of the tag: whether a zero value is +// dropped. structaccess decides ForceSendFields from this, so reading it wrongly means a +// field is sent when it should be absent, or absent when it should be sent. +func TestOmitEmptyMatchesEncodingJSON(t *testing.T) { + for _, tc := range []struct { + tag string + omitEmpty bool + }{ + {tag: `json:"name"`, omitEmpty: false}, + {tag: `json:"name,omitempty"`, omitEmpty: true}, + {tag: `json:",omitempty"`, omitEmpty: true}, + {tag: `json:"name,string,omitempty"`, omitEmpty: true}, + } { + t.Run(tc.tag, func(t *testing.T) { + typ := reflect.StructOf([]reflect.StructField{{ + Name: "Field", + Type: reflect.TypeOf(""), + Tag: reflect.StructTag(tc.tag), + }}) + + blob, err := json.Marshal(reflect.New(typ).Interface()) + require.NoError(t, err) + + dropped := string(blob) == "{}" + assert.Equal(t, tc.omitEmpty, dropped, + "encoding/json emitted %s for a zero value", blob) + assert.Equal(t, tc.omitEmpty, structtag.JSONTag(typ.Field(0).Tag.Get("json")).OmitEmpty()) + }) + } +} diff --git a/libs/structs/structwalk/jsonagreement_test.go b/libs/structs/structwalk/jsonagreement_test.go new file mode 100644 index 00000000000..22fbca1075f --- /dev/null +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -0,0 +1,100 @@ +package structwalk_test + +import ( + "encoding/json" + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/cli/libs/structs/structwalk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWalkVisitsExactlyWhatJSONEmits checks the value walk against encoding/json over the +// shape corpus. A path structwalk does not visit is a path structdiff cannot report drift +// on, and a path it visits but the wire format drops is a change that can never be sent. +func TestWalkVisitsExactlyWhatJSONEmits(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + var visited []string + require.NoError(t, structwalk.Walk(shape.Value, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { + visited = append(visited, path.String()) + })) + slices.Sort(visited) + + blob, err := json.Marshal(shape.Value) + require.NoError(t, err) + var emitted map[string]any + require.NoError(t, json.Unmarshal(blob, &emitted)) + var want []string + for name := range emitted { + want = append(want, name) + } + slices.Sort(want) + + if shape.Gap("structwalk.Walk") { + assert.NotEqual(t, want, visited, + "structwalk.Walk now agrees with encoding/json here -- remove the recorded gap") + t.Logf("recorded gap: encoding/json emits %v, Walk visits %v", want, visited) + return + } + + assert.Equal(t, want, visited, "encoding/json emitted %s", blob) + + for _, name := range shape.Unreachable { + assert.NotContains(t, visited, name, + "%q never reaches the wire, so the walk must not offer it", name) + } + }) + } +} + +// isScalarKind mirrors what the value walk treats as a leaf, so the two walks are compared +// on the same footing. +func isScalarKind(k reflect.Kind) bool { + switch k { + case reflect.Bool, reflect.String, + reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return true + default: + return false + } +} + +// TestWalkTypeCoversTheType checks the type walk, which differs from the value walk only +// where a value cannot reach what its type declares -- an embedded nil pointer contributes +// its fields to the type and nothing to the wire. +func TestWalkTypeCoversTheType(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + var visited []string + require.NoError(t, structwalk.WalkType(reflect.TypeOf(shape.Value), func(path *structpath.PatternNode, typ reflect.Type, _ *reflect.StructField) bool { + if isScalarKind(typ.Kind()) { + visited = append(visited, path.String()) + } + return true + })) + slices.Sort(visited) + + want := append([]string(nil), shape.Fields()...) + slices.Sort(want) + if shape.Gap("structwalk.WalkType") { + assert.NotEqual(t, want, visited, + "structwalk.WalkType now agrees with encoding/json here -- remove the recorded gap") + t.Logf("recorded gap: the type declares %v, WalkType visits %v", want, visited) + return + } + assert.Equal(t, want, visited) + + for _, name := range shape.Unreachable { + assert.NotContains(t, visited, name, + "%q never reaches the wire, so the type walk must not offer it", name) + } + }) + } +} From 413c4a1b3e26d01cc45a9f93f96d23823b6a6f7c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:04:11 +0200 Subject: [PATCH 21/28] structstest: drop an unused export, modernise the field loop JSONLeaves had no caller once Check took the self-marshaling set from the internal helper, and KnownDivergence was never used. task fmt rewrote the reflection loop to reflect.TypeFor and Type.Fields. --- bundle/config/structstest/resources_test.go | 5 ++--- bundle/config/structstest/structstest.go | 18 +++--------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 95098bb0d61..58d5bcc33c3 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -56,11 +56,10 @@ func baseResourceFields(extra ...string) []string { // structstest.Check. Driving it off the struct by reflection means a newly added resource // is covered without touching this test. func TestResourceTypesAgreeWithJSON(t *testing.T) { - rt := reflect.TypeOf(config.Resources{}) + rt := reflect.TypeFor[config.Resources]() var checked int - for i := range rt.NumField() { - field := rt.Field(i) + for field := range rt.Fields() { if field.Type.Kind() != reflect.Map { continue } diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 6deccf010fa..b8cfae0f4d1 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -163,13 +163,9 @@ func Check(t reflect.Type) (Report, error) { return report, nil } -// JSONLeaves marshals v and returns its scalar leaves keyed by the structpath rendering -// of their location, which is the dialect every libs/structs package speaks. -func JSONLeaves(v any) (map[string]string, error) { - leaves, _, err := jsonLeaves(v) - return leaves, err -} - +// jsonLeaves marshals v and returns its scalar leaves keyed by the structpath rendering of +// their location, which is the dialect every libs/structs package speaks, plus the subset of +// those leaves whose Go type marshalled itself as a scalar. func jsonLeaves(v any) (map[string]string, map[string]bool, error) { blob, err := json.Marshal(v) if err != nil { @@ -397,14 +393,6 @@ func coveredBy(known []string, path string) bool { return false } -// KnownDivergence records a disagreement that exists today and is not this test's to fix. -// Every entry must say why it is here and what removes it. -type KnownDivergence struct { - Type string - Paths []string - Reason string -} - // Filter removes the known divergences from a report, so the test fails only on new ones. // An entry covers the path it names and everything under it, so listing a field that is // lost wholesale does not mean enumerating each of its leaves. From 91b01566ac6e6b63269f3dae1afab4c687ea7a35 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:32:13 +0200 Subject: [PATCH 22/28] structstest: make the recorded divergences ratchets, not exemptions From the adversarial review. Four ways the checks were weaker than they read: Filter dropped SelfMarshalingScalars from the report it returned, so the callers' check on that category could never fire and the category was silently ignored rather than reported. A known-divergence entry only filtered; nothing noticed when the underlying bug was fixed and the entry went stale. Filter now returns the entries that matched nothing and both tests fail on them. That immediately found two stale entries. Prefix coverage applied to every category, so an entry naming a field could absorb an unrelated Get or value failure at a path beneath it. It now applies only to the two walk categories, where a field lost wholesale really does take its leaves with it. The per-shape gaps asserted merely that *some* disagreement remained, which a different regression would satisfy. They now hold the exact current output, so any change in behaviour fails and the entry has to be revisited. Paths inside a free-form any field are a category of their own now, like self-marshaling scalars: structwalk does not traverse an interface and structaccess cannot validate a path through one, so the whole subtree is opaque and listing individual paths would only pin the filler's choice of map key. With that, the state and remote types need no recorded divergences at all. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 22 ++- bundle/config/structstest/structstest.go | 165 +++++++++++++----- bundle/direct/dresources/structs_test.go | 39 ++--- .../structs/internal/jsonshapes/jsonshapes.go | 46 +++-- libs/structs/structdiff/jsonagreement_test.go | 18 +- libs/structs/structtag/jsonagreement_test.go | 4 +- libs/structs/structwalk/jsonagreement_test.go | 20 ++- 7 files changed, 202 insertions(+), 112 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 58d5bcc33c3..880f9ba1967 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -36,16 +36,6 @@ var knownDivergences = map[string][]string{ "postgres_synced_tables": baseResourceFields(), } -// interfaceFieldPaths are free-form any fields. structwalk documents that it does not -// traverse an interface, so these never reach a visit callback and structdiff never -// reports a change to one. Intentional, but it means drift in a serialized dashboard or a -// cluster policy definition is invisible to the packages. -var interfaceFieldPaths = map[string][]string{ - "dashboards": {"serialized_dashboard"}, - "genie_spaces": {"serialized_space"}, - "cluster_policies": {"definition", "policy_family_definition_overrides"}, -} - // baseResourceFields returns the paths a resource gains from BaseResource, plus any extra // fields the resource declares alongside it. They are lost together, by one cause. func baseResourceFields(extra ...string) []string { @@ -75,8 +65,16 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { var known []string known = append(known, knownDivergences[group]...) - known = append(known, interfaceFieldPaths[group]...) - report = report.Filter(known) + report, stale := report.Filter(known) + require.Empty(t, stale, + "these recorded divergences no longer occur -- remove them from the list: %v", stale) + if len(report.InsideFreeFormField) > 0 { + // A known limitation: structwalk does not traverse an interface and structaccess + // cannot validate a path through one, so a free-form field is opaque to both. + t.Logf("%d path(s) inside a free-form any field: %v", + len(report.InsideFreeFormField), report.InsideFreeFormField) + report.InsideFreeFormField = nil + } if len(report.SelfMarshalingScalars) > 0 { // A known structwalk limitation, tracked as one item rather than one entry per // timestamp field, because every new SDK time field joins it. diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index b8cfae0f4d1..297e19fa657 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -48,6 +48,13 @@ type Report struct { // the value stored at the path. ValueMismatch []string + // InsideFreeFormField are paths that sit inside a free-form any field. structwalk does not + // traverse an interface and structaccess cannot validate a path through one, so nothing + // below such a field is visible to the packages -- a serialized dashboard or a cluster + // policy definition authored as inline YAML is opaque to them. One known limitation rather + // than a list of paths, which would depend on the filler's choice of key. + InsideFreeFormField []string + // SelfMarshalingScalars are paths whose Go type is a struct that marshals itself as a // scalar through its own MarshalJSON -- duration.Duration and the SDK's time wrapper. // structwalk looks for scalar *fields* and finds none inside them, so it never visits @@ -59,7 +66,8 @@ type Report struct { // Empty reports whether the type and the libs/structs packages agree completely. func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && - len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 + len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && + len(r.InsideFreeFormField) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -75,6 +83,7 @@ func (r Report) String() string { {"structaccess.ValidatePath rejects", r.ValidateFailed}, {"structaccess.Get and encoding/json disagree on the value at", r.ValueMismatch}, {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, + {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, } { if len(s.paths) == 0 { continue @@ -102,7 +111,7 @@ func Check(t reflect.Type) (Report, error) { FillNonZero(ptr.Elem()) v := ptr.Interface() - jsonLeaves, selfMarshaling, err := jsonLeaves(v) + jsonLeaves, marks, err := jsonLeaves(v) if err != nil { return Report{}, err } @@ -117,7 +126,11 @@ func Check(t reflect.Type) (Report, error) { var report Report for path, want := range jsonLeaves { - if selfMarshaling[path] { + if marks.freeForm[path] { + report.InsideFreeFormField = append(report.InsideFreeFormField, path) + continue + } + if marks.selfMarshaling[path] { report.SelfMarshalingScalars = append(report.SelfMarshalingScalars, path) continue } @@ -150,6 +163,12 @@ func Check(t reflect.Type) (Report, error) { } for path := range walkLeaves { if _, ok := jsonLeaves[path]; !ok { + if marks.freeFormField[path] { + // The any field itself: the walk offers it as a scalar leaf while the wire format + // carries whatever it holds, which is the same limitation seen from the other side. + report.InsideFreeFormField = append(report.InsideFreeFormField, path) + continue + } report.WalkExtra = append(report.WalkExtra, path) } } @@ -166,28 +185,48 @@ func Check(t reflect.Type) (Report, error) { // jsonLeaves marshals v and returns its scalar leaves keyed by the structpath rendering of // their location, which is the dialect every libs/structs package speaks, plus the subset of // those leaves whose Go type marshalled itself as a scalar. -func jsonLeaves(v any) (map[string]string, map[string]bool, error) { +// leafMarks records leaves that need a category of their own rather than a path-by-path +// comparison. +type leafMarks struct { + // selfMarshaling are leaves whose Go type is a struct that marshalled itself as a scalar. + selfMarshaling map[string]bool + // freeForm are leaves below an any field; freeFormField holds the any fields themselves. + freeForm map[string]bool + freeFormField map[string]bool +} + +func jsonLeaves(v any) (map[string]string, leafMarks, error) { + marks := leafMarks{ + selfMarshaling: map[string]bool{}, + freeForm: map[string]bool{}, + freeFormField: map[string]bool{}, + } + blob, err := json.Marshal(v) if err != nil { - return nil, nil, fmt.Errorf("structstest: marshal %T: %w", v, err) + return nil, marks, fmt.Errorf("structstest: marshal %T: %w", v, err) } var generic any if err := json.Unmarshal(blob, &generic); err != nil { - return nil, nil, fmt.Errorf("structstest: unmarshal %T: %w", v, err) + return nil, marks, fmt.Errorf("structstest: unmarshal %T: %w", v, err) } out := map[string]string{} - selfMarshaling := map[string]bool{} - flatten(nil, reflect.TypeOf(v), generic, out, selfMarshaling) - return out, selfMarshaling, nil + flatten(nil, reflect.TypeOf(v), generic, out, marks, false) + return out, marks, nil } // flatten walks the decoded JSON alongside the Go type, because the path syntax for an // object member depends on which one it is: a struct field is .name, a map entry is // ['name'], and only the type knows the difference. -func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string]string, selfMarshaling map[string]bool) { +func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string]string, marks leafMarks, freeForm bool) { for typ != nil && typ.Kind() == reflect.Pointer { typ = typ.Elem() } + if typ != nil && typ.Kind() == reflect.Interface { + // Everything below an any field is opaque to the packages. + marks.freeFormField[path.String()] = true + freeForm = true + } switch value := v.(type) { case map[string]any: @@ -206,7 +245,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] // its elements at the parent path, while the wire format keeps the // __embed__ key. Follow the walkers, or every such type reads as a // disagreement when it is really the convention working. - flatten(path, sf.Type, member, out, selfMarshaling) + flatten(path, sf.Type, member, out, marks, freeForm) continue } if sf, _, ok := structaccess.FindStructFieldByKeyType(typ, key); ok { @@ -214,7 +253,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] } } } - flatten(next, memberType, member, out, selfMarshaling) + flatten(next, memberType, member, out, marks, freeForm) } case []any: var elemType reflect.Type @@ -222,16 +261,19 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] elemType = typ.Elem() } for i, member := range value { - flatten(structpath.NewIndex(path, i), elemType, member, out, selfMarshaling) + flatten(structpath.NewIndex(path, i), elemType, member, out, marks, freeForm) } case nil: // A JSON null carries no scalar leaf. default: out[path.String()] = render(value) + if freeForm { + marks.freeForm[path.String()] = true + } // A scalar on the wire whose Go type is a struct marshalled itself: the walkers // cannot see inside it. if typ != nil && typ.Kind() == reflect.Struct { - selfMarshaling[path.String()] = true + marks.selfMarshaling[path.String()] = true } } } @@ -360,10 +402,10 @@ func fillNonZero(v reflect.Value, depth int) { fillNonZero(val, depth+1) v.SetMapIndex(reflect.ValueOf("k").Convert(v.Type().Key()), val) case reflect.Interface: - // A free-form any field gets a scalar, not a map: structwalk deliberately does not - // traverse into an interface, so a composite here would show up as a path mismatch - // that says nothing about the type under test. - v.Set(reflect.ValueOf("x")) + // A free-form any field holds a composite in practice -- a cluster policy definition or + // a serialized dashboard authored as inline YAML -- and that is the case worth covering, + // because structwalk does not traverse into an interface and so cannot see any of it. + v.Set(reflect.ValueOf(map[string]any{"k": "v"})) case reflect.Struct: for i := range v.Type().NumField() { sf := v.Type().Field(i) @@ -380,37 +422,76 @@ func fillNonZero(v reflect.Value, depth int) { } } -// coveredBy reports whether path equals one of the entries or sits underneath it. -func coveredBy(known []string, path string) bool { - if slices.Contains(known, path) { - return true - } - for _, k := range known { - if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { - return true +// Filter removes the known divergences from a report and returns the entries that matched +// nothing, so a caller can fail when a recorded divergence has been fixed and the entry is +// stale. Without that, a known-divergence list is an exemption rather than a ratchet. +// +// Prefix coverage applies only to the two walk categories, where a field lost wholesale takes +// all of its leaves with it. The categories that name a specific failure -- Get, ValidatePath, +// a value mismatch -- match exactly, so an entry cannot quietly absorb an unrelated failure at +// a path beneath it. +func (r Report) Filter(known []string) (Report, []string) { + used := map[string]bool{} + + dropPrefix := func(paths []string) []string { + var out []string + for _, p := range paths { + if match, ok := coveredBy(known, pathOf(p)); ok { + used[match] = true + continue + } + out = append(out, p) } + return out } - return false -} - -// Filter removes the known divergences from a report, so the test fails only on new ones. -// An entry covers the path it names and everything under it, so listing a field that is -// lost wholesale does not mean enumerating each of its leaves. -func (r Report) Filter(known []string) Report { - drop := func(paths []string) []string { + dropExact := func(paths []string) []string { var out []string for _, p := range paths { - if !coveredBy(known, strings.SplitN(p, ":", 2)[0]) { - out = append(out, p) + if slices.Contains(known, pathOf(p)) { + used[pathOf(p)] = true + continue } + out = append(out, p) } return out } - return Report{ - WalkMissing: drop(r.WalkMissing), - WalkExtra: drop(r.WalkExtra), - GetFailed: drop(r.GetFailed), - ValidateFailed: drop(r.ValidateFailed), - ValueMismatch: drop(r.ValueMismatch), + + filtered := Report{ + WalkMissing: dropPrefix(r.WalkMissing), + WalkExtra: dropPrefix(r.WalkExtra), + GetFailed: dropExact(r.GetFailed), + ValidateFailed: dropExact(r.ValidateFailed), + ValueMismatch: dropExact(r.ValueMismatch), + // These two are categories, not per-path lists, so the caller decides what to do with + // them. Dropping them here would make that decision unreachable. + SelfMarshalingScalars: r.SelfMarshalingScalars, + InsideFreeFormField: r.InsideFreeFormField, + } + + var stale []string + for _, k := range known { + if !used[k] { + stale = append(stale, k) + } + } + return filtered, stale +} + +// pathOf strips the explanatory suffix some categories append after a colon. +func pathOf(reported string) string { + return strings.SplitN(reported, ":", 2)[0] +} + +// coveredBy reports which entry covers path: the one that equals it, or names a field it sits +// underneath. +func coveredBy(known []string, path string) (string, bool) { + for _, k := range known { + if path == k { + return k, true + } + if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { + return k, true + } } + return "", false } diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go index 0bd697a80b8..e8e33380c65 100644 --- a/bundle/direct/dresources/structs_test.go +++ b/bundle/direct/dresources/structs_test.go @@ -18,31 +18,19 @@ import ( // wrapper loses fields across Marshal -> Unmarshal. This covers whether the libs/structs // packages and encoding/json name and reach the same fields in the first place. -// knownStateDivergences and knownRemoteDivergences enumerate what disagrees today, so a -// new disagreement fails the test while these are worked through. -var ( - knownStateDivergences = map[string][]string{ - // Free-form any fields: structwalk documents that it does not traverse an interface, - // so drift inside a serialized dashboard or a cluster policy definition is invisible - // to it and to structdiff. - "dashboards": {"serialized_dashboard"}, - "genie_spaces": {"serialized_space"}, - "cluster_policies": {"definition", "policy_family_definition_overrides"}, - } - - knownRemoteDivergences = map[string][]string{ - "dashboards": {"serialized_dashboard"}, - "genie_spaces": {"serialized_space"}, - "cluster_policies": {"definition", "policy_family_definition_overrides"}, - } -) +// knownDivergences is where a per-path disagreement would be recorded. It is empty: the state +// and remote types agree with encoding/json on every path today, and the two limitations that +// remain -- free-form any fields and types that marshal themselves as a scalar -- are reported +// as categories rather than paths. A new disagreement fails the test rather than landing here +// silently. +var knownDivergences = map[string][]string{} func TestStateTypeAgreesWithJSON(t *testing.T) { - testAgreesWithJSON(t, (*Adapter).StateType, knownStateDivergences) + testAgreesWithJSON(t, (*Adapter).StateType, knownDivergences) } func TestRemoteTypeAgreesWithJSON(t *testing.T) { - testAgreesWithJSON(t, (*Adapter).RemoteType, knownRemoteDivergences) + testAgreesWithJSON(t, (*Adapter).RemoteType, knownDivergences) } // testAgreesWithJSON runs the check for every registered resource, so a newly supported @@ -57,7 +45,16 @@ func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known report, err := structstest.Check(typ) require.NoError(t, err) - report = report.Filter(known[resourceType]) + report, stale := report.Filter(known[resourceType]) + require.Empty(t, stale, + "these recorded divergences no longer occur -- remove them from the list: %v", stale) + if len(report.InsideFreeFormField) > 0 { + // A known limitation: structwalk does not traverse an interface and structaccess + // cannot validate a path through one, so a free-form field is opaque to both. + t.Logf("%d path(s) inside a free-form any field: %v", + len(report.InsideFreeFormField), report.InsideFreeFormField) + report.InsideFreeFormField = nil + } if len(report.SelfMarshalingScalars) > 0 { // A known structwalk limitation, tracked as one item rather than one entry per // timestamp field, because every new SDK time field joins it. diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index ab91a31f33d..126899879a8 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -7,8 +7,6 @@ // two same-named fields wins, and when encoding/json gives up and serializes neither. package jsonshapes -import "slices" - // Shape is one struct whose JSON behaviour a libs/structs package must match. type Shape struct { // Name identifies the shape in test output. @@ -29,10 +27,13 @@ type Shape struct { // "same as JSONFields". TypeFields []string - // KnownGaps names the entry points that disagree with encoding/json about this shape - // today: "structwalk.Walk", "structwalk.WalkType", "structdiff". The package's own test asserts the disagreement is still there, so fixing - // it breaks the test and the entry has to be removed: a ratchet, not an exemption. - KnownGaps []string + // WalkGap, WalkTypeGap and DiffGap record what a package does today where it disagrees + // with encoding/json. They hold the exact current output, not merely a "this is broken" + // flag, so any change to the behaviour -- including a different wrong answer -- fails the + // package's test and forces the entry to be revisited. Nil means "must agree". + WalkGap []string + WalkTypeGap []string + DiffGap []string // KnownSetGap are json names encoding/json can unmarshal into but structaccess.Set // cannot reach today. Consumers assert that Set still fails for them, so fixing the gap @@ -88,7 +89,7 @@ type SideB struct { // and emits neither, so the field is not readable or writable either. type SameDepthConflict struct { SideA - SideB + SideB //nolint:govet // the repeated json tag is the point: both embeds declare "value" } type DiamondLeft struct { @@ -102,7 +103,7 @@ type DiamondRight struct { // Diamond reaches one Leaf by two routes of equal length: ambiguous, like SameDepthConflict. type Diamond struct { DiamondLeft - DiamondRight + DiamondRight //nolint:govet // the repeated json tag is the point: both routes reach Leaf } type NilSide struct { @@ -115,7 +116,7 @@ type NilSide struct { // declaration and wrongly concludes the field is reachable. type AmbiguousViaNilPointer struct { *NilSide - SideB + SideB //nolint:govet // the repeated json tag is the point: both embeds declare "value" } // Cyclic embeds a pointer to itself, so a type-level search that does not remember where it @@ -152,11 +153,6 @@ type SkippedField struct { unexported string //nolint:unused // present to prove it is ignored } -// Gap reports whether the named entry point is recorded as disagreeing about this shape. -func (s Shape) Gap(entryPoint string) bool { - return slices.Contains(s.KnownGaps, entryPoint) -} - // Fields returns the names a type-level walk should yield for the shape. func (s Shape) Fields() []string { if len(s.TypeFields) > 0 { @@ -179,7 +175,8 @@ func Shapes() []Shape { JSONFields: []string{"value"}, // Both walks visit each declaration, so a shadowed field is reported twice while the // wire format carries one value. - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType"}, + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, }, { Name: "same depth conflict", @@ -189,21 +186,32 @@ func Shapes() []Shape { // structaccess reports the name as not found, as encoding/json does. The walks still // visit both declarations and structdiff still reports a change at the path, so the // engine can plan an update for a field that cannot be serialized. - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, + // Once per declaration, since structdiff walks both. + DiffGap: []string{"value", "value"}, }, { Name: "diamond", Value: &Diamond{DiamondLeft: DiamondLeft{Leaf: Leaf{Value: "l"}}, DiamondRight: DiamondRight{Leaf: Leaf{Value: "r"}}}, JSONFields: nil, Unreachable: []string{"value"}, - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, + // Once per declaration, since structdiff walks both. + DiffGap: []string{"value", "value"}, }, { Name: "ambiguous via nil pointer", Value: &AmbiguousViaNilPointer{SideB: SideB{Value: "b"}}, //exhaustruct:ignore JSONFields: nil, Unreachable: []string{"value"}, - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + // The value walk reaches only the non-nil declaration; the type walk sees both. + WalkGap: []string{"value"}, + WalkTypeGap: []string{"value", "value"}, + // Once per declaration, since structdiff walks both. + // Once: the nil side is not reachable in the value, so only one declaration is walked. + DiffGap: []string{"value"}, }, { Name: "cyclic embed", @@ -212,7 +220,7 @@ func Shapes() []Shape { // The embedded *Cyclic promotes name a level down, where encoding/json shadows it // with the outer one. The type walk reports both; the value walk agrees, because the // corpus leaves the pointer nil. - KnownGaps: []string{"structwalk.WalkType"}, + WalkTypeGap: []string{"name", "name"}, }, { Name: "nil pointer embed", diff --git a/libs/structs/structdiff/jsonagreement_test.go b/libs/structs/structdiff/jsonagreement_test.go index eebdfbf79ab..ca82c72c529 100644 --- a/libs/structs/structdiff/jsonagreement_test.go +++ b/libs/structs/structdiff/jsonagreement_test.go @@ -58,10 +58,14 @@ func TestDiffNeverReportsAnUnreachableField(t *testing.T) { reported = append(reported, change.Path.String()) } } - if shape.Gap("structdiff") { - assert.NotEmpty(t, reported, - "structdiff no longer reports an unreachable field -- remove the recorded gap") - t.Logf("recorded gap: structdiff reports %v, which encoding/json does not serialize", reported) + if shape.DiffGap != nil { + // The recorded gap is the exact set structdiff reports today, so a change in + // either direction fails here. + slices.Sort(reported) + gap := slices.Clone(shape.DiffGap) + slices.Sort(gap) + assert.Equal(t, gap, reported, + "structdiff changed here -- update or remove the recorded gap") return } assert.Empty(t, reported, @@ -77,10 +81,8 @@ func cloneWith(t *testing.T, shape jsonshapes.Shape, name, value string) any { clone := freshLike(shape.Value) if err := structaccess.SetByString(clone, name, value); err != nil { - for _, gap := range shape.KnownSetGap { - if gap == name { - return nil - } + if slices.Contains(shape.KnownSetGap, name) { + return nil } t.Fatalf("cannot set %q: %v", name, err) } diff --git a/libs/structs/structtag/jsonagreement_test.go b/libs/structs/structtag/jsonagreement_test.go index 7897de63760..7631aea3e12 100644 --- a/libs/structs/structtag/jsonagreement_test.go +++ b/libs/structs/structtag/jsonagreement_test.go @@ -36,7 +36,7 @@ func TestJSONTagNameMatchesEncodingJSON(t *testing.T) { t.Run(tc.tag, func(t *testing.T) { typ := reflect.StructOf([]reflect.StructField{{ Name: "Field", - Type: reflect.TypeOf(""), + Type: reflect.TypeFor[string](), Tag: reflect.StructTag(tc.tag), }}) value := reflect.New(typ) @@ -86,7 +86,7 @@ func TestOmitEmptyMatchesEncodingJSON(t *testing.T) { t.Run(tc.tag, func(t *testing.T) { typ := reflect.StructOf([]reflect.StructField{{ Name: "Field", - Type: reflect.TypeOf(""), + Type: reflect.TypeFor[string](), Tag: reflect.StructTag(tc.tag), }}) diff --git a/libs/structs/structwalk/jsonagreement_test.go b/libs/structs/structwalk/jsonagreement_test.go index 22fbca1075f..69afd4ed657 100644 --- a/libs/structs/structwalk/jsonagreement_test.go +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -35,10 +35,13 @@ func TestWalkVisitsExactlyWhatJSONEmits(t *testing.T) { } slices.Sort(want) - if shape.Gap("structwalk.Walk") { - assert.NotEqual(t, want, visited, - "structwalk.Walk now agrees with encoding/json here -- remove the recorded gap") - t.Logf("recorded gap: encoding/json emits %v, Walk visits %v", want, visited) + if shape.WalkGap != nil { + // A recorded gap holds the exact current output, so a different wrong answer + // fails here too rather than passing as "still broken". + gap := slices.Clone(shape.WalkGap) + slices.Sort(gap) + assert.Equal(t, gap, visited, + "structwalk.Walk changed here; encoding/json emits %v -- update or remove the recorded gap", want) return } @@ -83,10 +86,11 @@ func TestWalkTypeCoversTheType(t *testing.T) { want := append([]string(nil), shape.Fields()...) slices.Sort(want) - if shape.Gap("structwalk.WalkType") { - assert.NotEqual(t, want, visited, - "structwalk.WalkType now agrees with encoding/json here -- remove the recorded gap") - t.Logf("recorded gap: the type declares %v, WalkType visits %v", want, visited) + if shape.WalkTypeGap != nil { + gap := slices.Clone(shape.WalkTypeGap) + slices.Sort(gap) + assert.Equal(t, gap, visited, + "structwalk.WalkType changed here; the type declares %v -- update or remove the recorded gap", want) return } assert.Equal(t, want, visited) From eb4445b9c33c292068eecf29c68d99db7680db84 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:18:54 +0200 Subject: [PATCH 23/28] jsonshapes: cover a tagged embed and an option-only one A json name on an anonymous field makes it a named field to encoding/json; a tag that sets only an option leaves it flattened. Both shapes are in the corpus now, and Leaves flattens a shape's JSON to leaf paths so the corpus's expectations stay comparable once a shape nests. Also: coveredBy takes the longest matching entry rather than the first, so overlapping known-divergence entries are each credited with what they alone cover instead of one absorbing everything and leaving the other looking stale. --- bundle/config/structstest/structstest.go | 16 ++-- .../structs/internal/jsonshapes/jsonshapes.go | 75 +++++++++++++++++++ .../internal/jsonshapes/jsonshapes_test.go | 4 +- .../structaccess/jsonagreement_test.go | 9 +-- libs/structs/structwalk/jsonagreement_test.go | 4 +- 5 files changed, 93 insertions(+), 15 deletions(-) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 297e19fa657..b06b552885b 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -482,16 +482,22 @@ func pathOf(reported string) string { return strings.SplitN(reported, ":", 2)[0] } -// coveredBy reports which entry covers path: the one that equals it, or names a field it sits -// underneath. +// coveredBy reports which entry covers path: the one that equals it, or the longest one that +// names a field it sits underneath. Longest wins so that overlapping entries -- "a" and "a.b" +// -- are each credited with what they alone cover, rather than the first one absorbing +// everything and leaving the other looking stale. func coveredBy(known []string, path string) (string, bool) { + best := "" for _, k := range known { if path == k { return k, true } - if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { - return k, true + if !strings.HasPrefix(path, k) || !strings.ContainsAny(path[len(k):len(k)+1], ".[") { + continue + } + if len(k) > len(best) { + best = k } } - return "", false + return best, best != "" } diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index 126899879a8..b995b727f3a 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -7,6 +7,12 @@ // two same-named fields wins, and when encoding/json gives up and serializes neither. package jsonshapes +import ( + "encoding/json" + "fmt" + "strconv" +) + // Shape is one struct whose JSON behaviour a libs/structs package must match. type Shape struct { // Name identifies the shape in test output. @@ -145,6 +151,22 @@ type SetPointerEmbed struct { Own string `json:"own,omitempty"` } +// TaggedEmbed carries a json name on an anonymous field, which makes it an ordinary field to +// encoding/json: it serializes as a nested object under that name rather than being flattened. +type TaggedEmbed struct { + Leaf `json:"leaf"` + + Own string `json:"own,omitempty"` +} + +// OptionOnlyEmbed sets an option on the embed's tag without giving it a name, which leaves it +// flattened -- the presence of a tag is not what decides. +type OptionOnlyEmbed struct { + Leaf `json:",omitempty"` + + Own string `json:"own,omitempty"` +} + // SkippedField declares a field encoding/json never serializes. type SkippedField struct { Kept string `json:"kept,omitempty"` @@ -238,6 +260,18 @@ func Shapes() []Shape { // an allocated embed behind and changes what the type marshals to. KnownSetGap: []string{"value"}, }, + { + Name: "tagged embed is a named field", + Value: &TaggedEmbed{Leaf: Leaf{Value: "v"}, Own: "o"}, + JSONFields: []string{"leaf.value", "own"}, + // Not flattened, so the outer object has no "value" member. + Unreachable: []string{"value"}, + }, + { + Name: "option-only embed stays flattened", + Value: &OptionOnlyEmbed{Leaf: Leaf{Value: "v"}, Own: "o"}, + JSONFields: []string{"value", "own"}, + }, { Name: "skipped field", Value: &SkippedField{Kept: "k", Skipped: "s"}, //exhaustruct:ignore @@ -246,3 +280,44 @@ func Shapes() []Shape { }, } } + +// Leaves marshals v and returns its scalar leaves keyed by path, so a shape's JSONFields can +// be compared against what encoding/json actually produces even when a shape nests. +// +// Object members are joined with a dot, which is the struct-field rendering. That is enough +// here because no shape in the corpus contains a map; the type-aware version, which has to +// tell a map entry's ['key'] from a field's .name, lives in bundle/config/structstest. +func Leaves(v any) (map[string]string, error) { + blob, err := json.Marshal(v) + if err != nil { + return nil, err + } + var generic any + if err := json.Unmarshal(blob, &generic); err != nil { + return nil, err + } + out := map[string]string{} + flattenLeaves("", generic, out) + return out, nil +} + +func flattenLeaves(prefix string, v any, out map[string]string) { + switch value := v.(type) { + case map[string]any: + for key, member := range value { + path := key + if prefix != "" { + path = prefix + "." + key + } + flattenLeaves(path, member, out) + } + case []any: + for i, member := range value { + flattenLeaves(prefix+"["+strconv.Itoa(i)+"]", member, out) + } + case nil: + // A JSON null carries no scalar leaf. + default: + out[prefix] = fmt.Sprintf("%v", value) + } +} diff --git a/libs/structs/internal/jsonshapes/jsonshapes_test.go b/libs/structs/internal/jsonshapes/jsonshapes_test.go index 9ebebae226f..012228e0636 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes_test.go +++ b/libs/structs/internal/jsonshapes/jsonshapes_test.go @@ -19,8 +19,8 @@ func TestCorpusMatchesEncodingJSON(t *testing.T) { blob, err := json.Marshal(shape.Value) require.NoError(t, err) - var got map[string]any - require.NoError(t, json.Unmarshal(blob, &got)) + got, err := jsonshapes.Leaves(shape.Value) + require.NoError(t, err) var names []string for name := range got { diff --git a/libs/structs/structaccess/jsonagreement_test.go b/libs/structs/structaccess/jsonagreement_test.go index 2a68de4944f..786f9bbd0d2 100644 --- a/libs/structs/structaccess/jsonagreement_test.go +++ b/libs/structs/structaccess/jsonagreement_test.go @@ -1,7 +1,6 @@ package structaccess_test import ( - "encoding/json" "reflect" "slices" "testing" @@ -44,12 +43,10 @@ func TestAgreesWithEncodingJSON(t *testing.T) { require.NoError(t, structaccess.SetByString(fresh, name, "written"), "%q is on the wire, so Set must reach it", name) - blob, err := json.Marshal(fresh) + leaves, err := jsonshapes.Leaves(fresh) require.NoError(t, err) - var got map[string]any - require.NoError(t, json.Unmarshal(blob, &got)) - assert.Equal(t, "written", got[name], - "Set wrote somewhere encoding/json does not serialize: %s", blob) + assert.Equal(t, "written", leaves[name], + "Set wrote somewhere encoding/json does not serialize: %v", leaves) } for _, name := range shape.Unreachable { diff --git a/libs/structs/structwalk/jsonagreement_test.go b/libs/structs/structwalk/jsonagreement_test.go index 69afd4ed657..cc94d3b5e28 100644 --- a/libs/structs/structwalk/jsonagreement_test.go +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -27,8 +27,8 @@ func TestWalkVisitsExactlyWhatJSONEmits(t *testing.T) { blob, err := json.Marshal(shape.Value) require.NoError(t, err) - var emitted map[string]any - require.NoError(t, json.Unmarshal(blob, &emitted)) + emitted, err := jsonshapes.Leaves(shape.Value) + require.NoError(t, err) var want []string for name := range emitted { want = append(want, name) From 53be9cb61ac1372d5102f25a38127fe02d313f6f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:21:01 +0200 Subject: [PATCH 24/28] jsonshapes: drop a redundant loop-variable copy --- libs/structs/internal/jsonshapes/jsonshapes.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index b995b727f3a..541b022a9f1 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -305,11 +305,10 @@ func flattenLeaves(prefix string, v any, out map[string]string) { switch value := v.(type) { case map[string]any: for key, member := range value { - path := key if prefix != "" { - path = prefix + "." + key + key = prefix + "." + key } - flattenLeaves(path, member, out) + flattenLeaves(key, member, out) } case []any: for i, member := range value { From b176c2201617643be20fa2eff18b5090ab9b62be Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:52:55 +0200 Subject: [PATCH 25/28] structstest: inventory containers, and ratchet the two tolerated categories Three coverage holes from the final review round. Check inventoried scalar leaves only, so a field encoding/json emits as {} or [] contributed nothing to compare and a field the packages could not reach at all would have passed unnoticed. Container paths are now collected too and each one has to resolve through ValidatePath and Get. Every resource, state and remote type already satisfies that. The free-form and self-marshaling categories were logged and cleared, so a newly introduced blind spot passed silently. Both are ratcheted now, each at the level where the ratchet says something: - Free-form any fields are listed by name per resource. Which resources have one is stable, so a new one has to be added here deliberately. That immediately caught a real difference: cluster policies have two in the config type and none in the state type, because the definition is normalized to the string the API takes before deploy. - Self-marshaling scalars are ratcheted on the Go *types* that behave this way, not the paths. A new timestamp field of duration.Duration or the SDK time wrapper says nothing; a new type that hides itself from the walkers is a finding, and fails. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 48 +++++++++--- bundle/config/structstest/structstest.go | 83 +++++++++++++++++---- bundle/direct/dresources/structs_test.go | 46 +++++++++--- 3 files changed, 144 insertions(+), 33 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 880f9ba1967..edeaee28fd5 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -2,11 +2,14 @@ package structstest_test import ( "reflect" + "slices" + "strings" "testing" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/structstest" "github.com/databricks/cli/libs/structs/structtag" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -36,6 +39,27 @@ var knownDivergences = map[string][]string{ "postgres_synced_tables": baseResourceFields(), } +// freeFormFields lists the any-typed fields of each resource. Everything at or below one is +// invisible to the packages. +var freeFormFields = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, +} + +// freeFormFieldNames reduces the reported paths to the distinct top-level field each sits under, +// so the expectation does not depend on the filler's choice of map key. +func freeFormFieldNames(paths []string) []string { + var out []string + for _, path := range paths { + name, _, _ := strings.Cut(strings.SplitN(path, ":", 2)[0], ".") + if !slices.Contains(out, name) { + out = append(out, name) + } + } + return out +} + // baseResourceFields returns the paths a resource gains from BaseResource, plus any extra // fields the resource declares alongside it. They are lost together, by one cause. func baseResourceFields(extra ...string) []string { @@ -68,19 +92,23 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { report, stale := report.Filter(known) require.Empty(t, stale, "these recorded divergences no longer occur -- remove them from the list: %v", stale) - if len(report.InsideFreeFormField) > 0 { - // A known limitation: structwalk does not traverse an interface and structaccess - // cannot validate a path through one, so a free-form field is opaque to both. - t.Logf("%d path(s) inside a free-form any field: %v", - len(report.InsideFreeFormField), report.InsideFreeFormField) - report.InsideFreeFormField = nil - } + // A known limitation: structwalk does not traverse an interface and structaccess cannot + // validate a path through one, so a free-form field is opaque to both. Which resources + // have one is stable, so it is ratcheted by name: a new free-form field is a new blind + // spot and has to be added here deliberately. + assert.ElementsMatch(t, freeFormFields[group], freeFormFieldNames(report.InsideFreeFormField), + "free-form any fields changed for %s", group) + report.InsideFreeFormField = nil if len(report.SelfMarshalingScalars) > 0 { - // A known structwalk limitation, tracked as one item rather than one entry per - // timestamp field, because every new SDK time field joins it. - t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + // A known structwalk limitation. The ratchet is on the *types* that behave this + // way, not the paths: a new field of a type already known to hide itself tells us + // nothing, while a new such type is a finding. + assert.Subset(t, structstest.KnownSelfMarshalingTypes, report.SelfMarshalingTypes, + "a Go type that marshals itself as a scalar and so is invisible to structwalk") + t.Logf("%d self-marshaling scalar field(s): %v", len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) report.SelfMarshalingScalars = nil + report.SelfMarshalingTypes = nil } require.True(t, report.Empty(), "%s (%s) disagrees with encoding/json:%s", group, elem, report) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index b06b552885b..234ff0fc8fa 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -25,6 +25,16 @@ import ( "github.com/databricks/cli/libs/structs/structwalk" ) +// KnownSelfMarshalingTypes are the Go types that serialize themselves as a scalar through +// their own MarshalJSON, which is why structwalk never visits a field of one: it looks for +// scalar fields and finds none inside. A new *type* here is a new way for a field to hide from +// the packages, so callers assert the set stays within this list rather than listing every +// field of these types. +var KnownSelfMarshalingTypes = []string{ + "duration.Duration", + "time.Time", +} + // Report lists the disagreements found for one type. A field is identified by the JSON // path encoding/json puts it at, which is the only name all the packages share. type Report struct { @@ -55,19 +65,30 @@ type Report struct { // than a list of paths, which would depend on the filler's choice of key. InsideFreeFormField []string + // ContainerUnreachable are paths at which encoding/json emits an object or an array that + // structaccess cannot resolve. Scalar leaves alone would miss them: a field serialized as + // {} or [] contributes no leaf, so a field the packages cannot reach at all would otherwise + // go unnoticed. + ContainerUnreachable []string + // SelfMarshalingScalars are paths whose Go type is a struct that marshals itself as a // scalar through its own MarshalJSON -- duration.Duration and the SDK's time wrapper. // structwalk looks for scalar *fields* and finds none inside them, so it never visits - // them and structdiff never reports drift on them. One known limitation rather than a - // per-field list, because every new timestamp field in the SDK joins it. + // them and structdiff never reports drift on them. SelfMarshalingScalars []string + + // SelfMarshalingTypes are the distinct Go types behind SelfMarshalingScalars. Callers + // ratchet on these rather than on the paths: a new timestamp field of a type already known + // to behave this way tells them nothing, while a new *type* that hides itself from the + // walkers is a finding. + SelfMarshalingTypes []string } // Empty reports whether the type and the libs/structs packages agree completely. func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && - len(r.InsideFreeFormField) == 0 + len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -84,6 +105,7 @@ func (r Report) String() string { {"structaccess.Get and encoding/json disagree on the value at", r.ValueMismatch}, {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, + {"is emitted as an object or array structaccess cannot resolve", r.ContainerUnreachable}, } { if len(s.paths) == 0 { continue @@ -130,8 +152,11 @@ func Check(t reflect.Type) (Report, error) { report.InsideFreeFormField = append(report.InsideFreeFormField, path) continue } - if marks.selfMarshaling[path] { + if typeName, ok := marks.selfMarshaling[path]; ok { report.SelfMarshalingScalars = append(report.SelfMarshalingScalars, path) + if !slices.Contains(report.SelfMarshalingTypes, typeName) { + report.SelfMarshalingTypes = append(report.SelfMarshalingTypes, typeName) + } continue } if _, ok := walkLeaves[path]; !ok { @@ -161,6 +186,27 @@ func Check(t reflect.Type) (Report, error) { fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) } } + for path := range marks.containers { + if marks.freeForm[path] || marks.freeFormField[path] { + continue + } + node, err := structpath.ParsePath(path) + if err != nil { + report.ContainerUnreachable = append(report.ContainerUnreachable, path+": "+err.Error()) + continue + } + if skippedByTag(reflect.TypeOf(v), node) { + continue + } + if err := structaccess.ValidatePath(reflect.TypeOf(v), node); err != nil { + report.ContainerUnreachable = append(report.ContainerUnreachable, path+": "+err.Error()) + continue + } + if _, err := structaccess.Get(v, node); err != nil { + report.ContainerUnreachable = append(report.ContainerUnreachable, path+": "+err.Error()) + } + } + for path := range walkLeaves { if _, ok := jsonLeaves[path]; !ok { if marks.freeFormField[path] { @@ -188,18 +234,21 @@ func Check(t reflect.Type) (Report, error) { // leafMarks records leaves that need a category of their own rather than a path-by-path // comparison. type leafMarks struct { - // selfMarshaling are leaves whose Go type is a struct that marshalled itself as a scalar. - selfMarshaling map[string]bool + // selfMarshaling maps such a leaf to the Go type that marshalled itself. + selfMarshaling map[string]string // freeForm are leaves below an any field; freeFormField holds the any fields themselves. freeForm map[string]bool freeFormField map[string]bool + // containers are paths at which an object or array is emitted. + containers map[string]bool } func jsonLeaves(v any) (map[string]string, leafMarks, error) { marks := leafMarks{ - selfMarshaling: map[string]bool{}, + selfMarshaling: map[string]string{}, freeForm: map[string]bool{}, freeFormField: map[string]bool{}, + containers: map[string]bool{}, } blob, err := json.Marshal(v) @@ -230,6 +279,9 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] switch value := v.(type) { case map[string]any: + if path != nil { + marks.containers[path.String()] = true + } isMap := typ != nil && typ.Kind() == reflect.Map for key, member := range value { var next *structpath.PathNode @@ -256,6 +308,9 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] flatten(next, memberType, member, out, marks, freeForm) } case []any: + if path != nil { + marks.containers[path.String()] = true + } var elemType reflect.Type if typ != nil && (typ.Kind() == reflect.Slice || typ.Kind() == reflect.Array) { elemType = typ.Elem() @@ -273,7 +328,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] // A scalar on the wire whose Go type is a struct marshalled itself: the walkers // cannot see inside it. if typ != nil && typ.Kind() == reflect.Struct { - marks.selfMarshaling[path.String()] = true + marks.selfMarshaling[path.String()] = typ.String() } } } @@ -457,14 +512,16 @@ func (r Report) Filter(known []string) (Report, []string) { } filtered := Report{ - WalkMissing: dropPrefix(r.WalkMissing), - WalkExtra: dropPrefix(r.WalkExtra), - GetFailed: dropExact(r.GetFailed), - ValidateFailed: dropExact(r.ValidateFailed), - ValueMismatch: dropExact(r.ValueMismatch), + WalkMissing: dropPrefix(r.WalkMissing), + WalkExtra: dropPrefix(r.WalkExtra), + GetFailed: dropExact(r.GetFailed), + ContainerUnreachable: dropExact(r.ContainerUnreachable), + ValidateFailed: dropExact(r.ValidateFailed), + ValueMismatch: dropExact(r.ValueMismatch), // These two are categories, not per-path lists, so the caller decides what to do with // them. Dropping them here would make that decision unreachable. SelfMarshalingScalars: r.SelfMarshalingScalars, + SelfMarshalingTypes: r.SelfMarshalingTypes, InsideFreeFormField: r.InsideFreeFormField, } diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go index e8e33380c65..650828ede2a 100644 --- a/bundle/direct/dresources/structs_test.go +++ b/bundle/direct/dresources/structs_test.go @@ -2,9 +2,12 @@ package dresources import ( "reflect" + "slices" + "strings" "testing" "github.com/databricks/cli/bundle/config/structstest" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -25,6 +28,27 @@ import ( // silently. var knownDivergences = map[string][]string{} +// freeFormFields lists the any-typed fields of each resource's state and remote types. Unlike +// the config types, cluster policies have none: the state type carries definition as the string +// the API takes, which ConfigureClusterPolicyDefinition has already normalized. +var freeFormFields = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, +} + +// topLevelNames reduces reported paths to the distinct top-level field each sits under, so the +// expectation does not depend on the filler's choice of map key. +func topLevelNames(paths []string) []string { + var out []string + for _, path := range paths { + name, _, _ := strings.Cut(strings.SplitN(path, ":", 2)[0], ".") + if !slices.Contains(out, name) { + out = append(out, name) + } + } + return out +} + func TestStateTypeAgreesWithJSON(t *testing.T) { testAgreesWithJSON(t, (*Adapter).StateType, knownDivergences) } @@ -48,19 +72,21 @@ func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known report, stale := report.Filter(known[resourceType]) require.Empty(t, stale, "these recorded divergences no longer occur -- remove them from the list: %v", stale) - if len(report.InsideFreeFormField) > 0 { - // A known limitation: structwalk does not traverse an interface and structaccess - // cannot validate a path through one, so a free-form field is opaque to both. - t.Logf("%d path(s) inside a free-form any field: %v", - len(report.InsideFreeFormField), report.InsideFreeFormField) - report.InsideFreeFormField = nil - } + // Free-form any fields are opaque to the packages; which resources have one is stable, + // so it is ratcheted by name rather than logged away. + assert.ElementsMatch(t, freeFormFields[resourceType], topLevelNames(report.InsideFreeFormField), + "free-form any fields changed for %s", resourceType) + report.InsideFreeFormField = nil if len(report.SelfMarshalingScalars) > 0 { - // A known structwalk limitation, tracked as one item rather than one entry per - // timestamp field, because every new SDK time field joins it. - t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + // A known structwalk limitation. The ratchet is on the *types* that behave this + // way, not the paths: a new field of a type already known to hide itself tells us + // nothing, while a new such type is a finding. + assert.Subset(t, structstest.KnownSelfMarshalingTypes, report.SelfMarshalingTypes, + "a Go type that marshals itself as a scalar and so is invisible to structwalk") + t.Logf("%d self-marshaling scalar field(s): %v", len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) report.SelfMarshalingScalars = nil + report.SelfMarshalingTypes = nil } require.True(t, report.Empty(), "%s (%s) disagrees with encoding/json:%s", resourceType, typ, report) From 38c5cdb850f7006b5b836f80b5ce851810aab546 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:09:15 +0200 Subject: [PATCH 26/28] structstest: count structwalk's visits instead of collapsing them Check recorded the walk's leaves in a map, so two visits to one path became one entry and a shadowed embedded field looked like agreement. That is the worst failure mode for a test whose whole job is to notice disagreement, and it was hiding real cases. Visits are counted now, and six resource types turn out to have one: apps (id, url, lifecycle.prevent_destroy), pipelines and alerts (id), and job_runs, clusters and sql_warehouses (lifecycle.prevent_destroy). Each embeds BaseResource alongside an SDK type that declares the same json name, or two structs that each carry a Lifecycle. encoding/json serializes the shallower field and nothing else, so the second visit is a field that cannot reach the wire under that name -- and structdiff reports a change at that path twice. Recorded per resource rather than fixed: making structwalk resolve a name the way encoding/json does is a change to the walk itself, and it would move the refschema golden. The state and remote types have none of these, so the duplication is confined to the config types. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 19 ++++++++++++++++++ bundle/config/structstest/structstest.go | 22 ++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index edeaee28fd5..8cd9a179cc3 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -39,6 +39,21 @@ var knownDivergences = map[string][]string{ "postgres_synced_tables": baseResourceFields(), } +// walkDuplicates lists the paths structwalk visits twice for a resource, because the resource +// embeds BaseResource alongside an SDK type that declares the same json name, or two structs +// that each carry a Lifecycle. encoding/json serializes the shallower one and nothing else, so +// the second visit is a field that cannot reach the wire under that name -- and structdiff +// reports a change at the path twice. Ratcheted by name: fixing structwalk to resolve a name +// the way encoding/json does empties these, and a new shadowed field has to be added here. +var walkDuplicates = map[string][]string{ + "job_runs": {"lifecycle.prevent_destroy"}, + "pipelines": {"id"}, + "clusters": {"lifecycle.prevent_destroy"}, + "apps": {"id", "url", "lifecycle.prevent_destroy"}, + "alerts": {"id"}, + "sql_warehouses": {"lifecycle.prevent_destroy"}, +} + // freeFormFields lists the any-typed fields of each resource. Everything at or below one is // invisible to the packages. var freeFormFields = map[string][]string{ @@ -96,6 +111,10 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { // validate a path through one, so a free-form field is opaque to both. Which resources // have one is stable, so it is ratcheted by name: a new free-form field is a new blind // spot and has to be added here deliberately. + assert.ElementsMatch(t, walkDuplicates[group], report.WalkDuplicated, + "paths structwalk visits twice changed for %s", group) + report.WalkDuplicated = nil + assert.ElementsMatch(t, freeFormFields[group], freeFormFieldNames(report.InsideFreeFormField), "free-form any fields changed for %s", group) report.InsideFreeFormField = nil diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 234ff0fc8fa..3bc80fed292 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -65,6 +65,13 @@ type Report struct { // than a list of paths, which would depend on the filler's choice of key. InsideFreeFormField []string + // WalkDuplicated are paths structwalk visits more than once. encoding/json serializes a + // name once, so a second visit is a second field under one name -- a shadowed embed -- and + // structdiff would report a change at that path twice. Carried through Filter untouched: + // callers ratchet on it by name, like the other categories that name a limitation rather + // than a path-by-path failure. + WalkDuplicated []string + // ContainerUnreachable are paths at which encoding/json emits an object or an array that // structaccess cannot resolve. Scalar leaves alone would miss them: a field serialized as // {} or [] contributes no leaf, so a field the packages cannot reach at all would otherwise @@ -88,7 +95,8 @@ type Report struct { func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && - len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 + len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 && + len(r.WalkDuplicated) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -106,6 +114,7 @@ func (r Report) String() string { {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, {"is emitted as an object or array structaccess cannot resolve", r.ContainerUnreachable}, + {"structwalk visits more than once", r.WalkDuplicated}, } { if len(s.paths) == 0 { continue @@ -139,7 +148,11 @@ func Check(t reflect.Type) (Report, error) { } walkLeaves := map[string]string{} + walkVisits := map[string]int{} err = structwalk.Walk(v, func(path *structpath.PathNode, val any, _ *reflect.StructField) { + // Counted, not just recorded: a map would collapse two visits to one path and hide a + // shadowed embedded field, which is agreement reported where there is none. + walkVisits[path.String()]++ walkLeaves[path.String()] = render(val) }) if err != nil { @@ -186,6 +199,12 @@ func Check(t reflect.Type) (Report, error) { fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) } } + for path, visits := range walkVisits { + if visits > 1 && !marks.freeForm[path] && !marks.freeFormField[path] { + report.WalkDuplicated = append(report.WalkDuplicated, path) + } + } + for path := range marks.containers { if marks.freeForm[path] || marks.freeFormField[path] { continue @@ -516,6 +535,7 @@ func (r Report) Filter(known []string) (Report, []string) { WalkExtra: dropPrefix(r.WalkExtra), GetFailed: dropExact(r.GetFailed), ContainerUnreachable: dropExact(r.ContainerUnreachable), + WalkDuplicated: r.WalkDuplicated, ValidateFailed: dropExact(r.ValidateFailed), ValueMismatch: dropExact(r.ValueMismatch), // These two are categories, not per-path lists, so the caller decides what to do with From 069c0af62cd8f32c378201440a21adf947033bf2 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:30:35 +0200 Subject: [PATCH 27/28] structstest: stop treating a dash-named field as skipped FillNonZero skipped any field whose parsed json name was "-", which includes json:"-,omitempty" -- a field encoding/json does serialize, under the name "-". Left at its zero value it was then omitted from the marshal output, so the harness saw no disagreement and passed: a false pass on exactly the shape structaccess could not resolve. It uses structaccess.IsSkippedField now, and the corpus carries a dash-named field alongside the genuinely skipped one so all four packages are held to the distinction. --- bundle/config/structstest/structstest.go | 2 +- libs/structs/internal/jsonshapes/jsonshapes.go | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 3bc80fed292..5a564ad852e 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -486,7 +486,7 @@ func fillNonZero(v reflect.Value, depth int) { if !sf.IsExported() || sf.Name == "ForceSendFields" { continue } - if structtag.JSONTag(sf.Tag.Get("json")).Name() == "-" { + if structaccess.IsSkippedField(sf) { continue } fillNonZero(v.Field(i), depth+1) diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index 541b022a9f1..6c5f07d95c8 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -167,10 +167,12 @@ type OptionOnlyEmbed struct { Own string `json:"own,omitempty"` } -// SkippedField declares a field encoding/json never serializes. +// SkippedField declares a field encoding/json never serializes, alongside one whose tag names +// it "-": only the exact tag json:"-" is a skip. type SkippedField struct { - Kept string `json:"kept,omitempty"` - Skipped string `json:"-"` + Kept string `json:"kept,omitempty"` + Skipped string `json:"-"` + DashNamed string `json:"-,omitempty"` //nolint:staticcheck // the odd tag is the point unexported string //nolint:unused // present to prove it is ignored } @@ -273,10 +275,9 @@ func Shapes() []Shape { JSONFields: []string{"value", "own"}, }, { - Name: "skipped field", - Value: &SkippedField{Kept: "k", Skipped: "s"}, //exhaustruct:ignore - JSONFields: []string{"kept"}, - Unreachable: []string{"-"}, + Name: "skipped and dash-named fields", + Value: &SkippedField{Kept: "k", Skipped: "s", DashNamed: "d"}, //exhaustruct:ignore + JSONFields: []string{"kept", "-"}, }, } } From 5625399a7d8e64ab3931cdd0a15b3807dada2e62 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:36:33 +0200 Subject: [PATCH 28/28] structstest: compare wire types, and report the __embed__ rename Two more false-pass paths in the harness itself. render normalised every scalar to its text, so a JSON string and a JSON number of the same digits compared equal. A field tagged json:",string" puts a number on the wire as "1" while the packages expose the int behind it -- a real difference in what is at the path, reported as agreement. The JSON side is decoded with UseNumber now and both sides render with their type, so number:1 and string:1 no longer match while 1, 1.0 and 1e0 still do. flatten silently rewrote the EmbeddedSlice convention: __embed__ carries the slice on the wire while the walkers put its elements at the parent path. Following the walkers is right -- it is how the state file and the engine's paths relate -- but doing it silently meant the harness could not notice a change to the convention. The rename is reported now, and both tests assert that __embed__ is the only key it ever applies to. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 8 ++ bundle/config/structstest/structstest.go | 91 ++++++++++++++++----- bundle/direct/dresources/structs_test.go | 8 ++ 3 files changed, 88 insertions(+), 19 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 8cd9a179cc3..9a49c297712 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -111,6 +111,14 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { // validate a path through one, so a free-form field is opaque to both. Which resources // have one is stable, so it is ratcheted by name: a new free-form field is a new blind // spot and has to be added here deliberately. + // The EmbeddedSlice convention renames exactly one key: __embed__ carries the slice + // while the walkers put its elements at the parent path. Anything else renamed would + // be a change to the convention. + for _, path := range report.RenamedByConvention { + assert.Equal(t, "__embed__", path, "only __embed__ is renamed by convention") + } + report.RenamedByConvention = nil + assert.ElementsMatch(t, walkDuplicates[group], report.WalkDuplicated, "paths structwalk visits twice changed for %s", group) report.WalkDuplicated = nil diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 5a564ad852e..62634e13b91 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -12,6 +12,7 @@ package structstest import ( + "bytes" "encoding/json" "fmt" "reflect" @@ -72,6 +73,12 @@ type Report struct { // than a path-by-path failure. WalkDuplicated []string + // RenamedByConvention are paths where the packages deliberately use a different name than + // the wire: an EmbeddedSlice field is tagged __embed__ but the walkers put its elements at + // the parent path. Reported rather than silently rewritten, so the convention is visible and + // a change to it is noticed. + RenamedByConvention []string + // ContainerUnreachable are paths at which encoding/json emits an object or an array that // structaccess cannot resolve. Scalar leaves alone would miss them: a field serialized as // {} or [] contributes no leaf, so a field the packages cannot reach at all would otherwise @@ -96,7 +103,7 @@ func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 && - len(r.WalkDuplicated) == 0 + len(r.WalkDuplicated) == 0 && len(r.RenamedByConvention) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -115,6 +122,7 @@ func (r Report) String() string { {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, {"is emitted as an object or array structaccess cannot resolve", r.ContainerUnreachable}, {"structwalk visits more than once", r.WalkDuplicated}, + {"is at a different path on the wire, by the EmbeddedSlice convention", r.RenamedByConvention}, } { if len(s.paths) == 0 { continue @@ -153,7 +161,7 @@ func Check(t reflect.Type) (Report, error) { // Counted, not just recorded: a map would collapse two visits to one path and hide a // shadowed embedded field, which is agreement reported where there is none. walkVisits[path.String()]++ - walkLeaves[path.String()] = render(val) + walkLeaves[path.String()] = renderGo(val) }) if err != nil { return Report{}, fmt.Errorf("structstest: walk %s: %w", t, err) @@ -194,11 +202,15 @@ func Check(t reflect.Type) (Report, error) { report.GetFailed = append(report.GetFailed, path+": "+err.Error()) continue } - if render(got) != want { + if renderGo(got) != want { report.ValueMismatch = append(report.ValueMismatch, - fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) + fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, renderGo(got), want)) } } + for path := range marks.renamed { + report.RenamedByConvention = append(report.RenamedByConvention, path) + } + for path, visits := range walkVisits { if visits > 1 && !marks.freeForm[path] && !marks.freeFormField[path] { report.WalkDuplicated = append(report.WalkDuplicated, path) @@ -260,6 +272,8 @@ type leafMarks struct { freeFormField map[string]bool // containers are paths at which an object or array is emitted. containers map[string]bool + // renamed are wire paths the packages present under a different name by convention. + renamed map[string]bool } func jsonLeaves(v any) (map[string]string, leafMarks, error) { @@ -268,14 +282,20 @@ func jsonLeaves(v any) (map[string]string, leafMarks, error) { freeForm: map[string]bool{}, freeFormField: map[string]bool{}, containers: map[string]bool{}, + renamed: map[string]bool{}, } blob, err := json.Marshal(v) if err != nil { return nil, marks, fmt.Errorf("structstest: marshal %T: %w", v, err) } + // UseNumber keeps a JSON number distinct from a JSON string, so a field tagged + // json:",string" -- which puts a number on the wire as "1" -- is not mistaken for agreement + // with the Go int behind it. + decoder := json.NewDecoder(bytes.NewReader(blob)) + decoder.UseNumber() var generic any - if err := json.Unmarshal(blob, &generic); err != nil { + if err := decoder.Decode(&generic); err != nil { return nil, marks, fmt.Errorf("structstest: unmarshal %T: %w", v, err) } out := map[string]string{} @@ -312,10 +332,11 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] next = structpath.NewStringKey(path, key) if typ != nil && typ.Kind() == reflect.Struct { if sf, ok := embeddedSliceField(typ, key); ok { - // An EmbeddedSlice field is transparent by design: the walkers put - // its elements at the parent path, while the wire format keeps the - // __embed__ key. Follow the walkers, or every such type reads as a - // disagreement when it is really the convention working. + // An EmbeddedSlice field is transparent by design: the walkers put its + // elements at the parent path, while the wire format keeps the __embed__ + // key. Follow the walkers so the rest of the type can be compared, and + // record the rename so it is visible rather than assumed. + marks.renamed[next.String()] = true flatten(path, sf.Type, member, out, marks, freeForm) continue } @@ -340,7 +361,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] case nil: // A JSON null carries no scalar leaf. default: - out[path.String()] = render(value) + out[path.String()] = renderWire(value) if freeForm { marks.freeForm[path.String()] = true } @@ -365,15 +386,32 @@ func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool return reflect.StructField{}, false } -// render normalises a scalar so a value decoded from JSON and the same value read out of -// the struct compare equal: JSON numbers decode to float64, the struct holds int64 and -// friends, and a nil pointer reads back as nil. -func render(v any) string { +// renderWire describes a scalar as it appears on the wire, keeping the JSON type: a number and +// the string of the same digits are different values, which is the difference json:",string" +// makes and the reason the two renderings are not one function. +func renderWire(v any) string { + switch value := v.(type) { + case nil: + return "" + case json.Number: + return "number:" + normalizeNumber(value.String()) + case string: + return "string:" + value + case bool: + return fmt.Sprintf("bool:%v", value) + default: + return fmt.Sprintf("other:%v", value) + } +} + +// renderGo describes a Go value in the same vocabulary, so the two can be compared. A pointer +// is followed, since the wire carries what it points at. +func renderGo(v any) string { if v == nil { return "" } rv := reflect.ValueOf(v) - for rv.Kind() == reflect.Pointer { + for rv.Kind() == reflect.Pointer || rv.Kind() == reflect.Interface { if rv.IsNil() { return "" } @@ -381,14 +419,28 @@ func render(v any) string { } switch rv.Kind() { case reflect.Float32, reflect.Float64: - return strconv.FormatFloat(rv.Float(), 'g', -1, 64) + return "number:" + normalizeNumber(strconv.FormatFloat(rv.Float(), 'f', -1, 64)) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return strconv.FormatFloat(float64(rv.Int()), 'g', -1, 64) + return "number:" + normalizeNumber(strconv.FormatInt(rv.Int(), 10)) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return strconv.FormatFloat(float64(rv.Uint()), 'g', -1, 64) + return "number:" + normalizeNumber(strconv.FormatUint(rv.Uint(), 10)) + case reflect.String: + return "string:" + rv.String() + case reflect.Bool: + return fmt.Sprintf("bool:%v", rv.Bool()) default: - return fmt.Sprintf("%v", rv.Interface()) + return fmt.Sprintf("other:%v", rv.Interface()) + } +} + +// normalizeNumber renders a number the same way whichever side it came from, so 1 and 1.0 and +// 1e0 compare equal without letting a string masquerade as a number. +func normalizeNumber(text string) string { + f, err := strconv.ParseFloat(text, 64) + if err != nil { + return text } + return strconv.FormatFloat(f, 'g', -1, 64) } // skippedByTag reports whether the last segment of path names a field structaccess @@ -536,6 +588,7 @@ func (r Report) Filter(known []string) (Report, []string) { GetFailed: dropExact(r.GetFailed), ContainerUnreachable: dropExact(r.ContainerUnreachable), WalkDuplicated: r.WalkDuplicated, + RenamedByConvention: r.RenamedByConvention, ValidateFailed: dropExact(r.ValidateFailed), ValueMismatch: dropExact(r.ValueMismatch), // These two are categories, not per-path lists, so the caller decides what to do with diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go index 650828ede2a..c054c0d2a0f 100644 --- a/bundle/direct/dresources/structs_test.go +++ b/bundle/direct/dresources/structs_test.go @@ -72,6 +72,14 @@ func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known report, stale := report.Filter(known[resourceType]) require.Empty(t, stale, "these recorded divergences no longer occur -- remove them from the list: %v", stale) + // The EmbeddedSlice convention renames exactly one key: __embed__ carries the slice + // while the walkers put its elements at the parent path. Anything else renamed would + // be a change to the convention. + for _, path := range report.RenamedByConvention { + assert.Equal(t, "__embed__", path, "only __embed__ is renamed by convention") + } + report.RenamedByConvention = nil + // Free-form any fields are opaque to the packages; which resources have one is stable, // so it is ratcheted by name rather than logged away. assert.ElementsMatch(t, freeFormFields[resourceType], topLevelNames(report.InsideFreeFormField),