From 6571b0c4f00bdc69996042e1a3596138fa6e2d72 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 10:20:32 +0000 Subject: [PATCH 1/8] Reject empty/control-char names and incomplete pipeline libraries at validate validate --strict previously accepted empty model names, UC/serving names with control characters, and pipeline file: {}. Those fail later with API 400 or invalid-URL errors; fail during initialize instead. --- .nextchanges/bundles/illegal-identifiers.md | 1 + .../catalogs/empty-name/out.test.toml | 4 +- .../resources/catalogs/empty-name/output.txt | 18 +-- .../resources/catalogs/empty-name/script | 5 +- .../resources/catalogs/empty-name/test.toml | 7 +- .../models/empty-name/out.deploy.direct.txt | 11 -- .../empty-name/out.deploy.terraform.txt | 14 --- .../resources/models/empty-name/out.test.toml | 2 +- .../resources/models/empty-name/output.txt | 13 ++ .../bundle/resources/models/empty-name/script | 7 +- .../resources/models/empty-name/test.toml | 3 +- .../empty_resources/empty_dict/output.txt | 24 +++- .../empty_resources/with_grants/output.txt | 24 +++- .../with_permissions/output.txt | 24 +++- .../illegal_identifiers/databricks.yml | 24 ++++ .../illegal_identifiers/out.test.toml | 2 + .../validate/illegal_identifiers/output.txt | 25 ++++ .../validate/illegal_identifiers/script | 3 + .../validate/models/missing_name/output.txt | 6 +- .../bundle/validate/required/databricks.yml | 15 --- .../bundle/validate/required/output.txt | 16 +-- bundle/config/validate/invalid_identifiers.go | 118 ++++++++++++++++++ .../validate/invalid_identifiers_test.go | 92 ++++++++++++++ bundle/config/validate/required.go | 2 + 24 files changed, 359 insertions(+), 101 deletions(-) create mode 100644 .nextchanges/bundles/illegal-identifiers.md delete mode 100644 acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt delete mode 100644 acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt create mode 100644 acceptance/bundle/validate/illegal_identifiers/databricks.yml create mode 100644 acceptance/bundle/validate/illegal_identifiers/out.test.toml create mode 100644 acceptance/bundle/validate/illegal_identifiers/output.txt create mode 100644 acceptance/bundle/validate/illegal_identifiers/script create mode 100644 bundle/config/validate/invalid_identifiers.go create mode 100644 bundle/config/validate/invalid_identifiers_test.go diff --git a/.nextchanges/bundles/illegal-identifiers.md b/.nextchanges/bundles/illegal-identifiers.md new file mode 100644 index 00000000000..880871e5203 --- /dev/null +++ b/.nextchanges/bundles/illegal-identifiers.md @@ -0,0 +1 @@ +# Reject empty/control-char resource names and incomplete pipeline libraries at validate. diff --git a/acceptance/bundle/resources/catalogs/empty-name/out.test.toml b/acceptance/bundle/resources/catalogs/empty-name/out.test.toml index c502b28221b..98ea5040486 100644 --- a/acceptance/bundle/resources/catalogs/empty-name/out.test.toml +++ b/acceptance/bundle/resources/catalogs/empty-name/out.test.toml @@ -1,2 +1,2 @@ -Cloud = true -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/catalogs/empty-name/output.txt b/acceptance/bundle/resources/catalogs/empty-name/output.txt index 51b04630435..b7d3ca5f488 100644 --- a/acceptance/bundle/resources/catalogs/empty-name/output.txt +++ b/acceptance/bundle/resources/catalogs/empty-name/output.txt @@ -1,11 +1,13 @@ ->>> musterr [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/catalog-empty-name-[UNIQUE_NAME]/default/files... -Error: cannot create resources.catalogs.mycatalog: Invalid input: RPC CreateCatalog Field managedcatalog.CatalogInfo.name: name "" is not a valid name. Valid names cannot contain spaces, periods, forward slashes, or control characters. (400 INVALID_PARAMETER_VALUE) +>>> musterr [CLI] bundle validate --strict +Error: catalog name is required + at resources.catalogs.mycatalog + in databricks.yml:7:7 -Endpoint: POST [DATABRICKS_URL]/api/2.1/unity-catalog/catalogs -HTTP Status: 400 Bad Request -API error_code: INVALID_PARAMETER_VALUE -API message: Invalid input: RPC CreateCatalog Field managedcatalog.CatalogInfo.name: name "" is not a valid name. Valid names cannot contain spaces, periods, forward slashes, or control characters. +Name: catalog-empty-name-[UNIQUE_NAME] +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/catalog-empty-name-[UNIQUE_NAME]/default -Files: 5 uploaded, 0 deleted +Found 1 error diff --git a/acceptance/bundle/resources/catalogs/empty-name/script b/acceptance/bundle/resources/catalogs/empty-name/script index dc9e56639a9..fdb05451e99 100644 --- a/acceptance/bundle/resources/catalogs/empty-name/script +++ b/acceptance/bundle/resources/catalogs/empty-name/script @@ -1,5 +1,4 @@ -# The deploy fails only after the files are uploaded, so $UNIQUE_NAME in the bundle -# name keeps concurrent cloud legs apart and lets the sweeper find what is left. +# Empty name is rejected at validate; deploy never reaches the create API. envsubst < databricks.yml.tmpl > databricks.yml -trace musterr $CLI bundle deploy +trace musterr $CLI bundle validate --strict diff --git a/acceptance/bundle/resources/catalogs/empty-name/test.toml b/acceptance/bundle/resources/catalogs/empty-name/test.toml index 458319b24ad..46bb4996368 100644 --- a/acceptance/bundle/resources/catalogs/empty-name/test.toml +++ b/acceptance/bundle/resources/catalogs/empty-name/test.toml @@ -1,8 +1,3 @@ -# The golden asserts UC's message verbatim, so run on cloud to catch it drifting. -Cloud = true +# Empty names fail at validate; no cloud API call is made. RecordRequests = false Ignore = [".databricks"] - -# Terraform rejects catalog resources before any API call, so there is nothing to -# assert on that engine. -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt b/acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt deleted file mode 100644 index 50232dbed92..00000000000 --- a/acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt +++ /dev/null @@ -1,11 +0,0 @@ - ->>> musterr [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/model-empty-name-[UNIQUE_NAME]/default/files... -Error: cannot create resources.models.mymodel: Got an invalid name ''. Registered Model names cannot be empty strings. (400 INVALID_PARAMETER_VALUE) - -Endpoint: POST [DATABRICKS_URL]/api/2.0/mlflow/registered-models/create -HTTP Status: 400 Bad Request -API error_code: INVALID_PARAMETER_VALUE -API message: Got an invalid name ''. Registered Model names cannot be empty strings. - -Files: 6 uploaded, 0 deleted diff --git a/acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt b/acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt deleted file mode 100644 index adb61687bf8..00000000000 --- a/acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt +++ /dev/null @@ -1,14 +0,0 @@ - ->>> musterr [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/model-empty-name-[UNIQUE_NAME]/default/files... -Error: terraform apply: exit status 1 - -Error: cannot create mlflow model: Got an invalid name ''. Registered Model names cannot be empty strings. - - with databricks_mlflow_model.mymodel, - on bundle.tf.json line 17, in resource.databricks_mlflow_model.mymodel: - 17: } - - - -Files: 6 uploaded, 0 deleted diff --git a/acceptance/bundle/resources/models/empty-name/out.test.toml b/acceptance/bundle/resources/models/empty-name/out.test.toml index 2a13818c13f..98ea5040486 100644 --- a/acceptance/bundle/resources/models/empty-name/out.test.toml +++ b/acceptance/bundle/resources/models/empty-name/out.test.toml @@ -1,2 +1,2 @@ -Cloud = true +Cloud = false EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/models/empty-name/output.txt b/acceptance/bundle/resources/models/empty-name/output.txt index e69de29bb2d..12804787fe4 100644 --- a/acceptance/bundle/resources/models/empty-name/output.txt +++ b/acceptance/bundle/resources/models/empty-name/output.txt @@ -0,0 +1,13 @@ + +>>> musterr [CLI] bundle validate --strict +Error: model name is required + at resources.models.mymodel + in databricks.yml:7:7 + +Name: model-empty-name-[UNIQUE_NAME] +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/model-empty-name-[UNIQUE_NAME]/default + +Found 1 error diff --git a/acceptance/bundle/resources/models/empty-name/script b/acceptance/bundle/resources/models/empty-name/script index 0336c9c6ca0..fdb05451e99 100644 --- a/acceptance/bundle/resources/models/empty-name/script +++ b/acceptance/bundle/resources/models/empty-name/script @@ -1,7 +1,4 @@ -# The deploy fails only after the files are uploaded, so $UNIQUE_NAME in the bundle -# name keeps concurrent cloud legs apart and lets the sweeper find what is left. +# Empty name is rejected at validate; deploy never reaches the create API. envsubst < databricks.yml.tmpl > databricks.yml -# Both engines reach the create call, but terraform wraps the message in its own -# output, so the goldens are per-engine. -trace musterr $CLI bundle deploy &> out.deploy.$DATABRICKS_BUNDLE_ENGINE.txt +trace musterr $CLI bundle validate --strict diff --git a/acceptance/bundle/resources/models/empty-name/test.toml b/acceptance/bundle/resources/models/empty-name/test.toml index 64c48c920e6..46bb4996368 100644 --- a/acceptance/bundle/resources/models/empty-name/test.toml +++ b/acceptance/bundle/resources/models/empty-name/test.toml @@ -1,4 +1,3 @@ -# The golden asserts MLflow's message verbatim, so run on cloud to catch it drifting. -Cloud = true +# Empty names fail at validate; no cloud API call is made. RecordRequests = false Ignore = [".databricks"] diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index 0ff8111602e..275c6fef6b2 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -33,7 +33,7 @@ } === resources.models.rname === -Warning: required field "name" is not set +Error: model name is required at resources.models.rname in databricks.yml:6:12 @@ -53,6 +53,18 @@ Warning: required field "name" is not set } === resources.registered_models.rname === +Error: registered_model catalog_name is required + at resources.registered_models.rname + in databricks.yml:6:12 + +Error: registered_model name is required + at resources.registered_models.rname + in databricks.yml:6:12 + +Error: registered_model schema_name is required + at resources.registered_models.rname + in databricks.yml:6:12 + { "registered_models": { "rname": {} @@ -79,11 +91,11 @@ Warning: required field "table_name" is not set } === resources.schemas.rname === -Warning: required field "catalog_name" is not set +Error: schema catalog_name is required at resources.schemas.rname in databricks.yml:6:12 -Warning: required field "name" is not set +Error: schema name is required at resources.schemas.rname in databricks.yml:6:12 @@ -94,15 +106,15 @@ Warning: required field "name" is not set } === resources.volumes.rname === -Warning: required field "catalog_name" is not set +Error: volume catalog_name is required at resources.volumes.rname in databricks.yml:6:12 -Warning: required field "name" is not set +Error: volume name is required at resources.volumes.rname in databricks.yml:6:12 -Warning: required field "schema_name" is not set +Error: volume schema_name is required at resources.volumes.rname in databricks.yml:6:12 diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index 38c4dc55d19..942461ea447 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -45,7 +45,7 @@ Warning: unknown field: grants at resources.models.rname in databricks.yml:7:7 -Warning: required field "name" is not set +Error: model name is required at resources.models.rname in databricks.yml:7:7 @@ -69,6 +69,18 @@ Warning: unknown field: grants } === resources.registered_models.rname === +Error: registered_model catalog_name is required + at resources.registered_models.rname + in databricks.yml:7:7 + +Error: registered_model name is required + at resources.registered_models.rname + in databricks.yml:7:7 + +Error: registered_model schema_name is required + at resources.registered_models.rname + in databricks.yml:7:7 + { "registered_models": { "rname": { @@ -101,11 +113,11 @@ Warning: required field "table_name" is not set } === resources.schemas.rname === -Warning: required field "catalog_name" is not set +Error: schema catalog_name is required at resources.schemas.rname in databricks.yml:7:7 -Warning: required field "name" is not set +Error: schema name is required at resources.schemas.rname in databricks.yml:7:7 @@ -118,15 +130,15 @@ Warning: required field "name" is not set } === resources.volumes.rname === -Warning: required field "catalog_name" is not set +Error: volume catalog_name is required at resources.volumes.rname in databricks.yml:7:7 -Warning: required field "name" is not set +Error: volume name is required at resources.volumes.rname in databricks.yml:7:7 -Warning: required field "schema_name" is not set +Error: volume schema_name is required at resources.volumes.rname in databricks.yml:7:7 diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index cef0b18baa3..ec455f413ca 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -33,7 +33,7 @@ } === resources.models.rname === -Warning: required field "name" is not set +Error: model name is required at resources.models.rname in databricks.yml:7:7 @@ -57,6 +57,18 @@ Warning: unknown field: permissions at resources.registered_models.rname in databricks.yml:7:7 +Error: registered_model catalog_name is required + at resources.registered_models.rname + in databricks.yml:7:7 + +Error: registered_model name is required + at resources.registered_models.rname + in databricks.yml:7:7 + +Error: registered_model schema_name is required + at resources.registered_models.rname + in databricks.yml:7:7 + { "registered_models": { "rname": {} @@ -91,11 +103,11 @@ Warning: unknown field: permissions at resources.schemas.rname in databricks.yml:7:7 -Warning: required field "catalog_name" is not set +Error: schema catalog_name is required at resources.schemas.rname in databricks.yml:7:7 -Warning: required field "name" is not set +Error: schema name is required at resources.schemas.rname in databricks.yml:7:7 @@ -110,15 +122,15 @@ Warning: unknown field: permissions at resources.volumes.rname in databricks.yml:7:7 -Warning: required field "catalog_name" is not set +Error: volume catalog_name is required at resources.volumes.rname in databricks.yml:7:7 -Warning: required field "name" is not set +Error: volume name is required at resources.volumes.rname in databricks.yml:7:7 -Warning: required field "schema_name" is not set +Error: volume schema_name is required at resources.volumes.rname in databricks.yml:7:7 diff --git a/acceptance/bundle/validate/illegal_identifiers/databricks.yml b/acceptance/bundle/validate/illegal_identifiers/databricks.yml new file mode 100644 index 00000000000..1eee2131acd --- /dev/null +++ b/acceptance/bundle/validate/illegal_identifiers/databricks.yml @@ -0,0 +1,24 @@ +bundle: + name: illegal-identifiers + +resources: + models: + empty_model: + name: "" + + volumes: + tab_volume: + name: "tab\there" + catalog_name: main + schema_name: default + volume_type: MANAGED + + model_serving_endpoints: + newline_endpoint: + name: "line1\nline2" + + pipelines: + incomplete_file: + name: incomplete-file + libraries: + - file: {} diff --git a/acceptance/bundle/validate/illegal_identifiers/out.test.toml b/acceptance/bundle/validate/illegal_identifiers/out.test.toml new file mode 100644 index 00000000000..98ea5040486 --- /dev/null +++ b/acceptance/bundle/validate/illegal_identifiers/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/validate/illegal_identifiers/output.txt b/acceptance/bundle/validate/illegal_identifiers/output.txt new file mode 100644 index 00000000000..5431a19b2e2 --- /dev/null +++ b/acceptance/bundle/validate/illegal_identifiers/output.txt @@ -0,0 +1,25 @@ + +>>> [CLI] bundle validate --strict +Error: model name is required + at resources.models.empty_model + in databricks.yml:7:7 + +Error: model_serving_endpoint name must not contain control characters + at resources.model_serving_endpoints.newline_endpoint + in databricks.yml:18:7 + +Error: volume name must not contain control characters + at resources.volumes.tab_volume + in databricks.yml:11:7 + +Error: pipeline library file path is required + at resources.pipelines.incomplete_file.libraries[0].file + in databricks.yml:24:11 + +Name: illegal-identifiers +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/illegal-identifiers/default + +Found 4 errors diff --git a/acceptance/bundle/validate/illegal_identifiers/script b/acceptance/bundle/validate/illegal_identifiers/script new file mode 100644 index 00000000000..a60bf03ac20 --- /dev/null +++ b/acceptance/bundle/validate/illegal_identifiers/script @@ -0,0 +1,3 @@ +# Empty / control-char identifiers and incomplete pipeline libraries must fail +# validate (not only deploy with a late API 400). +musterr trace $CLI bundle validate --strict diff --git a/acceptance/bundle/validate/models/missing_name/output.txt b/acceptance/bundle/validate/models/missing_name/output.txt index 24e6c1d2ca5..8b14513df25 100644 --- a/acceptance/bundle/validate/models/missing_name/output.txt +++ b/acceptance/bundle/validate/models/missing_name/output.txt @@ -1,4 +1,4 @@ -Warning: required field "name" is not set +Error: model name is required at resources.models.mymodel in databricks.yml:6:14 @@ -8,4 +8,6 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 1 warning +Found 1 error + +Exit code: 1 diff --git a/acceptance/bundle/validate/required/databricks.yml b/acceptance/bundle/validate/required/databricks.yml index 82175fb553c..6a56fba028f 100644 --- a/acceptance/bundle/validate/required/databricks.yml +++ b/acceptance/bundle/validate/required/databricks.yml @@ -7,15 +7,6 @@ artifacts: - {} resources: - # Required field name is missing. - models: - my_model_1: - description: "hello" - - # Empty string should not trigger a warning. - my_model_2: - name: "" - jobs: my_job_1: tasks: @@ -27,9 +18,3 @@ resources: # job_id not being set should trigger a warning. - task_key: "task_key2" run_job_task: - - # Catalog name and schema name are required. - # but are not set. - volumes: - my_volume: - name: "baz" diff --git a/acceptance/bundle/validate/required/output.txt b/acceptance/bundle/validate/required/output.txt index ae3e6dad2f8..d47a929e90c 100644 --- a/acceptance/bundle/validate/required/output.txt +++ b/acceptance/bundle/validate/required/output.txt @@ -1,20 +1,8 @@ >>> [CLI] bundle validate -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 - -Warning: required field "schema_name" is not set - at resources.volumes.my_volume - in databricks.yml:35:7 + in databricks.yml:20:24 Warning: required field "source" is not set at artifacts.my_artifact.files[0] @@ -26,4 +14,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 5 warnings +Found 2 warnings diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go new file mode 100644 index 00000000000..dc6f915ecc3 --- /dev/null +++ b/bundle/config/validate/invalid_identifiers.go @@ -0,0 +1,118 @@ +package validate + +import ( + "context" + "fmt" + "strings" + "unicode" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +// errorForInvalidIdentifiers rejects empty and control-character names that the +// backend (or URL layer) rejects with 400 / "invalid control character in URL". +func errorForInvalidIdentifiers(_ context.Context, b *bundle.Bundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + for key, model := range b.Config.Resources.Models { + diags = diags.Extend(identifierDiag(b, "model", "name", model.Name, "resources.models."+key)) + } + for key, catalog := range b.Config.Resources.Catalogs { + diags = diags.Extend(identifierDiag(b, "catalog", "name", catalog.Name, "resources.catalogs."+key)) + } + for key, schema := range b.Config.Resources.Schemas { + path := "resources.schemas." + key + diags = diags.Extend(identifierDiag(b, "schema", "name", schema.Name, path)) + diags = diags.Extend(identifierDiag(b, "schema", "catalog_name", schema.CatalogName, path)) + } + for key, volume := range b.Config.Resources.Volumes { + path := "resources.volumes." + key + diags = diags.Extend(identifierDiag(b, "volume", "name", volume.Name, path)) + diags = diags.Extend(identifierDiag(b, "volume", "catalog_name", volume.CatalogName, path)) + diags = diags.Extend(identifierDiag(b, "volume", "schema_name", volume.SchemaName, path)) + } + for key, loc := range b.Config.Resources.ExternalLocations { + diags = diags.Extend(identifierDiag(b, "external_location", "name", loc.Name, "resources.external_locations."+key)) + } + for key, model := range b.Config.Resources.RegisteredModels { + path := "resources.registered_models." + key + diags = diags.Extend(identifierDiag(b, "registered_model", "name", model.Name, path)) + diags = diags.Extend(identifierDiag(b, "registered_model", "catalog_name", model.CatalogName, path)) + diags = diags.Extend(identifierDiag(b, "registered_model", "schema_name", model.SchemaName, path)) + } + for key, endpoint := range b.Config.Resources.ModelServingEndpoints { + diags = diags.Extend(identifierDiag(b, "model_serving_endpoint", "name", endpoint.Name, "resources.model_serving_endpoints."+key)) + } + + sortDiagnostics(diags) + return diags +} + +// errorForIncompletePipelineLibraries rejects file/notebook/glob entries with no path. +// YAML like `file: {}` unmarshals to a non-nil struct with an empty path; the API +// then returns 400 ("file paths must be set"). +func errorForIncompletePipelineLibraries(_ context.Context, b *bundle.Bundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + for key, pipeline := range b.Config.Resources.Pipelines { + for i, lib := range pipeline.Libraries { + base := fmt.Sprintf("resources.pipelines.%s.libraries[%d]", key, i) + if lib.File != nil && strings.TrimSpace(lib.File.Path) == "" { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "pipeline library file path is required", + Locations: b.Config.GetLocations(base), + Paths: []dyn.Path{dyn.MustPathFromString(base + ".file")}, + }) + } + if lib.Notebook != nil && strings.TrimSpace(lib.Notebook.Path) == "" { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "pipeline library notebook path is required", + Locations: b.Config.GetLocations(base), + Paths: []dyn.Path{dyn.MustPathFromString(base + ".notebook")}, + }) + } + if lib.Glob != nil && strings.TrimSpace(lib.Glob.Include) == "" { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "pipeline library glob include is required", + Locations: b.Config.GetLocations(base), + Paths: []dyn.Path{dyn.MustPathFromString(base + ".glob")}, + }) + } + } + } + + sortDiagnostics(diags) + return diags +} + +func identifierDiag(b *bundle.Bundle, resource, field, value, locPath string) diag.Diagnostics { + reason := invalidIdentifierReason(value) + if reason == "" { + return nil + } + return diag.Diagnostics{{ + Severity: diag.Error, + Summary: fmt.Sprintf("%s %s %s", resource, field, reason), + Locations: b.Config.GetLocations(locPath), + Paths: []dyn.Path{dyn.MustPathFromString(locPath)}, + }} +} + +func invalidIdentifierReason(name string) string { + if strings.TrimSpace(name) == "" { + return "is required" + } + if containsControlCharacter(name) { + return "must not contain control characters" + } + return "" +} + +func containsControlCharacter(s string) bool { + return strings.ContainsFunc(s, unicode.IsControl) +} diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go new file mode 100644 index 00000000000..1f06b0d5a1a --- /dev/null +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -0,0 +1,92 @@ +package validate_test + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/databricks/databricks-sdk-go/service/ml" + "github.com/databricks/databricks-sdk-go/service/pipelines" + "github.com/databricks/databricks-sdk-go/service/serving" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Models: map[string]*resources.MlflowModel{ + "empty": {CreateModelRequest: ml.CreateModelRequest{Name: ""}}, + }, + Volumes: map[string]*resources.Volume{ + "ctrl": { + CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ + Name: "tab\there", + CatalogName: "main", + SchemaName: "default", + VolumeType: catalog.VolumeTypeManaged, + }, + }, + }, + ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ + "nl": { + CreateServingEndpoint: serving.CreateServingEndpoint{ + Name: "line1\nline2", + }, + }, + }, + }, + }, + } + + diags := validate.Required().Apply(t.Context(), b) + require.True(t, diags.HasError()) + + summaries := diagSummaries(diags) + assert.Contains(t, summaries, "model name is required") + assert.Contains(t, summaries, "volume name must not contain control characters") + assert.Contains(t, summaries, "model_serving_endpoint name must not contain control characters") +} + +func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "p": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "p", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{}}, + {Notebook: &pipelines.NotebookLibrary{}}, + {Glob: &pipelines.PathPattern{}}, + {File: &pipelines.FileLibrary{Path: "ok.py"}}, + }, + }, + }, + }, + }, + }, + } + + diags := validate.Required().Apply(t.Context(), b) + require.True(t, diags.HasError()) + + summaries := diagSummaries(diags) + assert.Contains(t, summaries, "pipeline library file path is required") + assert.Contains(t, summaries, "pipeline library notebook path is required") + assert.Contains(t, summaries, "pipeline library glob include is required") +} + +func diagSummaries(diags diag.Diagnostics) []string { + out := make([]string, 0, len(diags)) + for _, d := range diags { + out = append(out, d.Summary) + } + return out +} diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index b886c2c1d73..3e9ad6130cc 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -245,6 +245,8 @@ func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics diags := errorForMissingFields(ctx, b) diags = diags.Extend(errorForInvalidGrants(ctx, b)) diags = diags.Extend(errorForInvalidSecretScopePermissions(ctx, b)) + diags = diags.Extend(errorForInvalidIdentifiers(ctx, b)) + diags = diags.Extend(errorForIncompletePipelineLibraries(ctx, b)) if diags.HasError() { return diags } From 0dbf53567eb49734ed8d8ffd6fb9ffff41401692 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 11:15:14 +0000 Subject: [PATCH 2/8] Address review feedback on invalid identifier validation. Keep omitted UC parent fields as warnings, point diagnostics at the field path, tighten tests, and drop duplicate empty-name acceptance coverage. --- .nextchanges/bundles/illegal-identifiers.md | 1 - .nextchanges/bundles/invalid-identifiers.md | 1 + .../catalogs/empty-name/databricks.yml.tmpl | 7 -- .../resources/catalogs/empty-name/output.txt | 13 ---- .../resources/catalogs/empty-name/script | 4 -- .../resources/catalogs/empty-name/test.toml | 3 - .../models/empty-name/databricks.yml.tmpl | 7 -- .../resources/models/empty-name/out.test.toml | 2 - .../resources/models/empty-name/output.txt | 13 ---- .../bundle/resources/models/empty-name/script | 4 -- .../resources/models/empty-name/test.toml | 3 - .../empty_resources/empty_dict/output.txt | 28 ++------ .../empty_resources/with_grants/output.txt | 28 ++------ .../with_permissions/output.txt | 28 ++------ .../illegal_identifiers/out.test.toml | 2 - .../validate/illegal_identifiers/output.txt | 25 ------- .../validate/illegal_identifiers/script | 3 - .../databricks.yml | 12 +++- .../invalid_identifiers}/out.test.toml | 0 .../validate/invalid_identifiers/output.txt | 33 +++++++++ .../validate/invalid_identifiers/script | 2 + .../validate/models/missing_name/output.txt | 2 +- .../validate/models/user_id/databricks.yml | 1 + .../bundle/validate/models/user_id/output.txt | 8 +-- .../bundle/validate/required/databricks.yml | 5 ++ .../bundle/validate/required/output.txt | 10 ++- .../validate/volume_defaults/databricks.yml | 9 +++ .../validate/volume_defaults/output.txt | 55 ++++----------- bundle/config/validate/invalid_identifiers.go | 58 ++++++++------- .../validate/invalid_identifiers_test.go | 70 ++++++++++++++++--- 30 files changed, 194 insertions(+), 243 deletions(-) delete mode 100644 .nextchanges/bundles/illegal-identifiers.md create mode 100644 .nextchanges/bundles/invalid-identifiers.md delete mode 100644 acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl delete mode 100644 acceptance/bundle/resources/catalogs/empty-name/output.txt delete mode 100644 acceptance/bundle/resources/catalogs/empty-name/script delete mode 100644 acceptance/bundle/resources/catalogs/empty-name/test.toml delete mode 100644 acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl delete mode 100644 acceptance/bundle/resources/models/empty-name/out.test.toml delete mode 100644 acceptance/bundle/resources/models/empty-name/output.txt delete mode 100644 acceptance/bundle/resources/models/empty-name/script delete mode 100644 acceptance/bundle/resources/models/empty-name/test.toml delete mode 100644 acceptance/bundle/validate/illegal_identifiers/out.test.toml delete mode 100644 acceptance/bundle/validate/illegal_identifiers/output.txt delete mode 100644 acceptance/bundle/validate/illegal_identifiers/script rename acceptance/bundle/validate/{illegal_identifiers => invalid_identifiers}/databricks.yml (64%) rename acceptance/bundle/{resources/catalogs/empty-name => validate/invalid_identifiers}/out.test.toml (100%) create mode 100644 acceptance/bundle/validate/invalid_identifiers/output.txt create mode 100644 acceptance/bundle/validate/invalid_identifiers/script diff --git a/.nextchanges/bundles/illegal-identifiers.md b/.nextchanges/bundles/illegal-identifiers.md deleted file mode 100644 index 880871e5203..00000000000 --- a/.nextchanges/bundles/illegal-identifiers.md +++ /dev/null @@ -1 +0,0 @@ -# Reject empty/control-char resource names and incomplete pipeline libraries at validate. diff --git a/.nextchanges/bundles/invalid-identifiers.md b/.nextchanges/bundles/invalid-identifiers.md new file mode 100644 index 00000000000..3e09f53dab9 --- /dev/null +++ b/.nextchanges/bundles/invalid-identifiers.md @@ -0,0 +1 @@ +Reject empty or control-character resource identifiers and incomplete pipeline library paths during bundle validation. diff --git a/acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl b/acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl deleted file mode 100644 index 290f0ef2d0d..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl +++ /dev/null @@ -1,7 +0,0 @@ -bundle: - name: catalog-empty-name-$UNIQUE_NAME - -resources: - catalogs: - mycatalog: - name: "" diff --git a/acceptance/bundle/resources/catalogs/empty-name/output.txt b/acceptance/bundle/resources/catalogs/empty-name/output.txt deleted file mode 100644 index b7d3ca5f488..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/output.txt +++ /dev/null @@ -1,13 +0,0 @@ - ->>> musterr [CLI] bundle validate --strict -Error: catalog name is required - at resources.catalogs.mycatalog - in databricks.yml:7:7 - -Name: catalog-empty-name-[UNIQUE_NAME] -Target: default -Workspace: - User: [USERNAME] - Path: /Workspace/Users/[USERNAME]/.bundle/catalog-empty-name-[UNIQUE_NAME]/default - -Found 1 error diff --git a/acceptance/bundle/resources/catalogs/empty-name/script b/acceptance/bundle/resources/catalogs/empty-name/script deleted file mode 100644 index fdb05451e99..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/script +++ /dev/null @@ -1,4 +0,0 @@ -# Empty name is rejected at validate; deploy never reaches the create API. -envsubst < databricks.yml.tmpl > databricks.yml - -trace musterr $CLI bundle validate --strict diff --git a/acceptance/bundle/resources/catalogs/empty-name/test.toml b/acceptance/bundle/resources/catalogs/empty-name/test.toml deleted file mode 100644 index 46bb4996368..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/test.toml +++ /dev/null @@ -1,3 +0,0 @@ -# Empty names fail at validate; no cloud API call is made. -RecordRequests = false -Ignore = [".databricks"] diff --git a/acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl b/acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl deleted file mode 100644 index a5fd377721f..00000000000 --- a/acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl +++ /dev/null @@ -1,7 +0,0 @@ -bundle: - name: model-empty-name-$UNIQUE_NAME - -resources: - models: - mymodel: - name: "" diff --git a/acceptance/bundle/resources/models/empty-name/out.test.toml b/acceptance/bundle/resources/models/empty-name/out.test.toml deleted file mode 100644 index 98ea5040486..00000000000 --- a/acceptance/bundle/resources/models/empty-name/out.test.toml +++ /dev/null @@ -1,2 +0,0 @@ -Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/models/empty-name/output.txt b/acceptance/bundle/resources/models/empty-name/output.txt deleted file mode 100644 index 12804787fe4..00000000000 --- a/acceptance/bundle/resources/models/empty-name/output.txt +++ /dev/null @@ -1,13 +0,0 @@ - ->>> musterr [CLI] bundle validate --strict -Error: model name is required - at resources.models.mymodel - in databricks.yml:7:7 - -Name: model-empty-name-[UNIQUE_NAME] -Target: default -Workspace: - User: [USERNAME] - Path: /Workspace/Users/[USERNAME]/.bundle/model-empty-name-[UNIQUE_NAME]/default - -Found 1 error diff --git a/acceptance/bundle/resources/models/empty-name/script b/acceptance/bundle/resources/models/empty-name/script deleted file mode 100644 index fdb05451e99..00000000000 --- a/acceptance/bundle/resources/models/empty-name/script +++ /dev/null @@ -1,4 +0,0 @@ -# Empty name is rejected at validate; deploy never reaches the create API. -envsubst < databricks.yml.tmpl > databricks.yml - -trace musterr $CLI bundle validate --strict diff --git a/acceptance/bundle/resources/models/empty-name/test.toml b/acceptance/bundle/resources/models/empty-name/test.toml deleted file mode 100644 index 46bb4996368..00000000000 --- a/acceptance/bundle/resources/models/empty-name/test.toml +++ /dev/null @@ -1,3 +0,0 @@ -# Empty names fail at validate; no cloud API call is made. -RecordRequests = false -Ignore = [".databricks"] diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index 275c6fef6b2..f716dee13e7 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -34,7 +34,7 @@ === resources.models.rname === Error: model name is required - at resources.models.rname + at resources.models.rname.name in databricks.yml:6:12 { @@ -53,16 +53,8 @@ Error: model name is required } === resources.registered_models.rname === -Error: registered_model catalog_name is required - at resources.registered_models.rname - in databricks.yml:6:12 - Error: registered_model name is required - at resources.registered_models.rname - in databricks.yml:6:12 - -Error: registered_model schema_name is required - at resources.registered_models.rname + at resources.registered_models.rname.name in databricks.yml:6:12 { @@ -91,12 +83,8 @@ Warning: required field "table_name" is not set } === resources.schemas.rname === -Error: schema catalog_name is required - at resources.schemas.rname - in databricks.yml:6:12 - Error: schema name is required - at resources.schemas.rname + at resources.schemas.rname.name in databricks.yml:6:12 { @@ -106,16 +94,8 @@ Error: schema name is required } === resources.volumes.rname === -Error: volume catalog_name is required - at resources.volumes.rname - in databricks.yml:6:12 - Error: volume name is required - at resources.volumes.rname - in databricks.yml:6:12 - -Error: volume schema_name is required - at resources.volumes.rname + at resources.volumes.rname.name in databricks.yml:6:12 { diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index 942461ea447..dc73fcdb8b0 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -46,7 +46,7 @@ Warning: unknown field: grants in databricks.yml:7:7 Error: model name is required - at resources.models.rname + at resources.models.rname.name in databricks.yml:7:7 { @@ -69,16 +69,8 @@ Warning: unknown field: grants } === resources.registered_models.rname === -Error: registered_model catalog_name is required - at resources.registered_models.rname - in databricks.yml:7:7 - Error: registered_model name is required - at resources.registered_models.rname - in databricks.yml:7:7 - -Error: registered_model schema_name is required - at resources.registered_models.rname + at resources.registered_models.rname.name in databricks.yml:7:7 { @@ -113,12 +105,8 @@ Warning: required field "table_name" is not set } === resources.schemas.rname === -Error: schema catalog_name is required - at resources.schemas.rname - in databricks.yml:7:7 - Error: schema name is required - at resources.schemas.rname + at resources.schemas.rname.name in databricks.yml:7:7 { @@ -130,16 +118,8 @@ Error: schema name is required } === resources.volumes.rname === -Error: volume catalog_name is required - at resources.volumes.rname - in databricks.yml:7:7 - Error: volume name is required - at resources.volumes.rname - in databricks.yml:7:7 - -Error: volume schema_name is required - at resources.volumes.rname + at resources.volumes.rname.name in databricks.yml:7:7 { diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index ec455f413ca..a822b585303 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -34,7 +34,7 @@ === resources.models.rname === Error: model name is required - at resources.models.rname + at resources.models.rname.name in databricks.yml:7:7 { @@ -57,16 +57,8 @@ Warning: unknown field: permissions at resources.registered_models.rname in databricks.yml:7:7 -Error: registered_model catalog_name is required - at resources.registered_models.rname - in databricks.yml:7:7 - Error: registered_model name is required - at resources.registered_models.rname - in databricks.yml:7:7 - -Error: registered_model schema_name is required - at resources.registered_models.rname + at resources.registered_models.rname.name in databricks.yml:7:7 { @@ -103,12 +95,8 @@ Warning: unknown field: permissions at resources.schemas.rname in databricks.yml:7:7 -Error: schema catalog_name is required - at resources.schemas.rname - in databricks.yml:7:7 - Error: schema name is required - at resources.schemas.rname + at resources.schemas.rname.name in databricks.yml:7:7 { @@ -122,16 +110,8 @@ Warning: unknown field: permissions at resources.volumes.rname in databricks.yml:7:7 -Error: volume catalog_name is required - at resources.volumes.rname - in databricks.yml:7:7 - Error: volume name is required - at resources.volumes.rname - in databricks.yml:7:7 - -Error: volume schema_name is required - at resources.volumes.rname + at resources.volumes.rname.name in databricks.yml:7:7 { diff --git a/acceptance/bundle/validate/illegal_identifiers/out.test.toml b/acceptance/bundle/validate/illegal_identifiers/out.test.toml deleted file mode 100644 index 98ea5040486..00000000000 --- a/acceptance/bundle/validate/illegal_identifiers/out.test.toml +++ /dev/null @@ -1,2 +0,0 @@ -Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/validate/illegal_identifiers/output.txt b/acceptance/bundle/validate/illegal_identifiers/output.txt deleted file mode 100644 index 5431a19b2e2..00000000000 --- a/acceptance/bundle/validate/illegal_identifiers/output.txt +++ /dev/null @@ -1,25 +0,0 @@ - ->>> [CLI] bundle validate --strict -Error: model name is required - at resources.models.empty_model - in databricks.yml:7:7 - -Error: model_serving_endpoint name must not contain control characters - at resources.model_serving_endpoints.newline_endpoint - in databricks.yml:18:7 - -Error: volume name must not contain control characters - at resources.volumes.tab_volume - in databricks.yml:11:7 - -Error: pipeline library file path is required - at resources.pipelines.incomplete_file.libraries[0].file - in databricks.yml:24:11 - -Name: illegal-identifiers -Target: default -Workspace: - User: [USERNAME] - Path: /Workspace/Users/[USERNAME]/.bundle/illegal-identifiers/default - -Found 4 errors diff --git a/acceptance/bundle/validate/illegal_identifiers/script b/acceptance/bundle/validate/illegal_identifiers/script deleted file mode 100644 index a60bf03ac20..00000000000 --- a/acceptance/bundle/validate/illegal_identifiers/script +++ /dev/null @@ -1,3 +0,0 @@ -# Empty / control-char identifiers and incomplete pipeline libraries must fail -# validate (not only deploy with a late API 400). -musterr trace $CLI bundle validate --strict diff --git a/acceptance/bundle/validate/illegal_identifiers/databricks.yml b/acceptance/bundle/validate/invalid_identifiers/databricks.yml similarity index 64% rename from acceptance/bundle/validate/illegal_identifiers/databricks.yml rename to acceptance/bundle/validate/invalid_identifiers/databricks.yml index 1eee2131acd..80b542a1cdf 100644 --- a/acceptance/bundle/validate/illegal_identifiers/databricks.yml +++ b/acceptance/bundle/validate/invalid_identifiers/databricks.yml @@ -1,11 +1,15 @@ bundle: - name: illegal-identifiers + name: invalid-identifiers resources: models: empty_model: name: "" + catalogs: + empty_catalog: + name: "" + volumes: tab_volume: name: "tab\there" @@ -13,6 +17,12 @@ resources: schema_name: default volume_type: MANAGED + tab_catalog: + name: valid + catalog_name: "main\tcatalog" + schema_name: default + volume_type: MANAGED + model_serving_endpoints: newline_endpoint: name: "line1\nline2" diff --git a/acceptance/bundle/resources/catalogs/empty-name/out.test.toml b/acceptance/bundle/validate/invalid_identifiers/out.test.toml similarity index 100% rename from acceptance/bundle/resources/catalogs/empty-name/out.test.toml rename to acceptance/bundle/validate/invalid_identifiers/out.test.toml diff --git a/acceptance/bundle/validate/invalid_identifiers/output.txt b/acceptance/bundle/validate/invalid_identifiers/output.txt new file mode 100644 index 00000000000..af2ffb78797 --- /dev/null +++ b/acceptance/bundle/validate/invalid_identifiers/output.txt @@ -0,0 +1,33 @@ + +>>> musterr [CLI] bundle validate +Error: catalog name is required + at resources.catalogs.empty_catalog.name + in databricks.yml:11:13 + +Error: model name is required + at resources.models.empty_model.name + in databricks.yml:7:13 + +Error: model_serving_endpoint name must not contain control characters + at resources.model_serving_endpoints.newline_endpoint.name + in databricks.yml:28:13 + +Error: volume catalog_name must not contain control characters + at resources.volumes.tab_catalog.catalog_name + in databricks.yml:22:21 + +Error: volume name must not contain control characters + at resources.volumes.tab_volume.name + in databricks.yml:15:13 + +Error: pipeline library file path is required + at resources.pipelines.incomplete_file.libraries[0].file + in databricks.yml:34:11 + +Name: invalid-identifiers +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/invalid-identifiers/default + +Found 6 errors diff --git a/acceptance/bundle/validate/invalid_identifiers/script b/acceptance/bundle/validate/invalid_identifiers/script new file mode 100644 index 00000000000..3d3b920843f --- /dev/null +++ b/acceptance/bundle/validate/invalid_identifiers/script @@ -0,0 +1,2 @@ +# Catch invalid values before deployment reaches the API. +trace musterr $CLI bundle validate diff --git a/acceptance/bundle/validate/models/missing_name/output.txt b/acceptance/bundle/validate/models/missing_name/output.txt index 8b14513df25..dcbbaa92615 100644 --- a/acceptance/bundle/validate/models/missing_name/output.txt +++ b/acceptance/bundle/validate/models/missing_name/output.txt @@ -1,5 +1,5 @@ Error: model name is required - at resources.models.mymodel + at resources.models.mymodel.name in databricks.yml:6:14 Name: test-bundle diff --git a/acceptance/bundle/validate/models/user_id/databricks.yml b/acceptance/bundle/validate/models/user_id/databricks.yml index 61fc1746331..d34c78144bd 100644 --- a/acceptance/bundle/validate/models/user_id/databricks.yml +++ b/acceptance/bundle/validate/models/user_id/databricks.yml @@ -4,4 +4,5 @@ bundle: resources: models: mymodel: + name: mymodel user_id: 123 diff --git a/acceptance/bundle/validate/models/user_id/output.txt b/acceptance/bundle/validate/models/user_id/output.txt index 95c7ddde0e5..ec9c10f52e3 100644 --- a/acceptance/bundle/validate/models/user_id/output.txt +++ b/acceptance/bundle/validate/models/user_id/output.txt @@ -1,10 +1,6 @@ Warning: unknown field: user_id at resources.models.mymodel - in databricks.yml:7:7 - -Warning: required field "name" is not set - at resources.models.mymodel - in databricks.yml:7:7 + in databricks.yml:8:7 Name: test-bundle Target: default @@ -12,4 +8,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 2 warnings +Found 1 warning diff --git a/acceptance/bundle/validate/required/databricks.yml b/acceptance/bundle/validate/required/databricks.yml index 6a56fba028f..41d22c9bdda 100644 --- a/acceptance/bundle/validate/required/databricks.yml +++ b/acceptance/bundle/validate/required/databricks.yml @@ -18,3 +18,8 @@ resources: # job_id not being set should trigger a warning. - task_key: "task_key2" run_job_task: + + # Catalog name and schema name are required. + volumes: + my_volume: + name: "baz" diff --git a/acceptance/bundle/validate/required/output.txt b/acceptance/bundle/validate/required/output.txt index d47a929e90c..13fe5147375 100644 --- a/acceptance/bundle/validate/required/output.txt +++ b/acceptance/bundle/validate/required/output.txt @@ -1,9 +1,17 @@ >>> [CLI] bundle validate +Warning: required field "catalog_name" is not set + at resources.volumes.my_volume + in databricks.yml:25:7 + Warning: required field "job_id" is not set at resources.jobs.my_job_1.tasks[1].run_job_task in databricks.yml:20:24 +Warning: required field "schema_name" is not set + at resources.volumes.my_volume + in databricks.yml:25:7 + Warning: required field "source" is not set at artifacts.my_artifact.files[0] in databricks.yml:7:9 @@ -14,4 +22,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 2 warnings +Found 4 warnings diff --git a/acceptance/bundle/validate/volume_defaults/databricks.yml b/acceptance/bundle/validate/volume_defaults/databricks.yml index 159db5c7511..9fb56a3a966 100644 --- a/acceptance/bundle/validate/volume_defaults/databricks.yml +++ b/acceptance/bundle/validate/volume_defaults/databricks.yml @@ -4,10 +4,19 @@ bundle: resources: volumes: v1: + name: v1 + catalog_name: main + schema_name: default volume_type: "" v2: + name: v2 + catalog_name: main + schema_name: default volume_type: "already-set" v3: + name: v3 + catalog_name: main + schema_name: default comment: hello diff --git a/acceptance/bundle/validate/volume_defaults/output.txt b/acceptance/bundle/validate/volume_defaults/output.txt index ecdbd7ae7b7..d92703ffb69 100644 --- a/acceptance/bundle/validate/volume_defaults/output.txt +++ b/acceptance/bundle/validate/volume_defaults/output.txt @@ -1,59 +1,32 @@ -Warning: required field "catalog_name" is not set - at resources.volumes.v2 - in databricks.yml:10:7 - -Warning: required field "catalog_name" is not set - at resources.volumes.v3 - in databricks.yml:13:7 - -Warning: required field "catalog_name" is not set - at resources.volumes.v1 - in databricks.yml:7:7 - -Warning: required field "name" is not set - at resources.volumes.v2 - in databricks.yml:10:7 - -Warning: required field "name" is not set - at resources.volumes.v3 - in databricks.yml:13:7 - -Warning: required field "name" is not set - at resources.volumes.v1 - in databricks.yml:7:7 - -Warning: required field "schema_name" is not set - at resources.volumes.v2 - in databricks.yml:10:7 - -Warning: required field "schema_name" is not set - at resources.volumes.v3 - in databricks.yml:13:7 - -Warning: required field "schema_name" is not set - at resources.volumes.v1 - in databricks.yml:7:7 - Warning: invalid value "" for enum field. Valid values are [EXTERNAL MANAGED] at resources.volumes.v1.volume_type - in databricks.yml:7:20 + in databricks.yml:10:20 Warning: invalid value "already-set" for enum field. Valid values are [EXTERNAL MANAGED] at resources.volumes.v2.volume_type - in databricks.yml:10:20 + in databricks.yml:16:20 { "v1": { - "volume_path": "/Volumes///", + "catalog_name": "main", + "name": "v1", + "schema_name": "default", + "volume_path": "/Volumes/main/default/v1", "volume_type": "" }, "v2": { - "volume_path": "/Volumes///", + "catalog_name": "main", + "name": "v2", + "schema_name": "default", + "volume_path": "/Volumes/main/default/v2", "volume_type": "already-set" }, "v3": { + "catalog_name": "main", "comment": "hello", - "volume_path": "/Volumes///", + "name": "v3", + "schema_name": "default", + "volume_path": "/Volumes/main/default/v3", "volume_type": "MANAGED" } } diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go index dc6f915ecc3..b722be24a4e 100644 --- a/bundle/config/validate/invalid_identifiers.go +++ b/bundle/config/validate/invalid_identifiers.go @@ -11,48 +11,45 @@ import ( "github.com/databricks/cli/libs/dyn" ) -// errorForInvalidIdentifiers rejects empty and control-character names that the -// backend (or URL layer) rejects with 400 / "invalid control character in URL". +// errorForInvalidIdentifiers rejects empty, blank, or control-character identifiers. func errorForInvalidIdentifiers(_ context.Context, b *bundle.Bundle) diag.Diagnostics { diags := diag.Diagnostics{} for key, model := range b.Config.Resources.Models { - diags = diags.Extend(identifierDiag(b, "model", "name", model.Name, "resources.models."+key)) + diags = diags.Extend(identifierDiag(b, "model", "name", model.Name, "resources.models."+key, true)) } for key, catalog := range b.Config.Resources.Catalogs { - diags = diags.Extend(identifierDiag(b, "catalog", "name", catalog.Name, "resources.catalogs."+key)) + diags = diags.Extend(identifierDiag(b, "catalog", "name", catalog.Name, "resources.catalogs."+key, true)) } for key, schema := range b.Config.Resources.Schemas { path := "resources.schemas." + key - diags = diags.Extend(identifierDiag(b, "schema", "name", schema.Name, path)) - diags = diags.Extend(identifierDiag(b, "schema", "catalog_name", schema.CatalogName, path)) + diags = diags.Extend(identifierDiag(b, "schema", "name", schema.Name, path, true)) + diags = diags.Extend(identifierDiag(b, "schema", "catalog_name", schema.CatalogName, path, false)) } for key, volume := range b.Config.Resources.Volumes { path := "resources.volumes." + key - diags = diags.Extend(identifierDiag(b, "volume", "name", volume.Name, path)) - diags = diags.Extend(identifierDiag(b, "volume", "catalog_name", volume.CatalogName, path)) - diags = diags.Extend(identifierDiag(b, "volume", "schema_name", volume.SchemaName, path)) + diags = diags.Extend(identifierDiag(b, "volume", "name", volume.Name, path, true)) + diags = diags.Extend(identifierDiag(b, "volume", "catalog_name", volume.CatalogName, path, false)) + diags = diags.Extend(identifierDiag(b, "volume", "schema_name", volume.SchemaName, path, false)) } for key, loc := range b.Config.Resources.ExternalLocations { - diags = diags.Extend(identifierDiag(b, "external_location", "name", loc.Name, "resources.external_locations."+key)) + diags = diags.Extend(identifierDiag(b, "external_location", "name", loc.Name, "resources.external_locations."+key, true)) } for key, model := range b.Config.Resources.RegisteredModels { path := "resources.registered_models." + key - diags = diags.Extend(identifierDiag(b, "registered_model", "name", model.Name, path)) - diags = diags.Extend(identifierDiag(b, "registered_model", "catalog_name", model.CatalogName, path)) - diags = diags.Extend(identifierDiag(b, "registered_model", "schema_name", model.SchemaName, path)) + diags = diags.Extend(identifierDiag(b, "registered_model", "name", model.Name, path, true)) + diags = diags.Extend(identifierDiag(b, "registered_model", "catalog_name", model.CatalogName, path, false)) + diags = diags.Extend(identifierDiag(b, "registered_model", "schema_name", model.SchemaName, path, false)) } for key, endpoint := range b.Config.Resources.ModelServingEndpoints { - diags = diags.Extend(identifierDiag(b, "model_serving_endpoint", "name", endpoint.Name, "resources.model_serving_endpoints."+key)) + diags = diags.Extend(identifierDiag(b, "model_serving_endpoint", "name", endpoint.Name, "resources.model_serving_endpoints."+key, true)) } sortDiagnostics(diags) return diags } -// errorForIncompletePipelineLibraries rejects file/notebook/glob entries with no path. -// YAML like `file: {}` unmarshals to a non-nil struct with an empty path; the API -// then returns 400 ("file paths must be set"). +// errorForIncompletePipelineLibraries rejects file, notebook, and glob entries without paths. func errorForIncompletePipelineLibraries(_ context.Context, b *bundle.Bundle) diag.Diagnostics { diags := diag.Diagnostics{} @@ -90,26 +87,39 @@ func errorForIncompletePipelineLibraries(_ context.Context, b *bundle.Bundle) di return diags } -func identifierDiag(b *bundle.Bundle, resource, field, value, locPath string) diag.Diagnostics { - reason := invalidIdentifierReason(value) +func identifierDiag(b *bundle.Bundle, resource, field, value, resourcePath string, required bool) diag.Diagnostics { + reason := invalidIdentifierReason(value, required) if reason == "" { return nil } + + fieldPath := resourcePath + "." + field + locations := b.Config.GetLocations(fieldPath) + if len(locations) == 0 { + locations = b.Config.GetLocations(resourcePath) + } + return diag.Diagnostics{{ Severity: diag.Error, Summary: fmt.Sprintf("%s %s %s", resource, field, reason), - Locations: b.Config.GetLocations(locPath), - Paths: []dyn.Path{dyn.MustPathFromString(locPath)}, + Locations: locations, + Paths: []dyn.Path{dyn.MustPathFromString(fieldPath)}, }} } -func invalidIdentifierReason(name string) string { - if strings.TrimSpace(name) == "" { +func invalidIdentifierReason(value string, required bool) string { + if value == "" { + if !required { + return "" + } return "is required" } - if containsControlCharacter(name) { + if containsControlCharacter(value) { return "must not contain control characters" } + if strings.TrimSpace(value) == "" { + return "must not be blank" + } return "" } diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go index 1f06b0d5a1a..b52aacbb166 100644 --- a/bundle/config/validate/invalid_identifiers_test.go +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -23,6 +23,9 @@ func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { Models: map[string]*resources.MlflowModel{ "empty": {CreateModelRequest: ml.CreateModelRequest{Name: ""}}, }, + Catalogs: map[string]*resources.Catalog{ + "blank": {CreateCatalog: catalog.CreateCatalog{Name: " "}}, + }, Volumes: map[string]*resources.Volume{ "ctrl": { CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ @@ -44,13 +47,15 @@ func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { }, } - diags := validate.Required().Apply(t.Context(), b) + diags := bundle.Apply(t.Context(), b, validate.Required()) require.True(t, diags.HasError()) - summaries := diagSummaries(diags) - assert.Contains(t, summaries, "model name is required") - assert.Contains(t, summaries, "volume name must not contain control characters") - assert.Contains(t, summaries, "model_serving_endpoint name must not contain control characters") + assert.ElementsMatch(t, []string{ + "catalog name must not be blank", + "model name is required", + "volume name must not contain control characters", + "model_serving_endpoint name must not contain control characters", + }, diagSummaries(diags)) } func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { @@ -74,13 +79,58 @@ func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { }, } - diags := validate.Required().Apply(t.Context(), b) + diags := bundle.Apply(t.Context(), b, validate.Required()) require.True(t, diags.HasError()) - summaries := diagSummaries(diags) - assert.Contains(t, summaries, "pipeline library file path is required") - assert.Contains(t, summaries, "pipeline library notebook path is required") - assert.Contains(t, summaries, "pipeline library glob include is required") + assert.ElementsMatch(t, []string{ + "pipeline library file path is required", + "pipeline library notebook path is required", + "pipeline library glob include is required", + }, diagSummaries(diags)) +} + +func TestRequiredAcceptsValidIdentifiersAndPipelineLibraries(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Models: map[string]*resources.MlflowModel{ + "model": {CreateModelRequest: ml.CreateModelRequest{Name: "model"}}, + }, + Pipelines: map[string]*resources.Pipeline{ + "pipeline": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "pipeline", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{Path: "file.py"}}, + {Notebook: &pipelines.NotebookLibrary{Path: "notebook.py"}}, + {Glob: &pipelines.PathPattern{Include: "src/**"}}, + }, + }, + }, + }, + }, + }, + } + + assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) +} + +func TestRequiredAcceptsMissingOptionalUCParents(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + RegisteredModels: map[string]*resources.RegisteredModel{ + "model": { + CreateRegisteredModelRequest: catalog.CreateRegisteredModelRequest{ + Name: "model", + }, + }, + }, + }, + }, + } + + assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) } func diagSummaries(diags diag.Diagnostics) []string { From 6739060cbb849d10c814938f4a5da2dbeba0bb20 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 11:53:59 +0000 Subject: [PATCH 3/8] Fix panic and widen coverage in invalid identifier validation Build diagnostic paths structurally with dyn.NewPath instead of parsing concatenated strings: a resource key containing a path metacharacter (e.g. "weird[0]key") made MustPathFromString panic during bundle validate. Drive the identifier checks off generated.RequiredFields rather than a hand-picked list of resource types, so control characters and blank values are caught consistently (vector search endpoints, apps, experiments and others were previously silent). Explicit empty strings on UC parent fields are now rejected too; omitted parents keep warning. Stop short-circuiting before warnForMissingFields so one validate run reports every issue, and name the offending character in the diagnostic detail since control characters are invisible in terminal output. --- .nextchanges/bundles/invalid-identifiers.md | 4 +- .../empty_resources/empty_dict/output.txt | 29 ++- .../empty_resources/with_grants/output.txt | 29 ++- .../with_permissions/output.txt | 29 ++- .../invalid_identifiers/databricks.yml | 15 ++ .../invalid_identifiers/out.test.toml | 2 +- .../validate/invalid_identifiers/output.txt | 26 ++- .../validate/invalid_identifiers/test.toml | 3 + bundle/config/validate/invalid_identifiers.go | 212 ++++++++++++------ .../validate/invalid_identifiers_test.go | 102 +++++++++ bundle/config/validate/required.go | 8 +- 11 files changed, 348 insertions(+), 111 deletions(-) create mode 100644 acceptance/bundle/validate/invalid_identifiers/test.toml diff --git a/.nextchanges/bundles/invalid-identifiers.md b/.nextchanges/bundles/invalid-identifiers.md index 3e09f53dab9..796251a3d74 100644 --- a/.nextchanges/bundles/invalid-identifiers.md +++ b/.nextchanges/bundles/invalid-identifiers.md @@ -1 +1,3 @@ -Reject empty or control-character resource identifiers and incomplete pipeline library paths during bundle validation. +Reject empty, blank, or control-character resource identifiers and incomplete pipeline library paths during bundle validation. + +Configs that previously only warned on missing names (for example models and apps) now fail validate. Explicit empty strings for UC parent fields such as catalog_name are rejected; omitted parents still warn. diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index f716dee13e7..9918a43061a 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -87,6 +87,10 @@ Error: schema name is required at resources.schemas.rname.name in databricks.yml:6:12 +Warning: required field "catalog_name" is not set + at resources.schemas.rname + in databricks.yml:6:12 + { "schemas": { "rname": {} @@ -98,6 +102,14 @@ Error: volume name is required at resources.volumes.rname.name in databricks.yml:6:12 +Warning: required field "catalog_name" is not set + at resources.volumes.rname + in databricks.yml:6:12 + +Warning: required field "schema_name" is not set + at resources.volumes.rname + in databricks.yml:6:12 + { "volumes": { "rname": { @@ -135,15 +147,10 @@ Error: dashboard warehouse_id is required } === resources.apps.rname === -Warning: required field "name" is not set - at resources.apps.rname - in databricks.yml:6:12 - -Error: Missing app source code path or git source +Error: app name is required + at resources.apps.rname.name in databricks.yml:6:12 -app resource 'rname' should have either source_code_path or git_source field - { "apps": { "rname": { @@ -169,8 +176,8 @@ Error: sql_warehouse name is required } === resources.secret_scopes.rname === -Warning: required field "name" is not set - at resources.secret_scopes.rname +Error: secret_scope name is required + at resources.secret_scopes.rname.name in databricks.yml:6:12 { @@ -180,8 +187,8 @@ Warning: required field "name" is not set } === resources.alerts.rname === -Warning: required field "display_name" is not set - at resources.alerts.rname +Error: alert display_name is required + at resources.alerts.rname.display_name in databricks.yml:6:12 Warning: required field "evaluation" is not set diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index dc73fcdb8b0..8f80d07a8ae 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -109,6 +109,10 @@ Error: schema name is required at resources.schemas.rname.name in databricks.yml:7:7 +Warning: required field "catalog_name" is not set + at resources.schemas.rname + in databricks.yml:7:7 + { "schemas": { "rname": { @@ -122,6 +126,14 @@ Error: volume name is required at resources.volumes.rname.name in databricks.yml:7:7 +Warning: required field "catalog_name" is not set + at resources.volumes.rname + in databricks.yml:7:7 + +Warning: required field "schema_name" is not set + at resources.volumes.rname + in databricks.yml:7:7 + { "volumes": { "rname": { @@ -172,15 +184,10 @@ Warning: unknown field: grants at resources.apps.rname in databricks.yml:7:7 -Warning: required field "name" is not set - at resources.apps.rname - in databricks.yml:7:7 - -Error: Missing app source code path or git source +Error: app name is required + at resources.apps.rname.name in databricks.yml:7:7 -app resource 'rname' should have either source_code_path or git_source field - { "apps": { "rname": { @@ -214,8 +221,8 @@ Warning: unknown field: grants at resources.secret_scopes.rname in databricks.yml:7:7 -Warning: required field "name" is not set - at resources.secret_scopes.rname +Error: secret_scope name is required + at resources.secret_scopes.rname.name in databricks.yml:7:7 { @@ -229,8 +236,8 @@ Warning: unknown field: grants at resources.alerts.rname in databricks.yml:7:7 -Warning: required field "display_name" is not set - at resources.alerts.rname +Error: alert display_name is required + at resources.alerts.rname.display_name in databricks.yml:7:7 Warning: required field "evaluation" is not set diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index a822b585303..7cdc4040d3b 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -99,6 +99,10 @@ Error: schema name is required at resources.schemas.rname.name in databricks.yml:7:7 +Warning: required field "catalog_name" is not set + at resources.schemas.rname + in databricks.yml:7:7 + { "schemas": { "rname": {} @@ -114,6 +118,14 @@ Error: volume name is required at resources.volumes.rname.name in databricks.yml:7:7 +Warning: required field "catalog_name" is not set + at resources.volumes.rname + in databricks.yml:7:7 + +Warning: required field "schema_name" is not set + at resources.volumes.rname + in databricks.yml:7:7 + { "volumes": { "rname": { @@ -151,15 +163,10 @@ Error: dashboard warehouse_id is required } === resources.apps.rname === -Warning: required field "name" is not set - at resources.apps.rname - in databricks.yml:7:7 - -Error: Missing app source code path or git source +Error: app name is required + at resources.apps.rname.name in databricks.yml:7:7 -app resource 'rname' should have either source_code_path or git_source field - { "apps": { "rname": { @@ -185,8 +192,8 @@ Error: sql_warehouse name is required } === resources.secret_scopes.rname === -Warning: required field "name" is not set - at resources.secret_scopes.rname +Error: secret_scope name is required + at resources.secret_scopes.rname.name in databricks.yml:7:7 { @@ -196,8 +203,8 @@ Warning: required field "name" is not set } === resources.alerts.rname === -Warning: required field "display_name" is not set - at resources.alerts.rname +Error: alert display_name is required + at resources.alerts.rname.display_name in databricks.yml:7:7 Warning: required field "evaluation" is not set diff --git a/acceptance/bundle/validate/invalid_identifiers/databricks.yml b/acceptance/bundle/validate/invalid_identifiers/databricks.yml index 80b542a1cdf..73a6f8a65be 100644 --- a/acceptance/bundle/validate/invalid_identifiers/databricks.yml +++ b/acceptance/bundle/validate/invalid_identifiers/databricks.yml @@ -23,10 +23,25 @@ resources: schema_name: default volume_type: MANAGED + empty_catalog_name: + name: valid + catalog_name: "" + schema_name: default + volume_type: MANAGED + model_serving_endpoints: newline_endpoint: name: "line1\nline2" + vector_search_endpoints: + tab_endpoint: + name: "bad\tname" + endpoint_type: STANDARD + + experiments: + blank_experiment: + name: " " + pipelines: incomplete_file: name: incomplete-file diff --git a/acceptance/bundle/validate/invalid_identifiers/out.test.toml b/acceptance/bundle/validate/invalid_identifiers/out.test.toml index 98ea5040486..d2059b4b5d7 100644 --- a/acceptance/bundle/validate/invalid_identifiers/out.test.toml +++ b/acceptance/bundle/validate/invalid_identifiers/out.test.toml @@ -1,2 +1,2 @@ Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform"] diff --git a/acceptance/bundle/validate/invalid_identifiers/output.txt b/acceptance/bundle/validate/invalid_identifiers/output.txt index af2ffb78797..ef3af03f5fb 100644 --- a/acceptance/bundle/validate/invalid_identifiers/output.txt +++ b/acceptance/bundle/validate/invalid_identifiers/output.txt @@ -4,25 +4,45 @@ Error: catalog name is required at resources.catalogs.empty_catalog.name in databricks.yml:11:13 +Error: experiment name must not be blank + at resources.experiments.blank_experiment.name + in databricks.yml:43:13 + Error: model name is required at resources.models.empty_model.name in databricks.yml:7:13 Error: model_serving_endpoint name must not contain control characters at resources.model_serving_endpoints.newline_endpoint.name - in databricks.yml:28:13 + in databricks.yml:34:13 + +U+000A at byte offset 5 + +Error: vector_search_endpoint name must not contain control characters + at resources.vector_search_endpoints.tab_endpoint.name + in databricks.yml:38:13 + +U+0009 at byte offset 3 + +Error: volume catalog_name is required + at resources.volumes.empty_catalog_name.catalog_name + in databricks.yml:28:21 Error: volume catalog_name must not contain control characters at resources.volumes.tab_catalog.catalog_name in databricks.yml:22:21 +U+0009 at byte offset 4 + Error: volume name must not contain control characters at resources.volumes.tab_volume.name in databricks.yml:15:13 +U+0009 at byte offset 3 + Error: pipeline library file path is required at resources.pipelines.incomplete_file.libraries[0].file - in databricks.yml:34:11 + in databricks.yml:49:17 Name: invalid-identifiers Target: default @@ -30,4 +50,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/invalid-identifiers/default -Found 6 errors +Found 9 errors diff --git a/acceptance/bundle/validate/invalid_identifiers/test.toml b/acceptance/bundle/validate/invalid_identifiers/test.toml new file mode 100644 index 00000000000..784aae87258 --- /dev/null +++ b/acceptance/bundle/validate/invalid_identifiers/test.toml @@ -0,0 +1,3 @@ +Cloud = false +# Validation fails before the deployment engine is consulted. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform"] diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go index b722be24a4e..ba736763002 100644 --- a/bundle/config/validate/invalid_identifiers.go +++ b/bundle/config/validate/invalid_identifiers.go @@ -3,82 +3,100 @@ package validate import ( "context" "fmt" + "slices" "strings" "unicode" + "unicode/utf8" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/internal/validation/generated" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" ) -// errorForInvalidIdentifiers rejects empty, blank, or control-character identifiers. -func errorForInvalidIdentifiers(_ context.Context, b *bundle.Bundle) diag.Diagnostics { +// errorForInvalidIdentifiers rejects empty, blank, or control-character values on the +// identifier fields listed in generated.RequiredFields. The backend rejects all three +// with a 400, so failing here avoids a partial deploy. +func errorForInvalidIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diags := diag.Diagnostics{} - for key, model := range b.Config.Resources.Models { - diags = diags.Extend(identifierDiag(b, "model", "name", model.Name, "resources.models."+key, true)) - } - for key, catalog := range b.Config.Resources.Catalogs { - diags = diags.Extend(identifierDiag(b, "catalog", "name", catalog.Name, "resources.catalogs."+key, true)) - } - for key, schema := range b.Config.Resources.Schemas { - path := "resources.schemas." + key - diags = diags.Extend(identifierDiag(b, "schema", "name", schema.Name, path, true)) - diags = diags.Extend(identifierDiag(b, "schema", "catalog_name", schema.CatalogName, path, false)) - } - for key, volume := range b.Config.Resources.Volumes { - path := "resources.volumes." + key - diags = diags.Extend(identifierDiag(b, "volume", "name", volume.Name, path, true)) - diags = diags.Extend(identifierDiag(b, "volume", "catalog_name", volume.CatalogName, path, false)) - diags = diags.Extend(identifierDiag(b, "volume", "schema_name", volume.SchemaName, path, false)) - } - for key, loc := range b.Config.Resources.ExternalLocations { - diags = diags.Extend(identifierDiag(b, "external_location", "name", loc.Name, "resources.external_locations."+key, true)) - } - for key, model := range b.Config.Resources.RegisteredModels { - path := "resources.registered_models." + key - diags = diags.Extend(identifierDiag(b, "registered_model", "name", model.Name, path, true)) - diags = diags.Extend(identifierDiag(b, "registered_model", "catalog_name", model.CatalogName, path, false)) - diags = diags.Extend(identifierDiag(b, "registered_model", "schema_name", model.SchemaName, path, false)) + trie := &dyn.TrieNode{} + for k := range generated.RequiredFields { + pattern, err := dyn.NewPatternFromString(k) + if err != nil { + return diag.FromErr(fmt.Errorf("invalid pattern %q for identifier validation: %w", k, err)) + } + if err := trie.Insert(pattern); err != nil { + return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", k, err)) + } } - for key, endpoint := range b.Config.Resources.ModelServingEndpoints { - diags = diags.Extend(identifierDiag(b, "model_serving_endpoint", "name", endpoint.Name, "resources.model_serving_endpoints."+key, true)) + + err := dyn.WalkReadOnly(b.Config.Value(), func(p dyn.Path, v dyn.Value) error { + pattern, ok := trie.SearchPath(p) + if !ok { + return nil + } + for _, field := range generated.RequiredFields[pattern.String()] { + if !isIdentifierField(field) { + continue + } + diags = diags.Extend(identifierFieldDiag(b, p, v, field, missingIdentifierIsError(field))) + } + return nil + }) + if err != nil { + return diag.FromErr(err) } + diags = diags.Extend(errorForRegisteredModelIdentifiers(ctx, b)) + sortDiagnostics(diags) return diags } +// errorForRegisteredModelIdentifiers covers registered_models, which the OpenAPI spec +// does not mark required, so generated.RequiredFields has no entry for them. +func errorForRegisteredModelIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + _, err := dyn.MapByPattern( + b.Config.Value(), + dyn.NewPattern(dyn.Key("resources"), dyn.Key("registered_models"), dyn.AnyKey()), + func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + diags = diags.Extend(identifierFieldDiag(b, p, v, "name", true)) + diags = diags.Extend(identifierFieldDiag(b, p, v, "catalog_name", false)) + diags = diags.Extend(identifierFieldDiag(b, p, v, "schema_name", false)) + return v, nil + }, + ) + if err != nil { + return diag.FromErr(err) + } + return diags +} + // errorForIncompletePipelineLibraries rejects file, notebook, and glob entries without paths. -func errorForIncompletePipelineLibraries(_ context.Context, b *bundle.Bundle) diag.Diagnostics { +func errorForIncompletePipelineLibraries(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diags := diag.Diagnostics{} for key, pipeline := range b.Config.Resources.Pipelines { for i, lib := range pipeline.Libraries { - base := fmt.Sprintf("resources.pipelines.%s.libraries[%d]", key, i) + base := dyn.NewPath( + dyn.Key("resources"), + dyn.Key("pipelines"), + dyn.Key(key), + dyn.Key("libraries"), + dyn.Index(i), + ) if lib.File != nil && strings.TrimSpace(lib.File.Path) == "" { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "pipeline library file path is required", - Locations: b.Config.GetLocations(base), - Paths: []dyn.Path{dyn.MustPathFromString(base + ".file")}, - }) + diags = diags.Append(libraryPathDiag(b, base, "file", "pipeline library file path is required")) } if lib.Notebook != nil && strings.TrimSpace(lib.Notebook.Path) == "" { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "pipeline library notebook path is required", - Locations: b.Config.GetLocations(base), - Paths: []dyn.Path{dyn.MustPathFromString(base + ".notebook")}, - }) + diags = diags.Append(libraryPathDiag(b, base, "notebook", "pipeline library notebook path is required")) } if lib.Glob != nil && strings.TrimSpace(lib.Glob.Include) == "" { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "pipeline library glob include is required", - Locations: b.Config.GetLocations(base), - Paths: []dyn.Path{dyn.MustPathFromString(base + ".glob")}, - }) + diags = diags.Append(libraryPathDiag(b, base, "glob", "pipeline library glob include is required")) } } } @@ -87,42 +105,96 @@ func errorForIncompletePipelineLibraries(_ context.Context, b *bundle.Bundle) di return diags } -func identifierDiag(b *bundle.Bundle, resource, field, value, resourcePath string, required bool) diag.Diagnostics { - reason := invalidIdentifierReason(value, required) - if reason == "" { +func libraryPathDiag(b *bundle.Bundle, base dyn.Path, field, summary string) diag.Diagnostic { + fieldPath := base.Append(dyn.Key(field)) + return diag.Diagnostic{ + Severity: diag.Error, + Summary: summary, + Locations: locationsFor(b, fieldPath, base), + Paths: []dyn.Path{fieldPath}, + } +} + +// locationsFor resolves the location of path, falling back to fallback when the field +// carries none of its own (an omitted field has no location to point at). +func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { + if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { + return v.Locations() + } + v, err := dyn.GetByPath(b.Config.Value(), fallback) + if err != nil { return nil } + return v.Locations() +} - fieldPath := resourcePath + "." + field - locations := b.Config.GetLocations(fieldPath) - if len(locations) == 0 { - locations = b.Config.GetLocations(resourcePath) +func identifierFieldDiag(b *bundle.Bundle, resourcePath dyn.Path, resource dyn.Value, field string, missingIsError bool) diag.Diagnostics { + vv := resource.Get(field) + switch vv.Kind() { + case dyn.KindInvalid, dyn.KindNil: + if !missingIsError { + return nil + } + return identifierDiagAt(b, resourcePath, field, "is required", "") + case dyn.KindString: + reason, detail := invalidIdentifierReason(vv.MustString()) + if reason == "" { + return nil + } + return identifierDiagAt(b, resourcePath, field, reason, detail) + default: + return nil } +} +func identifierDiagAt(b *bundle.Bundle, resourcePath dyn.Path, field, reason, detail string) diag.Diagnostics { + fieldPath := slices.Clone(resourcePath).Append(dyn.Key(field)) return diag.Diagnostics{{ Severity: diag.Error, - Summary: fmt.Sprintf("%s %s %s", resource, field, reason), - Locations: locations, - Paths: []dyn.Path{dyn.MustPathFromString(fieldPath)}, + Summary: fmt.Sprintf("%s %s %s", resourceSingularName(resourcePath), field, reason), + Detail: detail, + Locations: locationsFor(b, fieldPath, resourcePath), + Paths: []dyn.Path{fieldPath}, }} } -func invalidIdentifierReason(value string, required bool) string { +func invalidIdentifierReason(value string) (reason, detail string) { if value == "" { - if !required { - return "" - } - return "is required" + return "is required", "" } - if containsControlCharacter(value) { - return "must not contain control characters" + if i := strings.IndexFunc(value, unicode.IsControl); i >= 0 { + r, _ := utf8.DecodeRuneInString(value[i:]) + return "must not contain control characters", fmt.Sprintf("%U at byte offset %d", r, i) } if strings.TrimSpace(value) == "" { - return "must not be blank" + return "must not be blank", "" } - return "" + return "", "" } -func containsControlCharacter(s string) bool { - return strings.ContainsFunc(s, unicode.IsControl) +func isIdentifierField(field string) bool { + return field == "name" || strings.HasSuffix(field, "_name") +} + +// missingIdentifierIsError reports whether an omitted identifier is an error rather than +// a warning. Only the resource's own name is required; UC parents and other *_name +// references may be filled in elsewhere, so those keep warning when omitted. +func missingIdentifierIsError(field string) bool { + switch field { + case "name", "display_name", "instance_pool_name": + return true + default: + return false + } +} + +func resourceSingularName(path dyn.Path) string { + if len(path) >= 2 && path[0].Key() == "resources" { + plural := path[1].Key() + if desc, ok := config.SupportedResources()[plural]; ok && desc.SingularName != "" { + return desc.SingularName + } + return plural + } + return "resource" } diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go index b52aacbb166..7949f7cd5e8 100644 --- a/bundle/config/validate/invalid_identifiers_test.go +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -8,10 +8,12 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/ml" "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/databricks/databricks-sdk-go/service/serving" + "github.com/databricks/databricks-sdk-go/service/vectorsearch" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -35,6 +37,14 @@ func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { VolumeType: catalog.VolumeTypeManaged, }, }, + "empty_parent": { + CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ + Name: "v", + CatalogName: "", + SchemaName: "default", + VolumeType: catalog.VolumeTypeManaged, + }, + }, }, ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ "nl": { @@ -43,6 +53,17 @@ func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { }, }, }, + VectorSearchEndpoints: map[string]*resources.VectorSearchEndpoint{ + "vse": { + CreateEndpoint: vectorsearch.CreateEndpoint{ + Name: "bad\tname", + EndpointType: vectorsearch.EndpointTypeStandard, + }, + }, + }, + Experiments: map[string]*resources.MlflowExperiment{ + "e": {CreateExperiment: ml.CreateExperiment{Name: ""}}, + }, }, }, } @@ -55,9 +76,35 @@ func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { "model name is required", "volume name must not contain control characters", "model_serving_endpoint name must not contain control characters", + "vector_search_endpoint name must not contain control characters", + "experiment name is required", + // empty catalog_name is omitted by FromTyped(omitempty) in tests; warning only. + "required field \"catalog_name\" is not set", }, diagSummaries(diags)) } +func TestRequiredRejectsExplicitEmptyUCParentInDyn(t *testing.T) { + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "volumes": dyn.V(map[string]dyn.Value{ + "v": dyn.V(map[string]dyn.Value{ + "name": dyn.V("v"), + "catalog_name": dyn.V(""), + "schema_name": dyn.V("default"), + "volume_type": dyn.V("MANAGED"), + }), + }), + }), + }), nil + })) + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.Contains(t, diagSummaries(diags), "volume catalog_name is required") +} + func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ @@ -89,6 +136,40 @@ func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { }, diagSummaries(diags)) } +func TestRequiredDoesNotPanicOnMetacharacterResourceKeys(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "weird[0]key": { + CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ + Name: "", + CatalogName: "main", + SchemaName: "default", + VolumeType: catalog.VolumeTypeManaged, + }, + }, + }, + Pipelines: map[string]*resources.Pipeline{ + "weird[0]pipe": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "p", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{}}, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.Contains(t, diagSummaries(diags), "volume name is required") + assert.Contains(t, diagSummaries(diags), "pipeline library file path is required") +} + func TestRequiredAcceptsValidIdentifiersAndPipelineLibraries(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ @@ -133,6 +214,27 @@ func TestRequiredAcceptsMissingOptionalUCParents(t *testing.T) { assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) } +func TestRequiredRejectsBlankOptionalUCParent(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + RegisteredModels: map[string]*resources.RegisteredModel{ + "model": { + CreateRegisteredModelRequest: catalog.CreateRegisteredModelRequest{ + Name: "model", + CatalogName: " ", + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.Contains(t, diagSummaries(diags), "registered_model catalog_name must not be blank") +} + func diagSummaries(diags diag.Diagnostics) []string { out := make([]string, 0, len(diags)) for _, d := range diags { diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index 3e9ad6130cc..3fd1458c6dd 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -53,6 +53,10 @@ func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostic fields := generated.RequiredFields[pattern.String()] for _, field := range fields { + // errorForInvalidIdentifiers already reports these as errors. + if missingIdentifierIsError(field) { + continue + } vv := v.Get(field) if vv.Kind() == dyn.KindInvalid || vv.Kind() == dyn.KindNil { diags = diags.Append(diag.Diagnostic{ @@ -247,9 +251,7 @@ func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics diags = diags.Extend(errorForInvalidSecretScopePermissions(ctx, b)) diags = diags.Extend(errorForInvalidIdentifiers(ctx, b)) diags = diags.Extend(errorForIncompletePipelineLibraries(ctx, b)) - if diags.HasError() { - return diags - } + // Collected even when there are errors, so one run reports every issue. diags = diags.Extend(warnForMissingFields(ctx, b)) return diags } From 7af0f2c691b4ae989548a78484826486b4e11e25 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 12:47:25 +0000 Subject: [PATCH 4/8] Unify required-field and identifier validation into one walk. Collapse OpenAPI warnings, backend-only errors, and blank/control-char checks into a compiled rule set so nested fields keep accurate messages and validate stays a single config walk. --- .../dashboard_required_name/output.txt | 2 +- .../output.txt | 2 +- .../empty_resources/empty_dict/output.txt | 6 +- .../empty_resources/with_grants/output.txt | 6 +- .../with_permissions/output.txt | 6 +- .../invalid_identifiers/out.test.toml | 2 +- .../validate/invalid_identifiers/output.txt | 16 +- .../validate/invalid_identifiers/test.toml | 3 - .../sql_warehouse_required_name/output.txt | 10 +- bundle/config/validate/invalid_identifiers.go | 200 ---------------- bundle/config/validate/pipeline_libraries.go | 48 ++++ .../validate/pipeline_libraries_test.go | 66 ++++++ bundle/config/validate/required.go | 124 +--------- bundle/config/validate/required_fields.go | 219 ++++++++++++++++++ ...ifiers_test.go => required_fields_test.go} | 86 +++---- 15 files changed, 399 insertions(+), 397 deletions(-) delete mode 100644 acceptance/bundle/validate/invalid_identifiers/test.toml delete mode 100644 bundle/config/validate/invalid_identifiers.go create mode 100644 bundle/config/validate/pipeline_libraries.go create mode 100644 bundle/config/validate/pipeline_libraries_test.go create mode 100644 bundle/config/validate/required_fields.go rename bundle/config/validate/{invalid_identifiers_test.go => required_fields_test.go} (76%) diff --git a/acceptance/bundle/validate/dashboard_required_name/output.txt b/acceptance/bundle/validate/dashboard_required_name/output.txt index 3c185500b6e..353c6ce93af 100644 --- a/acceptance/bundle/validate/dashboard_required_name/output.txt +++ b/acceptance/bundle/validate/dashboard_required_name/output.txt @@ -1,7 +1,7 @@ >>> [CLI] bundle validate Error: dashboard display_name is required - at resources.dashboards.my_dashboard + at resources.dashboards.my_dashboard.display_name in databricks.yml:8:7 Name: test-bundle diff --git a/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt b/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt index 9e0b38d9271..2160d3288ef 100644 --- a/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt +++ b/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt @@ -1,7 +1,7 @@ >>> [CLI] bundle validate Error: dashboard warehouse_id is required - at resources.dashboards.my_dashboard + at resources.dashboards.my_dashboard.warehouse_id in databricks.yml:8:7 Name: test-bundle diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index 9918a43061a..2895a29ecb3 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -130,11 +130,11 @@ Warning: required field "schema_name" is not set === resources.dashboards.rname === Error: dashboard display_name is required - at resources.dashboards.rname + at resources.dashboards.rname.display_name in databricks.yml:6:12 Error: dashboard warehouse_id is required - at resources.dashboards.rname + at resources.dashboards.rname.warehouse_id in databricks.yml:6:12 { @@ -161,7 +161,7 @@ Error: app name is required === resources.sql_warehouses.rname === Error: sql_warehouse name is required - at resources.sql_warehouses.rname + at resources.sql_warehouses.rname.name in databricks.yml:6:12 { diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index 8f80d07a8ae..132b3387910 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -163,11 +163,11 @@ Warning: unknown field: grants in databricks.yml:7:7 Error: dashboard display_name is required - at resources.dashboards.rname + at resources.dashboards.rname.display_name in databricks.yml:7:7 Error: dashboard warehouse_id is required - at resources.dashboards.rname + at resources.dashboards.rname.warehouse_id in databricks.yml:7:7 { @@ -202,7 +202,7 @@ Warning: unknown field: grants in databricks.yml:7:7 Error: sql_warehouse name is required - at resources.sql_warehouses.rname + at resources.sql_warehouses.rname.name in databricks.yml:7:7 { diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index 7cdc4040d3b..fbe4bb946ca 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -146,11 +146,11 @@ Warning: required field "schema_name" is not set === resources.dashboards.rname === Error: dashboard display_name is required - at resources.dashboards.rname + at resources.dashboards.rname.display_name in databricks.yml:7:7 Error: dashboard warehouse_id is required - at resources.dashboards.rname + at resources.dashboards.rname.warehouse_id in databricks.yml:7:7 { @@ -177,7 +177,7 @@ Error: app name is required === resources.sql_warehouses.rname === Error: sql_warehouse name is required - at resources.sql_warehouses.rname + at resources.sql_warehouses.rname.name in databricks.yml:7:7 { diff --git a/acceptance/bundle/validate/invalid_identifiers/out.test.toml b/acceptance/bundle/validate/invalid_identifiers/out.test.toml index d2059b4b5d7..98ea5040486 100644 --- a/acceptance/bundle/validate/invalid_identifiers/out.test.toml +++ b/acceptance/bundle/validate/invalid_identifiers/out.test.toml @@ -1,2 +1,2 @@ Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform"] +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/validate/invalid_identifiers/output.txt b/acceptance/bundle/validate/invalid_identifiers/output.txt index ef3af03f5fb..e10f4c117dd 100644 --- a/acceptance/bundle/validate/invalid_identifiers/output.txt +++ b/acceptance/bundle/validate/invalid_identifiers/output.txt @@ -16,13 +16,17 @@ Error: model_serving_endpoint name must not contain control characters at resources.model_serving_endpoints.newline_endpoint.name in databricks.yml:34:13 -U+000A at byte offset 5 +The value contains U+000A at byte offset 5 + +Error: pipeline library file path is required + at resources.pipelines.incomplete_file.libraries[0].file + in databricks.yml:49:17 Error: vector_search_endpoint name must not contain control characters at resources.vector_search_endpoints.tab_endpoint.name in databricks.yml:38:13 -U+0009 at byte offset 3 +The value contains U+0009 at byte offset 3 Error: volume catalog_name is required at resources.volumes.empty_catalog_name.catalog_name @@ -32,17 +36,13 @@ Error: volume catalog_name must not contain control characters at resources.volumes.tab_catalog.catalog_name in databricks.yml:22:21 -U+0009 at byte offset 4 +The value contains U+0009 at byte offset 4 Error: volume name must not contain control characters at resources.volumes.tab_volume.name in databricks.yml:15:13 -U+0009 at byte offset 3 - -Error: pipeline library file path is required - at resources.pipelines.incomplete_file.libraries[0].file - in databricks.yml:49:17 +The value contains U+0009 at byte offset 3 Name: invalid-identifiers Target: default diff --git a/acceptance/bundle/validate/invalid_identifiers/test.toml b/acceptance/bundle/validate/invalid_identifiers/test.toml deleted file mode 100644 index 784aae87258..00000000000 --- a/acceptance/bundle/validate/invalid_identifiers/test.toml +++ /dev/null @@ -1,3 +0,0 @@ -Cloud = false -# Validation fails before the deployment engine is consulted. -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform"] diff --git a/acceptance/bundle/validate/sql_warehouse_required_name/output.txt b/acceptance/bundle/validate/sql_warehouse_required_name/output.txt index fc66fed24e1..4bba9b58c00 100644 --- a/acceptance/bundle/validate/sql_warehouse_required_name/output.txt +++ b/acceptance/bundle/validate/sql_warehouse_required_name/output.txt @@ -1,13 +1,13 @@ >>> [CLI] bundle validate Error: sql_warehouse name is required - at resources.sql_warehouses.blank_warehouse - in databricks.yml:11:7 - -Error: sql_warehouse name is required - at resources.sql_warehouses.my_warehouse + at resources.sql_warehouses.my_warehouse.name in databricks.yml:8:7 +Error: sql_warehouse name must not be blank + at resources.sql_warehouses.blank_warehouse.name + in databricks.yml:11:13 + Name: test-bundle Target: default Workspace: diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go deleted file mode 100644 index ba736763002..00000000000 --- a/bundle/config/validate/invalid_identifiers.go +++ /dev/null @@ -1,200 +0,0 @@ -package validate - -import ( - "context" - "fmt" - "slices" - "strings" - "unicode" - "unicode/utf8" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/internal/validation/generated" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" -) - -// errorForInvalidIdentifiers rejects empty, blank, or control-character values on the -// identifier fields listed in generated.RequiredFields. The backend rejects all three -// with a 400, so failing here avoids a partial deploy. -func errorForInvalidIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := diag.Diagnostics{} - - trie := &dyn.TrieNode{} - for k := range generated.RequiredFields { - pattern, err := dyn.NewPatternFromString(k) - if err != nil { - return diag.FromErr(fmt.Errorf("invalid pattern %q for identifier validation: %w", k, err)) - } - if err := trie.Insert(pattern); err != nil { - return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", k, err)) - } - } - - err := dyn.WalkReadOnly(b.Config.Value(), func(p dyn.Path, v dyn.Value) error { - pattern, ok := trie.SearchPath(p) - if !ok { - return nil - } - for _, field := range generated.RequiredFields[pattern.String()] { - if !isIdentifierField(field) { - continue - } - diags = diags.Extend(identifierFieldDiag(b, p, v, field, missingIdentifierIsError(field))) - } - return nil - }) - if err != nil { - return diag.FromErr(err) - } - - diags = diags.Extend(errorForRegisteredModelIdentifiers(ctx, b)) - - sortDiagnostics(diags) - return diags -} - -// errorForRegisteredModelIdentifiers covers registered_models, which the OpenAPI spec -// does not mark required, so generated.RequiredFields has no entry for them. -func errorForRegisteredModelIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := diag.Diagnostics{} - - _, err := dyn.MapByPattern( - b.Config.Value(), - dyn.NewPattern(dyn.Key("resources"), dyn.Key("registered_models"), dyn.AnyKey()), - func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - diags = diags.Extend(identifierFieldDiag(b, p, v, "name", true)) - diags = diags.Extend(identifierFieldDiag(b, p, v, "catalog_name", false)) - diags = diags.Extend(identifierFieldDiag(b, p, v, "schema_name", false)) - return v, nil - }, - ) - if err != nil { - return diag.FromErr(err) - } - return diags -} - -// errorForIncompletePipelineLibraries rejects file, notebook, and glob entries without paths. -func errorForIncompletePipelineLibraries(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := diag.Diagnostics{} - - for key, pipeline := range b.Config.Resources.Pipelines { - for i, lib := range pipeline.Libraries { - base := dyn.NewPath( - dyn.Key("resources"), - dyn.Key("pipelines"), - dyn.Key(key), - dyn.Key("libraries"), - dyn.Index(i), - ) - if lib.File != nil && strings.TrimSpace(lib.File.Path) == "" { - diags = diags.Append(libraryPathDiag(b, base, "file", "pipeline library file path is required")) - } - if lib.Notebook != nil && strings.TrimSpace(lib.Notebook.Path) == "" { - diags = diags.Append(libraryPathDiag(b, base, "notebook", "pipeline library notebook path is required")) - } - if lib.Glob != nil && strings.TrimSpace(lib.Glob.Include) == "" { - diags = diags.Append(libraryPathDiag(b, base, "glob", "pipeline library glob include is required")) - } - } - } - - sortDiagnostics(diags) - return diags -} - -func libraryPathDiag(b *bundle.Bundle, base dyn.Path, field, summary string) diag.Diagnostic { - fieldPath := base.Append(dyn.Key(field)) - return diag.Diagnostic{ - Severity: diag.Error, - Summary: summary, - Locations: locationsFor(b, fieldPath, base), - Paths: []dyn.Path{fieldPath}, - } -} - -// locationsFor resolves the location of path, falling back to fallback when the field -// carries none of its own (an omitted field has no location to point at). -func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { - if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { - return v.Locations() - } - v, err := dyn.GetByPath(b.Config.Value(), fallback) - if err != nil { - return nil - } - return v.Locations() -} - -func identifierFieldDiag(b *bundle.Bundle, resourcePath dyn.Path, resource dyn.Value, field string, missingIsError bool) diag.Diagnostics { - vv := resource.Get(field) - switch vv.Kind() { - case dyn.KindInvalid, dyn.KindNil: - if !missingIsError { - return nil - } - return identifierDiagAt(b, resourcePath, field, "is required", "") - case dyn.KindString: - reason, detail := invalidIdentifierReason(vv.MustString()) - if reason == "" { - return nil - } - return identifierDiagAt(b, resourcePath, field, reason, detail) - default: - return nil - } -} - -func identifierDiagAt(b *bundle.Bundle, resourcePath dyn.Path, field, reason, detail string) diag.Diagnostics { - fieldPath := slices.Clone(resourcePath).Append(dyn.Key(field)) - return diag.Diagnostics{{ - Severity: diag.Error, - Summary: fmt.Sprintf("%s %s %s", resourceSingularName(resourcePath), field, reason), - Detail: detail, - Locations: locationsFor(b, fieldPath, resourcePath), - Paths: []dyn.Path{fieldPath}, - }} -} - -func invalidIdentifierReason(value string) (reason, detail string) { - if value == "" { - return "is required", "" - } - if i := strings.IndexFunc(value, unicode.IsControl); i >= 0 { - r, _ := utf8.DecodeRuneInString(value[i:]) - return "must not contain control characters", fmt.Sprintf("%U at byte offset %d", r, i) - } - if strings.TrimSpace(value) == "" { - return "must not be blank", "" - } - return "", "" -} - -func isIdentifierField(field string) bool { - return field == "name" || strings.HasSuffix(field, "_name") -} - -// missingIdentifierIsError reports whether an omitted identifier is an error rather than -// a warning. Only the resource's own name is required; UC parents and other *_name -// references may be filled in elsewhere, so those keep warning when omitted. -func missingIdentifierIsError(field string) bool { - switch field { - case "name", "display_name", "instance_pool_name": - return true - default: - return false - } -} - -func resourceSingularName(path dyn.Path) string { - if len(path) >= 2 && path[0].Key() == "resources" { - plural := path[1].Key() - if desc, ok := config.SupportedResources()[plural]; ok && desc.SingularName != "" { - return desc.SingularName - } - return plural - } - return "resource" -} diff --git a/bundle/config/validate/pipeline_libraries.go b/bundle/config/validate/pipeline_libraries.go new file mode 100644 index 00000000000..ff4ac06879b --- /dev/null +++ b/bundle/config/validate/pipeline_libraries.go @@ -0,0 +1,48 @@ +package validate + +import ( + "context" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +// errorForIncompletePipelineLibraries rejects file, notebook, and glob entries +// without paths because the pipelines API rejects them. +func errorForIncompletePipelineLibraries(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + for key, pipeline := range b.Config.Resources.Pipelines { + for i, library := range pipeline.Libraries { + base := dyn.NewPath( + dyn.Key("resources"), + dyn.Key("pipelines"), + dyn.Key(key), + dyn.Key("libraries"), + dyn.Index(i), + ) + if library.File != nil && strings.TrimSpace(library.File.Path) == "" { + diags = diags.Append(libraryPathDiag(b, base, "file", "pipeline library file path is required")) + } + if library.Notebook != nil && strings.TrimSpace(library.Notebook.Path) == "" { + diags = diags.Append(libraryPathDiag(b, base, "notebook", "pipeline library notebook path is required")) + } + if library.Glob != nil && strings.TrimSpace(library.Glob.Include) == "" { + diags = diags.Append(libraryPathDiag(b, base, "glob", "pipeline library glob include is required")) + } + } + } + return diags +} + +// libraryPathDiag reports a missing path on one pipeline library variant. +func libraryPathDiag(b *bundle.Bundle, base dyn.Path, field, summary string) diag.Diagnostic { + fieldPath := base.Append(dyn.Key(field)) + return diag.Diagnostic{ + Severity: diag.Error, + Summary: summary, + Locations: locationsFor(b, fieldPath, base), + Paths: []dyn.Path{fieldPath}, + } +} diff --git a/bundle/config/validate/pipeline_libraries_test.go b/bundle/config/validate/pipeline_libraries_test.go new file mode 100644 index 00000000000..17f3e9e4734 --- /dev/null +++ b/bundle/config/validate/pipeline_libraries_test.go @@ -0,0 +1,66 @@ +package validate_test + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/databricks-sdk-go/service/pipelines" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "weird[0]key": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "p", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{}}, + {Notebook: &pipelines.NotebookLibrary{}}, + {Glob: &pipelines.PathPattern{}}, + {File: &pipelines.FileLibrary{Path: "ok.py"}}, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.ElementsMatch(t, []string{ + "pipeline library file path is required", + "pipeline library notebook path is required", + "pipeline library glob include is required", + }, diagSummaries(diags)) +} + +func TestRequiredAcceptsCompletePipelineLibraries(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "pipeline", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{Path: "file.py"}}, + {Notebook: &pipelines.NotebookLibrary{Path: "notebook.py"}}, + {Glob: &pipelines.PathPattern{Include: "src/**"}}, + }, + }, + }, + }, + }, + }, + } + + assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) +} diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index 3fd1458c6dd..6686f698899 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -5,10 +5,8 @@ import ( "context" "fmt" "slices" - "strings" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/internal/validation/generated" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" ) @@ -23,66 +21,14 @@ func (f *required) Name() string { return "validate:required" } -// Warn for missing fields, based on annotations in the Go SDK / OpenAPI spec. -func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := diag.Diagnostics{} - - // Generate prefix tree for all required fields. - trie := &dyn.TrieNode{} - for k := range generated.RequiredFields { - pattern, err := dyn.NewPatternFromString(k) - if err != nil { - return diag.FromErr(fmt.Errorf("invalid pattern %q for required field validation: %w", k, err)) - } - - err = trie.Insert(pattern) - if err != nil { - return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", k, err)) - } - } - - err := dyn.WalkReadOnly(b.Config.Value(), func(p dyn.Path, v dyn.Value) error { - // If the path is not found in the prefix tree, we do not need to validate any required - // fields in it. - pattern, ok := trie.SearchPath(p) - if !ok { - return nil - } - - cloneP := slices.Clone(p) - - fields := generated.RequiredFields[pattern.String()] - for _, field := range fields { - // errorForInvalidIdentifiers already reports these as errors. - if missingIdentifierIsError(field) { - continue - } - vv := v.Get(field) - if vv.Kind() == dyn.KindInvalid || vv.Kind() == dyn.KindNil { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("required field %q is not set", field), - Locations: v.Locations(), - Paths: []dyn.Path{cloneP}, - }) - } - } - return nil - }) - if err != nil { - return diag.FromErr(err) - } - - sortDiagnostics(diags) - - return diags -} - // sortDiagnostics orders diagnostics deterministically, since they are collected // by walking maps with random iteration order. func sortDiagnostics(diags diag.Diagnostics) { slices.SortFunc(diags, func(a, b diag.Diagnostic) int { - // First sort by summary + // Keep errors ahead of warnings, then sort each group by summary. + if n := cmp.Compare(a.Severity, b.Severity); n != 0 { + return n + } if n := cmp.Compare(a.Summary, b.Summary); n != 0 { return n } @@ -97,62 +43,6 @@ func sortDiagnostics(diags diag.Diagnostics) { }) } -// Bespoke code to error for fields that are not marked as required in the Go SDK / OpenAPI spec. -func errorForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Dashboards should always have a name and warehouse_id. - var nameLocations []dyn.Location - var namePaths []dyn.Path - var warehouseIdLocations []dyn.Location - var warehouseIdPaths []dyn.Path - - diags := diag.Diagnostics{} - for key, dashboard := range b.Config.Resources.Dashboards { - if dashboard.DisplayName == "" { - nameLocations = append(nameLocations, b.Config.GetLocations("resources.dashboards."+key)...) - namePaths = append(namePaths, dyn.MustPathFromString("resources.dashboards."+key)) - } - if dashboard.WarehouseId == "" { - warehouseIdLocations = append(warehouseIdLocations, b.Config.GetLocations("resources.dashboards."+key)...) - warehouseIdPaths = append(warehouseIdPaths, dyn.MustPathFromString("resources.dashboards."+key)) - } - } - - if len(nameLocations) > 0 { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "dashboard display_name is required", - Locations: nameLocations, - Paths: namePaths, - }) - } - if len(warehouseIdLocations) > 0 { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "dashboard warehouse_id is required", - Locations: warehouseIdLocations, - Paths: warehouseIdPaths, - }) - } - - // sql_warehouses.name is optional in the SDK (json:"name,omitempty") but required - // by the backend, which rejects whitespace-only names (name.trim.nonEmpty). - for key, warehouse := range b.Config.Resources.SqlWarehouses { - if strings.TrimSpace(warehouse.Name) == "" { - path := "resources.sql_warehouses." + key - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "sql_warehouse name is required", - Locations: b.Config.GetLocations(path), - Paths: []dyn.Path{dyn.MustPathFromString(path)}, - }) - } - } - - sortDiagnostics(diags) - - return diags -} - // errorForInvalidGrants errors for grants the backend rejects or that never converge: // a missing principal is rejected, and an empty privileges list re-plans forever because // the backend drops principals with no privileges. Erroring here (rather than warning) @@ -246,12 +136,10 @@ func isMissingOrEmptySequence(v dyn.Value) bool { } func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := errorForMissingFields(ctx, b) + diags := validateRequiredFields(ctx, b) diags = diags.Extend(errorForInvalidGrants(ctx, b)) diags = diags.Extend(errorForInvalidSecretScopePermissions(ctx, b)) - diags = diags.Extend(errorForInvalidIdentifiers(ctx, b)) diags = diags.Extend(errorForIncompletePipelineLibraries(ctx, b)) - // Collected even when there are errors, so one run reports every issue. - diags = diags.Extend(warnForMissingFields(ctx, b)) + sortDiagnostics(diags) return diags } diff --git a/bundle/config/validate/required_fields.go b/bundle/config/validate/required_fields.go new file mode 100644 index 00000000000..7fa60508499 --- /dev/null +++ b/bundle/config/validate/required_fields.go @@ -0,0 +1,219 @@ +package validate + +import ( + "context" + "fmt" + "maps" + "slices" + "strings" + "sync" + "unicode" + "unicode/utf8" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/internal/validation/generated" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +type missingFieldBehavior int + +const ( + warnIfMissing missingFieldBehavior = iota + errorIfMissing + ignoreIfMissing +) + +type fieldRule struct { + name string + missing missingFieldBehavior + validateIdentifier bool +} + +type compiledFieldRules struct { + trie *dyn.TrieNode + fields map[string][]fieldRule +} + +var requiredFieldRules = sync.OnceValues(compileRequiredFieldRules) + +// supplementalFieldRules covers backend requirements missing from the OpenAPI spec. +var supplementalFieldRules = map[string][]fieldRule{ + "resources.dashboards.*": { + {name: "display_name", missing: errorIfMissing, validateIdentifier: true}, + {name: "warehouse_id", missing: errorIfMissing}, + }, + "resources.registered_models.*": { + {name: "name", missing: errorIfMissing, validateIdentifier: true}, + {name: "catalog_name", missing: ignoreIfMissing, validateIdentifier: true}, + {name: "schema_name", missing: ignoreIfMissing, validateIdentifier: true}, + }, + "resources.sql_warehouses.*": { + {name: "name", missing: errorIfMissing, validateIdentifier: true}, + }, +} + +// compileRequiredFieldRules combines generated and backend-specific field requirements. +func compileRequiredFieldRules() (compiledFieldRules, error) { + fields := make(map[string][]fieldRule, len(generated.RequiredFields)+len(supplementalFieldRules)) + for pattern, required := range generated.RequiredFields { + rules := make([]fieldRule, 0, len(required)) + for _, field := range required { + rule := fieldRule{name: field, missing: warnIfMissing} + if isResourcePathPattern(pattern) && isIdentifierField(field) { + rule.validateIdentifier = true + if isResourceNameField(field) { + rule.missing = errorIfMissing + } + } + if pattern == "bundle" && field == "name" { + rule.validateIdentifier = true + rule.missing = errorIfMissing + } + rules = append(rules, rule) + } + fields[pattern] = rules + } + maps.Copy(fields, supplementalFieldRules) + + trie := &dyn.TrieNode{} + for value := range fields { + pattern, err := dyn.NewPatternFromString(value) + if err != nil { + return compiledFieldRules{}, fmt.Errorf("invalid pattern %q for required field validation: %w", value, err) + } + if err := trie.Insert(pattern); err != nil { + return compiledFieldRules{}, fmt.Errorf("failed to insert pattern %q into trie: %w", value, err) + } + } + return compiledFieldRules{trie: trie, fields: fields}, nil +} + +// validateRequiredFields checks generated OpenAPI requirements and supplemental +// backend requirements in one walk of the resolved configuration. +func validateRequiredFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + rules, err := requiredFieldRules() + if err != nil { + return diag.FromErr(err) + } + + var diags diag.Diagnostics + err = dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { + pattern, ok := rules.trie.SearchPath(path) + if !ok { + return nil + } + for _, rule := range rules.fields[pattern.String()] { + field := value.Get(rule.name) + if field.Kind() == dyn.KindInvalid || field.Kind() == dyn.KindNil { + switch rule.missing { + case errorIfMissing: + diags = diags.Append(identifierOrRequiredFieldDiag(b, path, rule, "is required", "")) + case warnIfMissing: + diags = diags.Append(missingFieldWarning(path, value, rule.name)) + case ignoreIfMissing: + continue + } + continue + } + if field.Kind() != dyn.KindString || !rule.validateIdentifier { + continue + } + reason, detail := invalidIdentifierReason(field.MustString()) + if reason != "" { + diags = diags.Append(identifierOrRequiredFieldDiag(b, path, rule, reason, detail)) + } + } + return nil + }) + if err != nil { + return diag.FromErr(err) + } + return diags +} + +// identifierOrRequiredFieldDiag reports an invalid or absent required field. +func identifierOrRequiredFieldDiag(b *bundle.Bundle, resourcePath dyn.Path, rule fieldRule, reason, detail string) diag.Diagnostic { + fieldPath := resourcePath.Append(dyn.Key(rule.name)) + entity := requiredObjectName(resourcePath) + summary := fmt.Sprintf("%s %s %s", entity, rule.name, reason) + return diag.Diagnostic{ + Severity: diag.Error, + Summary: summary, + Detail: detail, + Locations: locationsFor(b, fieldPath, resourcePath), + Paths: []dyn.Path{fieldPath}, + } +} + +// missingFieldWarning preserves the warning behavior for ordinary OpenAPI requirements. +func missingFieldWarning(path dyn.Path, value dyn.Value, field string) diag.Diagnostic { + return diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("required field %q is not set", field), + Locations: value.Locations(), + Paths: []dyn.Path{slices.Clone(path)}, + } +} + +// locationsFor resolves the location of path, falling back to fallback when the field +// carries none of its own (an omitted field has no location to point at). +func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { + if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { + return v.Locations() + } + v, err := dyn.GetByPath(b.Config.Value(), fallback) + if err != nil { + return nil + } + return v.Locations() +} + +// invalidIdentifierReason explains why a resource-level identifier is invalid. +func invalidIdentifierReason(value string) (reason, detail string) { + if value == "" { + return "is required", "" + } + if i := strings.IndexFunc(value, unicode.IsControl); i >= 0 { + r, _ := utf8.DecodeRuneInString(value[i:]) + return "must not contain control characters", fmt.Sprintf("The value contains %U at byte offset %d", r, i) + } + if strings.TrimSpace(value) == "" { + return "must not be blank", "" + } + return "", "" +} + +// isIdentifierField reports whether a field carries a resource identifier or reference. +func isIdentifierField(field string) bool { + return field == "name" || strings.HasSuffix(field, "_name") +} + +// isResourceNameField reports whether a field names the resource itself. +func isResourceNameField(field string) bool { + switch field { + case "name", "display_name", "instance_pool_name": + return true + default: + return false + } +} + +// isResourcePathPattern reports whether a pattern selects top-level bundle resources. +func isResourcePathPattern(pattern string) bool { + parts := strings.Split(pattern, ".") + return len(parts) == 3 && parts[0] == "resources" && parts[2] == "*" +} + +// requiredObjectName returns the object name used in a required-field diagnostic. +func requiredObjectName(path dyn.Path) string { + if len(path) == 1 { + return path[0].Key() + } + plural := path[1].Key() + if desc, ok := config.SupportedResources()[plural]; ok && desc.SingularName != "" { + return desc.SingularName + } + return plural +} diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/required_fields_test.go similarity index 76% rename from bundle/config/validate/invalid_identifiers_test.go rename to bundle/config/validate/required_fields_test.go index 7949f7cd5e8..86d0a1fb8d5 100644 --- a/bundle/config/validate/invalid_identifiers_test.go +++ b/bundle/config/validate/required_fields_test.go @@ -11,7 +11,6 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/ml" - "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/databricks/databricks-sdk-go/service/serving" "github.com/databricks/databricks-sdk-go/service/vectorsearch" "github.com/stretchr/testify/assert" @@ -105,38 +104,46 @@ func TestRequiredRejectsExplicitEmptyUCParentInDyn(t *testing.T) { assert.Contains(t, diagSummaries(diags), "volume catalog_name is required") } -func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Resources: config.Resources{ - Pipelines: map[string]*resources.Pipeline{ - "p": { - CreatePipeline: pipelines.CreatePipeline{ - Name: "p", - Libraries: []pipelines.PipelineLibrary{ - {File: &pipelines.FileLibrary{}}, - {Notebook: &pipelines.NotebookLibrary{}}, - {Glob: &pipelines.PathPattern{}}, - {File: &pipelines.FileLibrary{Path: "ok.py"}}, - }, - }, - }, - }, - }, - }, - } +func TestRequiredIdentifierValidationScope(t *testing.T) { + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V(" \t"), + }), + "resources": dyn.V(map[string]dyn.Value{ + "dashboards": dyn.V(map[string]dyn.Value{ + "dashboard": dyn.V(map[string]dyn.Value{ + "display_name": dyn.V("bad\nname"), + "warehouse_id": dyn.V("warehouse"), + }), + }), + "jobs": dyn.V(map[string]dyn.Value{ + "job": dyn.V(map[string]dyn.Value{ + "parameters": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{"default": dyn.V("value")}), + }), + }), + }), + "sql_warehouses": dyn.V(map[string]dyn.Value{ + "warehouse": dyn.V(map[string]dyn.Value{ + "name": dyn.V(" "), + }), + }), + }), + }), nil + })) diags := bundle.Apply(t.Context(), b, validate.Required()) - require.True(t, diags.HasError()) - assert.ElementsMatch(t, []string{ - "pipeline library file path is required", - "pipeline library notebook path is required", - "pipeline library glob include is required", + "bundle name must not contain control characters", + "dashboard display_name must not contain control characters", + "required field \"name\" is not set", + "sql_warehouse name must not be blank", }, diagSummaries(diags)) } -func TestRequiredDoesNotPanicOnMetacharacterResourceKeys(t *testing.T) { +func TestRequiredDoesNotPanicOnMetacharacterResourceKey(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Resources: config.Resources{ @@ -150,16 +157,6 @@ func TestRequiredDoesNotPanicOnMetacharacterResourceKeys(t *testing.T) { }, }, }, - Pipelines: map[string]*resources.Pipeline{ - "weird[0]pipe": { - CreatePipeline: pipelines.CreatePipeline{ - Name: "p", - Libraries: []pipelines.PipelineLibrary{ - {File: &pipelines.FileLibrary{}}, - }, - }, - }, - }, }, }, } @@ -167,28 +164,15 @@ func TestRequiredDoesNotPanicOnMetacharacterResourceKeys(t *testing.T) { diags := bundle.Apply(t.Context(), b, validate.Required()) require.True(t, diags.HasError()) assert.Contains(t, diagSummaries(diags), "volume name is required") - assert.Contains(t, diagSummaries(diags), "pipeline library file path is required") } -func TestRequiredAcceptsValidIdentifiersAndPipelineLibraries(t *testing.T) { +func TestRequiredAcceptsValidIdentifiers(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Resources: config.Resources{ Models: map[string]*resources.MlflowModel{ "model": {CreateModelRequest: ml.CreateModelRequest{Name: "model"}}, }, - Pipelines: map[string]*resources.Pipeline{ - "pipeline": { - CreatePipeline: pipelines.CreatePipeline{ - Name: "pipeline", - Libraries: []pipelines.PipelineLibrary{ - {File: &pipelines.FileLibrary{Path: "file.py"}}, - {Notebook: &pipelines.NotebookLibrary{Path: "notebook.py"}}, - {Glob: &pipelines.PathPattern{Include: "src/**"}}, - }, - }, - }, - }, }, }, } From 810378fcb09794766d2e4811201ed68966486f96 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 13:15:10 +0000 Subject: [PATCH 5/8] Simplify identifier validation into an explicit allowlist. Split OpenAPI required-field warnings from identifier checks and drop the heuristic rule engine so validation policy stays readable and intentional. --- bundle/config/validate/invalid_identifiers.go | 171 ++++++++++++++ ...ds_test.go => invalid_identifiers_test.go} | 0 bundle/config/validate/required.go | 70 +++++- bundle/config/validate/required_fields.go | 219 ------------------ 4 files changed, 240 insertions(+), 220 deletions(-) create mode 100644 bundle/config/validate/invalid_identifiers.go rename bundle/config/validate/{required_fields_test.go => invalid_identifiers_test.go} (100%) delete mode 100644 bundle/config/validate/required_fields.go diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go new file mode 100644 index 00000000000..2182f30f23f --- /dev/null +++ b/bundle/config/validate/invalid_identifiers.go @@ -0,0 +1,171 @@ +package validate + +import ( + "context" + "fmt" + "strings" + "sync" + "unicode" + "unicode/utf8" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/internal/validation/generated" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +type compiledIdentifierRules struct { + trie *dyn.TrieNode + fields map[string][]string +} + +// identifierFields explicitly defines identifier semantics instead of assuming every +// field ending in "_name" is an identifier. True means omission is also an error. +var identifierFields = map[string]bool{ + "catalog_name": false, + "credential_name": false, + "database_instance_name": false, + "database_name": false, + "display_name": true, + "endpoint_name": false, + "instance_pool_name": true, + "name": true, + "output_schema_name": false, + "schema_name": false, + "table_name": false, +} + +// supplementalIdentifierFields covers backend requirements absent from OpenAPI. +var supplementalIdentifierFields = map[string][]string{ + "resources.dashboards.*": {"display_name"}, + "resources.registered_models.*": {"catalog_name", "name", "schema_name"}, + "resources.sql_warehouses.*": {"name"}, +} + +var identifierRules = sync.OnceValues(compileIdentifierRules) + +func compileIdentifierRules() (compiledIdentifierRules, error) { + fields := make(map[string][]string) + for pattern, required := range generated.RequiredFields { + if !isIdentifierObjectPattern(pattern) { + continue + } + for _, field := range required { + if _, ok := identifierFields[field]; ok { + fields[pattern] = append(fields[pattern], field) + } + } + } + for pattern, supplemental := range supplementalIdentifierFields { + fields[pattern] = append(fields[pattern], supplemental...) + } + + trie := &dyn.TrieNode{} + for value := range fields { + pattern, err := dyn.NewPatternFromString(value) + if err != nil { + return compiledIdentifierRules{}, fmt.Errorf("invalid pattern %q for identifier validation: %w", value, err) + } + if err := trie.Insert(pattern); err != nil { + return compiledIdentifierRules{}, fmt.Errorf("failed to insert pattern %q into trie: %w", value, err) + } + } + return compiledIdentifierRules{trie: trie, fields: fields}, nil +} + +// validateIdentifiers rejects values that the backend cannot use as identifiers. +func validateIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + rules, err := identifierRules() + if err != nil { + return diag.FromErr(err) + } + + var diags diag.Diagnostics + err = dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { + pattern, ok := rules.trie.SearchPath(path) + if !ok { + return nil + } + for _, name := range rules.fields[pattern.String()] { + field := value.Get(name) + if field.Kind() == dyn.KindInvalid || field.Kind() == dyn.KindNil { + if identifierFields[name] { + diags = diags.Append(identifierDiag(b, path, name, "is required", "")) + } + continue + } + if field.Kind() != dyn.KindString { + continue + } + reason, detail := invalidIdentifierReason(field.MustString()) + if reason != "" { + diags = diags.Append(identifierDiag(b, path, name, reason, detail)) + } + } + return nil + }) + if err != nil { + return diag.FromErr(err) + } + return diags +} + +func missingIdentifierIsError(pattern, field string) bool { + return isIdentifierObjectPattern(pattern) && identifierFields[field] +} + +func isIdentifierObjectPattern(pattern string) bool { + if pattern == "bundle" { + return true + } + parts := strings.Split(pattern, ".") + return len(parts) == 3 && parts[0] == "resources" && parts[2] == "*" +} + +func identifierDiag(b *bundle.Bundle, resourcePath dyn.Path, field, reason, detail string) diag.Diagnostic { + fieldPath := resourcePath.Append(dyn.Key(field)) + return diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("%s %s %s", requiredObjectName(resourcePath), field, reason), + Detail: detail, + Locations: locationsFor(b, fieldPath, resourcePath), + Paths: []dyn.Path{fieldPath}, + } +} + +func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { + if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { + return v.Locations() + } + v, err := dyn.GetByPath(b.Config.Value(), fallback) + if err != nil { + return nil + } + return v.Locations() +} + +func invalidIdentifierReason(value string) (reason, detail string) { + if value == "" { + return "is required", "" + } + if i := strings.IndexFunc(value, unicode.IsControl); i >= 0 { + r, _ := utf8.DecodeRuneInString(value[i:]) + return "must not contain control characters", fmt.Sprintf("The value contains %U at byte offset %d", r, i) + } + if strings.TrimSpace(value) == "" { + return "must not be blank", "" + } + return "", "" +} + +func requiredObjectName(path dyn.Path) string { + if len(path) == 1 { + return path[0].Key() + } + plural := path[1].Key() + if desc, ok := config.SupportedResources()[plural]; ok && desc.SingularName != "" { + return desc.SingularName + } + return plural +} diff --git a/bundle/config/validate/required_fields_test.go b/bundle/config/validate/invalid_identifiers_test.go similarity index 100% rename from bundle/config/validate/required_fields_test.go rename to bundle/config/validate/invalid_identifiers_test.go diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index 6686f698899..2c72cdd49b8 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -7,6 +7,7 @@ import ( "slices" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/internal/validation/generated" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" ) @@ -21,6 +22,71 @@ func (f *required) Name() string { return "validate:required" } +// warnForMissingFields reports fields marked as required by the OpenAPI spec. +func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + trie := &dyn.TrieNode{} + for value := range generated.RequiredFields { + pattern, err := dyn.NewPatternFromString(value) + if err != nil { + return diag.FromErr(fmt.Errorf("invalid pattern %q for required field validation: %w", value, err)) + } + if err := trie.Insert(pattern); err != nil { + return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", value, err)) + } + } + + var diags diag.Diagnostics + err := dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { + pattern, ok := trie.SearchPath(path) + if !ok { + return nil + } + for _, field := range generated.RequiredFields[pattern.String()] { + if missingIdentifierIsError(pattern.String(), field) { + continue + } + v := value.Get(field) + if v.Kind() != dyn.KindInvalid && v.Kind() != dyn.KindNil { + continue + } + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("required field %q is not set", field), + Locations: value.Locations(), + Paths: []dyn.Path{slices.Clone(path)}, + }) + } + return nil + }) + if err != nil { + return diag.FromErr(err) + } + return diags +} + +// errorForMissingDashboardWarehouseID covers a backend requirement absent from OpenAPI. +func errorForMissingDashboardWarehouseID(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + for key, dashboard := range b.Config.Resources.Dashboards { + if dashboard.WarehouseId != "" { + continue + } + resourcePath := dyn.NewPath( + dyn.Key("resources"), + dyn.Key("dashboards"), + dyn.Key(key), + ) + fieldPath := resourcePath.Append(dyn.Key("warehouse_id")) + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "dashboard warehouse_id is required", + Locations: locationsFor(b, fieldPath, resourcePath), + Paths: []dyn.Path{fieldPath}, + }) + } + return diags +} + // sortDiagnostics orders diagnostics deterministically, since they are collected // by walking maps with random iteration order. func sortDiagnostics(diags diag.Diagnostics) { @@ -136,10 +202,12 @@ func isMissingOrEmptySequence(v dyn.Value) bool { } func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := validateRequiredFields(ctx, b) + diags := validateIdentifiers(ctx, b) + diags = diags.Extend(errorForMissingDashboardWarehouseID(ctx, b)) diags = diags.Extend(errorForInvalidGrants(ctx, b)) diags = diags.Extend(errorForInvalidSecretScopePermissions(ctx, b)) diags = diags.Extend(errorForIncompletePipelineLibraries(ctx, b)) + diags = diags.Extend(warnForMissingFields(ctx, b)) sortDiagnostics(diags) return diags } diff --git a/bundle/config/validate/required_fields.go b/bundle/config/validate/required_fields.go deleted file mode 100644 index 7fa60508499..00000000000 --- a/bundle/config/validate/required_fields.go +++ /dev/null @@ -1,219 +0,0 @@ -package validate - -import ( - "context" - "fmt" - "maps" - "slices" - "strings" - "sync" - "unicode" - "unicode/utf8" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/internal/validation/generated" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" -) - -type missingFieldBehavior int - -const ( - warnIfMissing missingFieldBehavior = iota - errorIfMissing - ignoreIfMissing -) - -type fieldRule struct { - name string - missing missingFieldBehavior - validateIdentifier bool -} - -type compiledFieldRules struct { - trie *dyn.TrieNode - fields map[string][]fieldRule -} - -var requiredFieldRules = sync.OnceValues(compileRequiredFieldRules) - -// supplementalFieldRules covers backend requirements missing from the OpenAPI spec. -var supplementalFieldRules = map[string][]fieldRule{ - "resources.dashboards.*": { - {name: "display_name", missing: errorIfMissing, validateIdentifier: true}, - {name: "warehouse_id", missing: errorIfMissing}, - }, - "resources.registered_models.*": { - {name: "name", missing: errorIfMissing, validateIdentifier: true}, - {name: "catalog_name", missing: ignoreIfMissing, validateIdentifier: true}, - {name: "schema_name", missing: ignoreIfMissing, validateIdentifier: true}, - }, - "resources.sql_warehouses.*": { - {name: "name", missing: errorIfMissing, validateIdentifier: true}, - }, -} - -// compileRequiredFieldRules combines generated and backend-specific field requirements. -func compileRequiredFieldRules() (compiledFieldRules, error) { - fields := make(map[string][]fieldRule, len(generated.RequiredFields)+len(supplementalFieldRules)) - for pattern, required := range generated.RequiredFields { - rules := make([]fieldRule, 0, len(required)) - for _, field := range required { - rule := fieldRule{name: field, missing: warnIfMissing} - if isResourcePathPattern(pattern) && isIdentifierField(field) { - rule.validateIdentifier = true - if isResourceNameField(field) { - rule.missing = errorIfMissing - } - } - if pattern == "bundle" && field == "name" { - rule.validateIdentifier = true - rule.missing = errorIfMissing - } - rules = append(rules, rule) - } - fields[pattern] = rules - } - maps.Copy(fields, supplementalFieldRules) - - trie := &dyn.TrieNode{} - for value := range fields { - pattern, err := dyn.NewPatternFromString(value) - if err != nil { - return compiledFieldRules{}, fmt.Errorf("invalid pattern %q for required field validation: %w", value, err) - } - if err := trie.Insert(pattern); err != nil { - return compiledFieldRules{}, fmt.Errorf("failed to insert pattern %q into trie: %w", value, err) - } - } - return compiledFieldRules{trie: trie, fields: fields}, nil -} - -// validateRequiredFields checks generated OpenAPI requirements and supplemental -// backend requirements in one walk of the resolved configuration. -func validateRequiredFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - rules, err := requiredFieldRules() - if err != nil { - return diag.FromErr(err) - } - - var diags diag.Diagnostics - err = dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { - pattern, ok := rules.trie.SearchPath(path) - if !ok { - return nil - } - for _, rule := range rules.fields[pattern.String()] { - field := value.Get(rule.name) - if field.Kind() == dyn.KindInvalid || field.Kind() == dyn.KindNil { - switch rule.missing { - case errorIfMissing: - diags = diags.Append(identifierOrRequiredFieldDiag(b, path, rule, "is required", "")) - case warnIfMissing: - diags = diags.Append(missingFieldWarning(path, value, rule.name)) - case ignoreIfMissing: - continue - } - continue - } - if field.Kind() != dyn.KindString || !rule.validateIdentifier { - continue - } - reason, detail := invalidIdentifierReason(field.MustString()) - if reason != "" { - diags = diags.Append(identifierOrRequiredFieldDiag(b, path, rule, reason, detail)) - } - } - return nil - }) - if err != nil { - return diag.FromErr(err) - } - return diags -} - -// identifierOrRequiredFieldDiag reports an invalid or absent required field. -func identifierOrRequiredFieldDiag(b *bundle.Bundle, resourcePath dyn.Path, rule fieldRule, reason, detail string) diag.Diagnostic { - fieldPath := resourcePath.Append(dyn.Key(rule.name)) - entity := requiredObjectName(resourcePath) - summary := fmt.Sprintf("%s %s %s", entity, rule.name, reason) - return diag.Diagnostic{ - Severity: diag.Error, - Summary: summary, - Detail: detail, - Locations: locationsFor(b, fieldPath, resourcePath), - Paths: []dyn.Path{fieldPath}, - } -} - -// missingFieldWarning preserves the warning behavior for ordinary OpenAPI requirements. -func missingFieldWarning(path dyn.Path, value dyn.Value, field string) diag.Diagnostic { - return diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("required field %q is not set", field), - Locations: value.Locations(), - Paths: []dyn.Path{slices.Clone(path)}, - } -} - -// locationsFor resolves the location of path, falling back to fallback when the field -// carries none of its own (an omitted field has no location to point at). -func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { - if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { - return v.Locations() - } - v, err := dyn.GetByPath(b.Config.Value(), fallback) - if err != nil { - return nil - } - return v.Locations() -} - -// invalidIdentifierReason explains why a resource-level identifier is invalid. -func invalidIdentifierReason(value string) (reason, detail string) { - if value == "" { - return "is required", "" - } - if i := strings.IndexFunc(value, unicode.IsControl); i >= 0 { - r, _ := utf8.DecodeRuneInString(value[i:]) - return "must not contain control characters", fmt.Sprintf("The value contains %U at byte offset %d", r, i) - } - if strings.TrimSpace(value) == "" { - return "must not be blank", "" - } - return "", "" -} - -// isIdentifierField reports whether a field carries a resource identifier or reference. -func isIdentifierField(field string) bool { - return field == "name" || strings.HasSuffix(field, "_name") -} - -// isResourceNameField reports whether a field names the resource itself. -func isResourceNameField(field string) bool { - switch field { - case "name", "display_name", "instance_pool_name": - return true - default: - return false - } -} - -// isResourcePathPattern reports whether a pattern selects top-level bundle resources. -func isResourcePathPattern(pattern string) bool { - parts := strings.Split(pattern, ".") - return len(parts) == 3 && parts[0] == "resources" && parts[2] == "*" -} - -// requiredObjectName returns the object name used in a required-field diagnostic. -func requiredObjectName(path dyn.Path) string { - if len(path) == 1 { - return path[0].Key() - } - plural := path[1].Key() - if desc, ok := config.SupportedResources()[plural]; ok && desc.SingularName != "" { - return desc.SingularName - } - return plural -} From dc7dbe7e2e86a8469e8f15989921b5d524881d5f Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Thu, 27 Aug 2026 11:45:02 +0000 Subject: [PATCH 6/8] Validate identifiers from typed resource fields instead of a generated trie. --- bundle/config/validate/invalid_identifiers.go | 244 ++++++++++-------- .../validate/invalid_identifiers_test.go | 49 ++++ bundle/config/validate/pipeline_libraries.go | 6 +- bundle/config/validate/required.go | 6 +- 4 files changed, 193 insertions(+), 112 deletions(-) diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go index 2182f30f23f..27679ad91e9 100644 --- a/bundle/config/validate/invalid_identifiers.go +++ b/bundle/config/validate/invalid_identifiers.go @@ -4,145 +4,169 @@ import ( "context" "fmt" "strings" - "sync" "unicode" "unicode/utf8" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/internal/validation/generated" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" ) -type compiledIdentifierRules struct { - trie *dyn.TrieNode - fields map[string][]string -} - -// identifierFields explicitly defines identifier semantics instead of assuming every -// field ending in "_name" is an identifier. True means omission is also an error. -var identifierFields = map[string]bool{ - "catalog_name": false, - "credential_name": false, - "database_instance_name": false, - "database_name": false, - "display_name": true, - "endpoint_name": false, - "instance_pool_name": true, - "name": true, - "output_schema_name": false, - "schema_name": false, - "table_name": false, -} - -// supplementalIdentifierFields covers backend requirements absent from OpenAPI. -var supplementalIdentifierFields = map[string][]string{ - "resources.dashboards.*": {"display_name"}, - "resources.registered_models.*": {"catalog_name", "name", "schema_name"}, - "resources.sql_warehouses.*": {"name"}, -} - -var identifierRules = sync.OnceValues(compileIdentifierRules) +// validateIdentifiers rejects values that the backend cannot use as identifiers. +func validateIdentifiers(_ context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics -func compileIdentifierRules() (compiledIdentifierRules, error) { - fields := make(map[string][]string) - for pattern, required := range generated.RequiredFields { - if !isIdentifierObjectPattern(pattern) { - continue - } - for _, field := range required { - if _, ok := identifierFields[field]; ok { - fields[pattern] = append(fields[pattern], field) - } - } - } - for pattern, supplemental := range supplementalIdentifierFields { - fields[pattern] = append(fields[pattern], supplemental...) + bundlePath := dyn.NewPath(dyn.Key("bundle")) + if pathExists(b, bundlePath) { + diags = diags.Extend(validateIdentifier(b, bundlePath, "name", b.Config.Bundle.Name, true)) } - trie := &dyn.TrieNode{} - for value := range fields { - pattern, err := dyn.NewPatternFromString(value) - if err != nil { - return compiledIdentifierRules{}, fmt.Errorf("invalid pattern %q for identifier validation: %w", value, err) - } - if err := trie.Insert(pattern); err != nil { - return compiledIdentifierRules{}, fmt.Errorf("failed to insert pattern %q into trie: %w", value, err) - } - } - return compiledIdentifierRules{trie: trie, fields: fields}, nil -} - -// validateIdentifiers rejects values that the backend cannot use as identifiers. -func validateIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - rules, err := identifierRules() - if err != nil { - return diag.FromErr(err) + for key, resource := range b.Config.Resources.Alerts { + diags = diags.Extend(validateResourceIdentifier(b, "alerts", key, "display_name", resource.DisplayName, true)) } - - var diags diag.Diagnostics - err = dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { - pattern, ok := rules.trie.SearchPath(path) - if !ok { - return nil - } - for _, name := range rules.fields[pattern.String()] { - field := value.Get(name) - if field.Kind() == dyn.KindInvalid || field.Kind() == dyn.KindNil { - if identifierFields[name] { - diags = diags.Append(identifierDiag(b, path, name, "is required", "")) - } - continue - } - if field.Kind() != dyn.KindString { - continue - } - reason, detail := invalidIdentifierReason(field.MustString()) - if reason != "" { - diags = diags.Append(identifierDiag(b, path, name, reason, detail)) - } - } - return nil - }) - if err != nil { - return diag.FromErr(err) + for key, resource := range b.Config.Resources.Apps { + diags = diags.Extend(validateResourceIdentifier(b, "apps", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.Catalogs { + diags = diags.Extend(validateResourceIdentifier(b, "catalogs", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.Dashboards { + diags = diags.Extend(validateResourceIdentifier(b, "dashboards", key, "display_name", resource.DisplayName, true)) + } + for key, resource := range b.Config.Resources.DatabaseCatalogs { + diags = diags.Extend(validateResourceIdentifier(b, "database_catalogs", key, "database_instance_name", resource.DatabaseInstanceName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "database_catalogs", key, "database_name", resource.DatabaseName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "database_catalogs", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.DatabaseInstances { + diags = diags.Extend(validateResourceIdentifier(b, "database_instances", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.Experiments { + diags = diags.Extend(validateResourceIdentifier(b, "experiments", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.ExternalLocations { + diags = diags.Extend(validateResourceIdentifier(b, "external_locations", key, "credential_name", resource.CredentialName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "external_locations", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.InstancePools { + diags = diags.Extend(validateResourceIdentifier(b, "instance_pools", key, "instance_pool_name", resource.InstancePoolName, true)) + } + for key, resource := range b.Config.Resources.ModelServingEndpoints { + diags = diags.Extend(validateResourceIdentifier(b, "model_serving_endpoints", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.Models { + diags = diags.Extend(validateResourceIdentifier(b, "models", key, "name", resource.Name, true)) } + for key, resource := range b.Config.Resources.QualityMonitors { + diags = diags.Extend(validateResourceIdentifier(b, "quality_monitors", key, "output_schema_name", resource.OutputSchemaName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "quality_monitors", key, "table_name", resource.TableName, false)) + } + for key, resource := range b.Config.Resources.RegisteredModels { + diags = diags.Extend(validateResourceIdentifier(b, "registered_models", key, "catalog_name", resource.CatalogName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "registered_models", key, "name", resource.Name, true)) + diags = diags.Extend(validateResourceIdentifier(b, "registered_models", key, "schema_name", resource.SchemaName, false)) + } + for key, resource := range b.Config.Resources.Schemas { + diags = diags.Extend(validateResourceIdentifier(b, "schemas", key, "catalog_name", resource.CatalogName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "schemas", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.Secrets { + diags = diags.Extend(validateResourceIdentifier(b, "secrets", key, "catalog_name", resource.CatalogName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "secrets", key, "name", resource.Name, true)) + diags = diags.Extend(validateResourceIdentifier(b, "secrets", key, "schema_name", resource.SchemaName, false)) + } + for key, resource := range b.Config.Resources.SecretScopes { + diags = diags.Extend(validateResourceIdentifier(b, "secret_scopes", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.SqlWarehouses { + diags = diags.Extend(validateResourceIdentifier(b, "sql_warehouses", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.SyncedDatabaseTables { + diags = diags.Extend(validateResourceIdentifier(b, "synced_database_tables", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.VectorSearchEndpoints { + diags = diags.Extend(validateResourceIdentifier(b, "vector_search_endpoints", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.VectorSearchIndexes { + diags = diags.Extend(validateResourceIdentifier(b, "vector_search_indexes", key, "endpoint_name", resource.EndpointName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "vector_search_indexes", key, "name", resource.Name, true)) + } + for key, resource := range b.Config.Resources.Volumes { + diags = diags.Extend(validateResourceIdentifier(b, "volumes", key, "catalog_name", resource.CatalogName, false)) + diags = diags.Extend(validateResourceIdentifier(b, "volumes", key, "name", resource.Name, true)) + diags = diags.Extend(validateResourceIdentifier(b, "volumes", key, "schema_name", resource.SchemaName, false)) + } + return diags } func missingIdentifierIsError(pattern, field string) bool { - return isIdentifierObjectPattern(pattern) && identifierFields[field] + switch pattern { + case "bundle": + return field == "name" + case "resources.alerts.*", "resources.dashboards.*": + return field == "display_name" + case "resources.instance_pools.*": + return field == "instance_pool_name" + case "resources.apps.*", + "resources.catalogs.*", + "resources.database_catalogs.*", + "resources.database_instances.*", + "resources.experiments.*", + "resources.external_locations.*", + "resources.model_serving_endpoints.*", + "resources.models.*", + "resources.registered_models.*", + "resources.schemas.*", + "resources.secret_scopes.*", + "resources.secrets.*", + "resources.sql_warehouses.*", + "resources.synced_database_tables.*", + "resources.vector_search_endpoints.*", + "resources.vector_search_indexes.*", + "resources.volumes.*": + return field == "name" + default: + return false + } } -func isIdentifierObjectPattern(pattern string) bool { - if pattern == "bundle" { - return true - } - parts := strings.Split(pattern, ".") - return len(parts) == 3 && parts[0] == "resources" && parts[2] == "*" +func validateResourceIdentifier(b *bundle.Bundle, resourceType, key, field, value string, required bool) diag.Diagnostics { + resourcePath := dyn.NewPath( + dyn.Key("resources"), + dyn.Key(resourceType), + dyn.Key(key), + ) + return validateIdentifier(b, resourcePath, field, value, required) } -func identifierDiag(b *bundle.Bundle, resourcePath dyn.Path, field, reason, detail string) diag.Diagnostic { +func validateIdentifier(b *bundle.Bundle, resourcePath dyn.Path, field, value string, required bool) diag.Diagnostics { fieldPath := resourcePath.Append(dyn.Key(field)) - return diag.Diagnostic{ + locations := b.Config.GetLocations(fieldPath.String()) + if value == "" && !required && !pathExists(b, fieldPath) { + return nil + } + + reason, detail := invalidIdentifierReason(value) + if reason == "" { + return nil + } + if len(locations) == 0 { + locations = b.Config.GetLocations(resourcePath.String()) + } + return diag.Diagnostics{{ Severity: diag.Error, Summary: fmt.Sprintf("%s %s %s", requiredObjectName(resourcePath), field, reason), Detail: detail, - Locations: locationsFor(b, fieldPath, resourcePath), + Locations: locations, Paths: []dyn.Path{fieldPath}, - } + }} } -func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { - if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { - return v.Locations() - } - v, err := dyn.GetByPath(b.Config.Value(), fallback) - if err != nil { - return nil - } - return v.Locations() +func pathExists(b *bundle.Bundle, path dyn.Path) bool { + _, err := dyn.GetByPath(b.Config.Value(), path) + return err == nil } func invalidIdentifierReason(value string) (reason, detail string) { diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go index 86d0a1fb8d5..edfb518cb9e 100644 --- a/bundle/config/validate/invalid_identifiers_test.go +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -219,6 +219,55 @@ func TestRequiredRejectsBlankOptionalUCParent(t *testing.T) { assert.Contains(t, diagSummaries(diags), "registered_model catalog_name must not be blank") } +func TestRequiredRejectsMissingResourceIdentifiers(t *testing.T) { + tests := []struct { + resourceType string + summary string + }{ + {"alerts", "alert display_name is required"}, + {"apps", "app name is required"}, + {"catalogs", "catalog name is required"}, + {"dashboards", "dashboard display_name is required"}, + {"database_catalogs", "database_catalog name is required"}, + {"database_instances", "database_instance name is required"}, + {"experiments", "experiment name is required"}, + {"external_locations", "external_location name is required"}, + {"instance_pools", "instance_pool instance_pool_name is required"}, + {"model_serving_endpoints", "model_serving_endpoint name is required"}, + {"models", "model name is required"}, + {"registered_models", "registered_model name is required"}, + {"schemas", "schema name is required"}, + {"secret_scopes", "secret_scope name is required"}, + {"secrets", "secret name is required"}, + {"sql_warehouses", "sql_warehouse name is required"}, + {"synced_database_tables", "synced_database_table name is required"}, + {"vector_search_endpoints", "vector_search_endpoint name is required"}, + {"vector_search_indexes", "vector_search_index name is required"}, + {"volumes", "volume name is required"}, + } + + for _, tt := range tests { + t.Run(tt.resourceType, func(t *testing.T) { + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V("bundle"), + }), + "resources": dyn.V(map[string]dyn.Value{ + tt.resourceType: dyn.V(map[string]dyn.Value{ + "weird[0]key": dyn.V(map[string]dyn.Value{}), + }), + }), + }), nil + })) + + diags := bundle.Apply(t.Context(), b, validate.Required()) + assert.Contains(t, diagSummaries(diags), tt.summary) + }) + } +} + func diagSummaries(diags diag.Diagnostics) []string { out := make([]string, 0, len(diags)) for _, d := range diags { diff --git a/bundle/config/validate/pipeline_libraries.go b/bundle/config/validate/pipeline_libraries.go index ff4ac06879b..020cbf0c868 100644 --- a/bundle/config/validate/pipeline_libraries.go +++ b/bundle/config/validate/pipeline_libraries.go @@ -39,10 +39,14 @@ func errorForIncompletePipelineLibraries(ctx context.Context, b *bundle.Bundle) // libraryPathDiag reports a missing path on one pipeline library variant. func libraryPathDiag(b *bundle.Bundle, base dyn.Path, field, summary string) diag.Diagnostic { fieldPath := base.Append(dyn.Key(field)) + locations := b.Config.GetLocations(fieldPath.String()) + if len(locations) == 0 { + locations = b.Config.GetLocations(base.String()) + } return diag.Diagnostic{ Severity: diag.Error, Summary: summary, - Locations: locationsFor(b, fieldPath, base), + Locations: locations, Paths: []dyn.Path{fieldPath}, } } diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index 2c72cdd49b8..febc17e1e5f 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -77,10 +77,14 @@ func errorForMissingDashboardWarehouseID(ctx context.Context, b *bundle.Bundle) dyn.Key(key), ) fieldPath := resourcePath.Append(dyn.Key("warehouse_id")) + locations := b.Config.GetLocations(fieldPath.String()) + if len(locations) == 0 { + locations = b.Config.GetLocations(resourcePath.String()) + } diags = diags.Append(diag.Diagnostic{ Severity: diag.Error, Summary: "dashboard warehouse_id is required", - Locations: locationsFor(b, fieldPath, resourcePath), + Locations: locations, Paths: []dyn.Path{fieldPath}, }) } From b4854cd21eb04bacdc75c53874cfc8d68a1a673e Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Thu, 27 Aug 2026 14:05:25 +0000 Subject: [PATCH 7/8] Look up diagnostic locations by structural path. GetLocations parses a string, so resource keys with '[' or '.' lose their YAML source location. --- bundle/config/validate/invalid_identifiers.go | 13 ++++++- .../validate/invalid_identifiers_test.go | 38 ++++++++++--------- bundle/config/validate/pipeline_libraries.go | 4 +- .../validate/pipeline_libraries_test.go | 26 +++++++++++++ bundle/config/validate/required.go | 4 +- 5 files changed, 61 insertions(+), 24 deletions(-) diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go index 27679ad91e9..d0d8567088e 100644 --- a/bundle/config/validate/invalid_identifiers.go +++ b/bundle/config/validate/invalid_identifiers.go @@ -143,7 +143,7 @@ func validateResourceIdentifier(b *bundle.Bundle, resourceType, key, field, valu func validateIdentifier(b *bundle.Bundle, resourcePath dyn.Path, field, value string, required bool) diag.Diagnostics { fieldPath := resourcePath.Append(dyn.Key(field)) - locations := b.Config.GetLocations(fieldPath.String()) + locations := locationsAtPath(b, fieldPath) if value == "" && !required && !pathExists(b, fieldPath) { return nil } @@ -153,7 +153,7 @@ func validateIdentifier(b *bundle.Bundle, resourcePath dyn.Path, field, value st return nil } if len(locations) == 0 { - locations = b.Config.GetLocations(resourcePath.String()) + locations = locationsAtPath(b, resourcePath) } return diag.Diagnostics{{ Severity: diag.Error, @@ -164,6 +164,15 @@ func validateIdentifier(b *bundle.Bundle, resourcePath dyn.Path, field, value st }} } +// locationsAtPath avoids GetLocations: string paths do not round-trip keys with '[' or '.'. +func locationsAtPath(b *bundle.Bundle, path dyn.Path) []dyn.Location { + value, err := dyn.GetByPath(b.Config.Value(), path) + if err != nil { + return nil + } + return value.Locations() +} + func pathExists(b *bundle.Bundle, path dyn.Path) bool { _, err := dyn.GetByPath(b.Config.Value(), path) return err == nil diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go index edfb518cb9e..9c0f5c0cbf1 100644 --- a/bundle/config/validate/invalid_identifiers_test.go +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -143,27 +143,29 @@ func TestRequiredIdentifierValidationScope(t *testing.T) { }, diagSummaries(diags)) } -func TestRequiredDoesNotPanicOnMetacharacterResourceKey(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "weird[0]key": { - CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ - Name: "", - CatalogName: "main", - SchemaName: "default", - VolumeType: catalog.VolumeTypeManaged, - }, - }, - }, - }, - }, - } +func TestRequiredPreservesLocationForMetacharacterResourceKey(t *testing.T) { + location := dyn.Location{File: "databricks.yml", Line: 6, Column: 13} + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "volumes": dyn.V(map[string]dyn.Value{ + "weird[0]key": dyn.V(map[string]dyn.Value{ + "name": dyn.NewValue("", []dyn.Location{location}), + "catalog_name": dyn.V("main"), + "schema_name": dyn.V("default"), + "volume_type": dyn.V("MANAGED"), + }), + }), + }), + }), nil + })) diags := bundle.Apply(t.Context(), b, validate.Required()) require.True(t, diags.HasError()) - assert.Contains(t, diagSummaries(diags), "volume name is required") + require.Len(t, diags, 1) + assert.Equal(t, "volume name is required", diags[0].Summary) + assert.Equal(t, []dyn.Location{location}, diags[0].Locations) } func TestRequiredAcceptsValidIdentifiers(t *testing.T) { diff --git a/bundle/config/validate/pipeline_libraries.go b/bundle/config/validate/pipeline_libraries.go index 020cbf0c868..d09440b07a5 100644 --- a/bundle/config/validate/pipeline_libraries.go +++ b/bundle/config/validate/pipeline_libraries.go @@ -39,9 +39,9 @@ func errorForIncompletePipelineLibraries(ctx context.Context, b *bundle.Bundle) // libraryPathDiag reports a missing path on one pipeline library variant. func libraryPathDiag(b *bundle.Bundle, base dyn.Path, field, summary string) diag.Diagnostic { fieldPath := base.Append(dyn.Key(field)) - locations := b.Config.GetLocations(fieldPath.String()) + locations := locationsAtPath(b, fieldPath) if len(locations) == 0 { - locations = b.Config.GetLocations(base.String()) + locations = locationsAtPath(b, base) } return diag.Diagnostic{ Severity: diag.Error, diff --git a/bundle/config/validate/pipeline_libraries_test.go b/bundle/config/validate/pipeline_libraries_test.go index 17f3e9e4734..9953404e53d 100644 --- a/bundle/config/validate/pipeline_libraries_test.go +++ b/bundle/config/validate/pipeline_libraries_test.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,6 +43,31 @@ func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { }, diagSummaries(diags)) } +func TestRequiredPreservesPipelineLibraryLocationForMetacharacterResourceKey(t *testing.T) { + location := dyn.Location{File: "databricks.yml", Line: 8, Column: 17} + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "pipelines": dyn.V(map[string]dyn.Value{ + "weird[0]key": dyn.V(map[string]dyn.Value{ + "libraries": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "file": dyn.NewValue(map[string]dyn.Value{}, []dyn.Location{location}), + }), + }), + }), + }), + }), + }), nil + })) + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.Len(t, diags, 1) + assert.Equal(t, "pipeline library file path is required", diags[0].Summary) + assert.Equal(t, []dyn.Location{location}, diags[0].Locations) +} + func TestRequiredAcceptsCompletePipelineLibraries(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index febc17e1e5f..dd22338330b 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -77,9 +77,9 @@ func errorForMissingDashboardWarehouseID(ctx context.Context, b *bundle.Bundle) dyn.Key(key), ) fieldPath := resourcePath.Append(dyn.Key("warehouse_id")) - locations := b.Config.GetLocations(fieldPath.String()) + locations := locationsAtPath(b, fieldPath) if len(locations) == 0 { - locations = b.Config.GetLocations(resourcePath.String()) + locations = locationsAtPath(b, resourcePath) } diags = diags.Append(diag.Diagnostic{ Severity: diag.Error, From c74f17905c25d78bf984a68c3f0b77cf8df1d744 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Fri, 28 Aug 2026 11:31:11 +0000 Subject: [PATCH 8/8] Drop drive-by required.go rewrites from identifier validation. Keep the original warn path, HasError early return, and warehouse_id check; only use structural paths where GetLocations panics. --- .../output.txt | 2 +- .../empty_resources/empty_dict/output.txt | 30 +---- .../empty_resources/with_grants/output.txt | 30 +---- .../with_permissions/output.txt | 30 +---- .../validate/invalid_identifiers/output.txt | 8 +- bundle/config/validate/invalid_identifiers.go | 32 +---- .../validate/invalid_identifiers_test.go | 3 - bundle/config/validate/required.go | 122 ++++++++++-------- 8 files changed, 77 insertions(+), 180 deletions(-) diff --git a/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt b/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt index 2160d3288ef..9e0b38d9271 100644 --- a/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt +++ b/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt @@ -1,7 +1,7 @@ >>> [CLI] bundle validate Error: dashboard warehouse_id is required - at resources.dashboards.my_dashboard.warehouse_id + at resources.dashboards.my_dashboard in databricks.yml:8:7 Name: test-bundle diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index 2895a29ecb3..d8d55774ef7 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -87,10 +87,6 @@ Error: schema name is required at resources.schemas.rname.name in databricks.yml:6:12 -Warning: required field "catalog_name" is not set - at resources.schemas.rname - in databricks.yml:6:12 - { "schemas": { "rname": {} @@ -102,14 +98,6 @@ Error: volume name is required at resources.volumes.rname.name in databricks.yml:6:12 -Warning: required field "catalog_name" is not set - at resources.volumes.rname - in databricks.yml:6:12 - -Warning: required field "schema_name" is not set - at resources.volumes.rname - in databricks.yml:6:12 - { "volumes": { "rname": { @@ -134,7 +122,7 @@ Error: dashboard display_name is required in databricks.yml:6:12 Error: dashboard warehouse_id is required - at resources.dashboards.rname.warehouse_id + at resources.dashboards.rname in databricks.yml:6:12 { @@ -191,22 +179,6 @@ Error: alert display_name is required at resources.alerts.rname.display_name in databricks.yml:6:12 -Warning: required field "evaluation" is not set - at resources.alerts.rname - in databricks.yml:6:12 - -Warning: required field "query_text" is not set - at resources.alerts.rname - in databricks.yml:6:12 - -Warning: required field "schedule" is not set - at resources.alerts.rname - in databricks.yml:6:12 - -Warning: required field "warehouse_id" is not set - at resources.alerts.rname - in databricks.yml:6:12 - { "alerts": { "rname": { diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index 132b3387910..2c75a041318 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -109,10 +109,6 @@ Error: schema name is required at resources.schemas.rname.name in databricks.yml:7:7 -Warning: required field "catalog_name" is not set - at resources.schemas.rname - in databricks.yml:7:7 - { "schemas": { "rname": { @@ -126,14 +122,6 @@ Error: volume name is required at resources.volumes.rname.name in databricks.yml:7:7 -Warning: required field "catalog_name" is not set - at resources.volumes.rname - in databricks.yml:7:7 - -Warning: required field "schema_name" is not set - at resources.volumes.rname - in databricks.yml:7:7 - { "volumes": { "rname": { @@ -167,7 +155,7 @@ Error: dashboard display_name is required in databricks.yml:7:7 Error: dashboard warehouse_id is required - at resources.dashboards.rname.warehouse_id + at resources.dashboards.rname in databricks.yml:7:7 { @@ -240,22 +228,6 @@ Error: alert display_name is required at resources.alerts.rname.display_name in databricks.yml:7:7 -Warning: required field "evaluation" is not set - at resources.alerts.rname - in databricks.yml:7:7 - -Warning: required field "query_text" is not set - at resources.alerts.rname - in databricks.yml:7:7 - -Warning: required field "schedule" is not set - at resources.alerts.rname - in databricks.yml:7:7 - -Warning: required field "warehouse_id" is not set - at resources.alerts.rname - in databricks.yml:7:7 - { "alerts": { "rname": { diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index fbe4bb946ca..bd0bfe3549c 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -99,10 +99,6 @@ Error: schema name is required at resources.schemas.rname.name in databricks.yml:7:7 -Warning: required field "catalog_name" is not set - at resources.schemas.rname - in databricks.yml:7:7 - { "schemas": { "rname": {} @@ -118,14 +114,6 @@ Error: volume name is required at resources.volumes.rname.name in databricks.yml:7:7 -Warning: required field "catalog_name" is not set - at resources.volumes.rname - in databricks.yml:7:7 - -Warning: required field "schema_name" is not set - at resources.volumes.rname - in databricks.yml:7:7 - { "volumes": { "rname": { @@ -150,7 +138,7 @@ Error: dashboard display_name is required in databricks.yml:7:7 Error: dashboard warehouse_id is required - at resources.dashboards.rname.warehouse_id + at resources.dashboards.rname in databricks.yml:7:7 { @@ -207,22 +195,6 @@ Error: alert display_name is required at resources.alerts.rname.display_name in databricks.yml:7:7 -Warning: required field "evaluation" is not set - at resources.alerts.rname - in databricks.yml:7:7 - -Warning: required field "query_text" is not set - at resources.alerts.rname - in databricks.yml:7:7 - -Warning: required field "schedule" is not set - at resources.alerts.rname - in databricks.yml:7:7 - -Warning: required field "warehouse_id" is not set - at resources.alerts.rname - in databricks.yml:7:7 - { "alerts": { "rname": { diff --git a/acceptance/bundle/validate/invalid_identifiers/output.txt b/acceptance/bundle/validate/invalid_identifiers/output.txt index e10f4c117dd..bd3f263b3cb 100644 --- a/acceptance/bundle/validate/invalid_identifiers/output.txt +++ b/acceptance/bundle/validate/invalid_identifiers/output.txt @@ -18,10 +18,6 @@ Error: model_serving_endpoint name must not contain control characters The value contains U+000A at byte offset 5 -Error: pipeline library file path is required - at resources.pipelines.incomplete_file.libraries[0].file - in databricks.yml:49:17 - Error: vector_search_endpoint name must not contain control characters at resources.vector_search_endpoints.tab_endpoint.name in databricks.yml:38:13 @@ -44,6 +40,10 @@ Error: volume name must not contain control characters The value contains U+0009 at byte offset 3 +Error: pipeline library file path is required + at resources.pipelines.incomplete_file.libraries[0].file + in databricks.yml:49:17 + Name: invalid-identifiers Target: default Workspace: diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go index d0d8567088e..53480a82821 100644 --- a/bundle/config/validate/invalid_identifiers.go +++ b/bundle/config/validate/invalid_identifiers.go @@ -98,40 +98,10 @@ func validateIdentifiers(_ context.Context, b *bundle.Bundle) diag.Diagnostics { diags = diags.Extend(validateResourceIdentifier(b, "volumes", key, "schema_name", resource.SchemaName, false)) } + sortDiagnostics(diags) return diags } -func missingIdentifierIsError(pattern, field string) bool { - switch pattern { - case "bundle": - return field == "name" - case "resources.alerts.*", "resources.dashboards.*": - return field == "display_name" - case "resources.instance_pools.*": - return field == "instance_pool_name" - case "resources.apps.*", - "resources.catalogs.*", - "resources.database_catalogs.*", - "resources.database_instances.*", - "resources.experiments.*", - "resources.external_locations.*", - "resources.model_serving_endpoints.*", - "resources.models.*", - "resources.registered_models.*", - "resources.schemas.*", - "resources.secret_scopes.*", - "resources.secrets.*", - "resources.sql_warehouses.*", - "resources.synced_database_tables.*", - "resources.vector_search_endpoints.*", - "resources.vector_search_indexes.*", - "resources.volumes.*": - return field == "name" - default: - return false - } -} - func validateResourceIdentifier(b *bundle.Bundle, resourceType, key, field, value string, required bool) diag.Diagnostics { resourcePath := dyn.NewPath( dyn.Key("resources"), diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go index 9c0f5c0cbf1..9bdc94226be 100644 --- a/bundle/config/validate/invalid_identifiers_test.go +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -77,8 +77,6 @@ func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { "model_serving_endpoint name must not contain control characters", "vector_search_endpoint name must not contain control characters", "experiment name is required", - // empty catalog_name is omitted by FromTyped(omitempty) in tests; warning only. - "required field \"catalog_name\" is not set", }, diagSummaries(diags)) } @@ -138,7 +136,6 @@ func TestRequiredIdentifierValidationScope(t *testing.T) { assert.ElementsMatch(t, []string{ "bundle name must not contain control characters", "dashboard display_name must not contain control characters", - "required field \"name\" is not set", "sql_warehouse name must not be blank", }, diagSummaries(diags)) } diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index dd22338330b..7541167f59c 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -22,72 +22,54 @@ func (f *required) Name() string { return "validate:required" } -// warnForMissingFields reports fields marked as required by the OpenAPI spec. +// Warn for missing fields, based on annotations in the Go SDK / OpenAPI spec. func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + // Generate prefix tree for all required fields. trie := &dyn.TrieNode{} - for value := range generated.RequiredFields { - pattern, err := dyn.NewPatternFromString(value) + for k := range generated.RequiredFields { + pattern, err := dyn.NewPatternFromString(k) if err != nil { - return diag.FromErr(fmt.Errorf("invalid pattern %q for required field validation: %w", value, err)) + return diag.FromErr(fmt.Errorf("invalid pattern %q for required field validation: %w", k, err)) } - if err := trie.Insert(pattern); err != nil { - return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", value, err)) + + err = trie.Insert(pattern) + if err != nil { + return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", k, err)) } } - var diags diag.Diagnostics - err := dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { - pattern, ok := trie.SearchPath(path) + err := dyn.WalkReadOnly(b.Config.Value(), func(p dyn.Path, v dyn.Value) error { + // If the path is not found in the prefix tree, we do not need to validate any required + // fields in it. + pattern, ok := trie.SearchPath(p) if !ok { return nil } - for _, field := range generated.RequiredFields[pattern.String()] { - if missingIdentifierIsError(pattern.String(), field) { - continue - } - v := value.Get(field) - if v.Kind() != dyn.KindInvalid && v.Kind() != dyn.KindNil { - continue + + cloneP := slices.Clone(p) + + fields := generated.RequiredFields[pattern.String()] + for _, field := range fields { + vv := v.Get(field) + if vv.Kind() == dyn.KindInvalid || vv.Kind() == dyn.KindNil { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("required field %q is not set", field), + Locations: v.Locations(), + Paths: []dyn.Path{cloneP}, + }) } - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("required field %q is not set", field), - Locations: value.Locations(), - Paths: []dyn.Path{slices.Clone(path)}, - }) } return nil }) if err != nil { return diag.FromErr(err) } - return diags -} -// errorForMissingDashboardWarehouseID covers a backend requirement absent from OpenAPI. -func errorForMissingDashboardWarehouseID(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - var diags diag.Diagnostics - for key, dashboard := range b.Config.Resources.Dashboards { - if dashboard.WarehouseId != "" { - continue - } - resourcePath := dyn.NewPath( - dyn.Key("resources"), - dyn.Key("dashboards"), - dyn.Key(key), - ) - fieldPath := resourcePath.Append(dyn.Key("warehouse_id")) - locations := locationsAtPath(b, fieldPath) - if len(locations) == 0 { - locations = locationsAtPath(b, resourcePath) - } - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "dashboard warehouse_id is required", - Locations: locations, - Paths: []dyn.Path{fieldPath}, - }) - } + sortDiagnostics(diags) + return diags } @@ -95,10 +77,7 @@ func errorForMissingDashboardWarehouseID(ctx context.Context, b *bundle.Bundle) // by walking maps with random iteration order. func sortDiagnostics(diags diag.Diagnostics) { slices.SortFunc(diags, func(a, b diag.Diagnostic) int { - // Keep errors ahead of warnings, then sort each group by summary. - if n := cmp.Compare(a.Severity, b.Severity); n != 0 { - return n - } + // First sort by summary if n := cmp.Compare(a.Summary, b.Summary); n != 0 { return n } @@ -113,6 +92,39 @@ func sortDiagnostics(diags diag.Diagnostics) { }) } +// Bespoke code to error for fields that are not marked as required in the Go SDK / OpenAPI spec. +func errorForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + // Dashboards should always have a warehouse_id. + var warehouseIdLocations []dyn.Location + var warehouseIdPaths []dyn.Path + + diags := diag.Diagnostics{} + for key, dashboard := range b.Config.Resources.Dashboards { + if dashboard.WarehouseId == "" { + resourcePath := dyn.NewPath( + dyn.Key("resources"), + dyn.Key("dashboards"), + dyn.Key(key), + ) + warehouseIdLocations = append(warehouseIdLocations, locationsAtPath(b, resourcePath)...) + warehouseIdPaths = append(warehouseIdPaths, resourcePath) + } + } + + if len(warehouseIdLocations) > 0 { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "dashboard warehouse_id is required", + Locations: warehouseIdLocations, + Paths: warehouseIdPaths, + }) + } + + sortDiagnostics(diags) + + return diags +} + // errorForInvalidGrants errors for grants the backend rejects or that never converge: // a missing principal is rejected, and an empty privileges list re-plans forever because // the backend drops principals with no privileges. Erroring here (rather than warning) @@ -207,11 +219,13 @@ func isMissingOrEmptySequence(v dyn.Value) bool { func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diags := validateIdentifiers(ctx, b) - diags = diags.Extend(errorForMissingDashboardWarehouseID(ctx, b)) + diags = diags.Extend(errorForMissingFields(ctx, b)) diags = diags.Extend(errorForInvalidGrants(ctx, b)) diags = diags.Extend(errorForInvalidSecretScopePermissions(ctx, b)) diags = diags.Extend(errorForIncompletePipelineLibraries(ctx, b)) + if diags.HasError() { + return diags + } diags = diags.Extend(warnForMissingFields(ctx, b)) - sortDiagnostics(diags) return diags }