Correctly handle zero and null values when converting typed/dynamic (#3230)

denik · web-flow · commit a5d0248af872 · 2025-07-30T14:12:10.000Z
## Changes When converting between struct and dyn.Value, respect omitempty/omitzero and ForceSendFields. The conversion should now match what would happen if you did json encode/decode. ## Why There is a lot of conversion between structs and dyn.Value and information is lost each time. This means typed structs in bundle.Config are not completely accurate. There consequences of this: - In direct deployment we cannot read resource definitions from bundle.Config, we need to go through JSON to get correct typed structs. - Some typed code does the wrong thing, there are known and probably some unknown cases. Example of known case: [acceptance/bundle/resources/jobs/instance_pool_and_node_type/test.toml](https://github.com/databricks/cli/pull/3230/files#diff-f988a9ad8b0ddd1050e215cc4dbe15f834702033421f7463003075d967e2b256)
diff --git a/acceptance/bundle/integration_whl/base/test.toml b/acceptance/bundle/integration_whl/base/test.toml
diff --git a/acceptance/bundle/quality_monitor/output.txt b/acceptance/bundle/quality_monitor/output.txt
@@ -1,13 +1,5 @@
 
 >>> [CLI] bundle validate -o json -t development
-Warning: required field "quartz_cron_expression" is not set
-  at resources.quality_monitors.my_monitor.schedule
-  in databricks.yml:17:9
-
-Warning: required field "timezone_id" is not set
-  at resources.quality_monitors.my_monitor.schedule
-  in databricks.yml:17:9
-
 {
   "mode": "development",
   "quality_monitors": {
@@ -23,7 +15,6 @@ Warning: required field "timezone_id" is not set
         "timestamp_col": "timestamp"
       },
       "output_schema_name": "main.dev",
-      "schedule": null,
       "table_name": "main.test.dev"
     }
   }
diff --git a/acceptance/bundle/resources/jobs/instance_pool_and_node_type/out.test.toml b/acceptance/bundle/resources/jobs/instance_pool_and_node_type/out.test.toml
@@ -2,4 +2,4 @@ Local = true
 Cloud = false
 
 [EnvMatrix]
-  DATABRICKS_CLI_DEPLOYMENT = ["terraform"]
+  DATABRICKS_CLI_DEPLOYMENT = ["terraform", "direct-exp"]
diff --git a/acceptance/bundle/resources/jobs/instance_pool_and_node_type/output.txt b/acceptance/bundle/resources/jobs/instance_pool_and_node_type/output.txt
@@ -5,7 +5,6 @@
     "new_cluster": {
       "data_security_mode": "USER_ISOLATION",
       "instance_pool_id": "$TEST_INSTANCE_POOL_ID",
-      "node_type_id": "",
       "num_workers": 1,
       "spark_version": "$DEFAULT_SPARK_VERSION"
     },
@@ -27,7 +26,6 @@
     "new_cluster": {
       "data_security_mode": "USER_ISOLATION",
       "instance_pool_id": "$TEST_INSTANCE_POOL_ID",
-      "node_type_id": "",
       "num_workers": 1,
       "spark_version": "$DEFAULT_SPARK_VERSION"
     },
diff --git a/acceptance/bundle/resources/jobs/instance_pool_and_node_type/test.toml b/acceptance/bundle/resources/jobs/instance_pool_and_node_type/test.toml
@@ -1,18 +1 @@
 RecordRequests = true
