From bd21cadd5c721cb39157111d6cea3dc983b8c968 Mon Sep 17 00:00:00 2001 From: Daniel Spangenberger Date: Mon, 24 Aug 2026 11:05:46 -0400 Subject: [PATCH 1/2] feat(openapi): enforce required query params, add --query escape hatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec-required query params were registered as ordinary optional flags, so a missing one only surfaced as a server 400 after a round trip. They are now marked required on the cobra flag (and labelled "(required)" in help), so the failure is local and immediate. The reverse gap was worse: when the server requires a query param the spec omits entirely, the operation was uncallable — no flag existed to send it. A repeatable --query key=value flag is now registered on every generated command and merged into the query string alongside the declared flags. Keys are sent verbatim so the server sees exactly what was typed, repeating a key sends every value (array params), and a key that duplicates an explicitly-set declared flag errors instead of silently picking one. Constraint: `omni --schema` must stay zero-friction (no args, no token, no API call), but cobra validates required flags before RunE, so the existing RunE/Args short-circuit could not cover them Constraint: --query must never panic flag registration on a spec that declares a param slugifying to "query" Rejected: mark required only when "--schema" is absent from os.Args | untestable in-process and couples flag wiring to process argv Rejected: clear the required annotation in PreRunE | leaves the requirement relaxed for any later invocation on the same command object; PreRunE now rewrites the annotation to the correct value every run instead Rejected: last-one-wins when --query duplicates a declared flag | silently drops a value the user explicitly asked for Confidence: high Scope-risk: moderate Directive: --query is registered after spec params and body-shorthand flags and guarded by a Lookup; keep that ordering so a future flag named "query" degrades rather than panics Not-tested: real server behavior for repeated --query keys on a param the API does not treat as an array Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s --- cmd/omni/agent_help.go | 3 + internal/openapi/generate.go | 120 +++++++++++++- internal/openapi/generate_test.go | 260 ++++++++++++++++++++++++++++++ 3 files changed, 381 insertions(+), 2 deletions(-) diff --git a/cmd/omni/agent_help.go b/cmd/omni/agent_help.go index 32fbe30..684fbfb 100644 --- a/cmd/omni/agent_help.go +++ b/cmd/omni/agent_help.go @@ -132,6 +132,7 @@ and maps, so you can name a leaf without knowing the container shape: then exit (works on every API command) --field PATH With --schema: drill into a dotted field path of the body --depth N With --schema: cap nesting depth (lower = smaller) + --query K=V Extra query parameter not declared in the spec (repeatable) ## Tips - Use "omni ai generate-query" to answer data questions — it picks fields and filters for you. @@ -143,6 +144,8 @@ and maps, so you can name a leaf without knowing the container shape: The few hand-written commands (config *, agent-help, models create-branch, users set-attributes) have no Arguments section — read their Usage line. - Query parameters are flags: omni models list --page-size 10 +- Params the spec marks required are enforced before the request; a missing one fails locally. +- If the server demands a query param the spec doesn't declare, send it with --query key=value (repeatable). - Flag names are kebab-case, and spelling is forgiving: case, dashes and underscores are ignored, so --branch-id, --branchId, --branch_id and --branchid all mean the same flag. --help always shows the canonical form. diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index 04bf96e..07ffce7 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -234,6 +234,12 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { query.Set(qf.Param.Name, val) } } + if err := applyExtraQueryParams(cmd, queryFlags, query); err != nil { + return err + } + if err := checkRequiredQueryParams(queryFlags, query); err != nil { + return err + } if len(query) > 0 { path += "?" + query.Encode() } @@ -280,7 +286,10 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { // canonical --branch-id no matter how the spec spelled the param. cmd.Flags().SetNormalizeFunc(NormalizeFlagName) - // Register query params as flags + // Register query params as flags. Params the spec marks required are marked + // required on the flag too, so a missing one fails here instead of costing a + // round trip and a server 400. + hasRequiredQuery := false for _, qf := range queryFlags { desc := qf.Param.Description if len(qf.Param.Enum) > 0 { @@ -295,7 +304,14 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { } desc += qf.Note } + if qf.Param.Required { + desc = strings.TrimSpace(desc + " (required)") + } cmd.Flags().String(qf.Name, "", desc) + if qf.Param.Required { + cmd.MarkFlagRequired(qf.Name) + hasRequiredQuery = true + } } // If the operation accepts a body, add --body and --json-body flags @@ -311,6 +327,15 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { applyBodyShorthand(cmd, op, sh) } + // Escape hatch for query params the spec doesn't declare: without it, an + // operation whose server requires an undeclared param is simply uncallable. + // Registered after every other flag and guarded so a spec param or shorthand + // flag that resolves to "query" keeps its own name instead of panicking the + // flag registration. + if cmd.Flags().Lookup(queryFlagName) == nil { + cmd.Flags().StringArray(queryFlagName, nil, "send an extra query parameter not declared in the spec (repeatable, key=value)") + } + // Describe the positional args in the long help. Cobra's usage line only // shows the placeholders, so without this the spec's param descriptions // (e.g. "branch name", not "branch UUID") never reach the reader. @@ -325,10 +350,33 @@ func buildCommand(op *operationInfo, exec Executor) *cobra.Command { // Args/RunE, so the short-circuit wraps the final versions. Registered for // every operation — bodyless ones still describe their args, query flags and // response shape. - RegisterSchemaFlag(cmd, func(c *cobra.Command, names SchemaFlags) error { + names := RegisterSchemaFlag(cmd, func(c *cobra.Command, names SchemaFlags) error { return emitBodySchema(c, op, names) }) + // Required query flags must not block --schema: it's pure local discovery + // with no API call, so it needs no params at all. Cobra runs PreRunE before + // ValidateRequiredFlags, so toggling the annotation here is enough. It's + // rewritten on every invocation (not just cleared) so a --schema run can't + // leave the requirement relaxed for a later one. + if hasRequiredQuery { + cmd.PreRunE = func(c *cobra.Command, args []string) error { + required := "true" + if schemaRequested(c, names.Schema) { + required = "false" + } + for _, qf := range queryFlags { + if !qf.Param.Required { + continue + } + if err := c.Flags().SetAnnotation(qf.Name, cobra.BashCompOneRequiredFlag, []string{required}); err != nil { + return err + } + } + return nil + } + } + return cmd } @@ -379,6 +427,74 @@ func firstLine(s string) string { return strings.TrimSpace(s) } +// queryFlagName is the generic escape hatch flag for query params the spec +// doesn't declare. +const queryFlagName = "query" + +// applyExtraQueryParams merges repeatable --query key=value pairs into the +// query string. Keys are sent verbatim — the server sees exactly what the user +// typed. A key that also has an explicitly-set declared flag is ambiguous and +// errors rather than silently picking one; repeating the same key is allowed +// and sends every value (some params are arrays). +func applyExtraQueryParams(cmd *cobra.Command, queryFlags []queryFlag, query url.Values) error { + extras, err := cmd.Flags().GetStringArray(queryFlagName) + if err != nil { + // The flag isn't registered — a spec param claimed the name. + return nil + } + for _, kv := range extras { + key, val, ok := strings.Cut(kv, "=") + if !ok || key == "" { + return fmt.Errorf("invalid --query value %q: expected key=value", kv) + } + for _, qf := range queryFlags { + // Match the way flags themselves match: any spelling differing only + // in case or dash/underscore placement is the same parameter, under + // either the spec's name or the flag it was registered as. + if flagLookupKey(key) != flagLookupKey(qf.Param.Name) && flagLookupKey(key) != flagLookupKey(qf.Name) { + continue + } + if cmd.Flags().Changed(qf.Name) { + return fmt.Errorf("--query %s=... conflicts with --%s; set that parameter one way or the other", key, qf.Name) + } + } + query.Add(key, val) + } + return nil +} + +// checkRequiredQueryParams verifies every spec-required query param ended up in +// the query string with a non-empty value. MarkFlagRequired only proves the flag +// was supplied, so `--q=` (or `--query q=`) would otherwise pass validation and +// then be dropped as empty — exactly the server 400 round trip this is meant to +// avoid. A value under either the spec spelling or the flag spelling counts, +// mirroring how --query conflicts are detected. +func checkRequiredQueryParams(queryFlags []queryFlag, query url.Values) error { + var missing []string + for _, qf := range queryFlags { + if !qf.Param.Required { + continue + } + if hasNonEmptyValue(query[qf.Param.Name]) || hasNonEmptyValue(query[qf.Name]) { + continue + } + missing = append(missing, `"`+qf.Name+`"`) + } + if len(missing) > 0 { + return fmt.Errorf("required flag(s) %s cannot be empty", strings.Join(missing, ", ")) + } + return nil +} + +func hasNonEmptyValue(vals []string) bool { + for _, v := range vals { + if v != "" { + return true + } + } + return false +} + // schemaRequested reports whether the schema discovery flag — registered under // name, which is not always "schema" (see RegisterSchemaFlag) — is set. func schemaRequested(cmd *cobra.Command, name string) bool { diff --git a/internal/openapi/generate_test.go b/internal/openapi/generate_test.go index b8ce76b..0dc0a0c 100644 --- a/internal/openapi/generate_test.go +++ b/internal/openapi/generate_test.go @@ -1,6 +1,7 @@ package openapi import ( + "bytes" "encoding/json" "fmt" "os" @@ -11,6 +12,7 @@ import ( "github.com/pb33f/libopenapi" v3 "github.com/pb33f/libopenapi/datamodel/high/v3" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) // --------------------------------------------------------------------------- @@ -358,6 +360,246 @@ func TestBuildCommand_QueryFlags(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Required query params + the generic --query escape hatch +// --------------------------------------------------------------------------- + +// listOp is a GET operation with one required query param and one optional one. +func listOp() *operationInfo { + return &operationInfo{ + Tag: "test", + OperationID: "testListItems", + Method: "GET", + Path: "/api/v1/items", + QueryParams: []paramInfo{ + {Name: "connectionId", In: "query", Required: true}, + {Name: "page_size", In: "query"}, + }, + } +} + +// A query param the spec marks required must fail client-side when it's +// missing, instead of costing a round trip and a server 400. +func TestBuildCommand_RequiredQueryParamMissing(t *testing.T) { + called := false + exec := func(req APIRequest) error { called = true; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--page-size", "10"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected an error for a missing required query param, got nil") + } + if !strings.Contains(err.Error(), "connection-id") { + t.Errorf("error = %q, want it to name the missing flag", err.Error()) + } + if called { + t.Error("executor should not run when a required flag is missing") + } + + // The flag's usage should advertise that it's required. + usage := cmd.Flags().Lookup("connection-id").Usage + if !strings.Contains(usage, "(required)") { + t.Errorf("usage = %q, want it to mention (required)", usage) + } +} + +func TestBuildCommand_RequiredQueryParamPresent(t *testing.T) { + var captured APIRequest + exec := func(req APIRequest) error { captured = req; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SetArgs([]string{"--connectionid", "c-1"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.Contains(captured.Path, "connectionId=c-1") { + t.Errorf("path %q missing connectionId=c-1", captured.Path) + } +} + +// --query is the escape hatch for params the spec doesn't declare. It's +// repeatable, and repeating a key sends every value. +func TestBuildCommand_ExtraQueryParams(t *testing.T) { + var captured APIRequest + exec := func(req APIRequest) error { captured = req; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SetArgs([]string{ + "--connectionid", "c-1", + "--query", "undeclared=yes", + "--query", "tag=a", + "--query", "tag=b", + "--query", "empty=", + }) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + for _, want := range []string{"connectionId=c-1", "undeclared=yes", "tag=a&tag=b", "empty="} { + if !strings.Contains(captured.Path, want) { + t.Errorf("path %q missing %q", captured.Path, want) + } + } +} + +func TestBuildCommand_ExtraQueryParamMalformed(t *testing.T) { + exec := func(req APIRequest) error { return nil } + + for _, bad := range []string{"noequals", "=novalue"} { + cmd := buildCommand(listOp(), exec) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--connectionid", "c-1", "--query", bad}) + + err := cmd.Execute() + if err == nil { + t.Fatalf("--query %q: expected an error, got nil", bad) + } + if !strings.Contains(err.Error(), "key=value") { + t.Errorf("--query %q: error = %q, want it to explain key=value", bad, err.Error()) + } + } +} + +// Sending the same param both ways is ambiguous — error rather than silently +// picking one. Both the spec spelling and the flag spelling are caught. +func TestBuildCommand_ExtraQueryParamConflict(t *testing.T) { + for _, key := range []string{"connectionId", "connectionid", "connection-id"} { + called := false + exec := func(req APIRequest) error { called = true; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--connectionid", "c-1", "--query", key + "=c-2"}) + + err := cmd.Execute() + if err == nil { + t.Fatalf("--query %s=: expected a conflict error, got nil", key) + } + if !strings.Contains(err.Error(), "conflicts with --connection-id") { + t.Errorf("error = %q, want it to name the conflicting flag", err.Error()) + } + if called { + t.Error("executor should not run on a conflicting --query") + } + } +} + +// An unset declared flag isn't a conflict — --query can supply its value. +func TestBuildCommand_ExtraQueryParamNoConflictWhenFlagUnset(t *testing.T) { + var captured APIRequest + exec := func(req APIRequest) error { captured = req; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SetArgs([]string{"--connectionid", "c-1", "--query", "page_size=25"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.Contains(captured.Path, "page_size=25") { + t.Errorf("path %q missing page_size=25", captured.Path) + } +} + +// A spec param that slugifies to "query" owns the flag name; the escape hatch +// steps aside rather than panicking the flag registration. +func TestBuildCommand_QueryParamNamedQuery(t *testing.T) { + var captured APIRequest + exec := func(req APIRequest) error { captured = req; return nil } + + op := &operationInfo{ + Tag: "test", + OperationID: "testSearch", + Method: "GET", + Path: "/api/v1/search", + QueryParams: []paramInfo{{Name: "query", In: "query"}}, + } + + cmd := buildCommand(op, exec) + cmd.SetArgs([]string{"--query", "revenue"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.Contains(captured.Path, "query=revenue") { + t.Errorf("path %q missing query=revenue", captured.Path) + } +} + +// --schema is local discovery with no API call, so it must stay zero-friction +// even on an operation with required query params. +func TestBuildCommand_SchemaIgnoresRequiredQueryParams(t *testing.T) { + spec := `{ + "openapi": "3.1.0", + "info": {"title": "test", "version": "1.0"}, + "paths": { + "/api/v1/widgets": { + "post": { + "operationId": "widgetsCreate", + "tags": ["widgets"], + "parameters": [ + {"name": "connectionId", "in": "query", "required": true, "schema": {"type": "string"}} + ], + "requestBody": { + "content": {"application/json": {"schema": { + "type": "object", + "required": ["name"], + "properties": {"name": {"type": "string"}} + }}} + }, + "responses": {"200": {"description": "ok"}} + } + } + } + }` + + called := false + // Each case gets a fresh command tree; cobra keeps parsed flag state on the + // command, and the real CLI runs one command per process. + run := func(args ...string) (string, error) { + exec := func(req APIRequest) error { called = true; return nil } + cmds, err := GenerateCommands([]byte(spec), exec) + if err != nil { + t.Fatalf("GenerateCommands: %v", err) + } + group := cmds[0] + var buf bytes.Buffer + group.SetOut(&buf) + group.SetErr(&buf) + group.SilenceUsage = true + group.SilenceErrors = true + group.SetArgs(args) + execErr := group.Execute() + return buf.String(), execErr + } + + out, err := run("create", "--schema") + if err != nil { + t.Fatalf("Execute --schema: %v\n%s", err, out) + } + if called { + t.Error("--schema must not make an API call") + } + if !strings.Contains(out, `"name"`) { + t.Errorf("schema output missing body fields: %s", out) + } + + // Without --schema the required param is still enforced. + if _, err := run("create", "--body", "{}"); err == nil { + t.Fatal("expected a missing-required-flag error without --schema") + } + + // ...and supplying it goes through. + if out, err := run("create", "--body", "{}", "--connectionid", "c-1"); err != nil { + t.Fatalf("Execute with required param: %v\n%s", err, out) + } + if !called { + t.Error("expected the API call to run once the required param was set") + } +} + // 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) { @@ -1283,6 +1525,24 @@ func TestSpecCoverage(t *testing.T) { } } + // Query params the spec marks required are enforced client-side, so + // give each one a dummy value — otherwise the command could never + // reach RunE and the operation would read as uncovered. + setErr := "" + sub.Flags().VisitAll(func(f *pflag.Flag) { + ann := f.Annotations[cobra.BashCompOneRequiredFlag] + if setErr != "" || len(ann) == 0 || ann[0] != "true" { + return + } + if err := sub.Flags().Set(f.Name, "test-value"); err != nil { + setErr = fmt.Sprintf("%s: set %s flag: %v", key, f.Name, err) + } + }) + if setErr != "" { + failures = append(failures, setErr) + continue + } + if sub.RunE == nil { failures = append(failures, fmt.Sprintf("%s: no RunE", key)) continue From f26bdd33974eccba646bbefbc5d53a31ac5651b9 Mon Sep 17 00:00:00 2001 From: Daniel Spangenberger Date: Mon, 24 Aug 2026 12:17:06 -0400 Subject: [PATCH 2/2] fix(openapi): reject empty values for required query params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MarkFlagRequired only proves a flag was supplied, so `omni content search --q=` passed local validation and then had the empty value dropped when the query string was built — the request went out with no ?q= at all and earned exactly the server 400 this feature was meant to eliminate. Required query params are now validated against the assembled query string at the start of RunE, after --query extras are merged, so every path into the parameter is covered. A value under either the spec spelling or the flag spelling satisfies it, mirroring how --query conflicts are detected. TestSpecCoverage calls RunE directly and so bypasses cobra's own required-flag check; it now sets a dummy value for each required query param, which is what surfaced the five real spec operations affected (content search, ai-eval runs-list, models dbt-sync, models yaml-delete, query wait). Constraint: the check must run after --query merging, or the escape hatch could supply a value the validator can't see Rejected: reject empty strings at flag-parse time via a custom pflag Value | would also block legitimately empty optional params, which the API accepts Rejected: send required params even when empty | trades a clear local error for an opaque server 400, the exact failure being fixed Confidence: high Scope-risk: narrow Directive: keep checkRequiredQueryParams downstream of applyExtraQueryParams; ordering is load-bearing Not-tested: servers that treat an explicitly empty required param as valid Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s --- internal/openapi/generate.go | 11 ++- internal/openapi/generate_test.go | 135 ++++++++++++++++++++++++++---- 2 files changed, 131 insertions(+), 15 deletions(-) diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index 07ffce7..ccc72ed 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -470,12 +470,21 @@ func applyExtraQueryParams(cmd *cobra.Command, queryFlags []queryFlag, query url // avoid. A value under either the spec spelling or the flag spelling counts, // mirroring how --query conflicts are detected. func checkRequiredQueryParams(queryFlags []queryFlag, query url.Values) error { + // Index the assembled query by the same normalized key flags match on, so a + // value that arrived through --query counts no matter which spelling of the + // param the user typed. + byKey := map[string][]string{} + for k, vals := range query { + key := flagLookupKey(k) + byKey[key] = append(byKey[key], vals...) + } + var missing []string for _, qf := range queryFlags { if !qf.Param.Required { continue } - if hasNonEmptyValue(query[qf.Param.Name]) || hasNonEmptyValue(query[qf.Name]) { + if hasNonEmptyValue(byKey[flagLookupKey(qf.Param.Name)]) || hasNonEmptyValue(byKey[flagLookupKey(qf.Name)]) { continue } missing = append(missing, `"`+qf.Name+`"`) diff --git a/internal/openapi/generate_test.go b/internal/openapi/generate_test.go index 0dc0a0c..fb7215b 100644 --- a/internal/openapi/generate_test.go +++ b/internal/openapi/generate_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "net/url" "os" "sort" "strings" @@ -421,6 +422,96 @@ func TestBuildCommand_RequiredQueryParamPresent(t *testing.T) { } } +// MarkFlagRequired only proves the flag was supplied, so an explicit empty +// value (--connectionid=) would otherwise sail past validation and then be +// dropped from the query string — the server 400 this is meant to prevent. +func TestBuildCommand_RequiredQueryParamEmpty(t *testing.T) { + cases := [][]string{ + {"--connectionid="}, // explicit empty declared flag + {"--connectionid", ""}, // same, separate-arg form + {"--connectionid=", "--page-size", "10"}, // empty alongside other params + } + + for _, args := range cases { + called := false + exec := func(req APIRequest) error { called = true; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs(args) + + err := cmd.Execute() + if err == nil { + t.Fatalf("args %v: expected an error for an empty required param, got nil", args) + } + if !strings.Contains(err.Error(), "connection-id") { + t.Errorf("args %v: error = %q, want it to name the flag", args, err.Error()) + } + if called { + t.Errorf("args %v: executor should not run for an empty required param", args) + } + } +} + +// The emptiness check runs on the assembled query string, so it also covers +// values that arrived through the --query escape hatch (under either the spec +// spelling or the flag spelling). +func TestCheckRequiredQueryParams(t *testing.T) { + op := listOp() + cases := []struct { + name string + query url.Values + wantErr bool + }{ + {"absent", url.Values{}, true}, + {"empty under spec name", url.Values{"connectionId": {""}}, true}, + {"empty under flag name", url.Values{"connectionid": {""}}, true}, + {"set under spec name", url.Values{"connectionId": {"c-1"}}, false}, + {"set under flag name", url.Values{"connectionid": {"c-1"}}, false}, + {"one of several non-empty", url.Values{"connectionId": {"", "c-1"}}, false}, + {"optional param empty is fine", url.Values{"connectionId": {"c-1"}, "page_size": {""}}, false}, + } + + for _, c := range cases { + err := checkRequiredQueryParams(resolveQueryFlags(op), c.query) + if c.wantErr { + if err == nil { + t.Errorf("%s: expected an error, got nil", c.name) + } else if !strings.Contains(err.Error(), "connection-id") { + t.Errorf("%s: error = %q, want it to name the flag", c.name, err.Error()) + } + continue + } + if err != nil { + t.Errorf("%s: unexpected error: %v", c.name, err) + } + } +} + +// A declared required param has its own flag, so --query is not a substitute +// for it: cobra still insists the flag itself be supplied. +func TestBuildCommand_ExtraQueryDoesNotSubstituteForRequiredFlag(t *testing.T) { + called := false + exec := func(req APIRequest) error { called = true; return nil } + + cmd := buildCommand(listOp(), exec) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs([]string{"--query", "connectionId=c-1"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected the required flag to still be enforced, got nil") + } + if !strings.Contains(err.Error(), "connection-id") { + t.Errorf("error = %q, want it to name the missing flag", err.Error()) + } + if called { + t.Error("executor should not run when the required flag is missing") + } +} + // --query is the escape hatch for params the spec doesn't declare. It's // repeatable, and repeating a key sends every value. func TestBuildCommand_ExtraQueryParams(t *testing.T) { @@ -1394,12 +1485,13 @@ func TestRealSpec_NoFlagCollisions(t *testing.T) { // specOperation holds data we parse from the spec independently of the // command generator, so we can cross-reference what the generator produced. type specOperation struct { - Tag string - OperationID string - Method string - Path string - PathParams []string - HasBody bool + Tag string + OperationID string + Method string + Path string + PathParams []string + RequiredQuery []string // flag names of required query params + HasBody bool } // parseSpecOperations reads the OpenAPI spec directly (bypassing our generator) @@ -1443,20 +1535,26 @@ func parseSpecOperations(t *testing.T, specData []byte) []specOperation { tag = op.Tags[0] } - var pathParams []string + var pathParams, requiredQuery []string for _, p := range op.Parameters { - if p.In == "path" { + switch p.In { + case "path": pathParams = append(pathParams, p.Name) + case "query": + if boolVal(p.Required) { + requiredQuery = append(requiredQuery, slugify(p.Name)) + } } } ops = append(ops, specOperation{ - Tag: tag, - OperationID: op.OperationId, - Method: method, - Path: pathStr, - PathParams: pathParams, - HasBody: op.RequestBody != nil, + Tag: tag, + OperationID: op.OperationId, + Method: method, + Path: pathStr, + PathParams: pathParams, + RequiredQuery: requiredQuery, + HasBody: op.RequestBody != nil, }) } } @@ -1516,6 +1614,15 @@ func TestSpecCoverage(t *testing.T) { args[i] = "test-id" } + // Required query params must carry a non-empty value, same as they + // would on a real invocation. + for _, flagName := range sop.RequiredQuery { + if err := sub.Flags().Set(flagName, "test-value"); err != nil { + failures = append(failures, fmt.Sprintf("%s: set required query flag %s: %v", key, flagName, err)) + continue + } + } + // Operations with a request body (POST/PUT/PATCH) need --body set, // otherwise the command would try to read from stdin. if sop.HasBody {