diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 1f54cef5fac..e0b2133502c 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -15,6 +15,7 @@ * direct: Fix spurious update when `apply_policy_default_values: true` is set on job task, for-each-task, or job cluster new_cluster ([#5731](https://github.com/databricks/cli/pull/5731)). Also fix spurious updates for for-each-task clusters due to missing backend defaults for `data_security_mode`, `node_type_id`, `driver_node_type_id`, `driver_instance_pool_id`, `enable_elastic_disk`, and `enable_local_disk_encryption`. * direct: Cluster resize now falls back to regular update if resize fails due to `INVALID_STATE` ([#5716](https://github.com/databricks/cli/pull/5716)). * Fixed `bundle deployment migrate` failing on `model_serving_endpoints`/`database_instances` with permissions (regression since v1.5.0) ([#5775](https://github.com/databricks/cli/pull/5775)). + * direct: Fix deploy bug when a `postgres_projects`, `postgres_branches`, or `postgres_endpoints` field is set to its zero value (e.g. `enable_pg_native_login: false`, `replace_existing: false`). ### Dependency updates diff --git a/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl b/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl index d78bdf6812c..aaa4b0c1c83 100644 --- a/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl +++ b/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl @@ -6,3 +6,4 @@ resources: foo: project_id: test-pg-project-$UNIQUE_NAME display_name: Test Postgres Project + enable_pg_native_login: false diff --git a/acceptance/bundle/resources/postgres_projects/basic/databricks.yml.tmpl b/acceptance/bundle/resources/postgres_projects/basic/databricks.yml.tmpl index 140b9116c0e..c399622a738 100644 --- a/acceptance/bundle/resources/postgres_projects/basic/databricks.yml.tmpl +++ b/acceptance/bundle/resources/postgres_projects/basic/databricks.yml.tmpl @@ -9,6 +9,7 @@ resources: my_project: project_id: test-pg-proj-$UNIQUE_NAME display_name: "Test Postgres Project" + enable_pg_native_login: false pg_version: 16 history_retention_duration: "604800s" default_endpoint_settings: diff --git a/acceptance/bundle/resources/postgres_projects/basic/out.requests.json b/acceptance/bundle/resources/postgres_projects/basic/out.requests.json index 938ae44f313..3ab697ae02a 100644 --- a/acceptance/bundle/resources/postgres_projects/basic/out.requests.json +++ b/acceptance/bundle/resources/postgres_projects/basic/out.requests.json @@ -12,6 +12,7 @@ "suspend_timeout_duration": "300s" }, "display_name": "Test Postgres Project", + "enable_pg_native_login": false, "history_retention_duration": "604800s", "pg_version": 16 } diff --git a/bundle/config/resources_types_test.go b/bundle/config/resources_types_test.go index 5d2a7298c6f..4dd0b18d26a 100644 --- a/bundle/config/resources_types_test.go +++ b/bundle/config/resources_types_test.go @@ -1,12 +1,18 @@ package config import ( + "encoding/json" "reflect" + "slices" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/cli/libs/structs/structtag" ) func TestResourcesTypesMap(t *testing.T) { @@ -20,3 +26,85 @@ func TestResourcesTypesMap(t *testing.T) { assert.True(t, ok, "resources type for 'jobs.permissions' not found in ResourcesTypes map") assert.Equal(t, reflect.TypeFor[[]resources.JobPermission](), typ, "resources type for 'jobs.permissions' mismatch") } + +// TestResourceTypesZeroValueFieldsSerialize guards against the ForceSendFields +// routing bug fixed in libs/dyn/convert: a field declared in a struct embedded +// more than one level deep (e.g. PostgresProject -> PostgresProjectConfig -> +// ProjectSpec) had its zero value recorded in the wrong struct's ForceSendFields, +// which the SDK marshaler rejects with "field X cannot be found in struct Y". +// The direct engine hits this path when it serializes planned state to JSON. +// +// For every registered resource type it sets every omitempty scalar field (at any +// depth) to its zero value, converts via ToTyped, and marshals - the same round +// trip the direct engine performs. Any newly added resource whose wrapper embeds +// an SDK spec is covered automatically. +func TestResourceTypesZeroValueFieldsSerialize(t *testing.T) { + names := make([]string, 0, len(ResourcesTypes)) + for name := range ResourcesTypes { + names = append(names, name) + } + slices.Sort(names) + + for _, name := range names { + t.Run(name, func(t *testing.T) { + typ := ResourcesTypes[name] + zeros := zeroValueScalars(typ, 0, map[reflect.Type]bool{}) + if zeros.Kind() != dyn.KindMap { + return + } + + ptr := reflect.New(typ) + require.NoError(t, convert.ToTyped(ptr.Interface(), zeros)) + + _, err := json.Marshal(ptr.Interface()) + require.NoError(t, err) + }) + } +} + +// zeroValueScalars builds a [dyn.Value] map that sets every omitempty scalar field +// reachable through embedded anonymous structs to its zero value. Those are exactly +// the fields the convert layer records in ForceSendFields, so they exercise the +// routing logic. depth and seen bound recursion against deep or recursive types. +func zeroValueScalars(t reflect.Type, depth int, seen map[reflect.Type]bool) dyn.Value { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct || depth > 6 || seen[t] { + return dyn.NilValue + } + seen[t] = true + defer delete(seen, t) + + m := dyn.NewMapping() + for f := range t.Fields() { + if f.Anonymous { + if sub := zeroValueScalars(f.Type, depth+1, seen); sub.Kind() == dyn.KindMap { + for _, p := range sub.MustMap().Pairs() { + m.SetLoc(p.Key.MustString(), nil, p.Value) + } + } + continue + } + + tag := structtag.JSONTag(f.Tag.Get("json")) + name := tag.Name() + if name == "" || name == "-" || !f.IsExported() || !tag.OmitEmpty() { + continue + } + + switch f.Type.Kind() { + case reflect.Bool: + m.SetLoc(name, nil, dyn.V(false)) + case reflect.String: + m.SetLoc(name, nil, dyn.V("")) + case reflect.Int, reflect.Int32, reflect.Int64: + m.SetLoc(name, nil, dyn.V(int64(0))) + case reflect.Float32, reflect.Float64: + m.SetLoc(name, nil, dyn.V(float64(0))) + default: + // Only basic types are eligible for ForceSendFields; skip the rest. + } + } + return dyn.V(m) +} diff --git a/libs/dyn/convert/struct_info.go b/libs/dyn/convert/struct_info.go index 7e5b0bc741e..69e8f868ed5 100644 --- a/libs/dyn/convert/struct_info.go +++ b/libs/dyn/convert/struct_info.go @@ -28,9 +28,15 @@ type structInfo struct { // Maps JSON-name of the field to Golang struct name GolangNames map[string]string - // ForceSendFieldsStructKey maps the JSON-name of the field to which ForceSendFields slice it belongs to: - // -1 for direct fields, embedded struct index for embedded fields - ForceSendFieldsStructKey map[string]int + // ForceSendFieldsIndex maps the JSON-name of the field to the index path (for + // use with [reflect.Value.FieldByIndex]) of the ForceSendFields slice that + // governs it: the one declared by the struct that also declares the field. + // The path is static per type, so we resolve it once here rather than walking + // the value at conversion time. It can be more than one element deep because a + // field's declaring struct may be embedded several levels down (e.g. + // PostgresProject -> PostgresProjectConfig -> ProjectSpec). A field whose + // declaring struct has no ForceSendFields has no entry. + ForceSendFieldsIndex map[string][]int } // structInfoCache caches type information. @@ -58,10 +64,10 @@ func getStructInfo(typ reflect.Type) structInfo { // buildStructInfo populates a new [structInfo] for the given type. func buildStructInfo(typ reflect.Type) structInfo { out := structInfo{ - Fields: make(map[string][]int), - ForceEmpty: make(map[string]bool), - GolangNames: make(map[string]string), - ForceSendFieldsStructKey: make(map[string]int), + Fields: make(map[string][]int), + ForceEmpty: make(map[string]bool), + GolangNames: make(map[string]string), + ForceSendFieldsIndex: make(map[string][]int), } // Queue holds the indexes of the structs to visit. @@ -81,6 +87,15 @@ func buildStructInfo(typ reflect.Type) structInfo { styp = styp.Elem() } + // Index path to the ForceSendFields declared by this struct, if any. All + // fields declared directly by this struct are governed by it. The len==1 + // check excludes a ForceSendFields promoted from an embedded struct: that + // one governs the embedded struct's own fields, which we visit separately. + var forceSendFieldsIndex []int + if sf, ok := styp.FieldByName("ForceSendFields"); ok && len(sf.Index) == 1 { + forceSendFieldsIndex = append(slices.Clone(prefix), sf.Index...) + } + nf := styp.NumField() for j := range nf { sf := styp.Field(j) @@ -119,13 +134,10 @@ func buildStructInfo(typ reflect.Type) structInfo { } out.GolangNames[name] = sf.Name - // Determine which ForceSendFields this field belongs to - if len(prefix) == 0 { - // Direct field on the main struct - out.ForceSendFieldsStructKey[name] = -1 - } else { - // Field on embedded struct - out.ForceSendFieldsStructKey[name] = prefix[0] + // The field is declared directly in this struct, so it is governed by + // this struct's ForceSendFields (if it has one). + if forceSendFieldsIndex != nil { + out.ForceSendFieldsIndex[name] = forceSendFieldsIndex } } } @@ -142,40 +154,15 @@ type FieldValue struct { func (s *structInfo) FieldValues(v reflect.Value) []FieldValue { out := make([]FieldValue, 0, len(s.Fields)) - // Collect ForceSendFields from all levels for field inclusion logic - forceSendFieldsMap := getForceSendFieldsValues(v) - for _, k := range s.FieldNames { - index := s.Fields[k] - fv := v - - // Locate value in struct (it could be an embedded type). - for i, x := range index { - if i > 0 { - if fv.Kind() == reflect.Pointer && fv.Type().Elem().Kind() == reflect.Struct { - if fv.IsNil() { - fv = reflect.Value{} - break - } - fv = fv.Elem() - } - } - fv = fv.Field(x) - } + fv := fieldByIndex(v, s.Fields[k]) if fv.IsValid() { isForced := true // TODO: we should use isEmptyForOmitEmpty instead of IsZero() if fv.IsZero() { - goName := s.GolangNames[k] - structKey := s.ForceSendFieldsStructKey[k] - if fieldValue, exists := forceSendFieldsMap[structKey]; exists { - forceSendFields := fieldValue.Interface().([]string) - isForced = slices.Contains(forceSendFields, goName) - } else { - isForced = false - } + isForced = s.isForceSend(v, k) } out = append(out, FieldValue{ @@ -189,45 +176,36 @@ func (s *structInfo) FieldValues(v reflect.Value) []FieldValue { return out } -// Type of [dyn.Value]. -var configValueType = reflect.TypeFor[dyn.Value]() - -// getForceSendFieldsValues collects ForceSendFields reflect.Values -// Returns map[structKey]reflect.Value where structKey is -1 for direct fields, embedded index for embedded fields -func getForceSendFieldsValues(v reflect.Value) map[int]reflect.Value { - if !v.IsValid() || v.Type().Kind() != reflect.Struct { - return make(map[int]reflect.Value) +// isForceSend reports whether the field named k is listed in the ForceSendFields +// that governs it (see structInfo.ForceSendFieldsIndex). +func (s *structInfo) isForceSend(v reflect.Value, k string) bool { + index, ok := s.ForceSendFieldsIndex[k] + if !ok { + return false } - - result := make(map[int]reflect.Value) - - for i := range v.Type().NumField() { - field := v.Type().Field(i) - fieldValue := v.Field(i) - - if field.Name == "ForceSendFields" && !field.Anonymous { - // Direct ForceSendFields (structKey = -1) - result[-1] = fieldValue - } else if field.Anonymous { - // Embedded struct - check for ForceSendFields inside it - if embeddedStruct := deref(fieldValue); embeddedStruct.IsValid() { - if forceSendField := embeddedStruct.FieldByName("ForceSendFields"); forceSendField.IsValid() { - result[i] = forceSendField - } - } - } + fsf := fieldByIndex(v, index) + if !fsf.IsValid() { + return false } - - return result + return slices.Contains(fsf.Interface().([]string), s.GolangNames[k]) } -// deref dereferences a pointer, returning invalid value if nil -func deref(v reflect.Value) reflect.Value { - if v.Kind() == reflect.Pointer { - if v.IsNil() { - return reflect.Value{} +// fieldByIndex resolves the value at the given index path, dereferencing embedded +// pointer structs on the way. It returns an invalid value if a nil pointer is met. +func fieldByIndex(v reflect.Value, index []int) reflect.Value { + for i, x := range index { + if i > 0 { + if v.Kind() == reflect.Pointer && v.Type().Elem().Kind() == reflect.Struct { + if v.IsNil() { + return reflect.Value{} + } + v = v.Elem() + } } - return v.Elem() + v = v.Field(x) } return v } + +// Type of [dyn.Value]. +var configValueType = reflect.TypeFor[dyn.Value]() diff --git a/libs/dyn/convert/to_typed.go b/libs/dyn/convert/to_typed.go index 6fb63d1f0ba..73ac79bca47 100644 --- a/libs/dyn/convert/to_typed.go +++ b/libs/dyn/convert/to_typed.go @@ -75,9 +75,6 @@ func toTypedStruct(dst reflect.Value, src dyn.Value) error { info := getStructInfo(dst.Type()) - forceSendFieldLocations := getForceSendFieldsValues(dst) - forceSendFieldsMap := make(map[int][]string) - for _, pair := range src.MustMap().Pairs() { pk := pair.Key pv := pair.Value @@ -90,20 +87,7 @@ func toTypedStruct(dst reflect.Value, src dyn.Value) error { continue } - // Create intermediate structs embedded as pointer types. - // Code inspired by [reflect.FieldByIndex] implementation. - f := dst - for i, x := range index { - if i > 0 { - if f.Kind() == reflect.Pointer { - if f.IsNil() { - f.Set(reflect.New(f.Type().Elem())) - } - f = f.Elem() - } - } - f = f.Field(x) - } + f := fieldByIndexAlloc(dst, index) err := ToTyped(f.Addr().Interface(), pv) if err != nil { @@ -111,22 +95,14 @@ func toTypedStruct(dst reflect.Value, src dyn.Value) error { } if pv.IsZero() { - // Use first index as key: -1 for direct fields, struct index for embedded fields - var structKey int - if len(index) == 1 { - structKey = -1 // Direct field - } else { - structKey = index[0] // Embedded struct index + // The field's zero value must still serialize, so append its Go name + // to the ForceSendFields of the struct that declares it. That struct + // shares a prefix with the field's index path, so it is already + // allocated by the walk above. + if fsfIndex, ok := info.ForceSendFieldsIndex[jsonKey]; ok { + fsf := fieldByIndexAlloc(dst, fsfIndex) + fsf.Set(reflect.Append(fsf, reflect.ValueOf(info.GolangNames[jsonKey]))) } - - forceSendFieldsMap[structKey] = append(forceSendFieldsMap[structKey], info.GolangNames[jsonKey]) - } - } - - // Set ForceSendFields using precalculated locations - for structKey, fields := range forceSendFieldsMap { - if forceSendFieldLocation, exists := forceSendFieldLocations[structKey]; exists { - forceSendFieldLocation.Set(reflect.ValueOf(fields)) } } @@ -156,6 +132,24 @@ func toTypedStruct(dst reflect.Value, src dyn.Value) error { } } +// fieldByIndexAlloc resolves the value at the given index path within an addressable +// struct, allocating intermediate structs embedded as pointer types along the way. +// Code inspired by [reflect.FieldByIndex] implementation. +func fieldByIndexAlloc(v reflect.Value, index []int) reflect.Value { + for i, x := range index { + if i > 0 { + if v.Kind() == reflect.Pointer { + if v.IsNil() { + v.Set(reflect.New(v.Type().Elem())) + } + v = v.Elem() + } + } + v = v.Field(x) + } + return v +} + func toTypedMap(dst reflect.Value, src dyn.Value) error { switch src.Kind() { case dyn.KindMap: diff --git a/libs/dyn/convert/to_typed_test.go b/libs/dyn/convert/to_typed_test.go index 7b95d17056d..fd8213c16b1 100644 --- a/libs/dyn/convert/to_typed_test.go +++ b/libs/dyn/convert/to_typed_test.go @@ -754,3 +754,37 @@ func TestToTypedFieldByNameBugRegressionTest(t *testing.T) { assert.Equal(t, "test-job", out.Name) assert.Empty(t, out.Permissions) } + +func TestToTypedDeeplyEmbeddedStructForceSendFields(t *testing.T) { + // Mirrors resources.PostgresProject -> PostgresProjectConfig -> ProjectSpec: + // Spec is embedded two levels down and Wrapper shadows ForceSendFields to keep + // its own direct field out of Spec's ForceSendFields. A zero-value spec field + // must route to Spec.ForceSendFields, not Wrapper's, otherwise the SDK marshaler + // fails with "field ... cannot be found in struct". + type Spec struct { + SpecField bool `json:"spec_field,omitempty"` + ForceSendFields []string `json:"-"` + } + + type Wrapper struct { + Spec + WrapperField string `json:"wrapper_field,omitempty"` + ForceSendFields []string `json:"-"` + } + + type Outer struct { + Wrapper + } + + var out Outer + m := dyn.Mapping{} + m.SetLoc("spec_field", nil, dyn.V(false)) + m.SetLoc("wrapper_field", nil, dyn.V("")) + v := dyn.V(m) + + err := ToTyped(&out, v) + require.NoError(t, err) + + assert.Equal(t, []string{"SpecField"}, out.Spec.ForceSendFields) + assert.Equal(t, []string{"WrapperField"}, out.ForceSendFields) +}