diff --git a/docs/adr/0017-discover-pagination.md b/docs/adr/0017-discover-pagination.md new file mode 100644 index 0000000..f69eb4e --- /dev/null +++ b/docs/adr/0017-discover-pagination.md @@ -0,0 +1,61 @@ +# ADR-0017 — Discover pagination: signed body, applied page, capped `total` + +**Status:** Accepted +**Date:** 2026-10-27 + +## Context + +0.1.0 read `limit` and `offset` from the query string, which the request +signature does not cover. The OAN profile of the spec moves them into +`DiscoverAction` and answers with `DiscoverPagination` (`total`, `limit`, +`offset`, all required), counted in resources rather than catalogs. + +## Decision + +1. **The page is read from the signed body.** `?limit=` or `?offset=` on + `/discover` is refused with `SCH_INVALID_FORMAT`, naming + `message.limit`/`message.offset`. +2. **The unit is the resource.** The page is a slice of ranked resource ids; + a catalog may span pages. +3. **An oversize `limit` is clamped to `maxPageSize`; a page past the + retrieval depth is refused.** `maxPageSize` may not sit below + `defaultPageSize`, which config validation enforces. +4. **The response echoes the applied page**, not the requested one. `limit` + is the page size applied, not the count on the page: a short last page + still reports it. +5. **`total` is the ranked list's length, capped at + `maxCandidatesPerMode`** — the last page a caller can fetch. +6. **The cap is applied to the fused list before the page is cut.** + `maxCandidatesPerMode` counts ids *per retrieval mode*, so fusing several + modes yields a list longer than any one of them returned. Slicing first and + capping the count afterwards produces a last page carrying more resources + than `total` leaves room for, and `offset + count == total` never becomes + true. The storage layer caps, and the request layer reports what it is given. +7. **Order is stable across pages.** RRF breaks score ties by id. + +## Alternatives considered + +- **Ignore query-string pagination.** Answers the default page with a 200; a + client walking offsets loops on page one with nothing telling it why. +- **Refuse an oversize `limit`.** The caller would get no results for a + query that has them; clamping and echoing the applied value loses nothing. +- **Echo the requested page.** A caller that asked for 5000 and got 100 would + compute its next offset from 5000 and skip results. +- **An exact `total` from a count query.** About 100x the cost of the search + it accompanies, measured in A19, for a number no caller can page to. +- **Order by score alone.** Equal scores are common — every id only one mode + returned, at the same rank — and two that swap between runs put one result + on two pages and another on none. + +## Consequences + +- Breaking for 0.1.0 callers that paginate with the query string. +- `total` understates a broad query with more matches than the retrieval + depth. The spec allows this: a CN must treat it as the end of what is + pageable, not as an absolute match count. +- A page past the retrieval depth is a 400, not an empty page. +- `offset >= maxCandidatesPerMode` is the refusal test, not + `offset + limit > maxCandidatesPerMode`. `pageOf` already truncates a page + whose tail runs past the end, so testing the tail refuses the last page at + every limit that does not divide the depth exactly, and the caller never + reaches the `offset + count == total` that marks the end of the results. diff --git a/docs/adr/README.md b/docs/adr/README.md index aee8be0..6a073df 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -29,6 +29,7 @@ Start from [`0000-template.md`](0000-template.md). | [0014](0014-seams-that-ship-with-only-a-no-op.md) | `CatalogReplicator`: what a seam must carry to ship | A7 | | [0015](0015-master-catalogs-and-inheritance-refused.md) | Master catalogs and resource inheritance are refused at intake | A1 | | [0016](0016-router-net-http-servemux.md) | `net/http.ServeMux` for HTTP routing, superseding 0001 | D1 | +| [0017](0017-discover-pagination.md) | Discover pagination: signed body, applied page, capped `total` | #51 | **Source** points at the design document: `D` rows are the Technology Decisions table, `T` rows are TRD Alignment, `A` rows are Amendments. diff --git a/examples/verify.sh b/examples/verify.sh index 57bd3cf..165f8e5 100755 --- a/examples/verify.sh +++ b/examples/verify.sh @@ -366,15 +366,16 @@ refusal_case 18-discover-unparsable-jsonpath.json \ "SCH_INVALID_JSONPATH" "\$.message.intent.filters.expression" printf '%s\n' "--- pagination" -body="$(curl -sS -X POST "$BASE/discover?limit=1&offset=0" \ - -H 'Content-Type: application/json' \ - --data-binary @"$DIR/03-discover-schema-context.json")" +body="$(python3 -c 'import json,sys; b=json.load(open(sys.argv[1])); b["message"].update(limit=1, offset=0); print(json.dumps(b))' \ + "$DIR/03-discover-schema-context.json" | + curl -sS -X POST "$BASE/discover" \ + -H 'Content-Type: application/json' --data-binary @-)" got="$(resources "$body")" n="$(printf '%s' "$got" | awk -F, 'NF{print NF}')" if [ "${n:-0}" = "1" ]; then - ok "?limit=1 -> 1 resource [$got]" + ok "limit=1 -> 1 resource [$got]" else - bad "?limit=1 -> ${n:-0} resources [$got]" + bad "limit=1 -> ${n:-0} resources [$got]" fi echo diff --git a/src/beckn/actions.go b/src/beckn/actions.go index 0692610..dc6a8dd 100644 --- a/src/beckn/actions.go +++ b/src/beckn/actions.go @@ -137,16 +137,25 @@ type CatalogStats struct { // DiscoverAction is the `message` of a discover request. type DiscoverAction struct { Intent Intent `json:"intent"` + Limit int `json:"limit,omitempty"` + Offset int `json:"offset,omitempty"` } -// OnDiscoverAction is the `message` of the discover response, and it is exactly -// one field. +// OnDiscoverAction is the `message` of the discover response. // // It must NOT grow a Degraded field. The v2.0.0 schema is -// additionalProperties:false with `catalogs` as its only property, so an extra +// additionalProperties:false with only `catalogs` and `pagination`, so an extra // key is a response that fails its own schema — and `omitempty` would hide that // on the ordinary path while shipping the invalid body on the degraded one, the // path that matters. The degraded list travels as X-Beckn-Degraded (C11). type OnDiscoverAction struct { - Catalogs []Catalog `json:"catalogs"` + Catalogs []Catalog `json:"catalogs"` + Pagination *DiscoverPagination `json:"pagination,omitempty"` +} + +// DiscoverPagination is the page the answer applied (ADR-0017). +type DiscoverPagination struct { + Limit int `json:"limit"` + Offset int `json:"offset"` + Total int `json:"total"` } diff --git a/src/beckn/schema_conformance_test.go b/src/beckn/schema_conformance_test.go index df5efd5..8574c43 100644 --- a/src/beckn/schema_conformance_test.go +++ b/src/beckn/schema_conformance_test.go @@ -69,6 +69,7 @@ func bindings() []binding { {"DiscoverAction", DiscoverAction{}, schemaPath("DiscoverAction")}, {"OnDiscoverAction", OnDiscoverAction{}, schemaPath("OnDiscoverAction")}, + {"DiscoverPagination", DiscoverPagination{}, schemaPath("DiscoverPagination")}, {"Nack", Nack{}, schemaPath("NackBadRequest")}, {"NackMessage", NackMessage{}, schemaPath("NackBadRequest", "properties", "message")}, diff --git a/src/discover/controller.go b/src/discover/controller.go index 45f6691..b57da02 100644 --- a/src/discover/controller.go +++ b/src/discover/controller.go @@ -2,10 +2,8 @@ package discover import ( "encoding/json" - "fmt" "net/http" "net/url" - "strconv" "strings" "time" @@ -68,19 +66,19 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { observeIntent(ctx, envelope.Context, action.Intent) - page, err := pageFrom(r.URL.Query()) - if err != nil { + if err := rejectQueryStringPagination(r.URL.Query()); err != nil { httpx.WriteNack(ctx, w, c.errors, envelope.Context.MessageID, err) return } + page := pageFrom(action) - catalogs, degraded, err := c.service.Discover(ctx, envelope.Context, action.Intent, page) + answer, degraded, err := c.service.Discover(ctx, envelope.Context, action.Intent, page) if err != nil { httpx.WriteNack(ctx, w, c.errors, envelope.Context.MessageID, err) return } - observeResult(ctx, catalogs) + observeResult(ctx, answer.Catalogs) // Before the body: WriteJSON writes the status line, and a header set after // that is a header nobody receives. @@ -90,45 +88,26 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { body := httpx.Envelope[beckn.OnDiscoverAction]{ Context: responseContext(envelope.Context), - Message: beckn.OnDiscoverAction{Catalogs: catalogs}, + Message: answer, } if err := httpx.WriteJSON(ctx, w, http.StatusOK, body); err != nil { httpx.WriteNack(ctx, w, c.errors, envelope.Context.MessageID, err) } } -// pageFrom reads pagination off the query string, where it lives rather than -// inside the intent. -// -// An unreadable value is refused, not defaulted: zero already means "the default -// page size" to the mapper, so reading `limit=twenty` as zero would answer a page -// the caller never asked for and say nothing about it. -func pageFrom(query url.Values) (Page, error) { - limit, err := intParam(query, "limit") - if err != nil { - return Page{}, err - } - offset, err := intParam(query, "offset") - if err != nil { - return Page{}, err +// rejectQueryStringPagination refuses a page the request signature does not +// cover (ADR-0017). +func rejectQueryStringPagination(query url.Values) error { + if query.Has("limit") || query.Has("offset") { + return apperrors.Schema(beckn.CodeSchemaInvalidFormat, + "pagination moved from the query string to message.limit and message.offset").At(messageRoot) } - return Page{Limit: limit, Offset: offset}, nil + return nil } -// intParam reads one numeric query parameter. Absent is zero, which the mapper -// resolves to its own default; present and unreadable is a refusal. -func intParam(query url.Values, name string) (int, error) { - raw := query.Get(name) - if raw == "" { - return 0, nil - } - - value, err := strconv.Atoi(raw) - if err != nil { - return 0, apperrors.Schema(beckn.CodeSchemaInvalidFormat, - fmt.Sprintf("%s is not a whole number", name)).At("$." + name) - } - return value, nil +// pageFrom reads the page out of the signed body. +func pageFrom(action beckn.DiscoverAction) Page { + return Page{Limit: action.Limit, Offset: action.Offset} } // responseContext turns the request's envelope into the response's. The diff --git a/src/discover/controller_test.go b/src/discover/controller_test.go index c14d147..a15c1b7 100644 --- a/src/discover/controller_test.go +++ b/src/discover/controller_test.go @@ -312,14 +312,12 @@ func TestAnUnavailableModeIsA400WhenTheDeploymentAsksToBe(t *testing.T) { } } -// Pagination arrives as query parameters rather than inside the intent, and an -// oversize limit is clamped rather than refused (C11's neighbour: the caller -// still gets the results they asked about). -func TestLimitAndOffsetAreReadFromTheQueryString(t *testing.T) { +// Pagination arrives in the body; an oversize limit is clamped (ADR-0017). +func TestLimitAndOffsetAreReadFromTheBody(t *testing.T) { repo := &stubRepo{capabilities: everything()} handler := mount(t, repo, settings()) - if got := post(t, handler, "/discover?limit=1000&offset=40", wheat).Code; got != http.StatusOK { + if got := post(t, handler, "/discover", `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"limit":1000,"offset":40}}`).Code; got != http.StatusOK { t.Fatalf("status = %d, want 200 — an oversize limit is clamped, not refused", got) } if repo.gotQuery.Limit != settings().Search.MaxPageSize { @@ -330,13 +328,37 @@ func TestLimitAndOffsetAreReadFromTheQueryString(t *testing.T) { } } +// A 0.1.0 caller still sending ?limit= must get a 400, not a silent default page. +func TestQueryStringPaginationIsRefused(t *testing.T) { + for _, path := range []string{"/discover?limit=5", "/discover?offset=40"} { + t.Run(path, func(t *testing.T) { + repo := &stubRepo{capabilities: everything()} + + recorder := post(t, mount(t, repo, settings()), path, wheat) + if recorder.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", recorder.Code, recorder.Body) + } + body := recorder.Body.String() + if !strings.Contains(body, string(beckn.CodeSchemaInvalidFormat)) { + t.Errorf("body = %s, want SCH_INVALID_FORMAT", body) + } + if !strings.Contains(body, "message.limit") { + t.Errorf("body = %s, want the refusal to name message.limit/offset", body) + } + if repo.calls != 0 { + t.Errorf("the backend was searched %d times; a refused page runs no query", repo.calls) + } + }) + } +} + // A limit that is not a number is refused rather than read as zero. Zero means // "the default page size", so a typo would be answered confidently with a page // the caller did not ask for. func TestAPaginationParameterThatIsNotANumberIsRefused(t *testing.T) { repo := &stubRepo{capabilities: everything()} - recorder := post(t, mount(t, repo, settings()), "/discover?limit=twenty", wheat) + recorder := post(t, mount(t, repo, settings()), "/discover", `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"limit":"twenty"}}`) if recorder.Code != http.StatusBadRequest { t.Fatalf("status = %d, want 400; body: %s", recorder.Code, recorder.Body) } @@ -345,13 +367,12 @@ func TestAPaginationParameterThatIsNotANumberIsRefused(t *testing.T) { } } -// The same refusal for offset, which pageFrom reads with its own call to -// intParam rather than reusing limit's — a valid limit must not mask an -// unreadable offset. +// The same refusal for offset — a valid limit must not mask an unreadable +// offset. func TestAnUnreadableOffsetIsRefused(t *testing.T) { repo := &stubRepo{capabilities: everything()} - recorder := post(t, mount(t, repo, settings()), "/discover?limit=10&offset=twenty", wheat) + recorder := post(t, mount(t, repo, settings()), "/discover", `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"limit":10,"offset":"twenty"}}`) if recorder.Code != http.StatusBadRequest { t.Fatalf("status = %d, want 400; body: %s", recorder.Code, recorder.Body) } diff --git a/src/discover/intent_mapper.go b/src/discover/intent_mapper.go index 5390f09..0f935a1 100644 --- a/src/discover/intent_mapper.go +++ b/src/discover/intent_mapper.go @@ -14,7 +14,7 @@ import ( "github.com/OpenAgriNet/discovery-service/src/platform/jsonpath" ) -// Page is the request's pagination, which arrives as HTTP query parameters +// Page is the request's pagination, which arrives in the message body // rather than inside the intent. A struct rather than two adjacent ints, because // `limit, offset` and `offset, limit` compile identically. type Page struct { @@ -207,12 +207,7 @@ func mapSchemaContext(envelope beckn.Context) ([]domain.SchemaFilter, []domain.F return out, faults } -// mapPage resolves the page, clamping what can be clamped honestly and refusing -// what cannot. -// -// A limit over MaxPageSize is clamped, because the caller still gets the results -// they asked about. A page past MaxCandidatesPerMode is refused, because it would -// come back empty and an empty page 26 is indistinguishable from the end. +// mapPage clamps an oversize limit, refuses a page past the depth (ADR-0017). func mapPage(page Page, search config.Search) (limit, offset int, faults []domain.Fault) { limit = page.Limit if limit <= 0 { @@ -227,13 +222,13 @@ func mapPage(page Page, search config.Search) (limit, offset int, faults []domai offset = 0 } - if offset+limit > search.MaxCandidatesPerMode { + if search.OffsetBeyondDepth(offset) { faults = append(faults, domain.Fault{ Path: "$['offset']", Code: string(beckn.CodeSchemaInvalidFormat), Message: fmt.Sprintf( - "offset %d plus limit %d passes the retrieval depth of %d", - offset, limit, search.MaxCandidatesPerMode), + "offset %d starts past the retrieval depth of %d", + offset, search.MaxCandidatesPerMode), }) } return limit, offset, faults diff --git a/src/discover/intent_mapper_test.go b/src/discover/intent_mapper_test.go index 08e8550..32b1a7e 100644 --- a/src/discover/intent_mapper_test.go +++ b/src/discover/intent_mapper_test.go @@ -501,16 +501,18 @@ func TestValidateConstraintReportsEveryFaultNotJustTheFirst(t *testing.T) { } } -// mapPage's own boundary: `>` refuses, so a page landing EXACTLY on the -// retrieval depth must be answered, not refused. TestLimitIsClampedAndAPagePastTheRetrievalDepthIsRefused -// only asserts the over side of this line. -func TestAPageExactlyAtTheRetrievalDepthIsNotRefused(t *testing.T) { +// mapPage's own boundary: the guard reads the offset alone, so the last +// reachable offset must be answered however large the limit is. A limit that +// overhangs the depth is the tail the page slice truncates, not a refusal — +// otherwise the final results are unreachable at any limit the depth does not +// divide evenly. +func TestTheLastReachableOffsetIsNotRefused(t *testing.T) { cfg := settings() - page := discover.Page{Limit: 100, Offset: cfg.Search.MaxCandidatesPerMode - 100} + page := discover.Page{Limit: 100, Offset: cfg.Search.MaxCandidatesPerMode - 1} _, fatal, _ := discover.MapIntent(beckn.Intent{TextSearch: "wheat"}, beckn.Context{}, page, cfg) if len(fatal) != 0 { - t.Fatalf("fatal = %s, want none — offset+limit lands exactly on the depth, not past it", codesOf(fatal)) + t.Fatalf("fatal = %s, want none — the offset is inside the depth, only the limit overhangs it", codesOf(fatal)) } } diff --git a/src/discover/service.go b/src/discover/service.go index 2960c12..2b697ae 100644 --- a/src/discover/service.go +++ b/src/discover/service.go @@ -38,10 +38,10 @@ func NewService(repo domain.SearchRepository, cfg config.Config) *Service { // rather than inside them, because it reaches the caller as a header (C11). func (s *Service) Discover( ctx context.Context, envelope beckn.Context, intent beckn.Intent, page Page, -) ([]beckn.Catalog, []string, error) { +) (beckn.OnDiscoverAction, []string, error) { query, fatal, partial := MapIntent(intent, envelope, page, s.cfg) if len(fatal) > 0 { - return nil, nil, refusal(fatal) + return beckn.OnDiscoverAction{}, nil, refusal(fatal) } reportPartials(ctx, partial) @@ -54,12 +54,12 @@ func (s *Service) Discover( modes, degraded, err := s.negotiate(query) if err != nil { - return nil, nil, err + return beckn.OnDiscoverAction{}, nil, err } result, err := s.repo.Search(ctx, query, modes) if err != nil { - return nil, nil, typedSearchFailure(ctx, err) + return beckn.OnDiscoverAction{}, nil, typedSearchFailure(ctx, err) } // Both halves, joined once: negotiate's are the modes this deployment cannot @@ -68,7 +68,17 @@ func (s *Service) Discover( degraded = append(degraded, result.Degraded...) observeRetrieval(ctx, modes, degraded) - return render(result.Catalogs), degraded, nil + return beckn.OnDiscoverAction{ + Catalogs: render(result.Catalogs), + // The applied page. Total is capped here, at the one place every + // backend's answer passes through, so a store that ranks deeper than + // the depth cannot report a total no caller can page to (ADR-0017). + Pagination: &beckn.DiscoverPagination{ + Limit: query.Limit, + Offset: query.Offset, + Total: s.cfg.Search.CapToDepth(result.Total), + }, + }, degraded, nil } // typedSearchFailure says whose mistake a failed search was. diff --git a/src/discover/service_test.go b/src/discover/service_test.go index 6ca6e73..a4ed799 100644 --- a/src/discover/service_test.go +++ b/src/discover/service_test.go @@ -81,11 +81,11 @@ func hasMode(modes []domain.Capability, wanted domain.Capability) bool { func codeOf(t *testing.T, err error) beckn.ErrorCode { t.Helper() - fault := apperrors.FromError(err) - if fault == nil { - t.Fatalf("expected a fault, got nil") + if fault := apperrors.FromError(err); fault != nil { + return fault.Code } - return fault.Code + t.Fatalf("expected a fault, got nil") + return "" } // A textSearch on a deployment with no semantic mode answers, and says what it @@ -99,8 +99,9 @@ func TestAModeTheBackendLacksIsNamedRatherThanDropped(t *testing.T) { result: domain.SearchResult{Catalogs: []domain.Catalog{{ID: "c1"}}}, } - catalogs, degraded, err := discover.NewService(repo, settings()). + answer, degraded, err := discover.NewService(repo, settings()). Discover(t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat seeds"}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -181,13 +182,14 @@ func TestSpatialStillAnswersWithTextSearchOff(t *testing.T) { cfg := settings() cfg.Search.EnableTextSearch = false - catalogs, _, err := discover.NewService(repo, cfg).Discover( + answer, _, err := discover.NewService(repo, cfg).Discover( t.Context(), beckn.Context{}, beckn.Intent{Spatial: []beckn.SpatialConstraint{{ Op: beckn.OpSIntersects, Geometry: bengaluru(), }}}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -214,12 +216,13 @@ func TestAFilterABackendCannotRunIsDegradedRatherThanIgnored(t *testing.T) { result: domain.SearchResult{Catalogs: []domain.Catalog{{ID: "c1"}}}, } - catalogs, degraded, err := discover.NewService(repo, settings()).Discover( + answer, degraded, err := discover.NewService(repo, settings()).Discover( t.Context(), beckn.Context{}, beckn.Intent{Filters: &beckn.Filters{ Type: "jsonpath", Expression: `$.catalogs[*].resources[*] ? (@.grade == "A")`, }}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -389,11 +392,27 @@ func TestAnOversizeLimitIsClampedAndAnUnreachablePageIsRefused(t *testing.T) { if _, _, err := service.Discover( t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, - discover.Page{Limit: 20, Offset: 490}); err == nil { + discover.Page{Limit: 20, Offset: 500}); err == nil { t.Error("a page past the retrieval depth was answered; want a refusal naming the boundary") } } +// A total past the retrieval depth would point the caller at a page it cannot fetch. +func TestTheTotalIsReturnedAndCappedAtTheRetrievalDepth(t *testing.T) { + for matched, want := range map[int]int{42: 42, 900: settings().Search.MaxCandidatesPerMode} { + repo := &stubRepo{capabilities: everything(), result: domain.SearchResult{Total: matched}} + + answer, _, err := discover.NewService(repo, settings()).Discover( + t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, discover.Page{}) + if err != nil { + t.Fatalf("Discover: %v", err) + } + if got := answer.Pagination.Total; got != want { + t.Errorf("matched %d: Total = %d, want %d", matched, got, want) + } + } +} + // A fatal mapper fault refuses the request and runs no query. Continuing would // WIDEN it, and a widened answer is indistinguishable at the caller from a // correct one. @@ -512,8 +531,9 @@ func TestARenderedCatalogCarriesItsDocumentsVerbatim(t *testing.T) { }}, }}}} - catalogs, _, err := discover.NewService(repo, settings()). + answer, _, err := discover.NewService(repo, settings()). Discover(t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -576,8 +596,9 @@ func TestNoOfferWhoseResourcesAreAllOffThePageIsRendered(t *testing.T) { t.Fatalf("seeding: %v", err) } - catalogs, _, err := discover.NewService(repo, settings()). + answer, _, err := discover.NewService(repo, settings()). Discover(t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -657,8 +678,9 @@ func TestACatalogWithAnUnreadableDocumentIsDropped(t *testing.T) { Document: json.RawMessage(`not json`), }}}} - catalogs, _, err := discover.NewService(repo, settings()). + answer, _, err := discover.NewService(repo, settings()). Discover(t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -679,8 +701,9 @@ func TestAResourceWithAnUnreadableDocumentIsDropped(t *testing.T) { }, }}}} - catalogs, _, err := discover.NewService(repo, settings()). + answer, _, err := discover.NewService(repo, settings()). Discover(t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } @@ -700,8 +723,9 @@ func TestAnOfferWithAnUnreadableDocumentIsDropped(t *testing.T) { }, }}}} - catalogs, _, err := discover.NewService(repo, settings()). + answer, _, err := discover.NewService(repo, settings()). Discover(t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } diff --git a/src/domain/query.go b/src/domain/query.go index f6270e4..bf5e486 100644 --- a/src/domain/query.go +++ b/src/domain/query.go @@ -125,11 +125,17 @@ type SearchQuery struct { // SearchResult is one page of an answered query. // // Degraded names the retrieval modes that did not contribute and reaches the -// caller as the X-Beckn-Degraded header, not a body key (C11). There is no -// Total (A19). +// caller as the X-Beckn-Degraded header, not a body key (C11). +// +// Total is the length of the ranked list the page was cut from, before paging. +// It is NOT yet the number a caller can page to: a backend may rank deeper than +// the retrieval depth, so the request layer caps it at Search.MaxCandidatesPerMode +// before putting it on the wire (ADR-0017). Capping there rather than here is +// what lets a backend that has no depth of its own still answer correctly. type SearchResult struct { Catalogs []Catalog Degraded []string + Total int } // Scope is the request-wide gate every retrieval mode reads (A6). diff --git a/src/platform/config/config.go b/src/platform/config/config.go index fef62cc..5663641 100644 --- a/src/platform/config/config.go +++ b/src/platform/config/config.go @@ -146,9 +146,10 @@ type Search struct { MaxPageSize int `env:"SEARCH_MAX_PAGE_SIZE" envDefault:"100"` // How many ids one retrieval mode may return into fusion. Also the reachable - // pagination depth: a request whose offset + limit passes it is refused, since - // slicing past the end of the fused list would answer with an empty page that - // reads exactly like the end of the results. + // pagination depth: a request whose offset starts at or past it is refused, + // because slicing past the end of the fused list would answer with an empty + // page that reads exactly like the end of the results. Read it through + // OffsetBeyondDepth and CapToDepth rather than comparing against it directly. MaxCandidatesPerMode int `env:"SEARCH_MAX_CANDIDATES_PER_MODE" envDefault:"500"` // The largest S_DWITHIN radius a caller may ask for. @@ -180,6 +181,21 @@ type Search struct { EnableTextSearch bool `env:"SEARCH_ENABLE_TEXT_SEARCH" envDefault:"true"` } +// OffsetBeyondDepth says whether a page starts at or past the last id a caller +// can reach. The request layer turns a true into a client fault and the storage +// layer into a guard; the comparison itself is written once, here, so the two +// cannot drift into refusing different pages. +func (s Search) OffsetBeyondDepth(offset int) bool { + return offset >= s.MaxCandidatesPerMode +} + +// CapToDepth trims a count to what a caller can page to. Fusing several modes +// yields more ids than any one of them retrieved, so a fused list is capped +// before it is either sliced or reported as a total (ADR-0017). +func (s Search) CapToDepth(n int) int { + return min(n, s.MaxCandidatesPerMode) +} + // Embeddings configures the Embedder seam — one struct for both paths (A3). type Embeddings struct { // noop (the default — semantic search is deferred, A5), hashing (CI), @@ -219,11 +235,11 @@ type Validation struct { // beckn.yaml is fetched at boot rather than baked into the image, so a // deployment names the document it validates against. // - // The default is a TAG, and that is the whole reason there can be one. This + // The default is a SHA, and that is the whole reason there can be one. This // field carried no default until 2026-09-09, on the argument that which spec // a network trusts is not a repository-wide decision — true of a branch, // which would let an upstream merge change the validator under a running - // deployment without a deploy. refs/tags/core-v2.0.0-lts cannot move, so the + // deployment without a deploy. A pinned commit cannot move, so the // default names one immutable document and a network that trusts a different // one still overrides it. tests/testdata/beckn-v2.0.0.yaml is byte-identical // to it, so the fetch path and an air-gapped mount agree. @@ -231,7 +247,7 @@ type Validation struct { // The cost is that boot now makes a network call before falling back to // SpecCachePath. LoadSpecIndex already warns loudly and falls back, so an // offline boot is slower and noisier, not broken. - SpecURL string `env:"VALIDATION_SPEC_URL" envDefault:"https://raw.githubusercontent.com/beckn/protocol-specifications-v2/refs/tags/core-v2.0.0-lts/api/v2.0.0/beckn.yaml"` + SpecURL string `env:"VALIDATION_SPEC_URL" envDefault:"https://raw.githubusercontent.com/OpenAgriNet/protocol-specifications-v2/a8451c48909e27bb86e26dec21ce7f71c5cb90b2/api/v2.0.0/beckn.yaml"` // Where the fetched spec is cached, and what an air-gapped deployment // mounts in place of the fetch. diff --git a/src/storage/conformance/discover.go b/src/storage/conformance/discover.go index 5d97b95..370559c 100644 --- a/src/storage/conformance/discover.go +++ b/src/storage/conformance/discover.go @@ -30,8 +30,8 @@ import ( // not the order: Postgres ranks by ts_rank_cd, memory has no relevance to // rank by. They agree on the stable (catalog_id, id) tail, which the one // deliberately text-free pagination case pins. -// - No case asserts a total; there is none (A19). What a paginating caller -// depends on is pinned instead — the offset SLICES a stable order. +// - The pagination case asserts Total and that the offset SLICES a stable +// order. // - No case names `jsonpath` to SEARCH with: since Task 22 Postgres executes // the subset and memory declines it, which is what puts the filter's own // tests on the Postgres side. The one case naming it asserts what a backend @@ -642,10 +642,12 @@ func pageTwoDoesNotOverlapPageOne() Case { t.Errorf("page two is %v, want [r-02 r-03]", got) } - // There is no total to assert (A19). What remains is the property - // a caller walking pages actually depends on: the offset SLICES a - // stable order rather than re-ranking it, so page two holds the - // next two and not two of the same. + if first.Total != 5 || second.Total != 5 { + t.Errorf("Total = %d and %d, want 5 on both pages", first.Total, second.Total) + } + + // The offset SLICES a stable order rather than re-ranking it, so + // page two holds the next two and not two of the same. }, } } diff --git a/src/storage/memory/search.go b/src/storage/memory/search.go index 1063c9e..e8c904e 100644 --- a/src/storage/memory/search.go +++ b/src/storage/memory/search.go @@ -62,9 +62,12 @@ func (r *Repository) Search( page = nil } + // The full ranked length: this store has no retrieval depth to cap against, + // and domain.SearchResult.Total says the request layer caps it. return domain.SearchResult{ Catalogs: r.hydrate(pageOf(page, query.Offset, query.Limit), scope), Degraded: degraded, + Total: len(page), }, nil } @@ -285,11 +288,7 @@ func pageOf(ranked []domain.Resource, offset, limit int) []domain.Resource { if offset >= len(ranked) { return nil } - end := offset + limit - if end > len(ranked) { - end = len(ranked) - } - return ranked[offset:end] + return ranked[offset:min(offset+limit, len(ranked))] } // hydrate folds the page back into catalogs, in the PAGE's order. diff --git a/src/storage/postgres/fusion.go b/src/storage/postgres/fusion.go index 787495f..87e330b 100644 --- a/src/storage/postgres/fusion.go +++ b/src/storage/postgres/fusion.go @@ -22,8 +22,16 @@ const rrfK = 60.0 // list, and intersecting would turn that into an empty page — at the caller, // indistinguishable from "nothing matched". func RRF(ranked ...[]string) []string { - scores := make(map[string]float64) - order := make([]string, 0) + // Sized for the worst case, every list disjoint. Fusing N modes at the + // retrieval depth is thousands of ids; letting the map and slice grow into + // that costs 32 allocations and 169KB against 6 and 79KB presized. Same + // wall time either way — this is for the garbage collector, not latency. + capacity := 0 + for _, list := range ranked { + capacity += len(list) + } + scores := make(map[string]float64, capacity) + order := make([]string, 0, capacity) for _, list := range ranked { for index, id := range list { @@ -36,11 +44,7 @@ func RRF(ranked ...[]string) []string { } } - // By score, then by id. The tiebreak is not cosmetic: the page is a SLICE of - // this list, so two equally-scored ids that swap between runs of the same - // query put one of them on both page 1 and page 2 and the other on neither. - // Ties are the common case — every id only one mode returned, at the same - // rank, scores identically. + // By score, then by id, so pages are stable (ADR-0017). slices.SortFunc(order, func(left, right string) int { if byScore := cmp.Compare(scores[right], scores[left]); byScore != 0 { return byScore diff --git a/src/storage/postgres/read_path_test.go b/src/storage/postgres/read_path_test.go index aee05d2..6eb25fa 100644 --- a/src/storage/postgres/read_path_test.go +++ b/src/storage/postgres/read_path_test.go @@ -475,6 +475,58 @@ func TestAPagePastTheRetrievalDepthIsAFaultAndNotAnEmptyPage(t *testing.T) { } } +// The cap counts ids PER MODE, so two modes over the same corpus fuse to more +// ids than either retrieved. Total is the fused list's length capped at the +// depth, so a page cut from an uncapped list runs past the total the caller is +// paging against — it reads the last page, finds more resources on it than +// `total` leaves room for, and `offset + count == total` never comes true. +// +// Two catalogs whose text differs, so lexical and fuzzy rank them differently +// and their top-`depth` lists are not the same ids. One catalog of identical +// lots does not reproduce it: both modes return the same ids and the union is +// the cap. +func TestTheLastPageDoesNotRunPastTheTotal(t *testing.T) { + alpha := make([]domain.ResourcePatch, 0, 12) + for index := range 12 { + alpha = append(alpha, searchable( + fmt.Sprintf("a-%02d", index), fmt.Sprintf("kharif seed lot %02d of the northern district consignment", index), + "", "https://beckn.org/Agri", "SeedLot")) + } + bravo := make([]domain.ResourcePatch, 0, 6) + for index := range 6 { + bravo = append(bravo, searchable( + fmt.Sprintf("b-%02d", index), fmt.Sprintf("khariff %02d", index), + "", "https://beckn.org/Agri", "SeedLot")) + } + + repository, _ := publish(t, nil, + readFixture{catalog: "cat-overrun-a", resources: alpha}, + readFixture{catalog: "cat-overrun-b", resources: bravo}) + + depth := searchConfig().MaxCandidatesPerMode + offset := depth - 2 + + result, err := repository.Search(context.Background(), + domain.SearchQuery{Text: "kharif", Limit: 4, Offset: offset}, bothTextModes) + if err != nil { + t.Fatalf("the last reachable page was refused: %v", err) + } + + if result.Total != depth { + t.Fatalf("Total = %d, want it capped at the depth %d — the corpus must "+ + "fuse to more ids than the cap for this test to mean anything", result.Total, depth) + } + got := 0 + for _, catalog := range result.Catalogs { + got += len(catalog.Resources) + } + if offset+got != result.Total { + t.Errorf("offset %d + %d resources = %d, want it to land on Total %d — "+ + "a caller cannot tell it has reached the end otherwise", + offset, got, offset+got, result.Total) + } +} + // A mode that returns exactly its cap is a mode whose list is a truncation // rather than the answer, and that is the one state the count guards read. The // corpus is wider than the cap on the ordinary query, not on a pathological diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index 8ba65af..abf980a 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -93,7 +93,7 @@ type outcome struct { func (s *SearchRepository) Search( ctx context.Context, query domain.SearchQuery, modes []domain.Capability, ) (domain.SearchResult, error) { - if err := s.withinRetrievalDepth(query); err != nil { + if err := s.ensureWithinRetrievalDepth(query); err != nil { return domain.SearchResult{}, err } @@ -105,40 +105,42 @@ func (s *SearchRepository) Search( // calls now(); it is for the backends that have none. scope := domain.Scope{NetworkID: query.NetworkID, Now: time.Now().UTC()} - ranked, filtering, degraded := s.negotiate(modes) + rankedModes, filtering, degraded := s.negotiate(modes) - lists, failed, fatal := fold(s.retrieve(ctx, query, ranked, scope)) + rankedIDs, failed, fatal := partitionOutcomes(s.retrieve(ctx, query, rankedModes, scope)) if fatal != nil { return domain.SearchResult{}, fatal } degraded = append(degraded, failed...) - if len(lists) == 0 && filtering { + if len(rankedIDs) == 0 && filtering { ids, err := s.filterOnly(ctx, query, scope) if err != nil { return domain.SearchResult{}, err } - lists = append(lists, ids) + rankedIDs = append(rankedIDs, ids) } - fused := RRF(lists...) + // Capped before the page is cut, not after (ADR-0017). + fused := RRF(rankedIDs...) + fused = fused[:s.search.CapToDepth(len(fused))] page := pageOf(fused, query.Offset, query.Limit) catalogs, err := s.hydrator.Hydrate(ctx, page, scope) if err != nil { return domain.SearchResult{}, err } - return domain.SearchResult{Catalogs: catalogs, Degraded: degraded}, nil + return domain.SearchResult{Catalogs: catalogs, Degraded: degraded, Total: len(fused)}, nil } -// withinRetrievalDepth refuses a page past what any mode retrieves. Checked +// ensureWithinRetrievalDepth refuses a page past what any mode retrieves. Checked // before anything runs, and named in the error so the caller can tell it from // the end of the results. -func (s *SearchRepository) withinRetrievalDepth(query domain.SearchQuery) error { - if query.Offset+query.Limit > s.search.MaxCandidatesPerMode { +func (s *SearchRepository) ensureWithinRetrievalDepth(query domain.SearchQuery) error { + if s.search.OffsetBeyondDepth(query.Offset) { return fmt.Errorf( - "%w: offset %d plus limit %d passes the %d ids a mode retrieves", - domain.ErrRetrievalDepth, query.Offset, query.Limit, s.search.MaxCandidatesPerMode) + "%w: offset %d starts past the %d ids a mode retrieves", + domain.ErrRetrievalDepth, query.Offset, s.search.MaxCandidatesPerMode) } return nil } @@ -159,9 +161,9 @@ func (s *SearchRepository) filterOnly( return ids, nil } -// fold splits the per-mode outcomes into the lists to fuse and the modes to +// partitionOutcomes splits the per-mode outcomes into the lists to fuse and the modes to // report. A failed mode is RECORDED in X-Beckn-Degraded, not fatal. -func fold(outcomes []outcome) (ranked [][]string, degraded []string, fatal error) { +func partitionOutcomes(outcomes []outcome) (rankedIDs [][]string, degraded []string, fatal error) { for _, result := range outcomes { switch { // The one failure that is not the deployment's, and it cannot be @@ -173,10 +175,10 @@ func fold(outcomes []outcome) (ranked [][]string, degraded []string, fatal error case result.err != nil: degraded = append(degraded, string(result.mode)) default: - ranked = append(ranked, result.ids) + rankedIDs = append(rankedIDs, result.ids) } } - return ranked, degraded, nil + return rankedIDs, degraded, nil } // callersFilter re-labels the store's refusal of the caller's own attribute @@ -223,7 +225,7 @@ func callersFilter(query domain.SearchQuery, err error) error { // aSpatialOnlyIntentIsAnsweredRatherThanDegraded holds the two to one answer. func (s *SearchRepository) negotiate( modes []domain.Capability, -) (ranked []domain.Capability, filtering bool, degraded []string) { +) (rankedModes []domain.Capability, filtering bool, degraded []string) { declared := s.Capabilities() for _, mode := range modes { switch { @@ -239,10 +241,10 @@ func (s *SearchRepository) negotiate( // ran. degraded = append(degraded, string(mode)) default: - ranked = append(ranked, mode) + rankedModes = append(rankedModes, mode) } } - return ranked, filtering, degraded + return rankedModes, filtering, degraded } // retrieve fans the ranked modes out and waits for all of them — a barrier, not @@ -278,9 +280,5 @@ func pageOf(fused []string, offset, limit int) []string { if offset >= len(fused) { return nil } - end := offset + limit - if end > len(fused) { - end = len(fused) - } - return fused[offset:end] + return fused[offset:min(offset+limit, len(fused))] } diff --git a/tests/acceptance/suite_test.go b/tests/acceptance/suite_test.go index fecdd7f..905fa7d 100644 --- a/tests/acceptance/suite_test.go +++ b/tests/acceptance/suite_test.go @@ -20,7 +20,6 @@ import ( "net/http" "net/http/httptest" "reflect" - "strconv" "strings" "testing" "time" @@ -351,10 +350,7 @@ func (s *service) discoverOn(t *testing.T, networkID string, intent map[string]a // discoverPaged is discover with an explicit page size. // -// The page is a query parameter rather than a member of the intent: `limit` and -// `offset` are read off the URL, which is the one part of the transport the -// plan does not spell out. Named here rather than in each scenario so that if -// it moves into the body there is one call site to move. +// The page is `limit` in the message, beside the intent. // // Two scenarios need it because the default page is twenty and their fixtures // are forty: a scenario asserting on a count would otherwise be asserting on @@ -364,12 +360,12 @@ func (s *service) discoverPaged( ) []beckn.Catalog { t.Helper() - body := envelope(beckn.ActionDiscover, map[string]any{"intent": intent}) + body := envelope(beckn.ActionDiscover, map[string]any{"intent": intent, "limit": limit}) if networkID != "" { onNetwork(networkID)(nested(body, "context")) } - answer := s.post(t, "/discover?limit="+strconv.Itoa(limit), body) + answer := s.post(t, "/discover", body) if answer.status != http.StatusOK { t.Fatalf("POST /discover = %d, want 200\nbody: %s", answer.status, answer.body) } diff --git a/tests/testdata/beckn-v2.0.0.yaml b/tests/testdata/beckn-v2.0.0.yaml index 0a57179..95a8663 100644 --- a/tests/testdata/beckn-v2.0.0.yaml +++ b/tests/testdata/beckn-v2.0.0.yaml @@ -166,6 +166,24 @@ paths: const: discover message: $ref: '#/components/schemas/DiscoverAction' + examples: + paginatedDiscover: + summary: Second page of results, counted in resources (limit/offset) + value: + context: + version: '2.0.0' + action: discover + messageId: 770e8400-e29b-41d4-a716-446655440004 + transactionId: 770e8400-e29b-41d4-a716-446655440005 + timestamp: '2026-03-04T11:00:00.000Z' + bapId: bap.shoply.com + bapUri: https://bap.shoply.com + networkId: retail-network + message: + intent: + textSearch: premium laptop + limit: 3 + offset: 3 responses: '200': $ref: '#/components/responses/Ack' @@ -220,6 +238,54 @@ paths: const: on_discover message: $ref: '#/components/schemas/OnDiscoverAction' + examples: + paginatedOnDiscover: + summary: Second page of a result set paginated by resource. The + three resources span two catalogs, and CAT-TECHMART-2026 + continues from the previous page. + value: + context: + version: '2.0.0' + action: on_discover + messageId: 770e8400-e29b-41d4-a716-446655440004 + transactionId: 770e8400-e29b-41d4-a716-446655440005 + timestamp: '2026-03-04T11:00:02.000Z' + bapId: bap.shoply.com + bapUri: https://bap.shoply.com + bppId: bpp.techmart.com + bppUri: https://bpp.techmart.com + networkId: retail-network + message: + catalogs: + - id: CAT-TECHMART-2026 + descriptor: + name: TechMart Product Catalog + provider: + id: provider-techmart-001 + descriptor: + name: TechMart Electronics + resources: + - id: ITEM-LAPTOP-004 + descriptor: + name: Premium Laptop Pro 14 + - id: ITEM-LAPTOP-005 + descriptor: + name: Premium Laptop Pro 16 + - id: CAT-GADGETHUB-2026 + descriptor: + name: GadgetHub Catalog + provider: + id: provider-gadgethub-002 + descriptor: + name: GadgetHub + resources: + - id: ITEM-LAPTOP-006 + descriptor: + name: Premium Laptop Air + pagination: + total: 137 + limit: 3 + offset: 3 responses: '200': $ref: '#/components/responses/Ack' @@ -2290,6 +2356,24 @@ components: properties: intent: $ref: '#/components/schemas/Intent' + limit: + type: integer + minimum: 1 + description: >- + Maximum number of resources the DS SHOULD return in the + on_discover page, summed across the catalogs array. If omitted, + the DS applies its own default. No ceiling is fixed here because + the maximum page size is DS policy; a DS clamps an oversize + limit to its own maximum and reports the applied value in + pagination.limit. + offset: + type: integer + minimum: 0 + default: 0 + description: >- + Zero-based index, relative to the DS's full aggregated result + set, of the first resource the DS SHOULD return. Counted in + resources, not catalogs. additionalProperties: false required: - intent @@ -2375,6 +2459,8 @@ components: Array of catalogs matching the discovery request.' items: $ref: '#/components/schemas/Catalog' + pagination: + $ref: '#/components/schemas/DiscoverPagination' additionalProperties: false OnInitAction: @@ -4500,6 +4586,53 @@ components: ISO 8601 timestamp set by the CS each time the subscription record is modified. + DiscoverPagination: + type: object + description: "Pagination metadata accompanying an /on_discover callback,\ + \ describing the window of results returned relative to the DS's full\ + \ aggregated result set for the discovery intent.\n\nThe unit of pagination\ + \ is the resource, not the catalog: total, limit, and offset all count\ + \ entries of Catalog.resources summed across the catalogs array. A single\ + \ Catalog MAY therefore be split across consecutive pages, repeating its\ + \ id, provider, and bppId on each page while carrying a different subset\ + \ of its resources.\n\nProduced by the DS; consumed by the CN to determine\ + \ whether further pages exist and to construct the next /discover request.\ + \ Distinct from Pagination, which is produced by the CS for the deprecated\ + \ /catalog/search and /catalog/subscription endpoints." + properties: + total: + type: integer + minimum: 0 + description: >- + Number of resources reachable by paging, not the count in this + page. This is the count of matches the DS retained, which MAY be + capped at its retrieval depth: a DS that stops ranking after N + candidates reports at most N, because no page beyond that can be + fetched. A CN MUST therefore treat total as the end of what is + pageable and MUST NOT present it as an absolute match count. + limit: + type: integer + minimum: 1 + description: >- + Page size the DS actually applied, which MAY differ from the + limit the CN requested because the DS clamped it. This is a + ceiling, not a count: the last page carries fewer resources + than limit, and limit does not shrink to match. A CN that + wants the number of resources returned counts the catalogs + array; it has reached the end of the results when offset plus + that count equals total. + offset: + type: integer + minimum: 0 + description: >- + Zero-based index of the first resource in this page relative to + the DS's full aggregated result set. + required: + - total + - limit + - offset + additionalProperties: false + Pagination: type: object description: "Metadata accompanying a paginated list response that describes\