From 2e564f01ec6c632249eda86b4744975b7070b88a Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 10:20:32 +0000 Subject: [PATCH 1/5] 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 | 5 +- .../resources/catalogs/empty-name/output.txt | 18 +-- .../resources/catalogs/empty-name/script | 5 +- .../resources/catalogs/empty-name/test.toml | 8 +- .../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(+), 103 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 8c52d40aa2d..98ea5040486 100644 --- a/acceptance/bundle/resources/catalogs/empty-name/out.test.toml +++ b/acceptance/bundle/resources/catalogs/empty-name/out.test.toml @@ -1,3 +1,2 @@ -Cloud = true -RequiresUnityCatalog = 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 829c6239443..46bb4996368 100644 --- a/acceptance/bundle/resources/catalogs/empty-name/test.toml +++ b/acceptance/bundle/resources/catalogs/empty-name/test.toml @@ -1,9 +1,3 @@ -# The golden asserts UC's message verbatim, so run on cloud to catch it drifting. -Cloud = true -RequiresUnityCatalog = 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 1a03f809f0117b6ff6ab0e5eb6ba5fa57e9cf33b Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 11:15:14 +0000 Subject: [PATCH 2/5] 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 e32f860995c877aad02414a8ffbe33db8e9df456 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 11:53:59 +0000 Subject: [PATCH 3/5] 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 5d1337f863f5d8bdf7f4395aedabed9dab2ba34a Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 12:47:25 +0000 Subject: [PATCH 4/5] 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 16a7c539e37d6197dc86e015c439cef159e2f128 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Tue, 18 Aug 2026 13:15:10 +0000 Subject: [PATCH 5/5] 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 -}