mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 22:10:25 +00:00
fix(cli): pad item list --format json pages to completion when --limit is not set (BUG-4024) (#2279)
* fix(cli): pad item list --format json pages to completion when --limit is not set (BUG-4024) `pad item list --format json` stopped at the CLI's own 200 default and said so on stderr only, so a consumer parsing the array saw 200 of 204 rows with no signal in the payload. Ruling: with --format json and no --limit (cobra Changed, not a value compare) the CLI follows X-Pad-Next-Offset to the end, sends no limit and prints no notice. The payload stays a plain array; no API, schema or MCP change. An explicit --limit, including --limit 0, keeps today's behaviour and its stderr notice. --limit 0 has always meant "the default" (clamped to 200), not "no limit"; a test pins that. Table and markdown are untouched. --offset with no --limit pages from that offset to the end. One private pager (listAllPages) now backs ListAllItems and ListAllCollectionItems (the old names stay as wrappers) and new *Report variants taking a start offset and returning the first page's undeclared filter keys, so the BUG-3480 warning still fires. A non-advancing next offset, or more than 1000 pages, is an error and a non-zero exit with a message naming the cap, never a partial array. This also applies to the existing callers (reconcile, github status, project standup), which used to return a short list for the non-advancing case. Claude-Session: https://claude.ai/code/session_013aLLQx2EARjEzmWHbExjtm * fix(cli): the list pager requires has-more pages to be full and contiguous (BUG-4024 R2-F1) listAllPages accepted any next offset greater than the current one, so an empty has-more page, or a next offset that skipped rows never returned, followed by a has-more=false page, ended the loop on a partial list with no error. The server sets has-more only when it trimmed the probe row (handlers_items.go listItemsPage), so such a page holds exactly `limit` rows and next-offset == offset+len(items) with len(items) > 0. The pager now requires exactly that and otherwise returns an error with nil items. The check subsumes the old `NextOffset <= offset` test. Tests: empty page (advancing and non-advancing next offset), a jump, and one short, at the client level; the jump at the `pad item list --format json` level (non-zero exit, empty stdout). Claude-Session: https://claude.ai/code/session_013aLLQx2EARjEzmWHbExjtm * fix(cli): the list pager reads a negative start offset as 0, as the server does (BUG-4024 R3-F1) Claude-Session: https://claude.ai/code/session_01UiMM6MkrYr1nkuaNkSmkeD
This commit is contained in:
@@ -0,0 +1,534 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"os"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/cli"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
padserver "github.com/PerpetualSoftware/pad/internal/server"
|
||||
"github.com/PerpetualSoftware/pad/internal/store"
|
||||
"github.com/PerpetualSoftware/pad/internal/store/storetest"
|
||||
)
|
||||
|
||||
// BUG-4024: `pad item list --format json` with no --limit used to stop at the
|
||||
// CLI's own 200 default and say so on stderr only, so a consumer parsing the
|
||||
// array saw 200 of 204 rows and no signal. It now pages to completion.
|
||||
|
||||
// pagedItemServer is a fake of the list endpoints: total rows, at most
|
||||
// serverPage per response, X-Pad-Has-More / X-Pad-Next-Offset like the real
|
||||
// server. It records each request's raw query. Rows are ids "it-<n>".
|
||||
type pagedItemServer struct {
|
||||
mu sync.Mutex
|
||||
total int
|
||||
serverPage int
|
||||
noHeaders bool // an older server: no paging state at all
|
||||
foreverAt int // if >0, always claim more (next offset = offset+foreverAt)
|
||||
jump bool // page one claims a next offset past its rows, then the end
|
||||
stuck bool // claim more but never advance the next offset
|
||||
undeclared string // X-Pad-Undeclared-Filter-Keys, first page only
|
||||
withCode bool // rows carry a code context (what reconcile keeps)
|
||||
queries []string
|
||||
}
|
||||
|
||||
func (p *pagedItemServer) handler() http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if !strings.HasSuffix(r.URL.Path, "/items") {
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
p.mu.Lock()
|
||||
defer p.mu.Unlock()
|
||||
p.queries = append(p.queries, r.URL.RawQuery)
|
||||
from, _ := strconv.Atoi(r.URL.Query().Get("offset"))
|
||||
size := p.serverPage
|
||||
if l, err := strconv.Atoi(r.URL.Query().Get("limit")); err == nil && l > 0 && l < size {
|
||||
size = l
|
||||
}
|
||||
if p.noHeaders {
|
||||
size = p.total
|
||||
}
|
||||
to := min(from+size, p.total)
|
||||
if p.undeclared != "" && from == 0 {
|
||||
w.Header().Set("X-Pad-Undeclared-Filter-Keys", p.undeclared)
|
||||
}
|
||||
switch {
|
||||
case p.noHeaders:
|
||||
case p.jump:
|
||||
if from == 0 {
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(to+size))
|
||||
} else {
|
||||
w.Header().Set("X-Pad-Has-More", "false")
|
||||
}
|
||||
case p.stuck:
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(from))
|
||||
case p.foreverAt > 0:
|
||||
to = from + p.foreverAt
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(to))
|
||||
case to < p.total:
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(to))
|
||||
default:
|
||||
w.Header().Set("X-Pad-Has-More", "false")
|
||||
}
|
||||
items := []models.Item{}
|
||||
for i := from; i < to; i++ {
|
||||
it := models.Item{ID: fmt.Sprintf("it-%d", i), Title: fmt.Sprintf("Item %d", i), CollectionSlug: "tasks"}
|
||||
if p.withCode {
|
||||
it.CodeContext = &models.ItemCodeContext{Provider: "github"}
|
||||
}
|
||||
items = append(items, it)
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(items)
|
||||
})
|
||||
}
|
||||
|
||||
func (p *pagedItemServer) queryValues(t *testing.T) []string {
|
||||
t.Helper()
|
||||
p.mu.Lock()
|
||||
defer p.mu.Unlock()
|
||||
return append([]string(nil), p.queries...)
|
||||
}
|
||||
|
||||
// runItemList runs `pad item list <args>` in the given format and returns
|
||||
// stdout, stderr and the command's error.
|
||||
func runItemList(t *testing.T, format string, args ...string) (string, string, error) {
|
||||
t.Helper()
|
||||
formatFlag = format
|
||||
cmd := listCmd()
|
||||
cmd.SilenceUsage, cmd.SilenceErrors = true, true
|
||||
cmd.SetArgs(args)
|
||||
var err error
|
||||
var stdout string
|
||||
stderr := captureStderr(t, func() {
|
||||
stdout = captureStdoutDraining(t, func() { err = cmd.Execute() })
|
||||
})
|
||||
return stdout, stderr, err
|
||||
}
|
||||
|
||||
// captureStdoutDraining is captureStdout for output bigger than a pipe buffer
|
||||
// (64 KiB): a few thousand JSON rows block the writer if nothing reads until
|
||||
// fn returns.
|
||||
func captureStdoutDraining(t *testing.T, fn func()) string {
|
||||
t.Helper()
|
||||
orig := os.Stdout
|
||||
r, w, err := os.Pipe()
|
||||
if err != nil {
|
||||
t.Fatalf("os.Pipe: %v", err)
|
||||
}
|
||||
os.Stdout = w
|
||||
done := make(chan string, 1)
|
||||
go func() {
|
||||
b, _ := io.ReadAll(r)
|
||||
done <- string(b)
|
||||
}()
|
||||
fn()
|
||||
os.Stdout = orig
|
||||
_ = w.Close()
|
||||
out := <-done
|
||||
_ = r.Close()
|
||||
return out
|
||||
}
|
||||
|
||||
func decodeSummaries(t *testing.T, out string) []map[string]any {
|
||||
t.Helper()
|
||||
var rows []map[string]any
|
||||
if err := json.Unmarshal([]byte(out), &rows); err != nil {
|
||||
t.Fatalf("stdout is not a JSON array: %v\n%.300s", err, out)
|
||||
}
|
||||
return rows
|
||||
}
|
||||
|
||||
func TestBUG4024_JSONListPagesToCompletionWithoutALimit(t *testing.T) {
|
||||
p := &pagedItemServer{total: 2350, serverPage: 1000}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, errOut, err := runItemList(t, "json")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 2350 {
|
||||
t.Fatalf("JSON array has %d rows, want all 2350", n)
|
||||
}
|
||||
for _, q := range p.queryValues(t) {
|
||||
if strings.Contains(q, "limit=") {
|
||||
t.Fatalf("auto-page sent a limit: %q", q)
|
||||
}
|
||||
}
|
||||
if got := len(p.queryValues(t)); got != 3 {
|
||||
t.Fatalf("%d requests, want 3 pages", got)
|
||||
}
|
||||
if strings.Contains(errOut, "more exist") || strings.Contains(errOut, "showing") {
|
||||
t.Fatalf("auto-page printed a truncation notice: %q", errOut)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_JSONListAcrossTheDefaultPageBoundary(t *testing.T) {
|
||||
p := &pagedItemServer{total: 205, serverPage: 1000}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, _, err := runItemList(t, "json")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 205 {
|
||||
t.Fatalf("got %d rows, want 205", n)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_ExplicitLimitStaysCappedWithTheNotice(t *testing.T) {
|
||||
p := &pagedItemServer{total: 50, serverPage: 1000}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, errOut, err := runItemList(t, "json", "--limit", "5")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 5 {
|
||||
t.Fatalf("got %d rows, want the explicit cap of 5", n)
|
||||
}
|
||||
if qs := p.queryValues(t); len(qs) != 1 || !strings.Contains(qs[0], "limit=5") {
|
||||
t.Fatalf("requests %q, want one with limit=5", qs)
|
||||
}
|
||||
if !strings.Contains(errOut, "more exist") {
|
||||
t.Fatalf("explicit --limit lost its stderr notice: %q", errOut)
|
||||
}
|
||||
}
|
||||
|
||||
// What --limit 0 means today (BUG-4024 Q1), pinned: it is "the default", so
|
||||
// the CLI clamps it to defaultItemListLimit (200) and sends limit=200, one
|
||||
// page, with the notice. It is NOT "no limit" and does not auto-page.
|
||||
func TestBUG4024_LimitZeroMeansTheDefaultCapNotUnlimited(t *testing.T) {
|
||||
p := &pagedItemServer{total: 450, serverPage: 1000}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, errOut, err := runItemList(t, "json", "--limit", "0")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != defaultItemListLimit {
|
||||
t.Fatalf("got %d rows, want the default cap %d", n, defaultItemListLimit)
|
||||
}
|
||||
if qs := p.queryValues(t); len(qs) != 1 || !strings.Contains(qs[0], "limit=200") {
|
||||
t.Fatalf("requests %q, want one with limit=200", qs)
|
||||
}
|
||||
if !strings.Contains(errOut, "more exist") {
|
||||
t.Fatalf("--limit 0 lost its stderr notice: %q", errOut)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_OlderServerWithoutPagingHeadersIsOneRequest(t *testing.T) {
|
||||
p := &pagedItemServer{total: 7, serverPage: 3, noHeaders: true}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, _, err := runItemList(t, "json")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 7 {
|
||||
t.Fatalf("got %d rows, want 7", n)
|
||||
}
|
||||
if qs := p.queryValues(t); len(qs) != 1 || strings.Contains(qs[0], "limit=") {
|
||||
t.Fatalf("requests %q, want one without a limit", qs)
|
||||
}
|
||||
}
|
||||
|
||||
// An older server sends no paging state, and the unpaged notice falls back to
|
||||
// "len == limit" as its guess. On the auto-page path there is no limit, so a
|
||||
// list that happens to hold exactly 200 rows must not be reported as capped.
|
||||
func TestBUG4024_AutoPageNeverPrintsTheCappedGuessOnAnOlderServer(t *testing.T) {
|
||||
p := &pagedItemServer{total: defaultItemListLimit, serverPage: 1000, noHeaders: true}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, errOut, err := runItemList(t, "json")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != defaultItemListLimit {
|
||||
t.Fatalf("got %d rows, want %d", n, defaultItemListLimit)
|
||||
}
|
||||
if strings.Contains(errOut, "capped") || strings.Contains(errOut, "showing") {
|
||||
t.Fatalf("auto-page printed a truncation notice: %q", errOut)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_OffsetWithoutALimitPagesFromThatOffsetToTheEnd(t *testing.T) {
|
||||
p := &pagedItemServer{total: 2350, serverPage: 1000}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, _, err := runItemList(t, "json", "--offset", "1500")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
rows := decodeSummaries(t, out)
|
||||
if len(rows) != 850 {
|
||||
t.Fatalf("got %d rows, want 850 (2350 - 1500)", len(rows))
|
||||
}
|
||||
if qs := p.queryValues(t); len(qs) != 1 || !strings.Contains(qs[0], "offset=1500") {
|
||||
t.Fatalf("requests %q, want one page starting at offset=1500", qs)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_NonAdvancingOffsetIsAnErrorNotAShortArray(t *testing.T) {
|
||||
p := &pagedItemServer{total: 500, serverPage: 100, stuck: true}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, _, err := runItemList(t, "json")
|
||||
if err == nil || !strings.Contains(err.Error(), "does not advance") {
|
||||
t.Fatalf("err = %v, want a does-not-advance error", err)
|
||||
}
|
||||
if strings.TrimSpace(out) != "" {
|
||||
t.Fatalf("a partial array reached stdout: %.200s", out)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_SkippingNextOffsetIsAnErrorNotAShortArray(t *testing.T) {
|
||||
p := &pagedItemServer{total: 500, serverPage: 100, jump: true}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, _, err := runItemList(t, "json")
|
||||
if err == nil || !strings.Contains(err.Error(), "rather than return a partial list") {
|
||||
t.Fatalf("err = %v, want a partial-list refusal", err)
|
||||
}
|
||||
if strings.TrimSpace(out) != "" {
|
||||
t.Fatalf("a partial array reached stdout: %.200s", out)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_PageCapIsANonZeroExitNamingTheCapNeverAPartialArray(t *testing.T) {
|
||||
defer cli.SetMaxListPages(3)()
|
||||
p := &pagedItemServer{total: 1 << 30, serverPage: 10, foreverAt: 10}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, _, err := runItemList(t, "json")
|
||||
if err == nil || !strings.Contains(err.Error(), "3-page cap") {
|
||||
t.Fatalf("err = %v, want an error naming the 3-page cap", err)
|
||||
}
|
||||
if strings.TrimSpace(out) != "" {
|
||||
t.Fatalf("a partial array reached stdout: %.200s", out)
|
||||
}
|
||||
if got := len(p.queryValues(t)); got != 3 {
|
||||
t.Fatalf("%d requests, want exactly the cap of 3", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_TableAndMarkdownKeepTheCapAndTheNotice(t *testing.T) {
|
||||
for _, format := range []string{"table", "markdown"} {
|
||||
t.Run(format, func(t *testing.T) {
|
||||
p := &pagedItemServer{total: 450, serverPage: 1000}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
_, errOut, err := runItemList(t, format)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if qs := p.queryValues(t); len(qs) != 1 || !strings.Contains(qs[0], "limit=200") {
|
||||
t.Fatalf("requests %q, want one with limit=200", qs)
|
||||
}
|
||||
if !strings.Contains(errOut, "more exist") {
|
||||
t.Fatalf("%s lost its stderr notice: %q", format, errOut)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_UndeclaredKeyWarningSurvivesPaging(t *testing.T) {
|
||||
p := &pagedItemServer{total: 25, serverPage: 10, undeclared: "legacy_key"}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, errOut, err := runItemList(t, "json")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 25 {
|
||||
t.Fatalf("got %d rows, want 25", n)
|
||||
}
|
||||
if strings.Count(errOut, "which no schema declares") != 1 || !strings.Contains(errOut, "legacy_key") {
|
||||
t.Fatalf("stderr %q, want the undeclared-key warning exactly once", errOut)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_CollectionListPagesAndKeepsTheUndeclaredWarning(t *testing.T) {
|
||||
p := &pagedItemServer{total: 25, serverPage: 10, undeclared: "legacy_key"}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
out, errOut, err := runItemList(t, "json", "tasks")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 25 {
|
||||
t.Fatalf("got %d rows, want 25", n)
|
||||
}
|
||||
if !strings.Contains(errOut, "legacy_key") {
|
||||
t.Fatalf("stderr %q lost the undeclared-key warning", errOut)
|
||||
}
|
||||
}
|
||||
|
||||
// The alias fallback still works on the auto-page path: an older server that
|
||||
// does not resolve `task` 404s it, and the retry as `tasks` pages.
|
||||
func TestBUG4024_CollectionAliasFallbackStillPagesToCompletion(t *testing.T) {
|
||||
p := &pagedItemServer{total: 25, serverPage: 10}
|
||||
inner := p.handler()
|
||||
setupFormatRoutingTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
switch {
|
||||
case r.URL.Path == "/api/v1/server/capabilities":
|
||||
_, _ = w.Write([]byte(`{"collection_resolution":false}`))
|
||||
case strings.Contains(r.URL.Path, "/collections/task/items"):
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
_, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"Collection not found"}}`))
|
||||
default:
|
||||
inner.ServeHTTP(w, r)
|
||||
}
|
||||
}))
|
||||
out, _, err := runItemList(t, "json", "task")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 25 {
|
||||
t.Fatalf("got %d rows, want 25", n)
|
||||
}
|
||||
}
|
||||
|
||||
// --- the existing ListAll* callers: normal multi-page input is unchanged ---
|
||||
// (reconcile and github status are also covered by bug3793_reconcile_pages_test.go.)
|
||||
|
||||
func TestBUG4024_CallerReconcileReadsEveryPage(t *testing.T) {
|
||||
p := &pagedItemServer{total: 7, serverPage: 3, withCode: true}
|
||||
setupFormatRoutingTest(t, p.handler())
|
||||
got, err := reconcileTargetItems(cli.NewClientFromURL(urlFlag), "demo", nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(got) != 7 {
|
||||
t.Fatalf("got %d items, want 7", len(got))
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_CallerGitHubStatusScansEveryPage(t *testing.T) {
|
||||
p := &pagedItemServer{total: 7, serverPage: 3}
|
||||
inner := p.handler()
|
||||
setupFormatRoutingTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if strings.HasSuffix(r.URL.Path, "/collections") {
|
||||
_ = json.NewEncoder(w).Encode([]models.Collection{{Slug: "tasks"}})
|
||||
return
|
||||
}
|
||||
inner.ServeHTTP(w, r)
|
||||
}))
|
||||
got, err := githubStatusScanItems(cli.NewClientFromURL(urlFlag), "demo")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(got) != 7 {
|
||||
t.Fatalf("got %d items, want 7", len(got))
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_CallerStandupReadsEveryInProgressPage(t *testing.T) {
|
||||
p := &pagedItemServer{total: 7, serverPage: 3}
|
||||
inner := p.handler()
|
||||
setupFormatRoutingTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
switch {
|
||||
case strings.HasSuffix(r.URL.Path, "/dashboard"):
|
||||
_, _ = w.Write([]byte(`{}`))
|
||||
case strings.HasSuffix(r.URL.Path, "/collections"):
|
||||
_, _ = w.Write([]byte(`[]`))
|
||||
default:
|
||||
inner.ServeHTTP(w, r)
|
||||
}
|
||||
}))
|
||||
formatFlag = "json"
|
||||
cmd := standupCmd()
|
||||
cmd.SilenceUsage, cmd.SilenceErrors = true, true
|
||||
cmd.SetArgs(nil)
|
||||
var err error
|
||||
out := captureStdout(t, func() { err = cmd.Execute() })
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
var got struct {
|
||||
InProgress []map[string]any `json:"in_progress"`
|
||||
}
|
||||
if jerr := json.Unmarshal([]byte(out), &got); jerr != nil {
|
||||
t.Fatalf("standup output: %v\n%.300s", jerr, out)
|
||||
}
|
||||
if len(got.InProgress) != 7 {
|
||||
t.Fatalf("in_progress has %d items, want 7", len(got.InProgress))
|
||||
}
|
||||
}
|
||||
|
||||
// --- a real server over the real store, on both backends ---
|
||||
// The fake server above proves the CLI follows the headers it is told about.
|
||||
// This proves the headers the real list endpoint sends are the ones the pager
|
||||
// follows: 205 rows is one past the CLI's old 200 default.
|
||||
|
||||
func TestBUG4024_RealServerJSONListReturnsEveryRowOnBothBackends(t *testing.T) {
|
||||
backends := []struct {
|
||||
name string
|
||||
open func(*testing.T) *store.Store
|
||||
}{
|
||||
{"SQLite", storetest.NewSQLite},
|
||||
{"Postgres", storetest.NewPostgres}, // skips unless PAD_TEST_POSTGRES_URL is set
|
||||
}
|
||||
const total = 205
|
||||
for _, b := range backends {
|
||||
t.Run(b.name, func(t *testing.T) {
|
||||
s := b.open(t)
|
||||
api := padserver.New(s)
|
||||
t.Cleanup(api.Stop)
|
||||
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Demo", Slug: "demo"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := s.SeedCollectionsFromTemplate(ws.ID, "startup"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
tasks, err := s.GetCollectionBySlug(ws.ID, "tasks")
|
||||
if err != nil || tasks == nil {
|
||||
t.Fatalf("tasks collection: %v", err)
|
||||
}
|
||||
for i := 0; i < total; i++ {
|
||||
if _, err := s.CreateItem(ws.ID, tasks.ID, models.ItemCreate{Title: fmt.Sprintf("Task %03d", i), Fields: `{"status":"open"}`}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
setupFormatRoutingTest(t, api)
|
||||
|
||||
out, errOut, err := runItemList(t, "json", "tasks")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
rows := decodeSummaries(t, out)
|
||||
if len(rows) != total {
|
||||
t.Fatalf("collection list: %d rows, want all %d (stderr %q)", len(rows), total, errOut)
|
||||
}
|
||||
seen := map[any]bool{}
|
||||
for _, r := range rows {
|
||||
seen[r["ref"]] = true
|
||||
}
|
||||
if len(seen) != total {
|
||||
t.Fatalf("collection list repeated rows: %d distinct refs of %d", len(seen), total)
|
||||
}
|
||||
|
||||
out, _, err = runItemList(t, "json")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// The workspace list also carries the template's seeded system
|
||||
// items, so it is at least the 205 tasks, each ref once.
|
||||
all := decodeSummaries(t, out)
|
||||
refs := map[any]bool{}
|
||||
for _, r := range all {
|
||||
refs[r["ref"]] = true
|
||||
}
|
||||
if len(all) < total || len(refs) != len(all) {
|
||||
t.Fatalf("workspace list: %d rows, %d distinct refs; want at least %d, no repeats", len(all), len(refs), total)
|
||||
}
|
||||
|
||||
// An explicit --limit is still the caller's cap.
|
||||
out, errOut, err = runItemList(t, "json", "tasks", "--limit", "50")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if n := len(decodeSummaries(t, out)); n != 50 || !strings.Contains(errOut, "more exist") {
|
||||
t.Fatalf("--limit 50: %d rows, stderr %q; want 50 rows and the notice", n, errOut)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
+37
-11
@@ -434,11 +434,13 @@ Run with --help-collections to see available collections and their status values
|
||||
// --- list ---
|
||||
|
||||
// Default + hard-max caps on `pad item list` result size (TASK-2000).
|
||||
// The default is a safety net so a bare `pad item list` / `--all` can't dump
|
||||
// the whole workspace into an agent's context; the max clamps an explicit
|
||||
// oversized `--limit`. Both bound the JSON/table payload, never the item body
|
||||
// (that's handled by the summary projection). Generous enough that normal
|
||||
// workspaces aren't truncated; small enough that pathological ones are.
|
||||
// The default is a safety net so a bare table/markdown `pad item list` /
|
||||
// `--all` can't dump the whole workspace into an agent's context; the max
|
||||
// clamps an explicit oversized `--limit`. Both bound the table payload, never
|
||||
// the item body (that's handled by the summary projection). `--format json`
|
||||
// WITHOUT `--limit` is not capped: a consumer parsing the array cannot see a
|
||||
// stderr notice, so it pages to completion instead (BUG-4024). An explicit
|
||||
// `--limit` (including 0, which means the default) keeps the cap.
|
||||
const (
|
||||
defaultItemListLimit = 200
|
||||
maxItemListLimit = 1000
|
||||
@@ -447,6 +449,7 @@ const (
|
||||
// printItemListTruncation tells a human, on stderr, that the list stopped
|
||||
// short and how to see the rest. The server says so exactly (BUG-3704); an
|
||||
// older server does not, and then len==limit is the hedged guess it always was.
|
||||
// The JSON auto-page path (BUG-4024) never calls it: nothing was cut short.
|
||||
func printItemListTruncation(page cli.ItemListPage, n, limit int) {
|
||||
switch {
|
||||
case page.Known && page.HasMore:
|
||||
@@ -543,10 +546,19 @@ in that collection are shown. Items with status "done" are hidden by default.`,
|
||||
if groupBy != "" {
|
||||
params.Set("group_by", groupBy)
|
||||
}
|
||||
// --format json with no --limit pages to completion (BUG-4024):
|
||||
// the array is the whole answer, since a caller parsing it never
|
||||
// sees the stderr notice. Detection is whether the flag was
|
||||
// passed, not its value, so an explicit --limit (even 0) keeps
|
||||
// the capped behaviour below.
|
||||
autoPage := formatFlag == "json" && !cmd.Flags().Changed("limit")
|
||||
|
||||
// Apply a default limit + hard-max clamp (TASK-2000). Unbounded
|
||||
// lists (esp. --all, which spans every collection) can dump
|
||||
// megabytes into an agent's context; cap them. An explicit
|
||||
// oversized --limit is clamped down rather than rejected.
|
||||
// --limit 0 is "the default": it is clamped to
|
||||
// defaultItemListLimit, not "no limit" (BUG-4024).
|
||||
effectiveLimit := limitNum
|
||||
if effectiveLimit <= 0 {
|
||||
effectiveLimit = defaultItemListLimit
|
||||
@@ -554,9 +566,11 @@ in that collection are shown. Items with status "done" are hidden by default.`,
|
||||
if effectiveLimit > maxItemListLimit {
|
||||
effectiveLimit = maxItemListLimit
|
||||
}
|
||||
params.Set("limit", fmt.Sprintf("%d", effectiveLimit))
|
||||
if offsetNum > 0 {
|
||||
params.Set("offset", fmt.Sprintf("%d", offsetNum))
|
||||
if !autoPage {
|
||||
params.Set("limit", fmt.Sprintf("%d", effectiveLimit))
|
||||
if offsetNum > 0 {
|
||||
params.Set("offset", fmt.Sprintf("%d", offsetNum))
|
||||
}
|
||||
}
|
||||
|
||||
// Apply arbitrary --field key=value filters as query params.
|
||||
@@ -587,10 +601,19 @@ in that collection are shown. Items with status "done" are hidden by default.`,
|
||||
// by its singular alias (BUG-2630); the fallback preserves
|
||||
// shorthand against pre-resolver servers (BUG-2578).
|
||||
items, err = cli.WithCollectionAliasFallback(args[0], client.CollectionNotFoundIsAuthoritative, func(slug string) ([]models.Item, error) {
|
||||
got, pg, lerr := client.ListCollectionItemsReport(ws, slug, params)
|
||||
var got []models.Item
|
||||
var pg cli.ItemListPage
|
||||
var lerr error
|
||||
if autoPage {
|
||||
got, pg, lerr = client.ListAllCollectionItemsReport(ws, slug, params, offsetNum)
|
||||
} else {
|
||||
got, pg, lerr = client.ListCollectionItemsReport(ws, slug, params)
|
||||
}
|
||||
page = pg
|
||||
return got, lerr
|
||||
})
|
||||
} else if autoPage {
|
||||
items, page, err = client.ListAllItemsReport(ws, params, offsetNum)
|
||||
} else {
|
||||
items, page, err = client.ListItemsReport(ws, params)
|
||||
}
|
||||
@@ -604,7 +627,10 @@ in that collection are shown. Items with status "done" are hidden by default.`,
|
||||
}
|
||||
// The truncation notice goes to stderr after the output, in every
|
||||
// format (BUG-3704).
|
||||
defer printItemListTruncation(page, len(items), effectiveLimit)
|
||||
// The auto-page path read to the end, so there is nothing to say.
|
||||
if !autoPage {
|
||||
defer printItemListTruncation(page, len(items), effectiveLimit)
|
||||
}
|
||||
|
||||
if formatFlag == "json" {
|
||||
// Default to the token-light summary shape (drops the rich
|
||||
@@ -653,7 +679,7 @@ in that collection are shown. Items with status "done" are hidden by default.`,
|
||||
cmd.MarkFlagsMutuallyExclusive("parent", "unparented")
|
||||
cmd.Flags().StringVar(&sortBy, "sort", "", "sort order (e.g. priority:desc,created_at:asc)")
|
||||
cmd.Flags().StringVar(&groupBy, "group-by", "", "group results by field")
|
||||
cmd.Flags().IntVar(&limitNum, "limit", 0, fmt.Sprintf("max number of items to return (default %d, max %d)", defaultItemListLimit, maxItemListLimit))
|
||||
cmd.Flags().IntVar(&limitNum, "limit", 0, fmt.Sprintf("max number of items to return (default %d, max %d; with --format json and no --limit, every matching item is returned)", defaultItemListLimit, maxItemListLimit))
|
||||
cmd.Flags().IntVar(&offsetNum, "offset", 0, "skip the first N items (for pagination)")
|
||||
cmd.Flags().BoolVar(&showAll, "all", false, "include done/completed/archived items")
|
||||
cmd.Flags().BoolVar(&fullOutput, "full", false, "JSON output only: include full content + all fields (default: token-light summary shape)")
|
||||
|
||||
+1
-1
@@ -14,7 +14,7 @@ REST API at `/api/v1/`. Key endpoints:
|
||||
- `GET /workspaces/{ws}/collections/{coll}/field-usage` — per field key, how many live items hold a value, and per string value how many hold it (TASK-2188): what the web schema editor shows before a field or option is removed. Owner-only plus full visibility, the PATCH's gates. The PATCH itself answers with an additive `omitempty` `warnings.orphaned` [{field, option?, items}] for each removed field, or option removed from a field that stays a select/multi_select, that live items still hold, read after the commit (a renamed option's migrated values are not listed). The values stay stored: a removed field's come back if a field with that key is declared again; a removed option's stay on their items, survive a `fields_patch` of other keys, and are refused on a write that sets them or a full `fields` blob that carries them. The CLI prints the list to stderr
|
||||
- `GET /workspaces/{ws}/archived-collections`, `POST /workspaces/{ws}/archived-collections/{ref}/restore` — list archived collections (id, name, slug, prefix, icon, live `item_count`, `archived_at`) and restore one by id or slug, its items with it (TASK-2189). Owner-only, like the archive. A restore cannot collide: slugs are allocated unique across archived rows too, so a newer same-named collection took `x-2`. 404 for a ref that is not an archived collection of this workspace. The restore publishes `collection_updated`, and the item-sync epoch fingerprints the live collection set, so delta clients resync and receive the restored items. CLI: `pad collection archived` / `pad collection restore <slug>`; MCP `pad_collection` `list-archived` / `restore` (v0.72)
|
||||
- `GET/POST /workspaces/{ws}/collections/{coll}/items` — item CRUD
|
||||
- **The list is bounded and paged** (BUG-3704; the same on `GET /workspaces/{ws}/items`). `limit` defaults to, and is capped at, **1000**; a missing, zero, negative or non-numeric `limit` means 1000. It used to mean every matching item with its body. `offset` skips rows, and it now works on the `?search=` path too (it used to be ignored there). Every response sets `X-Pad-Has-More: true|false`, and `X-Pad-Next-Offset: <n>` when there is more. The body stays a bare array. The order always ends on the item id, so offset pages neither repeat nor skip rows that tie on the sort. **Contract change:** a client that does not read these headers now gets the first 1000 rows of a longer list, not all of them. On this repo's own binary the CLI, MCP and web read them; an external or older client listing past 1000 rows must page. `pad item list` keeps its own 200 default, prints the exact next step on stderr when there is more, and takes `--offset`. `pad_item` `list` (MCP) says `has_more` and never pages.
|
||||
- **The list is bounded and paged** (BUG-3704; the same on `GET /workspaces/{ws}/items`). `limit` defaults to, and is capped at, **1000**; a missing, zero, negative or non-numeric `limit` means 1000. It used to mean every matching item with its body. `offset` skips rows, and it now works on the `?search=` path too (it used to be ignored there). Every response sets `X-Pad-Has-More: true|false`, and `X-Pad-Next-Offset: <n>` when there is more. The body stays a bare array. The order always ends on the item id, so offset pages neither repeat nor skip rows that tie on the sort. **Contract change:** a client that does not read these headers now gets the first 1000 rows of a longer list, not all of them. On this repo's own binary the CLI, MCP and web read them; an external or older client listing past 1000 rows must page. `pad item list` keeps its own 200 default for table and markdown output (and for any explicit `--limit`, including 0, which means the default), prints the exact next step on stderr when there is more, and takes `--offset`. With `--format json` and no `--limit`, it follows `X-Pad-Next-Offset` to the end (from `--offset` if given) and prints the whole array with no notice, since a consumer parsing the array never sees stderr (BUG-4024); it fails, rather than print a partial array, if the server's next offset does not advance or the list runs past 1000 pages. `pad_item` `list` (MCP) says `has_more` and never pages.
|
||||
- **At most 50 field filters** per list request (BUG-3703); more is a `400 validation_error` "invalid list filter: N field filters, at most 50", before any store work. Undeclared keys are checked in sorted order, and the check stops at the first one no item stores, which decides the 400.
|
||||
- **`?search=` keeps the list's filters** (BUG-3686): `include_archived=true` includes archived (soft-deleted) matches, and an explicit `sort` orders the matches. Without `sort` they come in relevance order, ending on the item id. These used to be ignored whenever the list carried a search term. **`tag` matches an element exactly** (BUG-3706): case-sensitive, with no wildcards, on both SQLite and Postgres. On SQLite it used to be a substring match that ignored case and treated `%` and `_` as wildcards. On Postgres, a tag holding a quote used to fail the request.
|
||||
- **Field filters on the list:** any query parameter that is not a known list parameter filters on that field. **It must NAME a field** (BUG-3480): one the collection's schema declares (on the workspace-wide `GET /workspaces/{ws}/items`, one any collection the caller can see declares), else `400 validation_error` "invalid list filter ...". It used to answer `200 []`, which is how the web Library page's `?all=true` and WebMCP's `?collection=` went unnoticed. A key no schema declares but some item in scope STORES is still honoured (the BUG-2850 census of undeclared keys) and named in the `X-Pad-Undeclared-Filter-Keys` response header, which `pad item list` prints to stderr. A key that is not a field key (letters, digits, `_`, `-`) is refused too; the store used to drop it and answer the unfiltered list. `parent` / `plan` are consumed as the parent filter first, unless the collection declares a field by that name. A value containing a comma is an OR over its trimmed pieces (`?status=open,done`, which is what `pad item list --status open,done` sends). **Documented limit (BUG-3167):** over the query string, a literal value that itself contains a comma cannot be filtered for exactly, because the comma is the delimiter. A repeated-parameter form was declined, because an older server reads only the first value and silently narrows the result. The census when this was written found 0 comma-containing option values (2,025 select/multi_select options across 27 workspaces). In-process callers are unaffected: `store.ListItems` matches `ItemListParams.Fields` EXACTLY and takes an OR only through `FieldsAnyOf`, so the uniqueness check, playbook routing, and schema-driven completed-work and bootstrap queries match a comma value as itself
|
||||
|
||||
@@ -0,0 +1,182 @@
|
||||
package cli
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-4024: ListAllItems and ListAllCollectionItems share one pager
|
||||
// (listAllPages) that takes a start offset, keeps the first page's
|
||||
// undeclared-filter keys, and refuses to return a partial list as a whole one.
|
||||
|
||||
func pagerServer(t *testing.T, total, page int, mutate func(w http.ResponseWriter, from, to int)) (*httptest.Server, *[]string) {
|
||||
t.Helper()
|
||||
var offsets []string
|
||||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
offsets = append(offsets, r.URL.Query().Get("offset"))
|
||||
from, _ := strconv.Atoi(r.URL.Query().Get("offset"))
|
||||
to := min(from+page, total)
|
||||
if to < total {
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(to))
|
||||
} else {
|
||||
w.Header().Set("X-Pad-Has-More", "false")
|
||||
}
|
||||
if mutate != nil {
|
||||
mutate(w, from, to)
|
||||
}
|
||||
var items []models.Item
|
||||
for i := from; i < to; i++ {
|
||||
items = append(items, models.Item{ID: fmt.Sprintf("it-%d", i)})
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(items)
|
||||
}))
|
||||
t.Cleanup(ts.Close)
|
||||
t.Setenv("HOME", t.TempDir())
|
||||
return ts, &offsets
|
||||
}
|
||||
|
||||
func TestBUG4024_PagerStartsAtTheGivenOffset(t *testing.T) {
|
||||
ts, offsets := pagerServer(t, 10, 3, nil)
|
||||
c := NewClientFromURL(ts.URL)
|
||||
for name, run := range map[string]func() ([]models.Item, ItemListPage, error){
|
||||
"workspace": func() ([]models.Item, ItemListPage, error) { return c.ListAllItemsReport("ws", url.Values{}, 4) },
|
||||
"collection": func() ([]models.Item, ItemListPage, error) {
|
||||
return c.ListAllCollectionItemsReport("ws", "tasks", url.Values{}, 4)
|
||||
},
|
||||
} {
|
||||
*offsets = nil
|
||||
got, _, err := run()
|
||||
if err != nil {
|
||||
t.Fatalf("%s: %v", name, err)
|
||||
}
|
||||
if len(got) != 6 || got[0].ID != "it-4" {
|
||||
t.Fatalf("%s: got %d items starting at %v, want 6 starting at it-4", name, len(got), got)
|
||||
}
|
||||
if fmt.Sprint(*offsets) != "[4 7]" {
|
||||
t.Fatalf("%s: offsets asked %q, want [4 7]", name, *offsets)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A negative start offset (`--offset -5`) is sent as no offset, so the server
|
||||
// reads from 0, as it does for any bad offset. The pager must count from 0
|
||||
// too, or its contiguity check refuses the server's correct next offset.
|
||||
func TestBUG4024_PagerTreatsANegativeStartOffsetAsZero(t *testing.T) {
|
||||
ts, offsets := pagerServer(t, 7, 3, nil)
|
||||
c := NewClientFromURL(ts.URL)
|
||||
got, _, err := c.ListAllItemsReport("ws", url.Values{}, -5)
|
||||
if err != nil {
|
||||
t.Fatalf("start offset -5: %v", err)
|
||||
}
|
||||
if len(got) != 7 || got[0].ID != "it-0" {
|
||||
t.Fatalf("got %d items starting at %v, want 7 starting at it-0", len(got), got)
|
||||
}
|
||||
if fmt.Sprint(*offsets) != "[ 3 6]" {
|
||||
t.Fatalf("offsets asked %q, want [ 3 6]", *offsets)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_PagerKeepsTheFirstPagesUndeclaredKeys(t *testing.T) {
|
||||
ts, _ := pagerServer(t, 7, 3, func(w http.ResponseWriter, from, _ int) {
|
||||
if from == 0 {
|
||||
w.Header().Set("X-Pad-Undeclared-Filter-Keys", "a,b")
|
||||
}
|
||||
})
|
||||
c := NewClientFromURL(ts.URL)
|
||||
_, page, err := c.ListAllItemsReport("ws", url.Values{}, 0)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if strings.Join(page.UndeclaredKeys, ",") != "a,b" {
|
||||
t.Fatalf("UndeclaredKeys = %v, want [a b] from the first page", page.UndeclaredKeys)
|
||||
}
|
||||
if !page.Known || page.HasMore {
|
||||
t.Fatalf("page = %+v, want Known and not HasMore at the end", page)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_PagerErrorsOnANonAdvancingOffset(t *testing.T) {
|
||||
ts, _ := pagerServer(t, 100, 10, func(w http.ResponseWriter, from, _ int) {
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(from))
|
||||
})
|
||||
c := NewClientFromURL(ts.URL)
|
||||
got, err := c.ListAllItems("ws", url.Values{})
|
||||
if err == nil || !strings.Contains(err.Error(), "does not advance") {
|
||||
t.Fatalf("err = %v (items %d), want a does-not-advance error", err, len(got))
|
||||
}
|
||||
if got != nil {
|
||||
t.Fatalf("a short list came back with the error: %d items", len(got))
|
||||
}
|
||||
}
|
||||
|
||||
// R2-F1: a has-more page must be full and contiguous; every other shape is an
|
||||
// error with no items, even when a later page says has-more=false.
|
||||
func TestBUG4024_PagerRefusesAMalformedHasMorePage(t *testing.T) {
|
||||
cases := map[string]struct{ rows, next int }{
|
||||
"empty page with an advancing next offset": {0, 5},
|
||||
"empty page that does not advance at all": {0, 0},
|
||||
"next offset jumps past the rows returned": {3, 9},
|
||||
"next offset one short of the rows": {3, 2},
|
||||
}
|
||||
for name, tc := range cases {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
// first page: serve tc.rows rows; the next request (if any) is the final page.
|
||||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
from, _ := strconv.Atoi(r.URL.Query().Get("offset"))
|
||||
var items []models.Item
|
||||
if from == 0 {
|
||||
w.Header().Set("X-Pad-Has-More", "true")
|
||||
w.Header().Set("X-Pad-Next-Offset", strconv.Itoa(tc.next))
|
||||
for i := 0; i < tc.rows; i++ {
|
||||
items = append(items, models.Item{ID: fmt.Sprintf("it-%d", i)})
|
||||
}
|
||||
} else {
|
||||
w.Header().Set("X-Pad-Has-More", "false")
|
||||
items = []models.Item{{ID: "late"}}
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(items)
|
||||
}))
|
||||
t.Cleanup(ts.Close)
|
||||
t.Setenv("HOME", t.TempDir())
|
||||
got, err := NewClientFromURL(ts.URL).ListAllItems("ws", url.Values{})
|
||||
if err == nil || !strings.Contains(err.Error(), "rather than return a partial list") {
|
||||
t.Fatalf("err = %v (items %d), want a partial-list refusal", err, len(got))
|
||||
}
|
||||
if got != nil {
|
||||
t.Fatalf("a short list came back with the error: %d items", len(got))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_PagerErrorsAtThePageCapNamingIt(t *testing.T) {
|
||||
defer SetMaxListPages(3)()
|
||||
ts, offsets := pagerServer(t, 1000, 10, nil)
|
||||
c := NewClientFromURL(ts.URL)
|
||||
got, err := c.ListAllCollectionItems("ws", "tasks", url.Values{})
|
||||
if err == nil || !strings.Contains(err.Error(), "3-page cap") {
|
||||
t.Fatalf("err = %v, want one naming the 3-page cap", err)
|
||||
}
|
||||
if got != nil || len(*offsets) != 3 {
|
||||
t.Fatalf("items %d, requests %d; want none returned and exactly 3 requests", len(got), len(*offsets))
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG4024_PagerAcceptsAListOfExactlyTheCap(t *testing.T) {
|
||||
defer SetMaxListPages(3)()
|
||||
ts, _ := pagerServer(t, 30, 10, nil)
|
||||
got, err := NewClientFromURL(ts.URL).ListAllItems("ws", url.Values{})
|
||||
if err != nil || len(got) != 30 {
|
||||
t.Fatalf("got %d items, err %v; want all 30 over exactly 3 pages", len(got), err)
|
||||
}
|
||||
}
|
||||
+76
-31
@@ -289,11 +289,36 @@ func itemListPage(h http.Header) ItemListPage {
|
||||
return p
|
||||
}
|
||||
|
||||
// ListAllItems is ListItems for a caller that needs every matching row (a
|
||||
// report): it follows the server's next-offset until has-more is false
|
||||
// (BUG-3704). params' limit and offset are its own. A server older than
|
||||
// BUG-3704 sends no paging state and lists everything in one response.
|
||||
func (c *Client) ListAllItems(wsSlug string, params url.Values) ([]models.Item, error) {
|
||||
// maxListPages bounds how many pages a ListAll* call reads. It is a runaway
|
||||
// guard against a server that reports has-more forever, not a size policy:
|
||||
// at the server's 1000 rows per page it is about a million rows. A var so a
|
||||
// test can use a small cap (BUG-4024).
|
||||
var maxListPages = 1000
|
||||
|
||||
// SetMaxListPages replaces the page cap and returns a func that restores it.
|
||||
// It is a test hook: the production cap is not configurable.
|
||||
func SetMaxListPages(n int) (restore func()) {
|
||||
prev := maxListPages
|
||||
maxListPages = n
|
||||
return func() { maxListPages = prev }
|
||||
}
|
||||
|
||||
// listAllPages is the one pager behind every ListAll* method (BUG-3704,
|
||||
// BUG-3793, BUG-4024). It reads from startOffset, follows the server's
|
||||
// next-offset until has-more is false, and keeps the FIRST page's
|
||||
// UndeclaredKeys (a filter warning is a property of the query, and the first
|
||||
// response is where it is raised). A server older than BUG-3704 sends no
|
||||
// paging state, so one response is everything. A has-more page must be full
|
||||
// and contiguous: the server sets has-more only when it trimmed the probe row
|
||||
// (handlers_items.go listItemsPage), so such a page holds exactly `limit` rows
|
||||
// and its next-offset is offset+limit, i.e. len(items) > 0 and next-offset ==
|
||||
// offset+len(items) (BUG-4024). An empty has-more page, a next-offset that
|
||||
// skips rows never returned, or one that falls short is an error, as is a
|
||||
// list longer than maxListPages: handing back a short list as though it were
|
||||
// the whole one is this bug's own class. A negative startOffset reads from 0,
|
||||
// as the server reads any bad offset.
|
||||
// params' limit and offset are the pager's own.
|
||||
func listAllPages(params url.Values, startOffset int, fetch func(url.Values) ([]models.Item, ItemListPage, error)) ([]models.Item, ItemListPage, error) {
|
||||
q := url.Values{}
|
||||
for k, v := range params {
|
||||
q[k] = v
|
||||
@@ -301,46 +326,66 @@ func (c *Client) ListAllItems(wsSlug string, params url.Values) ([]models.Item,
|
||||
q.Del("limit")
|
||||
q.Del("offset")
|
||||
var all []models.Item
|
||||
for offset := 0; ; {
|
||||
var first ItemListPage
|
||||
// A negative offset is sent as none, and the server reads none as 0, so
|
||||
// the contiguity check must count from 0 as well.
|
||||
offset := max(startOffset, 0)
|
||||
for pages := 1; ; pages++ {
|
||||
if offset > 0 {
|
||||
q.Set("offset", strconv.Itoa(offset))
|
||||
}
|
||||
items, page, err := c.ListItemsReport(wsSlug, q)
|
||||
items, page, err := fetch(q)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
return nil, ItemListPage{}, err
|
||||
}
|
||||
if pages == 1 {
|
||||
first = ItemListPage{UndeclaredKeys: page.UndeclaredKeys}
|
||||
}
|
||||
all = append(all, items...)
|
||||
if !page.HasMore || page.NextOffset <= offset {
|
||||
return all, nil
|
||||
if !page.HasMore {
|
||||
first.Known = page.Known
|
||||
return all, first, nil
|
||||
}
|
||||
if len(items) == 0 || page.NextOffset != offset+len(items) {
|
||||
return nil, ItemListPage{}, fmt.Errorf("the server reported more items (X-Pad-Has-More) but its next offset %d does not advance contiguously: the page at offset %d held %d items, so the next offset should be %d; stopped after %d items rather than return a partial list", page.NextOffset, offset, len(items), offset+len(items), len(all))
|
||||
}
|
||||
if pages >= maxListPages {
|
||||
return nil, ItemListPage{}, fmt.Errorf("the list is longer than the %d-page cap (%d items read, server still reports X-Pad-Has-More); narrow the filter or pass --limit", maxListPages, len(all))
|
||||
}
|
||||
offset = page.NextOffset
|
||||
}
|
||||
}
|
||||
|
||||
// ListAllItems is ListItems for a caller that needs every matching row (a
|
||||
// report): it follows the server's next-offset until has-more is false
|
||||
// (BUG-3704). params' limit and offset are its own. A server older than
|
||||
// BUG-3704 sends no paging state and lists everything in one response.
|
||||
func (c *Client) ListAllItems(wsSlug string, params url.Values) ([]models.Item, error) {
|
||||
items, _, err := c.ListAllItemsReport(wsSlug, params, 0)
|
||||
return items, err
|
||||
}
|
||||
|
||||
// ListAllItemsReport is ListAllItems from startOffset, also returning the
|
||||
// first page's UndeclaredKeys (BUG-4024).
|
||||
func (c *Client) ListAllItemsReport(wsSlug string, params url.Values, startOffset int) ([]models.Item, ItemListPage, error) {
|
||||
return listAllPages(params, startOffset, func(q url.Values) ([]models.Item, ItemListPage, error) {
|
||||
return c.ListItemsReport(wsSlug, q)
|
||||
})
|
||||
}
|
||||
|
||||
// ListAllCollectionItems is ListAllItems for one collection: it follows the
|
||||
// server's next-offset until has-more is false (BUG-3793).
|
||||
func (c *Client) ListAllCollectionItems(wsSlug, collSlug string, params url.Values) ([]models.Item, error) {
|
||||
q := url.Values{}
|
||||
for k, v := range params {
|
||||
q[k] = v
|
||||
}
|
||||
q.Del("limit")
|
||||
q.Del("offset")
|
||||
var all []models.Item
|
||||
for offset := 0; ; {
|
||||
if offset > 0 {
|
||||
q.Set("offset", strconv.Itoa(offset))
|
||||
}
|
||||
items, page, err := c.ListCollectionItemsReport(wsSlug, collSlug, q)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
all = append(all, items...)
|
||||
if !page.HasMore || page.NextOffset <= offset {
|
||||
return all, nil
|
||||
}
|
||||
offset = page.NextOffset
|
||||
}
|
||||
items, _, err := c.ListAllCollectionItemsReport(wsSlug, collSlug, params, 0)
|
||||
return items, err
|
||||
}
|
||||
|
||||
// ListAllCollectionItemsReport is ListAllCollectionItems from startOffset,
|
||||
// also returning the first page's UndeclaredKeys (BUG-4024).
|
||||
func (c *Client) ListAllCollectionItemsReport(wsSlug, collSlug string, params url.Values, startOffset int) ([]models.Item, ItemListPage, error) {
|
||||
return listAllPages(params, startOffset, func(q url.Values) ([]models.Item, ItemListPage, error) {
|
||||
return c.ListCollectionItemsReport(wsSlug, collSlug, q)
|
||||
})
|
||||
}
|
||||
|
||||
// ListItemsReport is ListItems plus what the response says besides its rows
|
||||
|
||||
Reference in New Issue
Block a user