diff --git a/libs/structs/structaccess/bundle_test.go b/libs/structs/structaccess/bundle_test.go index 1d75afe0b10..895ef9820db 100644 --- a/libs/structs/structaccess/bundle_test.go +++ b/libs/structs/structaccess/bundle_test.go @@ -76,3 +76,38 @@ 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.ForceSendFields, "BudgetPolicyId") + + value, err = GetByString(project, "budget_policy_id") + require.NoError(t, err) + // 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)) + 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..9a10f45aa56 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,28 +264,50 @@ 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) { - t := v.Type() - +// 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) { // 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) + // 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 +} + +// 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() @@ -299,59 +315,35 @@ 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 - } + out = append(out, fv) } - - return reflect.Value{}, reflect.StructField{}, -1, false + return out } -// 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..02752e456ea 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,9 +155,9 @@ 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) + return updateForceSendFields(owner, sf.Name, converted, sf) } // setMapValue sets a value in a map @@ -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 diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 9144c61c3cd..e736daf877c 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" @@ -816,3 +817,66 @@ func TestSet_StringZeroIntoOmitemptyNumberIsForced(t *testing.T) { require.NoError(t, err) assert.Contains(t, string(blob), `"max_concurrent_runs":0`) } + +// 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 +} + +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{} + + 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) + + // The same field json.Marshal picks, which is the contract being matched. + blob, err := json.Marshal(target) + require.NoError(t, err) + assert.JSONEq(t, `{"value":"set"}`, string(blob)) +} 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 }