From cf16ee338814d95b8aaab75e9dccc7db50c5167b Mon Sep 17 00:00:00 2001 From: Daniel Spangenberger Date: Mon, 24 Aug 2026 11:05:04 -0400 Subject: [PATCH 1/5] feat(body): accept --body @file and validate JSON before sending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agents reach for curl syntax (--body @/tmp/body.json) or pass a bare file path. Both were sent verbatim as the request body, and the API answered {"detail": "Bad Request: Invalid JSON"} — a message that reads like a body-SHAPE problem and sends the caller back to re-read the schema for a mistake that was purely about transport. --body/--json-body now resolve "@path" (and curl's "@-") to file contents under the same 10 MB cap as stdin, and every JSON-media-type body is run through json.Valid before any network call. A value that looks like a path (/, ./, ../, ~/ prefix, or an existing file) gets an error naming both working forms instead of a parse error. Constraint: bytes must reach the server unchanged — validation uses json.Valid and never re-serializes, so field order and formatting survive Constraint: body shorthand sets --body internally to marshaled JSON; that path stays valid and untouched Rejected: schema-aware validation of the body | needs the full JSON Schema evaluator and would reject bodies the API actually accepts Rejected: silently treating a bare existing path as a file | hides the typo class this is meant to surface, and changes what an existing script sends Confidence: high Scope-risk: narrow Directive: multipart/form-data operations (uploads) skip validation via operationInfo.BodyNonJSON — keep that carve-out if more media types appear Not-tested: reading from a FIFO or /dev/stdin via @path Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s --- README.md | 5 + cmd/omni/agent_help.go | 3 +- internal/openapi/body_input.go | 173 +++++++++++++ internal/openapi/body_input_test.go | 377 ++++++++++++++++++++++++++++ internal/openapi/generate.go | 46 ++-- 5 files changed, 589 insertions(+), 15 deletions(-) create mode 100644 internal/openapi/body_input.go create mode 100644 internal/openapi/body_input_test.go diff --git a/README.md b/README.md index 44a6a64..d692638 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,11 @@ After installing, restart your shell and try: `omni `, `omni ai `, `om The CLI embeds the OpenAPI spec (`api/openapi.json`) into the binary. At startup it parses the spec and generates cobra subcommands for every operation. Each API tag becomes a command group, path params become positional args, query params become flags, and request bodies are passed via `--body` or stdin. +A request body can be given three ways: inline JSON (`--body '{"name":"x"}'`), a file +(`--body @path/to/body.json`), or stdin (`--body - < path/to/body.json`). The body is +checked as JSON before the request is sent, so a typo fails locally instead of coming +back as a generic API 400. + Adding a new API endpoint requires no code changes — update `api/openapi.json` (or run `make sync-spec`) and rebuild. ## Auth diff --git a/cmd/omni/agent_help.go b/cmd/omni/agent_help.go index e70be6f..ec29b14 100644 --- a/cmd/omni/agent_help.go +++ b/cmd/omni/agent_help.go @@ -99,7 +99,8 @@ name a leaf without knowing the container shape: --token TOKEN API token (overrides env/config) --base-url URL API base URL (overrides config) --profile NAME Config profile to use - --body JSON Request body (JSON string or "-" for stdin) + --body JSON Request body: JSON string, @path/to/file.json, or "-" for stdin + (a bare path is rejected client-side — prefix it with @) --schema Print the request body's schema + example, then exit --field PATH With --schema: drill into a dotted field path --depth N With --schema: cap nesting depth (lower = smaller) diff --git a/internal/openapi/body_input.go b/internal/openapi/body_input.go new file mode 100644 index 0000000..72a0d87 --- /dev/null +++ b/internal/openapi/body_input.go @@ -0,0 +1,173 @@ +package openapi + +import ( + "encoding/json" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +// resolveBody turns a raw --body/--json-body flag value into the bytes to send. +// +// "-" read from stdin +// "@path" read from a file ("@-" is stdin too, curl-style) +// anything else is the literal body +// +// When validateJSON is set the result is checked with json.Valid before any +// network call, because the API answers a non-JSON body with a generic +// "Invalid JSON" 400 that reads like a body-shape problem. flagName is the flag +// the value came from, so the error text names the flag the caller typed. +// +// The bytes are never re-serialized — what the caller supplied is what gets +// sent. +func resolveBody(raw, flagName string, validateJSON bool) ([]byte, error) { + switch { + case raw == "-": + data, err := readStdin() + if err != nil { + return nil, fmt.Errorf("reading stdin: %w", err) + } + return checkBody(data, raw, flagName, "stdin", validateJSON) + + case strings.HasPrefix(raw, "@"): + path := strings.TrimPrefix(raw, "@") + if path == "-" { + data, err := readStdin() + if err != nil { + return nil, fmt.Errorf("reading stdin: %w", err) + } + return checkBody(data, raw, flagName, "stdin", validateJSON) + } + if strings.TrimSpace(path) == "" { + return nil, fmt.Errorf("--%s: no file path after \"@\"; use --%s @path/to/body.json", flagName, flagName) + } + path = expandHome(path) + data, err := readBodyFile(path) + if err != nil { + return nil, err + } + return checkBody(data, raw, flagName, fmt.Sprintf("file %s", path), validateJSON) + + default: + return checkBody([]byte(raw), raw, flagName, "", validateJSON) + } +} + +// readBodyFile reads a body file, enforcing the same size cap as stdin. +func readBodyFile(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("body file not found: %s", path) + } + return nil, fmt.Errorf("reading body file %s: %w", path, err) + } + defer f.Close() + + data, err := io.ReadAll(io.LimitReader(f, maxStdinSize+1)) + if err != nil { + return nil, fmt.Errorf("reading body file %s: %w", path, err) + } + if len(data) > maxStdinSize { + return nil, fmt.Errorf("body file %s exceeds maximum size of 10 MB", path) + } + return data, nil +} + +// checkBody validates data and returns it unchanged. source describes where +// the bytes came from ("stdin", "file X") or is empty when they came straight +// from the flag value — only in that case can the raw value itself be a +// mistyped file path. Operations whose request body isn't JSON (e.g. the +// multipart upload endpoints) pass validateJSON=false and get the bytes back +// untouched. +func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([]byte, error) { + if !validateJSON { + return data, nil + } + + if len(strings.TrimSpace(string(data))) == 0 { + if source != "" { + return nil, fmt.Errorf("no request body read from %s", source) + } + return nil, fmt.Errorf("--%s is empty; omit the flag to send no body", flagName) + } + + if json.Valid(data) { + return data, nil + } + + if source == "" && looksLikePath(raw) { + return nil, fmt.Errorf("--%s looks like a file path, not JSON: %s\nread the file instead: --%s @%s (or --%s - < %s)", + flagName, raw, flagName, raw, flagName, raw) + } + + where := "--" + flagName + if source != "" { + where = source + } + return nil, fmt.Errorf("request body from %s is not valid JSON: %s", where, jsonProblem(data)) +} + +// looksLikePath reports whether a flag value is more plausibly a file path than +// a JSON document — an absolute/relative path prefix, or the name of a file +// that actually exists. +func looksLikePath(raw string) bool { + if raw == "" || strings.ContainsAny(raw, " \t\r\n") { + return false + } + for _, prefix := range []string{"/", "./", "../", "~/"} { + if strings.HasPrefix(raw, prefix) { + return true + } + } + if info, err := os.Stat(expandHome(raw)); err == nil && !info.IsDir() { + return true + } + return false +} + +// jsonProblem renders a short parse diagnostic: the syntax error plus the few +// bytes around the offset it points at. +func jsonProblem(data []byte) string { + var v json.RawMessage + err := json.Unmarshal(data, &v) + if err == nil { + return "unexpected trailing data" + } + + syn, ok := err.(*json.SyntaxError) + if !ok { + return err.Error() + } + + offset := int(syn.Offset) + if offset < 0 || offset > len(data) { + return syn.Error() + } + start := offset - 20 + if start < 0 { + start = 0 + } + end := offset + 20 + if end > len(data) { + end = len(data) + } + return fmt.Sprintf("%s (at byte %d, near %q)", syn.Error(), offset, string(data[start:end])) +} + +// expandHome expands a leading "~/" — shells don't expand it after "@". +func expandHome(path string) string { + if path != "~" && !strings.HasPrefix(path, "~/") { + return path + } + home, err := os.UserHomeDir() + if err != nil { + return path + } + if path == "~" { + return home + } + return filepath.Join(home, strings.TrimPrefix(path, "~/")) +} diff --git a/internal/openapi/body_input_test.go b/internal/openapi/body_input_test.go new file mode 100644 index 0000000..e6fc010 --- /dev/null +++ b/internal/openapi/body_input_test.go @@ -0,0 +1,377 @@ +package openapi + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// --------------------------------------------------------------------------- +// --body transport +// +// The API answers any non-JSON body with a generic 400 "Invalid JSON", which +// reads like a body-SHAPE problem. These tests pin the client-side handling +// that keeps callers out of that dead end: @file input, and rejecting a body +// that isn't JSON before any network call. +// --------------------------------------------------------------------------- + +// writeTempBody writes content to a temp file and returns its path. +func writeTempBody(t *testing.T, name, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("writing temp body: %v", err) + } + return path +} + +// "--body @file.json" reads the body from disk, byte for byte. +func TestResolveBody_AtFile(t *testing.T) { + content := `{"modelId":"abc","prompt":"hi"}` + path := writeTempBody(t, "body.json", content) + + got, err := resolveBody("@"+path, "body", true) + if err != nil { + t.Fatalf("resolveBody: %v", err) + } + if string(got) != content { + t.Errorf("body = %q, want %q", string(got), content) + } +} + +// A missing @file is a client-side error that names the path. +func TestResolveBody_AtFileMissing(t *testing.T) { + missing := filepath.Join(t.TempDir(), "nope.json") + + _, err := resolveBody("@"+missing, "body", true) + if err == nil { + t.Fatal("expected an error for a missing body file, got nil") + } + if !strings.Contains(err.Error(), missing) { + t.Errorf("error = %q, want it to name the path %q", err.Error(), missing) + } + if !strings.Contains(err.Error(), "not found") { + t.Errorf("error = %q, want it to say the file was not found", err.Error()) + } +} + +// The 10 MB stdin cap applies to @file input too. +func TestResolveBody_AtFileTooLarge(t *testing.T) { + path := writeTempBody(t, "big.json", strings.Repeat("x", maxStdinSize+1)) + + _, err := resolveBody("@"+path, "body", true) + if err == nil { + t.Fatal("expected an error for an oversized body file, got nil") + } + if !strings.Contains(err.Error(), "10 MB") { + t.Errorf("error = %q, want it to mention 10 MB", err.Error()) + } +} + +// "@" with no path is a usage error, not a stat of the empty string. +func TestResolveBody_AtWithoutPath(t *testing.T) { + if _, err := resolveBody("@", "body", true); err == nil { + t.Fatal("expected an error for a bare @, got nil") + } +} + +// A file whose contents aren't JSON is rejected before the request is made. +func TestResolveBody_AtFileInvalidJSON(t *testing.T) { + path := writeTempBody(t, "body.json", "modelId: abc\n") + + _, err := resolveBody("@"+path, "body", true) + if err == nil { + t.Fatal("expected an error for a non-JSON body file, got nil") + } + if !strings.Contains(err.Error(), "not valid JSON") { + t.Errorf("error = %q, want it to say the body is not valid JSON", err.Error()) + } + if !strings.Contains(err.Error(), path) { + t.Errorf("error = %q, want it to name the file %q", err.Error(), path) + } +} + +// The curl-style mistake: passing a bare file path as the body. The error must +// point at the @file / stdin forms rather than leave the caller re-reading the +// body schema. +func TestResolveBody_BareFilePathHint(t *testing.T) { + path := writeTempBody(t, "body.json", `{"a":1}`) + + for _, raw := range []string{path, "/tmp/does-not-exist.json", "./body.json", "~/body.json"} { + _, err := resolveBody(raw, "body", true) + if err == nil { + t.Fatalf("resolveBody(%q): expected an error, got nil", raw) + } + msg := err.Error() + if !strings.Contains(msg, "looks like a file path") { + t.Errorf("resolveBody(%q) error = %q, want the file-path hint", raw, msg) + } + if !strings.Contains(msg, "--body @"+raw) { + t.Errorf("resolveBody(%q) error = %q, want it to suggest --body @%s", raw, msg, raw) + } + if !strings.Contains(msg, "--body - < "+raw) { + t.Errorf("resolveBody(%q) error = %q, want it to suggest the stdin form", raw, msg) + } + } +} + +// Non-JSON that isn't path-shaped gets a plain parse error with a position. +func TestResolveBody_InvalidJSONDiagnostic(t *testing.T) { + _, err := resolveBody(`{"a":1,}`, "body", true) + if err == nil { + t.Fatal("expected an error for malformed JSON, got nil") + } + msg := err.Error() + if !strings.Contains(msg, "not valid JSON") { + t.Errorf("error = %q, want it to say the body is not valid JSON", msg) + } + if !strings.Contains(msg, "byte ") { + t.Errorf("error = %q, want it to include the parse position", msg) + } + if strings.Contains(msg, "looks like a file path") { + t.Errorf("error = %q, should not offer the file-path hint here", msg) + } +} + +// The error names whichever flag the caller actually used. +func TestResolveBody_NamesTheFlagUsed(t *testing.T) { + _, err := resolveBody("not json", "json-body", true) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "--json-body") { + t.Errorf("error = %q, want it to name --json-body", err.Error()) + } +} + +// Valid JSON passes through byte-identical: no reformatting, no re-ordering. +func TestResolveBody_ValidJSONPassthrough(t *testing.T) { + cases := []string{ + `{"b":1, "a": [1,2,3]}`, + ` {"padded": true} `, + `[1,2,3]`, + `"a bare string"`, + `null`, + } + for _, raw := range cases { + got, err := resolveBody(raw, "body", true) + if err != nil { + t.Fatalf("resolveBody(%q): %v", raw, err) + } + if string(got) != raw { + t.Errorf("resolveBody(%q) = %q, want the bytes unchanged", raw, string(got)) + } + } +} + +// Non-JSON media types (the multipart upload endpoints) skip validation. +func TestResolveBody_SkipsValidationForNonJSON(t *testing.T) { + raw := "--boundary\r\nnot json\r\n" + got, err := resolveBody(raw, "body", false) + if err != nil { + t.Fatalf("resolveBody: %v", err) + } + if string(got) != raw { + t.Errorf("body = %q, want it unchanged", string(got)) + } +} + +// "-" still reads stdin, and stdin is validated the same way. +func TestResolveBody_Stdin(t *testing.T) { + withStdin(t, `{"from":"stdin"}`, func() { + got, err := resolveBody("-", "body", true) + if err != nil { + t.Fatalf("resolveBody: %v", err) + } + if string(got) != `{"from":"stdin"}` { + t.Errorf("body = %q, want the stdin bytes", string(got)) + } + }) + + withStdin(t, "not json", func() { + _, err := resolveBody("-", "body", true) + if err == nil { + t.Fatal("expected an error for non-JSON stdin, got nil") + } + if !strings.Contains(err.Error(), "stdin") { + t.Errorf("error = %q, want it to name stdin as the source", err.Error()) + } + }) +} + +// "@-" is the curl spelling of stdin. +func TestResolveBody_AtDashIsStdin(t *testing.T) { + withStdin(t, `{"from":"stdin"}`, func() { + got, err := resolveBody("@-", "body", true) + if err != nil { + t.Fatalf("resolveBody: %v", err) + } + if string(got) != `{"from":"stdin"}` { + t.Errorf("body = %q, want the stdin bytes", string(got)) + } + }) +} + +// An empty source is reported as empty rather than as a JSON syntax error. +func TestResolveBody_EmptySources(t *testing.T) { + path := writeTempBody(t, "empty.json", "") + if _, err := resolveBody("@"+path, "body", true); err == nil { + t.Fatal("expected an error for an empty body file, got nil") + } else if !strings.Contains(err.Error(), "no request body") { + t.Errorf("error = %q, want it to report an empty body", err.Error()) + } + + if _, err := resolveBody(" ", "body", true); err == nil { + t.Fatal("expected an error for a whitespace-only --body, got nil") + } +} + +// withStdin swaps os.Stdin for a pipe carrying content for the duration of fn. +func withStdin(t *testing.T, content string, fn func()) { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + orig := os.Stdin + os.Stdin = r + defer func() { os.Stdin = orig; r.Close() }() + + go func() { + w.Write([]byte(content)) + w.Close() + }() + fn() +} + +// --------------------------------------------------------------------------- +// End-to-end through a generated command +// --------------------------------------------------------------------------- + +func bodyCmd(t *testing.T, exec Executor) *cobra.Command { + t.Helper() + cmd := buildCommand(&operationInfo{ + Tag: "test", + OperationID: "testCreateWidget", + Method: "POST", + Path: "/api/v1/widgets", + HasBody: true, + }, exec) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + return cmd +} + +// "omni ... --body @file.json" sends the file's bytes. +func TestBuildCommand_BodyAtFile(t *testing.T) { + content := `{"key":"val"}` + path := writeTempBody(t, "body.json", content) + + var captured APIRequest + cmd := bodyCmd(t, func(req APIRequest) error { captured = req; return nil }) + cmd.SetArgs([]string{"--body", "@" + path}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if string(captured.Body) != content { + t.Errorf("body = %q, want %q", string(captured.Body), content) + } +} + +// The hidden --json-body alias gets identical treatment. +func TestBuildCommand_JSONBodyAtFile(t *testing.T) { + content := `{"key":"val"}` + path := writeTempBody(t, "body.json", content) + + var captured APIRequest + cmd := bodyCmd(t, func(req APIRequest) error { captured = req; return nil }) + cmd.SetArgs([]string{"--json-body", "@" + path}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if string(captured.Body) != content { + t.Errorf("body = %q, want %q", string(captured.Body), content) + } +} + +// An invalid body must fail before the executor runs — no request is made. +func TestBuildCommand_InvalidBodyMakesNoRequest(t *testing.T) { + called := false + cmd := bodyCmd(t, func(req APIRequest) error { called = true; return nil }) + cmd.SetArgs([]string{"--body", "/tmp/some-body.json"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected an error for a bare file path, got nil") + } + if called { + t.Error("executor ran despite an invalid body") + } + if !strings.Contains(err.Error(), "looks like a file path") { + t.Errorf("error = %q, want the file-path hint", err.Error()) + } +} + +// Body shorthand assembles its own JSON and must not trip the new validation. +func TestBodyShorthand_SurvivesBodyValidation(t *testing.T) { + var captured APIRequest + op := &operationInfo{ + Tag: "ai", + OperationID: "aiSearchOmniDocs", + Method: "POST", + Path: "/api/v1/ai/search-omni-docs", + HasBody: true, + } + cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"How do I add a format to a dimension?"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if string(captured.Body) != `{"question":"How do I add a format to a dimension?"}` { + t.Errorf("body = %q, want the assembled shorthand JSON", string(captured.Body)) + } +} + +// Shorthand commands accept --body @file for the full JSON form. +func TestBodyShorthand_AtFileBody(t *testing.T) { + content := `{"question":"why?"}` + path := writeTempBody(t, "body.json", content) + + var captured APIRequest + op := &operationInfo{ + Tag: "ai", + OperationID: "aiSearchOmniDocs", + Method: "POST", + Path: "/api/v1/ai/search-omni-docs", + HasBody: true, + } + cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--body", "@" + path}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if string(captured.Body) != content { + t.Errorf("body = %q, want %q", string(captured.Body), content) + } +} + +func TestExpandHome(t *testing.T) { + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home dir") + } + if got := expandHome("~/x.json"); got != filepath.Join(home, "x.json") { + t.Errorf("expandHome(~/x.json) = %q, want %q", got, filepath.Join(home, "x.json")) + } + // "~" only expands as a path prefix, never mid-string. + if got := expandHome("/tmp/~/x.json"); got != "/tmp/~/x.json" { + t.Errorf("expandHome = %q, want it unchanged", got) + } +} diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index a0662bb..ee9988f 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -92,6 +92,7 @@ type operationInfo struct { QueryParams []paramInfo HasBody bool BodySchema *base.SchemaProxy // request body schema, when HasBody + BodyNonJSON bool // request body uses a non-JSON media type (e.g. multipart) Deprecated bool } @@ -159,6 +160,7 @@ func extractOperations(pathStr string, item *v3.PathItem, groups map[string][]*o if op.RequestBody != nil { info.HasBody = true info.BodySchema = requestBodySchema(op.RequestBody) + info.BodyNonJSON = !requestBodyIsJSON(op.RequestBody) } groups[tag] = append(groups[tag], info) @@ -212,7 +214,7 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { path += "?" + query.Encode() } - // Read body from stdin or flags + // Read body from stdin, a file, or the flag value itself var body []byte if op.HasBody { bodyFlag, _ := cmd.Flags().GetString("body") @@ -222,21 +224,20 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { return fmt.Errorf("cannot use both --body and --json-body; use one or the other") } - effectiveBody := bodyFlag + effectiveBody, flagName := bodyFlag, "body" if jsonBodyFlag != "" { - effectiveBody = jsonBodyFlag + effectiveBody, flagName = jsonBodyFlag, "json-body" } - if effectiveBody == "-" || effectiveBody == "" { - if effectiveBody == "-" { - var err error - body, err = readStdin() - if err != nil { - return fmt.Errorf("reading stdin: %w", err) - } + if effectiveBody != "" { + var err error + body, err = resolveBody(effectiveBody, flagName, !op.BodyNonJSON) + if err != nil { + // A body input error is self-explanatory; the usage + // block would bury the hint. + cmd.SilenceUsage = true + return err } - } else if effectiveBody != "" { - body = []byte(effectiveBody) } } @@ -261,8 +262,8 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { // If the operation accepts a body, add --body and --json-body flags if op.HasBody { - cmd.Flags().String("body", "", `request body as JSON string, or "-" for stdin (run with --schema to see its shape)`) - cmd.Flags().String("json-body", "", `request body as JSON string, or "-" for stdin (alias for --body)`) + cmd.Flags().String("body", "", `request body as JSON string, "@path/to/file.json" to read a file, or "-" for stdin (run with --schema to see its shape)`) + cmd.Flags().String("json-body", "", `request body as JSON string, "@path/to/file.json", or "-" for stdin (alias for --body)`) cmd.Flags().MarkHidden("json-body") } @@ -337,6 +338,23 @@ func requestBodySchema(rb *v3.RequestBody) *base.SchemaProxy { return first } +// requestBodyIsJSON reports whether a request body is sent as JSON. Bodies with +// no declared content default to JSON — that's what the CLI sends. Only the +// media types the spec actually declares (today: multipart/form-data uploads) +// opt out of client-side JSON validation. +func requestBodyIsJSON(rb *v3.RequestBody) bool { + if rb == nil || rb.Content == nil || rb.Content.Len() == 0 { + return true + } + for pair := rb.Content.First(); pair != nil; pair = pair.Next() { + mt := pair.Key() + if mt == "application/json" || strings.HasSuffix(mt, "+json") { + return true + } + } + return false +} + // commandName derives a CLI subcommand name from the operationId or method+path. func commandName(op *operationInfo) string { if op.OperationID != "" { From 08277387d0962c4215f159eb6e6483389cc4054e Mon Sep 17 00:00:00 2001 From: Daniel Spangenberger Date: Mon, 24 Aug 2026 12:18:14 -0400 Subject: [PATCH 2/5] fix(body): reach the transport hint in three cases review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #75 found three ways the new diagnostics went unreached: 1. `--body ''` never got validated — the resolve step keyed off a non-empty string, so the explicit-empty error was dead code. It now keys off Flag.Changed, which distinguishes "flag omitted" (send no body, unchanged) from "flag typed as empty" (usually an unexpanded shell variable). The shorthand wrapper defers to the same path instead of quietly assembling a body from positional args. 2. A quoted path with spaces got a JSON parse error instead of the hint — looksLikePath bailed on whitespace before it checked prefixes or the filesystem, but the shell strips the quotes, so those values arrive looking ordinary. The whitespace guard now only narrows the stat() branch, to newlines, and the suggested commands are re-quoted so they paste back. 3. Multipart operations skipped path detection along with JSON validation, so the endpoints where "--body @file" is the whole point sent the path literally. Transport checks (empty input, path-shaped value) now run for every media type; only json.Valid stays conditional. Constraint: valid JSON is tested before path-shape, so a file that happens to be named "{}" can't hijack a legitimate body Rejected: treating `--body ''` as "send no body" | it is indistinguishable from an unexpanded "$BODY" and silently posts nothing Confidence: high Scope-risk: narrow Not-tested: a shorthand command given `--body ''` and zero positional args — the arg validator rejects it first Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s --- internal/openapi/body_input.go | 45 ++++++--- internal/openapi/body_input_test.go | 150 ++++++++++++++++++++++++++++ internal/openapi/body_shorthand.go | 7 ++ internal/openapi/generate.go | 7 +- 4 files changed, 194 insertions(+), 15 deletions(-) diff --git a/internal/openapi/body_input.go b/internal/openapi/body_input.go index 72a0d87..735c875 100644 --- a/internal/openapi/body_input.go +++ b/internal/openapi/body_input.go @@ -79,14 +79,13 @@ func readBodyFile(path string) ([]byte, error) { // checkBody validates data and returns it unchanged. source describes where // the bytes came from ("stdin", "file X") or is empty when they came straight // from the flag value — only in that case can the raw value itself be a -// mistyped file path. Operations whose request body isn't JSON (e.g. the -// multipart upload endpoints) pass validateJSON=false and get the bytes back -// untouched. +// mistyped file path. +// +// Transport checks (empty input, a path-shaped flag value) run for every +// operation; only the JSON validity check is conditional, so the multipart +// upload endpoints still get the "--body @path" hint they'd otherwise need +// most. func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([]byte, error) { - if !validateJSON { - return data, nil - } - if len(strings.TrimSpace(string(data))) == 0 { if source != "" { return nil, fmt.Errorf("no request body read from %s", source) @@ -94,13 +93,19 @@ func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([] return nil, fmt.Errorf("--%s is empty; omit the flag to send no body", flagName) } - if json.Valid(data) { + // Valid JSON is never a path, so this test comes first: it stops a file + // that happens to be named "{}" from hijacking a legitimate body. + if validateJSON && json.Valid(data) { return data, nil } if source == "" && looksLikePath(raw) { - return nil, fmt.Errorf("--%s looks like a file path, not JSON: %s\nread the file instead: --%s @%s (or --%s - < %s)", - flagName, raw, flagName, raw, flagName, raw) + return nil, fmt.Errorf("--%s looks like a file path, not a request body: %s\nread the file instead: --%s %s (or --%s - < %s)", + flagName, raw, flagName, shellQuote("@"+raw), flagName, shellQuote(raw)) + } + + if !validateJSON { + return data, nil } where := "--" + flagName @@ -110,11 +115,21 @@ func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([] return nil, fmt.Errorf("request body from %s is not valid JSON: %s", where, jsonProblem(data)) } +// shellQuote wraps a value in double quotes when it contains whitespace, so the +// suggested command in an error message can be pasted as-is. +func shellQuote(s string) string { + if !strings.ContainsAny(s, " \t") { + return s + } + return `"` + strings.ReplaceAll(s, `"`, `\"`) + `"` +} + // looksLikePath reports whether a flag value is more plausibly a file path than -// a JSON document — an absolute/relative path prefix, or the name of a file -// that actually exists. +// a request body — an absolute/relative path prefix, or the name of a file that +// actually exists. Paths with spaces count: the shell strips the quotes, so the +// value arrives here looking ordinary. func looksLikePath(raw string) bool { - if raw == "" || strings.ContainsAny(raw, " \t\r\n") { + if raw == "" { return false } for _, prefix := range []string{"/", "./", "../", "~/"} { @@ -122,6 +137,10 @@ func looksLikePath(raw string) bool { return true } } + // A multi-line value is a pasted document, not a path worth stat-ing. + if strings.ContainsAny(raw, "\r\n") { + return false + } if info, err := os.Stat(expandHome(raw)); err == nil && !info.IsDir() { return true } diff --git a/internal/openapi/body_input_test.go b/internal/openapi/body_input_test.go index e6fc010..77edaf3 100644 --- a/internal/openapi/body_input_test.go +++ b/internal/openapi/body_input_test.go @@ -167,6 +167,27 @@ func TestResolveBody_ValidJSONPassthrough(t *testing.T) { } } +// A quoted path with spaces reaches us looking ordinary — it still gets the +// hint, and the suggested commands come back re-quoted so they can be pasted. +func TestResolveBody_PathWithSpacesHint(t *testing.T) { + path := writeTempBody(t, "request body.json", `{"a":1}`) + + _, err := resolveBody(path, "body", true) + if err == nil { + t.Fatalf("resolveBody(%q): expected an error, got nil", path) + } + msg := err.Error() + if !strings.Contains(msg, "looks like a file path") { + t.Errorf("error = %q, want the file-path hint", msg) + } + if !strings.Contains(msg, `--body "@`+path+`"`) { + t.Errorf("error = %q, want a quoted --body @%s suggestion", msg, path) + } + if !strings.Contains(msg, `--body - < "`+path+`"`) { + t.Errorf("error = %q, want a quoted stdin suggestion", msg) + } +} + // Non-JSON media types (the multipart upload endpoints) skip validation. func TestResolveBody_SkipsValidationForNonJSON(t *testing.T) { raw := "--boundary\r\nnot json\r\n" @@ -179,6 +200,31 @@ func TestResolveBody_SkipsValidationForNonJSON(t *testing.T) { } } +// Skipping JSON validation must not skip the transport diagnostics: a +// multipart operation handed a bare path still gets the @file hint. +func TestResolveBody_NonJSONStillGetsPathHint(t *testing.T) { + _, err := resolveBody("/tmp/request.multipart", "body", false) + if err == nil { + t.Fatal("expected the file-path hint for a non-JSON body, got nil") + } + if !strings.Contains(err.Error(), "--body @/tmp/request.multipart") { + t.Errorf("error = %q, want it to suggest --body @/tmp/request.multipart", err.Error()) + } + + // An existing file named as a bare path counts too, whatever its contents. + path := writeTempBody(t, "upload.bin", "\x00\x01binary") + if _, err := resolveBody(path, "body", false); err == nil { + t.Fatalf("resolveBody(%q): expected the file-path hint, got nil", path) + } +} + +// The empty-body error is a transport check, so it applies to every media type. +func TestResolveBody_EmptyNonJSON(t *testing.T) { + if _, err := resolveBody("", "body", false); err == nil { + t.Fatal("expected an error for an explicitly empty --body, got nil") + } +} + // "-" still reads stdin, and stdin is validated the same way. func TestResolveBody_Stdin(t *testing.T) { withStdin(t, `{"from":"stdin"}`, func() { @@ -315,6 +361,110 @@ func TestBuildCommand_InvalidBodyMakesNoRequest(t *testing.T) { } } +// A multipart operation keeps the transport diagnostics but not the JSON +// validity check. +func TestBuildCommand_NonJSONBodyOperation(t *testing.T) { + op := &operationInfo{ + Tag: "uploads", + OperationID: "uploadsCreate", + Method: "POST", + Path: "/api/v1/uploads", + HasBody: true, + BodyNonJSON: true, + } + + // Non-JSON content goes through untouched. + var captured APIRequest + cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) + cmd.SilenceUsage, cmd.SilenceErrors = true, true + cmd.SetArgs([]string{"--body", "not json at all"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if string(captured.Body) != "not json at all" { + t.Errorf("body = %q, want it unchanged", string(captured.Body)) + } + + // A bare path still gets caught before the request. + called := false + cmd2 := buildCommand(op, func(req APIRequest) error { called = true; return nil }) + cmd2.SilenceUsage, cmd2.SilenceErrors = true, true + cmd2.SetArgs([]string{"--body", "/tmp/upload.csv"}) + err := cmd2.Execute() + if err == nil { + t.Fatal("expected the file-path hint, got nil") + } + if called { + t.Error("executor ran with a path as the body") + } + if !strings.Contains(err.Error(), "--body @/tmp/upload.csv") { + t.Errorf("error = %q, want it to suggest --body @/tmp/upload.csv", err.Error()) + } +} + +// An explicitly empty --body is a typo (usually a shell variable that didn't +// expand), not a request to send nothing. Omitting the flag still sends no body. +func TestBuildCommand_ExplicitlyEmptyBody(t *testing.T) { + for _, flag := range []string{"--body", "--json-body"} { + called := false + cmd := bodyCmd(t, func(req APIRequest) error { called = true; return nil }) + cmd.SetArgs([]string{flag, ""}) + + err := cmd.Execute() + if err == nil { + t.Fatalf("%s '': expected an error, got nil", flag) + } + if called { + t.Errorf("%s '': executor ran despite an empty body", flag) + } + if !strings.Contains(err.Error(), flag+" is empty") { + t.Errorf("%s '' error = %q, want it to report the empty flag", flag, err.Error()) + } + } + + // Omitting the flag entirely is still a bodiless request. + var captured APIRequest + sent := false + cmd := bodyCmd(t, func(req APIRequest) error { captured = req; sent = true; return nil }) + cmd.SetArgs([]string{}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute without --body: %v", err) + } + if !sent { + t.Fatal("executor did not run without --body") + } + if captured.Body != nil { + t.Errorf("body = %q, want nil when --body is omitted", string(captured.Body)) + } +} + +// The same holds on a shorthand command: an empty --body isn't shorthand input. +func TestBodyShorthand_ExplicitlyEmptyBody(t *testing.T) { + called := false + op := &operationInfo{ + Tag: "ai", + OperationID: "aiSearchOmniDocs", + Method: "POST", + Path: "/api/v1/ai/search-omni-docs", + HasBody: true, + } + cmd := buildCommand(op, func(req APIRequest) error { called = true; return nil }) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--body", "", "some question"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected an error for an empty --body, got nil") + } + if called { + t.Error("executor ran despite an empty body") + } + if !strings.Contains(err.Error(), "--body is empty") { + t.Errorf("error = %q, want it to report the empty flag", err.Error()) + } +} + // Body shorthand assembles its own JSON and must not trip the new validation. func TestBodyShorthand_SurvivesBodyValidation(t *testing.T) { var captured APIRequest diff --git a/internal/openapi/body_shorthand.go b/internal/openapi/body_shorthand.go index d034205..d5406e5 100644 --- a/internal/openapi/body_shorthand.go +++ b/internal/openapi/body_shorthand.go @@ -326,6 +326,13 @@ func applyBodyShorthand(cmd *cobra.Command, op *operationInfo, sh *BodyShorthand return originalRunE(cmd, args) } + // An explicitly empty --body/--json-body is a typo, not an invitation + // to assemble one from shorthand input. Hand it back to the generated + // RunE so it reports the same error every other command does. + if cmd.Flags().Changed("body") || cmd.Flags().Changed("json-body") { + return originalRunE(cmd, args[:numPathParams]) + } + // Assemble body from shorthand args and promoted flags body, err := assembleBody(sh, args, numPathParams, cmd, op.BodySchema) if err != nil { diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index ee9988f..85328b9 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -224,12 +224,15 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { return fmt.Errorf("cannot use both --body and --json-body; use one or the other") } + // An explicitly empty --body is a mistake worth reporting, so + // track whether the flag was typed at all rather than inferring + // it from the value. effectiveBody, flagName := bodyFlag, "body" - if jsonBodyFlag != "" { + if jsonBodyFlag != "" || (cmd.Flags().Changed("json-body") && !cmd.Flags().Changed("body")) { effectiveBody, flagName = jsonBodyFlag, "json-body" } - if effectiveBody != "" { + if effectiveBody != "" || cmd.Flags().Changed(flagName) { var err error body, err = resolveBody(effectiveBody, flagName, !op.BodyNonJSON) if err != nil { From 3d9b0e1859613faaa6e3234c43435faff7cc6ec3 Mon Sep 17 00:00:00 2001 From: Daniel Spangenberger Date: Mon, 24 Aug 2026 21:45:02 -0400 Subject: [PATCH 3/5] fix(body): select body mode by flag presence, refuse multipart endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Body-mode selection compared flag values, so `--body ''` looked unset: shorthand commands demanded their positional args and never reached the "--body is empty" diagnostic, and `--body '' --json-body '{}'` slipped past the mutual-exclusion check. Both now key off Changed. Multipart uploads were advertised as callable with `--body @file`, but auth.Do labels every request application/json and the CLI builds no part framing, so those bytes could never satisfy the API. Those operations now fail client-side naming the media type and printing the equivalent curl command, and the help text, agent-help and README say the same. Constraint: auth.Do carries no media type — a body is always sent as application/json Rejected: implement real multipart transport | needs media type plumbing through APIRequest plus part construction; out of scope for a bug-fix pass Rejected: send raw bytes for multipart ops as before | mislabeled and unframed, guaranteed API rejection dressed up as CLI support Confidence: high Scope-risk: narrow Directive: if multipart is ever implemented, drop unsupportedBodyError together with the help/README caveats — they must not outlive it Not-tested: no live API call against the uploads endpoints Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s --- README.md | 6 + cmd/omni/agent_help.go | 5 +- internal/openapi/body_input.go | 31 ++-- internal/openapi/body_input_test.go | 232 ++++++++++++++---------- internal/openapi/body_shorthand.go | 35 ++-- internal/openapi/body_shorthand_test.go | 15 +- internal/openapi/generate.go | 112 +++++++++--- internal/openapi/generate_test.go | 15 ++ 8 files changed, 287 insertions(+), 164 deletions(-) diff --git a/README.md b/README.md index d692638..ce14d62 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,12 @@ A request body can be given three ways: inline JSON (`--body '{"name":"x"}'`), a checked as JSON before the request is sent, so a typo fails locally instead of coming back as a generic API 400. +The few endpoints whose request body is `multipart/form-data` (`uploads create`, +`uploads replace-data`) cannot be called by the CLI yet — every request it sends is +labelled `application/json` and it builds no multipart framing. Those commands fail +client-side and print the equivalent `curl` command instead of sending something the +API would reject. + Adding a new API endpoint requires no code changes — update `api/openapi.json` (or run `make sync-spec`) and rebuild. ## Auth diff --git a/cmd/omni/agent_help.go b/cmd/omni/agent_help.go index ec29b14..6938d83 100644 --- a/cmd/omni/agent_help.go +++ b/cmd/omni/agent_help.go @@ -72,7 +72,7 @@ Set OMNI_API_TOKEN env var, or run: omni config init query Execute and wait for semantic queries scim SCIM user/group provisioning schedules Manage delivery schedules - uploads Upload and manage CSV files + uploads List and delete CSV uploads (create/replace need multipart — use curl) unstable Unstable/preview commands (document import/export) user-attributes User attribute definitions users User/group role management, set user attribute values @@ -101,6 +101,9 @@ name a leaf without knowing the container shape: --profile NAME Config profile to use --body JSON Request body: JSON string, @path/to/file.json, or "-" for stdin (a bare path is rejected client-side — prefix it with @) + Endpoints whose body is multipart/form-data (uploads create, + uploads replace-data) cannot be called by the CLI; it says so + and prints the equivalent curl command. --schema Print the request body's schema + example, then exit --field PATH With --schema: drill into a dotted field path --depth N With --schema: cap nesting depth (lower = smaller) diff --git a/internal/openapi/body_input.go b/internal/openapi/body_input.go index 735c875..dea2ec0 100644 --- a/internal/openapi/body_input.go +++ b/internal/openapi/body_input.go @@ -15,21 +15,21 @@ import ( // "@path" read from a file ("@-" is stdin too, curl-style) // anything else is the literal body // -// When validateJSON is set the result is checked with json.Valid before any -// network call, because the API answers a non-JSON body with a generic -// "Invalid JSON" 400 that reads like a body-shape problem. flagName is the flag -// the value came from, so the error text names the flag the caller typed. +// The result is checked with json.Valid before any network call, because the +// API answers a non-JSON body with a generic "Invalid JSON" 400 that reads like +// a body-shape problem. flagName is the flag the value came from, so the error +// text names the flag the caller typed. // // The bytes are never re-serialized — what the caller supplied is what gets // sent. -func resolveBody(raw, flagName string, validateJSON bool) ([]byte, error) { +func resolveBody(raw, flagName string) ([]byte, error) { switch { case raw == "-": data, err := readStdin() if err != nil { return nil, fmt.Errorf("reading stdin: %w", err) } - return checkBody(data, raw, flagName, "stdin", validateJSON) + return checkBody(data, raw, flagName, "stdin") case strings.HasPrefix(raw, "@"): path := strings.TrimPrefix(raw, "@") @@ -38,7 +38,7 @@ func resolveBody(raw, flagName string, validateJSON bool) ([]byte, error) { if err != nil { return nil, fmt.Errorf("reading stdin: %w", err) } - return checkBody(data, raw, flagName, "stdin", validateJSON) + return checkBody(data, raw, flagName, "stdin") } if strings.TrimSpace(path) == "" { return nil, fmt.Errorf("--%s: no file path after \"@\"; use --%s @path/to/body.json", flagName, flagName) @@ -48,10 +48,10 @@ func resolveBody(raw, flagName string, validateJSON bool) ([]byte, error) { if err != nil { return nil, err } - return checkBody(data, raw, flagName, fmt.Sprintf("file %s", path), validateJSON) + return checkBody(data, raw, flagName, fmt.Sprintf("file %s", path)) default: - return checkBody([]byte(raw), raw, flagName, "", validateJSON) + return checkBody([]byte(raw), raw, flagName, "") } } @@ -80,12 +80,7 @@ func readBodyFile(path string) ([]byte, error) { // the bytes came from ("stdin", "file X") or is empty when they came straight // from the flag value — only in that case can the raw value itself be a // mistyped file path. -// -// Transport checks (empty input, a path-shaped flag value) run for every -// operation; only the JSON validity check is conditional, so the multipart -// upload endpoints still get the "--body @path" hint they'd otherwise need -// most. -func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([]byte, error) { +func checkBody(data []byte, raw, flagName, source string) ([]byte, error) { if len(strings.TrimSpace(string(data))) == 0 { if source != "" { return nil, fmt.Errorf("no request body read from %s", source) @@ -95,7 +90,7 @@ func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([] // Valid JSON is never a path, so this test comes first: it stops a file // that happens to be named "{}" from hijacking a legitimate body. - if validateJSON && json.Valid(data) { + if json.Valid(data) { return data, nil } @@ -104,10 +99,6 @@ func checkBody(data []byte, raw, flagName, source string, validateJSON bool) ([] flagName, raw, flagName, shellQuote("@"+raw), flagName, shellQuote(raw)) } - if !validateJSON { - return data, nil - } - where := "--" + flagName if source != "" { where = source diff --git a/internal/openapi/body_input_test.go b/internal/openapi/body_input_test.go index 77edaf3..e76b6d5 100644 --- a/internal/openapi/body_input_test.go +++ b/internal/openapi/body_input_test.go @@ -33,7 +33,7 @@ func TestResolveBody_AtFile(t *testing.T) { content := `{"modelId":"abc","prompt":"hi"}` path := writeTempBody(t, "body.json", content) - got, err := resolveBody("@"+path, "body", true) + got, err := resolveBody("@"+path, "body") if err != nil { t.Fatalf("resolveBody: %v", err) } @@ -46,7 +46,7 @@ func TestResolveBody_AtFile(t *testing.T) { func TestResolveBody_AtFileMissing(t *testing.T) { missing := filepath.Join(t.TempDir(), "nope.json") - _, err := resolveBody("@"+missing, "body", true) + _, err := resolveBody("@"+missing, "body") if err == nil { t.Fatal("expected an error for a missing body file, got nil") } @@ -62,7 +62,7 @@ func TestResolveBody_AtFileMissing(t *testing.T) { func TestResolveBody_AtFileTooLarge(t *testing.T) { path := writeTempBody(t, "big.json", strings.Repeat("x", maxStdinSize+1)) - _, err := resolveBody("@"+path, "body", true) + _, err := resolveBody("@"+path, "body") if err == nil { t.Fatal("expected an error for an oversized body file, got nil") } @@ -73,7 +73,7 @@ func TestResolveBody_AtFileTooLarge(t *testing.T) { // "@" with no path is a usage error, not a stat of the empty string. func TestResolveBody_AtWithoutPath(t *testing.T) { - if _, err := resolveBody("@", "body", true); err == nil { + if _, err := resolveBody("@", "body"); err == nil { t.Fatal("expected an error for a bare @, got nil") } } @@ -82,7 +82,7 @@ func TestResolveBody_AtWithoutPath(t *testing.T) { func TestResolveBody_AtFileInvalidJSON(t *testing.T) { path := writeTempBody(t, "body.json", "modelId: abc\n") - _, err := resolveBody("@"+path, "body", true) + _, err := resolveBody("@"+path, "body") if err == nil { t.Fatal("expected an error for a non-JSON body file, got nil") } @@ -101,7 +101,7 @@ func TestResolveBody_BareFilePathHint(t *testing.T) { path := writeTempBody(t, "body.json", `{"a":1}`) for _, raw := range []string{path, "/tmp/does-not-exist.json", "./body.json", "~/body.json"} { - _, err := resolveBody(raw, "body", true) + _, err := resolveBody(raw, "body") if err == nil { t.Fatalf("resolveBody(%q): expected an error, got nil", raw) } @@ -120,7 +120,7 @@ func TestResolveBody_BareFilePathHint(t *testing.T) { // Non-JSON that isn't path-shaped gets a plain parse error with a position. func TestResolveBody_InvalidJSONDiagnostic(t *testing.T) { - _, err := resolveBody(`{"a":1,}`, "body", true) + _, err := resolveBody(`{"a":1,}`, "body") if err == nil { t.Fatal("expected an error for malformed JSON, got nil") } @@ -138,7 +138,7 @@ func TestResolveBody_InvalidJSONDiagnostic(t *testing.T) { // The error names whichever flag the caller actually used. func TestResolveBody_NamesTheFlagUsed(t *testing.T) { - _, err := resolveBody("not json", "json-body", true) + _, err := resolveBody("not json", "json-body") if err == nil { t.Fatal("expected an error, got nil") } @@ -157,7 +157,7 @@ func TestResolveBody_ValidJSONPassthrough(t *testing.T) { `null`, } for _, raw := range cases { - got, err := resolveBody(raw, "body", true) + got, err := resolveBody(raw, "body") if err != nil { t.Fatalf("resolveBody(%q): %v", raw, err) } @@ -172,7 +172,7 @@ func TestResolveBody_ValidJSONPassthrough(t *testing.T) { func TestResolveBody_PathWithSpacesHint(t *testing.T) { path := writeTempBody(t, "request body.json", `{"a":1}`) - _, err := resolveBody(path, "body", true) + _, err := resolveBody(path, "body") if err == nil { t.Fatalf("resolveBody(%q): expected an error, got nil", path) } @@ -188,47 +188,10 @@ func TestResolveBody_PathWithSpacesHint(t *testing.T) { } } -// Non-JSON media types (the multipart upload endpoints) skip validation. -func TestResolveBody_SkipsValidationForNonJSON(t *testing.T) { - raw := "--boundary\r\nnot json\r\n" - got, err := resolveBody(raw, "body", false) - if err != nil { - t.Fatalf("resolveBody: %v", err) - } - if string(got) != raw { - t.Errorf("body = %q, want it unchanged", string(got)) - } -} - -// Skipping JSON validation must not skip the transport diagnostics: a -// multipart operation handed a bare path still gets the @file hint. -func TestResolveBody_NonJSONStillGetsPathHint(t *testing.T) { - _, err := resolveBody("/tmp/request.multipart", "body", false) - if err == nil { - t.Fatal("expected the file-path hint for a non-JSON body, got nil") - } - if !strings.Contains(err.Error(), "--body @/tmp/request.multipart") { - t.Errorf("error = %q, want it to suggest --body @/tmp/request.multipart", err.Error()) - } - - // An existing file named as a bare path counts too, whatever its contents. - path := writeTempBody(t, "upload.bin", "\x00\x01binary") - if _, err := resolveBody(path, "body", false); err == nil { - t.Fatalf("resolveBody(%q): expected the file-path hint, got nil", path) - } -} - -// The empty-body error is a transport check, so it applies to every media type. -func TestResolveBody_EmptyNonJSON(t *testing.T) { - if _, err := resolveBody("", "body", false); err == nil { - t.Fatal("expected an error for an explicitly empty --body, got nil") - } -} - // "-" still reads stdin, and stdin is validated the same way. func TestResolveBody_Stdin(t *testing.T) { withStdin(t, `{"from":"stdin"}`, func() { - got, err := resolveBody("-", "body", true) + got, err := resolveBody("-", "body") if err != nil { t.Fatalf("resolveBody: %v", err) } @@ -238,7 +201,7 @@ func TestResolveBody_Stdin(t *testing.T) { }) withStdin(t, "not json", func() { - _, err := resolveBody("-", "body", true) + _, err := resolveBody("-", "body") if err == nil { t.Fatal("expected an error for non-JSON stdin, got nil") } @@ -251,7 +214,7 @@ func TestResolveBody_Stdin(t *testing.T) { // "@-" is the curl spelling of stdin. func TestResolveBody_AtDashIsStdin(t *testing.T) { withStdin(t, `{"from":"stdin"}`, func() { - got, err := resolveBody("@-", "body", true) + got, err := resolveBody("@-", "body") if err != nil { t.Fatalf("resolveBody: %v", err) } @@ -264,13 +227,13 @@ func TestResolveBody_AtDashIsStdin(t *testing.T) { // An empty source is reported as empty rather than as a JSON syntax error. func TestResolveBody_EmptySources(t *testing.T) { path := writeTempBody(t, "empty.json", "") - if _, err := resolveBody("@"+path, "body", true); err == nil { + if _, err := resolveBody("@"+path, "body"); err == nil { t.Fatal("expected an error for an empty body file, got nil") } else if !strings.Contains(err.Error(), "no request body") { t.Errorf("error = %q, want it to report an empty body", err.Error()) } - if _, err := resolveBody(" ", "body", true); err == nil { + if _, err := resolveBody(" ", "body"); err == nil { t.Fatal("expected an error for a whitespace-only --body, got nil") } } @@ -361,8 +324,9 @@ func TestBuildCommand_InvalidBodyMakesNoRequest(t *testing.T) { } } -// A multipart operation keeps the transport diagnostics but not the JSON -// validity check. +// A multipart operation cannot be sent at all: auth.Do labels every body +// application/json and the CLI builds no part framing, so it fails client-side +// instead of shipping bytes the API will reject. func TestBuildCommand_NonJSONBodyOperation(t *testing.T) { op := &operationInfo{ Tag: "uploads", @@ -371,34 +335,86 @@ func TestBuildCommand_NonJSONBodyOperation(t *testing.T) { Path: "/api/v1/uploads", HasBody: true, BodyNonJSON: true, + BodyMedia: "multipart/form-data", } - // Non-JSON content goes through untouched. - var captured APIRequest - cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) - cmd.SilenceUsage, cmd.SilenceErrors = true, true - cmd.SetArgs([]string{"--body", "not json at all"}) - if err := cmd.Execute(); err != nil { - t.Fatalf("Execute: %v", err) - } - if string(captured.Body) != "not json at all" { - t.Errorf("body = %q, want it unchanged", string(captured.Body)) + for _, args := range [][]string{ + {"--body", "@/tmp/upload.csv"}, + {"--body", `{"modelId":"m"}`}, + {}, + } { + called := false + cmd := buildCommand(op, func(req APIRequest) error { called = true; return nil }) + cmd.SilenceUsage, cmd.SilenceErrors = true, true + cmd.SetArgs(args) + + err := cmd.Execute() + if err == nil { + t.Fatalf("%v: expected an unsupported-body error, got nil", args) + } + if called { + t.Errorf("%v: executor ran for an unsendable media type", args) + } + if !strings.Contains(err.Error(), "multipart/form-data") { + t.Errorf("%v: error = %q, want it to name the media type", args, err.Error()) + } } +} + +// The multipart error points at a curl command that names the required parts, +// with the binary part shown as a file upload. +func TestGenerateCommands_MultipartCurlHint(t *testing.T) { + spec := `{ + "openapi": "3.1.0", + "info": {"title": "test", "version": "1.0"}, + "paths": { + "/api/v1/uploads": { + "post": { + "operationId": "uploadsCreate", + "tags": ["Uploads"], + "requestBody": { + "required": true, + "content": { + "multipart/form-data": { + "schema": { + "type": "object", + "properties": { + "file": {"type": "string", "format": "binary"}, + "modelId": {"type": "string"} + }, + "required": ["file", "modelId"] + } + } + } + }, + "responses": {"201": {"description": "ok"}} + } + } + } + }` - // A bare path still gets caught before the request. called := false - cmd2 := buildCommand(op, func(req APIRequest) error { called = true; return nil }) - cmd2.SilenceUsage, cmd2.SilenceErrors = true, true - cmd2.SetArgs([]string{"--body", "/tmp/upload.csv"}) - err := cmd2.Execute() + cmds, err := GenerateCommands([]byte(spec), func(req APIRequest) error { called = true; return nil }) + if err != nil { + t.Fatalf("GenerateCommands: %v", err) + } + + root := &cobra.Command{Use: "omni"} + root.AddCommand(cmds...) + root.SilenceUsage, root.SilenceErrors = true, true + root.SetArgs([]string{"uploads", "create", "--body", "@upload.csv"}) + + err = root.Execute() if err == nil { - t.Fatal("expected the file-path hint, got nil") + t.Fatal("expected an unsupported-body error, got nil") } if called { - t.Error("executor ran with a path as the body") + t.Error("executor ran for a multipart operation") } - if !strings.Contains(err.Error(), "--body @/tmp/upload.csv") { - t.Errorf("error = %q, want it to suggest --body @/tmp/upload.csv", err.Error()) + for _, want := range []string{"multipart/form-data", "curl -X POST", "-F file=@path/to/file.csv", "-F modelId=MODELID"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to contain %q", err.Error(), want) + } } } @@ -438,30 +454,58 @@ func TestBuildCommand_ExplicitlyEmptyBody(t *testing.T) { } } -// The same holds on a shorthand command: an empty --body isn't shorthand input. +// The same holds on a shorthand command: an empty --body isn't shorthand input, +// and it must reach the empty-body diagnostic rather than an arg-count error — +// the caller supplied no shorthand positional either. func TestBodyShorthand_ExplicitlyEmptyBody(t *testing.T) { - called := false - op := &operationInfo{ - Tag: "ai", - OperationID: "aiSearchOmniDocs", - Method: "POST", - Path: "/api/v1/ai/search-omni-docs", - HasBody: true, - } - cmd := buildCommand(op, func(req APIRequest) error { called = true; return nil }) - cmd.SilenceUsage = true - cmd.SilenceErrors = true - cmd.SetArgs([]string{"--body", "", "some question"}) + for _, flag := range []string{"--body", "--json-body"} { + called := false + op := &operationInfo{ + Tag: "ai", + OperationID: "aiSearchOmniDocs", + Method: "POST", + Path: "/api/v1/ai/search-omni-docs", + HasBody: true, + } + cmd := buildCommand(op, func(req APIRequest) error { called = true; return nil }) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{flag, ""}) - err := cmd.Execute() - if err == nil { - t.Fatal("expected an error for an empty --body, got nil") - } - if called { - t.Error("executor ran despite an empty body") + err := cmd.Execute() + if err == nil { + t.Fatalf("%s '': expected an error, got nil", flag) + } + if called { + t.Errorf("%s '': executor ran despite an empty body", flag) + } + if !strings.Contains(err.Error(), flag+" is empty") { + t.Errorf("%s '': error = %q, want it to report the empty flag", flag, err.Error()) + } } - if !strings.Contains(err.Error(), "--body is empty") { - t.Errorf("error = %q, want it to report the empty flag", err.Error()) +} + +// --body and --json-body conflict on being typed at all, not on their values. +func TestBuildCommand_BodyAndJSONBodyConflict(t *testing.T) { + for _, args := range [][]string{ + {"--body", `{"a":1}`, "--json-body", `{"b":2}`}, + {"--body", "", "--json-body", `{"b":2}`}, + {"--body", `{"a":1}`, "--json-body", ""}, + } { + called := false + cmd := bodyCmd(t, func(req APIRequest) error { called = true; return nil }) + cmd.SetArgs(args) + + err := cmd.Execute() + if err == nil { + t.Fatalf("%v: expected a conflict error, got nil", args) + } + if called { + t.Errorf("%v: executor ran despite conflicting flags", args) + } + if !strings.Contains(err.Error(), "cannot use both --body and --json-body") { + t.Errorf("%v: error = %q, want the conflict message", args, err.Error()) + } } } diff --git a/internal/openapi/body_shorthand.go b/internal/openapi/body_shorthand.go index d5406e5..706e13b 100644 --- a/internal/openapi/body_shorthand.go +++ b/internal/openapi/body_shorthand.go @@ -302,18 +302,11 @@ func applyBodyShorthand(cmd *cobra.Command, op *operationInfo, sh *BodyShorthand // Wrap the original RunE to assemble body from shorthand args originalRunE := cmd.RunE cmd.RunE = func(cmd *cobra.Command, args []string) error { - bodyFlag, _ := cmd.Flags().GetString("body") - jsonBodyFlag, _ := cmd.Flags().GetString("json-body") - - rawBody := bodyFlag - if jsonBodyFlag != "" { - rawBody = jsonBodyFlag - } - - // If --body/--json-body is provided, use existing behavior — but - // reject explicitly-set shorthand flags rather than silently - // dropping them from the request. - if rawBody != "" { + // Body mode goes by whether the flag was typed, not by its value: an + // explicitly empty --body is a typo, not an invitation to assemble one + // from shorthand input. The generated RunE reports it like every other + // command does. + if bodyFlagUsed(cmd) { var conflicting []string for _, f := range sh.Flags { if cmd.Flags().Changed(f.FlagName) { @@ -323,13 +316,6 @@ func applyBodyShorthand(cmd *cobra.Command, op *operationInfo, sh *BodyShorthand if len(conflicting) > 0 { return fmt.Errorf("%s cannot be combined with --body; include the field(s) in the JSON body instead", strings.Join(conflicting, ", ")) } - return originalRunE(cmd, args) - } - - // An explicitly empty --body/--json-body is a typo, not an invitation - // to assemble one from shorthand input. Hand it back to the generated - // RunE so it reports the same error every other command does. - if cmd.Flags().Changed("body") || cmd.Flags().Changed("json-body") { return originalRunE(cmd, args[:numPathParams]) } @@ -357,10 +343,7 @@ func applyBodyShorthand(cmd *cobra.Command, op *operationInfo, sh *BodyShorthand // - numPathParams + numShorthandArgs args (when using shorthand mode) func flexibleArgs(numPathParams, numShorthandArgs int) cobra.PositionalArgs { return func(cmd *cobra.Command, args []string) error { - bodyFlag, _ := cmd.Flags().GetString("body") - jsonBodyFlag, _ := cmd.Flags().GetString("json-body") - - if bodyFlag != "" || jsonBodyFlag != "" { + if bodyFlagUsed(cmd) { if len(args) != numPathParams { return fmt.Errorf("accepts %d arg(s) when --body is used, received %d", numPathParams, len(args)) } @@ -383,6 +366,12 @@ func flexibleArgs(numPathParams, numShorthandArgs int) cobra.PositionalArgs { } } +// bodyFlagUsed reports whether the caller typed --body or --json-body, whatever +// value they gave it. +func bodyFlagUsed(cmd *cobra.Command) bool { + return cmd.Flags().Changed("body") || cmd.Flags().Changed("json-body") +} + // assembleBody builds a JSON body from shorthand positional args and promoted flags. func assembleBody(sh *BodyShorthand, args []string, pathParamCount int, cmd *cobra.Command, bodySchema *base.SchemaProxy) ([]byte, error) { body := map[string]interface{}{} diff --git a/internal/openapi/body_shorthand_test.go b/internal/openapi/body_shorthand_test.go index ed9c315..59e4c9d 100644 --- a/internal/openapi/body_shorthand_test.go +++ b/internal/openapi/body_shorthand_test.go @@ -276,10 +276,20 @@ func TestFlexibleArgs_ShorthandMode(t *testing.T) { } } +// mustSetFlag marks a flag as explicitly provided, the way parsing a command +// line does — body mode is selected by Changed, not by the value. +func mustSetFlag(t *testing.T, cmd *cobra.Command, name, value string) { + t.Helper() + if err := cmd.Flags().Set(name, value); err != nil { + t.Fatalf("set --%s: %v", name, err) + } +} + func TestFlexibleArgs_BodyMode(t *testing.T) { cmd := &cobra.Command{Use: "test"} - cmd.Flags().String("body", `{"key":"val"}`, "") + cmd.Flags().String("body", "", "") cmd.Flags().String("json-body", "", "") + mustSetFlag(t, cmd, "body", `{"key":"val"}`) validator := flexibleArgs(1, 1) // 1 arg (path param only) should pass when --body is set @@ -296,7 +306,8 @@ func TestFlexibleArgs_BodyMode(t *testing.T) { func TestFlexibleArgs_JsonBodyMode(t *testing.T) { cmd := &cobra.Command{Use: "test"} cmd.Flags().String("body", "", "") - cmd.Flags().String("json-body", `{"key":"val"}`, "") + cmd.Flags().String("json-body", "", "") + mustSetFlag(t, cmd, "json-body", `{"key":"val"}`) validator := flexibleArgs(1, 1) if err := validator(cmd, []string{"path-param"}); err != nil { diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index 85328b9..e06ff44 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -4,6 +4,7 @@ package openapi import ( + "errors" "fmt" "io" "net/url" @@ -92,7 +93,8 @@ type operationInfo struct { QueryParams []paramInfo HasBody bool BodySchema *base.SchemaProxy // request body schema, when HasBody - BodyNonJSON bool // request body uses a non-JSON media type (e.g. multipart) + BodyNonJSON bool // request body uses a media type the CLI cannot send (e.g. multipart) + BodyMedia string // media type the request body is declared with Deprecated bool } @@ -160,7 +162,8 @@ func extractOperations(pathStr string, item *v3.PathItem, groups map[string][]*o if op.RequestBody != nil { info.HasBody = true info.BodySchema = requestBodySchema(op.RequestBody) - info.BodyNonJSON = !requestBodyIsJSON(op.RequestBody) + info.BodyMedia = requestBodyMediaType(op.RequestBody) + info.BodyNonJSON = !isJSONMediaType(info.BodyMedia) } groups[tag] = append(groups[tag], info) @@ -181,6 +184,9 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { } long := op.Description + if op.BodyNonJSON { + long = strings.TrimSpace(fmt.Sprintf("NOT SUPPORTED: this endpoint requires a %s request body, which the omni CLI cannot send yet.\n\n%s", op.BodyMedia, long)) + } if op.Deprecated { long = "DEPRECATED: " + long } @@ -217,24 +223,27 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { // Read body from stdin, a file, or the flag value itself var body []byte if op.HasBody { - bodyFlag, _ := cmd.Flags().GetString("body") - jsonBodyFlag, _ := cmd.Flags().GetString("json-body") + if op.BodyNonJSON { + cmd.SilenceUsage = true + return unsupportedBodyError(op) + } - if bodyFlag != "" && jsonBodyFlag != "" { + // Flag selection goes by Changed, not by value: an explicitly + // empty --body is a mistake worth reporting rather than an + // unset flag. + if cmd.Flags().Changed("body") && cmd.Flags().Changed("json-body") { return fmt.Errorf("cannot use both --body and --json-body; use one or the other") } - // An explicitly empty --body is a mistake worth reporting, so - // track whether the flag was typed at all rather than inferring - // it from the value. - effectiveBody, flagName := bodyFlag, "body" - if jsonBodyFlag != "" || (cmd.Flags().Changed("json-body") && !cmd.Flags().Changed("body")) { - effectiveBody, flagName = jsonBodyFlag, "json-body" + flagName := "body" + if cmd.Flags().Changed("json-body") { + flagName = "json-body" } - if effectiveBody != "" || cmd.Flags().Changed(flagName) { + if cmd.Flags().Changed(flagName) { + raw, _ := cmd.Flags().GetString(flagName) var err error - body, err = resolveBody(effectiveBody, flagName, !op.BodyNonJSON) + body, err = resolveBody(raw, flagName) if err != nil { // A body input error is self-explanatory; the usage // block would bury the hint. @@ -263,9 +272,15 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { cmd.Flags().String(flagName, "", desc) } - // If the operation accepts a body, add --body and --json-body flags + // If the operation accepts a body, add --body and --json-body flags. They + // stay registered for unsendable media types so that `--body ...` reports + // the real reason instead of "unknown flag". if op.HasBody { - cmd.Flags().String("body", "", `request body as JSON string, "@path/to/file.json" to read a file, or "-" for stdin (run with --schema to see its shape)`) + bodyDesc := `request body as JSON string, "@path/to/file.json" to read a file, or "-" for stdin (run with --schema to see its shape)` + if op.BodyNonJSON { + bodyDesc = fmt.Sprintf("not supported: this endpoint requires a %s request body, which the omni CLI cannot send yet", op.BodyMedia) + } + cmd.Flags().String("body", "", bodyDesc) cmd.Flags().String("json-body", "", `request body as JSON string, "@path/to/file.json", or "-" for stdin (alias for --body)`) cmd.Flags().MarkHidden("json-body") } @@ -341,21 +356,70 @@ func requestBodySchema(rb *v3.RequestBody) *base.SchemaProxy { return first } -// requestBodyIsJSON reports whether a request body is sent as JSON. Bodies with -// no declared content default to JSON — that's what the CLI sends. Only the -// media types the spec actually declares (today: multipart/form-data uploads) -// opt out of client-side JSON validation. -func requestBodyIsJSON(rb *v3.RequestBody) bool { +// requestBodyMediaType returns the media type an operation's request body is +// declared with, preferring JSON. Bodies with no declared content default to +// JSON — that's what the CLI sends. +func requestBodyMediaType(rb *v3.RequestBody) string { if rb == nil || rb.Content == nil || rb.Content.Len() == 0 { - return true + return "application/json" } + first := "" for pair := rb.Content.First(); pair != nil; pair = pair.Next() { mt := pair.Key() - if mt == "application/json" || strings.HasSuffix(mt, "+json") { - return true + if isJSONMediaType(mt) { + return mt + } + if first == "" { + first = mt + } + } + return first +} + +// isJSONMediaType reports whether a media type is sent as a JSON document. +func isJSONMediaType(mt string) bool { + return mt == "application/json" || strings.HasSuffix(mt, "+json") +} + +// unsupportedBodyError explains that an operation's body cannot be sent by the +// CLI. auth.Do labels every request application/json and passes the bytes +// through unchanged, so a multipart body would arrive mislabeled and without +// its part framing — better to say so than to send something the API rejects. +func unsupportedBodyError(op *operationInfo) error { + msg := fmt.Sprintf("this endpoint requires a %s request body, which the omni CLI cannot send yet", op.BodyMedia) + if hint := curlHint(op); hint != "" { + msg += "\nsend it with curl instead:\n " + hint + } + return errors.New(msg) +} + +// curlHint renders a curl command for a multipart operation, naming the parts +// the spec marks required. +func curlHint(op *operationInfo) string { + if op.BodyMedia != "multipart/form-data" || op.BodySchema == nil { + return "" + } + schema := op.BodySchema.Schema() + if schema == nil { + return "" + } + + path := op.Path + for _, p := range op.PathParams { + path = strings.Replace(path, "{"+p.Name+"}", strings.ToUpper(slugify(p.Name)), 1) + } + + cmd := fmt.Sprintf("curl -X %s \"$OMNI_BASE_URL%s\" -H \"Authorization: Bearer $OMNI_API_TOKEN\"", op.Method, path) + for _, name := range schema.Required { + value := strings.ToUpper(slugify(name)) + if prop, ok := schema.Properties.Get(name); ok && prop != nil { + if ps := prop.Schema(); ps != nil && ps.Format == "binary" { + value = "@path/to/file.csv" + } } + cmd += fmt.Sprintf(" -F %s=%s", name, value) } - return false + return cmd } // commandName derives a CLI subcommand name from the operationId or method+path. diff --git a/internal/openapi/generate_test.go b/internal/openapi/generate_test.go index c0b8f2d..b5083e5 100644 --- a/internal/openapi/generate_test.go +++ b/internal/openapi/generate_test.go @@ -433,6 +433,7 @@ type specOperation struct { Path string PathParams []string HasBody bool + BodyMedia string } // parseSpecOperations reads the OpenAPI spec directly (bypassing our generator) @@ -490,6 +491,7 @@ func parseSpecOperations(t *testing.T, specData []byte) []specOperation { Path: pathStr, PathParams: pathParams, HasBody: op.RequestBody != nil, + BodyMedia: requestBodyMediaType(op.RequestBody), }) } } @@ -562,6 +564,19 @@ func TestSpecCoverage(t *testing.T) { failures = append(failures, fmt.Sprintf("%s: no RunE", key)) continue } + + // Media types the CLI can't build (multipart uploads) are covered + // by refusing client-side with a message that names the media type. + if sop.HasBody && !isJSONMediaType(sop.BodyMedia) { + err := sub.RunE(sub, args) + if err == nil || !strings.Contains(err.Error(), sop.BodyMedia) { + failures = append(failures, fmt.Sprintf("%s: want an unsupported-%s error, got %v", key, sop.BodyMedia, err)) + continue + } + called[sop.OperationID] = true + continue + } + if err := sub.RunE(sub, args); err != nil { failures = append(failures, fmt.Sprintf("%s: RunE: %v", key, err)) continue From ffebcee76c5efede78c4fef409aab7d020fdcaa9 Mon Sep 17 00:00:00 2001 From: Ernesto Ongaro Date: Thu, 20 Aug 2026 23:20:41 +0100 Subject: [PATCH 4/5] feat(openapi): support multipart form requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constraint: 3d9b0e1 refused multipart client-side; its directive says drop unsupportedBodyError when multipart lands Rejected: keep the refusal for uploads not yet verified | end-to-end round trip against a local server confirms framing and Content-Type Confidence: high Scope-risk: moderate Directive: --body on a multipart op is a JSON map of field values, not raw file bytes — bodyFlagIsJSON() must keep returning true for multipart or #75's @file reading and validation stop applying there Not-tested: no live call against the real Omni uploads API Co-Authored-By: Claude Opus 5 --- README.md | 31 ++- cmd/omni/agent_help.go | 18 +- cmd/omni/main.go | 3 +- internal/auth/auth.go | 12 +- internal/auth/auth_test.go | 20 ++ internal/openapi/body_input_test.go | 122 ++++------- internal/openapi/generate.go | 227 +++++++++---------- internal/openapi/generate_test.go | 80 +++++-- internal/openapi/multipart.go | 326 ++++++++++++++++++++++++++++ internal/openapi/multipart_test.go | 291 +++++++++++++++++++++++++ 10 files changed, 902 insertions(+), 228 deletions(-) create mode 100644 internal/openapi/multipart.go create mode 100644 internal/openapi/multipart_test.go diff --git a/README.md b/README.md index ce14d62..4c92829 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,25 @@ omni documents list omni --help ``` +### Upload a CSV + +Multipart request fields are generated as normal CLI flags. Binary OpenAPI +fields accept a local file path: + +```bash +omni uploads create \ + --file ./people.csv \ + --model-id 00000000-0000-0000-0000-000000000000 \ + --view-name people +``` + +Multipart commands also accept `--body` JSON for compatibility. Values for +binary fields are interpreted as file paths: + +```bash +omni uploads create --body '{"file":"./people.csv","modelId":"00000000-0000-0000-0000-000000000000"}' +``` + ## Shell completions `omni` supports tab completion for bash, zsh, fish, and PowerShell. Pick your shell below and run the snippet once — tab completion works on every new shell thereafter. @@ -120,18 +139,18 @@ After installing, restart your shell and try: `omni `, `omni ai `, `om ## How it works -The CLI embeds the OpenAPI spec (`api/openapi.json`) into the binary. At startup it parses the spec and generates cobra subcommands for every operation. Each API tag becomes a command group, path params become positional args, query params become flags, and request bodies are passed via `--body` or stdin. +The CLI embeds the OpenAPI spec (`api/openapi.json`) into the binary. At startup it parses the spec and generates cobra subcommands for every operation. Each API tag becomes a command group, path params become positional args, query params become flags, and JSON request bodies are passed via `--body` or stdin. For `multipart/form-data` bodies, top-level schema properties become flags and binary properties are read from file paths. A request body can be given three ways: inline JSON (`--body '{"name":"x"}'`), a file (`--body @path/to/body.json`), or stdin (`--body - < path/to/body.json`). The body is checked as JSON before the request is sent, so a typo fails locally instead of coming back as a generic API 400. -The few endpoints whose request body is `multipart/form-data` (`uploads create`, -`uploads replace-data`) cannot be called by the CLI yet — every request it sends is -labelled `application/json` and it builds no multipart framing. Those commands fail -client-side and print the equivalent `curl` command instead of sending something the -API would reject. +Endpoints whose request body is `multipart/form-data` (`uploads create`, +`uploads replace-data`) take the same `--body` forms: the JSON is a map of field +values, checked locally and then framed into form parts, with binary fields given +as file paths. Their schema fields are also exposed as flags — see +[Upload a CSV](#upload-a-csv). Adding a new API endpoint requires no code changes — update `api/openapi.json` (or run `make sync-spec`) and rebuild. diff --git a/cmd/omni/agent_help.go b/cmd/omni/agent_help.go index 6938d83..8a797be 100644 --- a/cmd/omni/agent_help.go +++ b/cmd/omni/agent_help.go @@ -58,6 +58,10 @@ Set OMNI_API_TOKEN env var, or run: omni config init ### Search Omni documentation omni ai search-omni-docs --body '{"query":"how do I..."}' +### Upload a CSV +Multipart body fields are generated as flags; binary fields take file paths. + omni uploads create --file ./people.csv --model-id MODEL_ID --view-name people + ## Command Groups ai AI-powered query generation, jobs, doc search ai-eval AI eval prompt set management @@ -72,7 +76,7 @@ Set OMNI_API_TOKEN env var, or run: omni config init query Execute and wait for semantic queries scim SCIM user/group provisioning schedules Manage delivery schedules - uploads List and delete CSV uploads (create/replace need multipart — use curl) + uploads Upload and manage CSV files unstable Unstable/preview commands (document import/export) user-attributes User attribute definitions users User/group role management, set user attribute values @@ -94,6 +98,10 @@ name a leaf without knowing the container shape: omni documents v2-create --schema --field queryPresentations.data # just the tiles map omni documents v2-create --schema --field queryPresentations.data.query +For multipart/form-data commands, top-level schema properties are also exposed +as flags. Binary properties are labeled "file path" in --help. --body remains +available; binary values in its JSON object are interpreted as file paths. + ## Common Flags --compact Non-indented JSON output --token TOKEN API token (overrides env/config) @@ -101,9 +109,9 @@ name a leaf without knowing the container shape: --profile NAME Config profile to use --body JSON Request body: JSON string, @path/to/file.json, or "-" for stdin (a bare path is rejected client-side — prefix it with @) - Endpoints whose body is multipart/form-data (uploads create, - uploads replace-data) cannot be called by the CLI; it says so - and prints the equivalent curl command. + For multipart/form-data endpoints (uploads create, uploads + replace-data) the JSON is a map of field values; binary + fields take file paths. --schema Print the request body's schema + example, then exit --field PATH With --schema: drill into a dotted field path --depth N With --schema: cap nesting depth (lower = smaller) @@ -112,7 +120,7 @@ name a leaf without knowing the container shape: - Use "omni ai generate-query" to answer data questions — it picks fields and filters for you. - Set a user's attribute values: omni users set-attributes --attr region=us-east - Path parameters are positional args: omni dashboards download -- Query parameters are flags: omni models list --pagesize 10 +- Query parameters are flags: omni models list --page-size 10 - Run "omni --help" to see all commands in a group. - Run "omni --help" to see flags for a specific command. ` diff --git a/cmd/omni/main.go b/cmd/omni/main.go index a92fc49..1154aec 100644 --- a/cmd/omni/main.go +++ b/cmd/omni/main.go @@ -54,7 +54,6 @@ func main() { root.PersistentFlags().Bool("compact", false, "compact JSON output (no indentation)") root.PersistentFlags().StringP("format", "o", "", "output format: json, human, auto (default auto: human on TTY, json when piped)") - // Hand-written commands (not from spec) addConfigCommands(root) addAgentHelpCommand(root) @@ -101,7 +100,7 @@ func executeAPICall(req openapi.APIRequest) error { // scripts piping JSON shouldn't get decorative noise on stderr. sp := maybeStartSpinner(format) - resp, err := auth.Do(cfg, req.Method, req.Path, req.Body) + resp, err := auth.DoWithContentType(cfg, req.Method, req.Path, req.Body, req.ContentType) sp.Stop() if err != nil { return err diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 6954222..271fe9c 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -12,6 +12,13 @@ import ( // Do executes an authenticated HTTP request against the Omni API. func Do(cfg *config.ResolvedConfig, method, path string, body []byte) (*http.Response, error) { + return DoWithContentType(cfg, method, path, body, "application/json") +} + +// DoWithContentType executes an authenticated request using the caller's +// declared request media type. Multipart callers must include the boundary in +// contentType (for example, multipart/form-data; boundary=...). +func DoWithContentType(cfg *config.ResolvedConfig, method, path string, body []byte, contentType string) (*http.Response, error) { baseURL := strings.TrimRight(cfg.BaseURL, "/") url := baseURL + path @@ -32,7 +39,10 @@ func Do(cfg *config.ResolvedConfig, method, path string, body []byte) (*http.Res } req.Header.Set("Authorization", "Bearer "+cfg.Token) - req.Header.Set("Content-Type", "application/json") + if contentType == "" { + contentType = "application/json" + } + req.Header.Set("Content-Type", contentType) req.Header.Set("Accept", "application/json") resp, err := http.DefaultClient.Do(req) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 9a02a5b..dd05be6 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -46,6 +46,26 @@ func TestDo_SetsHeaders(t *testing.T) { } } +func TestDoWithContentType_PreservesMultipartBoundary(t *testing.T) { + var gotContentType string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotContentType = r.Header.Get("Content-Type") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := &config.ResolvedConfig{Token: "tok", BaseURL: srv.URL} + want := "multipart/form-data; boundary=test-boundary" + resp, err := DoWithContentType(cfg, "POST", "/uploads", []byte("body"), want) + if err != nil { + t.Fatalf("DoWithContentType: %v", err) + } + resp.Body.Close() + if gotContentType != want { + t.Errorf("Content-Type = %q, want %q", gotContentType, want) + } +} + // Verify that the HTTP method (GET, POST, etc.) and URL path are forwarded // correctly to the server. func TestDo_MethodAndPath(t *testing.T) { diff --git a/internal/openapi/body_input_test.go b/internal/openapi/body_input_test.go index e76b6d5..38fc261 100644 --- a/internal/openapi/body_input_test.go +++ b/internal/openapi/body_input_test.go @@ -324,97 +324,59 @@ func TestBuildCommand_InvalidBodyMakesNoRequest(t *testing.T) { } } -// A multipart operation cannot be sent at all: auth.Do labels every body -// application/json and the CLI builds no part framing, so it fails client-side -// instead of shipping bytes the API will reject. -func TestBuildCommand_NonJSONBodyOperation(t *testing.T) { +// A multipart operation still goes through the shared body pipeline: --body is +// read, validated as JSON, and then framed into form parts. The media type the +// spec declares is what the request is labelled with. +func TestBuildCommand_MultipartBodyOperation(t *testing.T) { op := &operationInfo{ - Tag: "uploads", - OperationID: "uploadsCreate", - Method: "POST", - Path: "/api/v1/uploads", - HasBody: true, - BodyNonJSON: true, - BodyMedia: "multipart/form-data", + Tag: "uploads", + OperationID: "uploadsCreate", + Method: "POST", + Path: "/api/v1/uploads", + HasBody: true, + BodyMediaType: "multipart/form-data", + BodyFields: []multipartFieldInfo{ + {Name: "modelId", FlagName: "model-id", Type: "string"}, + }, } - for _, args := range [][]string{ - {"--body", "@/tmp/upload.csv"}, - {"--body", `{"modelId":"m"}`}, - {}, - } { - called := false - cmd := buildCommand(op, func(req APIRequest) error { called = true; return nil }) - cmd.SilenceUsage, cmd.SilenceErrors = true, true - cmd.SetArgs(args) - - err := cmd.Execute() - if err == nil { - t.Fatalf("%v: expected an unsupported-body error, got nil", args) - } - if called { - t.Errorf("%v: executor ran for an unsendable media type", args) - } - if !strings.Contains(err.Error(), "multipart/form-data") { - t.Errorf("%v: error = %q, want it to name the media type", args, err.Error()) - } + var captured APIRequest + cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) + cmd.SetArgs([]string{"--body", `{"modelId":"m"}`}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.HasPrefix(captured.ContentType, "multipart/form-data; boundary=") { + t.Errorf("ContentType = %q, want a multipart type with a boundary", captured.ContentType) + } + if !strings.Contains(string(captured.Body), `name="modelId"`) { + t.Errorf("body = %q, want a modelId part", string(captured.Body)) } } -// The multipart error points at a curl command that names the required parts, -// with the binary part shown as a file upload. -func TestGenerateCommands_MultipartCurlHint(t *testing.T) { - spec := `{ - "openapi": "3.1.0", - "info": {"title": "test", "version": "1.0"}, - "paths": { - "/api/v1/uploads": { - "post": { - "operationId": "uploadsCreate", - "tags": ["Uploads"], - "requestBody": { - "required": true, - "content": { - "multipart/form-data": { - "schema": { - "type": "object", - "properties": { - "file": {"type": "string", "format": "binary"}, - "modelId": {"type": "string"} - }, - "required": ["file", "modelId"] - } - } - } - }, - "responses": {"201": {"description": "ok"}} - } - } - } - }` - - called := false - cmds, err := GenerateCommands([]byte(spec), func(req APIRequest) error { called = true; return nil }) - if err != nil { - t.Fatalf("GenerateCommands: %v", err) +// The file-path hint still fires on a multipart command, where a bare path is +// the most likely mistake of all. +func TestBuildCommand_MultipartBodyPathHint(t *testing.T) { + op := &operationInfo{ + Tag: "uploads", + OperationID: "uploadsCreate", + Method: "POST", + Path: "/api/v1/uploads", + HasBody: true, + BodyMediaType: "multipart/form-data", } - root := &cobra.Command{Use: "omni"} - root.AddCommand(cmds...) - root.SilenceUsage, root.SilenceErrors = true, true - root.SetArgs([]string{"uploads", "create", "--body", "@upload.csv"}) + called := false + cmd := buildCommand(op, func(req APIRequest) error { called = true; return nil }) + cmd.SilenceUsage, cmd.SilenceErrors = true, true + cmd.SetArgs([]string{"--body", "/tmp/upload.csv"}) - err = root.Execute() - if err == nil { - t.Fatal("expected an unsupported-body error, got nil") + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "looks like a file path") { + t.Fatalf("error = %v, want the file-path hint", err) } if called { - t.Error("executor ran for a multipart operation") - } - for _, want := range []string{"multipart/form-data", "curl -X POST", "-F file=@path/to/file.csv", "-F modelId=MODELID"} { - if !strings.Contains(err.Error(), want) { - t.Errorf("error = %q, want it to contain %q", err.Error(), want) - } + t.Error("executor ran despite an invalid body") } } diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index e06ff44..6bbf3b0 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -4,7 +4,6 @@ package openapi import ( - "errors" "fmt" "io" "net/url" @@ -20,10 +19,11 @@ import ( // APIRequest is passed to the executor callback when a generated command runs. type APIRequest struct { - Cmd *cobra.Command - Method string - Path string // fully resolved path with query string - Body []byte // nil for GET/DELETE + Cmd *cobra.Command + Method string + Path string // fully resolved path with query string + Body []byte // nil for GET/DELETE + ContentType string // request body media type; defaults to application/json when empty } // Executor is the callback that actually makes the HTTP request. @@ -83,19 +83,19 @@ type paramInfo struct { } type operationInfo struct { - Tag string - OperationID string - Summary string - Description string - Method string - Path string - PathParams []paramInfo - QueryParams []paramInfo - HasBody bool - BodySchema *base.SchemaProxy // request body schema, when HasBody - BodyNonJSON bool // request body uses a media type the CLI cannot send (e.g. multipart) - BodyMedia string // media type the request body is declared with - Deprecated bool + Tag string + OperationID string + Summary string + Description string + Method string + Path string + PathParams []paramInfo + QueryParams []paramInfo + HasBody bool + BodySchema *base.SchemaProxy // request body schema, when HasBody + BodyMediaType string + BodyFields []multipartFieldInfo + Deprecated bool } func extractOperations(pathStr string, item *v3.PathItem, groups map[string][]*operationInfo) { @@ -161,9 +161,14 @@ func extractOperations(pathStr string, item *v3.PathItem, groups map[string][]*o // Check for request body if op.RequestBody != nil { info.HasBody = true - info.BodySchema = requestBodySchema(op.RequestBody) - info.BodyMedia = requestBodyMediaType(op.RequestBody) - info.BodyNonJSON = !isJSONMediaType(info.BodyMedia) + mediaType, media := requestBodyMediaType(op.RequestBody) + info.BodyMediaType = mediaType + if media != nil { + info.BodySchema = media.Schema + if mediaType == "multipart/form-data" { + info.BodyFields = multipartFields(media.Schema, media.Encoding) + } + } } groups[tag] = append(groups[tag], info) @@ -175,7 +180,7 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { name := commandName(op) use := name for _, p := range op.PathParams { - use += " <" + slugify(p.Name) + ">" + use += " <" + cliFlagName(p.Name) + ">" } short := op.Summary @@ -184,9 +189,6 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { } long := op.Description - if op.BodyNonJSON { - long = strings.TrimSpace(fmt.Sprintf("NOT SUPPORTED: this endpoint requires a %s request body, which the omni CLI cannot send yet.\n\n%s", op.BodyMedia, long)) - } if op.Deprecated { long = "DEPRECATED: " + long } @@ -207,10 +209,9 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { // Build query string from flags query := url.Values{} for _, q := range op.QueryParams { - flagName := slugify(q.Name) - val, err := cmd.Flags().GetString(flagName) + val, err := queryFlagValue(cmd, q.Name) if err != nil { - continue + return err } if val != "" { query.Set(q.Name, val) @@ -220,14 +221,10 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { path += "?" + query.Encode() } - // Read body from stdin, a file, or the flag value itself + // Read body from stdin, a file, or the flag value itself. var body []byte + contentType := op.BodyMediaType if op.HasBody { - if op.BodyNonJSON { - cmd.SilenceUsage = true - return unsupportedBodyError(op) - } - // Flag selection goes by Changed, not by value: an explicitly // empty --body is a mistake worth reporting rather than an // unset flag. @@ -240,7 +237,8 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { flagName = "json-body" } - if cmd.Flags().Changed(flagName) { + bodyProvided := cmd.Flags().Changed(flagName) + if bodyProvided { raw, _ := cmd.Flags().GetString(flagName) var err error body, err = resolveBody(raw, flagName) @@ -251,38 +249,59 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { return err } } + + if op.BodyMediaType == "multipart/form-data" { + var err error + body, contentType, err = buildMultipartBody(cmd, body, bodyProvided, op.BodyFields) + if err != nil { + return err + } + } } return exec(APIRequest{ - Cmd: cmd, - Method: op.Method, - Path: path, - Body: body, + Cmd: cmd, + Method: op.Method, + Path: path, + Body: body, + ContentType: contentType, }) }, } // Register query params as flags for _, q := range op.QueryParams { - flagName := slugify(q.Name) + flagName := cliFlagName(q.Name) desc := q.Description if len(q.Enum) > 0 { desc += fmt.Sprintf(" [%s]", strings.Join(q.Enum, ", ")) } cmd.Flags().String(flagName, "", desc) + + // Before camelCase names were normalized, flags such as modelId were + // exposed as --modelid. Keep a hidden deprecated alias so existing + // scripts continue to work while help and new usage use --model-id. + legacyName := slugify(q.Name) + if legacyName != flagName { + cmd.Flags().String(legacyName, "", "deprecated alias for --"+flagName) + _ = cmd.Flags().MarkDeprecated(legacyName, "use --"+flagName+" instead") + _ = cmd.Flags().MarkHidden(legacyName) + } } - // If the operation accepts a body, add --body and --json-body flags. They - // stay registered for unsendable media types so that `--body ...` reports - // the real reason instead of "unknown flag". + // If the operation accepts a body, add --body and --json-body flags. if op.HasBody { - bodyDesc := `request body as JSON string, "@path/to/file.json" to read a file, or "-" for stdin (run with --schema to see its shape)` - if op.BodyNonJSON { - bodyDesc = fmt.Sprintf("not supported: this endpoint requires a %s request body, which the omni CLI cannot send yet", op.BodyMedia) + bodyHelp := `request body as JSON string, "@path/to/file.json" to read a file, or "-" for stdin (run with --schema to see its shape)` + if op.BodyMediaType == "multipart/form-data" { + bodyHelp = `multipart fields as JSON string, "@path/to/file.json", or "-" for stdin; binary field values are file paths` } - cmd.Flags().String("body", "", bodyDesc) + cmd.Flags().String("body", "", bodyHelp) cmd.Flags().String("json-body", "", `request body as JSON string, "@path/to/file.json", or "-" for stdin (alias for --body)`) cmd.Flags().MarkHidden("json-body") + + if op.BodyMediaType == "multipart/form-data" { + registerMultipartFlags(cmd, op.BodyFields) + } } // Apply body shorthand if one exists for this operation @@ -334,92 +353,44 @@ func schemaRequested(cmd *cobra.Command) bool { return err == nil && v } -// requestBodySchema returns the schema for a request body, preferring the -// application/json media type and falling back to the first declared one. -func requestBodySchema(rb *v3.RequestBody) *base.SchemaProxy { +// requestBodyMediaType returns the media type and definition the CLI will use, +// preferring application/json for backward compatibility and otherwise using +// the first declared media type. +func requestBodyMediaType(rb *v3.RequestBody) (string, *v3.MediaType) { if rb == nil || rb.Content == nil { - return nil + return "", nil } - var first *base.SchemaProxy + var firstType string + var first *v3.MediaType for pair := rb.Content.First(); pair != nil; pair = pair.Next() { mt := pair.Value() - if mt == nil || mt.Schema == nil { + if mt == nil { continue } if pair.Key() == "application/json" { - return mt.Schema + return pair.Key(), mt } if first == nil { - first = mt.Schema - } - } - return first -} - -// requestBodyMediaType returns the media type an operation's request body is -// declared with, preferring JSON. Bodies with no declared content default to -// JSON — that's what the CLI sends. -func requestBodyMediaType(rb *v3.RequestBody) string { - if rb == nil || rb.Content == nil || rb.Content.Len() == 0 { - return "application/json" - } - first := "" - for pair := rb.Content.First(); pair != nil; pair = pair.Next() { - mt := pair.Key() - if isJSONMediaType(mt) { - return mt - } - if first == "" { + firstType = pair.Key() first = mt } } - return first -} - -// isJSONMediaType reports whether a media type is sent as a JSON document. -func isJSONMediaType(mt string) bool { - return mt == "application/json" || strings.HasSuffix(mt, "+json") + return firstType, first } -// unsupportedBodyError explains that an operation's body cannot be sent by the -// CLI. auth.Do labels every request application/json and passes the bytes -// through unchanged, so a multipart body would arrive mislabeled and without -// its part framing — better to say so than to send something the API rejects. -func unsupportedBodyError(op *operationInfo) error { - msg := fmt.Sprintf("this endpoint requires a %s request body, which the omni CLI cannot send yet", op.BodyMedia) - if hint := curlHint(op); hint != "" { - msg += "\nsend it with curl instead:\n " + hint - } - return errors.New(msg) -} - -// curlHint renders a curl command for a multipart operation, naming the parts -// the spec marks required. -func curlHint(op *operationInfo) string { - if op.BodyMedia != "multipart/form-data" || op.BodySchema == nil { - return "" - } - schema := op.BodySchema.Schema() - if schema == nil { - return "" - } - - path := op.Path - for _, p := range op.PathParams { - path = strings.Replace(path, "{"+p.Name+"}", strings.ToUpper(slugify(p.Name)), 1) - } - - cmd := fmt.Sprintf("curl -X %s \"$OMNI_BASE_URL%s\" -H \"Authorization: Bearer $OMNI_API_TOKEN\"", op.Method, path) - for _, name := range schema.Required { - value := strings.ToUpper(slugify(name)) - if prop, ok := schema.Properties.Get(name); ok && prop != nil { - if ps := prop.Schema(); ps != nil && ps.Format == "binary" { - value = "@path/to/file.csv" - } - } - cmd += fmt.Sprintf(" -F %s=%s", name, value) - } - return cmd +// bodyFlagIsJSON reports whether the --body flag value is JSON, and so worth +// validating client-side. A JSON body is sent verbatim; a multipart body takes +// a JSON object of field values that buildMultipartBody turns into form parts. +// A body with no declared content defaults to JSON — that's what the CLI sends. +// Any other media type passes --body through as raw bytes, unvalidated. +func (op *operationInfo) bodyFlagIsJSON() bool { + switch { + case op.BodyMediaType == "", op.BodyMediaType == "application/json": + return true + case op.BodyMediaType == "multipart/form-data": + return true + } + return strings.HasSuffix(op.BodyMediaType, "+json") } // commandName derives a CLI subcommand name from the operationId or method+path. @@ -451,6 +422,24 @@ func slugify(s string) string { return s } +func cliFlagName(s string) string { + return slugify(camelToKebab(s)) +} + +func queryFlagValue(cmd *cobra.Command, parameterName string) (string, error) { + canonicalName := cliFlagName(parameterName) + legacyName := slugify(parameterName) + canonicalChanged := cmd.Flags().Changed(canonicalName) + legacyChanged := legacyName != canonicalName && cmd.Flags().Changed(legacyName) + if canonicalChanged && legacyChanged { + return "", fmt.Errorf("cannot use both --%s and deprecated --%s", canonicalName, legacyName) + } + if legacyChanged { + return cmd.Flags().GetString(legacyName) + } + return cmd.Flags().GetString(canonicalName) +} + func camelToKebab(s string) string { var result strings.Builder for i, r := range s { diff --git a/internal/openapi/generate_test.go b/internal/openapi/generate_test.go index b5083e5..91ae311 100644 --- a/internal/openapi/generate_test.go +++ b/internal/openapi/generate_test.go @@ -263,8 +263,8 @@ func TestGenerateCommands_PathParamsFollowPathOrder(t *testing.T) { } sub := cmds[0].Commands()[0] - if sub.Use != "get-item " { - t.Errorf("Use = %q, want %q", sub.Use, "get-item ") + if sub.Use != "get-item " { + t.Errorf("Use = %q, want %q", sub.Use, "get-item ") } // Positional args in path order must substitute into the matching slots. @@ -354,6 +354,61 @@ func TestBuildCommand_QueryFlags(t *testing.T) { } } +func TestBuildCommand_CamelCaseQueryFlags(t *testing.T) { + op := &operationInfo{ + Tag: "test", + OperationID: "testListItems", + Method: "GET", + Path: "/api/v1/items", + QueryParams: []paramInfo{ + {Name: "modelId", In: "query"}, + {Name: "searchTerm", In: "query"}, + }, + } + + t.Run("canonical kebab-case flags", func(t *testing.T) { + var captured APIRequest + cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) + if cmd.Flags().Lookup("model-id") == nil || cmd.Flags().Lookup("search-term") == nil { + t.Fatal("missing canonical camelCase query flags") + } + legacy := cmd.Flags().Lookup("modelid") + if legacy == nil || legacy.Deprecated == "" || !legacy.Hidden { + t.Fatalf("legacy --modelid flag = %#v, want hidden and deprecated", legacy) + } + cmd.SetArgs([]string{"--model-id", "model-123", "--search-term", "people"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.Contains(captured.Path, "modelId=model-123") || !strings.Contains(captured.Path, "searchTerm=people") { + t.Errorf("path = %q, want original OpenAPI parameter names", captured.Path) + } + }) + + t.Run("legacy aliases remain compatible", func(t *testing.T) { + var captured APIRequest + cmd := buildCommand(op, func(req APIRequest) error { captured = req; return nil }) + cmd.SetArgs([]string{"--modelid", "model-123", "--searchterm", "people"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.Contains(captured.Path, "modelId=model-123") || !strings.Contains(captured.Path, "searchTerm=people") { + t.Errorf("path = %q, want legacy aliases to preserve parameter names", captured.Path) + } + }) + + t.Run("canonical and legacy conflict", func(t *testing.T) { + cmd := buildCommand(op, func(req APIRequest) error { return nil }) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--model-id", "new", "--modelid", "old"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "cannot use both --model-id and deprecated --modelid") { + t.Fatalf("error = %v, want conflicting alias error", err) + } + }) +} + // Verify that operations with a request body (POST/PUT/PATCH) get a --body // flag, and the JSON value passed to it ends up in APIRequest.Body. func TestBuildCommand_BodyFlag(t *testing.T) { @@ -491,13 +546,20 @@ func parseSpecOperations(t *testing.T, specData []byte) []specOperation { Path: pathStr, PathParams: pathParams, HasBody: op.RequestBody != nil, - BodyMedia: requestBodyMediaType(op.RequestBody), + BodyMedia: specBodyMediaType(op.RequestBody), }) } } return ops } +// specBodyMediaType names the media type the generator will pick for a request +// body, so the coverage table records what each operation is sent as. +func specBodyMediaType(rb *v3.RequestBody) string { + mediaType, _ := requestBodyMediaType(rb) + return mediaType +} + func TestSpecCoverage(t *testing.T) { specData := loadSpec(t) specOps := parseSpecOperations(t, specData) @@ -565,18 +627,6 @@ func TestSpecCoverage(t *testing.T) { continue } - // Media types the CLI can't build (multipart uploads) are covered - // by refusing client-side with a message that names the media type. - if sop.HasBody && !isJSONMediaType(sop.BodyMedia) { - err := sub.RunE(sub, args) - if err == nil || !strings.Contains(err.Error(), sop.BodyMedia) { - failures = append(failures, fmt.Sprintf("%s: want an unsupported-%s error, got %v", key, sop.BodyMedia, err)) - continue - } - called[sop.OperationID] = true - continue - } - if err := sub.RunE(sub, args); err != nil { failures = append(failures, fmt.Sprintf("%s: RunE: %v", key, err)) continue diff --git a/internal/openapi/multipart.go b/internal/openapi/multipart.go new file mode 100644 index 0000000..7514c4e --- /dev/null +++ b/internal/openapi/multipart.go @@ -0,0 +1,326 @@ +package openapi + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "mime" + "mime/multipart" + "net/textproto" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + + "github.com/pb33f/libopenapi/datamodel/high/base" + v3 "github.com/pb33f/libopenapi/datamodel/high/v3" + "github.com/pb33f/libopenapi/orderedmap" + "github.com/spf13/cobra" +) + +// multipartFieldInfo is the CLI-facing subset of a multipart schema property. +// A binary field consumes a file path; other fields are serialized as form +// values according to their JSON type. +type multipartFieldInfo struct { + Name string + FlagName string + Description string + Type string + Required bool + Binary bool + ContentType string + Explode bool +} + +func multipartFields(schemaProxy *base.SchemaProxy, encodings *orderedmap.Map[string, *v3.Encoding]) []multipartFieldInfo { + fields := make([]multipartFieldInfo, 0) + indexes := map[string]int{} + required := map[string]bool{} + seen := map[*base.Schema]bool{} + + var collect func(*base.SchemaProxy) + collect = func(proxy *base.SchemaProxy) { + if proxy == nil || proxy.Schema() == nil { + return + } + schema := proxy.Schema() + if seen[schema] { + return + } + seen[schema] = true + + for _, name := range schema.Required { + required[name] = true + } + for _, parent := range schema.AllOf { + collect(parent) + } + if schema.Properties == nil { + return + } + for pair := schema.Properties.First(); pair != nil; pair = pair.Next() { + name := pair.Key() + property := pair.Value() + propertySchema := property.Schema() + field := multipartFieldInfo{Name: name, FlagName: cliFlagName(name), Type: "string", Explode: true} + if propertySchema != nil { + field.Description = propertySchema.Description + field.Type = schemaType(property) + field.Binary = strings.EqualFold(propertySchema.Format, "binary") || strings.EqualFold(propertySchema.ContentEncoding, "binary") + field.ContentType = propertySchema.ContentMediaType + } + if encodings != nil { + if encoding, ok := encodings.Get(name); ok && encoding != nil { + if encoding.ContentType != "" { + field.ContentType = encoding.ContentType + } + if encoding.Explode != nil { + field.Explode = *encoding.Explode + } + if encoding.ContentType == "application/octet-stream" { + field.Binary = true + } + } + } + + if index, ok := indexes[name]; ok { + fields[index] = field + } else { + indexes[name] = len(fields) + fields = append(fields, field) + } + } + } + + collect(schemaProxy) + for i := range fields { + fields[i].Required = required[fields[i].Name] + } + return fields +} + +func registerMultipartFlags(cmd *cobra.Command, fields []multipartFieldInfo) { + reserved := map[string]bool{ + "body": true, "json-body": true, "schema": true, "field": true, "depth": true, + "profile": true, "token": true, "base-url": true, "compact": true, "format": true, "help": true, + } + for i := range fields { + field := &fields[i] + flagName := field.FlagName + if reserved[flagName] || cmd.Flags().Lookup(flagName) != nil { + flagName = "form-" + flagName + } + field.FlagName = flagName + + description := field.Description + if description == "" { + description = "multipart field " + field.Name + } + if field.Binary { + description += " (file path)" + } + if field.Required { + description += " [required unless supplied via --body]" + } + cmd.Flags().String(flagName, "", description) + } +} + +func buildMultipartBody(cmd *cobra.Command, rawBody []byte, bodyProvided bool, fields []multipartFieldInfo) ([]byte, string, error) { + values := map[string]interface{}{} + if bodyProvided { + decoder := json.NewDecoder(bytes.NewReader(rawBody)) + decoder.UseNumber() + if err := decoder.Decode(&values); err != nil { + return nil, "", fmt.Errorf("invalid multipart --body JSON: %w", err) + } + } + + for _, field := range fields { + if !cmd.Flags().Changed(field.FlagName) { + continue + } + value, err := cmd.Flags().GetString(field.FlagName) + if err != nil { + return nil, "", fmt.Errorf("reading --%s: %w", field.FlagName, err) + } + parsed, err := parseMultipartFlagValue(field, value) + if err != nil { + return nil, "", fmt.Errorf("invalid --%s: %w", field.FlagName, err) + } + values[field.Name] = parsed + } + + // Flag-based invocation gets local required-field validation. Raw --body is + // deliberately passed through more loosely, matching JSON request behavior. + if !bodyProvided { + for _, field := range fields { + if field.Required { + if _, ok := values[field.Name]; !ok { + return nil, "", fmt.Errorf("required multipart field %q is missing (use --%s or --body)", field.Name, field.FlagName) + } + } + } + } + + var body bytes.Buffer + writer := multipart.NewWriter(&body) + known := map[string]bool{} + for _, field := range fields { + known[field.Name] = true + value, ok := values[field.Name] + if !ok { + continue + } + if err := writeMultipartValue(writer, field, value); err != nil { + return nil, "", err + } + } + + // Preserve extra properties supplied through --body even when the schema + // permits them without naming them explicitly. + var extras []string + for name := range values { + if !known[name] { + extras = append(extras, name) + } + } + sort.Strings(extras) + for _, name := range extras { + field := multipartFieldInfo{Name: name, Type: inferMultipartType(values[name]), Explode: true} + if err := writeMultipartValue(writer, field, values[name]); err != nil { + return nil, "", err + } + } + + if err := writer.Close(); err != nil { + return nil, "", fmt.Errorf("finalizing multipart body: %w", err) + } + return body.Bytes(), writer.FormDataContentType(), nil +} + +func parseMultipartFlagValue(field multipartFieldInfo, value string) (interface{}, error) { + if field.Binary || field.Type == "string" || field.Type == "" { + return value, nil + } + switch field.Type { + case "boolean": + return strconv.ParseBool(value) + case "integer": + return strconv.ParseInt(value, 10, 64) + case "number": + return strconv.ParseFloat(value, 64) + case "array", "object": + var parsed interface{} + decoder := json.NewDecoder(strings.NewReader(value)) + decoder.UseNumber() + if err := decoder.Decode(&parsed); err != nil { + return nil, fmt.Errorf("expected JSON %s: %w", field.Type, err) + } + return parsed, nil + default: + return value, nil + } +} + +func writeMultipartValue(writer *multipart.Writer, field multipartFieldInfo, value interface{}) error { + if values, ok := value.([]interface{}); ok && field.Explode { + for _, item := range values { + if err := writeMultipartSingleValue(writer, field, item); err != nil { + return err + } + } + return nil + } + return writeMultipartSingleValue(writer, field, value) +} + +func writeMultipartSingleValue(writer *multipart.Writer, field multipartFieldInfo, value interface{}) error { + if field.Binary { + path, ok := value.(string) + if !ok { + return fmt.Errorf("multipart file field %q must be a file path", field.Name) + } + file, err := os.Open(path) + if err != nil { + return fmt.Errorf("opening multipart file %q for field %q: %w", path, field.Name, err) + } + defer file.Close() + + contentType := field.ContentType + if contentType == "" { + contentType = "application/octet-stream" + } + header := make(textproto.MIMEHeader) + header.Set("Content-Disposition", mime.FormatMediaType("form-data", map[string]string{ + "name": field.Name, "filename": filepath.Base(path), + })) + header.Set("Content-Type", contentType) + part, err := writer.CreatePart(header) + if err != nil { + return fmt.Errorf("creating multipart file field %q: %w", field.Name, err) + } + if _, err := io.Copy(part, file); err != nil { + return fmt.Errorf("reading multipart file %q for field %q: %w", path, field.Name, err) + } + return nil + } + + contentType := field.ContentType + var encoded string + switch typed := value.(type) { + case string: + encoded = typed + case nil: + encoded = "null" + case json.Number: + encoded = typed.String() + case bool, float32, float64, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64: + encoded = fmt.Sprint(typed) + default: + data, err := json.Marshal(value) + if err != nil { + return fmt.Errorf("encoding multipart field %q: %w", field.Name, err) + } + encoded = string(data) + if contentType == "" { + contentType = "application/json" + } + } + + if contentType == "" { + part, err := writer.CreateFormField(field.Name) + if err != nil { + return fmt.Errorf("creating multipart field %q: %w", field.Name, err) + } + _, err = io.WriteString(part, encoded) + return err + } + + header := make(textproto.MIMEHeader) + header.Set("Content-Disposition", mime.FormatMediaType("form-data", map[string]string{"name": field.Name})) + header.Set("Content-Type", contentType) + part, err := writer.CreatePart(header) + if err != nil { + return fmt.Errorf("creating multipart field %q: %w", field.Name, err) + } + _, err = io.WriteString(part, encoded) + return err +} + +func inferMultipartType(value interface{}) string { + switch value.(type) { + case []interface{}: + return "array" + case map[string]interface{}: + return "object" + case bool: + return "boolean" + case json.Number, float64: + return "number" + default: + return "string" + } +} diff --git a/internal/openapi/multipart_test.go b/internal/openapi/multipart_test.go new file mode 100644 index 0000000..c3caf36 --- /dev/null +++ b/internal/openapi/multipart_test.go @@ -0,0 +1,291 @@ +package openapi + +import ( + "bytes" + "encoding/json" + "io" + "mime" + "mime/multipart" + "os" + "path/filepath" + "strings" + "testing" +) + +const multipartTestSpec = `{ + "openapi": "3.1.0", + "info": {"title": "multipart test", "version": "1.0"}, + "paths": { + "/uploads": { + "post": { + "operationId": "uploadsCreate", + "tags": ["Uploads"], + "requestBody": { + "required": true, + "content": { + "multipart/form-data": { + "schema": { + "type": "object", + "required": ["file", "modelId"], + "properties": { + "file": {"type": "string", "format": "binary", "description": "CSV to upload"}, + "modelId": {"type": "string", "description": "target model"}, + "viewName": {"type": "string"}, + "publish": {"type": "boolean"}, + "labels": {"type": "array", "items": {"type": "string"}} + } + }, + "encoding": { + "file": {"contentType": "text/csv"} + } + } + } + }, + "responses": {"201": {"description": "created"}} + } + } + } +}` + +type capturedPart struct { + fileName string + contentType string + values []string +} + +func parseCapturedMultipart(t *testing.T, request APIRequest) map[string]capturedPart { + t.Helper() + mediaType, params, err := mime.ParseMediaType(request.ContentType) + if err != nil { + t.Fatalf("ParseMediaType(%q): %v", request.ContentType, err) + } + if mediaType != "multipart/form-data" { + t.Fatalf("media type = %q, want multipart/form-data", mediaType) + } + boundary := params["boundary"] + if boundary == "" { + t.Fatal("multipart content type has no boundary") + } + + parts := map[string]capturedPart{} + reader := multipart.NewReader(bytes.NewReader(request.Body), boundary) + for { + part, err := reader.NextPart() + if err == io.EOF { + break + } + if err != nil { + t.Fatalf("NextPart: %v", err) + } + data, err := io.ReadAll(part) + if err != nil { + t.Fatalf("ReadAll(%s): %v", part.FormName(), err) + } + got := parts[part.FormName()] + got.fileName = part.FileName() + got.contentType = part.Header.Get("Content-Type") + got.values = append(got.values, string(data)) + parts[part.FormName()] = got + } + return parts +} + +func TestGenerateCommands_MultipartFlagsAndBody(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "people.csv") + if err := os.WriteFile(filePath, []byte("name\nAda\n"), 0o600); err != nil { + t.Fatal(err) + } + + var captured APIRequest + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + captured = request + return nil + }) + if err != nil { + t.Fatalf("GenerateCommands: %v", err) + } + command := commands[0].Commands()[0] + for _, flag := range []string{"file", "model-id", "view-name", "publish", "labels"} { + if command.Flags().Lookup(flag) == nil { + t.Errorf("missing generated --%s flag", flag) + } + } + + for flag, value := range map[string]string{ + "file": filePath, "model-id": "model-123", "view-name": "people", "publish": "true", "labels": `["one","two"]`, + } { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + if err := command.RunE(command, nil); err != nil { + t.Fatalf("RunE: %v", err) + } + + parts := parseCapturedMultipart(t, captured) + if got := parts["file"]; got.fileName != "people.csv" || got.contentType != "text/csv" || len(got.values) != 1 || got.values[0] != "name\nAda\n" { + t.Errorf("file part = %#v", got) + } + if got := parts["modelId"].values; len(got) != 1 || got[0] != "model-123" { + t.Errorf("modelId = %#v", got) + } + if got := parts["publish"].values; len(got) != 1 || got[0] != "true" { + t.Errorf("publish = %#v", got) + } + if got := parts["labels"].values; len(got) != 2 || got[0] != "one" || got[1] != "two" { + t.Errorf("labels = %#v", got) + } +} + +func TestGenerateCommands_MultipartJSONBody(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "data.csv") + if err := os.WriteFile(filePath, []byte("id\n1\n"), 0o600); err != nil { + t.Fatal(err) + } + + var captured APIRequest + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + captured = request + return nil + }) + if err != nil { + t.Fatalf("GenerateCommands: %v", err) + } + command := commands[0].Commands()[0] + body, err := json.Marshal(map[string]interface{}{ + "file": filePath, "modelId": "from-body", "extra": map[string]bool{"ok": true}, + }) + if err != nil { + t.Fatal(err) + } + if err := command.Flags().Set("body", string(body)); err != nil { + t.Fatal(err) + } + if err := command.RunE(command, nil); err != nil { + t.Fatalf("RunE: %v", err) + } + + parts := parseCapturedMultipart(t, captured) + if got := parts["modelId"].values; len(got) != 1 || got[0] != "from-body" { + t.Errorf("modelId = %#v", got) + } + if got := parts["extra"]; got.contentType != "application/json" || len(got.values) != 1 || got.values[0] != `{"ok":true}` { + t.Errorf("extra = %#v", got) + } +} + +func TestGenerateCommands_MultipartValidatesFlagInvocation(t *testing.T) { + called := false + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + called = true + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + if err := command.Flags().Set("model-id", "model-123"); err != nil { + t.Fatal(err) + } + err = command.RunE(command, nil) + if err == nil || !strings.Contains(err.Error(), `required multipart field "file"`) { + t.Fatalf("error = %v, want missing file", err) + } + if called { + t.Fatal("executor called for invalid multipart invocation") + } +} + +func TestRealSpec_UploadCommandsExposeFileFlags(t *testing.T) { + commands, err := GenerateCommands(loadSpec(t), func(request APIRequest) error { return nil }) + if err != nil { + t.Fatal(err) + } + var uploadsFound int + for _, group := range commands { + if group.Name() != "uploads" { + continue + } + for _, command := range group.Commands() { + if command.Name() != "create" && command.Name() != "replace-data" { + continue + } + uploadsFound++ + if command.Flags().Lookup("file") == nil { + t.Errorf("uploads %s has no generated --file flag", command.Name()) + } + } + } + if uploadsFound != 2 { + t.Fatalf("found %d multipart upload commands, want 2", uploadsFound) + } +} + +// Multipart bodies are JSON too, so they get the same --body handling as JSON +// operations: @file reading and a client-side validity check. +func TestGenerateCommands_MultipartBodyFromFile(t *testing.T) { + dir := t.TempDir() + csvPath := filepath.Join(dir, "data.csv") + if err := os.WriteFile(csvPath, []byte("id\n1\n"), 0o600); err != nil { + t.Fatal(err) + } + bodyPath := filepath.Join(dir, "body.json") + body, err := json.Marshal(map[string]string{"file": csvPath, "modelId": "from-file"}) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(bodyPath, body, 0o600); err != nil { + t.Fatal(err) + } + + var captured APIRequest + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + captured = request + return nil + }) + if err != nil { + t.Fatalf("GenerateCommands: %v", err) + } + command := commands[0].Commands()[0] + if err := command.Flags().Set("body", "@"+bodyPath); err != nil { + t.Fatal(err) + } + if err := command.RunE(command, nil); err != nil { + t.Fatalf("RunE: %v", err) + } + + parts := parseCapturedMultipart(t, captured) + if got := parts["modelId"].values; len(got) != 1 || got[0] != "from-file" { + t.Errorf("modelId = %#v", got) + } + if got := parts["file"]; got.fileName != "data.csv" { + t.Errorf("file = %#v", got) + } +} + +func TestGenerateCommands_MultipartBodyPathHint(t *testing.T) { + bodyPath := filepath.Join(t.TempDir(), "body.json") + if err := os.WriteFile(bodyPath, []byte(`{"modelId":"m"}`), 0o600); err != nil { + t.Fatal(err) + } + + called := false + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + called = true + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + if err := command.Flags().Set("body", bodyPath); err != nil { + t.Fatal(err) + } + err = command.RunE(command, nil) + if err == nil || !strings.Contains(err.Error(), "looks like a file path") { + t.Fatalf("error = %v, want file-path hint", err) + } + if called { + t.Fatal("executor called for a path-shaped --body") + } +} From fea02058f3e590901da3c4f3187ee4668bda86f7 Mon Sep 17 00:00:00 2001 From: Ernesto Ongaro Date: Tue, 25 Aug 2026 20:08:59 +0100 Subject: [PATCH 5/5] fix(openapi): close review findings left open in the multipart merge (#82) Follow-up to the multipart work now carried on this branch (ffebcee). `3d9b0e1` and `0827738` already covered the body-flag Changed state and the file-path hint; these are the findings from #72's review that no commit here has picked up yet. - Binary multipart field paths never expanded `~`, unlike `--body @path`, so `--file ~/people.csv` failed with "no such file or directory". - `--body null` decoded into a nil map and panicked ("assignment to entry in nil map") as soon as any generated flag was merged into it. - Array and object flag values decoded into interface{}, so an object was accepted where the schema says array, and anything after the first JSON value was silently dropped: `--labels '["a"] oops'` sent `["a"]`. The declared type is pinned now and the input must end there. - registerMultipartFlags checked its "form-" replacement against nothing, so two fields colliding on one flag name would register the same pflag twice and panic at startup, taking down every command, not just the upload. - requestBodyMediaType could prefer a schema-less application/json entry over a real multipart definition. Each fix has a regression test; without the source changes the tilde and nil-map tests fail, the latter by panicking. Not addressed: Copilot's note that the upload file is buffered in memory before the request is sent. Streaming means threading an io.Reader through APIRequest and internal/auth, which is wider than a review-fix pass. Claude-Session: https://claude.ai/code/session_01DYuiGGkmQifkCbF2qL8Lt6 Co-authored-by: Claude Opus 5 (1M context) --- internal/openapi/generate.go | 5 +- internal/openapi/multipart.go | 55 ++++++++++-- internal/openapi/multipart_test.go | 138 +++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 10 deletions(-) diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index 7df83c3..07bcfc9 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -417,7 +417,8 @@ func schemaRequested(cmd *cobra.Command, name string) bool { // requestBodyMediaType returns the media type and definition the CLI will use, // preferring application/json for backward compatibility and otherwise using -// the first declared media type. +// the first declared media type. Entries with no schema are skipped, so a +// schema-less application/json stub never shadows a real multipart definition. func requestBodyMediaType(rb *v3.RequestBody) (string, *v3.MediaType) { if rb == nil || rb.Content == nil { return "", nil @@ -426,7 +427,7 @@ func requestBodyMediaType(rb *v3.RequestBody) (string, *v3.MediaType) { var first *v3.MediaType for pair := rb.Content.First(); pair != nil; pair = pair.Next() { mt := pair.Value() - if mt == nil { + if mt == nil || mt.Schema == nil { continue } if pair.Key() == "application/json" { diff --git a/internal/openapi/multipart.go b/internal/openapi/multipart.go index f44191c..5ed77ee 100644 --- a/internal/openapi/multipart.go +++ b/internal/openapi/multipart.go @@ -106,12 +106,21 @@ func registerMultipartFlags(cmd *cobra.Command, fields []multipartFieldInfo) { "body": true, "json-body": true, "schema": true, "field": true, "depth": true, "profile": true, "token": true, "base-url": true, "compact": true, "format": true, "help": true, } + taken := func(name string) bool { + return reserved[name] || cmd.Flags().Lookup(name) != nil + } for i := range fields { field := &fields[i] flagName := field.FlagName - if reserved[flagName] || cmd.Flags().Lookup(flagName) != nil { + if taken(flagName) { flagName = "form-" + flagName } + // Two fields can collide on the same prefixed name. Registering a + // duplicate makes pflag panic, which would take down the whole CLI at + // startup, so keep suffixing until the name is free. + for suffix := 2; taken(flagName); suffix++ { + flagName = fmt.Sprintf("form-%s-%d", field.FlagName, suffix) + } field.FlagName = flagName description := field.Description @@ -136,6 +145,11 @@ func buildMultipartBody(cmd *cobra.Command, rawBody []byte, bodyProvided bool, f if err := decoder.Decode(&values); err != nil { return nil, "", fmt.Errorf("invalid multipart --body JSON: %w", err) } + // JSON "null" decodes into a nil map, which the flag merge below would + // panic on. + if values == nil { + return nil, "", fmt.Errorf("multipart --body must be a JSON object of field values") + } } for _, field := range fields { @@ -213,18 +227,41 @@ func parseMultipartFlagValue(field multipartFieldInfo, value string) (interface{ case "number": return strconv.ParseFloat(value, 64) case "array", "object": - var parsed interface{} - decoder := json.NewDecoder(strings.NewReader(value)) - decoder.UseNumber() - if err := decoder.Decode(&parsed); err != nil { - return nil, fmt.Errorf("expected JSON %s: %w", field.Type, err) - } - return parsed, nil + return decodeJSONFlagValue(field.Type, value) default: return value, nil } } +// decodeJSONFlagValue parses a JSON flag value against the field's declared +// type. Decoding into interface{} would accept an object where the schema says +// array and would silently ignore anything after the first value, so the type +// is pinned and the input must end there. +func decodeJSONFlagValue(fieldType, value string) (interface{}, error) { + decoder := json.NewDecoder(strings.NewReader(value)) + decoder.UseNumber() + + var parsed interface{} + if fieldType == "array" { + var typed []interface{} + if err := decoder.Decode(&typed); err != nil { + return nil, fmt.Errorf("expected JSON array: %w", err) + } + parsed = typed + } else { + var typed map[string]interface{} + if err := decoder.Decode(&typed); err != nil { + return nil, fmt.Errorf("expected JSON object: %w", err) + } + parsed = typed + } + + if err := decoder.Decode(new(json.RawMessage)); err != io.EOF { + return nil, fmt.Errorf("expected a single JSON %s with no trailing data", fieldType) + } + return parsed, nil +} + func writeMultipartValue(writer *multipart.Writer, field multipartFieldInfo, value interface{}) error { if values, ok := value.([]interface{}); ok && field.Explode { for _, item := range values { @@ -243,6 +280,8 @@ func writeMultipartSingleValue(writer *multipart.Writer, field multipartFieldInf if !ok { return fmt.Errorf("multipart file field %q must be a file path", field.Name) } + // Match --body @path: shells don't expand "~" inside a flag value. + path = expandHome(path) file, err := os.Open(path) if err != nil { return fmt.Errorf("opening multipart file %q for field %q: %w", path, field.Name, err) diff --git a/internal/openapi/multipart_test.go b/internal/openapi/multipart_test.go index c3caf36..697251f 100644 --- a/internal/openapi/multipart_test.go +++ b/internal/openapi/multipart_test.go @@ -10,6 +10,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/spf13/cobra" ) const multipartTestSpec = `{ @@ -289,3 +291,139 @@ func TestGenerateCommands_MultipartBodyPathHint(t *testing.T) { t.Fatal("executor called for a path-shaped --body") } } + +// An explicitly empty --body is a body the caller asked for, so it gets the +// "omit the flag" error rather than reaching buildMultipartBody as a bare EOF. +func TestGenerateCommands_MultipartEmptyBodyFlag(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "people.csv") + if err := os.WriteFile(filePath, []byte("name\nAda\n"), 0o600); err != nil { + t.Fatal(err) + } + + called := false + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + called = true + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + for flag, value := range map[string]string{"file": filePath, "model-id": "model-123", "body": ""} { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + + err = command.RunE(command, nil) + if err == nil || !strings.Contains(err.Error(), "--body is empty") { + t.Fatalf("error = %v, want the empty --body message", err) + } + if called { + t.Fatal("executor called for an empty --body") + } +} + +// Binary field values expand "~" the same way --body @path does; shells leave +// it alone inside a flag value. +func TestGenerateCommands_MultipartExpandsHomeInFilePath(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if err := os.WriteFile(filepath.Join(home, "people.csv"), []byte("name\nAda\n"), 0o600); err != nil { + t.Fatal(err) + } + + var captured APIRequest + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + captured = request + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + for flag, value := range map[string]string{"file": "~/people.csv", "model-id": "model-123"} { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + if err := command.RunE(command, nil); err != nil { + t.Fatalf("RunE: %v", err) + } + + parts := parseCapturedMultipart(t, captured) + if got := parts["file"]; got.fileName != "people.csv" || len(got.values) != 1 || got.values[0] != "name\nAda\n" { + t.Errorf("file part = %#v", got) + } +} + +// JSON "null" decodes into a nil map, which the flag merge would panic on. +func TestGenerateCommands_MultipartRejectsNullBody(t *testing.T) { + called := false + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + called = true + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + for flag, value := range map[string]string{"body": "null", "model-id": "model-123"} { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + + err = command.RunE(command, nil) + if err == nil || !strings.Contains(err.Error(), "JSON object of field values") { + t.Fatalf("error = %v, want a non-object --body error", err) + } + if called { + t.Fatal("executor called for a null --body") + } +} + +func TestParseMultipartFlagValue_ChecksTypeAndTrailingData(t *testing.T) { + array := multipartFieldInfo{Name: "labels", Type: "array"} + object := multipartFieldInfo{Name: "meta", Type: "object"} + + if _, err := parseMultipartFlagValue(array, `{"x":1}`); err == nil { + t.Error("an object passed for an array field was accepted") + } + if _, err := parseMultipartFlagValue(object, `["a"]`); err == nil { + t.Error("an array passed for an object field was accepted") + } + if _, err := parseMultipartFlagValue(array, `["a"] trailing`); err == nil { + t.Error("trailing data after an array was accepted") + } + if _, err := parseMultipartFlagValue(array, `["a","b"]`); err != nil { + t.Errorf("valid array rejected: %v", err) + } +} + +// Two fields whose flag names collide must not register the same pflag twice — +// pflag panics on a redefinition, taking down the whole CLI at startup. +func TestRegisterMultipartFlags_ResolvesCollisions(t *testing.T) { + command := &cobra.Command{Use: "upload"} + fields := []multipartFieldInfo{ + {Name: "body", FlagName: "body"}, + {Name: "Body", FlagName: "body"}, + {Name: "body_", FlagName: "body"}, + } + + registerMultipartFlags(command, fields) + + seen := map[string]bool{} + for _, field := range fields { + if field.FlagName == "body" { + t.Errorf("field %q kept the reserved --body name", field.Name) + } + if seen[field.FlagName] { + t.Errorf("duplicate flag name %q", field.FlagName) + } + seen[field.FlagName] = true + if command.Flags().Lookup(field.FlagName) == nil { + t.Errorf("flag --%s was not registered", field.FlagName) + } + } +}