From b61ecc7ae5e2221c037883b27efa4dc0aa4e50c2 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 21 Sep 2026 16:59:46 +0200 Subject: [PATCH 01/18] dresources: comment why RemapState is skipped when remote type matches state type Co-authored-by: Isaac --- bundle/direct/dresources/adapter.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 74c19c08445..226f6728d5c 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -531,6 +531,9 @@ func (a *Adapter) PrepareState(input any) (any, error) { } func (a *Adapter) RemapState(remoteState any) (any, error) { + // RemapState is optional: validate() only allows it to be absent when + // remoteType == stateType, so the remote is already the state type and + // needs no adaptation. if a.remapState == nil { return remoteState, nil } From 69d3c13cefa4d75c078f3797eac810921129a0ef Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 11:21:28 +0200 Subject: [PATCH 02/18] direct: auto-generate RemapState copiers from resource types Resources whose state type is a plain subset of the remote type had a hand-written RemapState that mechanically copied same-named fields and filtered ForceSendFields. Replace those with a copier compiled once from the (remote, state) type pair at package init and wired into the adapter when a resource has no custom RemapState. buildCopiers is driven off SupportedResources, so every auto-copied resource is validated at load: anything the copier cannot copy losslessly (kind-changing conversions, mismatched struct shapes) fails there rather than as silent drift at deploy time. Deletes 12 redundant RemapState methods; resources with genuine logic keep theirs. Co-authored-by: Isaac --- bundle/direct/dresources/adapter.go | 36 ++-- bundle/direct/dresources/all.go | 64 +++++++ bundle/direct/dresources/catalog.go | 16 -- bundle/direct/dresources/cluster_policy.go | 13 -- bundle/direct/dresources/copier.go | 173 +++++++++++++++++ bundle/direct/dresources/copier_test.go | 179 ++++++++++++++++++ bundle/direct/dresources/experiment.go | 10 - bundle/direct/dresources/external_location.go | 19 -- bundle/direct/dresources/instance_pool.go | 22 --- bundle/direct/dresources/model.go | 9 - .../dresources/postgres_snapshot_schedule.go | 8 - bundle/direct/dresources/registered_model.go | 24 --- bundle/direct/dresources/schema.go | 12 -- bundle/direct/dresources/snapshot.go | 10 - .../dresources/vector_search_endpoint.go | 12 -- bundle/direct/dresources/volume.go | 12 -- libs/utils/utils.go | 7 +- 17 files changed, 444 insertions(+), 182 deletions(-) create mode 100644 bundle/direct/dresources/copier.go create mode 100644 bundle/direct/dresources/copier_test.go diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 226f6728d5c..40406bcf8ca 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -130,6 +130,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: + copier *copier doDelete *calladapt.BoundCaller prepareInputConfig *calladapt.BoundCaller isEmptyState *calladapt.BoundCaller @@ -169,6 +170,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC adapter := &Adapter{ prepareState: nil, remapState: nil, + copier: nil, doRefresh: nil, doDelete: nil, doCreate: nil, @@ -233,11 +235,15 @@ func (a *Adapter) initMethods(resource any) error { return err } - // RemapState is optional when remote type already matches state type. + // RemapState is optional when remote type already matches state type, or when an + // auto-generated copier exists for this resource (see buildCopiers). a.remapState, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "RemapState") if err != nil { return err } + if a.remapState == nil { + a.copier = copiers[reflect.TypeOf(resource)] + } a.doRefresh, err = prepareCallRequired(resource, "DoRead") if err != nil { @@ -363,15 +369,16 @@ func (a *Adapter) validate() error { validations = append(validations, "DoDelete state", a.doDelete.InTypes[2], stateType) } - // If RemapState is implemented, validate its signature. - // Otherwise require remote type to equal state type so remapping isn't needed. + // If RemapState is implemented, validate its signature. Otherwise the remote type + // must equal the state type (no remapping needed) or an auto-generated copier must + // exist for this resource (built and validated in buildCopiers). if a.remapState != nil { validations = append( validations, "RemapState input", a.remapState.InTypes[0], remoteType, "RemapState return", a.remapState.OutTypes[0], stateType, ) - } else if remoteType != stateType { + } else if remoteType != stateType && a.copier == nil { return fmt.Errorf("RemapState method not found and remote type %v must match state type %v", remoteType, stateType) } @@ -531,18 +538,19 @@ func (a *Adapter) PrepareState(input any) (any, error) { } func (a *Adapter) RemapState(remoteState any) (any, error) { - // RemapState is optional: validate() only allows it to be absent when - // remoteType == stateType, so the remote is already the state type and - // needs no adaptation. - if a.remapState == nil { - return remoteState, nil + if a.remapState != nil { + outs, err := a.remapState.Call(remoteState) + if err != nil { + return nil, err + } + return outs[0], nil } - - outs, err := a.remapState.Call(remoteState) - if err != nil { - return nil, err + if a.copier != nil { + return a.copier.copy(remoteState), nil } - return outs[0], nil + // No custom method and no copier: validate() only allows this when + // remoteType == stateType, so the remote is already the state type. + return remoteState, nil } func (a *Adapter) DoRead(ctx context.Context, id string) (any, error) { diff --git a/bundle/direct/dresources/all.go b/bundle/direct/dresources/all.go index 27e8496b83e..799e38bee72 100644 --- a/bundle/direct/dresources/all.go +++ b/bundle/direct/dresources/all.go @@ -2,7 +2,11 @@ package dresources import ( "fmt" + "reflect" + "slices" + "strings" + "github.com/databricks/cli/libs/calladapt" "github.com/databricks/databricks-sdk-go" ) @@ -81,6 +85,66 @@ var SupportedResources = map[string]any{ "internal_immutable_snapshots": (*ResourceSnapshot)(nil), } +// copiers holds an auto-generated RemapState copier for every resource whose +// remote type differs from its state type and that does not supply a custom RemapState. +// It is built once, at package initialization, from SupportedResources — so a resource +// whose types cannot be safely copied fails at load, not at deploy time, and no separate +// test can forget to cover it. Resources that need real remapping logic keep a custom +// RemapState method and are skipped here. +var copiers = buildCopiers() + +func buildCopiers() map[reflect.Type]*copier { + iface := reflect.TypeFor[IResource]() + out := make(map[reflect.Type]*copier) + var errs []string + + for resourceType, resource := range SupportedResources { + implType := reflect.TypeOf(resource) + if _, done := out[implType]; done { + continue // same implementation registered under several keys (permissions, grants) + } + + remap, err := calladapt.PrepareCall(resource, iface, "RemapState") + if err != nil { + errs = append(errs, fmt.Sprintf("%s: RemapState: %v", resourceType, err)) + continue + } + if remap != nil { + continue // custom override + } + + prepareState, err := calladapt.PrepareCall(resource, iface, "PrepareState") + if err != nil || prepareState == nil { + errs = append(errs, fmt.Sprintf("%s: PrepareState: %v", resourceType, err)) + continue + } + doRead, err := calladapt.PrepareCall(resource, iface, "DoRead") + if err != nil || doRead == nil { + errs = append(errs, fmt.Sprintf("%s: DoRead: %v", resourceType, err)) + continue + } + + stateType := prepareState.OutTypes[0] + remoteType := doRead.OutTypes[0] + if remoteType == stateType { + continue // identity: the adapter returns the remote unchanged, no copier needed + } + + copier, err := compileCopier(remoteType, stateType) + if err != nil { + errs = append(errs, fmt.Sprintf("%s: %v", resourceType, err)) + continue + } + out[implType] = copier + } + + if len(errs) > 0 { + slices.Sort(errs) + panic("dresources: cannot build RemapState copiers:\n" + strings.Join(errs, "\n")) + } + return out +} + func InitAll(client *databricks.WorkspaceClient) (map[string]*Adapter, error) { result := make(map[string]*Adapter) for resourceType, resource := range SupportedResources { diff --git a/bundle/direct/dresources/catalog.go b/bundle/direct/dresources/catalog.go index 2bc8c4155be..26cf3780ea1 100644 --- a/bundle/direct/dresources/catalog.go +++ b/bundle/direct/dresources/catalog.go @@ -22,22 +22,6 @@ func (*ResourceCatalog) PrepareState(input *resources.Catalog) *catalog.CreateCa return &input.CreateCatalog } -func (*ResourceCatalog) RemapState(info *catalog.CatalogInfo) *catalog.CreateCatalog { - return &catalog.CreateCatalog{ - Comment: info.Comment, - ConnectionName: info.ConnectionName, - CustomMaxRetentionHours: info.CustomMaxRetentionHours, - ManagedEncryptionSettings: info.ManagedEncryptionSettings, - Name: info.Name, - Options: info.Options, - Properties: info.Properties, - ProviderName: info.ProviderName, - ShareName: info.ShareName, - StorageRoot: info.StorageRoot, - ForceSendFields: utils.FilterFields[catalog.CreateCatalog](info.ForceSendFields), - } -} - func (r *ResourceCatalog) DoRead(ctx context.Context, id string) (*catalog.CatalogInfo, error) { return r.client.Catalogs.GetByName(ctx, id) } diff --git a/bundle/direct/dresources/cluster_policy.go b/bundle/direct/dresources/cluster_policy.go index ae69390b9cc..d63a6f06dea 100644 --- a/bundle/direct/dresources/cluster_policy.go +++ b/bundle/direct/dresources/cluster_policy.go @@ -30,19 +30,6 @@ func (*ResourceClusterPolicy) PrepareState(input *resources.ClusterPolicy) *comp } // RemapState copies the config fields shared by Policy and CreatePolicy; -// output-only fields (policy_id, created_at_timestamp, creator_user_name, is_default) are not in the state. -func (*ResourceClusterPolicy) RemapState(remote *compute.Policy) *compute.CreatePolicy { - return &compute.CreatePolicy{ - Definition: remote.Definition, - Description: remote.Description, - Libraries: remote.Libraries, - MaxClustersPerUser: remote.MaxClustersPerUser, - Name: remote.Name, - PolicyFamilyDefinitionOverrides: remote.PolicyFamilyDefinitionOverrides, - PolicyFamilyId: remote.PolicyFamilyId, - ForceSendFields: utils.FilterFields[compute.CreatePolicy](remote.ForceSendFields), - } -} func (r *ResourceClusterPolicy) DoRead(ctx context.Context, id string) (*compute.Policy, error) { return r.client.ClusterPolicies.GetByPolicyId(ctx, id) diff --git a/bundle/direct/dresources/copier.go b/bundle/direct/dresources/copier.go new file mode 100644 index 00000000000..6efe4eaed9c --- /dev/null +++ b/bundle/direct/dresources/copier.go @@ -0,0 +1,173 @@ +package dresources + +import ( + "fmt" + "reflect" + + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structtag" + "github.com/databricks/cli/libs/utils" +) + +// copier is a reflection-built RemapState: it copies fields from a remote +// struct into a fresh state struct by matching JSON field names. It replaces the +// hand-written "dumb copy" RemapState methods (see README.md) for resources whose +// state type is a plain subset of the remote type. +// +// The plan is compiled once from the two types (see compileCopier); executing it +// on any pair of values of those types cannot fail. Anything the plan cannot copy +// safely is rejected at compile time, so a resource that needs real logic surfaces +// as a build/init error rather than a silent drift bug — see buildCopiers. +type copier struct { + stateElem reflect.Type // struct type behind *StateType + ops []copyOp +} + +type copyOp struct { + // forceSendFields ops recompute a ForceSendFields slice; field-copy ops move one field. + forceSendFields bool + + dstIndex []int // FieldByIndex path in the state struct + + // field-copy ops: + srcIndex []int // FieldByIndex path in the remote struct + convert bool // use reflect.Convert (distinct but same-underlying types) instead of a direct set + dstType reflect.Type // target type for Convert + + // forceSendFields ops: + ownerType reflect.Type // struct type owning the ForceSendFields slice, for validity filtering +} + +// jsonFieldInfo locates a top-level JSON field within a struct, resolved through +// encoding/json-flattened embeds. +type jsonFieldInfo struct { + index []int + typ reflect.Type +} + +// forceSendTarget locates a ForceSendFields slice within a struct tree and the +// struct type that owns it. +type forceSendTarget struct { + index []int + ownerType reflect.Type +} + +// compileCopier builds the copy plan for remoteType -> stateType (both pointers to +// structs). It returns an error if any state field that also exists on the remote cannot +// be copied without a value-changing conversion; such a field needs a custom RemapState. +func compileCopier(remoteType, stateType reflect.Type) (*copier, error) { + stateElem := stateType.Elem() + remoteElem := remoteType.Elem() + + dstFields, dstForceSend := flattenStruct(stateElem, nil) + srcFields, _ := flattenStruct(remoteElem, nil) + + var ops []copyOp + for name, dst := range dstFields { + src, ok := srcFields[name] + if !ok { + // Field absent from the remote type: always nil/zero in the remapped + // state. This is the missing_in_remote invariant the planner relies on. + continue + } + switch { + case dst.typ == src.typ: + ops = append(ops, copyOp{dstIndex: dst.index, srcIndex: src.index, dstType: dst.typ}) + case safeConvert(dst.typ, src.typ): + ops = append(ops, copyOp{dstIndex: dst.index, srcIndex: src.index, convert: true, dstType: dst.typ}) + default: + return nil, fmt.Errorf("field %q: state type %s cannot be copied from remote type %s; implement RemapState", name, dst.typ, src.typ) + } + } + + for _, target := range dstForceSend { + ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType}) + } + + return &copier{stateElem: stateElem, ops: ops}, nil +} + +// safeConvert reports whether a value of src can be converted to dst without changing +// the value. Distinct named types with the same underlying type (e.g. two string enums) +// qualify; kind-changing conversions (int<->string, float->int, []byte<->string) do not, +// because reflect.Convert would silently corrupt the value. +func safeConvert(dst, src reflect.Type) bool { + return src.ConvertibleTo(dst) && src.Kind() == dst.Kind() +} + +// rootForceSendFields returns the ForceSendFields slice of the remote struct's root, or nil. +func rootForceSendFields(remote reflect.Value) []string { + f := remote.FieldByName("ForceSendFields") + if !f.IsValid() || f.Kind() != reflect.Slice { + return nil + } + fields, _ := f.Interface().([]string) + return fields +} + +// copy builds a fresh *StateType populated from remote (a *RemoteType). +func (c *copier) copy(remote any) any { + src := reflect.ValueOf(remote).Elem() + dstPtr := reflect.New(c.stateElem) + dst := dstPtr.Elem() + + var srcForceSend []string + for _, op := range c.ops { + if op.forceSendFields { + if srcForceSend == nil { + srcForceSend = rootForceSendFields(src) + } + filtered := utils.FilterFieldsType(op.ownerType, srcForceSend) + dst.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) + continue + } + val := src.FieldByIndex(op.srcIndex) + if op.convert { + val = val.Convert(op.dstType) + } + dst.FieldByIndex(op.dstIndex).Set(val) + } + + return dstPtr.Interface() +} + +// flattenStruct enumerates a struct's top-level JSON fields (descending through +// encoding/json-flattened embeds and accumulating the field-index prefix) plus every +// ForceSendFields slice reachable through those embeds. Names follow the same JSON tag +// resolution the diff engine uses, so copied state compares consistently. +func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []forceSendTarget) { + fields := make(map[string]jsonFieldInfo) + var forceSend []forceSendTarget + + for i := range t.NumField() { + sf := t.Field(i) + if sf.PkgPath != "" { + continue // unexported + } + index := append(append([]int{}, prefix...), i) + + if sf.Name == "ForceSendFields" { + forceSend = append(forceSend, forceSendTarget{index: index, ownerType: t}) + continue + } + if structaccess.IsFlattenedEmbed(sf) { + nested, nestedForceSend := flattenStruct(sf.Type, index) + for name, info := range nested { + fields[name] = info + } + forceSend = append(forceSend, nestedForceSend...) + continue + } + if structaccess.IsSkippedField(sf) || sf.Name == structaccess.EmbeddedSliceFieldName { + continue + } + + name := structtag.JSONTag(sf.Tag.Get("json")).Name() + if name == "" { + name = sf.Name + } + fields[name] = jsonFieldInfo{index: index, typ: sf.Type} + } + + return fields, forceSend +} diff --git a/bundle/direct/dresources/copier_test.go b/bundle/direct/dresources/copier_test.go new file mode 100644 index 00000000000..c4120939e1a --- /dev/null +++ b/bundle/direct/dresources/copier_test.go @@ -0,0 +1,179 @@ +package dresources + +import ( + "reflect" + "sort" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestNoRedundantRemapState enforces the "only override when needed" rule: a resource that +// keeps a hand-written RemapState must actually do something the auto-generated copier cannot. +// For every resource that still has a RemapState whose remote and state types differ, it fully +// populates a remote value and fails if the copier would produce an identical result — meaning +// the method is dead boilerplate that should be deleted (see buildCopiers). +func TestNoRedundantRemapState(t *testing.T) { + seen := map[reflect.Type]bool{} + var redundant []string + + for resourceType, resource := range SupportedResources { + implType := reflect.TypeOf(resource) + if seen[implType] { + continue + } + seen[implType] = true + + m := reflect.ValueOf(resource).MethodByName("RemapState") + if !m.IsValid() { + continue + } + remoteType := m.Type().In(0) + stateType := m.Type().Out(0) + if remoteType == stateType { + continue // identity, the copier is not involved + } + + copier, err := compileCopier(remoteType, stateType) + if err != nil { + continue // copier cannot handle it, so the override is required + } + + remote := reflect.New(remoteType.Elem()) + fillValue(remote.Elem(), 0, map[reflect.Type]bool{}) + + want := m.Call([]reflect.Value{remote})[0].Interface() + got := copier.copy(remote.Interface()) + if reflect.DeepEqual(want, got) { + redundant = append(redundant, resourceType) + } + } + + sort.Strings(redundant) + assert.Empty(t, redundant, "these resources have a RemapState the auto-copier reproduces exactly; delete the method and let buildCopiers handle it") +} + +type copierKindA string + +type copierKindB string + +type copierRemote struct { + Name string `json:"name"` + Extra string `json:"extra"` // absent from state -> dropped + Kind copierKindA `json:"kind"` // same underlying type as state, distinct name -> converted + ForceSendFields []string +} + +type copierState struct { + Name string `json:"name"` + Missing string `json:"missing"` // absent from remote -> left zero + Kind copierKindB `json:"kind"` + ForceSendFields []string +} + +func TestRemapCopierCopy(t *testing.T) { + copier, err := compileCopier(reflect.TypeFor[*copierRemote](), reflect.TypeFor[*copierState]()) + require.NoError(t, err) + + remote := &copierRemote{ + Name: "n", + Extra: "e", + Kind: "classic", + ForceSendFields: []string{"Name", "Extra", "Kind"}, + } + got := copier.copy(remote).(*copierState) + + assert.Equal(t, "n", got.Name) + assert.Empty(t, got.Missing) // absent from remote + assert.Equal(t, copierKindB("classic"), got.Kind) // converted across enum types + assert.Equal(t, []string{"Name", "Kind"}, got.ForceSendFields) // "Extra" filtered out (not a state field) +} + +type copierUnsafeRemote struct { + N int `json:"n"` +} + +type copierUnsafeState struct { + N string `json:"n"` +} + +func TestRemapCopierCompileRejectsUnsafe(t *testing.T) { + _, err := compileCopier(reflect.TypeFor[*copierUnsafeRemote](), reflect.TypeFor[*copierUnsafeState]()) + require.Error(t, err) + assert.Contains(t, err.Error(), "implement RemapState") +} + +func TestSafeConvert(t *testing.T) { + tests := []struct { + name string + dst reflect.Type + src reflect.Type + want bool + }{ + {"identical", reflect.TypeFor[string](), reflect.TypeFor[string](), true}, + {"same underlying string enums", reflect.TypeFor[copierKindA](), reflect.TypeFor[copierKindB](), true}, + {"int to string", reflect.TypeFor[string](), reflect.TypeFor[int](), false}, + {"int64 to int32", reflect.TypeFor[int32](), reflect.TypeFor[int64](), false}, + {"bytes to string", reflect.TypeFor[string](), reflect.TypeFor[[]byte](), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, safeConvert(tt.dst, tt.src)) + }) + } +} + +// fillValue sets every exported field to a non-zero value, bounded by depth to avoid cycles. +// Each struct's ForceSendFields is populated with its own field names so the FSF-filtering +// path is exercised at every level. +func fillValue(v reflect.Value, depth int, visited map[reflect.Type]bool) { + if depth > 5 { + return + } + switch v.Kind() { + case reflect.String: + v.SetString("x") + 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.Pointer: + v.Set(reflect.New(v.Type().Elem())) + fillValue(v.Elem(), depth+1, visited) + case reflect.Slice: + s := reflect.MakeSlice(v.Type(), 1, 1) + fillValue(s.Index(0), depth+1, visited) + v.Set(s) + case reflect.Map: + m := reflect.MakeMap(v.Type()) + key := reflect.New(v.Type().Key()).Elem() + fillValue(key, depth+1, visited) + val := reflect.New(v.Type().Elem()).Elem() + fillValue(val, depth+1, visited) + m.SetMapIndex(key, val) + v.Set(m) + case reflect.Struct: + if visited[v.Type()] { + return + } + visited[v.Type()] = true + defer delete(visited, v.Type()) + var names []string + for i := range v.NumField() { + sf := v.Type().Field(i) + if sf.PkgPath != "" || sf.Name == "ForceSendFields" { + continue + } + names = append(names, sf.Name) + fillValue(v.Field(i), depth+1, visited) + } + if f := v.FieldByName("ForceSendFields"); f.IsValid() && f.Kind() == reflect.Slice && f.Type().Elem().Kind() == reflect.String { + f.Set(reflect.ValueOf(names)) + } + } +} diff --git a/bundle/direct/dresources/experiment.go b/bundle/direct/dresources/experiment.go index e4f2e8ebbd7..32b423da435 100644 --- a/bundle/direct/dresources/experiment.go +++ b/bundle/direct/dresources/experiment.go @@ -32,16 +32,6 @@ func (*ResourceExperiment) PrepareState(input *resources.MlflowExperiment) *ml.C } } -func (*ResourceExperiment) RemapState(experiment *ml.Experiment) *ml.CreateExperiment { - return &ml.CreateExperiment{ - Name: experiment.Name, - ArtifactLocation: experiment.ArtifactLocation, - Tags: experiment.Tags, - TraceLocation: experiment.TraceLocation, - ForceSendFields: utils.FilterFields[ml.CreateExperiment](experiment.ForceSendFields), - } -} - func (r *ResourceExperiment) DoRead(ctx context.Context, id string) (*ml.Experiment, error) { result, err := r.client.Experiments.GetExperiment(ctx, ml.GetExperimentRequest{ ExperimentId: id, diff --git a/bundle/direct/dresources/external_location.go b/bundle/direct/dresources/external_location.go index 26c957020ab..39b6c0dde0c 100644 --- a/bundle/direct/dresources/external_location.go +++ b/bundle/direct/dresources/external_location.go @@ -22,25 +22,6 @@ func (*ResourceExternalLocation) PrepareState(input *resources.ExternalLocation) return &input.CreateExternalLocation } -func (*ResourceExternalLocation) RemapState(info *catalog.ExternalLocationInfo) *catalog.CreateExternalLocation { - return &catalog.CreateExternalLocation{ - Comment: info.Comment, - CredentialName: info.CredentialName, - // Output-only fields mirrored into state to avoid churn in remapped config. - EffectiveEnableFileEvents: info.EffectiveEnableFileEvents, - EffectiveFileEventQueue: info.EffectiveFileEventQueue, - EnableFileEvents: info.EnableFileEvents, - EncryptionDetails: info.EncryptionDetails, - Fallback: info.Fallback, - FileEventQueue: info.FileEventQueue, - Name: info.Name, - ReadOnly: info.ReadOnly, - SkipValidation: false, // This is an input-only parameter, never returned by API - Url: info.Url, - ForceSendFields: utils.FilterFields[catalog.CreateExternalLocation](info.ForceSendFields), - } -} - func (r *ResourceExternalLocation) DoRead(ctx context.Context, id string) (*catalog.ExternalLocationInfo, error) { return r.client.ExternalLocations.GetByName(ctx, id) } diff --git a/bundle/direct/dresources/instance_pool.go b/bundle/direct/dresources/instance_pool.go index 37a1aeab447..68cbd420e06 100644 --- a/bundle/direct/dresources/instance_pool.go +++ b/bundle/direct/dresources/instance_pool.go @@ -22,28 +22,6 @@ func (*ResourceInstancePool) PrepareState(input *resources.InstancePool) *comput } // RemapState copies the config fields shared by GetInstancePool and CreateInstancePool; -// output-only fields (state, stats, default_tags, instance_pool_id) are not in the state. -func (*ResourceInstancePool) RemapState(remote *compute.GetInstancePool) *compute.CreateInstancePool { - return &compute.CreateInstancePool{ - AwsAttributes: remote.AwsAttributes, - AzureAttributes: remote.AzureAttributes, - CustomTags: remote.CustomTags, - DiskSpec: remote.DiskSpec, - EnableElasticDisk: remote.EnableElasticDisk, - GcpAttributes: remote.GcpAttributes, - IdleInstanceAutoterminationMinutes: remote.IdleInstanceAutoterminationMinutes, - InstancePoolName: remote.InstancePoolName, - MaxCapacity: remote.MaxCapacity, - MinIdleInstances: remote.MinIdleInstances, - NodeTypeFlexibility: remote.NodeTypeFlexibility, - NodeTypeId: remote.NodeTypeId, - PreloadedDockerImages: remote.PreloadedDockerImages, - PreloadedSparkVersions: remote.PreloadedSparkVersions, - RemoteDiskThroughput: remote.RemoteDiskThroughput, - TotalInitialRemoteDiskSize: remote.TotalInitialRemoteDiskSize, - ForceSendFields: utils.FilterFields[compute.CreateInstancePool](remote.ForceSendFields), - } -} func (r *ResourceInstancePool) DoRead(ctx context.Context, id string) (*compute.GetInstancePool, error) { return r.client.InstancePools.GetByInstancePoolId(ctx, id) diff --git a/bundle/direct/dresources/model.go b/bundle/direct/dresources/model.go index ad8a9cca5a3..90055cb286d 100644 --- a/bundle/direct/dresources/model.go +++ b/bundle/direct/dresources/model.go @@ -42,15 +42,6 @@ func (*ResourceMlflowModel) PrepareState(input *resources.MlflowModel) *ml.Creat return &input.CreateModelRequest } -func (*ResourceMlflowModel) RemapState(output *MlflowModelRemote) *ml.CreateModelRequest { - return &ml.CreateModelRequest{ - Name: output.Name, - Tags: output.Tags, - Description: output.Description, - ForceSendFields: utils.FilterFields[ml.CreateModelRequest](output.ForceSendFields), - } -} - func (r *ResourceMlflowModel) DoRead(ctx context.Context, id string) (*MlflowModelRemote, error) { response, err := r.client.ModelRegistry.GetModel(ctx, ml.GetModelRequest{ Name: id, diff --git a/bundle/direct/dresources/postgres_snapshot_schedule.go b/bundle/direct/dresources/postgres_snapshot_schedule.go index 3f73e8c729f..acddbf19323 100644 --- a/bundle/direct/dresources/postgres_snapshot_schedule.go +++ b/bundle/direct/dresources/postgres_snapshot_schedule.go @@ -57,14 +57,6 @@ func (*ResourcePostgresSnapshotSchedule) PrepareState(input *resources.PostgresS } } -func (*ResourcePostgresSnapshotSchedule) RemapState(remote *PostgresSnapshotScheduleRemote) *PostgresSnapshotScheduleState { - return &PostgresSnapshotScheduleState{ - Branch: remote.Branch, - Schedule: remote.Schedule, - ForceSendFields: nil, - } -} - // makePostgresSnapshotScheduleRemote converts the SDK SnapshotSchedule into the // remote shape. The API addresses the schedule by "{branch}/snapshot-schedule"; // branch is derived by stripping that suffix so it participates in drift detection. diff --git a/bundle/direct/dresources/registered_model.go b/bundle/direct/dresources/registered_model.go index 1a2fd468ebb..337fd588272 100644 --- a/bundle/direct/dresources/registered_model.go +++ b/bundle/direct/dresources/registered_model.go @@ -24,30 +24,6 @@ func (*ResourceRegisteredModel) PrepareState(input *resources.RegisteredModel) * return &input.CreateRegisteredModelRequest } -func (*ResourceRegisteredModel) RemapState(model *catalog.RegisteredModelInfo) *catalog.CreateRegisteredModelRequest { - return &catalog.CreateRegisteredModelRequest{ - CatalogName: model.CatalogName, - Comment: model.Comment, - Name: model.Name, - SchemaName: model.SchemaName, - StorageLocation: model.StorageLocation, - ForceSendFields: utils.FilterFields[catalog.CreateRegisteredModelRequest](model.ForceSendFields), - - Aliases: model.Aliases, - BrowseOnly: model.BrowseOnly, - FullName: model.FullName, - MetastoreId: model.MetastoreId, - Owner: model.Owner, - - // Output only fields. Remote changes to these are ignored via - // ignore_remote_changes in registered_models.yml rather than zeroed here. - CreatedAt: model.CreatedAt, - CreatedBy: model.CreatedBy, - UpdatedAt: model.UpdatedAt, - UpdatedBy: model.UpdatedBy, - } -} - func (r *ResourceRegisteredModel) DoRead(ctx context.Context, id string) (*catalog.RegisteredModelInfo, error) { return r.client.RegisteredModels.Get(ctx, catalog.GetRegisteredModelRequest{ FullName: id, diff --git a/bundle/direct/dresources/schema.go b/bundle/direct/dresources/schema.go index d80dac584bc..04c2e441861 100644 --- a/bundle/direct/dresources/schema.go +++ b/bundle/direct/dresources/schema.go @@ -23,18 +23,6 @@ func (*ResourceSchema) PrepareState(input *resources.Schema) *catalog.CreateSche return &input.CreateSchema } -func (*ResourceSchema) RemapState(info *catalog.SchemaInfo) *catalog.CreateSchema { - return &catalog.CreateSchema{ - CatalogName: info.CatalogName, - Comment: info.Comment, - CustomMaxRetentionHours: info.CustomMaxRetentionHours, - Name: info.Name, - Properties: info.Properties, - StorageRoot: info.StorageRoot, - ForceSendFields: utils.FilterFields[catalog.CreateSchema](info.ForceSendFields), - } -} - func (r *ResourceSchema) DoRead(ctx context.Context, id string) (*catalog.SchemaInfo, error) { return r.client.Schemas.GetByFullName(ctx, id) } diff --git a/bundle/direct/dresources/snapshot.go b/bundle/direct/dresources/snapshot.go index 5c59e5110c8..a049a5a9c6c 100644 --- a/bundle/direct/dresources/snapshot.go +++ b/bundle/direct/dresources/snapshot.go @@ -63,16 +63,6 @@ func (s *ResourceSnapshot) PrepareState(input *resources.Snapshot) *SnapshotStat } } -func (s *ResourceSnapshot) RemapState(remote *SnapshotRemote) *SnapshotState { - return &SnapshotState{ - RelativePath: remote.RelativePath, - FullPath: remote.FullPath, - BundleID: "", - ACL: nil, - ZipPath: "", - } -} - func (s *ResourceSnapshot) DoRead(ctx context.Context, id string) (*SnapshotRemote, error) { info, err := s.uploader.Get(ctx, id) if err != nil { diff --git a/bundle/direct/dresources/vector_search_endpoint.go b/bundle/direct/dresources/vector_search_endpoint.go index 12470872b62..1480fe56053 100644 --- a/bundle/direct/dresources/vector_search_endpoint.go +++ b/bundle/direct/dresources/vector_search_endpoint.go @@ -6,7 +6,6 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/structs/structpath" - "github.com/databricks/cli/libs/utils" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/marshal" "github.com/databricks/databricks-sdk-go/service/vectorsearch" @@ -61,17 +60,6 @@ func (*ResourceVectorSearchEndpoint) PrepareState(input *resources.VectorSearchE return &input.CreateEndpoint } -func (*ResourceVectorSearchEndpoint) RemapState(remote *VectorSearchEndpointRemote) *vectorsearch.CreateEndpoint { - return &vectorsearch.CreateEndpoint{ - Name: remote.Name, - EndpointType: remote.EndpointType, - BudgetPolicyId: remote.BudgetPolicyId, - UsagePolicyId: "", // Missing in remote - TargetQps: remote.TargetQps, - ForceSendFields: utils.FilterFields[vectorsearch.CreateEndpoint](remote.ForceSendFields, "UsagePolicyId"), - } -} - func (r *ResourceVectorSearchEndpoint) DoRead(ctx context.Context, id string) (*VectorSearchEndpointRemote, error) { info, err := r.client.VectorSearchEndpoints.GetEndpointByEndpointName(ctx, id) if err != nil { diff --git a/bundle/direct/dresources/volume.go b/bundle/direct/dresources/volume.go index 7cd1eaba1ce..a58326f58ce 100644 --- a/bundle/direct/dresources/volume.go +++ b/bundle/direct/dresources/volume.go @@ -25,18 +25,6 @@ func (*ResourceVolume) PrepareState(input *resources.Volume) *catalog.CreateVolu return &input.CreateVolumeRequestContent } -func (*ResourceVolume) RemapState(info *catalog.VolumeInfo) *catalog.CreateVolumeRequestContent { - return &catalog.CreateVolumeRequestContent{ - CatalogName: info.CatalogName, - Comment: info.Comment, - Name: info.Name, - SchemaName: info.SchemaName, - StorageLocation: info.StorageLocation, - VolumeType: info.VolumeType, - ForceSendFields: utils.FilterFields[catalog.CreateVolumeRequestContent](info.ForceSendFields), - } -} - func (r *ResourceVolume) DoRead(ctx context.Context, id string) (*catalog.VolumeInfo, error) { return r.client.Volumes.ReadByName(ctx, id) } diff --git a/libs/utils/utils.go b/libs/utils/utils.go index 1636594e0e7..eca9d77ca87 100644 --- a/libs/utils/utils.go +++ b/libs/utils/utils.go @@ -8,8 +8,13 @@ import ( // excluding any fields specified in the excludeFields list. // We must use that when copying structs because JSON marshaller in SDK crashes if it sees unknown field. func FilterFields[T any](fields []string, excludeFields ...string) []string { + return FilterFieldsType(reflect.TypeFor[T](), fields, excludeFields...) +} + +// FilterFieldsType is FilterFields with the destination type supplied as a reflect.Type +// rather than a type parameter, for callers that only know the type at runtime. +func FilterFieldsType(typeOfT reflect.Type, fields []string, excludeFields ...string) []string { var result []string - typeOfT := reflect.TypeFor[T]() excludeMap := make(map[string]bool) for _, exclude := range excludeFields { From 71c282809a0f5188d0a278a4a0426972c587b893 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 11:35:20 +0200 Subject: [PATCH 03/18] direct: document auto-generated copiers in dresources README Co-authored-by: Isaac --- bundle/direct/dresources/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index 5487042c965..da0bc228b51 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -75,6 +75,15 @@ Declaring a field under `hashed_fields` in `resources.yml` makes the engine pers ## RemapState is a dumb copy; DoRead owns all remapping +Most resources do not need a `RemapState` method at all. When `StateType` is a plain subset +of `RemoteType`, the framework auto-generates the copy from the two types: `buildCopiers` +(in `all.go`) compiles a copier per resource at package init, matching fields by JSON name, +filtering `ForceSendFields`, and applying only lossless conversions. Anything it cannot copy +safely (a kind-changing conversion, a mismatched struct shape) fails at load, so a resource +that needs real logic surfaces as a build error rather than silent drift. Write a `RemapState` +method only for that logic (derived fields, renames, per-field `ForceSendFields` rules); the +rest of this section governs those hand-written overrides. + `RemapState` converts `RemoteType` to `StateType` only because `StateType` is typically a subset of `RemoteType`. It must be a field-by-field copy (or no-op), never a place for logic. In particular, do not remap a differently-named field there (e.g. `state.x = remote.status.x`). From 85c9fe63e32e3bf94fa362534906fe4f2c86ccba Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 11:48:28 +0200 Subject: [PATCH 04/18] direct: satisfy linters in copier (exhaustruct, exhaustive, modernize) Co-authored-by: Isaac --- bundle/direct/dresources/copier.go | 13 ++++++------- bundle/direct/dresources/copier_test.go | 6 ++++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/bundle/direct/dresources/copier.go b/bundle/direct/dresources/copier.go index 6efe4eaed9c..f58c0a8a4a5 100644 --- a/bundle/direct/dresources/copier.go +++ b/bundle/direct/dresources/copier.go @@ -2,6 +2,7 @@ package dresources import ( "fmt" + "maps" "reflect" "github.com/databricks/cli/libs/structs/structaccess" @@ -72,16 +73,16 @@ func compileCopier(remoteType, stateType reflect.Type) (*copier, error) { } switch { case dst.typ == src.typ: - ops = append(ops, copyOp{dstIndex: dst.index, srcIndex: src.index, dstType: dst.typ}) + ops = append(ops, copyOp{forceSendFields: false, dstIndex: dst.index, srcIndex: src.index, convert: false, dstType: dst.typ, ownerType: nil}) case safeConvert(dst.typ, src.typ): - ops = append(ops, copyOp{dstIndex: dst.index, srcIndex: src.index, convert: true, dstType: dst.typ}) + ops = append(ops, copyOp{forceSendFields: false, dstIndex: dst.index, srcIndex: src.index, convert: true, dstType: dst.typ, ownerType: nil}) default: return nil, fmt.Errorf("field %q: state type %s cannot be copied from remote type %s; implement RemapState", name, dst.typ, src.typ) } } for _, target := range dstForceSend { - ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType}) + ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, srcIndex: nil, convert: false, dstType: nil, ownerType: target.ownerType}) } return &copier{stateElem: stateElem, ops: ops}, nil @@ -101,7 +102,7 @@ func rootForceSendFields(remote reflect.Value) []string { if !f.IsValid() || f.Kind() != reflect.Slice { return nil } - fields, _ := f.Interface().([]string) + fields, _ := reflect.TypeAssert[[]string](f) return fields } @@ -152,9 +153,7 @@ func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []fo } if structaccess.IsFlattenedEmbed(sf) { nested, nestedForceSend := flattenStruct(sf.Type, index) - for name, info := range nested { - fields[name] = info - } + maps.Copy(fields, nested) forceSend = append(forceSend, nestedForceSend...) continue } diff --git a/bundle/direct/dresources/copier_test.go b/bundle/direct/dresources/copier_test.go index c4120939e1a..89af8d18b96 100644 --- a/bundle/direct/dresources/copier_test.go +++ b/bundle/direct/dresources/copier_test.go @@ -2,7 +2,7 @@ package dresources import ( "reflect" - "sort" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -50,7 +50,7 @@ func TestNoRedundantRemapState(t *testing.T) { } } - sort.Strings(redundant) + slices.Sort(redundant) assert.Empty(t, redundant, "these resources have a RemapState the auto-copier reproduces exactly; delete the method and let buildCopiers handle it") } @@ -175,5 +175,7 @@ func fillValue(v reflect.Value, depth int, visited map[reflect.Type]bool) { if f := v.FieldByName("ForceSendFields"); f.IsValid() && f.Kind() == reflect.Slice && f.Type().Elem().Kind() == reflect.String { f.Set(reflect.ValueOf(names)) } + default: + // other kinds (interface, chan, func, array, complex, uintptr, ...) are left zero } } From 9da4567e37802acb34e4d22b80c241242a1efb07 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 12:21:55 +0200 Subject: [PATCH 05/18] direct: reuse fillNonZero in copier test, tidy table and README Drop the duplicate reflection filler in favor of the existing fillNonZero, populating ForceSendFields with a small dedicated helper. Field-per-line table in TestSafeConvert; shorten the README section. Co-authored-by: Isaac --- bundle/direct/dresources/README.md | 20 +++--- bundle/direct/dresources/copier_test.go | 90 +++++++++++++------------ 2 files changed, 55 insertions(+), 55 deletions(-) diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index da0bc228b51..02710ffd004 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -75,17 +75,15 @@ Declaring a field under `hashed_fields` in `resources.yml` makes the engine pers ## RemapState is a dumb copy; DoRead owns all remapping -Most resources do not need a `RemapState` method at all. When `StateType` is a plain subset -of `RemoteType`, the framework auto-generates the copy from the two types: `buildCopiers` -(in `all.go`) compiles a copier per resource at package init, matching fields by JSON name, -filtering `ForceSendFields`, and applying only lossless conversions. Anything it cannot copy -safely (a kind-changing conversion, a mismatched struct shape) fails at load, so a resource -that needs real logic surfaces as a build error rather than silent drift. Write a `RemapState` -method only for that logic (derived fields, renames, per-field `ForceSendFields` rules); the -rest of this section governs those hand-written overrides. - -`RemapState` converts `RemoteType` to `StateType` only because `StateType` is typically a -subset of `RemoteType`. It must be a field-by-field copy (or no-op), never a place for +New resources should not define `RemapState`. When `StateType` is a subset of `RemoteType`, +the framework copies it automatically: `buildCopiers` (in `all.go`) compiles a copier per +resource at package init, matching fields by JSON name, filtering `ForceSendFields`, and +applying only lossless conversions. Anything it cannot copy safely (a kind-changing conversion, +a mismatched struct shape) fails at load rather than as silent drift. Add a `RemapState` method +only for logic the copier cannot express — derived fields, renames, per-field `ForceSendFields` +rules — and the rest of this section governs those overrides. + +Such a `RemapState` must still be a field-by-field copy (or no-op), never a place for arbitrary logic. In particular, do not remap a differently-named field there (e.g. `state.x = remote.status.x`). Any remapping the API requires belongs in `DoRead`: add `x` directly to `RemoteType` and populate it from `status.x` inside `DoRead`. diff --git a/bundle/direct/dresources/copier_test.go b/bundle/direct/dresources/copier_test.go index 89af8d18b96..74dc77f8b63 100644 --- a/bundle/direct/dresources/copier_test.go +++ b/bundle/direct/dresources/copier_test.go @@ -41,7 +41,8 @@ func TestNoRedundantRemapState(t *testing.T) { } remote := reflect.New(remoteType.Elem()) - fillValue(remote.Elem(), 0, map[reflect.Type]bool{}) + fillNonZero(remote.Elem(), 0) + setForceSendFields(remote.Elem()) want := m.Call([]reflect.Value{remote})[0].Interface() got := copier.copy(remote.Interface()) @@ -111,11 +112,36 @@ func TestSafeConvert(t *testing.T) { src reflect.Type want bool }{ - {"identical", reflect.TypeFor[string](), reflect.TypeFor[string](), true}, - {"same underlying string enums", reflect.TypeFor[copierKindA](), reflect.TypeFor[copierKindB](), true}, - {"int to string", reflect.TypeFor[string](), reflect.TypeFor[int](), false}, - {"int64 to int32", reflect.TypeFor[int32](), reflect.TypeFor[int64](), false}, - {"bytes to string", reflect.TypeFor[string](), reflect.TypeFor[[]byte](), false}, + { + name: "identical", + dst: reflect.TypeFor[string](), + src: reflect.TypeFor[string](), + want: true, + }, + { + name: "same underlying string enums", + dst: reflect.TypeFor[copierKindA](), + src: reflect.TypeFor[copierKindB](), + want: true, + }, + { + name: "int to string", + dst: reflect.TypeFor[string](), + src: reflect.TypeFor[int](), + want: false, + }, + { + name: "int64 to int32", + dst: reflect.TypeFor[int32](), + src: reflect.TypeFor[int64](), + want: false, + }, + { + name: "bytes to string", + dst: reflect.TypeFor[string](), + src: reflect.TypeFor[[]byte](), + want: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -124,58 +150,34 @@ func TestSafeConvert(t *testing.T) { } } -// fillValue sets every exported field to a non-zero value, bounded by depth to avoid cycles. -// Each struct's ForceSendFields is populated with its own field names so the FSF-filtering -// path is exercised at every level. -func fillValue(v reflect.Value, depth int, visited map[reflect.Type]bool) { - if depth > 5 { - return - } +// setForceSendFields recursively sets every struct's ForceSendFields to its own field names, +// so the copier's ForceSendFields-filtering path is exercised at every level. fillNonZero +// deliberately leaves ForceSendFields empty (round-trip tests need that), so we populate it +// here. Map values are not addressable and cannot hold a relevant FSF source, so they are skipped. +func setForceSendFields(v reflect.Value) { switch v.Kind() { - case reflect.String: - v.SetString("x") - 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.Pointer: - v.Set(reflect.New(v.Type().Elem())) - fillValue(v.Elem(), depth+1, visited) + if !v.IsNil() { + setForceSendFields(v.Elem()) + } case reflect.Slice: - s := reflect.MakeSlice(v.Type(), 1, 1) - fillValue(s.Index(0), depth+1, visited) - v.Set(s) - case reflect.Map: - m := reflect.MakeMap(v.Type()) - key := reflect.New(v.Type().Key()).Elem() - fillValue(key, depth+1, visited) - val := reflect.New(v.Type().Elem()).Elem() - fillValue(val, depth+1, visited) - m.SetMapIndex(key, val) - v.Set(m) - case reflect.Struct: - if visited[v.Type()] { - return + for i := range v.Len() { + setForceSendFields(v.Index(i)) } - visited[v.Type()] = true - defer delete(visited, v.Type()) + case reflect.Struct: var names []string for i := range v.NumField() { sf := v.Type().Field(i) - if sf.PkgPath != "" || sf.Name == "ForceSendFields" { + if !sf.IsExported() || sf.Name == "ForceSendFields" { continue } names = append(names, sf.Name) - fillValue(v.Field(i), depth+1, visited) + setForceSendFields(v.Field(i)) } if f := v.FieldByName("ForceSendFields"); f.IsValid() && f.Kind() == reflect.Slice && f.Type().Elem().Kind() == reflect.String { f.Set(reflect.ValueOf(names)) } default: - // other kinds (interface, chan, func, array, complex, uintptr, ...) are left zero + // scalars and other kinds have no nested ForceSendFields to set } } From 99580c43f625bb8dedf4dce0709650b1b2b8ba90 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 12:28:58 +0200 Subject: [PATCH 06/18] structcopy: extract the generic copier into libs/structs/structcopy Move the reflection copier (compile-time type check, field/JSON-name copy, ForceSendFields filtering, lossless conversions) out of dresources into a reusable package beside structwalk/structdiff/structaccess. The direct engine wiring (buildCopiers, driven off SupportedResources) stays in dresources and now calls structcopy.Compile. Co-authored-by: Isaac --- bundle/direct/dresources/adapter.go | 5 +- bundle/direct/dresources/all.go | 9 +- bundle/direct/dresources/copier_test.go | 101 +--------------- .../structs/structcopy/structcopy.go | 111 +++++++++--------- libs/structs/structcopy/structcopy_test.go | 104 ++++++++++++++++ 5 files changed, 171 insertions(+), 159 deletions(-) rename bundle/direct/dresources/copier.go => libs/structs/structcopy/structcopy.go (60%) create mode 100644 libs/structs/structcopy/structcopy_test.go diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 40406bcf8ca..38282049f61 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/bundle/deployplan" "github.com/databricks/cli/libs/calladapt" + "github.com/databricks/cli/libs/structs/structcopy" "github.com/databricks/cli/libs/structs/structpath" "github.com/databricks/cli/libs/structs/structvar" "github.com/databricks/databricks-sdk-go" @@ -130,7 +131,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: - copier *copier + copier *structcopy.Copier doDelete *calladapt.BoundCaller prepareInputConfig *calladapt.BoundCaller isEmptyState *calladapt.BoundCaller @@ -546,7 +547,7 @@ func (a *Adapter) RemapState(remoteState any) (any, error) { return outs[0], nil } if a.copier != nil { - return a.copier.copy(remoteState), nil + return a.copier.Copy(remoteState), nil } // No custom method and no copier: validate() only allows this when // remoteType == stateType, so the remote is already the state type. diff --git a/bundle/direct/dresources/all.go b/bundle/direct/dresources/all.go index 799e38bee72..e6ebade129a 100644 --- a/bundle/direct/dresources/all.go +++ b/bundle/direct/dresources/all.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/databricks/cli/libs/calladapt" + "github.com/databricks/cli/libs/structs/structcopy" "github.com/databricks/databricks-sdk-go" ) @@ -93,9 +94,9 @@ var SupportedResources = map[string]any{ // RemapState method and are skipped here. var copiers = buildCopiers() -func buildCopiers() map[reflect.Type]*copier { +func buildCopiers() map[reflect.Type]*structcopy.Copier { iface := reflect.TypeFor[IResource]() - out := make(map[reflect.Type]*copier) + out := make(map[reflect.Type]*structcopy.Copier) var errs []string for resourceType, resource := range SupportedResources { @@ -130,9 +131,9 @@ func buildCopiers() map[reflect.Type]*copier { continue // identity: the adapter returns the remote unchanged, no copier needed } - copier, err := compileCopier(remoteType, stateType) + copier, err := structcopy.Compile(remoteType, stateType) if err != nil { - errs = append(errs, fmt.Sprintf("%s: %v", resourceType, err)) + errs = append(errs, fmt.Sprintf("%s: %v (implement RemapState for this resource)", resourceType, err)) continue } out[implType] = copier diff --git a/bundle/direct/dresources/copier_test.go b/bundle/direct/dresources/copier_test.go index 74dc77f8b63..f25cc9cd153 100644 --- a/bundle/direct/dresources/copier_test.go +++ b/bundle/direct/dresources/copier_test.go @@ -5,8 +5,8 @@ import ( "slices" "testing" + "github.com/databricks/cli/libs/structs/structcopy" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) // TestNoRedundantRemapState enforces the "only override when needed" rule: a resource that @@ -35,7 +35,7 @@ func TestNoRedundantRemapState(t *testing.T) { continue // identity, the copier is not involved } - copier, err := compileCopier(remoteType, stateType) + copier, err := structcopy.Compile(remoteType, stateType) if err != nil { continue // copier cannot handle it, so the override is required } @@ -45,7 +45,7 @@ func TestNoRedundantRemapState(t *testing.T) { setForceSendFields(remote.Elem()) want := m.Call([]reflect.Value{remote})[0].Interface() - got := copier.copy(remote.Interface()) + got := copier.Copy(remote.Interface()) if reflect.DeepEqual(want, got) { redundant = append(redundant, resourceType) } @@ -55,101 +55,6 @@ func TestNoRedundantRemapState(t *testing.T) { assert.Empty(t, redundant, "these resources have a RemapState the auto-copier reproduces exactly; delete the method and let buildCopiers handle it") } -type copierKindA string - -type copierKindB string - -type copierRemote struct { - Name string `json:"name"` - Extra string `json:"extra"` // absent from state -> dropped - Kind copierKindA `json:"kind"` // same underlying type as state, distinct name -> converted - ForceSendFields []string -} - -type copierState struct { - Name string `json:"name"` - Missing string `json:"missing"` // absent from remote -> left zero - Kind copierKindB `json:"kind"` - ForceSendFields []string -} - -func TestRemapCopierCopy(t *testing.T) { - copier, err := compileCopier(reflect.TypeFor[*copierRemote](), reflect.TypeFor[*copierState]()) - require.NoError(t, err) - - remote := &copierRemote{ - Name: "n", - Extra: "e", - Kind: "classic", - ForceSendFields: []string{"Name", "Extra", "Kind"}, - } - got := copier.copy(remote).(*copierState) - - assert.Equal(t, "n", got.Name) - assert.Empty(t, got.Missing) // absent from remote - assert.Equal(t, copierKindB("classic"), got.Kind) // converted across enum types - assert.Equal(t, []string{"Name", "Kind"}, got.ForceSendFields) // "Extra" filtered out (not a state field) -} - -type copierUnsafeRemote struct { - N int `json:"n"` -} - -type copierUnsafeState struct { - N string `json:"n"` -} - -func TestRemapCopierCompileRejectsUnsafe(t *testing.T) { - _, err := compileCopier(reflect.TypeFor[*copierUnsafeRemote](), reflect.TypeFor[*copierUnsafeState]()) - require.Error(t, err) - assert.Contains(t, err.Error(), "implement RemapState") -} - -func TestSafeConvert(t *testing.T) { - tests := []struct { - name string - dst reflect.Type - src reflect.Type - want bool - }{ - { - name: "identical", - dst: reflect.TypeFor[string](), - src: reflect.TypeFor[string](), - want: true, - }, - { - name: "same underlying string enums", - dst: reflect.TypeFor[copierKindA](), - src: reflect.TypeFor[copierKindB](), - want: true, - }, - { - name: "int to string", - dst: reflect.TypeFor[string](), - src: reflect.TypeFor[int](), - want: false, - }, - { - name: "int64 to int32", - dst: reflect.TypeFor[int32](), - src: reflect.TypeFor[int64](), - want: false, - }, - { - name: "bytes to string", - dst: reflect.TypeFor[string](), - src: reflect.TypeFor[[]byte](), - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, safeConvert(tt.dst, tt.src)) - }) - } -} - // setForceSendFields recursively sets every struct's ForceSendFields to its own field names, // so the copier's ForceSendFields-filtering path is exercised at every level. fillNonZero // deliberately leaves ForceSendFields empty (round-trip tests need that), so we populate it diff --git a/bundle/direct/dresources/copier.go b/libs/structs/structcopy/structcopy.go similarity index 60% rename from bundle/direct/dresources/copier.go rename to libs/structs/structcopy/structcopy.go index f58c0a8a4a5..6965e44cc7f 100644 --- a/bundle/direct/dresources/copier.go +++ b/libs/structs/structcopy/structcopy.go @@ -1,4 +1,11 @@ -package dresources +// Package structcopy compiles a reflection-based copier between two struct types. +// The copier moves fields that match by JSON name, filters ForceSendFields to the +// destination type, and applies only lossless conversions. The plan is compiled once +// from the two types (see Compile); executing it on any values of those types cannot fail. +// +// It underpins the direct engine's automatic RemapState (bundle/direct/dresources): when a +// resource's state type is a plain subset of its remote type, no hand-written copy is needed. +package structcopy import ( "fmt" @@ -10,28 +17,21 @@ import ( "github.com/databricks/cli/libs/utils" ) -// copier is a reflection-built RemapState: it copies fields from a remote -// struct into a fresh state struct by matching JSON field names. It replaces the -// hand-written "dumb copy" RemapState methods (see README.md) for resources whose -// state type is a plain subset of the remote type. -// -// The plan is compiled once from the two types (see compileCopier); executing it -// on any pair of values of those types cannot fail. Anything the plan cannot copy -// safely is rejected at compile time, so a resource that needs real logic surfaces -// as a build/init error rather than a silent drift bug — see buildCopiers. -type copier struct { - stateElem reflect.Type // struct type behind *StateType - ops []copyOp +// Copier copies fields from a source struct into a fresh destination struct by matching +// JSON field names. Build one with Compile; the compiled plan cannot fail at Copy time. +type Copier struct { + dstElem reflect.Type // struct type behind the destination pointer type + ops []copyOp } type copyOp struct { // forceSendFields ops recompute a ForceSendFields slice; field-copy ops move one field. forceSendFields bool - dstIndex []int // FieldByIndex path in the state struct + dstIndex []int // FieldByIndex path in the destination struct // field-copy ops: - srcIndex []int // FieldByIndex path in the remote struct + srcIndex []int // FieldByIndex path in the source struct convert bool // use reflect.Convert (distinct but same-underlying types) instead of a direct set dstType reflect.Type // target type for Convert @@ -53,22 +53,22 @@ type forceSendTarget struct { ownerType reflect.Type } -// compileCopier builds the copy plan for remoteType -> stateType (both pointers to -// structs). It returns an error if any state field that also exists on the remote cannot -// be copied without a value-changing conversion; such a field needs a custom RemapState. -func compileCopier(remoteType, stateType reflect.Type) (*copier, error) { - stateElem := stateType.Elem() - remoteElem := remoteType.Elem() +// Compile builds the copy plan for srcType -> dstType (both pointers to structs). It returns +// an error if a destination field that also exists on the source cannot be copied without a +// value-changing conversion, so callers can reject such a type pair up front rather than +// silently dropping the field. +func Compile(srcType, dstType reflect.Type) (*Copier, error) { + dstElem := dstType.Elem() + srcElem := srcType.Elem() - dstFields, dstForceSend := flattenStruct(stateElem, nil) - srcFields, _ := flattenStruct(remoteElem, nil) + dstFields, dstForceSend := flattenStruct(dstElem, nil) + srcFields, _ := flattenStruct(srcElem, nil) var ops []copyOp for name, dst := range dstFields { src, ok := srcFields[name] if !ok { - // Field absent from the remote type: always nil/zero in the remapped - // state. This is the missing_in_remote invariant the planner relies on. + // Field absent from the source type is left zero in the destination. continue } switch { @@ -77,7 +77,7 @@ func compileCopier(remoteType, stateType reflect.Type) (*copier, error) { case safeConvert(dst.typ, src.typ): ops = append(ops, copyOp{forceSendFields: false, dstIndex: dst.index, srcIndex: src.index, convert: true, dstType: dst.typ, ownerType: nil}) default: - return nil, fmt.Errorf("field %q: state type %s cannot be copied from remote type %s; implement RemapState", name, dst.typ, src.typ) + return nil, fmt.Errorf("field %q: destination type %s is not assignable or safely convertible from source type %s", name, dst.typ, src.typ) } } @@ -85,57 +85,58 @@ func compileCopier(remoteType, stateType reflect.Type) (*copier, error) { ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, srcIndex: nil, convert: false, dstType: nil, ownerType: target.ownerType}) } - return &copier{stateElem: stateElem, ops: ops}, nil -} - -// safeConvert reports whether a value of src can be converted to dst without changing -// the value. Distinct named types with the same underlying type (e.g. two string enums) -// qualify; kind-changing conversions (int<->string, float->int, []byte<->string) do not, -// because reflect.Convert would silently corrupt the value. -func safeConvert(dst, src reflect.Type) bool { - return src.ConvertibleTo(dst) && src.Kind() == dst.Kind() -} - -// rootForceSendFields returns the ForceSendFields slice of the remote struct's root, or nil. -func rootForceSendFields(remote reflect.Value) []string { - f := remote.FieldByName("ForceSendFields") - if !f.IsValid() || f.Kind() != reflect.Slice { - return nil - } - fields, _ := reflect.TypeAssert[[]string](f) - return fields + return &Copier{dstElem: dstElem, ops: ops}, nil } -// copy builds a fresh *StateType populated from remote (a *RemoteType). -func (c *copier) copy(remote any) any { - src := reflect.ValueOf(remote).Elem() - dstPtr := reflect.New(c.stateElem) - dst := dstPtr.Elem() +// Copy builds a fresh destination value (a pointer to the destination struct) populated +// from src (a pointer to the source struct). +func (c *Copier) Copy(src any) any { + srcVal := reflect.ValueOf(src).Elem() + dstPtr := reflect.New(c.dstElem) + dstVal := dstPtr.Elem() var srcForceSend []string for _, op := range c.ops { if op.forceSendFields { if srcForceSend == nil { - srcForceSend = rootForceSendFields(src) + srcForceSend = rootForceSendFields(srcVal) } filtered := utils.FilterFieldsType(op.ownerType, srcForceSend) - dst.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) + dstVal.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) continue } - val := src.FieldByIndex(op.srcIndex) + val := srcVal.FieldByIndex(op.srcIndex) if op.convert { val = val.Convert(op.dstType) } - dst.FieldByIndex(op.dstIndex).Set(val) + dstVal.FieldByIndex(op.dstIndex).Set(val) } return dstPtr.Interface() } +// safeConvert reports whether a value of src can be converted to dst without changing +// the value. Distinct named types with the same underlying type (e.g. two string enums) +// qualify; kind-changing conversions (int<->string, float->int, []byte<->string) do not, +// because reflect.Convert would silently corrupt the value. +func safeConvert(dst, src reflect.Type) bool { + return src.ConvertibleTo(dst) && src.Kind() == dst.Kind() +} + +// rootForceSendFields returns the ForceSendFields slice at the struct's root, or nil. +func rootForceSendFields(v reflect.Value) []string { + f := v.FieldByName("ForceSendFields") + if !f.IsValid() || f.Kind() != reflect.Slice { + return nil + } + fields, _ := reflect.TypeAssert[[]string](f) + return fields +} + // flattenStruct enumerates a struct's top-level JSON fields (descending through // encoding/json-flattened embeds and accumulating the field-index prefix) plus every // ForceSendFields slice reachable through those embeds. Names follow the same JSON tag -// resolution the diff engine uses, so copied state compares consistently. +// resolution the diff engine uses, so copied values compare consistently. func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []forceSendTarget) { fields := make(map[string]jsonFieldInfo) var forceSend []forceSendTarget diff --git a/libs/structs/structcopy/structcopy_test.go b/libs/structs/structcopy/structcopy_test.go new file mode 100644 index 00000000000..44cdc856616 --- /dev/null +++ b/libs/structs/structcopy/structcopy_test.go @@ -0,0 +1,104 @@ +package structcopy + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type copyKindA string + +type copyKindB string + +type copySource struct { + Name string `json:"name"` + Extra string `json:"extra"` // absent from dst -> dropped + Kind copyKindA `json:"kind"` // same underlying type as dst, distinct name -> converted + ForceSendFields []string +} + +type copyDest struct { + Name string `json:"name"` + Missing string `json:"missing"` // absent from src -> left zero + Kind copyKindB `json:"kind"` + ForceSendFields []string +} + +func TestCopy(t *testing.T) { + copier, err := Compile(reflect.TypeFor[*copySource](), reflect.TypeFor[*copyDest]()) + require.NoError(t, err) + + src := ©Source{ + Name: "n", + Extra: "e", + Kind: "classic", + ForceSendFields: []string{"Name", "Extra", "Kind"}, + } + got := copier.Copy(src).(*copyDest) + + assert.Equal(t, "n", got.Name) + assert.Empty(t, got.Missing) // absent from source + assert.Equal(t, copyKindB("classic"), got.Kind) // converted across enum types + assert.Equal(t, []string{"Name", "Kind"}, got.ForceSendFields) // "Extra" filtered out (not a dst field) +} + +type unsafeSource struct { + N int `json:"n"` +} + +type unsafeDest struct { + N string `json:"n"` +} + +func TestCompileRejectsUnsafe(t *testing.T) { + _, err := Compile(reflect.TypeFor[*unsafeSource](), reflect.TypeFor[*unsafeDest]()) + require.Error(t, err) + assert.Contains(t, err.Error(), "not assignable or safely convertible") +} + +func TestSafeConvert(t *testing.T) { + tests := []struct { + name string + dst reflect.Type + src reflect.Type + want bool + }{ + { + name: "identical", + dst: reflect.TypeFor[string](), + src: reflect.TypeFor[string](), + want: true, + }, + { + name: "same underlying string enums", + dst: reflect.TypeFor[copyKindA](), + src: reflect.TypeFor[copyKindB](), + want: true, + }, + { + name: "int to string", + dst: reflect.TypeFor[string](), + src: reflect.TypeFor[int](), + want: false, + }, + { + name: "int64 to int32", + dst: reflect.TypeFor[int32](), + src: reflect.TypeFor[int64](), + want: false, + }, + { + name: "bytes to string", + dst: reflect.TypeFor[string](), + src: reflect.TypeFor[[]byte](), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, safeConvert(tt.dst, tt.src)) + }) + } +} From e43632802f82d77e29c5b227993b6d9d7b64b6d6 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 23 Sep 2026 12:03:01 +0200 Subject: [PATCH 07/18] direct: delete clusters/sql_warehouses/secret_scopes RemapState (copier handles them) After the prep PRs made these dumb, the auto-generated copier reproduces them exactly (TestNoRedundantRemapState), so remove the methods. Co-authored-by: Isaac --- bundle/direct/dresources/cluster.go | 48 ----------------------- bundle/direct/dresources/secret_scope.go | 13 ------ bundle/direct/dresources/sql_warehouse.go | 24 ------------ 3 files changed, 85 deletions(-) diff --git a/bundle/direct/dresources/cluster.go b/bundle/direct/dresources/cluster.go index 7f1eb26dffc..cc590be69fe 100644 --- a/bundle/direct/dresources/cluster.go +++ b/bundle/direct/dresources/cluster.go @@ -102,54 +102,6 @@ func (r *ResourceCluster) PrepareState(input *resources.Cluster) *ClusterState { return s } -// RemapState maps the remote ClusterRemote to ClusterState for diff comparison. -// Started is derived from cluster state so the planner can detect start/stop changes. -func (r *ResourceCluster) RemapState(input *ClusterRemote) *ClusterState { - spec := &ClusterState{ - ClusterSpec: compute.ClusterSpec{ - ApplyPolicyDefaultValues: input.ApplyPolicyDefaultValues, - Autoscale: input.Autoscale, - AutoterminationMinutes: input.AutoterminationMinutes, - AwsAttributes: input.AwsAttributes, - AzureAttributes: input.AzureAttributes, - ClusterLogConf: input.ClusterLogConf, - ClusterName: input.ClusterName, - CustomTags: input.CustomTags, - DataSecurityMode: input.DataSecurityMode, - DependencyMode: input.DependencyMode, - DockerImage: input.DockerImage, - DriverInstancePoolId: input.DriverInstancePoolId, - DriverNodeTypeId: input.DriverNodeTypeId, - DriverNodeTypeFlexibility: input.DriverNodeTypeFlexibility, - EnableElasticDisk: input.EnableElasticDisk, - EnableLocalDiskEncryption: input.EnableLocalDiskEncryption, - GcpAttributes: input.GcpAttributes, - InitScripts: input.InitScripts, - InstancePoolId: input.InstancePoolId, - IsSingleNode: input.IsSingleNode, - Kind: input.Kind, - NodeTypeId: input.NodeTypeId, - NumWorkers: input.NumWorkers, - PolicyId: input.PolicyId, - RemoteDiskThroughput: input.RemoteDiskThroughput, - RuntimeEngine: input.RuntimeEngine, - SingleUserName: input.SingleUserName, - SparkConf: input.SparkConf, - SparkEnvVars: input.SparkEnvVars, - SparkVersion: input.SparkVersion, - SshPublicKeys: input.SshPublicKeys, - TotalInitialRemoteDiskSize: input.TotalInitialRemoteDiskSize, - UseMlRuntime: input.UseMlRuntime, - WorkloadType: input.WorkloadType, - WorkerNodeTypeFlexibility: input.WorkerNodeTypeFlexibility, - ForceSendFields: utils.FilterFields[compute.ClusterSpec](input.ForceSendFields), - }, - Lifecycle: input.Lifecycle, - Libraries: input.Libraries, - } - return spec -} - func (r *ResourceCluster) DoRead(ctx context.Context, id string) (*ClusterRemote, error) { var details *compute.ClusterDetails var libraries []compute.Library diff --git a/bundle/direct/dresources/secret_scope.go b/bundle/direct/dresources/secret_scope.go index 3b3b89d22a0..548a0cd521c 100644 --- a/bundle/direct/dresources/secret_scope.go +++ b/bundle/direct/dresources/secret_scope.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/libs/utils" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/marshal" "github.com/databricks/databricks-sdk-go/service/workspace" @@ -61,18 +60,6 @@ func (*ResourceSecretScope) PrepareState(input *resources.SecretScope) *SecretSc } } -func (*ResourceSecretScope) RemapState(remote *SecretScopeRemote) *SecretScopeConfig { - return &SecretScopeConfig{ - CreateScope: workspace.CreateScope{ - Scope: remote.Scope, - ScopeBackendType: remote.ScopeBackendType, - BackendAzureKeyvault: remote.BackendAzureKeyvault, - InitialManagePrincipal: "", - ForceSendFields: utils.FilterFields[workspace.CreateScope](remote.ForceSendFields), - }, - } -} - // DoRead fetches the secret scope by name. Since the Secrets API does not provide // a "get by name" endpoint (see https://docs.databricks.com/api/workspace/secrets), // we must list all scopes and filter by name to check if the scope still exists. diff --git a/bundle/direct/dresources/sql_warehouse.go b/bundle/direct/dresources/sql_warehouse.go index 3844bd8416f..25c3aa16139 100644 --- a/bundle/direct/dresources/sql_warehouse.go +++ b/bundle/direct/dresources/sql_warehouse.go @@ -68,30 +68,6 @@ func (*ResourceSqlWarehouse) PrepareState(input *resources.SqlWarehouse) *SqlWar return s } -// RemapState maps the remote SqlWarehouseRemote to SqlWarehouseState for diff comparison. -// Started is derived from warehouse state so the planner can detect start/stop changes. -func (*ResourceSqlWarehouse) RemapState(warehouse *SqlWarehouseRemote) *SqlWarehouseState { - return &SqlWarehouseState{ - CreateWarehouseRequest: sql.CreateWarehouseRequest{ - AutoStopMins: warehouse.AutoStopMins, - Channel: warehouse.Channel, - ClusterSize: warehouse.ClusterSize, - CreatorName: warehouse.CreatorName, - EnablePhoton: warehouse.EnablePhoton, - EnableServerlessCompute: warehouse.EnableServerlessCompute, - InstanceProfileArn: warehouse.InstanceProfileArn, - MaxNumClusters: warehouse.MaxNumClusters, - MinNumClusters: warehouse.MinNumClusters, - Name: warehouse.Name, - SpotInstancePolicy: warehouse.SpotInstancePolicy, - Tags: warehouse.Tags, - WarehouseType: sql.CreateWarehouseRequestWarehouseType(warehouse.WarehouseType), - ForceSendFields: utils.FilterFields[sql.CreateWarehouseRequest](warehouse.ForceSendFields), - }, - Lifecycle: warehouse.Lifecycle, - } -} - // DoRead reads the warehouse by id. func (r *ResourceSqlWarehouse) DoRead(ctx context.Context, id string) (*SqlWarehouseRemote, error) { warehouse, err := r.client.Warehouses.GetById(ctx, id) From f1148bfc6e130babaa709873e1e79c95cd06870f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 23 Sep 2026 12:20:59 +0200 Subject: [PATCH 08/18] direct: delete remaining dumb RemapState methods (copier handles them) Removes RemapState for apps, jobs, job_runs, pipelines, the postgres_* resources and quality_monitors - all plain subset copies the auto-generated copier reproduces. Only model_serving_endpoints, vector_search_indexes and secrets keep a RemapState for now; they need their own prep before the copier can take them, after which this PR (rebased) will delete them and remove the RemapState hook. Co-authored-by: Isaac --- bundle/direct/dresources/app.go | 10 -------- bundle/direct/dresources/job.go | 4 ---- bundle/direct/dresources/job_run.go | 10 -------- bundle/direct/dresources/job_run_test.go | 11 +++++++-- bundle/direct/dresources/pipeline.go | 8 ------- bundle/direct/dresources/postgres_branch.go | 18 -------------- bundle/direct/dresources/postgres_catalog.go | 7 ------ bundle/direct/dresources/postgres_database.go | 13 ---------- bundle/direct/dresources/postgres_endpoint.go | 13 ---------- bundle/direct/dresources/postgres_project.go | 12 ---------- bundle/direct/dresources/postgres_role.go | 13 ---------- .../dresources/postgres_synced_table.go | 7 ------ bundle/direct/dresources/quality_monitor.go | 24 ------------------- 13 files changed, 9 insertions(+), 141 deletions(-) diff --git a/bundle/direct/dresources/app.go b/bundle/direct/dresources/app.go index 22cd7cc2b4e..46f21a36230 100644 --- a/bundle/direct/dresources/app.go +++ b/bundle/direct/dresources/app.go @@ -73,18 +73,8 @@ func (*ResourceApp) PrepareState(input *resources.App) *AppState { return s } -// RemapState maps the remote AppRemote to AppState for diff comparison. // DoRead populates config, git_source, and source_code_path from the active // deployment when one exists, enabling drift detection for out-of-band redeploys. -// Started is derived from compute status so the planner can detect start/stop changes. -func (*ResourceApp) RemapState(remote *AppRemote) *AppState { - return &AppState{ - App: remote.App, - Config: remote.Config, - Lifecycle: remote.Lifecycle, - } -} - func (r *ResourceApp) DoRead(ctx context.Context, id string) (*AppRemote, error) { app, err := r.client.Apps.GetByName(ctx, id) if err != nil { diff --git a/bundle/direct/dresources/job.go b/bundle/direct/dresources/job.go index ba2cd8848dd..2ec50c56fdb 100644 --- a/bundle/direct/dresources/job.go +++ b/bundle/direct/dresources/job.go @@ -52,10 +52,6 @@ func (*ResourceJob) PrepareState(input *resources.Job) *jobs.JobSettings { return &input.JobSettings } -func (*ResourceJob) RemapState(remote *JobRemote) *jobs.JobSettings { - return &remote.JobSettings -} - func getTaskKey(x jobs.Task) (string, string) { return "task_key", x.TaskKey } diff --git a/bundle/direct/dresources/job_run.go b/bundle/direct/dresources/job_run.go index 36031ec612e..fc20dc61141 100644 --- a/bundle/direct/dresources/job_run.go +++ b/bundle/direct/dresources/job_run.go @@ -176,16 +176,6 @@ func (r *ResourceJobRun) DoRead(ctx context.Context, id string) (*JobRunRemote, return makeJobRunRemote(run), nil } -// RemapState extracts the fields used for diffing: the RunNow request and the -// outcome the run reached. Lifecycle has no remote counterpart. -func (*ResourceJobRun) RemapState(remote *JobRunRemote) *JobRunState { - return &JobRunState{ - RunNow: remote.RunNow, - ResultState: remote.ResultState, - Lifecycle: nil, - } -} - func (r *ResourceJobRun) DoCreate(ctx context.Context, config *JobRunState) (string, *JobRunRemote, error) { // Mint a token so an SDK retry of a lost response returns the same run. Set it // on a copy: recording it in state would drift from the empty config and recreate. diff --git a/bundle/direct/dresources/job_run_test.go b/bundle/direct/dresources/job_run_test.go index 5da8feb39e0..e38cb4da21c 100644 --- a/bundle/direct/dresources/job_run_test.go +++ b/bundle/direct/dresources/job_run_test.go @@ -290,9 +290,14 @@ func TestJobRunPrepareStateCopiesResolvedTriggers(t *testing.T) { assert.Empty(t, triggers.OnBundleDeploy) } -// The planner diffs RemapState(remote) against PrepareState(config), so a run +// The planner diffs the remapped remote against PrepareState(config), so a run // that did not end in SUCCESS has to surface as a difference on result_state. +// job_runs has no RemapState method, so this exercises the auto-generated copier. func TestJobRunRemapStateCarriesTheOutcome(t *testing.T) { + adapters, err := InitAll(nil) + require.NoError(t, err) + adapter := adapters["job_runs"] + for _, outcome := range []jobs.RunResultState{ jobs.RunResultStateSuccess, jobs.RunResultStateFailed, @@ -302,7 +307,9 @@ func TestJobRunRemapStateCarriesTheOutcome(t *testing.T) { t.Run(string(outcome), func(t *testing.T) { remote := &JobRunRemote{RunId: 123, ResultState: outcome} - state := (&ResourceJobRun{}).RemapState(remote) + remapped, err := adapter.RemapState(remote) + require.NoError(t, err) + state := remapped.(*JobRunState) assert.Equal(t, outcome, state.ResultState) assert.Nil(t, state.Lifecycle) diff --git a/bundle/direct/dresources/pipeline.go b/bundle/direct/dresources/pipeline.go index aa9aae7ee33..520a1ffbb0c 100644 --- a/bundle/direct/dresources/pipeline.go +++ b/bundle/direct/dresources/pipeline.go @@ -81,14 +81,6 @@ func (*ResourcePipeline) PrepareState(input *resources.Pipeline) *PipelineState } } -func (*ResourcePipeline) RemapState(remote *PipelineRemote) *PipelineState { - return &PipelineState{ - CreatePipeline: remote.CreatePipeline, - // cascade_on_destroy is input-only and absent from PipelineRemote, so it stays nil here. - CascadeOnDestroy: nil, - } -} - func (r *ResourcePipeline) DoRead(ctx context.Context, id string) (*PipelineRemote, error) { resp, err := r.client.Pipelines.GetByPipelineId(ctx, id) if err != nil { diff --git a/bundle/direct/dresources/postgres_branch.go b/bundle/direct/dresources/postgres_branch.go index 57bba56707d..75fb4b85c6f 100644 --- a/bundle/direct/dresources/postgres_branch.go +++ b/bundle/direct/dresources/postgres_branch.go @@ -59,24 +59,6 @@ func (*ResourcePostgresBranch) PrepareState(input *resources.PostgresBranch) *Po } } -func (*ResourcePostgresBranch) RemapState(remote *PostgresBranchRemote) *PostgresBranchState { - return &PostgresBranchState{ - BranchId: remote.BranchId, - Parent: remote.Parent, - - // replace_existing is a create-time-only flag; the GET API never returns - // it, so RemapState leaves it false. - ReplaceExisting: false, - - // purge_on_delete is a delete-time query parameter; the GET API never - // returns it, so RemapState leaves it false. - PurgeOnDelete: false, - ForceSendFields: nil, - - BranchSpec: remote.BranchSpec, - } -} - // makePostgresBranchRemote converts the SDK Branch into the embedded remote shape. // GET does not echo spec today (only status is returned); the embedded spec fields // stay at their zero values, and postgres_branches.yml suppresses phantom drift via diff --git a/bundle/direct/dresources/postgres_catalog.go b/bundle/direct/dresources/postgres_catalog.go index 348ba88960b..c1e9e76470e 100644 --- a/bundle/direct/dresources/postgres_catalog.go +++ b/bundle/direct/dresources/postgres_catalog.go @@ -53,13 +53,6 @@ func (*ResourcePostgresCatalog) PrepareState(input *resources.PostgresCatalog) * } } -func (*ResourcePostgresCatalog) RemapState(remote *PostgresCatalogRemote) *PostgresCatalogState { - return &PostgresCatalogState{ - CatalogId: remote.CatalogId, - CatalogCatalogSpec: remote.CatalogCatalogSpec, - } -} - // makePostgresCatalogRemote converts the SDK Catalog into the embedded remote shape. // GET does not echo spec today (only status is returned); the embedded spec fields // stay at their zero values, and postgres_catalogs.yml suppresses phantom drift via diff --git a/bundle/direct/dresources/postgres_database.go b/bundle/direct/dresources/postgres_database.go index 613f5d90270..55ada8df1a9 100644 --- a/bundle/direct/dresources/postgres_database.go +++ b/bundle/direct/dresources/postgres_database.go @@ -56,19 +56,6 @@ func (*ResourcePostgresDatabase) PrepareState(input *resources.PostgresDatabase) } } -func (*ResourcePostgresDatabase) RemapState(remote *PostgresDatabaseRemote) *PostgresDatabaseState { - return &PostgresDatabaseState{ - DatabaseId: remote.DatabaseId, - Parent: remote.Parent, - - // replace_existing is a create-time-only flag; the GET API never returns - // it, so RemapState leaves it false. - ReplaceExisting: false, - - DatabaseDatabaseSpec: remote.DatabaseDatabaseSpec, - } -} - // makePostgresDatabaseRemote converts the SDK Database into the embedded remote // shape. GET does not echo spec today (only status is returned); the embedded // spec fields stay at their zero values, and postgres_databases.yml suppresses phantom diff --git a/bundle/direct/dresources/postgres_endpoint.go b/bundle/direct/dresources/postgres_endpoint.go index 786f52b8b21..a2756767763 100644 --- a/bundle/direct/dresources/postgres_endpoint.go +++ b/bundle/direct/dresources/postgres_endpoint.go @@ -65,19 +65,6 @@ func (*ResourcePostgresEndpoint) PrepareState(input *resources.PostgresEndpoint) } } -func (*ResourcePostgresEndpoint) RemapState(remote *PostgresEndpointRemote) *PostgresEndpointState { - return &PostgresEndpointState{ - EndpointId: remote.EndpointId, - Parent: remote.Parent, - - // replace_existing is a create-time-only flag; the GET API never returns - // it, so RemapState leaves it false. - ReplaceExisting: false, - - EndpointSpec: remote.EndpointSpec, - } -} - // makePostgresEndpointRemote converts the SDK Endpoint into the embedded remote shape. // GET does not echo spec today (only status is returned); the embedded spec fields // stay at their zero values, and postgres_endpoints.yml suppresses phantom drift via diff --git a/bundle/direct/dresources/postgres_project.go b/bundle/direct/dresources/postgres_project.go index bdb886e9437..eb9cff1e412 100644 --- a/bundle/direct/dresources/postgres_project.go +++ b/bundle/direct/dresources/postgres_project.go @@ -59,18 +59,6 @@ func (*ResourcePostgresProject) PrepareState(input *resources.PostgresProject) * } } -func (*ResourcePostgresProject) RemapState(remote *PostgresProjectRemote) *PostgresProjectState { - return &PostgresProjectState{ - ProjectId: remote.ProjectId, - ProjectSpec: remote.ProjectSpec, - - // purge_on_delete is a delete-time query parameter; the GET API never - // returns it, so RemapState leaves it false. - PurgeOnDelete: false, - ForceSendFields: nil, - } -} - // makePostgresProjectRemote converts the SDK Project into the embedded remote shape. // GET does not echo spec today (only status is returned); the embedded spec fields // stay at their zero values, and postgres_projects.yml suppresses phantom drift via diff --git a/bundle/direct/dresources/postgres_role.go b/bundle/direct/dresources/postgres_role.go index 10ef250e37c..5fa963825f1 100644 --- a/bundle/direct/dresources/postgres_role.go +++ b/bundle/direct/dresources/postgres_role.go @@ -81,19 +81,6 @@ func (*ResourcePostgresRole) PrepareState(input *resources.PostgresRole) *Postgr } } -func (*ResourcePostgresRole) RemapState(remote *PostgresRoleRemote) *PostgresRoleState { - return &PostgresRoleState{ - RoleId: remote.RoleId, - Parent: remote.Parent, - - // replace_existing is a create-time-only flag; the GET API never returns - // it, so RemapState leaves it false. - ReplaceExisting: false, - - RoleRoleSpec: remote.RoleRoleSpec, - } -} - // makePostgresRoleRemote converts the SDK Role into the embedded remote shape. // GET does not echo spec today (only status is returned); the embedded spec fields // stay at their zero values, and postgres_roles.yml suppresses phantom drift via diff --git a/bundle/direct/dresources/postgres_synced_table.go b/bundle/direct/dresources/postgres_synced_table.go index f194df54e61..f5c8165df2e 100644 --- a/bundle/direct/dresources/postgres_synced_table.go +++ b/bundle/direct/dresources/postgres_synced_table.go @@ -60,13 +60,6 @@ func (*ResourcePostgresSyncedTable) PrepareState(input *resources.PostgresSynced } } -func (*ResourcePostgresSyncedTable) RemapState(remote *PostgresSyncedTableRemote) *PostgresSyncedTableState { - return &PostgresSyncedTableState{ - SyncedTableId: remote.SyncedTableId, - SyncedTableSyncedTableSpec: remote.SyncedTableSyncedTableSpec, - } -} - // makePostgresSyncedTableRemote converts the SDK SyncedTable into the embedded // remote shape. GET does not echo spec today (only status is returned); the // embedded spec fields stay at their zero values, and postgres_synced_tables.yml suppresses diff --git a/bundle/direct/dresources/quality_monitor.go b/bundle/direct/dresources/quality_monitor.go index c66fed4e0bb..5006f526c72 100644 --- a/bundle/direct/dresources/quality_monitor.go +++ b/bundle/direct/dresources/quality_monitor.go @@ -41,30 +41,6 @@ func (*ResourceQualityMonitor) PrepareState(input *resources.QualityMonitor) *Qu } } -func (*ResourceQualityMonitor) RemapState(info *catalog.MonitorInfo) *QualityMonitorState { - return &QualityMonitorState{ - CreateMonitor: catalog.CreateMonitor{ - AssetsDir: info.AssetsDir, - BaselineTableName: info.BaselineTableName, - CustomMetrics: info.CustomMetrics, - DataClassificationConfig: info.DataClassificationConfig, - InferenceLog: info.InferenceLog, - LatestMonitorFailureMsg: info.LatestMonitorFailureMsg, - Notifications: info.Notifications, - OutputSchemaName: info.OutputSchemaName, - Schedule: info.Schedule, - SkipBuiltinDashboard: false, - SlicingExprs: info.SlicingExprs, - Snapshot: info.Snapshot, - TableName: info.TableName, - TimeSeries: info.TimeSeries, - WarehouseId: "", - ForceSendFields: utils.FilterFields[catalog.CreateMonitor](info.ForceSendFields), - }, - TableName: info.TableName, - } -} - func (r *ResourceQualityMonitor) DoRead(ctx context.Context, id string) (*catalog.MonitorInfo, error) { //nolint:staticcheck // Direct quality_monitor resource still uses legacy monitor endpoints; v1 data-quality migration is separate work. return r.client.QualityMonitors.Get(ctx, catalog.GetQualityMonitorRequest{ From 084d1de98a7e3c3063beced253bac0dc20d2f724 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 1 Oct 2026 15:46:31 +0200 Subject: [PATCH 09/18] direct: delete model_serving_endpoints RemapState (copier reproduces it) #6812 surfaced ForceSendFields at the ModelServingEndpointRemote root, making its RemapState a plain copy. The auto-copier now reproduces it exactly (TestNoRedundantRemapState), so remove the method and let buildCopiers handle it. Co-authored-by: Isaac --- .../dresources/model_serving_endpoint.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/bundle/direct/dresources/model_serving_endpoint.go b/bundle/direct/dresources/model_serving_endpoint.go index f9c0a2314ac..b1dff6c3e89 100644 --- a/bundle/direct/dresources/model_serving_endpoint.go +++ b/bundle/direct/dresources/model_serving_endpoint.go @@ -93,25 +93,6 @@ func configOutputToInput(output *serving.EndpointCoreConfigOutput) *serving.Endp } } -func (*ResourceModelServingEndpoint) RemapState(state *ModelServingEndpointRemote) *serving.CreateServingEndpoint { - return &serving.CreateServingEndpoint{ - AiGateway: state.AiGateway, - BudgetPolicyId: state.BudgetPolicyId, - Config: state.Config, - Description: state.Description, - EmailNotifications: state.EmailNotifications, - Name: state.Name, - RouteOptimized: state.RouteOptimized, - Tags: state.Tags, - TelemetryConfig: state.TelemetryConfig, - ForceSendFields: utils.FilterFields[serving.CreateServingEndpoint](state.ForceSendFields), - - // Rate limits are a deprecated field that are not returned by the API on GET calls. Thus we map them to nil. - // TODO(shreyas): Add a warning when users try setting top level rate limits. - RateLimits: nil, - } -} - type ModelServingEndpointRemote struct { EndpointDetails *serving.ServingEndpointDetailed `json:"endpoint_details"` EndpointId string `json:"endpoint_id"` From 0da2c0803ab12cfd5772a6ada1174facf0f945b9 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 2 Oct 2026 11:37:29 +0200 Subject: [PATCH 10/18] =?UTF-8?q?structcopy:=20address=20review=20?= =?UTF-8?q?=E2=80=94=20correct=20ForceSendFields=20sourcing,=20reject=20un?= =?UTF-8?q?supported=20shapes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (gpt-6-astra + code-reviewer) of the copier: - Copy sourced every destination ForceSendFields from the single source root, so a multi-FSF type (postgres: root shadow + embedded spec) could fill the wrong slice. Now each FSF slice is sourced from the matching source slice (root<-root, embed<-same owner type). Harmless today (remote spec FSF is nil) but no longer latent-divergent. - Compile rejects anonymous pointer embeds (flattenStruct would panic on NumField) and JSON-name collisions (silent last-wins), consistent with the fail-at-load contract. - safeConvert restricted to scalar kinds; struct conversions ignore JSON tags and could silently drop or rename fields. - buildCopiers dedup records every impl, so permissions/grants aren't reprocessed per key. - copier_test setForceSendFields sets only the directly-declared slice, so a wrapper's embedded SDK FSF is genuinely exercised rather than clobbered. No current resource triggers the new guards or changes output (generate-check, vet, and acceptance for postgres/apps/mse/secret_scopes unchanged). Co-authored-by: Isaac --- bundle/direct/dresources/all.go | 4 +- bundle/direct/dresources/copier_test.go | 16 +++- libs/structs/structcopy/structcopy.go | 113 ++++++++++++++++++------ 3 files changed, 100 insertions(+), 33 deletions(-) diff --git a/bundle/direct/dresources/all.go b/bundle/direct/dresources/all.go index e6ebade129a..dc3d709ac64 100644 --- a/bundle/direct/dresources/all.go +++ b/bundle/direct/dresources/all.go @@ -97,13 +97,15 @@ var copiers = buildCopiers() func buildCopiers() map[reflect.Type]*structcopy.Copier { iface := reflect.TypeFor[IResource]() out := make(map[reflect.Type]*structcopy.Copier) + seen := make(map[reflect.Type]bool) var errs []string for resourceType, resource := range SupportedResources { implType := reflect.TypeOf(resource) - if _, done := out[implType]; done { + if seen[implType] { continue // same implementation registered under several keys (permissions, grants) } + seen[implType] = true remap, err := calladapt.PrepareCall(resource, iface, "RemapState") if err != nil { diff --git a/bundle/direct/dresources/copier_test.go b/bundle/direct/dresources/copier_test.go index f25cc9cd153..09fc4859b44 100644 --- a/bundle/direct/dresources/copier_test.go +++ b/bundle/direct/dresources/copier_test.go @@ -71,16 +71,26 @@ func setForceSendFields(v reflect.Value) { } case reflect.Struct: var names []string + fsfIndex := -1 for i := range v.NumField() { sf := v.Type().Field(i) - if !sf.IsExported() || sf.Name == "ForceSendFields" { + if !sf.IsExported() { + continue + } + if sf.Name == "ForceSendFields" { + fsfIndex = i // this struct's own slice, not one promoted from an embed continue } names = append(names, sf.Name) setForceSendFields(v.Field(i)) } - if f := v.FieldByName("ForceSendFields"); f.IsValid() && f.Kind() == reflect.Slice && f.Type().Elem().Kind() == reflect.String { - f.Set(reflect.ValueOf(names)) + // Set only the directly declared ForceSendFields. v.FieldByName would follow Go + // promotion and clobber an embedded SDK struct's own slice with the wrapper's field + // names, so a wrapper like AppRemote never genuinely exercises the embed's FSF path. + if fsfIndex >= 0 { + if f := v.Field(fsfIndex); f.Kind() == reflect.Slice && f.Type().Elem().Kind() == reflect.String { + f.Set(reflect.ValueOf(names)) + } } default: // scalars and other kinds have no nested ForceSendFields to set diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 6965e44cc7f..b490d360acb 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -9,7 +9,6 @@ package structcopy import ( "fmt" - "maps" "reflect" "github.com/databricks/cli/libs/structs/structaccess" @@ -36,7 +35,8 @@ type copyOp struct { dstType reflect.Type // target type for Convert // forceSendFields ops: - ownerType reflect.Type // struct type owning the ForceSendFields slice, for validity filtering + ownerType reflect.Type // struct type owning the destination ForceSendFields slice, for validity filtering + srcForceSendIndex []int // FieldByIndex path to the matching source ForceSendFields slice, nil if the source has none } // jsonFieldInfo locates a top-level JSON field within a struct, resolved through @@ -55,14 +55,21 @@ type forceSendTarget struct { // Compile builds the copy plan for srcType -> dstType (both pointers to structs). It returns // an error if a destination field that also exists on the source cannot be copied without a -// value-changing conversion, so callers can reject such a type pair up front rather than -// silently dropping the field. +// value-changing conversion, or if either type has a shape the copier does not support +// (an anonymous pointer embed, or a JSON-name collision), so callers reject such a type pair +// up front rather than silently dropping a field or panicking at copy time. func Compile(srcType, dstType reflect.Type) (*Copier, error) { dstElem := dstType.Elem() srcElem := srcType.Elem() - dstFields, dstForceSend := flattenStruct(dstElem, nil) - srcFields, _ := flattenStruct(srcElem, nil) + dstFields, dstForceSend, err := flattenStruct(dstElem, nil) + if err != nil { + return nil, fmt.Errorf("destination %s: %w", dstElem, err) + } + srcFields, srcForceSend, err := flattenStruct(srcElem, nil) + if err != nil { + return nil, fmt.Errorf("source %s: %w", srcElem, err) + } var ops []copyOp for name, dst := range dstFields { @@ -73,16 +80,33 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { } switch { case dst.typ == src.typ: - ops = append(ops, copyOp{forceSendFields: false, dstIndex: dst.index, srcIndex: src.index, convert: false, dstType: dst.typ, ownerType: nil}) + ops = append(ops, copyOp{dstIndex: dst.index, srcIndex: src.index, dstType: dst.typ}) case safeConvert(dst.typ, src.typ): - ops = append(ops, copyOp{forceSendFields: false, dstIndex: dst.index, srcIndex: src.index, convert: true, dstType: dst.typ, ownerType: nil}) + ops = append(ops, copyOp{dstIndex: dst.index, srcIndex: src.index, convert: true, dstType: dst.typ}) default: return nil, fmt.Errorf("field %q: destination type %s is not assignable or safely convertible from source type %s", name, dst.typ, src.typ) } } + // Match each destination ForceSendFields slice to the source slice that governs the same + // fields, so a slice is never populated from an unrelated struct's names. The top-level + // slice (index length 1) pairs with the source's top-level slice; a slice owned by an + // embedded struct pairs with the source slice owned by that same struct type. An + // unmatched destination slice is left empty. + var srcRootForceSend []int + srcForceSendByOwner := make(map[reflect.Type][]int, len(srcForceSend)) + for _, s := range srcForceSend { + if len(s.index) == 1 { + srcRootForceSend = s.index + } + srcForceSendByOwner[s.ownerType] = s.index + } for _, target := range dstForceSend { - ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, srcIndex: nil, convert: false, dstType: nil, ownerType: target.ownerType}) + srcIdx := srcForceSendByOwner[target.ownerType] + if len(target.index) == 1 { + srcIdx = srcRootForceSend + } + ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType, srcForceSendIndex: srcIdx}) } return &Copier{dstElem: dstElem, ops: ops}, nil @@ -95,11 +119,11 @@ func (c *Copier) Copy(src any) any { dstPtr := reflect.New(c.dstElem) dstVal := dstPtr.Elem() - var srcForceSend []string for _, op := range c.ops { if op.forceSendFields { - if srcForceSend == nil { - srcForceSend = rootForceSendFields(srcVal) + var srcForceSend []string + if op.srcForceSendIndex != nil { + srcForceSend, _ = reflect.TypeAssert[[]string](srcVal.FieldByIndex(op.srcForceSendIndex)) } filtered := utils.FilterFieldsType(op.ownerType, srcForceSend) dstVal.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) @@ -116,31 +140,50 @@ func (c *Copier) Copy(src any) any { } // safeConvert reports whether a value of src can be converted to dst without changing -// the value. Distinct named types with the same underlying type (e.g. two string enums) -// qualify; kind-changing conversions (int<->string, float->int, []byte<->string) do not, -// because reflect.Convert would silently corrupt the value. +// the value or its JSON meaning. Distinct named scalar types with the same underlying +// type (e.g. two string enums) qualify. Kind-changing conversions (int<->string, +// float->int, []byte<->string) do not, because reflect.Convert would silently corrupt +// the value. Composite kinds (struct/map/slice/array) are also excluded: Go permits +// converting between two structs with identical fields even when their JSON tags differ, +// which would silently drop or rename fields on marshal; such fields must match exactly +// (handled by the dst.typ == src.typ branch) or the resource keeps a custom RemapState. func safeConvert(dst, src reflect.Type) bool { - return src.ConvertibleTo(dst) && src.Kind() == dst.Kind() + return isScalarKind(src.Kind()) && src.ConvertibleTo(dst) && src.Kind() == dst.Kind() } -// rootForceSendFields returns the ForceSendFields slice at the struct's root, or nil. -func rootForceSendFields(v reflect.Value) []string { - f := v.FieldByName("ForceSendFields") - if !f.IsValid() || f.Kind() != reflect.Slice { - return nil +// isScalarKind reports whether k is a non-composite kind (bool, numeric, or string), +// for which a same-kind reflect.Convert is value- and JSON-preserving. +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 } - fields, _ := reflect.TypeAssert[[]string](f) - return fields } // flattenStruct enumerates a struct's top-level JSON fields (descending through // encoding/json-flattened embeds and accumulating the field-index prefix) plus every // ForceSendFields slice reachable through those embeds. Names follow the same JSON tag -// resolution the diff engine uses, so copied values compare consistently. -func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []forceSendTarget) { +// resolution the diff engine uses, so copied values compare consistently. It returns an +// error for shapes the copier does not model: an anonymous pointer embed (FieldByIndex +// would panic on a nil pointer at copy time) or two fields resolving to the same JSON name +// (encoding/json treats that as ambiguous; the copier would otherwise pick one silently). +func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []forceSendTarget, error) { fields := make(map[string]jsonFieldInfo) var forceSend []forceSendTarget + add := func(name string, info jsonFieldInfo) error { + if _, dup := fields[name]; dup { + return fmt.Errorf("json name %q is declared by more than one field", name) + } + fields[name] = info + return nil + } + for i := range t.NumField() { sf := t.Field(i) if sf.PkgPath != "" { @@ -153,8 +196,18 @@ func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []fo continue } if structaccess.IsFlattenedEmbed(sf) { - nested, nestedForceSend := flattenStruct(sf.Type, index) - maps.Copy(fields, nested) + if sf.Type.Kind() == reflect.Pointer { + return nil, nil, fmt.Errorf("anonymous pointer embed %s is not supported", sf.Type) + } + nested, nestedForceSend, err := flattenStruct(sf.Type, index) + if err != nil { + return nil, nil, err + } + for name, info := range nested { + if err := add(name, info); err != nil { + return nil, nil, err + } + } forceSend = append(forceSend, nestedForceSend...) continue } @@ -166,8 +219,10 @@ func flattenStruct(t reflect.Type, prefix []int) (map[string]jsonFieldInfo, []fo if name == "" { name = sf.Name } - fields[name] = jsonFieldInfo{index: index, typ: sf.Type} + if err := add(name, jsonFieldInfo{index: index, typ: sf.Type}); err != nil { + return nil, nil, err + } } - return fields, forceSend + return fields, forceSend, nil } From 6e97eea3b8e3d704b5d053478d8d7badd7ce3c08 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 2 Oct 2026 11:50:59 +0200 Subject: [PATCH 11/18] structcopy: source ForceSendFields from the root, filter by directly-declared fields Re-review (gpt-6-astra) of the first review fix: matching each destination ForceSendFields slice to a source slice by identical owner type was wrong twice. It dropped a forced TimeoutSeconds when the source embeds the same settings type (JobRemote -> JobSettings), and dropped markers like AutoStopMins when source and state use different SDK types (GetWarehouseResponse -> CreateWarehouseRequest; clusters and quality monitors too). Keep the pre-review single source-root slice but filter it to each destination owner's directly-declared field names (not FieldByName, which follows promotion). That is what the earlier review flagged for postgres: a wrapper re-declaring ForceSendFields above an embedded spec must not inherit the embed's promoted names. Direct-field filtering handles the shadow (excludes promoted) and the embedded spec (keeps its own) from one root slice, so every case is correct without owner matching. No current resource changes output (acceptance for sql_warehouses/clusters/jobs/ quality_monitors/postgres/apps/mse/secret_scopes unchanged). Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 65 ++++++++++++++++----------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index b490d360acb..3a1530f87d2 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -13,7 +13,6 @@ import ( "github.com/databricks/cli/libs/structs/structaccess" "github.com/databricks/cli/libs/structs/structtag" - "github.com/databricks/cli/libs/utils" ) // Copier copies fields from a source struct into a fresh destination struct by matching @@ -35,8 +34,7 @@ type copyOp struct { dstType reflect.Type // target type for Convert // forceSendFields ops: - ownerType reflect.Type // struct type owning the destination ForceSendFields slice, for validity filtering - srcForceSendIndex []int // FieldByIndex path to the matching source ForceSendFields slice, nil if the source has none + ownerType reflect.Type // struct type owning the ForceSendFields slice, for validity filtering } // jsonFieldInfo locates a top-level JSON field within a struct, resolved through @@ -66,7 +64,7 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { if err != nil { return nil, fmt.Errorf("destination %s: %w", dstElem, err) } - srcFields, srcForceSend, err := flattenStruct(srcElem, nil) + srcFields, _, err := flattenStruct(srcElem, nil) if err != nil { return nil, fmt.Errorf("source %s: %w", srcElem, err) } @@ -88,25 +86,8 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { } } - // Match each destination ForceSendFields slice to the source slice that governs the same - // fields, so a slice is never populated from an unrelated struct's names. The top-level - // slice (index length 1) pairs with the source's top-level slice; a slice owned by an - // embedded struct pairs with the source slice owned by that same struct type. An - // unmatched destination slice is left empty. - var srcRootForceSend []int - srcForceSendByOwner := make(map[reflect.Type][]int, len(srcForceSend)) - for _, s := range srcForceSend { - if len(s.index) == 1 { - srcRootForceSend = s.index - } - srcForceSendByOwner[s.ownerType] = s.index - } for _, target := range dstForceSend { - srcIdx := srcForceSendByOwner[target.ownerType] - if len(target.index) == 1 { - srcIdx = srcRootForceSend - } - ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType, srcForceSendIndex: srcIdx}) + ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType}) } return &Copier{dstElem: dstElem, ops: ops}, nil @@ -119,13 +100,18 @@ func (c *Copier) Copy(src any) any { dstPtr := reflect.New(c.dstElem) dstVal := dstPtr.Elem() + var srcForceSend []string for _, op := range c.ops { if op.forceSendFields { - var srcForceSend []string - if op.srcForceSendIndex != nil { - srcForceSend, _ = reflect.TypeAssert[[]string](srcVal.FieldByIndex(op.srcForceSendIndex)) + if srcForceSend == nil { + srcForceSend = rootForceSendFields(srcVal) } - filtered := utils.FilterFieldsType(op.ownerType, srcForceSend) + // Keep only names that are directly declared on this slice's owner. The source + // marker names are field names; a destination ForceSendFields slice may only list + // its own struct's fields. Filtering by directly-declared (not promoted) fields is + // what keeps a shadow slice — e.g. a wrapper that re-declares ForceSendFields above + // an embedded spec — from being populated with the embed's promoted field names. + filtered := filterOwnFields(op.ownerType, srcForceSend) dstVal.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) continue } @@ -165,6 +151,33 @@ func isScalarKind(k reflect.Kind) bool { } } +// rootForceSendFields returns the ForceSendFields slice at the struct's root, or nil. +func rootForceSendFields(v reflect.Value) []string { + f := v.FieldByName("ForceSendFields") + if !f.IsValid() || f.Kind() != reflect.Slice { + return nil + } + fields, _ := f.Interface().([]string) + return fields +} + +// filterOwnFields returns the names that name a field directly declared on ownerType. +// Unlike reflect.Type.FieldByName it does not follow promotion, so a promoted field of an +// embedded struct is not treated as belonging to the outer struct. +func filterOwnFields(ownerType reflect.Type, names []string) []string { + own := make(map[string]bool, ownerType.NumField()) + for i := range ownerType.NumField() { + own[ownerType.Field(i).Name] = true + } + var result []string + for _, name := range names { + if own[name] { + result = append(result, name) + } + } + return result +} + // flattenStruct enumerates a struct's top-level JSON fields (descending through // encoding/json-flattened embeds and accumulating the field-index prefix) plus every // ForceSendFields slice reachable through those embeds. Names follow the same JSON tag From 08de9994797ec233729a91295fe8a037759f6c78 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 2 Oct 2026 12:01:14 +0200 Subject: [PATCH 12/18] structcopy: reject sources with multiple ForceSendFields slices; test embedded case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review (gpt-6-astra): the single-root model (rootForceSendFields, via FieldByName) faithfully represents at most one source ForceSendFields slice. A source with both a directly-declared root slice and separate embedded slices would surface only the shallowest and silently drop the rest. No current resource has this shape, but a future one would diverge silently, so Compile now rejects it at load — matching the existing fail-at-load guards for anonymous pointer embeds and JSON-name collisions. Adds TestCopyEmbeddedForceSendFields (a wrapper's shadow slice stays empty while an embedded spec's slice keeps its own markers) and TestCompileRejectsMultipleSourceForceSend. Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 9 +++- libs/structs/structcopy/structcopy_test.go | 50 ++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 3a1530f87d2..609ab90537d 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -64,10 +64,17 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { if err != nil { return nil, fmt.Errorf("destination %s: %w", dstElem, err) } - srcFields, _, err := flattenStruct(srcElem, nil) + srcFields, srcForceSend, err := flattenStruct(srcElem, nil) if err != nil { return nil, fmt.Errorf("source %s: %w", srcElem, err) } + // Copy sources every destination ForceSendFields slice from a single root slice + // (rootForceSendFields, via FieldByName). That faithfully represents at most one source + // slice: with two or more, FieldByName surfaces only the shallowest and the rest would be + // silently dropped. Reject such a source up front rather than lose markers at copy time. + if len(srcForceSend) > 1 { + return nil, fmt.Errorf("source %s has %d ForceSendFields slices; the copier can source destination ForceSendFields from only one (implement RemapState for this resource)", srcElem, len(srcForceSend)) + } var ops []copyOp for name, dst := range dstFields { diff --git a/libs/structs/structcopy/structcopy_test.go b/libs/structs/structcopy/structcopy_test.go index 44cdc856616..0923e6b4514 100644 --- a/libs/structs/structcopy/structcopy_test.go +++ b/libs/structs/structcopy/structcopy_test.go @@ -58,6 +58,56 @@ func TestCompileRejectsUnsafe(t *testing.T) { assert.Contains(t, err.Error(), "not assignable or safely convertible") } +// SpecEmbed models an SDK struct (exported, as all SDK types are) that is embedded by both +// the remote and state wrappers and carries its own ForceSendFields. +type SpecEmbed struct { + Protected bool `json:"protected"` + ForceSendFields []string +} + +// embedSource embeds SpecEmbed, so its only ForceSendFields slice is the promoted one. +type embedSource struct { + SpecEmbed + Id string `json:"id"` +} + +// embedDest re-declares a root ForceSendFields above the embedded SpecEmbed, mirroring a +// state type whose wrapper shadows an embedded spec's slice (e.g. the postgres resources). +type embedDest struct { + SpecEmbed + Id string `json:"id"` + ForceSendFields []string +} + +// TestCopyEmbeddedForceSendFields pins the rule that a wrapper's own (shadow) ForceSendFields +// slice is populated only from the source's directly-declared markers, while an embedded +// struct's slice keeps its own: the promoted "Protected" marker must land on the embedded +// slice, not leak up into the wrapper's shadow slice. +func TestCopyEmbeddedForceSendFields(t *testing.T) { + copier, err := Compile(reflect.TypeFor[*embedSource](), reflect.TypeFor[*embedDest]()) + require.NoError(t, err) + + src := &embedSource{SpecEmbed: SpecEmbed{ForceSendFields: []string{"Protected"}}, Id: "x"} + got := copier.Copy(src).(*embedDest) + + assert.Empty(t, got.ForceSendFields) // shadow slice: no promoted names + assert.Equal(t, []string{"Protected"}, got.SpecEmbed.ForceSendFields) // embedded slice keeps its own +} + +// multiFSFSource has both a promoted (embedded) and a directly-declared root ForceSendFields, +// a shape the single-root copier cannot represent. +type multiFSFSource struct { + SpecEmbed + Id string `json:"id"` + ForceSendFields []string +} + +func TestCompileRejectsMultipleSourceForceSend(t *testing.T) { + _, err := Compile(reflect.TypeFor[*multiFSFSource](), reflect.TypeFor[*embedDest]()) + require.Error(t, err) + assert.Contains(t, err.Error(), "ForceSendFields slices") +} + func TestSafeConvert(t *testing.T) { tests := []struct { name string From 5f35fa2c3e01608d68e0bae2fbeb404db648c844 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 2 Oct 2026 12:10:08 +0200 Subject: [PATCH 13/18] structcopy: read the source ForceSendFields slice by its compiled index Fourth review (gpt-6-astra): rootForceSendFields used FieldByName, which traverses a json-tagged anonymous embed that flattenStruct treats as a plain field, so the guard's slice count could disagree with the runtime lookup (and an ambiguous FieldByName returns nothing, dropping a marker). Resolve the single slice from flattenStruct at compile time and read it by index in Copy, so compile and copy use one resolution. Not reachable by any current resource; the invariant no_drift suite stays green. Also document that markers match by Go field name: a field mapped across a differing Go name under the same JSON name is not force-sent on the destination, matching the SDK utils.FilterFields behavior the hand-written copies used. Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 42 +++++++++++++++------------ 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 609ab90537d..77338329244 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -20,6 +20,13 @@ import ( type Copier struct { dstElem reflect.Type // struct type behind the destination pointer type ops []copyOp + + // srcForceSendIndex is the FieldByIndex path to the source's single ForceSendFields + // slice, or nil if the source has none. Compile resolves it from flattenStruct (which + // Compile also uses to enforce the single-slice guard), so the read in Copy follows the + // exact same resolution — unlike FieldByName, which would also traverse a json-tagged + // anonymous embed that flattenStruct treats as a plain field. + srcForceSendIndex []int } type copyOp struct { @@ -68,13 +75,17 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { if err != nil { return nil, fmt.Errorf("source %s: %w", srcElem, err) } - // Copy sources every destination ForceSendFields slice from a single root slice - // (rootForceSendFields, via FieldByName). That faithfully represents at most one source - // slice: with two or more, FieldByName surfaces only the shallowest and the rest would be - // silently dropped. Reject such a source up front rather than lose markers at copy time. + // Copy sources every destination ForceSendFields slice from one source slice. That + // faithfully represents at most one source slice: with two or more there is no single + // slice to read, and markers from all but one would be silently dropped. Reject such a + // source up front rather than lose markers at copy time. if len(srcForceSend) > 1 { return nil, fmt.Errorf("source %s has %d ForceSendFields slices; the copier can source destination ForceSendFields from only one (implement RemapState for this resource)", srcElem, len(srcForceSend)) } + var srcForceSendIndex []int + if len(srcForceSend) == 1 { + srcForceSendIndex = srcForceSend[0].index + } var ops []copyOp for name, dst := range dstFields { @@ -97,7 +108,7 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType}) } - return &Copier{dstElem: dstElem, ops: ops}, nil + return &Copier{dstElem: dstElem, ops: ops, srcForceSendIndex: srcForceSendIndex}, nil } // Copy builds a fresh destination value (a pointer to the destination struct) populated @@ -108,11 +119,11 @@ func (c *Copier) Copy(src any) any { dstVal := dstPtr.Elem() var srcForceSend []string + if c.srcForceSendIndex != nil { + srcForceSend, _ = srcVal.FieldByIndex(c.srcForceSendIndex).Interface().([]string) + } for _, op := range c.ops { if op.forceSendFields { - if srcForceSend == nil { - srcForceSend = rootForceSendFields(srcVal) - } // Keep only names that are directly declared on this slice's owner. The source // marker names are field names; a destination ForceSendFields slice may only list // its own struct's fields. Filtering by directly-declared (not promoted) fields is @@ -158,19 +169,12 @@ func isScalarKind(k reflect.Kind) bool { } } -// rootForceSendFields returns the ForceSendFields slice at the struct's root, or nil. -func rootForceSendFields(v reflect.Value) []string { - f := v.FieldByName("ForceSendFields") - if !f.IsValid() || f.Kind() != reflect.Slice { - return nil - } - fields, _ := f.Interface().([]string) - return fields -} - // filterOwnFields returns the names that name a field directly declared on ownerType. // Unlike reflect.Type.FieldByName it does not follow promotion, so a promoted field of an -// embedded struct is not treated as belonging to the outer struct. +// embedded struct is not treated as belonging to the outer struct. Marker names are matched +// by Go field name (the ForceSendFields convention), so a field the copier maps across a +// differing Go name under the same JSON name is not force-sent on the destination — this +// matches the SDK's own utils.FilterFields behavior that the hand-written copies used. func filterOwnFields(ownerType reflect.Type, names []string) []string { own := make(map[string]bool, ownerType.NumField()) for i := range ownerType.NumField() { From bed791a753fcf3a5709923fe5be5f14e70a4ab79 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 2 Oct 2026 12:41:37 +0200 Subject: [PATCH 14/18] structcopy: satisfy modernize lint (reflect.TypeAssert, Type.Fields) Use reflect.TypeAssert and range-over-Type.Fields, matching the modernize linter and the rest of the repo (go 1.26). Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 77338329244..487df306f93 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -120,7 +120,7 @@ func (c *Copier) Copy(src any) any { var srcForceSend []string if c.srcForceSendIndex != nil { - srcForceSend, _ = srcVal.FieldByIndex(c.srcForceSendIndex).Interface().([]string) + srcForceSend, _ = reflect.TypeAssert[[]string](srcVal.FieldByIndex(c.srcForceSendIndex)) } for _, op := range c.ops { if op.forceSendFields { @@ -177,8 +177,8 @@ func isScalarKind(k reflect.Kind) bool { // matches the SDK's own utils.FilterFields behavior that the hand-written copies used. func filterOwnFields(ownerType reflect.Type, names []string) []string { own := make(map[string]bool, ownerType.NumField()) - for i := range ownerType.NumField() { - own[ownerType.Field(i).Name] = true + for field := range ownerType.Fields() { + own[field.Name] = true } var result []string for _, name := range names { From 3fbd69d65a556522009a73c8d8c69e19a0e4ad0d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 5 Oct 2026 16:00:20 +0200 Subject: [PATCH 15/18] dresources: trim RemapState README section; fix filterOwnFields comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the long RemapState/plan-classifier explanation with the actionable rule: RemoteType should be a superset of StateType and all remapping belongs in DoRead. Correct the filterOwnFields comment, which wrongly claimed it matches utils.FilterFields — FilterFields uses FieldByName and follows promotion, which is exactly what filterOwnFields avoids so a wrapper's shadow ForceSendFields does not inherit an embedded spec's field names. Co-authored-by: Isaac --- bundle/direct/dresources/README.md | 46 ++++++--------------------- libs/structs/structcopy/structcopy.go | 8 ++--- 2 files changed, 14 insertions(+), 40 deletions(-) diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index 02710ffd004..b9133f2f3dd 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -73,42 +73,16 @@ on field behaviour might result in recreate. See dstate/migrate.go on how to han Declaring a field under `hashed_fields` in `resources.yml` makes the engine persist only a `sha256:` content hash of it in state (via `CompactState`) rather than the full contents, for large equality-only fields that are never read back from state (e.g. `dashboards.serialized_dashboard`). -## RemapState is a dumb copy; DoRead owns all remapping - -New resources should not define `RemapState`. When `StateType` is a subset of `RemoteType`, -the framework copies it automatically: `buildCopiers` (in `all.go`) compiles a copier per -resource at package init, matching fields by JSON name, filtering `ForceSendFields`, and -applying only lossless conversions. Anything it cannot copy safely (a kind-changing conversion, -a mismatched struct shape) fails at load rather than as silent drift. Add a `RemapState` method -only for logic the copier cannot express — derived fields, renames, per-field `ForceSendFields` -rules — and the rest of this section governs those overrides. - -Such a `RemapState` must still be a field-by-field copy (or no-op), never a place for arbitrary -logic. In particular, do not remap a differently-named field there (e.g. `state.x = remote.status.x`). -Any remapping the API requires belongs in `DoRead`: add `x` directly to `RemoteType` and -populate it from `status.x` inside `DoRead`. - -This is not a style preference — the plan classifier depends on it. Because `RemapState` is a -dumb copy, a field absent from `RemoteType` is always nil/zero in the remapped remote state the -planner compares against: there is nothing to copy it from. So "path present in `StateType` but -absent from `RemoteType`" reliably means "field the API accepts on write but never echoes on -read," and its remote value is guaranteed meaningless (see `isFieldMissingInRemote` / -`missing_in_remote` in `bundle_plan.go`). - -A `RemapState` that instead synthesizes `state.x = remote.status.x` breaks this: `x` is absent -from `RemoteType`, so the classifier treats it as input-only and skips it (`missing_in_remote`), -yet the field actually has a real remote value under `status.x`. Genuine remote drift is then -silently suppressed. The fix is to make the field genuinely present in `RemoteType`, not to -smuggle it in via `RemapState`. - -So when a field comes back under a different path than `StateType` uses: - -1. Add the field to `RemoteType` (the struct returned by `DoRead`). -2. Populate it in `DoRead` by mapping from whatever the API returns under the other name. -3. Keep `RemapState` a trivial subset copy. - -This makes the field present in `InputType`, `StateType`, and `RemoteType`, so it participates -in normal drift detection and is no longer subject to the `missing_in_remote` suppression. +## RemoteType should be a superset of StateType; DoRead owns all remapping + +`RemoteType` should be a superset of `StateType`, so the framework can derive the state from +the remote automatically: `buildCopiers` (in `all.go`) compiles a copier per resource at package +init, matching fields by JSON name, filtering `ForceSendFields`, and applying only lossless +conversions. New resources should not define `RemapState`. + +Do all remapping in `DoRead`. When the API returns a field under a different path than +`StateType` uses, add the field directly to `RemoteType` and populate it in `DoRead` from +whatever the API returns, so it participates in normal drift detection. ## OverrideChangeDesc diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 487df306f93..24695f04299 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -171,10 +171,10 @@ func isScalarKind(k reflect.Kind) bool { // filterOwnFields returns the names that name a field directly declared on ownerType. // Unlike reflect.Type.FieldByName it does not follow promotion, so a promoted field of an -// embedded struct is not treated as belonging to the outer struct. Marker names are matched -// by Go field name (the ForceSendFields convention), so a field the copier maps across a -// differing Go name under the same JSON name is not force-sent on the destination — this -// matches the SDK's own utils.FilterFields behavior that the hand-written copies used. +// embedded struct is not treated as belonging to the outer struct — that is what keeps a +// wrapper's shadow ForceSendFields slice from inheriting an embedded spec's field names. +// Markers are matched by Go field name (the ForceSendFields convention), so a field the copier +// maps across a differing Go name under the same JSON name is not force-sent on the destination. func filterOwnFields(ownerType reflect.Type, names []string) []string { own := make(map[string]bool, ownerType.NumField()) for field := range ownerType.Fields() { From 7e688e30d46a3956dd0db99526ac2bc2079f156d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 5 Oct 2026 16:15:08 +0200 Subject: [PATCH 16/18] structcopy: precompute each ForceSendFields owner's field set at Compile Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 29 ++++++++++++++++----------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 24695f04299..7dd461a4bff 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -41,7 +41,7 @@ type copyOp struct { dstType reflect.Type // target type for Convert // forceSendFields ops: - ownerType reflect.Type // struct type owning the ForceSendFields slice, for validity filtering + ownFields map[string]bool // field names declared directly on the slice's owner struct } // jsonFieldInfo locates a top-level JSON field within a struct, resolved through @@ -105,7 +105,7 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { } for _, target := range dstForceSend { - ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownerType: target.ownerType}) + ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, ownFields: ownFieldNames(target.ownerType)}) } return &Copier{dstElem: dstElem, ops: ops, srcForceSendIndex: srcForceSendIndex}, nil @@ -129,7 +129,7 @@ func (c *Copier) Copy(src any) any { // its own struct's fields. Filtering by directly-declared (not promoted) fields is // what keeps a shadow slice — e.g. a wrapper that re-declares ForceSendFields above // an embedded spec — from being populated with the embed's promoted field names. - filtered := filterOwnFields(op.ownerType, srcForceSend) + filtered := filterOwnFields(op.ownFields, srcForceSend) dstVal.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) continue } @@ -169,17 +169,22 @@ func isScalarKind(k reflect.Kind) bool { } } -// filterOwnFields returns the names that name a field directly declared on ownerType. -// Unlike reflect.Type.FieldByName it does not follow promotion, so a promoted field of an -// embedded struct is not treated as belonging to the outer struct — that is what keeps a -// wrapper's shadow ForceSendFields slice from inheriting an embedded spec's field names. -// Markers are matched by Go field name (the ForceSendFields convention), so a field the copier -// maps across a differing Go name under the same JSON name is not force-sent on the destination. -func filterOwnFields(ownerType reflect.Type, names []string) []string { - own := make(map[string]bool, ownerType.NumField()) - for field := range ownerType.Fields() { +// ownFieldNames returns the names of fields declared directly on t. Unlike +// reflect.Type.FieldByName it does not follow promotion, so a promoted field of an embedded +// struct is not treated as belonging to the outer struct — that is what keeps a wrapper's +// shadow ForceSendFields slice from inheriting an embedded spec's field names. +func ownFieldNames(t reflect.Type) map[string]bool { + own := make(map[string]bool, t.NumField()) + for field := range t.Fields() { own[field.Name] = true } + return own +} + +// filterOwnFields returns the names present in own. Markers are matched by Go field name (the +// ForceSendFields convention), so a field the copier maps across a differing Go name under the +// same JSON name is not force-sent on the destination. +func filterOwnFields(own map[string]bool, names []string) []string { var result []string for _, name := range names { if own[name] { From 7205b8456d92a28e87fd0677e22077ebaa2b8826 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 5 Oct 2026 16:29:55 +0200 Subject: [PATCH 17/18] structcopy: clarify copyOp.forceSendFields comment Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 7dd461a4bff..2f42ea6df07 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -30,7 +30,8 @@ type Copier struct { } type copyOp struct { - // forceSendFields ops recompute a ForceSendFields slice; field-copy ops move one field. + // If true, this op writes a filtered ForceSendFields slice to dstIndex. + // Otherwise it copies the field at srcIndex to dstIndex. forceSendFields bool dstIndex []int // FieldByIndex path in the destination struct From 97d60849bbaf179c749fb622ef3175304a32c87b Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 5 Oct 2026 16:33:18 +0200 Subject: [PATCH 18/18] structcopy: build copy ops in destination declaration order Co-authored-by: Isaac --- libs/structs/structcopy/structcopy.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libs/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go index 2f42ea6df07..4b1c29eb47e 100644 --- a/libs/structs/structcopy/structcopy.go +++ b/libs/structs/structcopy/structcopy.go @@ -9,7 +9,9 @@ package structcopy import ( "fmt" + "maps" "reflect" + "slices" "github.com/databricks/cli/libs/structs/structaccess" "github.com/databricks/cli/libs/structs/structtag" @@ -89,7 +91,12 @@ func Compile(srcType, dstType reflect.Type) (*Copier, error) { } var ops []copyOp - for name, dst := range dstFields { + // Emit ops in destination declaration order so the plan is deterministic. + names := slices.SortedFunc(maps.Keys(dstFields), func(a, b string) int { + return slices.Compare(dstFields[a].index, dstFields[b].index) + }) + for _, name := range names { + dst := dstFields[name] src, ok := srcFields[name] if !ok { // Field absent from the source type is left zero in the destination.