From edd37f24e4cfc09dc3f4ff48a2e8b446622af989 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 21 Sep 2026 16:59:46 +0200 Subject: [PATCH 1/8] 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 6210a952112..696bcee59a6 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -517,6 +517,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 69d01bd5daa17874a2eb8dbe76e43b19dc52c0a7 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 11:21:28 +0200 Subject: [PATCH 2/8] 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 696bcee59a6..6f66d098417 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) } @@ -517,18 +524,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 dce443c85557e9ee09e9888457e03aa1444403e7 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 11:35:20 +0200 Subject: [PATCH 3/8] 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 3fe343fa1aee34e7df9fbcaf22fcefd09140eb06 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 11:48:28 +0200 Subject: [PATCH 4/8] 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 e992824e5970a82f2a46e9821d0a0673277ae5fc Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 12:21:55 +0200 Subject: [PATCH 5/8] 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 81f635f1881aac88dd5d94f5fad28d9601af38fe Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 22 Sep 2026 12:28:58 +0200 Subject: [PATCH 6/8] 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 6f66d098417..2d8caf86d65 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 @@ -532,7 +533,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 ded8289b8f41ad1c5ec2c37b05710a04bd08c7e6 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 23 Sep 2026 12:03:01 +0200 Subject: [PATCH 7/8] 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 0dd7d00e7cd..82c272cc213 100644 --- a/bundle/direct/dresources/cluster.go +++ b/bundle/direct/dresources/cluster.go @@ -103,54 +103,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 d64b2c457ab..be3125092cd 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/service/workspace" ) @@ -49,18 +48,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 c941009d1bab5bdaeb796c2043cf4eb4760b3500 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 23 Sep 2026 12:20:59 +0200 Subject: [PATCH 8/8] 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{