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..0f3d3cc1f4a 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")) @@ -234,124 +228,64 @@ 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 (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() - - // First pass: direct fields - if fv, sf, found := findFieldInStruct(v, key); found { - return fv, sf, -1, true +// 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) { + 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) - for i := range t.NumField() { - sf := t.Field(i) - if !sf.Anonymous { - continue - } - fv := v.Field(i) - // Dereference pointer anonymous structs - for fv.Kind() == reflect.Pointer { - if fv.IsNil() { - // Not initialized; can't descend - break + 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 } - fv = fv.Elem() - } - if fv.Kind() != reflect.Struct { - continue + cur = cur.Elem() } - if out, osf, found := findFieldInStruct(fv, key); found { - return out, osf, i, true + if cur.Kind() != reflect.Struct { + return reflect.Value{}, reflect.StructField{}, reflect.Value{}, false } + owner = cur + cur = cur.Field(i) } - - return reflect.Value{}, reflect.StructField{}, -1, false + return cur, sf, owner, true } -// 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..3d9603a3831 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" @@ -793,26 +794,432 @@ 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 +} + +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)) +} + +// 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 //nolint:govet // the repeated json tag is the point: both embeds declare "value" +} + +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)) +} + +// 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")) +} + +// 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)) +} + +// 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)) +} + +// 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.Error(t, structaccess.SetByString(job, "max_concurrent_runs", "")) - assert.Empty(t, job.ForceSendFields) - assert.Equal(t, "n", job.Name) + 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)) } -// 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 +// 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 +} - require.NoError(t, structaccess.SetByString(job, "max_concurrent_runs", "0")) - assert.Equal(t, 0, job.MaxConcurrentRuns) - assert.Contains(t, job.ForceSendFields, "MaxConcurrentRuns") +type taggedAsX struct { + Y string `json:"X"` +} + +type taggedBeatsUntagged struct { + untaggedX + taggedAsX +} - blob, err := json.Marshal(job) +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.Contains(t, string(blob), `"max_concurrent_runs":0`) + 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) +} + +// 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) +} + +// 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) + }) + } } diff --git a/libs/structs/structaccess/typecheck.go b/libs/structs/structaccess/typecheck.go index 7147fa0f435..924b17d5451 100644 --- a/libs/structs/structaccess/typecheck.go +++ b/libs/structs/structaccess/typecheck.go @@ -142,32 +142,111 @@ 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 - for sf := range t.Fields() { - if sf.PkgPath != "" { // unexported - continue +// 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 + } + + 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 + // round forever. + seen := map[reflect.Type]bool{t: true} + level := dedupeByType(embeddedIndexPaths(t, nil)) + for len(level) > 0 { + var next []embeddedPath + var found []candidate + for _, embed := range level { + 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) { + if seen[deeper.typ] { + continue + } + next = append(next, deeper) + } } - name := structtag.JSONTag(sf.Tag.Get("json")).Name() - if name == "-" || sf.Name == EmbeddedSliceFieldName { - name = "" + level = dedupeByType(next) + for _, embed := range level { + seen[embed.typ] = true } - if name != "" && name == key { - // Skip fields marked as internal/readonly - btag := structtag.BundleTag(sf.Tag.Get("bundle")) - if btag.Internal() || btag.ReadOnly() { - continue + if len(found) > 0 { + if c, ok := pickCandidate(found); ok { + return c.index, c.field, true } - return sf, t, true + return nil, reflect.StructField{}, false } } - // Second pass: search embedded anonymous structs recursively (flattening semantics) - for sf := range t.Fields() { - if !sf.Anonymous { + return nil, reflect.StructField{}, false +} + +// 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 + 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 +// 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 @@ -177,16 +256,116 @@ func FindStructFieldByKeyType(t reflect.Type, key string) (reflect.StructField, 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 + 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 +} + +// 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 == "-" { + continue } + tagged := name != "" + if !tagged { + name = sf.Name + } + if name != key { + continue + } + // 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 + } + out = append(out, candidate{ + index: append(append([]int{}, prefix...), i), + field: sf, + tagged: tagged, + }) } + return out +} - return reflect.StructField{}, reflect.TypeOf(nil), false +// 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) + } + } + if len(tagged) == 1 { + return tagged[0], true + } + return candidate{}, 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. 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 + } + 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 } 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 } 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)