diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index f30aa795683..3cead33bb7f 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,6 +10,7 @@ * Added `databricks aitools` command group for installing Databricks skills into your coding agents (Claude Code, Cursor, Codex CLI, OpenCode, GitHub Copilot, Antigravity). Skills are fetched from [github.com/databricks/databricks-agent-skills](https://github.com/databricks/databricks-agent-skills) and either symlinked into each agent's skills directory or copied into the current project. Use `databricks aitools install` to set up, `update` to pull newer versions, `list` to see what's available, and `uninstall` to remove them. * `[__settings__].default_profile` is now consulted as a fallback by `databricks api`, `databricks auth token`, and bundle commands when neither `--profile` nor `DATABRICKS_CONFIG_PROFILE` is set. `databricks auth token` continues to give precedence to `DATABRICKS_HOST` over `default_profile`. For bundle commands, `default_profile` only applies when the bundle does not pin its own `workspace.host`. +* `databricks postgres create-role --help` now documents the `--json` body shape and rejects the common mistake of wrapping the body in `{"role": ...}` client-side with a hint pointing at the correct shape ([#5111](https://github.com/databricks/cli/pull/5111)). ### Bundles * Make sure warnings asking for approval are understood by agents ([#5239](https://github.com/databricks/cli/pull/5239)) diff --git a/cmd/workspace/postgres/overrides.go b/cmd/workspace/postgres/overrides.go new file mode 100644 index 00000000000..0f5ca2030c7 --- /dev/null +++ b/cmd/workspace/postgres/overrides.go @@ -0,0 +1,78 @@ +package postgres + +import ( + "errors" + "fmt" + + "github.com/databricks/cli/libs/flags" + "github.com/databricks/databricks-sdk-go/service/postgres" + "github.com/spf13/cobra" +) + +// createRoleOverride appends an example body to the auto-generated help and +// rejects wrapped {"role": ...} bodies with a clear client-side error. +// The --json flag binds to the inner Role object (CreateRoleRequest.Role, +// JSON-tagged "role"), so users supply spec/name/etc. directly. Without an +// example, the auto-generated `// TODO: complex arg: spec` flags leave no +// hint about the body shape and the API's "Field 'role' is required" error +// is unhelpful when the request body is wrong. +func createRoleOverride(createRoleCmd *cobra.Command, _ *postgres.CreateRoleRequest) { + prevPreRunE := createRoleCmd.PreRunE + createRoleCmd.PreRunE = func(cmd *cobra.Command, args []string) error { + if err := rejectWrappedRoleJSON(cmd); err != nil { + return err + } + if prevPreRunE != nil { + return prevPreRunE(cmd, args) + } + return nil + } + + createRoleCmd.Long += ` + + Body shape (passed via --json): fields go directly on the Role object. + Do not wrap them in '{"role": ...}' — the CLI rejects wrapped bodies + client-side with a hint pointing to the right shape. + + Example — create a service-principal-backed role: + + databricks postgres create-role projects//branches/ \ + --role-id \ + --json '{"spec": {"identity_type": "SERVICE_PRINCIPAL", "postgres_role": "", "auth_method": "LAKEBASE_OAUTH_V1"}}' + + The example omits 'membership_roles' so the role starts with default + privileges only — grant database/schema/table access separately via + SQL, following least privilege. Set 'membership_roles' (e.g. + ["DATABRICKS_SUPERUSER"]) only when broad administrative access is + intentional. + + See databricks-sdk-go/service/postgres.RoleRoleSpec for the full set of + spec fields.` +} + +// rejectWrappedRoleJSON returns a clear error when --json is a top-level +// object containing a "role" key. Without this guard the generated unmarshal +// strips the unknown outer "role" field with a warning and ships an empty +// body, and the server rejects with a confusing "Field 'role' is required" +// message. +func rejectWrappedRoleJSON(cmd *cobra.Command) error { + // These checks are internal invariants — postgres create-role is a + // generated command and always has a *flags.JsonFlag for --json. A + // future codegen/refactor change could break that, and we want loud + // breakage rather than a silently-disabled guard. + flag := cmd.Flags().Lookup("json") + if flag == nil { + return errors.New("internal: postgres create-role expected a --json flag; this override is wired to the wrong command") + } + jf, ok := flag.Value.(*flags.JsonFlag) + if !ok { + return fmt.Errorf("internal: postgres create-role --json flag has unexpected type %T; expected *flags.JsonFlag", flag.Value) + } + return jf.RejectWrappedJSON("role", `databricks postgres create-role projects//branches/ \ + --role-id \ + --json '{"spec": {"identity_type": "SERVICE_PRINCIPAL", "postgres_role": "", "auth_method": "LAKEBASE_OAUTH_V1"}}'`) +} + +func init() { + createRoleOverrides = append(createRoleOverrides, createRoleOverride) +} diff --git a/cmd/workspace/postgres/overrides_test.go b/cmd/workspace/postgres/overrides_test.go new file mode 100644 index 00000000000..46b30426177 --- /dev/null +++ b/cmd/workspace/postgres/overrides_test.go @@ -0,0 +1,56 @@ +package postgres + +import ( + "testing" + + "github.com/databricks/cli/libs/flags" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func cmdWithJSON(t *testing.T, raw string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{} + var jf flags.JsonFlag + cmd.Flags().Var(&jf, "json", "JSON body") + if raw != "" { + require.NoError(t, jf.Set(raw)) + } + return cmd +} + +func TestRejectWrappedRoleJSON(t *testing.T) { + t.Run("rejects wrapped {role: ...}", func(t *testing.T) { + cmd := cmdWithJSON(t, `{"role":{"spec":{"identity_type":"SERVICE_PRINCIPAL"}}}`) + err := rejectWrappedRoleJSON(cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "should NOT be wrapped") + assert.Contains(t, err.Error(), `databricks postgres create-role`) + }) + + t.Run("passes when body has spec at top level", func(t *testing.T) { + cmd := cmdWithJSON(t, `{"spec":{"identity_type":"SERVICE_PRINCIPAL"}}`) + assert.NoError(t, rejectWrappedRoleJSON(cmd)) + }) + + t.Run("passes when --json was not provided", func(t *testing.T) { + cmd := cmdWithJSON(t, "") + assert.NoError(t, rejectWrappedRoleJSON(cmd)) + }) + + t.Run("passes through non-object JSON to the generated diagnostics path", func(t *testing.T) { + cmd := cmdWithJSON(t, `"not-an-object"`) + assert.NoError(t, rejectWrappedRoleJSON(cmd)) + }) + + t.Run("fails loudly when --json flag is absent on the command", func(t *testing.T) { + // Internal invariant: postgres create-role is a generated command and + // always has a --json flag. If a future codegen change drops it, this + // override is wired to the wrong command and should fail loudly so the + // regression is caught rather than silently disabling the guard. + err := rejectWrappedRoleJSON(&cobra.Command{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "internal:") + }) +} diff --git a/libs/flags/json_flag.go b/libs/flags/json_flag.go index d5f143c5654..133001d9081 100644 --- a/libs/flags/json_flag.go +++ b/libs/flags/json_flag.go @@ -113,3 +113,44 @@ func (j *JsonFlag) Unmarshal(v any) diag.Diagnostics { func (j *JsonFlag) Type() string { return "JSON" } + +// Raw returns the raw JSON bytes the flag was set to (or nil when the flag +// was not provided). Exposed so command overrides can do shape-level +// validation before the generated Unmarshal call. +func (j *JsonFlag) Raw() []byte { + return j.raw +} + +// RejectWrappedJSON returns a clear client-side error when the --json body +// is a top-level object containing outerKey. It detects the common mistake +// of wrapping the body in '{"": ...}' when the flag binds to the +// inner object (the SDK request type's JSON-tagged outer field). Returns +// nil when the flag is unset, when the body isn't an object, or when no +// outerKey is present at the top level. +// +// example, if non-empty, is appended to the error as a hint at the correct +// shape. Callers typically construct it as a self-contained command line. +// +// This is the shared helper used by the postgres command overrides; the +// same inner-body --json shape exists across siblings like create-branch, +// create-database, create-endpoint, and create-project. +func (j *JsonFlag) RejectWrappedJSON(outerKey, example string) error { + if len(j.raw) == 0 { + return nil + } + var top map[string]json.RawMessage + if err := json.Unmarshal(j.raw, &top); err != nil { + // Defer non-object inputs to the generated unmarshal so its + // diagnostics render the original parse error. + return nil //nolint:nilerr + } + if _, found := top[outerKey]; !found { + return nil + } + msg := fmt.Sprintf("--json should NOT be wrapped in '{%q: ...}'.\n\n"+ + "The flag binds to the inner object — supply its fields directly.", outerKey) + if example != "" { + msg += "\n\nExample:\n\n " + example + } + return fmt.Errorf("%s", msg) +} diff --git a/libs/flags/json_flag_test.go b/libs/flags/json_flag_test.go index 4bebf8b68c0..2ebda9f8678 100644 --- a/libs/flags/json_flag_test.go +++ b/libs/flags/json_flag_test.go @@ -284,3 +284,44 @@ func TestJsonUnmarshalForRequestWithForceSendFields(t *testing.T) { assert.NotContains(t, r.NewSettings.NotificationSettings.ForceSendFields, "NoAlertForSkippedRuns") assert.Contains(t, r.NewSettings.NotificationSettings.ForceSendFields, "NoAlertForCanceledRuns") } + +func TestRejectWrappedJSON(t *testing.T) { + tests := []struct { + name string + raw string + outerKey string + wantErr string + }{ + {name: "empty body passes", raw: "", outerKey: "role"}, + {name: "non-object body passes", raw: `"not-an-object"`, outerKey: "role"}, + {name: "object without outer key passes", raw: `{"spec":{"foo":"bar"}}`, outerKey: "role"}, + {name: "object with outer key rejected", raw: `{"role":{"spec":{"foo":"bar"}}}`, outerKey: "role", wantErr: `should NOT be wrapped`}, + {name: "object with outer key plus siblings still rejected", raw: `{"role":{},"other":1}`, outerKey: "role", wantErr: `should NOT be wrapped`}, + {name: "outer key differs from body's outer", raw: `{"branch":{}}`, outerKey: "role"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var jf JsonFlag + if tt.raw != "" { + require.NoError(t, jf.Set(tt.raw)) + } + err := jf.RejectWrappedJSON(tt.outerKey, "") + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + assert.NoError(t, err) + }) + } +} + +func TestRejectWrappedJSONIncludesExample(t *testing.T) { + var jf JsonFlag + require.NoError(t, jf.Set(`{"role":{}}`)) + err := jf.RejectWrappedJSON("role", "databricks postgres create-role --json '{\"spec\":{}}'") + require.Error(t, err) + assert.Contains(t, err.Error(), "Example:") + assert.Contains(t, err.Error(), `--json '{"spec":{}}'`) +}