diff --git a/acceptance/bundle/resources/dashboards/change-serialized-inline/out.plan.direct.json b/acceptance/bundle/resources/dashboards/change-serialized-inline/out.plan.direct.json index b9244b3ab9d..e14ec2fba2a 100644 --- a/acceptance/bundle/resources/dashboards/change-serialized-inline/out.plan.direct.json +++ b/acceptance/bundle/resources/dashboards/change-serialized-inline/out.plan.direct.json @@ -12,14 +12,7 @@ "embed_credentials": false, "parent_path": "/Workspace/Users/[USERNAME]/.bundle/change-serialized-inline-[UNIQUE_NAME]/default/resources", "published": true, - "serialized_dashboard": { - "pages": [ - { - "displayName": "Page One", - "name": "page1" - } - ] - }, + "serialized_dashboard": "{\"pages\":[{\"displayName\":\"Page One\",\"name\":\"page1\"}]}", "warehouse_id": "[TEST_DEFAULT_WAREHOUSE_ID]" } }, @@ -66,22 +59,8 @@ }, "serialized_dashboard": { "action": "update", - "old": { - "pages": [ - { - "displayName": "Page 1", - "name": "page1" - } - ] - }, - "new": { - "pages": [ - { - "displayName": "Page One", - "name": "page1" - } - ] - }, + "old": "{\"pages\":[{\"displayName\":\"Page 1\",\"name\":\"page1\"}]}", + "new": "{\"pages\":[{\"displayName\":\"Page One\",\"name\":\"page1\"}]}", "remote": "{\"pages\":[{\"displayName\":\"Page 1\",\"name\":\"page1\",\"pageType\":\"PAGE_TYPE_CANVAS\"}]}\n" }, "update_time": { diff --git a/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard.go b/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard.go index 27b55403d88..a17e5c6b07c 100644 --- a/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard.go +++ b/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard.go @@ -2,6 +2,7 @@ package resourcemutator import ( "context" + "encoding/json" "fmt" "github.com/databricks/cli/bundle" @@ -39,17 +40,49 @@ func (c configureDashboardSerializedDashboard) Apply(_ context.Context, b *bundl // Include "serialized_dashboard" field if "file_path" is set. // Note: the Terraform resource supports "file_path" natively, but we read the contents of the dashboard here // to be able to read file contents in Databricks Workspace (reading a dashboard file via file system fails there) - path, ok := v.Get(filePathFieldName).AsString() - if !ok { - return v, nil - } + filePath, hasFilePath := v.Get(filePathFieldName).AsString() + sd := v.Get(serializedDashboardFieldName) + + if hasFilePath { + // file_path and serialized_dashboard are two ways to provide the + // same content. Accepting both is ambiguous, so reject it instead + // of silently picking one. + if sd.IsValid() && sd.Kind() != dyn.KindNil { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "both file_path and serialized_dashboard are set; specify only one", + Locations: sd.Locations(), + }) + return v, nil + } - contents, err := b.SyncRoot.ReadFile(path) - if err != nil { - return dyn.InvalidValue, fmt.Errorf("failed to read serialized dashboard from file_path %s: %w", path, err) + contents, err := b.SyncRoot.ReadFile(filePath) + if err != nil { + return dyn.InvalidValue, fmt.Errorf("failed to read serialized dashboard from file_path %s: %w", filePath, err) + } + return dyn.Set(v, serializedDashboardFieldName, dyn.V(string(contents))) } - return dyn.Set(v, serializedDashboardFieldName, dyn.V(string(contents))) + // Marshal an inline structured serialized_dashboard to a JSON string + switch sd.Kind() { + case dyn.KindInvalid, dyn.KindNil, dyn.KindString: + // KindInvalid means serialized_dashboard is absent (neither it nor + // file_path is set); leave it for backend validation to reject. + return v, nil + case dyn.KindMap: + jsonBytes, err := json.Marshal(sd.AsAny()) + if err != nil { + return dyn.InvalidValue, fmt.Errorf("failed to marshal inline serialized_dashboard: %w", err) + } + return dyn.Set(v, serializedDashboardFieldName, dyn.V(string(jsonBytes))) + default: + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("serialized_dashboard must be a string or map, got %s", sd.Kind()), + Locations: sd.Locations(), + }) + return v, nil + } }) }) diff --git a/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard_test.go b/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard_test.go new file mode 100644 index 00000000000..b4437874b8c --- /dev/null +++ b/bundle/config/mutator/resourcemutator/configure_dashboards_serialized_dashboard_test.go @@ -0,0 +1,126 @@ +package resourcemutator_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/mutator/resourcemutator" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/vfs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConfigureDashboardSerializedDashboard(t *testing.T) { + const fileName = "dashboard.lvdash.json" + + tests := []struct { + name string + // filePath is set on the resource as-is (already sync-root-relative). + filePath string + // writeFile creates filePath with fileContents before the mutator runs. + writeFile bool + fileContents string + setSerialized bool + serializedDashboard any + // wantSerialized is the expected serialized_dashboard after a successful run. + wantSerialized any + // wantErr, when non-empty, is a substring expected in the diagnostics. + wantErr string + }{ + { + // The file is read verbatim, so formatting and the trailing newline + // are preserved (unlike the inline path, which re-marshals). + name: "file_path reads file contents verbatim", + filePath: fileName, + writeFile: true, + fileContents: `{"pages": 1}` + "\n", + wantSerialized: `{"pages": 1}` + "\n", + }, + { + // Inline maps are marshaled to a compact JSON string with sorted keys + // so config and state hold an identical string and don't drift. + name: "inline map is marshaled to a JSON string", + setSerialized: true, + serializedDashboard: map[string]any{"pages": 1}, + wantSerialized: `{"pages":1}`, + }, + { + name: "inline string is left unchanged", + setSerialized: true, + serializedDashboard: `{"pages":1}`, + wantSerialized: `{"pages":1}`, + }, + { + // Neither field set: the absent field must pass through, not error. + name: "neither file_path nor serialized_dashboard passes through", + wantSerialized: nil, + }, + { + name: "both file_path and serialized_dashboard is rejected", + filePath: fileName, + setSerialized: true, + serializedDashboard: map[string]any{"pages": 1}, + wantErr: "both file_path and serialized_dashboard are set; specify only one", + }, + { + name: "non-structured serialized_dashboard is rejected", + setSerialized: true, + serializedDashboard: true, + wantErr: "serialized_dashboard must be a string or map, got bool", + }, + { + name: "inline sequence is rejected", + setSerialized: true, + serializedDashboard: []any{map[string]any{"version": 1}}, + wantErr: "serialized_dashboard must be a string or map, got sequence", + }, + { + name: "unreadable file_path is an error", + filePath: "does_not_exist.json", + wantErr: "failed to read serialized dashboard", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + if tt.writeFile { + require.NoError(t, os.WriteFile(filepath.Join(dir, tt.filePath), []byte(tt.fileContents), 0o600)) + } + + dash := &resources.Dashboard{ + DashboardConfig: resources.DashboardConfig{DisplayName: "My Dashboard"}, + FilePath: tt.filePath, + } + if tt.setSerialized { + dash.SerializedDashboard = tt.serializedDashboard + } + + b := &bundle.Bundle{ + SyncRootPath: dir, + BundleRootPath: dir, + SyncRoot: vfs.MustNew(dir), + Config: config.Root{ + Resources: config.Resources{ + Dashboards: map[string]*resources.Dashboard{"my_dashboard": dash}, + }, + }, + } + + diags := bundle.ApplySeq(t.Context(), b, resourcemutator.ConfigureDashboardSerializedDashboard()) + + if tt.wantErr != "" { + require.Error(t, diags.Error()) + assert.ErrorContains(t, diags.Error(), tt.wantErr) + return + } + + require.NoError(t, diags.Error()) + assert.Equal(t, tt.wantSerialized, b.Config.Resources.Dashboards["my_dashboard"].SerializedDashboard) + }) + } +} diff --git a/bundle/deploy/terraform/tfdyn/convert_dashboard.go b/bundle/deploy/terraform/tfdyn/convert_dashboard.go index eb9260e8ec8..e55cb6b83de 100644 --- a/bundle/deploy/terraform/tfdyn/convert_dashboard.go +++ b/bundle/deploy/terraform/tfdyn/convert_dashboard.go @@ -2,7 +2,6 @@ package tfdyn import ( "context" - "encoding/json" "fmt" "github.com/databricks/cli/bundle/internal/tf/schema" @@ -11,48 +10,17 @@ import ( "github.com/databricks/cli/libs/log" ) -const ( - serializedDashboardFieldName = "serialized_dashboard" -) - -// Marshal "serialized_dashboard" as JSON if it is set in the input but not in the output. -func marshalSerializedDashboard(vin, vout dyn.Value) (dyn.Value, error) { - // Skip if the "serialized_dashboard" field is already set. - if v := vout.Get(serializedDashboardFieldName); v.IsValid() { - return vout, nil - } - - // Skip if the "serialized_dashboard" field on the input is not set. - v := vin.Get(serializedDashboardFieldName) - if !v.IsValid() { - return vout, nil - } - - // Marshal the "serialized_dashboard" field as JSON. - data, err := json.Marshal(v.AsAny()) - if err != nil { - return dyn.InvalidValue, fmt.Errorf("failed to marshal serialized_dashboard: %w", err) - } - - // Set the "serialized_dashboard" field on the output. - return dyn.Set(vout, serializedDashboardFieldName, dyn.V(string(data))) -} - func convertDashboardResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { var err error - // Normalize the output value to the target schema. + // Normalize the output value to the target schema. ConfigureDashboardSerializedDashboard + // normalizes serialized_dashboard to a JSON string before this runs, so it maps + // straight onto the schema's string field. vout, diags := convert.Normalize(schema.ResourceDashboard{}, vin) for _, diag := range diags { log.Debugf(ctx, "dashboard normalization diagnostic: %s", diag.Summary) } - // Marshal "serialized_dashboard" as JSON if it is set in the input but not in the output. - vout, err = marshalSerializedDashboard(vin, vout) - if err != nil { - return dyn.InvalidValue, err - } - // Drop the "file_path" field. It's always inlined into "serialized_dashboard". vout, err = dyn.DropKeys(vout, []string{"file_path"}) if err != nil { diff --git a/bundle/deploy/terraform/tfdyn/convert_dashboard_test.go b/bundle/deploy/terraform/tfdyn/convert_dashboard_test.go index d8f05dc76c1..aa23b90df78 100644 --- a/bundle/deploy/terraform/tfdyn/convert_dashboard_test.go +++ b/bundle/deploy/terraform/tfdyn/convert_dashboard_test.go @@ -77,17 +77,12 @@ func TestConvertDashboardSerializedDashboardString(t *testing.T) { }) } -func TestConvertDashboardSerializedDashboardAny(t *testing.T) { +func TestConvertDashboardDropsFilePath(t *testing.T) { + // ConfigureDashboardSerializedDashboard reads file_path into serialized_dashboard + // but keeps file_path around, so the converter sees both and must drop file_path. src := resources.Dashboard{ DashboardConfig: resources.DashboardConfig{ - SerializedDashboard: map[string]any{ - "pages": []map[string]any{ - { - "displayName": "New Page", - "layout": []map[string]any{}, - }, - }, - }, + SerializedDashboard: `{"pages":[{"displayName":"New Page","layout":[]}]}`, }, FilePath: "some/path/to/dashboard.lvdash.json", } diff --git a/bundle/direct/dresources/all_test.go b/bundle/direct/dresources/all_test.go index 36eaa3e27df..95742506887 100644 --- a/bundle/direct/dresources/all_test.go +++ b/bundle/direct/dresources/all_test.go @@ -256,21 +256,10 @@ var testConfig map[string]any = map[string]any{ "dashboards": &resources.Dashboard{ DashboardConfig: resources.DashboardConfig{ - DisplayName: "my-dashboard", - ParentPath: "/Workspace/Users/user@example.com", - WarehouseId: "test-warehouse-id", - // Use []any/map[string]any to mirror how this any-typed field is - // populated in production (JSON/dyn decoding); a typed []map[string]any - // can never come out of that path. - SerializedDashboard: map[string]any{ - "pages": []any{ - map[string]any{ - "name": "page1", - "displayName": "Page 1", - "pageType": "PAGE_TYPE_CANVAS", - }, - }, - }, + DisplayName: "my-dashboard", + ParentPath: "/Workspace/Users/user@example.com", + WarehouseId: "test-warehouse-id", + SerializedDashboard: `{"pages":[{"name":"page1","displayName":"Page 1","pageType":"PAGE_TYPE_CANVAS"}]}`, DatasetCatalog: "main", DatasetSchema: "myschema", diff --git a/bundle/direct/dresources/dashboard.go b/bundle/direct/dresources/dashboard.go index aaf1feea616..5103858974a 100644 --- a/bundle/direct/dresources/dashboard.go +++ b/bundle/direct/dresources/dashboard.go @@ -2,7 +2,6 @@ package dresources import ( "context" - "encoding/json" "fmt" "path" "slices" @@ -237,17 +236,12 @@ func prepareDashboardRequest(config *DashboardState) (dashboards.Dashboard, erro // Thus we need to filter such fields out. ForceSendFields: utils.FilterFields[dashboards.Dashboard](config.ForceSendFields), } - v := config.SerializedDashboard - if serializedDashboard, ok := v.(string); ok { - // If serialized dashboard is already a string, we can use it directly. - dashboard.SerializedDashboard = serializedDashboard - } else if v != nil { - // If it's inlined in the bundle config as a map, we need to marshal it to a string. - b, err := json.Marshal(v) - if err != nil { - return dashboards.Dashboard{}, fmt.Errorf("failed to marshal serialized dashboard: %w", err) - } - dashboard.SerializedDashboard = string(b) + switch v := config.SerializedDashboard.(type) { + case nil: + case string: + dashboard.SerializedDashboard = v + default: + return dashboards.Dashboard{}, fmt.Errorf("internal error: serialized_dashboard should have been normalized to a string, got %T", v) } return dashboard, nil }