diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 0f72007..1e86695 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -186,6 +186,26 @@ func Run(ctx context.Context, opts *Options) error { } } + // C12: -f/-F on GET should produce query-string params, not a + // JSON body. Pre-D3b the body got attached to GET and the server + // rejected with a confusing JSON-parse error. gh-compat: GET + // fields land on the URL. + if strings.EqualFold(method, http.MethodGet) && body != nil && isJSONBody { + qs, qerr := queryStringFromFields(opts.Fields, opts.RawFields, opts.IO.In) + if qerr != nil { + return qerr + } + if qs != "" { + if strings.Contains(endpoint, "?") { + endpoint += "&" + qs + } else { + endpoint += "?" + qs + } + } + body = nil + isJSONBody = false + } + headers, err := buildHeaders(opts, isJSONBody) if err != nil { return err @@ -411,7 +431,11 @@ func runJQ(out io.Writer, expr string, body []byte) error { // value runs into the next shell prompt or pipeline reader's line // boundary. We buffer the template output, then add `\n` iff missing. func runTemplate(out io.Writer, tmpl string, body []byte) error { - t, err := template.New("api").Parse(tmpl) + // C13: missingkey=error means `{{.totally_missing}}` returns a + // template-execution error instead of printing the literal string + // "". Scripts piping into `xargs` or `read` no longer + // silently consume an unintended marker. + t, err := template.New("api").Option("missingkey=error").Parse(tmpl) if err != nil { return fmt.Errorf("api: parse template: %w", err) } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 83c7742..f69641f 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -149,6 +149,72 @@ func TestRunTemplateExecution(t *testing.T) { } } +// TestRunTemplateRejectsMissingKey covers C13: missing keys must +// surface as a template-execution error, never as the literal string +// "". Scripts piping `api -t '{{.field}}'` into `read` were +// silently consuming the marker. +func TestRunTemplateRejectsMissingKey(t *testing.T) { + opts, tf := newOpts(t) + opts.Endpoint = "u" + opts.Template = `{{.totally_missing_field}}` + tf.Server.RegisterJSON("GET", "/api/v1/u", 200, map[string]any{"id": 1}) + + err := Run(context.Background(), opts) + if err == nil { + t.Fatal("expected error on missing template field") + } + if strings.Contains(tf.Out.String(), "") { + t.Errorf("C13 regression: output contains '': %q", tf.Out.String()) + } +} + +// TestRunGETWithFieldsBecomesQueryString covers C12: -f/-F on GET +// emits URL query params, not a body. Pre-D3b the server got a body +// on GET and responded with a confusing parse error. +func TestRunGETWithFieldsBecomesQueryString(t *testing.T) { + opts, tf := newOpts(t) + opts.Endpoint = "search" + opts.Method = "GET" + opts.Fields = []string{"q=hello", "sort=stars"} + var seenQuery string + tf.Server.Handle("GET", "/api/v1/search", func(w http.ResponseWriter, r *http.Request) { + seenQuery = r.URL.RawQuery + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ok":true}`)) + }) + + if err := Run(context.Background(), opts); err != nil { + t.Fatalf("Run: %v", err) + } + // Order-insensitive substring check: both params must appear. + for _, want := range []string{"q=hello", "sort=stars"} { + if !strings.Contains(seenQuery, want) { + t.Errorf("query missing %q; got: %s", want, seenQuery) + } + } +} + +// TestRunPaginateRejectsNonArrayResponse covers C11: --paginate +// against an endpoint returning a single object must error, not +// silently wrap the object in a one-element array. +func TestRunPaginateRejectsNonArrayResponse(t *testing.T) { + opts, tf := newOpts(t) + opts.Endpoint = "user" + opts.Paginate = true + tf.Server.Handle("GET", "/api/v1/user", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":1,"login":"mf"}`)) + }) + + err := Run(context.Background(), opts) + if err == nil { + t.Fatal("expected error for --paginate on object endpoint") + } + if !strings.Contains(err.Error(), "JSON array") { + t.Errorf("error should mention JSON array; got: %v", err) + } +} + func TestRunMutexJQAndTemplate(t *testing.T) { opts, _ := newOpts(t) opts.Endpoint = "u" diff --git a/pkg/cmd/api/fields.go b/pkg/cmd/api/fields.go index 998834a..fe242ce 100644 --- a/pkg/cmd/api/fields.go +++ b/pkg/cmd/api/fields.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "net/url" "os" "strconv" "strings" @@ -31,6 +32,39 @@ func parseFieldFlag(arg string) (key, value string, err error) { return arg[:idx], arg[idx+1:], nil } +// queryStringFromFields encodes -f/-F flags as URL query parameters for +// the GET-with-fields case (C-audit C12). Values flow through the same +// expansion as the body path: -F runs bool/number/@file detection, -f +// keeps strings verbatim. Order is preserved for stable URL shape. +func queryStringFromFields(typed, rawFields []string, stdin io.Reader) (string, error) { + entries, err := fieldEntriesFromFlags(typed, rawFields) + if err != nil { + return "", err + } + if len(entries) == 0 { + return "", nil + } + parts := make([]string, 0, len(entries)) + for _, e := range entries { + val := e.value + if e.raw { + val = e.value + } else { + // Typed (-F) supports @file; expand it just like the body + // path does so users don't have to switch to -f on GET. + if strings.HasPrefix(e.value, "@") { + data, ferr := readInput(strings.TrimPrefix(e.value, "@"), stdin) + if ferr != nil { + return "", ferr + } + val = string(data) + } + } + parts = append(parts, url.QueryEscape(e.key)+"="+url.QueryEscape(val)) + } + return strings.Join(parts, "&"), nil +} + // fieldEntriesFromFlags merges the -F and -f flag slices preserving the // caller's order, which becomes the JSON object insertion order. Errors // fail fast — one bad field aborts the whole call. diff --git a/pkg/cmd/api/pagination.go b/pkg/cmd/api/pagination.go index 25dca46..1480e0b 100644 --- a/pkg/cmd/api/pagination.go +++ b/pkg/cmd/api/pagination.go @@ -46,23 +46,37 @@ func runPaginated(ctx context.Context, opts *Options, client *api.Client, method concat := []json.RawMessage{} pages := []json.RawMessage{} + firstPage := true for raw, err := range client.DoPaginated(ctx, method, endpoint, reqOpts...) { if err != nil { return err } if opts.Slurp { pages = append(pages, raw) + firstPage = false continue } - // Try to extend the running concat slice with array elements; if - // the body isn't an array, fall back to emitting it as a single - // item so we never silently drop a page. + // C11: reject --paginate against a non-array endpoint. The + // pre-D3b behavior wrapped a single object in [obj] silently, + // which fooled scripts into believing they were paginating + // over a list (e.g., `api repos/o/r --paginate | jq '.[]'` + // emitted exactly one element no matter what). Surface the + // shape mismatch as soon as we see the first page; subsequent + // pages are an array by construction. var arr []json.RawMessage - if err := json.Unmarshal(raw, &arr); err == nil { - concat = append(concat, arr...) + if err := json.Unmarshal(raw, &arr); err != nil { + if firstPage { + return fmt.Errorf("api: --paginate requires a JSON array response; %s did not return one", endpoint) + } + // Defensive: a mid-stream non-array would be a server bug. + // Keep the legacy behavior (pass through as a single item) + // so we don't drop data; trust the first-page check above. + concat = append(concat, raw) + firstPage = false continue } - concat = append(concat, raw) + concat = append(concat, arr...) + firstPage = false } var assembled []byte