diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index 5487042c965..02710ffd004 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -75,8 +75,15 @@ Declaring a field under `hashed_fields` in `resources.yml` makes the engine pers ## RemapState is a dumb copy; DoRead owns all remapping -`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/adapter.go b/bundle/direct/dresources/adapter.go index 6210a952112..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,6 +131,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: + copier *structcopy.Copier doDelete *calladapt.BoundCaller prepareInputConfig *calladapt.BoundCaller isEmptyState *calladapt.BoundCaller @@ -169,6 +171,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 +236,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 +370,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,15 +525,19 @@ func (a *Adapter) PrepareState(input any) (any, error) { } func (a *Adapter) RemapState(remoteState any) (any, error) { - 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..e6ebade129a 100644 --- a/bundle/direct/dresources/all.go +++ b/bundle/direct/dresources/all.go @@ -2,7 +2,12 @@ package dresources import ( "fmt" + "reflect" + "slices" + "strings" + "github.com/databricks/cli/libs/calladapt" + "github.com/databricks/cli/libs/structs/structcopy" "github.com/databricks/databricks-sdk-go" ) @@ -81,6 +86,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]*structcopy.Copier { + iface := reflect.TypeFor[IResource]() + out := make(map[reflect.Type]*structcopy.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 := structcopy.Compile(remoteType, stateType) + if err != nil { + errs = append(errs, fmt.Sprintf("%s: %v (implement RemapState for this resource)", 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/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/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.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/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_test.go b/bundle/direct/dresources/copier_test.go new file mode 100644 index 00000000000..f25cc9cd153 --- /dev/null +++ b/bundle/direct/dresources/copier_test.go @@ -0,0 +1,88 @@ +package dresources + +import ( + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/structcopy" + "github.com/stretchr/testify/assert" +) + +// 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 := structcopy.Compile(remoteType, stateType) + if err != nil { + continue // copier cannot handle it, so the override is required + } + + remote := reflect.New(remoteType.Elem()) + fillNonZero(remote.Elem(), 0) + setForceSendFields(remote.Elem()) + + want := m.Call([]reflect.Value{remote})[0].Interface() + got := copier.Copy(remote.Interface()) + if reflect.DeepEqual(want, got) { + redundant = append(redundant, resourceType) + } + } + + 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") +} + +// 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.Pointer: + if !v.IsNil() { + setForceSendFields(v.Elem()) + } + case reflect.Slice: + for i := range v.Len() { + setForceSendFields(v.Index(i)) + } + case reflect.Struct: + var names []string + for i := range v.NumField() { + sf := v.Type().Field(i) + if !sf.IsExported() || sf.Name == "ForceSendFields" { + 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)) + } + default: + // scalars and other kinds have no nested ForceSendFields to set + } +} 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/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/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/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_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/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{ 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/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/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/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) 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/structs/structcopy/structcopy.go b/libs/structs/structcopy/structcopy.go new file mode 100644 index 00000000000..6965e44cc7f --- /dev/null +++ b/libs/structs/structcopy/structcopy.go @@ -0,0 +1,173 @@ +// 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" + "maps" + "reflect" + + "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 +// 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 destination struct + + // field-copy ops: + 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 + + // 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 +} + +// 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(dstElem, nil) + srcFields, _ := flattenStruct(srcElem, nil) + + var ops []copyOp + for name, dst := range dstFields { + src, ok := srcFields[name] + if !ok { + // Field absent from the source type is left zero in the destination. + continue + } + 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}) + 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: destination type %s is not assignable or safely convertible from source type %s", name, dst.typ, src.typ) + } + } + + for _, target := range dstForceSend { + ops = append(ops, copyOp{forceSendFields: true, dstIndex: target.index, srcIndex: nil, convert: false, dstType: nil, ownerType: target.ownerType}) + } + + return &Copier{dstElem: dstElem, ops: ops}, nil +} + +// 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(srcVal) + } + filtered := utils.FilterFieldsType(op.ownerType, srcForceSend) + dstVal.FieldByIndex(op.dstIndex).Set(reflect.ValueOf(filtered)) + continue + } + val := srcVal.FieldByIndex(op.srcIndex) + if op.convert { + val = val.Convert(op.dstType) + } + 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 values compare 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) + maps.Copy(fields, nested) + 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/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)) + }) + } +} 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 {