Skip to content

Commit f8f0230

Browse files
postgres: add --json body example to create-role help (#5111)
## Summary `databricks postgres create-role`'s `--json` flag binds to the inner `Role` object (`CreateRoleRequest.Role`, JSON-tagged `"role"`), so users must supply `spec` / `name` / etc. directly. Without an example this isn't obvious — the auto-generated help leaves the spec fields unflagged (`// TODO: complex arg: spec` in the generator), and the server's error when the body is wrong is vague: ``` Field 'role' is required and must contain at least one subfield with a non-default value ``` That fires whenever the inner `Role` has no recognized fields, which most commonly happens when a user wraps the body in `{"role": ...}` (matching the wire format the SDK marshals to). The CLI strips the unknown outer key with `Warning: unknown field: role` and ships an empty body. Walking out of that loop currently requires reading the SDK source. This adds a curated override (`cmd/workspace/postgres/overrides.go`) that appends a concrete service-principal-role example to `cmd.Long`, plus a short note on the wrapping pitfall. ### Help output (after) ``` Arguments: PARENT: The Branch where this Role is created. Format: projects/{project_id}/branches/{branch_id} Body shape (passed via --json): fields go directly on the Role object. Do not wrap them in '{"role": ...}' — the CLI will strip the unknown outer key and the server will reject the empty body with "Field 'role' is required". Example — create a service-principal-backed role: databricks postgres create-role projects/<PROJECT_ID>/branches/<BRANCH_ID> \ --role-id <SP_CLIENT_ID> \ --json '{"spec": {"identity_type": "SERVICE_PRINCIPAL", "postgres_role": "<SP_CLIENT_ID>", "auth_method": "LAKEBASE_OAUTH_V1", "membership_roles": ["DATABRICKS_SUPERUSER"]}}' ``` ### Scope This PR only touches `create-role`. The same shape gap (`// TODO: complex arg: spec` + opaque error) exists for `create-endpoint`, `create-branch`, `create-project`, and `create-database`. Happy to extend if the approach is right; left them out so reviewers can decide on the pattern first. ## Test plan - [x] `go build ./cmd/workspace/postgres/...` - [x] `databricks postgres create-role --help` shows the new section (output above) - [x] `make fmt` clean - [x] Reproduced the original confusion with a service-principal payload before the change; with this PR the example would have led me straight to the working body shape This pull request and its description were written by Isaac. --------- Co-authored-by: simon <4305831+simonfaltum@users.noreply.github.com> Co-authored-by: simon <simon.faltum@databricks.com>
1 parent 2cefbcc commit f8f0230

5 files changed

Lines changed: 217 additions & 0 deletions

File tree

NEXT_CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
* 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. Pick where they go with `--scope=project|global` (`--scope=both` is accepted on `update` and `list`).
1212
* `[__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`.
13+
* `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)).
1314

1415
### Bundles
1516
* Make sure warnings asking for approval are understood by agents ([#5239](https://github.com/databricks/cli/pull/5239))
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
package postgres
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
7+
"github.com/databricks/cli/libs/flags"
8+
"github.com/databricks/databricks-sdk-go/service/postgres"
9+
"github.com/spf13/cobra"
10+
)
11+
12+
// createRoleOverride appends an example body to the auto-generated help and
13+
// rejects wrapped {"role": ...} bodies with a clear client-side error.
14+
// The --json flag binds to the inner Role object (CreateRoleRequest.Role,
15+
// JSON-tagged "role"), so users supply spec/name/etc. directly. Without an
16+
// example, the auto-generated `// TODO: complex arg: spec` flags leave no
17+
// hint about the body shape and the API's "Field 'role' is required" error
18+
// is unhelpful when the request body is wrong.
19+
func createRoleOverride(createRoleCmd *cobra.Command, _ *postgres.CreateRoleRequest) {
20+
prevPreRunE := createRoleCmd.PreRunE
21+
createRoleCmd.PreRunE = func(cmd *cobra.Command, args []string) error {
22+
if err := rejectWrappedRoleJSON(cmd); err != nil {
23+
return err
24+
}
25+
if prevPreRunE != nil {
26+
return prevPreRunE(cmd, args)
27+
}
28+
return nil
29+
}
30+
31+
createRoleCmd.Long += `
32+
33+
Body shape (passed via --json): fields go directly on the Role object.
34+
Do not wrap them in '{"role": ...}' — the CLI rejects wrapped bodies
35+
client-side with a hint pointing to the right shape.
36+
37+
Example — create a service-principal-backed role:
38+
39+
databricks postgres create-role projects/<PROJECT_ID>/branches/<BRANCH_ID> \
40+
--role-id <SP_CLIENT_ID> \
41+
--json '{"spec": {"identity_type": "SERVICE_PRINCIPAL", "postgres_role": "<SP_CLIENT_ID>", "auth_method": "LAKEBASE_OAUTH_V1"}}'
42+
43+
The example omits 'membership_roles' so the role starts with default
44+
privileges only — grant database/schema/table access separately via
45+
SQL, following least privilege. Set 'membership_roles' (e.g.
46+
["DATABRICKS_SUPERUSER"]) only when broad administrative access is
47+
intentional.
48+
49+
See databricks-sdk-go/service/postgres.RoleRoleSpec for the full set of
50+
spec fields.`
51+
}
52+
53+
// rejectWrappedRoleJSON returns a clear error when --json is a top-level
54+
// object containing a "role" key. Without this guard the generated unmarshal
55+
// strips the unknown outer "role" field with a warning and ships an empty
56+
// body, and the server rejects with a confusing "Field 'role' is required"
57+
// message.
58+
func rejectWrappedRoleJSON(cmd *cobra.Command) error {
59+
// These checks are internal invariants — postgres create-role is a
60+
// generated command and always has a *flags.JsonFlag for --json. A
61+
// future codegen/refactor change could break that, and we want loud
62+
// breakage rather than a silently-disabled guard.
63+
flag := cmd.Flags().Lookup("json")
64+
if flag == nil {
65+
return errors.New("internal: postgres create-role expected a --json flag; this override is wired to the wrong command")
66+
}
67+
jf, ok := flag.Value.(*flags.JsonFlag)
68+
if !ok {
69+
return fmt.Errorf("internal: postgres create-role --json flag has unexpected type %T; expected *flags.JsonFlag", flag.Value)
70+
}
71+
return jf.RejectWrappedJSON("role", `databricks postgres create-role projects/<PROJECT_ID>/branches/<BRANCH_ID> \
72+
--role-id <SP_CLIENT_ID> \
73+
--json '{"spec": {"identity_type": "SERVICE_PRINCIPAL", "postgres_role": "<SP_CLIENT_ID>", "auth_method": "LAKEBASE_OAUTH_V1"}}'`)
74+
}
75+
76+
func init() {
77+
createRoleOverrides = append(createRoleOverrides, createRoleOverride)
78+
}
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package postgres
2+
3+
import (
4+
"testing"
5+
6+
"github.com/databricks/cli/libs/flags"
7+
"github.com/spf13/cobra"
8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
func cmdWithJSON(t *testing.T, raw string) *cobra.Command {
13+
t.Helper()
14+
cmd := &cobra.Command{}
15+
var jf flags.JsonFlag
16+
cmd.Flags().Var(&jf, "json", "JSON body")
17+
if raw != "" {
18+
require.NoError(t, jf.Set(raw))
19+
}
20+
return cmd
21+
}
22+
23+
func TestRejectWrappedRoleJSON(t *testing.T) {
24+
t.Run("rejects wrapped {role: ...}", func(t *testing.T) {
25+
cmd := cmdWithJSON(t, `{"role":{"spec":{"identity_type":"SERVICE_PRINCIPAL"}}}`)
26+
err := rejectWrappedRoleJSON(cmd)
27+
require.Error(t, err)
28+
assert.Contains(t, err.Error(), "should NOT be wrapped")
29+
assert.Contains(t, err.Error(), `databricks postgres create-role`)
30+
})
31+
32+
t.Run("passes when body has spec at top level", func(t *testing.T) {
33+
cmd := cmdWithJSON(t, `{"spec":{"identity_type":"SERVICE_PRINCIPAL"}}`)
34+
assert.NoError(t, rejectWrappedRoleJSON(cmd))
35+
})
36+
37+
t.Run("passes when --json was not provided", func(t *testing.T) {
38+
cmd := cmdWithJSON(t, "")
39+
assert.NoError(t, rejectWrappedRoleJSON(cmd))
40+
})
41+
42+
t.Run("passes through non-object JSON to the generated diagnostics path", func(t *testing.T) {
43+
cmd := cmdWithJSON(t, `"not-an-object"`)
44+
assert.NoError(t, rejectWrappedRoleJSON(cmd))
45+
})
46+
47+
t.Run("fails loudly when --json flag is absent on the command", func(t *testing.T) {
48+
// Internal invariant: postgres create-role is a generated command and
49+
// always has a --json flag. If a future codegen change drops it, this
50+
// override is wired to the wrong command and should fail loudly so the
51+
// regression is caught rather than silently disabling the guard.
52+
err := rejectWrappedRoleJSON(&cobra.Command{})
53+
require.Error(t, err)
54+
assert.Contains(t, err.Error(), "internal:")
55+
})
56+
}

libs/flags/json_flag.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,44 @@ func (j *JsonFlag) Unmarshal(v any) diag.Diagnostics {
113113
func (j *JsonFlag) Type() string {
114114
return "JSON"
115115
}
116+
117+
// Raw returns the raw JSON bytes the flag was set to (or nil when the flag
118+
// was not provided). Exposed so command overrides can do shape-level
119+
// validation before the generated Unmarshal call.
120+
func (j *JsonFlag) Raw() []byte {
121+
return j.raw
122+
}
123+
124+
// RejectWrappedJSON returns a clear client-side error when the --json body
125+
// is a top-level object containing outerKey. It detects the common mistake
126+
// of wrapping the body in '{"<outerKey>": ...}' when the flag binds to the
127+
// inner object (the SDK request type's JSON-tagged outer field). Returns
128+
// nil when the flag is unset, when the body isn't an object, or when no
129+
// outerKey is present at the top level.
130+
//
131+
// example, if non-empty, is appended to the error as a hint at the correct
132+
// shape. Callers typically construct it as a self-contained command line.
133+
//
134+
// This is the shared helper used by the postgres command overrides; the
135+
// same inner-body --json shape exists across siblings like create-branch,
136+
// create-database, create-endpoint, and create-project.
137+
func (j *JsonFlag) RejectWrappedJSON(outerKey, example string) error {
138+
if len(j.raw) == 0 {
139+
return nil
140+
}
141+
var top map[string]json.RawMessage
142+
if err := json.Unmarshal(j.raw, &top); err != nil {
143+
// Defer non-object inputs to the generated unmarshal so its
144+
// diagnostics render the original parse error.
145+
return nil //nolint:nilerr
146+
}
147+
if _, found := top[outerKey]; !found {
148+
return nil
149+
}
150+
msg := fmt.Sprintf("--json should NOT be wrapped in '{%q: ...}'.\n\n"+
151+
"The flag binds to the inner object — supply its fields directly.", outerKey)
152+
if example != "" {
153+
msg += "\n\nExample:\n\n " + example
154+
}
155+
return fmt.Errorf("%s", msg)
156+
}

libs/flags/json_flag_test.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,3 +284,44 @@ func TestJsonUnmarshalForRequestWithForceSendFields(t *testing.T) {
284284
assert.NotContains(t, r.NewSettings.NotificationSettings.ForceSendFields, "NoAlertForSkippedRuns")
285285
assert.Contains(t, r.NewSettings.NotificationSettings.ForceSendFields, "NoAlertForCanceledRuns")
286286
}
287+
288+
func TestRejectWrappedJSON(t *testing.T) {
289+
tests := []struct {
290+
name string
291+
raw string
292+
outerKey string
293+
wantErr string
294+
}{
295+
{name: "empty body passes", raw: "", outerKey: "role"},
296+
{name: "non-object body passes", raw: `"not-an-object"`, outerKey: "role"},
297+
{name: "object without outer key passes", raw: `{"spec":{"foo":"bar"}}`, outerKey: "role"},
298+
{name: "object with outer key rejected", raw: `{"role":{"spec":{"foo":"bar"}}}`, outerKey: "role", wantErr: `should NOT be wrapped`},
299+
{name: "object with outer key plus siblings still rejected", raw: `{"role":{},"other":1}`, outerKey: "role", wantErr: `should NOT be wrapped`},
300+
{name: "outer key differs from body's outer", raw: `{"branch":{}}`, outerKey: "role"},
301+
}
302+
303+
for _, tt := range tests {
304+
t.Run(tt.name, func(t *testing.T) {
305+
var jf JsonFlag
306+
if tt.raw != "" {
307+
require.NoError(t, jf.Set(tt.raw))
308+
}
309+
err := jf.RejectWrappedJSON(tt.outerKey, "")
310+
if tt.wantErr != "" {
311+
require.Error(t, err)
312+
assert.Contains(t, err.Error(), tt.wantErr)
313+
return
314+
}
315+
assert.NoError(t, err)
316+
})
317+
}
318+
}
319+
320+
func TestRejectWrappedJSONIncludesExample(t *testing.T) {
321+
var jf JsonFlag
322+
require.NoError(t, jf.Set(`{"role":{}}`))
323+
err := jf.RejectWrappedJSON("role", "databricks postgres create-role --json '{\"spec\":{}}'")
324+
require.Error(t, err)
325+
assert.Contains(t, err.Error(), "Example:")
326+
assert.Contains(t, err.Error(), `--json '{"spec":{}}'`)
327+
}

0 commit comments

Comments
 (0)