-
-# Fails on direct with
-#    --- FAIL: TestAccept/bundle/resources/jobs/instance_pool_and_node_type (0.00s)
-#        --- FAIL: TestAccept/bundle/resources/jobs/instance_pool_and_node_type/DATABRICKS_CLI_DEPLOYMENT=direct-exp (1.60s)
-#            acceptance_test.go:1178: Writing updated bundle config to databricks.yml. BundleConfig sections: default_name
-#            acceptance_test.go:722: Diff:
-#                --- bundle/resources/jobs/instance_pool_and_node_type/output.txt
-#                +++ /var/folders/5y/9kkdnjw91p11vsqwk0cvmk200000gp/T/TestAcceptbundleresourcesjobsinstance_pool_and_node_typeDATABRICKS_CLI_DEPLOYMENT=direct-exp3221363519/001/output.txt
-#                @@ -55,6 +55,7 @@
-#                     "new_cluster": {
-#                       "data_security_mode": "USER_ISOLATION",
-#                       "instance_pool_id": "$TEST_INSTANCE_POOL_ID",
-#                +      "node_type_id": "",
-#                       "num_workers": 1,
-#                       "spark_version": "$DEFAULT_SPARK_VERSION"
-#                     },
-EnvMatrix.DATABRICKS_CLI_DEPLOYMENT = ["terraform"]
diff --git a/acceptance/bundle/run_as/empty_override/output.txt b/acceptance/bundle/run_as/empty_override/output.txt
@@ -1,6 +1,2 @@
-Error: run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified
-  in override.yml:4:12
-
-
-Exit code (musterr): 1
-  "run_as": null,
+        "run_as": {
+  "run_as": {
diff --git a/acceptance/bundle/run_as/empty_override/script b/acceptance/bundle/run_as/empty_override/script
@@ -1 +1 @@
-musterr $CLI bundle validate -o json | grep run_as
+$CLI bundle validate -o json | grep run_as
diff --git a/acceptance/bundle/run_as/empty_run_as/output.txt b/acceptance/bundle/run_as/empty_run_as/output.txt
@@ -1,6 +1,7 @@
-Error: run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified
-  in databricks.yml:4:8
+Name: abc
+Target: default
+Workspace:
+  User: [USERNAME]
+  Path: /Workspace/Users/[USERNAME]/.bundle/abc/default
 
-
-Exit code (musterr): 1
-  "run_as": null,
+Validation OK!
diff --git a/acceptance/bundle/run_as/empty_run_as/script b/acceptance/bundle/run_as/empty_run_as/script
@@ -1 +1 @@
-musterr $CLI bundle validate -o json | grep run_as
+$CLI bundle validate
diff --git a/acceptance/bundle/run_as/empty_run_as/test.toml b/acceptance/bundle/run_as/empty_run_as/test.toml
@@ -0,0 +1 @@
+Ignore = [".databricks/.gitignore"]
diff --git a/acceptance/bundle/templates/default-python/integration_classic/out.validate.dev.json b/acceptance/bundle/templates/default-python/integration_classic/out.validate.dev.json
@@ -151,7 +151,6 @@
       "."
     ]
   },
-  "targets": null,
   "workspace": {
     "artifact_path": "/Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev/artifacts",
     "file_path": "/Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev/files",
diff --git a/acceptance/bundle/templates/default-python/integration_classic/output.txt b/acceptance/bundle/templates/default-python/integration_classic/output.txt
@@ -219,8 +219,8 @@ Validation OK!
 +        "schema": "project_name_[UNIQUE_NAME]_prod"
        }
      }
-@@ -154,10 +140,10 @@
-   "targets": null,
+@@ -153,10 +139,10 @@
+   },
    "workspace": {
 -    "artifact_path": "/Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev/artifacts",
 -    "file_path": "/Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev/files",
@@ -378,8 +378,8 @@ Resources:
 +        "schema": "project_name_[UNIQUE_NAME]_prod",
          "url": "[DATABRICKS_URL]/pipelines/[UUID]"
        }
-@@ -158,10 +144,10 @@
-   "targets": null,
+@@ -157,10 +143,10 @@
+   },
    "workspace": {
 -    "artifact_path": "/Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev/artifacts",
 -    "file_path": "/Workspace/Users/[USERNAME]/.bundle/project_name_[UNIQUE_NAME]/dev/files",
diff --git a/acceptance/bundle/validate/include_locations/output.dev.json b/acceptance/bundle/validate/include_locations/output.dev.json
@@ -78,18 +78,6 @@
         8,
         15
       ]
-    ],
-    "targets": [
-      [
-        0,
-        9,
-        3
-      ],
-      [
-        1,
-        2,
-        3
-      ]
     ]
   },
   "version": 1
diff --git a/acceptance/bundle/validate/include_locations/output.prod.json b/acceptance/bundle/validate/include_locations/output.prod.json
@@ -78,18 +78,6 @@
         18,
         15
       ]
-    ],
-    "targets": [
-      [
-        0,
-        9,
-        3
-      ],
-      [
-        1,
-        2,
-        3
-      ]
     ]
   },
   "version": 1
diff --git a/acceptance/bundle/validate/job-references/output.txt b/acceptance/bundle/validate/job-references/output.txt
@@ -54,7 +54,6 @@
       "."
     ]
   },
-  "targets": null,
   "workspace": {
     "artifact_path": "/Workspace/Users/[USERNAME]/.bundle/foobar/default/artifacts",
     "current_user": {
diff --git a/acceptance/bundle/variables/git-branch/output.txt b/acceptance/bundle/variables/git-branch/output.txt
@@ -5,7 +5,6 @@
     "environment": "prod",
     "git": {
       "actual_branch": "main",
-      "branch": "",
       "bundle_root_path": "."
     },
     "name": "git",
@@ -16,7 +15,6 @@
       "."
     ]
   },
