diff --git a/.nextchanges/bundles/7261.md b/.nextchanges/bundles/7261.md new file mode 100644 index 00000000000..277bad05511 --- /dev/null +++ b/.nextchanges/bundles/7261.md @@ -0,0 +1 @@ +* A null in an included file or in a target now removes the value it overrides, e.g. `targets.dev.resources.jobs.foo: null` leaves job `foo` out of target `dev`; a null that has nothing to remove is ignored without a warning. ([#7261](https://github.com/databricks/cli/pull/7261)) diff --git a/acceptance/bundle/deploy/mlops-stacks/output.txt b/acceptance/bundle/deploy/mlops-stacks/output.txt index 1b62cc0093e..61a027c7b7d 100644 --- a/acceptance/bundle/deploy/mlops-stacks/output.txt +++ b/acceptance/bundle/deploy/mlops-stacks/output.txt @@ -25,10 +25,6 @@ This directory contains an ML project based on the default [Databricks MLOps Stacks](https://github.com/databricks/mlops-stacks), >>> [CLI] bundle summary --var catalog_name=main -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -56,10 +52,6 @@ Resources: URL: (not deployed) >>> [CLI] bundle validate --var catalog_name=main -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -70,13 +62,9 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev -Found 2 warnings +Found 1 warning >>> [CLI] bundle deploy --var catalog_name=main -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -94,10 +82,6 @@ Files: 26 uploaded, 0 deleted Resources: 8 created, 0 changed, 0 deleted, 0 unchanged >>> [CLI] bundle summary --var catalog_name=main -o json -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -110,18 +94,10 @@ Warning: unknown field: description } === Assert the batch inference job actually exists -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -131,10 +107,6 @@ Warning: unknown field: description } >>> [CLI] bundle destroy --var catalog_name=main --auto-approve -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 @@ -150,10 +122,6 @@ All files and directories at the following location will be deleted: /Workspace/ Destroy: 4 deleted >>> [CLI] schemas delete main.project_name_[UNIQUE_NAME] -Warning: expected a string value, found null - at targets.dev.workspace.host - in databricks.yml:37:12 - Warning: unknown field: description at resources.experiments.experiment in resources/ml-artifacts-resource.yml:28:7 diff --git a/acceptance/bundle/run_as/empty_override/output.txt b/acceptance/bundle/run_as/empty_override/output.txt index 635d9eda9df..4c2f1879167 100644 --- a/acceptance/bundle/run_as/empty_override/output.txt +++ b/acceptance/bundle/run_as/empty_override/output.txt @@ -1,2 +1,6 @@ - "run_as": { - "run_as": { + +>>> [CLI] bundle validate -o json +{ + "run_as": null, + "job_run_as": null +} diff --git a/acceptance/bundle/run_as/empty_override/script b/acceptance/bundle/run_as/empty_override/script index de89daa9199..de9320c4290 100644 --- a/acceptance/bundle/run_as/empty_override/script +++ b/acceptance/bundle/run_as/empty_override/script @@ -1 +1,2 @@ -$CLI bundle validate -o json | grep run_as +# An empty run_as in a target override removes run_as. +trace $CLI bundle validate -o json | jq '{run_as, job_run_as: .resources.jobs.foo.run_as}' diff --git a/acceptance/bundle/validate/empty_tasks/output.txt b/acceptance/bundle/validate/empty_tasks/output.txt index 36e6a7e63a8..7a8fcd95b9e 100644 --- a/acceptance/bundle/validate/empty_tasks/output.txt +++ b/acceptance/bundle/validate/empty_tasks/output.txt @@ -12,8 +12,7 @@ "name": "job with empty tasks", "queue": { "enabled": true - }, - "tasks": null + } }, "valid": { "deployment": { diff --git a/acceptance/bundle/validate/nulls/include_removes/output.txt b/acceptance/bundle/validate/nulls/include_removes/output.txt index f55a8cad6a6..b17656ebfdd 100644 --- a/acceptance/bundle/validate/nulls/include_removes/output.txt +++ b/acceptance/bundle/validate/nulls/include_removes/output.txt @@ -1,58 +1,28 @@ >>> [CLI] bundle validate -o json -Warning: expected a string value, found null - at bundle.cluster_id - in override.yml:2:15 - -Warning: expected a string value, found null - at presets.name_prefix - in override.yml:5:16 - -Warning: expected a string value, found null - at presets.tags.removed - in override.yml:7:14 - -Warning: expected a string value, found null - at presets.tags.not_in_base - in override.yml:8:18 - -Error: multiple resources or scripts have been defined with the same key: bar - at resources.jobs.bar - in databricks.yml:34:7 - override.yml:23:10 +Error: no value assigned to required variable with_default. Variables are usually assigned in databricks.yml, and they can be overridden using "--var", the BUNDLE_VAR_with_default environment variable, or .databricks/bundle//variable-overrides.json Exit code: 1 { - "cluster_id": "base-cluster", + "cluster_id": null, "presets": { - "name_prefix": "base-", + "name_prefix": null, "tags": { - "kept": "1", - "removed": "1" + "kept": "1" } }, "sync": { - "include": [ - "databricks.yml" + "paths": [ + "." ] }, - "permissions": [ - { - "group_name": "users", - "level": "CAN_VIEW" - } - ], + "permissions": null, "variables": { - "with_default": { - "default": "base" - } - }, - "run_as": { - "user_name": "base@example.com" + "with_default": {} }, + "run_as": null, "jobs": [ - "bar", "foo" ] } diff --git a/acceptance/bundle/validate/nulls/include_target_overrides/output.txt b/acceptance/bundle/validate/nulls/include_target_overrides/output.txt index 77ea92b92a9..cff39585292 100644 --- a/acceptance/bundle/validate/nulls/include_target_overrides/output.txt +++ b/acceptance/bundle/validate/nulls/include_target_overrides/output.txt @@ -1,43 +1,21 @@ >>> [CLI] bundle validate -o json -t dev -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.description - in a.yml:7:24 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.tags.removed - in a.yml:10:22 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.tasks[0].max_retries - in a.yml:13:28 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.max_concurrent_runs - in b.yml:8:32 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.timeout_seconds - in b.yml:9:28 - { "foo": { "description": "description from b.yml", - "max_concurrent_runs": 10, - "timeout_seconds": 120, + "max_concurrent_runs": 1, + "timeout_seconds": null, "tags": { - "kept": "1", - "removed": "1" + "kept": "1" }, "tasks": [ { "task_key": "t1", - "max_retries": 3 + "max_retries": null } ] }, "jobs": [ - "bar", "foo" ] } diff --git a/acceptance/bundle/validate/nulls/resources_container/output.txt b/acceptance/bundle/validate/nulls/resources_container/output.txt index 1241151fda5..d44a21b582a 100644 --- a/acceptance/bundle/validate/nulls/resources_container/output.txt +++ b/acceptance/bundle/validate/nulls/resources_container/output.txt @@ -1,9 +1,9 @@ >>> [CLI] bundle validate -Error: expected a map at "resources", found nil - Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 1 error - -Exit code: 1 +Validation OK! diff --git a/acceptance/bundle/validate/nulls/resources_container/script b/acceptance/bundle/validate/nulls/resources_container/script index 253cc3d205d..93f223b138b 100644 --- a/acceptance/bundle/validate/nulls/resources_container/script +++ b/acceptance/bundle/validate/nulls/resources_container/script @@ -1,2 +1,2 @@ -# resources: null is rejected. +# A null resources section in databricks.yml has nothing to remove and is accepted. errcode trace $CLI bundle validate diff --git a/acceptance/bundle/validate/nulls/resources_container/test.toml b/acceptance/bundle/validate/nulls/resources_container/test.toml deleted file mode 100644 index 022e0f2c7e9..00000000000 --- a/acceptance/bundle/validate/nulls/resources_container/test.toml +++ /dev/null @@ -1 +0,0 @@ -Badness = 'An empty (null) resources section is an error without a location, while empty targets, variables or sync sections are accepted.' diff --git a/acceptance/bundle/validate/nulls/root_without_base/output.txt b/acceptance/bundle/validate/nulls/root_without_base/output.txt index 81520fc5908..90da8d094ee 100644 --- a/acceptance/bundle/validate/nulls/root_without_base/output.txt +++ b/acceptance/bundle/validate/nulls/root_without_base/output.txt @@ -1,17 +1,7 @@ >>> [CLI] bundle validate -o json -Warning: expected a string value, found null - at resources.jobs.foo.description - in databricks.yml:8:20 - -Warning: expected a string value, found null - at resources.jobs.foo.tags.not_in_base - in databricks.yml:11:22 - { - "email_notifications": { - "on_failure": null - }, + "email_notifications": {}, "max_concurrent_runs": 1, "name": "foo", "queue": { diff --git a/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt b/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt index 8cc844569f5..b58f46ea8bd 100644 --- a/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt +++ b/acceptance/bundle/validate/nulls/schedule_default_materialization/output.txt @@ -1,13 +1,5 @@ >>> [CLI] bundle validate -o json -Warning: required field "quartz_cron_expression" is not set - at resources.jobs.j.schedule - in databricks.yml:8:17 - -Warning: required field "timezone_id" is not set - at resources.jobs.j.schedule - in databricks.yml:8:17 - { "schedule": null } diff --git a/acceptance/bundle/validate/nulls/schedule_default_materialization/script b/acceptance/bundle/validate/nulls/schedule_default_materialization/script index 0c5e31164ff..e8611e6fa89 100644 --- a/acceptance/bundle/validate/nulls/schedule_default_materialization/script +++ b/acceptance/bundle/validate/nulls/schedule_default_materialization/script @@ -1,2 +1,2 @@ -# Defaults are not applied under an explicitly null block (schedule stays null). +# A null schedule in databricks.yml has nothing to remove: the job has no schedule. trace $CLI bundle validate -o json | jq '.resources.jobs.j | {schedule}' diff --git a/acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml b/acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml deleted file mode 100644 index 68e81befdb7..00000000000 --- a/acceptance/bundle/validate/nulls/schedule_default_materialization/test.toml +++ /dev/null @@ -1 +0,0 @@ -Badness = 'An explicitly null schedule is reported as missing its required fields.' diff --git a/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt b/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt index 7b9a2d8e2d6..efeac34a4b5 100644 --- a/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt +++ b/acceptance/bundle/validate/nulls/target_keyed_slices/output.txt @@ -1,24 +1,9 @@ >>> [CLI] bundle validate -o json -t dev -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.job_clusters[0].new_cluster.num_workers - in databricks.yml:45:30 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.parameters[0].default - in databricks.yml:48:24 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.tasks[0].max_retries - in databricks.yml:51:28 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.tasks[1].max_retries - in databricks.yml:54:28 - -Warning: expected a int value, found null - at targets.dev.resources.pipelines.bar.clusters[0].num_workers - in databricks.yml:61:28 +Warning: required field "default" is not set + at resources.jobs.foo.parameters[0] + in databricks.yml:15:11 + databricks.yml:47:15 { "job_clusters": [ @@ -26,21 +11,19 @@ Warning: expected a int value, found null "job_cluster_key": "c1", "new_cluster": { "node_type_id": "[NODE_TYPE_ID]", - "num_workers": 2, + "num_workers": 0, "spark_version": "15.4.x-scala2.12" } } ], "parameters": [ { - "default": "base", "name": "p1" } ], "tasks": [ { "job_cluster_key": "c1", - "max_retries": 3, "notebook_task": { "notebook_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/dev/files/nb" }, @@ -66,8 +49,7 @@ Warning: expected a int value, found null "pipeline_clusters": [ { "label": "default", - "node_type_id": "[NODE_TYPE_ID]", - "num_workers": 2 + "node_type_id": "[NODE_TYPE_ID]" } ] } diff --git a/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt b/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt index 551c3980f68..c4e6432ab54 100644 --- a/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt +++ b/acceptance/bundle/validate/nulls/target_removes_job_fields/output.txt @@ -1,51 +1,22 @@ >>> [CLI] bundle validate -o json -t dev -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.description - in databricks.yml:30:24 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.max_concurrent_runs - in databricks.yml:31:32 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.schedule.pause_status - in databricks.yml:34:27 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.tags.removed - in databricks.yml:36:22 - -Warning: expected a string value, found null - at targets.dev.resources.jobs.foo.tags.not_in_base - in databricks.yml:37:26 - -Warning: expected a int value, found null - at targets.dev.resources.jobs.foo.timeout_seconds - in databricks.yml:40:28 - { - "description": "base description", "email_notifications": { - "on_failure": [ - "base@example.com" - ], "on_success": [ "base@example.com" ] }, - "max_concurrent_runs": 5, + "max_concurrent_runs": 1, "name": "foo", "queue": { - "enabled": false + "enabled": true }, "schedule": { - "pause_status": "PAUSED", + "pause_status": "UNPAUSED", "quartz_cron_expression": "0 0 * * * ?", "timezone_id": "UTC" }, "tags": { - "kept": "1", - "removed": "1" + "kept": "1" } } diff --git a/acceptance/bundle/validate/nulls/target_removes_resource/output.txt b/acceptance/bundle/validate/nulls/target_removes_resource/output.txt index e4c5e98b82b..2c0b94dfe2c 100644 --- a/acceptance/bundle/validate/nulls/target_removes_resource/output.txt +++ b/acceptance/bundle/validate/nulls/target_removes_resource/output.txt @@ -1,20 +1,10 @@ >>> [CLI] bundle validate -o json -t dev -Error: job not_in_base is not defined - at resources.jobs.not_in_base - in databricks.yml:19:22 - - -Exit code: 1 { "jobs": [ - "bar", - "foo", - "not_in_base" + "foo" ], - "pipelines": [ - "baz" - ] + "pipelines": [] } >>> [CLI] bundle validate -o json -t prod diff --git a/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt b/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt index 0397ae3730c..7cb643f1ce1 100644 --- a/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt +++ b/acceptance/bundle/validate/nulls/target_removes_top_level_fields/output.txt @@ -1,77 +1,38 @@ >>> [CLI] bundle validate -o json -t dev --var with_default=from-cli --var with_lookup=from-cli -Warning: expected a string value, found null - at targets.dev.mode - in databricks.yml:45:11 - -Warning: expected a string value, found null - at targets.dev.cluster_id - in databricks.yml:46:17 - -Warning: expected a string value, found null - at targets.dev.workspace.root_path - in databricks.yml:50:18 - -Warning: expected a string value, found null - at targets.dev.presets.name_prefix - in databricks.yml:52:20 - -Warning: expected a string value, found null - at targets.dev.presets.tags.removed - in databricks.yml:54:18 - { "bundle": { - "mode": "development", - "cluster_id": "base-cluster", + "mode": null, + "cluster_id": null, "git": { - "branch": "base-branch" + "branch": null } }, - "root_path": "/Workspace/Users/[USERNAME]/base", + "root_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/dev", "presets": { - "name_prefix": "base-[USERNAME]-", + "name_prefix": null, "tags": { - "dev": "[USERNAME]", - "kept": "1", - "removed": "1" + "kept": "1" } }, "sync": { - "include": [ - "databricks.yml" - ], "paths": [ "." ] }, - "permissions": [ - { - "level": "CAN_MANAGE", - "user_name": "[USERNAME]" - } - ], + "permissions": null, "variables": { "with_default": { - "default": "base", "value": "from-cli" }, "with_lookup": { - "lookup": { - "cluster": "some-test-cluster" - }, "value": "from-cli" } }, - "run_as": { - "user_name": "base@example.com" - }, + "run_as": null, "artifacts": { "kept": { "build": "build-kept" - }, - "removed": { - "build": "build-removed" } } } diff --git a/acceptance/bundle/validate/presets_name_prefix_dev/output.txt b/acceptance/bundle/validate/presets_name_prefix_dev/output.txt index c7d93c71444..570cc29a6ff 100644 --- a/acceptance/bundle/validate/presets_name_prefix_dev/output.txt +++ b/acceptance/bundle/validate/presets_name_prefix_dev/output.txt @@ -32,9 +32,5 @@ presets: # PRESETS >>> [CLI] bundle validate -o json -Warning: expected a string value, found null - at presets.name_prefix - in databricks.yml:25:15 - === The end diff --git a/acceptance/bundle/validate/required/output.txt b/acceptance/bundle/validate/required/output.txt index ae3e6dad2f8..0c55c9cdb78 100644 --- a/acceptance/bundle/validate/required/output.txt +++ b/acceptance/bundle/validate/required/output.txt @@ -4,10 +4,6 @@ Warning: required field "catalog_name" is not set at resources.volumes.my_volume in databricks.yml:35:7 -Warning: required field "job_id" is not set - at resources.jobs.my_job_1.tasks[1].run_job_task - in databricks.yml:29:24 - Warning: required field "name" is not set at resources.models.my_model_1 in databricks.yml:13:7 @@ -26,4 +22,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 5 warnings +Found 4 warnings diff --git a/bundle/config/mutator/resourcemutator/run_as_test.go b/bundle/config/mutator/resourcemutator/run_as_test.go index 200939f2dd0..26c4ad39d6f 100644 --- a/bundle/config/mutator/resourcemutator/run_as_test.go +++ b/bundle/config/mutator/resourcemutator/run_as_test.go @@ -396,7 +396,7 @@ func TestRunAsIdentities(t *testing.T) { runAs string wantError bool }{ - {`null`, true}, + {`null`, false}, {`{}`, true}, {`{user_name: ""}`, true}, {`{service_principal_name: ""}`, true}, diff --git a/bundle/config/root.go b/bundle/config/root.go index a64b742630e..49f8c0379ea 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "reflect" + "slices" "strings" "github.com/databricks/cli/bundle/config/resources" @@ -38,6 +39,14 @@ type Root struct { //nolint:recvcheck // value receivers for read-only accessors value dyn.Value depth int + // withNulls is the configuration including the nil values of maps, which remove the + // value they are merged onto (see [Root.Merge] and [Root.MergeTargetOverrides]). + // value cannot hold them: a nil scalar does not convert to the typed configuration, + // and the conversion around every mutator drops the other nils. For a loaded file this + // is the whole file; after merging, only the target sections, which keep their nils + // until the target is merged. + withNulls dyn.Value + // Contains user defined variables Variables map[string]*variable.Variable `json:"variables,omitempty"` @@ -132,10 +141,18 @@ func LoadFromBytes(path string, raw []byte) (*Root, diag.Diagnostics) { } // Normalize dynamic configuration tree according to configuration type. - v, diags := convert.Normalize(r, v) + v, diags := convert.Normalize(r, v, convert.KeepNulls) + r.withNulls = v + + // A null has nothing to remove in a file on its own, so it is dropped, except where + // it declares an empty entry, e.g. a target. + nv, err := addDeclarations(dyn.DropNulls(v), dyn.NilValue, v) + if err != nil { + return nil, diags.Extend(diag.Errorf("failed to load %s: %v", path, err)) + } // Convert normalized configuration tree to typed configuration. - err = r.updateWithDynamicValue(v) + err = r.updateWithDynamicValue(nv) if err != nil { diags = diags.Extend(diag.Errorf("failed to load %s: %v", path, err)) return nil, diags @@ -163,9 +180,11 @@ func (r *Root) updateWithDynamicValue(nv dyn.Value) error { // Hack: restore state; it may be cleared by [ToTyped] if // the configuration equals nil (happens in tests). depth := r.depth + withNulls := r.withNulls defer func() { r.depth = depth + r.withNulls = withNulls }() // Convert normalized configuration tree to typed configuration. @@ -295,10 +314,132 @@ func (r *Root) InitializeVariables(vars []string) error { return nil } +// Merge merges other into r. Values in other take precedence, and a null in other +// removes the value from r. func (r *Root) Merge(other *Root) error { + nulls := targetSections(other.withNulls) + if r.withNulls.IsValid() { + var err error + nulls, err = merge.MergeKeepingNulls(r.withNulls, nulls) + if err != nil { + return err + } + } + r.withNulls = nulls + // Merge dynamic configuration values. return r.Mutate(func(root dyn.Value) (dyn.Value, error) { - return merge.Merge(root, other.value) + merged, err := merge.MergeRemovingNulls(root, other.withNulls) + if err != nil { + return dyn.InvalidValue, err + } + return addDeclarations(merged, root, other.withNulls) + }) +} + +// declarationSections are the maps in which a null entry declares an empty entry if +// there is no entry to remove: an empty "targets.dev:" declares target dev, and an +// empty "resources.jobs.foo:" is reported as an undefined job. The resources section +// holds one such map per resource type. +var declarationSections = []string{"targets", "environments", "variables", "artifacts", "scripts"} + +// addDeclarations returns v with the null entries of the declaration sections in nulls +// that have no value to remove in base. +func addDeclarations(v, base, nulls dyn.Value) (dyn.Value, error) { + var sections []dyn.Path + for _, section := range declarationSections { + sections = append(sections, dyn.NewPath(dyn.Key(section))) + } + if resources, ok := nulls.Get("resources").AsMap(); ok { + for _, typ := range resources.Keys() { + sections = append(sections, dyn.NewPath(dyn.Key("resources"), dyn.Key(typ.MustString()))) + } + } + + for _, section := range sections { + entries, err := dyn.GetByPath(nulls, section) + if err != nil || entries.Kind() != dyn.KindMap { + continue + } + for _, pair := range entries.MustMap().Pairs() { + if pair.Value.Kind() != dyn.KindNil { + continue + } + p := section.Append(dyn.Key(pair.Key.MustString())) + if bv, err := dyn.GetByPath(base, p); err == nil && bv.Kind() != dyn.KindNil { + continue + } + v, err = dyn.SetByPath(v, p, pair.Value) + if err != nil { + return dyn.InvalidValue, err + } + } + } + return v, nil +} + +// targetSections returns the sections of v that hold target overrides. +func targetSections(v dyn.Value) dyn.Value { + out := dyn.NewMapping() + for _, key := range []string{"targets", "environments"} { + if section := v.Get(key); section.IsValid() { + out.SetLoc(key, nil, section) + } + } + return dyn.V(out) +} + +// targetWithNulls returns the target as loaded, including its nulls. +func (r *Root) targetWithNulls(name string) dyn.Value { + targets := r.withNulls.Get("targets") + if !targets.IsValid() { + // The deprecated name, renamed to "targets" by the EnvironmentsToTargets mutator. + targets = r.withNulls.Get("environments") + } + return targets.Get(name) +} + +// setNulls returns dst with the nil map values of src set at the same paths. +func setNulls(dst, src dyn.Value) (dyn.Value, error) { + var nulls []dyn.Path + _, err := dyn.Walk(src, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + if v.Kind() == dyn.KindNil && len(p) > 0 && p[len(p)-1].Key() != "" { + nulls = append(nulls, slices.Clone(p)) + } + return v, nil + }) + if err != nil { + return dyn.InvalidValue, err + } + for _, p := range nulls { + dst, err = dyn.SetByPath(dst, p, dyn.NilValue) + if err != nil { + return dyn.InvalidValue, err + } + } + return dst, nil +} + +// setOrRemove returns v with the value at p set to nv without its nulls, or removed if +// nv is null. +func setOrRemove(v dyn.Value, p dyn.Path, nv dyn.Value) (dyn.Value, error) { + if nv.Kind() == dyn.KindNil { + return removeByPath(v, p) + } + return dyn.SetByPath(v, p, dyn.DropNulls(nv)) +} + +// removeByPath returns v without the value at p, if there is one. +func removeByPath(v dyn.Value, p dyn.Path) (dyn.Value, error) { + return dyn.Walk(v, func(vp dyn.Path, v dyn.Value) (dyn.Value, error) { + switch { + case vp.Equal(p): + return v, dyn.ErrDrop + case p.HasPrefix(vp): + return v, nil + default: + return v, dyn.ErrSkip + } }) } @@ -307,18 +448,23 @@ func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { reference, _ := dyn.GetByPath(rv, path) override, _ := dyn.GetByPath(ov, path) + // A null override removes the field. + if override.Kind() == dyn.KindNil { + return removeByPath(rv, path) + } + // Merge the override into the reference. var out dyn.Value var err error if reference.IsValid() && override.IsValid() { - out, err = merge.Merge(reference, override) + out, err = merge.MergeRemovingNulls(reference, override) if err != nil { return dyn.InvalidValue, err } } else if reference.IsValid() { out = reference } else if override.IsValid() { - out = override + out = dyn.DropNulls(override) } else { return rv, nil } @@ -339,6 +485,18 @@ func (r *Root) MergeTargetOverrides(name string) error { return err } + // A null in the target removes the value from the root; the target as merged so + // far has lost its nulls, so they are set again. + target, err = setNulls(target, r.targetWithNulls(name)) + if err != nil { + return fmt.Errorf("failed to apply the nulls of target=%s: %w", name, err) + } + + root, target, err = mergeKeyedSequencesWithNulls(root, target) + if err != nil { + return err + } + // Merge fields that can be merged 1:1. for _, f := range []string{ "bundle", @@ -362,29 +520,33 @@ func (r *Root) MergeTargetOverrides(name string) error { vDefault := variable.Get("default") if vDefault.Kind() != dyn.KindInvalid { defaultPath := varPath.Append(dyn.Key("default")) - root, err = dyn.SetByPath(root, defaultPath, vDefault) + root, err = setOrRemove(root, defaultPath, vDefault) if err != nil { return root, err } // If the target explicitly sets a default value, drop any lookup from the // root variable definition so SetVariables can assign this default. - lookupPath := varPath.Append(dyn.Key("lookup")) - root, err = dyn.SetByPath(root, lookupPath, dyn.NilValue) + if vDefault.Kind() != dyn.KindNil { + lookupPath := varPath.Append(dyn.Key("lookup")) + root, err = dyn.SetByPath(root, lookupPath, dyn.NilValue) + } } vLookup := variable.Get("lookup") if vLookup.Kind() != dyn.KindInvalid { lookupPath := varPath.Append(dyn.Key("lookup")) - root, err = dyn.SetByPath(root, lookupPath, vLookup) + root, err = setOrRemove(root, lookupPath, vLookup) if err != nil { return root, err } // If the target explicitly sets a lookup, drop any default value from the // root variable definition so lookup resolution remains authoritative. - defaultPath := varPath.Append(dyn.Key("default")) - root, err = dyn.SetByPath(root, defaultPath, dyn.NilValue) + if vLookup.Kind() != dyn.KindNil { + defaultPath := varPath.Append(dyn.Key("default")) + root, err = dyn.SetByPath(root, defaultPath, dyn.NilValue) + } } return root, err @@ -396,7 +558,7 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `run_as`. This field must be overwritten if set, not merged. if v := target.Get("run_as"); v.Kind() != dyn.KindInvalid { - root, err = dyn.Set(root, "run_as", v) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("run_as")), v) if err != nil { return err } @@ -412,7 +574,7 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `mode`. This field must be overwritten if set, not merged. if v := target.Get("mode"); v.Kind() != dyn.KindInvalid { - root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v) if err != nil { return err } @@ -420,7 +582,7 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `cluster_id`. This field must be overwritten if set, not merged. if v := target.Get("cluster_id"); v.Kind() != dyn.KindInvalid { - root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("cluster_id")), v) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("cluster_id")), v) if err != nil { return err } @@ -433,23 +595,124 @@ func (r *Root) MergeTargetOverrides(name string) error { ref = dyn.V(map[string]dyn.Value{}) } - // Merge the override into the reference. - out, err := merge.Merge(ref, v) - if err != nil { - return err + // Merge the override into the reference. A null override removes it. + out := v + if v.Kind() != dyn.KindNil { + out, err = merge.MergeRemovingNulls(ref, v) + if err != nil { + return err + } } // Set the merged value. - root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git")), out) + root, err = setOrRemove(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git")), out) if err != nil { return err } } + r.withNulls = dyn.InvalidValue + // Convert normalized configuration tree to typed configuration. return r.updateWithDynamicValue(root) } +// keyedSequences are the sequences whose elements the Merge* mutators in +// bundle/config/mutator/resourcemutator merge by key. Those run after the nulls are +// gone, so a sequence with nulls in its elements is merged by key with the target. +var keyedSequences = []struct { + resourceType string + field string + key string + keyFunc func(dyn.Value) string +}{ + {"jobs", "tasks", "task_key", stringKey}, + {"jobs", "job_clusters", "job_cluster_key", stringKey}, + {"jobs", "parameters", "name", stringKey}, + {"pipelines", "clusters", "label", pipelineClusterLabel}, +} + +func stringKey(v dyn.Value) string { + s, _ := v.AsString() + return s +} + +// pipelineClusterLabel matches the key of the MergePipelineClusters mutator: the label +// is case-insensitive and defaults to "default". +func pipelineClusterLabel(v dyn.Value) string { + s, ok := v.AsString() + if !ok { + return "default" + } + return strings.ToLower(s) +} + +// mergeKeyedSequencesWithNulls merges the keyed sequences of the target that hold nulls +// with the same sequence of the root, element by element with the same key, so that a +// null removes the value from the element it overrides. The merged sequence replaces the +// one in the target and is removed from the root. Resources and elements can still be +// ${...} references here, which are left as they are. +func mergeKeyedSequencesWithNulls(root, target dyn.Value) (dyn.Value, dyn.Value, error) { + for _, ks := range keyedSequences { + resources, ok := target.Get("resources").Get(ks.resourceType).AsMap() + if !ok { + continue + } + for _, pair := range resources.Pairs() { + override := pair.Value.Get(ks.field) + seq, ok := override.AsSequence() + if !ok || !hasNulls(override) || slices.ContainsFunc(seq, isNotMap) { + continue + } + + p := dyn.NewPath(dyn.Key("resources"), dyn.Key(ks.resourceType), dyn.Key(pair.Key.MustString()), dyn.Key(ks.field)) + reference, _ := dyn.GetByPath(root, p) + if reference.IsValid() { + base, ok := reference.AsSequence() + if !ok || slices.ContainsFunc(base, isNotMap) { + continue + } + seq = slices.Concat(base, seq) + } + + merged, err := merge.ElementsByKeyKeepingNulls(ks.key, ks.keyFunc)(p, dyn.NewValue(seq, override.Locations())) + if err != nil { + return dyn.InvalidValue, dyn.InvalidValue, err + } + target, err = dyn.SetByPath(target, p, dyn.DropNulls(merged)) + if err != nil { + return dyn.InvalidValue, dyn.InvalidValue, err + } + root, err = removeByPath(root, p) + if err != nil { + return dyn.InvalidValue, dyn.InvalidValue, err + } + } + } + return root, target, nil +} + +func isNotMap(v dyn.Value) bool { + return v.Kind() != dyn.KindMap +} + +// hasNulls reports whether v holds a nil map value at any depth. +func hasNulls(v dyn.Value) bool { + switch v.Kind() { + case dyn.KindMap: + for _, pair := range v.MustMap().Pairs() { + if pair.Value.Kind() == dyn.KindNil || hasNulls(pair.Value) { + return true + } + } + case dyn.KindSequence: + return slices.ContainsFunc(v.MustSequence(), hasNulls) + default: + // Leaf node. + } + return false +} + var allowedVariableDefinitions = []([]string){ {"default", "type", "description"}, {"default", "type"}, diff --git a/libs/dyn/convert/normalize.go b/libs/dyn/convert/normalize.go index e41ed4a0a87..17eb2c0d553 100644 --- a/libs/dyn/convert/normalize.go +++ b/libs/dyn/convert/normalize.go @@ -26,11 +26,17 @@ const ( // ToTyped, which force-sends it, and the backend rejects it (e.g. // "'' is not a valid cluster policy ID"). DropEmptyStrings + + // KeepNulls keeps the nil values of struct fields and map entries as they are, + // instead of dropping them with a warning (scalars) or keeping them (other types). + // Nil sequence elements are handled as without this option. + KeepNulls ) type normalizeOptions struct { includeMissingFields bool dropEmptyStrings bool + keepNulls bool } func Normalize(dst any, src dyn.Value, opts ...NormalizeOption) (dyn.Value, diag.Diagnostics) { @@ -41,6 +47,8 @@ func Normalize(dst any, src dyn.Value, opts ...NormalizeOption) (dyn.Value, diag n.includeMissingFields = true case DropEmptyStrings: n.dropEmptyStrings = true + case KeepNulls: + n.keepNulls = true } } @@ -167,6 +175,11 @@ func (n normalizeOptions) normalizeStruct(typ reflect.Type, src dyn.Value, seen continue } + if n.keepNulls && pv.Kind() == dyn.KindNil { + out.SetLoc(pk.MustString(), pk.Locations(), pv) + continue + } + // Normalize the value according to the field type. nv, err := n.normalizeType(typ.FieldByIndex(index).Type, pv, seen, path.Append(dyn.Key(pk.MustString()))) if err != nil { @@ -263,6 +276,11 @@ func (n normalizeOptions) normalizeMap(typ reflect.Type, src dyn.Value, seen []r pk := pair.Key pv := pair.Value + if n.keepNulls && pv.Kind() == dyn.KindNil { + out.SetLoc(pk.MustString(), pk.Locations(), pv) + continue + } + // Normalize the value according to the map element type. nv, err := n.normalizeType(typ.Elem(), pv, seen, path.Append(dyn.Key(pk.MustString()))) if err != nil { diff --git a/libs/dyn/convert/normalize_test.go b/libs/dyn/convert/normalize_test.go index ac95aaafac6..9cc4863d317 100644 --- a/libs/dyn/convert/normalize_test.go +++ b/libs/dyn/convert/normalize_test.go @@ -1016,3 +1016,40 @@ func TestNormalizeStructDropEmptyStringsKeepsNonEmpty(t *testing.T) { assert.True(t, ok) assert.Equal(t, "value", opt.MustString()) } + +func TestNormalizeKeepNulls(t *testing.T) { + type Inner struct { + Foo string `json:"foo"` + } + type Tmp struct { + Str string `json:"str"` + Int int `json:"int"` + Inner *Inner `json:"inner"` + Map map[string]string `json:"map"` + List []string `json:"list"` + } + + var typ Tmp + vin := dyn.V(map[string]dyn.Value{ + "str": dyn.NilValue, + "int": dyn.NilValue, + "inner": dyn.V(map[string]dyn.Value{"foo": dyn.NilValue}), + "map": dyn.V(map[string]dyn.Value{"key": dyn.NilValue}), + "list": dyn.V([]dyn.Value{dyn.V("a"), dyn.NilValue}), + }) + + vout, diags := Normalize(typ, vin, KeepNulls) + assert.Equal(t, diag.Diagnostics{{ + Severity: diag.Warning, + Summary: `expected a string value, found null`, + Locations: []diag.Location{{}}, + Paths: []*structpath.PathNode{structpath.NewIndex(structpath.NewStringKey(nil, "list"), 1)}, + }}, diags) + assert.Equal(t, map[string]any{ + "str": nil, + "int": nil, + "inner": map[string]any{"foo": nil}, + "map": map[string]any{"key": nil}, + "list": []any{"a"}, + }, vout.AsAny()) +} diff --git a/libs/dyn/drop_nulls.go b/libs/dyn/drop_nulls.go new file mode 100644 index 00000000000..0dd58dca977 --- /dev/null +++ b/libs/dyn/drop_nulls.go @@ -0,0 +1,28 @@ +package dyn + +// DropNulls returns v without the nil values of its maps, at any depth. +// Nil elements of sequences are kept. +func DropNulls(v Value) Value { + switch v.Kind() { + case KindMap: + m := v.MustMap() + out := newMappingWithSize(m.Len()) + for _, pair := range m.Pairs() { + if pair.Value.Kind() == KindNil { + continue + } + out.SetLoc(pair.Key.MustString(), pair.Key.Locations(), DropNulls(pair.Value)) + } + v.v = out + case KindSequence: + s := v.MustSequence() + out := make([]Value, len(s)) + for i := range s { + out[i] = DropNulls(s[i]) + } + v.v = out + default: + // Leaf node. + } + return v +} diff --git a/libs/dyn/drop_nulls_test.go b/libs/dyn/drop_nulls_test.go new file mode 100644 index 00000000000..38d51797e37 --- /dev/null +++ b/libs/dyn/drop_nulls_test.go @@ -0,0 +1,35 @@ +package dyn_test + +import ( + "testing" + + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + assert "github.com/databricks/cli/libs/dyn/dynassert" +) + +func TestDropNulls(t *testing.T) { + loc := diag.Location{File: "file", Line: 1, Column: 2} + v := dyn.NewValue(map[string]dyn.Value{ + "null": dyn.NilValue, + "map": dyn.V(map[string]dyn.Value{ + "null": dyn.NilValue, + "value": dyn.V("a"), + }), + "list": dyn.V([]dyn.Value{ + dyn.NilValue, + dyn.V(map[string]dyn.Value{"null": dyn.NilValue}), + }), + }, []diag.Location{loc}) + + out := dyn.DropNulls(v) + assert.Equal(t, map[string]any{ + "map": map[string]any{"value": "a"}, + "list": []any{nil, map[string]any{}}, + }, out.AsAny()) + assert.Equal(t, []diag.Location{loc}, out.Locations()) +} + +func TestDropNullsNil(t *testing.T) { + assert.Equal(t, dyn.NilValue, dyn.DropNulls(dyn.NilValue)) +} diff --git a/libs/dyn/merge/elements_by_key.go b/libs/dyn/merge/elements_by_key.go index 6bf71d41ae5..b503ce87a15 100644 --- a/libs/dyn/merge/elements_by_key.go +++ b/libs/dyn/merge/elements_by_key.go @@ -71,6 +71,10 @@ func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return e.doMap(nil, v, Merge) } +func (e elementsByKey) MapKeepingNulls(_ dyn.Path, v dyn.Value) (dyn.Value, error) { + return e.doMap(nil, v, MergeKeepingNulls) +} + func (e elementsByKey) MapWithOverride(p dyn.Path, v dyn.Value) (dyn.Value, error) { return e.doMap(nil, v, func(a, b dyn.Value) (dyn.Value, error) { return Override(a, b, OverrideVisitor{ @@ -105,3 +109,9 @@ func ElementsBySortedKey(key string, keyFunc func(dyn.Value) string) dyn.MapFunc func ElementsByKeyWithOverride(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { return elementsByKey{key, keyFunc, false}.MapWithOverride } + +// ElementsByKeyKeepingNulls is like [ElementsByKey], except that elements with the same +// key are merged with [MergeKeepingNulls]. +func ElementsByKeyKeepingNulls(key string, keyFunc func(dyn.Value) string) dyn.MapFunc { + return elementsByKey{key, keyFunc, false}.MapKeepingNulls +} diff --git a/libs/dyn/merge/elements_by_key_test.go b/libs/dyn/merge/elements_by_key_test.go index 2aa19306820..67593b7aa6e 100644 --- a/libs/dyn/merge/elements_by_key_test.go +++ b/libs/dyn/merge/elements_by_key_test.go @@ -107,3 +107,31 @@ func TestElementByKeyWithOverride(t *testing.T) { }, vout.Index(1).AsAny(), ) } + +func TestElementByKeyKeepingNulls(t *testing.T) { + vin := dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "key": dyn.V("foo"), + "removed": dyn.V(42), + "kept": dyn.V(42), + }), + dyn.V(map[string]dyn.Value{ + "key": dyn.V("foo"), + "removed": dyn.NilValue, + }), + }) + + keyFunc := func(v dyn.Value) string { + return v.MustString() + } + + vout, err := dyn.MapByPath(vin, dyn.EmptyPath, ElementsByKeyKeepingNulls("key", keyFunc)) + require.NoError(t, err) + assert.Equal(t, []any{ + map[string]any{ + "key": "foo", + "removed": nil, + "kept": 42, + }, + }, vout.AsAny()) +} diff --git a/libs/dyn/merge/merge.go b/libs/dyn/merge/merge.go index 374450ef240..a08f581c5f6 100644 --- a/libs/dyn/merge/merge.go +++ b/libs/dyn/merge/merge.go @@ -2,6 +2,7 @@ package merge import ( "fmt" + "slices" "github.com/databricks/cli/libs/dyn" ) @@ -33,15 +34,45 @@ import ( // incoming value is retained and the location of the existing value `a` is accumulated. // This is because the incoming value overwrites the existing value. func Merge(a, b dyn.Value) (dyn.Value, error) { - return merge(a, b) + return merge(a, b, nullIsIgnored) } -func merge(a, b dyn.Value) (dyn.Value, error) { +// MergeKeepingNulls is like [Merge], except that a nil in b replaces the value in a. +// The nils in b are kept in the result. +func MergeKeepingNulls(a, b dyn.Value) (dyn.Value, error) { + return merge(a, b, nullReplaces) +} + +// MergeRemovingNulls is like [Merge], except that a nil map value in b removes the +// same key from a. The nil map values of b are not added to the result, also where a +// has no value to remove (see [dyn.DropNulls]). A nil b leaves a unchanged. +func MergeRemovingNulls(a, b dyn.Value) (dyn.Value, error) { + return merge(a, b, nullRemoves) +} + +// nullMode is what a nil in the second value does to the first one. +type nullMode int + +const ( + nullIsIgnored nullMode = iota + nullReplaces + nullRemoves +) + +func merge(a, b dyn.Value, mode nullMode) (dyn.Value, error) { ak := a.Kind() bk := b.Kind() + // If b is nil and replaces a, return b. + if bk == dyn.KindNil && mode == nullReplaces { + return b, nil + } + // If a is nil, return b. if ak == dyn.KindNil { + if mode == nullRemoves { + b = dyn.DropNulls(b) + } return b.AppendLocationsFromValue(a), nil } @@ -56,12 +87,12 @@ func merge(a, b dyn.Value) (dyn.Value, error) { if bk != dyn.KindMap { return dyn.InvalidValue, fmt.Errorf("cannot merge map with %s", bk) } - return mergeMap(a, b) + return mergeMap(a, b, mode) case dyn.KindSequence: if bk != dyn.KindSequence { return dyn.InvalidValue, fmt.Errorf("cannot merge sequence with %s", bk) } - return mergeSequence(a, b) + return mergeSequence(a, b, mode) default: if ak != bk { return dyn.InvalidValue, fmt.Errorf("cannot merge %s with %s", ak, bk) @@ -70,7 +101,7 @@ func merge(a, b dyn.Value) (dyn.Value, error) { } } -func mergeMap(a, b dyn.Value) (dyn.Value, error) { +func mergeMap(a, b dyn.Value, mode nullMode) (dyn.Value, error) { out := dyn.NewMapping() am := a.MustMap() bm := b.MustMap() @@ -79,30 +110,45 @@ func mergeMap(a, b dyn.Value) (dyn.Value, error) { out.Merge(am) // Merge the values from b into the output map. + var removed []string for _, pair := range bm.Pairs() { pk := pair.Key key := pk.MustString() pv := pair.Value + if mode == nullRemoves && pv.Kind() == dyn.KindNil { + removed = append(removed, key) + continue + } if ov, ok := out.Get(pk); ok { // If the key already exists, merge the values. - merged, err := merge(ov, pv) + merged, err := merge(ov, pv, mode) if err != nil { return dyn.InvalidValue, err } out.SetLoc(key, pair.Key.Locations(), merged) } else { // Otherwise, just set the value. + if mode == nullRemoves { + pv = dyn.DropNulls(pv) + } out.SetLoc(key, pair.Key.Locations(), pv) } } + if len(removed) > 0 { + out = withoutKeys(out, removed) + } + // Preserve the location of the first value. Accumulate the locations of the second value. return dyn.NewValue(out, a.Locations()).AppendLocationsFromValue(b), nil } -func mergeSequence(a, b dyn.Value) (dyn.Value, error) { +func mergeSequence(a, b dyn.Value, mode nullMode) (dyn.Value, error) { as := a.MustSequence() bs := b.MustSequence() + if mode == nullRemoves { + bs = dyn.DropNulls(b).MustSequence() + } // Merging sequences means concatenating them. out := make([]dyn.Value, len(as)+len(bs)) @@ -117,3 +163,15 @@ func mergePrimitive(a, b dyn.Value) (dyn.Value, error) { // Merging primitive values means using the incoming value. return b.AppendLocationsFromValue(a), nil } + +// withoutKeys returns m without the given keys. +func withoutKeys(m dyn.Mapping, keys []string) dyn.Mapping { + out := dyn.NewMapping() + for _, pair := range m.Pairs() { + key := pair.Key.MustString() + if !slices.Contains(keys, key) { + out.SetLoc(key, pair.Key.Locations(), pair.Value) + } + } + return out +} diff --git a/libs/dyn/merge/merge_test.go b/libs/dyn/merge/merge_test.go index 9545c752258..92f89b0d31f 100644 --- a/libs/dyn/merge/merge_test.go +++ b/libs/dyn/merge/merge_test.go @@ -6,6 +6,7 @@ import ( "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestMergeMaps(t *testing.T) { @@ -299,3 +300,99 @@ func TestMergePrimitivesError(t *testing.T) { assert.Equal(t, dyn.InvalidValue, out) } } + +func TestMergeNullModes(t *testing.T) { + a := dyn.V(map[string]dyn.Value{ + "removed": dyn.V("a"), + "kept": dyn.V("a"), + "nested": dyn.V(map[string]dyn.Value{ + "removed": dyn.V("a"), + "kept": dyn.V("a"), + }), + "list": dyn.V([]dyn.Value{dyn.V("a")}), + }) + b := dyn.V(map[string]dyn.Value{ + "removed": dyn.NilValue, + "nested": dyn.V(map[string]dyn.Value{ + "removed": dyn.NilValue, + }), + "not_in_a": dyn.NilValue, + "new": dyn.V(map[string]dyn.Value{ + "null": dyn.NilValue, + "value": dyn.V("b"), + }), + "list": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{"null": dyn.NilValue}), + }), + }) + + for _, tc := range []struct { + name string + merge func(a, b dyn.Value) (dyn.Value, error) + want map[string]any + }{ + { + name: "ignored", + merge: Merge, + want: map[string]any{ + "removed": "a", + "kept": "a", + "nested": map[string]any{"removed": "a", "kept": "a"}, + "list": []any{"a", map[string]any{"null": nil}}, + "not_in_a": nil, + "new": map[string]any{"null": nil, "value": "b"}, + }, + }, + { + name: "keeping nulls", + merge: MergeKeepingNulls, + want: map[string]any{ + "removed": nil, + "kept": "a", + "nested": map[string]any{"removed": nil, "kept": "a"}, + "list": []any{"a", map[string]any{"null": nil}}, + "not_in_a": nil, + "new": map[string]any{"null": nil, "value": "b"}, + }, + }, + { + name: "removing nulls", + merge: MergeRemovingNulls, + want: map[string]any{ + "kept": "a", + "nested": map[string]any{"kept": "a"}, + "list": []any{"a", map[string]any{}}, + "new": map[string]any{"value": "b"}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := tc.merge(a, b) + require.NoError(t, err) + assert.Equal(t, tc.want, out.AsAny()) + }) + } +} + +func TestMergeNullModesNilRoot(t *testing.T) { + b := dyn.V(map[string]dyn.Value{ + "null": dyn.NilValue, + "value": dyn.V("b"), + }) + + out, err := MergeKeepingNulls(dyn.NilValue, b) + require.NoError(t, err) + assert.Equal(t, map[string]any{"null": nil, "value": "b"}, out.AsAny()) + + out, err = MergeRemovingNulls(dyn.NilValue, b) + require.NoError(t, err) + assert.Equal(t, map[string]any{"value": "b"}, out.AsAny()) + + out, err = MergeKeepingNulls(b, dyn.NilValue) + require.NoError(t, err) + assert.Equal(t, dyn.KindNil, out.Kind()) + + out, err = MergeRemovingNulls(b, dyn.NilValue) + require.NoError(t, err) + assert.Equal(t, map[string]any{"null": nil, "value": "b"}, out.AsAny()) +}