-  "targets": null,
   "variables": {
     "deployment_branch": {
       "default": "",
@@ -58,7 +56,6 @@ Validation OK!
       "."
     ]
   },
-  "targets": null,
   "variables": {
     "deployment_branch": {
       "default": "dev-branch",
diff --git a/acceptance/bundle/variables/host/output.txt b/acceptance/bundle/variables/host/output.txt
@@ -20,7 +20,6 @@ Error: failed during request visitor: parse "https://${var.host}": invalid chara
       "."
     ]
   },
-  "targets": null,
   "variables": {
     "host": {
       "default": "https://nonexistent123.staging.cloud.databricks.com"
diff --git a/acceptance/bundle/variables/prepend-workspace-var/output.txt b/acceptance/bundle/variables/prepend-workspace-var/output.txt
@@ -52,7 +52,6 @@ Warning: required field "package_name" is not set
       "."
     ]
   },
-  "targets": null,
   "variables": {
     "workspace_root": {
       "default": "/Users/[USERNAME]",
diff --git a/bundle/config/mutator/sync_default_path_test.go b/bundle/config/mutator/sync_default_path_test.go
@@ -34,7 +34,7 @@ func TestSyncDefaultPath_SkipIfSet(t *testing.T) {
 		{
 			name:   "nil",
 			paths:  dyn.V(nil),
-			expect: nil,
+			expect: []string{"."},
 		},
 		{
 			name:   "empty sequence",
diff --git a/bundle/tests/sync_test.go b/bundle/tests/sync_test.go
@@ -112,10 +112,10 @@ func TestSyncPathsNoRoot(t *testing.T) {
 	assert.Equal(t, filepath.FromSlash("sync/paths_no_root"), b.SyncRootPath)
 	assert.Equal(t, []string{"."}, b.Config.Sync.Paths)
 
-	// If set to nil, it won't sync anything.
+	// If set to nil, it's the same as not set, defaults to "."
 	b = loadTarget(t, "./sync/paths_no_root", "nil")
 	assert.Equal(t, filepath.FromSlash("sync/paths_no_root"), b.SyncRootPath)
-	assert.Empty(t, b.Config.Sync.Paths)
+	assert.Equal(t, []string{"."}, b.Config.Sync.Paths)
 
 	// If set to an empty sequence, it won't sync anything.
 	b = loadTarget(t, "./sync/paths_no_root", "empty")
diff --git a/libs/dyn/convert/from_typed.go b/libs/dyn/convert/from_typed.go
@@ -102,6 +102,8 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptio
 	refm, _ := ref.AsMap()
 	out := dyn.NewMapping()
 	info := getStructInfo(src.Type())
+	forceSendFields := getForceSendFields(src)
+
 	for _, fieldval := range info.FieldValues(src) {
 		k := fieldval.Key
 		v := fieldval.Value
@@ -128,6 +130,11 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptio
 
 		// Either if the key was set in the reference or the field is not zero-valued, we include it.
 		if ok || nv.Kind() != dyn.KindNil {
+			goName := info.GolangNames[k]
+			// If v isZero, it could be because it's a variable reference; so we check that nv is zero as well
+			if v.Kind() != reflect.Struct && v.IsZero() && nv.IsZero() && !info.ForceEmpty[k] && !slices.Contains(forceSendFields, goName) {
+				continue
+			}
 			out.SetLoc(k, refloc, nv)
 		}
 	}
@@ -327,3 +334,18 @@ func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOption
 
 	return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind())
 }
+
+func getForceSendFields(v reflect.Value) []string {
+	if !v.IsValid() || v.Kind() != reflect.Struct {
+		return nil
+	}
+	fsField := v.FieldByName("ForceSendFields")
+	if !fsField.IsValid() || fsField.Kind() != reflect.Slice {
+		return nil
+	}
+	result, ok := fsField.Interface().([]string)
+	if ok {
+		return result
+	}
+	return nil
+}
diff --git a/libs/dyn/convert/from_typed_test.go b/libs/dyn/convert/from_typed_test.go
@@ -104,6 +104,54 @@ func TestFromTypedStructSetFields(t *testing.T) {
 	}), nv)
 }
 
+func TestFromTypedStructSetFieldsIntoExistingFields(t *testing.T) {
+	type Tmp struct {
+		Foo string `json:"foo"`
+		Bar string `json:"bar"`
+	}
+
+	src := Tmp{
+		Foo: "foo",
+		Bar: "bar",
+	}
+
+	ref := dyn.V(map[string]dyn.Value{
+		"foo": dyn.V("foo_old"),
+		"bar": dyn.V("bar_old"),
+	})
+
+	nv, err := FromTyped(src, ref)
+	require.NoError(t, err)
+	dynassert.Equal(t, dyn.V(map[string]dyn.Value{
+		"foo": dyn.V("foo"),
+		"bar": dyn.V("bar"),
+	}), nv)
+}
+
+func TestFromTypedStructZeroFieldsIntoExistingFields(t *testing.T) {
+	type Tmp struct {
+		Foo             string   `json:"foo"`
+		Bar             string   `json:"bar,omitempty"`
+		BazBugz         string   `json:"baz_bugs,omitempty"`
+		ForceSendFields []string `json:"-"`
+	}
+
+	src := Tmp{ForceSendFields: []string{"BazBugz"}}
+
+	ref := dyn.V(map[string]dyn.Value{
+		"foo":      dyn.V("foo_old"),
+		"bar":      dyn.V("bar_old"),
+		"baz_bugs": dyn.V("baz_old"),
+	})
+
+	nv, err := FromTyped(src, ref)
+	require.NoError(t, err)
+	dynassert.Equal(t, dyn.V(map[string]dyn.Value{
+		"foo":      dyn.V(""),
+		"baz_bugs": dyn.V(""),
+	}), nv)
+}
+
 func TestFromTypedStructSetFieldsRetainLocation(t *testing.T) {
 	type Tmp struct {
 		Foo string `json:"foo"`
diff --git a/libs/dyn/convert/struct_info.go b/libs/dyn/convert/struct_info.go
@@ -2,10 +2,10 @@ package convert
 
 import (
 	"reflect"
-	"strings"
 	"sync"
 
 	"github.com/databricks/cli/libs/dyn"
+	"github.com/databricks/cli/libs/structdiff/structtag"
 )
 
 // structInfo holds the type information we need to efficiently
@@ -20,6 +20,12 @@ type structInfo struct {
 	// ValueField maps to the field with a [dyn.Value].
 	// The underlying type is expected to only have one of these.
 	ValueField []int
+
+	// Tracks which fields do not have omitempty annotation
+	ForceEmpty map[string]bool
+
+	// Maps JSON-name of the field to Golang struct name
+	GolangNames map[string]string
 }
 
 // structInfoCache caches type information.
@@ -47,7 +53,9 @@ func getStructInfo(typ reflect.Type) structInfo {
 // buildStructInfo populates a new [structInfo] for the given type.
 func buildStructInfo(typ reflect.Type) structInfo {
 	out := structInfo{
-		Fields: make(map[string][]int),
+		Fields:      make(map[string][]int),
+		ForceEmpty:  make(map[string]bool),
+		GolangNames: make(map[string]string),
 	}
 
 	// Queue holds the indexes of the structs to visit.
@@ -86,7 +94,8 @@ func buildStructInfo(typ reflect.Type) structInfo {
 				continue
 			}
 
-			name, _, _ := strings.Cut(sf.Tag.Get("json"), ",")
+			jtag := structtag.JSONTag(sf.Tag.Get("json"))
+			name := jtag.Name()
 			if name == "" || name == "-" {
 				continue
 			}
@@ -99,6 +108,10 @@ func buildStructInfo(typ reflect.Type) structInfo {
 
 			out.FieldNames = append(out.FieldNames, name)
 			out.Fields[name] = append(prefix, sf.Index...)
+			if !jtag.OmitEmpty() && !jtag.OmitZero() {
+				out.ForceEmpty[name] = true
+			}
+			out.GolangNames[name] = sf.Name
 		}
 	}
 
diff --git a/libs/dyn/convert/to_typed.go b/libs/dyn/convert/to_typed.go
diff --git a/libs/dyn/convert/to_typed_test.go b/libs/dyn/convert/to_typed_test.go
diff --git a/libs/dyn/time.go b/libs/dyn/time.go
diff --git a/libs/dyn/value.go b/libs/dyn/value.go

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-musterr $CLI bundle validate -o json \| grep run_as`
	`1`	`+$CLI bundle validate -o json \| grep run_as`
Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-musterr $CLI bundle validate -o json \| grep run_as`
	`1`	`+$CLI bundle validate`