From 12b8405c1749e9c21159540603e9d36f401c8c0f Mon Sep 17 00:00:00 2001 From: Prajwal TD <157502711+tdprajwal@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:52:24 +0530 Subject: [PATCH 1/9] feat: carry discover pagination in the signed body [#51] A discover answered one page and said nothing about it. The page could only be asked for in the query string, which the request signature does not cover and which no Beckn caller knows to send, and the answer had no member to say which page it was or whether more existed - so a short page and the last page read the same. The validator now loads the OAN profile of beckn.yaml, pinned at commit dfc1097 of OpenAgriNet/protocol-specifications-v2 (no tag exists yet; swap the SHA for one when it is cut, never for the branch). It adds limit/offset to DiscoverAction and an optional pagination to OnDiscoverAction. Recorded as A28 in the plan. - The page is read from the body, each field falling back to the deprecated query parameter of the same name, so existing callers (verify.sh among them) keep working. The body wins because it is the signed half. - pagination reports the page APPLIED, after default and clamp: a caller that asked for 5000 must read back 100, or its next offset skips results. - total is len(fused) and is sent only when exact. It is omitted whenever any retrieval list came back as long as MaxCandidatesPerMode, because that list may have been cut by its LIMIT. Rejected: reviving the count query, which A19 deleted for costing 100x the search. Reverting the cap check makes TestATotalIsReportedOnlyWhenNoModeHitItsCap report 8 where 10 matched. - The past-depth refusal is unchanged and now points at $['message']['offset']. - config refuses maxPageSize above 100, the schema's maximum, which would otherwise answer query-string callers with a pagination.limit that fails its own schema. Not in scope: the same profile lets context.networkId be an array. beckn.Context still reads a string, so a list is refused by the envelope parser exactly as before; its meaning for scope and visibleTo is its own issue. --- README.md | 12 +-- docker-compose.yml | 5 +- docs/design/implementation-plan.md | 11 ++- docs/publish-and-discover.md | 9 ++- docs/quickstart.md | 10 ++- src/beckn/actions.go | 35 ++++++-- src/beckn/schema_conformance_test.go | 37 ++++++++- src/discover/controller.go | 32 +++++--- src/discover/controller_test.go | 97 ++++++++++++++++++++++ src/discover/intent_mapper.go | 7 +- src/discover/intent_mapper_test.go | 4 + src/discover/service.go | 24 +++--- src/discover/service_test.go | 62 ++++++++++---- src/domain/query.go | 10 ++- src/platform/config/config.go | 31 +++++-- src/platform/config/config_test.go | 7 ++ src/platform/validation/spec_index.go | 2 +- src/storage/conformance/discover.go | 25 +++++- src/storage/memory/search.go | 4 + src/storage/postgres/read_path_test.go | 27 +++++++ src/storage/postgres/search_repository.go | 24 +++++- tests/acceptance/pagination_test.go | 98 +++++++++++++++++++++++ tests/acceptance/suite_test.go | 12 ++- tests/testdata/beckn-v2.0.0.yaml | 80 ++++++++++++------ 24 files changed, 553 insertions(+), 112 deletions(-) create mode 100644 tests/acceptance/pagination_test.go diff --git a/README.md b/README.md index db5c19e..25ae2f5 100644 --- a/README.md +++ b/README.md @@ -69,8 +69,9 @@ That stack applies its own migrations (they are compiled into the binary) and reads the Beckn specification from `tests/testdata/beckn-v2.0.0.yaml`, mounted at the cache path. The boot logs one warning about the registry fetch it did not do, which is why it works with no network. That file is byte-identical to -the `core-v2.0.0-lts` tag of `beckn/protocol-specifications-v2`; set -`VALIDATION_SPEC_URL` to its raw URL to exercise the fetch path instead. +the OAN profile of the spec at commit `dfc1097` of +`OpenAgriNet/protocol-specifications-v2` (A28); set `VALIDATION_SPEC_URL` to its +raw URL to exercise the fetch path instead. With the stack up, there is a worked catalog and the requests that find it: @@ -118,9 +119,10 @@ overrides; that file is gitignored. **Secrets — `DATABASE_URL` above all — belong in neither YAML file.** The Beckn specification is fetched at boot from `VALIDATION_SPEC_URL` and -cached under `.cache/beckn/`. It defaults to the `core-v2.0.0-lts` tag of -`beckn/protocol-specifications-v2` — a tag rather than a branch, so an upstream -merge cannot change the validator under a running deployment. It is not baked +cached under `.cache/beckn/`. It defaults to the OAN profile at commit +`dfc1097` of `OpenAgriNet/protocol-specifications-v2` (A28) — a commit rather +than a branch, so an upstream merge cannot change the validator under a running +deployment. It is not baked into the image; air-gapped deploys mount a cache file at `VALIDATION_SPEC_CACHE_PATH`. diff --git a/docker-compose.yml b/docker-compose.yml index 99d9297..8316f1b 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,8 +81,9 @@ services: APP_DOMAIN: ${APP_DOMAIN:-Agriculture} volumes: # The Beckn spec, which the service refuses to boot without. Byte-identical - # to the core-v2.0.0-lts tag of beckn/protocol-specifications-v2, which is - # also VALIDATION_SPEC_URL's default — so a normal boot fetches, compiles + # to the OAN profile at commit dfc1097 of + # OpenAgriNet/protocol-specifications-v2 (A28), which is also + # VALIDATION_SPEC_URL's default — so a normal boot fetches, compiles # and finds this file already holding the same bytes, and an offline boot # falls back to it with one accurate warning about the fetch that failed. # diff --git a/docs/design/implementation-plan.md b/docs/design/implementation-plan.md index fb4fcc7..f875b66 100644 --- a/docs/design/implementation-plan.md +++ b/docs/design/implementation-plan.md @@ -151,6 +151,7 @@ field carrying **an array of network ids** (not `PUBLIC`/`PRIVATE`). | **A25** | **This service is one of four layers, and the plan was written as if it were all of them. Tasks 25 and 26 are added, and 23a and 23c grow one requirement each.** The other three layers — experience, network, provider — run **beckn-onix** adapters that already emit OTel traces, metrics and audit logs, and reading them changes four things here. **Two questions this plan could not answer are answered elsewhere, not nowhere.** Relevance and accuracy need what the user did *after* the response; the experience-layer adapter has the user and already emits traces, so that is where the outcome event belongs. Performance per location and per commodity is refused here because coordinates and `filters.expression` are on the never-emitted list — but the provider adapter holds both as first-class call-plan fields, so it answers them without anyone widening a deny-list. The H3-coarsening policy call that `opentelemetry.md` raised against 23d is **withdrawn** on those grounds. **Our spans would currently be exported and then silently dropped.** onix's network trace pipeline admits only spans carrying `sender.id`, which this phase makes optional and usually absent, while the stage downstream of it rebuilds `trace_id` from an attribute spelled `transaction_id`, which we planned to spell `beckn.transactionId`. A span that fails either is stored, paid for and invisible, with nothing anywhere reporting an error. The filter is onix's bug to fix — it admits on one key and joins on another — and the spelling is ours: **23c emits `transaction_id` and `message_id` as aliases**, written from the same values at the same call site so they cannot drift. Two, not three — an earlier draft named `receiver.id`, which onix puts only on audit log records; its span spelling is `recipient.id`, which we already emit, so a third alias would be paid for everywhere and read nowhere. Buying admission by inventing a `sender.id` is explicitly refused; missing from a dashboard is recoverable, poisoning the network's only participant-identity join is not. **Metrics were dropped from this service for a reason that covers only half the ground.** A23 removed `metrics.go` because a stateless replica's in-memory counter is a partial nobody can reassemble — true of the facilitator's windowed aggregates, and the reason Task 24 exists. It is not true of a *level*: three configured ceilings (`DATABASE_MAX_CONNS`, the rate limiter, the body size) have no observable distance-to-limit, and a pool at 30 of 32 refuses nothing, so it produces no span to be seen in. An earlier draft of this amendment said a 429 "produces no span, no event and no counter". **That is false, and the correction matters because it is what the task's scope was sized against**: `Trace` is index 1 of `router.go`'s `chain`, above both `Envelope` and `RateLimit`, so a 429 and a body refusal each produce a span carrying the status and — post-23d — an `error` event, both projected from the single `logNack` in `response_writer.go` that every refusal already passes through. Only "no counter" was ever true, and a counter that restates a span fact is the `duration_ms` mistake one signal up. **Task 25 is therefore the levels, not the refusals**, and it is smaller for it. Conflating the operator set with the network set is why this service has no metrics at all. **Task 25** is the operator set and is **not blocked**; Task 24 stays blocked on the registry — *five of its twelve codes have since shipped, on 2026-09-10, derived in `otel/collector.yaml`; the block stands for the other seven. See Task 24.* **And the deny-list is a rule nothing enforces** — five sub-tasks comply with it by hand. **Task 26** turns it into a pin, over the exporter's output rather than over the code that builds it | 23, 24, 25, 26 | | **A26** | **The publish geometry budget becomes configuration.** `MaxGeometriesPerCatalog` moves from a `const` in `src/publish/geometry.go` to `config.Geo.MaxGeometriesPerCatalog` / `GEO_MAX_GEOMETRIES_PER_CATALOG`, default **256**, threaded `container → publish.NewService → derive → ExtractGeometries → catalogWalk`. **The default does not change and no behaviour over the ceiling changes** — the extra finds are still `PARTIAL` faults naming their paths, never a silent drop. What changes is who gets to pick the number. **The failure it causes is the wrong shape to be a build-time constant.** Over the ceiling the publish still answers HTTP 200 and the publisher is told, but those shapes are not searchable, and at the operator that reads as missing data rather than as a limit: a 100 km `S_DWITHIN` returns fewer markets than exist and nothing in the response says why. Measured against a real catalog — 273 Maharashtra markets carrying a `Point` under both `coverageAreas` and `market.location`, so 546 shapes — only the first 128 markets were findable by place under 256. **Config rather than a larger constant, for the reason `ResolutionCells` two rows above already gives:** how many shapes a catalog carries is a property of a deployment's publishers, and the ceiling is reached by publishing normally rather than by abuse. That is also the line that decides which neighbours do *not* move: `MaxCatalogWalkDepth` bounds a stack against a pathological document, which is abuse, and `MaxIndexCoverCells` is a property of the index rather than of a catalog. **Zero is refused at boot**, alongside the body ceiling and for the same reason — it reads either as "unlimited" or as "refuse everything", and a catalog that may hold no geometry is undiscoverable by place. **The duplicate declaration in `src/indexing/geo/h3.go` is deleted, not rewired.** It had zero call sites; two constants of one name in two packages is how an operator raises the ceiling in the half that is not read. **What the number stands in for is cells, not shapes:** it times `MaxIndexCoverCells` (8,192) is the worst case one publish can ask the index to hold, so a points-only catalog — one cell per shape — can raise it a long way and a polygon-heavy one cannot | 2, 17, 18 | | **A27** | **A deployment may decline free-text retrieval, and declining REFUSES where a missing backend mode degrades.** `Search.EnableTextSearch` / `SEARCH_ENABLE_TEXT_SEARCH`, **true by default** — the value that takes no decision has to be the one that behaves as before the flag existed. With it false, an intent carrying a non-empty `textSearch` is refused in `MapIntent` with `SCH_TYPE_NOT_SUPPORTED` at `$['message']['intent']['textSearch']`, and the message names `spatial` and `filters` as what still works. **The whole content of this amendment is that it is not `FailOnUnavailableMode` under another name**, because the two read alike and behave oppositely. That flag is about a mode the **backend** cannot run — `semantic` on a `noop` deployment — and false there degrades, answering with what the other modes found and naming the gap in `X-Beckn-Degraded`; the query the caller wrote survives without it. This one is the **deployment's** policy, and degrading would be the failure C11 exists to prevent: on a text-only intent the term IS the query, so running the remaining modes without it answers **the whole corpus under a 200** — a page indistinguishable at the caller from a correct one. That is the same widening `MapIntent`'s doc comment already refuses for an unreadable `targets` or an ignored SRID, which is why the guard sits there and not in `negotiate`. **Three details are load-bearing.** The guard reads the **trimmed** term, like every other reader of it: `" "` asks for no retrieval mode at all, so it stays the no-criterion refusal and calling it a disabled text search would name the wrong mistake. The intent's required-one-of **list** shrinks with the flag — `spatial or filters`, with a parenthetical saying why the third is absent — because offering `textSearch` to a caller this deployment would then refuse walks them into a second `400`; the **guard** does not shrink with it, since a term that was sent is a criterion that was sent and reporting it missing would be a false sentence stacked on the true one. And the wire message names the two criteria that still work rather than the environment variable, matching `negotiate` and the `filters.type` refusal: a caller cannot act on the variable, and the operator has `docs/publish-and-discover.md`. **`modesFor` is left alone.** It still maps a non-empty `Text` to all three ranked modes, because the refusal is total and unreachable past it; a second guard there would be a branch no test can reach and the kind of defence that rots into a contradiction | 2, 17, 19 | +| **A28** | **The page rides in the signed body, and the answer says which page it is (#51).** The schema is now the **OAN profile** of `beckn.yaml`, pinned at commit `dfc1097` of `OpenAgriNet/protocol-specifications-v2` (`VALIDATION_SPEC_URL`'s default and `tests/testdata/beckn-v2.0.0.yaml`, byte-identical) — a commit because the branch carries no tag yet; it becomes a tag when one is cut, never the branch name. That profile adds `limit` (1-100) and `offset` (>= 0) to `DiscoverAction` and an optional `pagination` (`limit`, `offset`, `total`) to `OnDiscoverAction`. **Three decisions.** (1) The page is read from the **body**, each field falling back to the query parameter of the same name. The body wins field by field because it is the half the request signature covers; the query string survives only so callers written before the profile keep working, and it still clamps where the body is bounded by the schema — a body `limit` of 101 is an L1 refusal, not a clamp. (2) `pagination` reports the page **applied**, after the default and the clamp: a caller that asked for 5000 must read back the 100 it got, or it computes its next offset from a page size it never received. (3) `total` is sent **only when exact**, and is never a count query — **A19**'s reason for deleting that query (150.6 ms against a 1.5 ms search) stands. It is `len(fused)`, the number the search already holds, and it is omitted whenever any retrieval list came back as long as `MaxCandidatesPerMode`, because such a list may have been cut by its `LIMIT` and the fusion of it is a truncation. A list of exactly the cap that happened to be complete is reported uncounted too — the one false negative, and the safe direction: an absent total says *narrow the query*, a short one says *this is everything* when it is not. Degradation does not suppress it; that is `X-Beckn-Degraded`'s job, and the total is the size of the set this answer pages through. The memory backend has no cap, so its total is always present. **Unchanged:** `offset + limit > MaxCandidatesPerMode` is still refused (now at `$['message']['offset']`), never answered with an empty page. **Not taken here:** the same profile makes `context.networkId` a string or an array; `beckn.Context.NetworkID` is still a string, so a list is refused by the envelope parser exactly as before — its meaning for scope and `visibleTo` is a separate issue. Supersedes A19's "no total" for this one exact case, and the "exactly one field" note on `OnDiscoverAction` | 11, 16, 19 | A6 and A7 exist for one requirement: *swap the text backend later, keep geo on PG, and let publish write to two stores.* Both build **seams plus conformance @@ -2165,7 +2166,9 @@ POST /discover "provider": { ... }, // fetched once per catalog on the page "resources": [ ... ], // only the ones that matched "offers": [ ... ] // only those touching a matched - } ] } } // resource, plus catalog-wide ones + } ], // resource, plus catalog-wide ones + "pagination": { "limit": 20, "offset": 0, "total": 45 } // A28 + } } ``` ```http @@ -2173,9 +2176,9 @@ X-Beckn-Degraded: structured ``` The degraded list is a **header, not a body field** (C11). `OnDiscoverAction` -declares `additionalProperties: false` with `catalogs` as its only property, so -a `degraded` key inside `message` is not an extension — it is a response that -fails its own schema. The header is absent when nothing degraded. +declares `additionalProperties: false` with `catalogs` and `pagination` (A28) as +its only properties, so a `degraded` key inside `message` is not an extension — +it is a response that fails its own schema. The header is absent when nothing degraded. ### The flow diff --git a/docs/publish-and-discover.md b/docs/publish-and-discover.md index aa78591..113ede6 100644 --- a/docs/publish-and-discover.md +++ b/docs/publish-and-discover.md @@ -19,9 +19,10 @@ That is the whole of it: | `GET /readyz` | the platform | can it also reach PostgreSQL | Four routes, no wildcard mount, no aliases. Both APIs are implemented against -the published Beckn v2.0.0 specification — -[`beckn/protocol-specifications-v2`](https://github.com/beckn/protocol-specifications-v2/blob/core-v2.0.0-lts/api/v2.0.0/beckn.yaml) -at the `core-v2.0.0-lts` tag, pinned in this repo as +the Beckn v2.0.0 specification as OpenAgriNet profiles it — +[`OpenAgriNet/protocol-specifications-v2`](https://github.com/OpenAgriNet/protocol-specifications-v2/blob/dfc109759b3781fde74539adc79df63fccbadb83/api/v2.0.0/beckn.yaml) +at commit `dfc1097`, which adds discover pagination to beckn's `core-v2.0.0-lts` +(A28), pinned in this repo as [`tests/testdata/beckn-v2.0.0.yaml`](../tests/testdata/beckn-v2.0.0.yaml). That document *is* the validator: the service loads it and validates every request against it, rather than restating the protocol in Go. @@ -469,7 +470,7 @@ matches no field fails the boot, so a typo cannot silently do nothing. | `EMBEDDING_MODEL` / `_ENDPOINT` / `_DIMENSIONS` | `nomic-embed-text` / `http://localhost:11434` / `768` | the Ollama seam | | `RATE_LIMIT_RPS` / `_BURST` | `20` / `40` | per-client limit; `burst >= rps` is enforced | | `VALIDATION_ENABLE_L1_SCHEMA` | `true` | Beckn schema validation of the payload | -| `VALIDATION_SPEC_URL` | the `core-v2.0.0-lts` raw URL | where to fetch the spec; a tag, so it cannot move under a running deployment | +| `VALIDATION_SPEC_URL` | the OAN profile's raw URL at commit `dfc1097` | where to fetch the spec; a commit, so it cannot move under a running deployment | | `VALIDATION_SPEC_CACHE_PATH` | `.cache/beckn/beckn.yaml` | where the spec is read from | | `AUTH_ENABLE_SIGNATURE_VERIFICATION` | `false` | `true` refuses the boot — see below | | `EXT_ALLOW_NETWORK_FETCH` | `false` | may a URL from a *request body* be fetched | diff --git a/docs/quickstart.md b/docs/quickstart.md index 77ab38c..389532f 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -270,20 +270,22 @@ does not remove the requirement. It is fetched from `VALIDATION_SPEC_URL`, which now defaults to ``` -https://raw.githubusercontent.com/beckn/protocol-specifications-v2/refs/tags/core-v2.0.0-lts/api/v2.0.0/beckn.yaml +https://raw.githubusercontent.com/OpenAgriNet/protocol-specifications-v2/dfc109759b3781fde74539adc79df63fccbadb83/api/v2.0.0/beckn.yaml ``` and cached under `VALIDATION_SPEC_CACHE_PATH` (`.cache/beckn/beckn.yaml`). That is why running the binary from the host needs no spec configuration at all: it fetches once and reads the cache afterwards. -A **tag**, not a branch, which is the whole reason there can be a default. A +A **commit**, not a branch, which is the whole reason there can be a default. A branch would let an upstream merge change the validator under a running -deployment without a deploy; `refs/tags/core-v2.0.0-lts` cannot move. A network +deployment without a deploy; a commit cannot move. It is OpenAgriNet's profile +of the Beckn spec rather than beckn's `core-v2.0.0-lts` tag because the profile +adds discover pagination (A28), and it becomes a tag when one is cut. A network that trusts a different document still overrides it. Compose additionally mounts `tests/testdata/beckn-v2.0.0.yaml` straight into -the cache path. That file is byte-identical to the tag, so the two paths agree +the cache path. That file is byte-identical to that commit, so the two paths agree — and the mount is what lets the container stack come up with no network at all. If the fetch fails, the boot logs one loud warning and falls back to the cache; that warning is accurate and is not a failure. diff --git a/src/beckn/actions.go b/src/beckn/actions.go index 0692610..caea0e8 100644 --- a/src/beckn/actions.go +++ b/src/beckn/actions.go @@ -135,18 +135,37 @@ type CatalogStats struct { } // DiscoverAction is the `message` of a discover request. +// +// Limit and Offset are the OAN profile's addition (A28): the page rides in the +// signed body rather than the query string, which the request signature does +// not cover. Pointers, because absent and zero differ — an absent field falls +// back to the query string, and the schema refuses a zero limit outright. 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 -// 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). +// It must NOT grow a Degraded field. The schema is additionalProperties:false +// with `catalogs` and `pagination` as its only properties, 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 *Pagination `json:"pagination,omitempty"` +} + +// Pagination is the page a discover answer APPLIED — after the default and the +// clamp, not as requested — so the caller computes the next offset from what +// it actually got. +// +// Total is a pointer and omitted when nil: it is sent only when exact (A28). +// Offset is never omitted, because zero is the first page and not "unset". +type Pagination struct { + Limit int `json:"limit"` + Offset int `json:"offset"` + Total *int `json:"total,omitempty"` } diff --git a/src/beckn/schema_conformance_test.go b/src/beckn/schema_conformance_test.go index df5efd5..9ec5ef5 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")}, + {"Pagination", Pagination{}, schemaPath("Pagination")}, {"Nack", Nack{}, schemaPath("NackBadRequest")}, {"NackMessage", NackMessage{}, schemaPath("NackBadRequest", "properties", "message")}, @@ -134,7 +135,7 @@ func TestWireStructsMatchTheSpecification(t *testing.T) { var found []deviation for _, b := range bindings() { - found = append(found, deviations(t, b, resolve(t, spec, b.path))...) + found = append(found, deviations(t, b, flattenAllOf(t, spec, resolve(t, spec, b.path)))...) } sort.Slice(found, func(i, j int) bool { return found[i].key < found[j].key }) @@ -157,6 +158,40 @@ func TestWireStructsMatchTheSpecification(t *testing.T) { } } +// flattenAllOf merges the properties of an allOf node's members into one +// properties map, following each member's local $ref. Pagination is the first +// bound schema composed this way — allOf[PaginationParams, {total}] in the OAN +// profile — and a walk that read only the top-level `properties` would find +// none and fail the binding rather than check it. A node without allOf comes +// back unchanged. +func flattenAllOf(t *testing.T, spec map[string]any, node map[string]any) map[string]any { + t.Helper() + + members, ok := node["allOf"].([]any) + if !ok { + return node + } + + merged := map[string]any{} + for _, member := range members { + part, ok := member.(map[string]any) + if !ok { + t.Fatalf("an allOf member is not an object: %v", member) + } + if ref, isRef := part["$ref"].(string); isRef { + part = resolve(t, spec, strings.Split(strings.TrimPrefix(ref, "#/"), "/")) + } + properties, ok := flattenAllOf(t, spec, part)["properties"].(map[string]any) + if !ok { + t.Fatalf("an allOf member declares no properties: %v", member) + } + for name, schema := range properties { + merged[name] = schema + } + } + return map[string]any{"properties": merged} +} + // deviations compares one struct's JSON tags against its schema node's // properties, in both directions. An extra tag ships a key the schema does not // declare; a missing one drops a key the schema does, which for a service that diff --git a/src/discover/controller.go b/src/discover/controller.go index 45f6691..57d2a98 100644 --- a/src/discover/controller.go +++ b/src/discover/controller.go @@ -68,19 +68,19 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { observeIntent(ctx, envelope.Context, action.Intent) - page, err := pageFrom(r.URL.Query()) + page, err := pageFrom(action, r.URL.Query()) if err != nil { httpx.WriteNack(ctx, w, c.errors, envelope.Context.MessageID, err) return } - 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,20 +90,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. +// pageFrom reads the page off the body, field by field falling back to the +// query string (A28). // -// 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) { +// The body is the signed half, so a field it names wins. The query string is +// the deprecated route and stays only so that callers written before the OAN +// profile added limit and offset to DiscoverAction keep working; the schema +// already bounds the body's values, which is why only the query's are parsed +// here. +// +// An unreadable query 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(action beckn.DiscoverAction, query url.Values) (Page, error) { limit, err := intParam(query, "limit") if err != nil { return Page{}, err @@ -112,6 +118,12 @@ func pageFrom(query url.Values) (Page, error) { if err != nil { return Page{}, err } + if action.Limit != nil { + limit = *action.Limit + } + if action.Offset != nil { + offset = *action.Offset + } return Page{Limit: limit, Offset: offset}, nil } diff --git a/src/discover/controller_test.go b/src/discover/controller_test.go index c14d147..11e6aee 100644 --- a/src/discover/controller_test.go +++ b/src/discover/controller_test.go @@ -412,3 +412,100 @@ func TestARefusedOperatorIsA400(t *testing.T) { t.Errorf("the backend was searched %d times; a refused intent runs no query", repo.calls) } } + +// The page travels in the SIGNED body now (A28). The OAN profile of the schema +// declares limit and offset on DiscoverAction, so a caller no longer needs the +// query string — which the request signature does not cover. +func TestLimitAndOffsetAreReadFromTheBody(t *testing.T) { + repo := &stubRepo{capabilities: everything()} + + recorder := post(t, mount(t, repo, settings()), "/discover", + `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"limit":5,"offset":10}}`) + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", recorder.Code, recorder.Body) + } + if repo.gotQuery.Limit != 5 || repo.gotQuery.Offset != 10 { + t.Errorf("page = limit %d offset %d, want the body's 5 and 10", repo.gotQuery.Limit, repo.gotQuery.Offset) + } +} + +// Each field the body names wins over the same field in the query string, and a +// field the body omits still falls back to it. The body is the signed half, so +// it is the one to believe when the two disagree. +func TestTheBodyWinsOverTheQueryStringFieldByField(t *testing.T) { + repo := &stubRepo{capabilities: everything()} + + recorder := post(t, mount(t, repo, settings()), "/discover?limit=7&offset=40", + `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"offset":10}}`) + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", recorder.Code, recorder.Body) + } + if repo.gotQuery.Offset != 10 { + t.Errorf("Offset = %d, want the body's 10 over the query's 40", repo.gotQuery.Offset) + } + if repo.gotQuery.Limit != 7 { + t.Errorf("Limit = %d, want the query's 7 — the body named none", repo.gotQuery.Limit) + } +} + +// The answer says which page it is: the limit and offset actually APPLIED, after +// the default and the clamp, and the total when the backend could count it. +func TestTheResponseReportsThePageItApplied(t *testing.T) { + total := 3 + repo := &stubRepo{ + capabilities: everything(), + result: domain.SearchResult{Catalogs: []domain.Catalog{{ID: "c1"}}, Total: &total}, + } + + message := onDiscover(t, post(t, mount(t, repo, settings()), "/discover?limit=1000&offset=4", wheat)) + + want := beckn.Pagination{Limit: settings().Search.MaxPageSize, Offset: 4} + if message.Pagination == nil { + t.Fatal("pagination is absent, want the applied page") + } + if got := *message.Pagination; got.Limit != want.Limit || got.Offset != want.Offset || + got.Total == nil || *got.Total != total { + t.Errorf("pagination = %+v (total %v), want limit %d offset %d total %d", + got, got.Total, want.Limit, want.Offset, total) + } +} + +// A total the backend could not count exactly is left OUT, not sent as zero or +// as a guess. A caller reads its absence as "more matched than a page walk can +// reach"; a zero would read as "nothing matched". +func TestAnUncountedTotalIsAbsentFromTheWire(t *testing.T) { + repo := &stubRepo{capabilities: everything(), result: domain.SearchResult{Total: nil}} + + recorder := post(t, mount(t, repo, settings()), "/discover", wheat) + envelope := decodeResponse(t, recorder) + + var message struct { + Pagination map[string]any `json:"pagination"` + } + if err := json.Unmarshal(envelope.Message, &message); err != nil { + t.Fatalf("decoding the message: %v", err) + } + pagination := message.Pagination + if pagination == nil { + t.Fatal("pagination is absent, want the applied page without a total") + } + if _, ok := pagination["total"]; ok { + t.Errorf("pagination = %v, want no total key", pagination) + } + if offset, ok := pagination["offset"]; !ok || offset != float64(0) { + t.Errorf("pagination = %v, want offset 0 present — zero is a real offset", pagination) + } +} + +func onDiscover(t *testing.T, recorder *httptest.ResponseRecorder) beckn.OnDiscoverAction { + t.Helper() + + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", recorder.Code, recorder.Body) + } + var message beckn.OnDiscoverAction + if err := json.Unmarshal(decodeResponse(t, recorder).Message, &message); err != nil { + t.Fatalf("decoding the message: %v", err) + } + return message +} diff --git a/src/discover/intent_mapper.go b/src/discover/intent_mapper.go index 5390f09..d23f76a 100644 --- a/src/discover/intent_mapper.go +++ b/src/discover/intent_mapper.go @@ -14,8 +14,9 @@ import ( "github.com/OpenAgriNet/discovery-service/src/platform/jsonpath" ) -// Page is the request's pagination, which arrives as HTTP query parameters -// rather than inside the intent. A struct rather than two adjacent ints, because +// Page is the request's pagination: DiscoverAction's limit and offset, each +// falling back to the deprecated query parameter of the same name (A28). Zero +// means "the default". A struct rather than two adjacent ints, because // `limit, offset` and `offset, limit` compile identically. type Page struct { Limit int @@ -229,7 +230,7 @@ func mapPage(page Page, search config.Search) (limit, offset int, faults []domai if offset+limit > search.MaxCandidatesPerMode { faults = append(faults, domain.Fault{ - Path: "$['offset']", + Path: "$['message']['offset']", Code: string(beckn.CodeSchemaInvalidFormat), Message: fmt.Sprintf( "offset %d plus limit %d passes the retrieval depth of %d", diff --git a/src/discover/intent_mapper_test.go b/src/discover/intent_mapper_test.go index 08e8550..2f512f8 100644 --- a/src/discover/intent_mapper_test.go +++ b/src/discover/intent_mapper_test.go @@ -324,6 +324,10 @@ func TestLimitIsClampedAndAPagePastTheRetrievalDepthIsRefused(t *testing.T) { if !strings.Contains(fatal[0].Message, "500") { t.Errorf("Message = %q, want it to name the boundary", fatal[0].Message) } + // The page is a body field now (A28), so the fault points into the body. + if fatal[0].Path != "$['message']['offset']" { + t.Errorf("Path = %q, want $['message']['offset']", fatal[0].Path) + } } // NetworkID is NOT set here. The service sets it from the envelope, and empty diff --git a/src/discover/service.go b/src/discover/service.go index 2960c12..2fc0e0a 100644 --- a/src/discover/service.go +++ b/src/discover/service.go @@ -33,15 +33,16 @@ func NewService(repo domain.SearchRepository, cfg config.Config) *Service { return &Service{repo: repo, cfg: cfg} } -// Discover answers one intent with one page of catalogs and the retrieval modes -// that did not contribute. The degraded list is returned beside the catalogs -// rather than inside them, because it reaches the caller as a header (C11). +// Discover answers one intent with one page of catalogs, the page it applied, +// and the retrieval modes that did not contribute. The degraded list is +// returned beside the message rather than inside it, 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 +55,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 +69,12 @@ 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), + // What MapIntent applied, not what was asked: a caller that sent 5000 + // must read back the 100 it got, or its next offset skips results. + Pagination: &beckn.Pagination{Limit: query.Limit, Offset: query.Offset, Total: result.Total}, + }, degraded, nil } // typedSearchFailure says whose mistake a failed search was. @@ -84,7 +90,7 @@ func typedSearchFailure(ctx context.Context, err error) error { if errors.Is(err, domain.ErrRetrievalDepth) { return apperrors. Schema(beckn.CodeSchemaInvalidFormat, err.Error()). - At(jsonpath.Dot("$['offset']")) + At(jsonpath.Dot("$['message']['offset']")) } // An expression the store's own parser refused is the caller's mistake too, diff --git a/src/discover/service_test.go b/src/discover/service_test.go index 6ca6e73..4dfecf9 100644 --- a/src/discover/service_test.go +++ b/src/discover/service_test.go @@ -99,14 +99,14 @@ 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{}) if err != nil { t.Fatalf("Discover: %v", err) } - if len(catalogs) != 1 { - t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(catalogs)) + if len(answer.Catalogs) != 1 { + t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(answer.Catalogs)) } if !slices.Equal(degraded, []string{string(domain.CapabilitySemantic)}) { t.Errorf("degraded = %v, want [semantic]", degraded) @@ -181,7 +181,7 @@ 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, @@ -192,8 +192,8 @@ func TestSpatialStillAnswersWithTextSearchOff(t *testing.T) { t.Fatalf("Discover: %v", err) } - if len(catalogs) != 1 { - t.Errorf("catalogs = %d, want the one the backend found", len(catalogs)) + if len(answer.Catalogs) != 1 { + t.Errorf("catalogs = %d, want the one the backend found", len(answer.Catalogs)) } if hasMode(repo.gotModes, domain.CapabilityLexical) { t.Errorf("modes = %v, want no ranked text mode — nothing asked for one", repo.gotModes) @@ -214,7 +214,7 @@ 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")`, @@ -224,8 +224,8 @@ func TestAFilterABackendCannotRunIsDegradedRatherThanIgnored(t *testing.T) { t.Fatalf("Discover: %v", err) } - if len(catalogs) != 1 { - t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(catalogs)) + if len(answer.Catalogs) != 1 { + t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(answer.Catalogs)) } if !slices.Equal(degraded, []string{string(domain.CapabilityJSONPath)}) { t.Errorf("degraded = %v, want [jsonpath] — a filter this backend cannot run and "+ @@ -512,11 +512,12 @@ 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{}) if err != nil { t.Fatalf("Discover: %v", err) } + catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Offers) != 1 { t.Fatalf("rendered = %+v, want one catalog carrying one offer", catalogs) } @@ -576,11 +577,12 @@ 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{}) if err != nil { t.Fatalf("Discover: %v", err) } + catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Resources) != 1 || catalogs[0].Resources[0].ID != "wheat" { t.Fatalf("rendered = %+v, want only the matched resource", catalogs) @@ -657,11 +659,12 @@ 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{}) if err != nil { t.Fatalf("Discover: %v", err) } + catalogs := answer.Catalogs if len(catalogs) != 0 { t.Errorf("catalogs = %+v, want none", catalogs) } @@ -679,11 +682,12 @@ 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{}) if err != nil { t.Fatalf("Discover: %v", err) } + catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Resources) != 1 || catalogs[0].Resources[0].ID != "r2" { t.Fatalf("resources = %+v, want only r2 — r1's document could not be read", catalogs[0].Resources) } @@ -700,11 +704,12 @@ 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{}) if err != nil { t.Fatalf("Discover: %v", err) } + catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Offers) != 1 || catalogs[0].Offers[0].ID != "o2" { t.Fatalf("offers = %+v, want only o2 — o1's document could not be read", catalogs[0].Offers) } @@ -808,3 +813,32 @@ func TestAMalformedFilterExpressionIsTheCallersFaultNotAFiveHundred(t *testing.T "internal that must not reach the caller", fault.Message, want) } } + +// The answer carries the page the service APPLIED — the default when none was +// asked for, the clamp when too much was — beside the backend's total. A caller +// that sent limit 5000 must read 100 back, or it will compute the next offset +// from a page size it never got. +func TestTheAnswerReportsTheAppliedPageAndTheBackendsTotal(t *testing.T) { + total := 42 + repo := &stubRepo{capabilities: everything(), result: domain.SearchResult{Total: &total}} + cfg := settings() + + answer, _, err := discover.NewService(repo, cfg).Discover( + t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, + discover.Page{Limit: 5000, Offset: 20}) + if err != nil { + t.Fatalf("Discover: %v", err) + } + + page := answer.Pagination + if page == nil { + t.Fatal("pagination is absent") + } + if page.Limit != cfg.Search.MaxPageSize || page.Offset != 20 { + t.Errorf("pagination = limit %d offset %d, want the clamped %d and 20", + page.Limit, page.Offset, cfg.Search.MaxPageSize) + } + if page.Total == nil || *page.Total != total { + t.Errorf("total = %v, want the backend's %d", page.Total, total) + } +} diff --git a/src/domain/query.go b/src/domain/query.go index f6270e4..c9938f0 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 size of the fused candidate set the page was sliced from, and +// nil whenever that size is not the number that matched — when a retrieval mode +// returned as many ids as its cap, it may have stopped early (A28). It is never +// a count query: A19 deleted that for costing 100x the search it accompanied, +// and this is the number the search already holds. 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..2a9ad7e 100644 --- a/src/platform/config/config.go +++ b/src/platform/config/config.go @@ -219,19 +219,24 @@ 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 - // 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, + // The default is IMMUTABLE, 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 - // 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. + // deployment without a deploy. A tag or a commit cannot move, so the default + // names one 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. + // + // It is the OAN profile at a COMMIT, not beckn's core-v2.0.0-lts tag (A28): + // the profile adds limit/offset to DiscoverAction and pagination to + // OnDiscoverAction, and its branch carries no tag yet. Swap the SHA for a + // tag when one is cut — never for the branch name. // // 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/dfc109759b3781fde74539adc79df63fccbadb83/api/v2.0.0/beckn.yaml"` // Where the fetched spec is cached, and what an air-gapped deployment // mounts in place of the fetch. @@ -633,9 +638,19 @@ func validateDatabase(database Database) error { ) } +// schemaMaxPageSize is PaginationParams.limit's `maximum` in the OAN profile of +// the spec (A28). A copy rather than a read of the document, because config +// validates before the spec index is built; the two drifting apart shows up as +// a boot refusal here or as a schema failure on the response, never silently. +const schemaMaxPageSize = 100 + func validateSearch(search Search) error { return errors.Join( require(search.DefaultPageSize > 0, "search.defaultPageSize %d is not positive", search.DefaultPageSize), + require(search.MaxPageSize <= schemaMaxPageSize, + "search.maxPageSize %d is above the schema's limit maximum of %d — a body asking for more is refused "+ + "before the clamp, and a query-string caller would be answered with a pagination.limit that fails its own schema", + search.MaxPageSize, schemaMaxPageSize), require(search.MaxPageSize >= search.DefaultPageSize, "search.maxPageSize %d is below search.defaultPageSize %d — a ceiling under the value it clamps", search.MaxPageSize, search.DefaultPageSize), diff --git a/src/platform/config/config_test.go b/src/platform/config/config_test.go index 6e93eb5..ac33bf2 100644 --- a/src/platform/config/config_test.go +++ b/src/platform/config/config_test.go @@ -377,6 +377,13 @@ func TestValidateRejects(t *testing.T) { "a candidate pool smaller than one page": { "search:\n maxPageSize: 100\n maxCandidatesPerMode: 50\n", "maxCandidatesPerMode", }, + // The schema caps a page at 100 (A28). Above it, a body asking for more + // is refused before the clamp is reached, and a query-string caller is + // answered with a pagination.limit that fails the response's own + // schema. + "a max page size above the schema's maximum": { + "search:\n maxPageSize: 101\n maxCandidatesPerMode: 500\n", "maxPageSize", + }, "an unloadable timezone": { "app:\n defaultTimezone: Mars/Olympus_Mons\n", "defaultTimezone", }, diff --git a/src/platform/validation/spec_index.go b/src/platform/validation/spec_index.go index eb7cc84..3368ef2 100644 --- a/src/platform/validation/spec_index.go +++ b/src/platform/validation/spec_index.go @@ -241,7 +241,7 @@ func loadFromCache(path string) (*SpecIndex, error) { // ages with the image), so an air-gapped deploy supplies one at // VALIDATION_SPEC_CACHE_PATH, and mounting it read-only is the right way to // supply a file the service must not be able to rewrite. docker-compose.yml does -// exactly that with a copy byte-identical to the core-v2.0.0-lts tag, and every +// exactly that with a copy byte-identical to VALIDATION_SPEC_URL's default, and every // boot with a reachable registry warned that it could not overwrite those bytes // with themselves. A warning that fires when nothing is wrong is one nobody reads // on the boot when something is. diff --git a/src/storage/conformance/discover.go b/src/storage/conformance/discover.go index 5d97b95..2041a33 100644 --- a/src/storage/conformance/discover.go +++ b/src/storage/conformance/discover.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "slices" + "strconv" "testing" "time" @@ -214,6 +215,15 @@ func pageOrder(result domain.SearchResult) []string { return ids } +// totalOf renders a result's total for a failure message, where a nil pointer +// would print as an address. +func totalOf(result domain.SearchResult) string { + if result.Total == nil { + return "absent" + } + return strconv.Itoa(*result.Total) +} + // offerIDs is every offer hydrated onto the page, sorted. func offerIDs(result domain.SearchResult) []string { ids := make([]string, 0) @@ -642,10 +652,17 @@ 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. + // The offset SLICES a stable order rather than re-ranking it, so + // page two holds the next two and not two of the same. + + // And both pages report the same exact total (A28): five matched, + // none of them capped, so the count is the size of the set being + // paged — whichever page asked. + for name, result := range map[string]domain.SearchResult{"one": first, "two": second} { + if result.Total == nil || *result.Total != len(resources) { + t.Errorf("page %s reports total %v, want %d", name, totalOf(result), len(resources)) + } + } }, } } diff --git a/src/storage/memory/search.go b/src/storage/memory/search.go index 1063c9e..e62efb0 100644 --- a/src/storage/memory/search.go +++ b/src/storage/memory/search.go @@ -62,9 +62,13 @@ func (r *Repository) Search( page = nil } + // This backend retrieves without a cap, so the ranked list is every match + // and its length is always the exact total. + total := len(page) return domain.SearchResult{ Catalogs: r.hydrate(pageOf(page, query.Offset, query.Limit), scope), Degraded: degraded, + Total: &total, }, nil } diff --git a/src/storage/postgres/read_path_test.go b/src/storage/postgres/read_path_test.go index aee05d2..cb54192 100644 --- a/src/storage/postgres/read_path_test.go +++ b/src/storage/postgres/read_path_test.go @@ -380,6 +380,33 @@ func TestPageTwoDoesNotOverlapPageOne(t *testing.T) { } } +// A mode that returned as many ids as its cap may have stopped early, so the +// fused list is a truncation and its length is not the number that matched. +// The total is then left OUT (A28) rather than reported short — this is the +// only backend with a cap, which is why the case lives here and not in the +// conformance suite. searchConfig's cap is 8; ten lots match. +func TestATotalIsReportedOnlyWhenNoModeHitItsCap(t *testing.T) { + repository, _ := publish(t, nil, readFixture{catalog: "cat-capped", resources: kharifLots(10)}) + + capped := search(t, repository, domain.SearchQuery{Text: "kharif", Limit: 4}) + if capped.Total != nil { + t.Errorf("total = %d with every mode at its cap of %d, want it absent", + *capped.Total, searchConfig().MaxCandidatesPerMode) + } +} + +func TestATotalUnderTheCapIsExact(t *testing.T) { + repository, _ := publish(t, nil, readFixture{catalog: "cat-counted", resources: kharifLots(6)}) + + result := search(t, repository, domain.SearchQuery{Text: "kharif", Limit: 4}) + if result.Total == nil || *result.Total != 6 { + t.Errorf("total = %v, want exactly 6", result.Total) + } + if got := len(matchedIDs(result)); got != 4 { + t.Errorf("the page holds %d, want 4 — the total counts the set, not the page", got) + } +} + // A mode that ERRORS is degraded, and the page is what the other modes found. // // The vectors are 768 wide because the column is, and the repository embeds at diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index 8ba65af..8375759 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -128,7 +128,29 @@ func (s *SearchRepository) Search( if err != nil { return domain.SearchResult{}, err } - return domain.SearchResult{Catalogs: catalogs, Degraded: degraded}, nil + return domain.SearchResult{ + Catalogs: catalogs, + Degraded: degraded, + Total: s.exactTotal(lists, fused), + }, nil +} + +// exactTotal is len(fused) when that is the number that matched, and nil when +// it is not (A28). +// +// A list as long as MaxCandidatesPerMode may have been cut by its LIMIT, so the +// fusion of it is a truncation. A list of exactly the cap that happened to be +// complete is reported as uncounted too — the one false negative, and the safe +// direction: an absent total says "narrow the query", a short one says "this is +// everything" when it is not. +func (s *SearchRepository) exactTotal(lists [][]string, fused []string) *int { + for _, ids := range lists { + if len(ids) >= s.search.MaxCandidatesPerMode { + return nil + } + } + total := len(fused) + return &total } // withinRetrievalDepth refuses a page past what any mode retrieves. Checked diff --git a/tests/acceptance/pagination_test.go b/tests/acceptance/pagination_test.go new file mode 100644 index 0000000..a11ca6c --- /dev/null +++ b/tests/acceptance/pagination_test.go @@ -0,0 +1,98 @@ +package acceptance + +import ( + "fmt" + "net/http" + "slices" + "testing" + + "github.com/OpenAgriNet/discovery-service/src/beckn" +) + +// fiveAtMajestic publishes five resources a spatial intent around Majestic +// matches, so a page size of two has to walk three pages to see them all. +func fiveAtMajestic(t *testing.T) *service { + t.Helper() + + list := make([]map[string]any, 0, 5) + for index := range 5 { + list = append(list, aResource(fmt.Sprintf("r-%d", index), fmt.Sprintf("Lot %d", index))) + } + svc := newService(t) + svc.publishCatalogs(t, aCatalog("c-lots", availableAt(majestic), resources(list...))) + return svc +} + +// pageAt asks for one page with limit and offset in the SIGNED body (A28), and +// returns the message so the scenario can read the pagination beside the +// catalogs. +func (s *service) pageAt(t *testing.T, limit, offset int) beckn.OnDiscoverAction { + t.Helper() + + answer := s.post(t, "/discover", envelope(beckn.ActionDiscover, map[string]any{ + "intent": spatial(dwithin(providerGeoPath, majestic, 5000)), + "limit": limit, + "offset": offset, + })) + if answer.status != http.StatusOK { + t.Fatalf("POST /discover limit %d offset %d = %d, want 200 — the OAN profile "+ + "declares both on DiscoverAction\nbody: %s", limit, offset, answer.status, answer.body) + } + + var body struct { + Message beckn.OnDiscoverAction `json:"message"` + } + answer.decode(t, &body) + return body.Message +} + +// A caller walking pages in the body reaches every match exactly once, and +// every page says where it is: the page it applied, and a total that tells the +// last page from a short one. Before A28 the body could not carry the page at +// all — the schema refused the key — and the answer carried nothing but +// catalogs, so a short page and the end were the same thing. +func TestTheBodyPagesThroughEveryMatchAndTheTotalMarksTheEnd(t *testing.T) { + svc := fiveAtMajestic(t) + + var seen []string + for offset := 0; offset < 5; offset += 2 { + message := svc.pageAt(t, 2, offset) + + page := message.Pagination + if page == nil { + t.Fatalf("offset %d: no pagination in the answer", offset) + } + if page.Limit != 2 || page.Offset != offset { + t.Errorf("offset %d: pagination reports limit %d offset %d, want 2 and %d", + offset, page.Limit, page.Offset, offset) + } + if page.Total == nil || *page.Total != 5 { + t.Errorf("offset %d: total = %v, want 5", offset, page.Total) + } + seen = append(seen, resourceIDs(message.Catalogs)...) + } + + slices.Sort(seen) + if want := []string{"r-0", "r-1", "r-2", "r-3", "r-4"}; !slices.Equal(seen, want) { + t.Errorf("walking the pages saw %v, want each of %v exactly once", seen, want) + } +} + +// The schema bounds the body's page, so a limit above its maximum is refused +// by L1 validation before the service clamps anything. The query string still +// clamps — it is outside the schema — which is one more reason it is the +// deprecated route. +func TestALimitAboveTheSchemaMaximumIsRefused(t *testing.T) { + svc := fiveAtMajestic(t) + + answer := svc.post(t, "/discover", envelope(beckn.ActionDiscover, map[string]any{ + "intent": spatial(dwithin(providerGeoPath, majestic, 5000)), + "limit": 101, + })) + if answer.status != http.StatusBadRequest { + t.Fatalf("limit 101 answered %d, want 400\nbody: %s", answer.status, answer.body) + } + if code := answer.nack(t).Message.Error.Code; code != beckn.CodeSchemaValidationFailed { + t.Errorf("the refusal is %s, want %s", code, beckn.CodeSchemaValidationFailed) + } +} diff --git a/tests/acceptance/suite_test.go b/tests/acceptance/suite_test.go index fecdd7f..701d7f0 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,9 @@ 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 a member of DiscoverAction, beside the intent rather than inside +// it (A28). It moved there from the query string, and this was the one call +// site to move. // // 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 +362,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..72a8386 100644 --- a/tests/testdata/beckn-v2.0.0.yaml +++ b/tests/testdata/beckn-v2.0.0.yaml @@ -2290,6 +2290,10 @@ components: properties: intent: $ref: '#/components/schemas/Intent' + limit: + $ref: '#/components/schemas/PaginationParams/properties/limit' + offset: + $ref: '#/components/schemas/PaginationParams/properties/offset' additionalProperties: false required: - intent @@ -2375,6 +2379,8 @@ components: Array of catalogs matching the discovery request.' items: $ref: '#/components/schemas/Catalog' + pagination: + $ref: '#/components/schemas/Pagination' additionalProperties: false OnInitAction: @@ -2983,12 +2989,22 @@ components: type: string format: uuid networkId: - description: "The unique identifier of the Beckn subnet within which this\ - \ transaction is occurring. Actors use this value to resolve the subnet's\ - \ registry URL before performing identity and address lookups. The RECOMMENDED\ - \ format is namespace_id followed by a slash followed by registry_id, for\ - \ example acmenet.org/charge-net." - type: string + description: "The unique identifier(s) of the Beckn subnet(s) within which\ + \ this transaction is occurring. Actors use this value to resolve the\ + \ subnet's registry URL before performing identity and address lookups.\ + \ The RECOMMENDED format is namespace_id followed by a slash followed\ + \ by registry_id, for example acmenet.org/charge-net.\n\nMAY be a single\ + \ string (single-network transaction) or an array of strings (multi-network\ + \ transaction). When an array is provided, order is significant: actors\ + \ MUST attempt registry resolution starting with the first entry, treating\ + \ it as the primary network, and MAY fall back to subsequent entries if\ + \ resolution against the primary fails." + oneOf: + - type: string + - type: array + items: + type: string + minItems: 1 timestamp: description: The date and time at which this message was generated by the sender, expressed in RFC 3339 date-time format. The receiving actor MAY @@ -4500,36 +4516,50 @@ components: ISO 8601 timestamp set by the CS each time the subscription record is modified. - Pagination: + PaginationParams: type: object - description: "Metadata accompanying a paginated list response that describes\ - \ the size, position, and count of the returned result set, enabling callers\ - \ to navigate through large result sets using offset-based pagination.\n\n\ - Pagination appears in list responses such as /catalog/subscription and /catalog/search.\ - \ The DS (Discovery Service) and CS (Cataloging Service) produce it; the CN\ - \ or PN consumes it to determine whether additional pages are available and\ - \ to construct subsequent requests.\n\nRelationship: Composed within the response\ - \ message objects of /catalog/subscription and /catalog/search endpoints." + description: >- + Reusable offset-based pagination fields shared between paginated + list requests and the Pagination response metadata that describes + them. On a request, limit/offset specify the caller's desired page; + on a response, they reflect the page actually applied by the server. properties: - total: - type: integer - minimum: 0 - description: >- - Total number of matching records across the entire result set, - not the count in this page. Assigned by the CS. limit: type: integer minimum: 1 + maximum: 100 description: >- - Maximum number of records returned in this page. Assigned by - the CS based on the request parameter or CS policy. + Maximum number of records in this page. On a request, the + caller's desired page size; if omitted, the server applies a + default. On a response, the page size actually applied. offset: type: integer minimum: 0 + default: 0 description: >- Zero-based index of the first record in this page relative to - the full result set. Assigned by the CS based on the request - parameter. + the full result set. On a request, specifies where to start; + on a response, reflects the offset actually applied. + + Pagination: + description: "Metadata accompanying a paginated list response that describes\ + \ the size, position, and count of the returned result set, enabling callers\ + \ to navigate through large result sets using offset-based pagination.\n\n\ + Pagination appears in list responses such as /catalog/subscription and /catalog/search.\ + \ The DS (Discovery Service) and CS (Cataloging Service) produce it; the CN\ + \ or PN consumes it to determine whether additional pages are available and\ + \ to construct subsequent requests.\n\nRelationship: Composed within the response\ + \ message objects of /catalog/subscription and /catalog/search endpoints." + allOf: + - $ref: '#/components/schemas/PaginationParams' + - type: object + properties: + total: + type: integer + minimum: 0 + description: >- + Total number of matching records across the entire result + set, not the count in this page. Assigned by the CS. CatalogSearchAction: type: object From 8724d06ba8694e4009761b3fd824897534839d08 Mon Sep 17 00:00:00 2001 From: Prajwal TD <157502711+tdprajwal@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:41:04 +0530 Subject: [PATCH 2/9] feat: read discover pagination from the request body [#51] Pagination now comes only from message.limit and message.offset; the query string is no longer read. The response returns the applied page as message.pagination. --- README.md | 12 +-- docker-compose.yml | 5 +- docs/design/implementation-plan.md | 11 +-- docs/publish-and-discover.md | 9 +- docs/quickstart.md | 10 +- examples/verify.sh | 11 ++- src/beckn/actions.go | 20 ++-- src/discover/controller.go | 55 +---------- src/discover/controller_test.go | 113 ++-------------------- src/discover/intent_mapper.go | 7 +- src/discover/intent_mapper_test.go | 4 - src/discover/service.go | 4 +- src/discover/service_test.go | 54 +++-------- src/domain/query.go | 10 +- src/platform/config/config.go | 29 ++---- src/platform/config/config_test.go | 7 -- src/platform/validation/spec_index.go | 2 +- src/storage/conformance/discover.go | 25 +---- src/storage/memory/search.go | 4 - src/storage/postgres/read_path_test.go | 27 ------ src/storage/postgres/search_repository.go | 24 +---- tests/acceptance/pagination_test.go | 98 ------------------- tests/acceptance/suite_test.go | 4 +- 23 files changed, 74 insertions(+), 471 deletions(-) delete mode 100644 tests/acceptance/pagination_test.go diff --git a/README.md b/README.md index 25ae2f5..db5c19e 100644 --- a/README.md +++ b/README.md @@ -69,9 +69,8 @@ That stack applies its own migrations (they are compiled into the binary) and reads the Beckn specification from `tests/testdata/beckn-v2.0.0.yaml`, mounted at the cache path. The boot logs one warning about the registry fetch it did not do, which is why it works with no network. That file is byte-identical to -the OAN profile of the spec at commit `dfc1097` of -`OpenAgriNet/protocol-specifications-v2` (A28); set `VALIDATION_SPEC_URL` to its -raw URL to exercise the fetch path instead. +the `core-v2.0.0-lts` tag of `beckn/protocol-specifications-v2`; set +`VALIDATION_SPEC_URL` to its raw URL to exercise the fetch path instead. With the stack up, there is a worked catalog and the requests that find it: @@ -119,10 +118,9 @@ overrides; that file is gitignored. **Secrets — `DATABASE_URL` above all — belong in neither YAML file.** The Beckn specification is fetched at boot from `VALIDATION_SPEC_URL` and -cached under `.cache/beckn/`. It defaults to the OAN profile at commit -`dfc1097` of `OpenAgriNet/protocol-specifications-v2` (A28) — a commit rather -than a branch, so an upstream merge cannot change the validator under a running -deployment. It is not baked +cached under `.cache/beckn/`. It defaults to the `core-v2.0.0-lts` tag of +`beckn/protocol-specifications-v2` — a tag rather than a branch, so an upstream +merge cannot change the validator under a running deployment. It is not baked into the image; air-gapped deploys mount a cache file at `VALIDATION_SPEC_CACHE_PATH`. diff --git a/docker-compose.yml b/docker-compose.yml index 8316f1b..99d9297 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,9 +81,8 @@ services: APP_DOMAIN: ${APP_DOMAIN:-Agriculture} volumes: # The Beckn spec, which the service refuses to boot without. Byte-identical - # to the OAN profile at commit dfc1097 of - # OpenAgriNet/protocol-specifications-v2 (A28), which is also - # VALIDATION_SPEC_URL's default — so a normal boot fetches, compiles + # to the core-v2.0.0-lts tag of beckn/protocol-specifications-v2, which is + # also VALIDATION_SPEC_URL's default — so a normal boot fetches, compiles # and finds this file already holding the same bytes, and an offline boot # falls back to it with one accurate warning about the fetch that failed. # diff --git a/docs/design/implementation-plan.md b/docs/design/implementation-plan.md index f875b66..fb4fcc7 100644 --- a/docs/design/implementation-plan.md +++ b/docs/design/implementation-plan.md @@ -151,7 +151,6 @@ field carrying **an array of network ids** (not `PUBLIC`/`PRIVATE`). | **A25** | **This service is one of four layers, and the plan was written as if it were all of them. Tasks 25 and 26 are added, and 23a and 23c grow one requirement each.** The other three layers — experience, network, provider — run **beckn-onix** adapters that already emit OTel traces, metrics and audit logs, and reading them changes four things here. **Two questions this plan could not answer are answered elsewhere, not nowhere.** Relevance and accuracy need what the user did *after* the response; the experience-layer adapter has the user and already emits traces, so that is where the outcome event belongs. Performance per location and per commodity is refused here because coordinates and `filters.expression` are on the never-emitted list — but the provider adapter holds both as first-class call-plan fields, so it answers them without anyone widening a deny-list. The H3-coarsening policy call that `opentelemetry.md` raised against 23d is **withdrawn** on those grounds. **Our spans would currently be exported and then silently dropped.** onix's network trace pipeline admits only spans carrying `sender.id`, which this phase makes optional and usually absent, while the stage downstream of it rebuilds `trace_id` from an attribute spelled `transaction_id`, which we planned to spell `beckn.transactionId`. A span that fails either is stored, paid for and invisible, with nothing anywhere reporting an error. The filter is onix's bug to fix — it admits on one key and joins on another — and the spelling is ours: **23c emits `transaction_id` and `message_id` as aliases**, written from the same values at the same call site so they cannot drift. Two, not three — an earlier draft named `receiver.id`, which onix puts only on audit log records; its span spelling is `recipient.id`, which we already emit, so a third alias would be paid for everywhere and read nowhere. Buying admission by inventing a `sender.id` is explicitly refused; missing from a dashboard is recoverable, poisoning the network's only participant-identity join is not. **Metrics were dropped from this service for a reason that covers only half the ground.** A23 removed `metrics.go` because a stateless replica's in-memory counter is a partial nobody can reassemble — true of the facilitator's windowed aggregates, and the reason Task 24 exists. It is not true of a *level*: three configured ceilings (`DATABASE_MAX_CONNS`, the rate limiter, the body size) have no observable distance-to-limit, and a pool at 30 of 32 refuses nothing, so it produces no span to be seen in. An earlier draft of this amendment said a 429 "produces no span, no event and no counter". **That is false, and the correction matters because it is what the task's scope was sized against**: `Trace` is index 1 of `router.go`'s `chain`, above both `Envelope` and `RateLimit`, so a 429 and a body refusal each produce a span carrying the status and — post-23d — an `error` event, both projected from the single `logNack` in `response_writer.go` that every refusal already passes through. Only "no counter" was ever true, and a counter that restates a span fact is the `duration_ms` mistake one signal up. **Task 25 is therefore the levels, not the refusals**, and it is smaller for it. Conflating the operator set with the network set is why this service has no metrics at all. **Task 25** is the operator set and is **not blocked**; Task 24 stays blocked on the registry — *five of its twelve codes have since shipped, on 2026-09-10, derived in `otel/collector.yaml`; the block stands for the other seven. See Task 24.* **And the deny-list is a rule nothing enforces** — five sub-tasks comply with it by hand. **Task 26** turns it into a pin, over the exporter's output rather than over the code that builds it | 23, 24, 25, 26 | | **A26** | **The publish geometry budget becomes configuration.** `MaxGeometriesPerCatalog` moves from a `const` in `src/publish/geometry.go` to `config.Geo.MaxGeometriesPerCatalog` / `GEO_MAX_GEOMETRIES_PER_CATALOG`, default **256**, threaded `container → publish.NewService → derive → ExtractGeometries → catalogWalk`. **The default does not change and no behaviour over the ceiling changes** — the extra finds are still `PARTIAL` faults naming their paths, never a silent drop. What changes is who gets to pick the number. **The failure it causes is the wrong shape to be a build-time constant.** Over the ceiling the publish still answers HTTP 200 and the publisher is told, but those shapes are not searchable, and at the operator that reads as missing data rather than as a limit: a 100 km `S_DWITHIN` returns fewer markets than exist and nothing in the response says why. Measured against a real catalog — 273 Maharashtra markets carrying a `Point` under both `coverageAreas` and `market.location`, so 546 shapes — only the first 128 markets were findable by place under 256. **Config rather than a larger constant, for the reason `ResolutionCells` two rows above already gives:** how many shapes a catalog carries is a property of a deployment's publishers, and the ceiling is reached by publishing normally rather than by abuse. That is also the line that decides which neighbours do *not* move: `MaxCatalogWalkDepth` bounds a stack against a pathological document, which is abuse, and `MaxIndexCoverCells` is a property of the index rather than of a catalog. **Zero is refused at boot**, alongside the body ceiling and for the same reason — it reads either as "unlimited" or as "refuse everything", and a catalog that may hold no geometry is undiscoverable by place. **The duplicate declaration in `src/indexing/geo/h3.go` is deleted, not rewired.** It had zero call sites; two constants of one name in two packages is how an operator raises the ceiling in the half that is not read. **What the number stands in for is cells, not shapes:** it times `MaxIndexCoverCells` (8,192) is the worst case one publish can ask the index to hold, so a points-only catalog — one cell per shape — can raise it a long way and a polygon-heavy one cannot | 2, 17, 18 | | **A27** | **A deployment may decline free-text retrieval, and declining REFUSES where a missing backend mode degrades.** `Search.EnableTextSearch` / `SEARCH_ENABLE_TEXT_SEARCH`, **true by default** — the value that takes no decision has to be the one that behaves as before the flag existed. With it false, an intent carrying a non-empty `textSearch` is refused in `MapIntent` with `SCH_TYPE_NOT_SUPPORTED` at `$['message']['intent']['textSearch']`, and the message names `spatial` and `filters` as what still works. **The whole content of this amendment is that it is not `FailOnUnavailableMode` under another name**, because the two read alike and behave oppositely. That flag is about a mode the **backend** cannot run — `semantic` on a `noop` deployment — and false there degrades, answering with what the other modes found and naming the gap in `X-Beckn-Degraded`; the query the caller wrote survives without it. This one is the **deployment's** policy, and degrading would be the failure C11 exists to prevent: on a text-only intent the term IS the query, so running the remaining modes without it answers **the whole corpus under a 200** — a page indistinguishable at the caller from a correct one. That is the same widening `MapIntent`'s doc comment already refuses for an unreadable `targets` or an ignored SRID, which is why the guard sits there and not in `negotiate`. **Three details are load-bearing.** The guard reads the **trimmed** term, like every other reader of it: `" "` asks for no retrieval mode at all, so it stays the no-criterion refusal and calling it a disabled text search would name the wrong mistake. The intent's required-one-of **list** shrinks with the flag — `spatial or filters`, with a parenthetical saying why the third is absent — because offering `textSearch` to a caller this deployment would then refuse walks them into a second `400`; the **guard** does not shrink with it, since a term that was sent is a criterion that was sent and reporting it missing would be a false sentence stacked on the true one. And the wire message names the two criteria that still work rather than the environment variable, matching `negotiate` and the `filters.type` refusal: a caller cannot act on the variable, and the operator has `docs/publish-and-discover.md`. **`modesFor` is left alone.** It still maps a non-empty `Text` to all three ranked modes, because the refusal is total and unreachable past it; a second guard there would be a branch no test can reach and the kind of defence that rots into a contradiction | 2, 17, 19 | -| **A28** | **The page rides in the signed body, and the answer says which page it is (#51).** The schema is now the **OAN profile** of `beckn.yaml`, pinned at commit `dfc1097` of `OpenAgriNet/protocol-specifications-v2` (`VALIDATION_SPEC_URL`'s default and `tests/testdata/beckn-v2.0.0.yaml`, byte-identical) — a commit because the branch carries no tag yet; it becomes a tag when one is cut, never the branch name. That profile adds `limit` (1-100) and `offset` (>= 0) to `DiscoverAction` and an optional `pagination` (`limit`, `offset`, `total`) to `OnDiscoverAction`. **Three decisions.** (1) The page is read from the **body**, each field falling back to the query parameter of the same name. The body wins field by field because it is the half the request signature covers; the query string survives only so callers written before the profile keep working, and it still clamps where the body is bounded by the schema — a body `limit` of 101 is an L1 refusal, not a clamp. (2) `pagination` reports the page **applied**, after the default and the clamp: a caller that asked for 5000 must read back the 100 it got, or it computes its next offset from a page size it never received. (3) `total` is sent **only when exact**, and is never a count query — **A19**'s reason for deleting that query (150.6 ms against a 1.5 ms search) stands. It is `len(fused)`, the number the search already holds, and it is omitted whenever any retrieval list came back as long as `MaxCandidatesPerMode`, because such a list may have been cut by its `LIMIT` and the fusion of it is a truncation. A list of exactly the cap that happened to be complete is reported uncounted too — the one false negative, and the safe direction: an absent total says *narrow the query*, a short one says *this is everything* when it is not. Degradation does not suppress it; that is `X-Beckn-Degraded`'s job, and the total is the size of the set this answer pages through. The memory backend has no cap, so its total is always present. **Unchanged:** `offset + limit > MaxCandidatesPerMode` is still refused (now at `$['message']['offset']`), never answered with an empty page. **Not taken here:** the same profile makes `context.networkId` a string or an array; `beckn.Context.NetworkID` is still a string, so a list is refused by the envelope parser exactly as before — its meaning for scope and `visibleTo` is a separate issue. Supersedes A19's "no total" for this one exact case, and the "exactly one field" note on `OnDiscoverAction` | 11, 16, 19 | A6 and A7 exist for one requirement: *swap the text backend later, keep geo on PG, and let publish write to two stores.* Both build **seams plus conformance @@ -2166,9 +2165,7 @@ POST /discover "provider": { ... }, // fetched once per catalog on the page "resources": [ ... ], // only the ones that matched "offers": [ ... ] // only those touching a matched - } ], // resource, plus catalog-wide ones - "pagination": { "limit": 20, "offset": 0, "total": 45 } // A28 - } } + } ] } } // resource, plus catalog-wide ones ``` ```http @@ -2176,9 +2173,9 @@ X-Beckn-Degraded: structured ``` The degraded list is a **header, not a body field** (C11). `OnDiscoverAction` -declares `additionalProperties: false` with `catalogs` and `pagination` (A28) as -its only properties, so a `degraded` key inside `message` is not an extension — -it is a response that fails its own schema. The header is absent when nothing degraded. +declares `additionalProperties: false` with `catalogs` as its only property, so +a `degraded` key inside `message` is not an extension — it is a response that +fails its own schema. The header is absent when nothing degraded. ### The flow diff --git a/docs/publish-and-discover.md b/docs/publish-and-discover.md index 113ede6..aa78591 100644 --- a/docs/publish-and-discover.md +++ b/docs/publish-and-discover.md @@ -19,10 +19,9 @@ That is the whole of it: | `GET /readyz` | the platform | can it also reach PostgreSQL | Four routes, no wildcard mount, no aliases. Both APIs are implemented against -the Beckn v2.0.0 specification as OpenAgriNet profiles it — -[`OpenAgriNet/protocol-specifications-v2`](https://github.com/OpenAgriNet/protocol-specifications-v2/blob/dfc109759b3781fde74539adc79df63fccbadb83/api/v2.0.0/beckn.yaml) -at commit `dfc1097`, which adds discover pagination to beckn's `core-v2.0.0-lts` -(A28), pinned in this repo as +the published Beckn v2.0.0 specification — +[`beckn/protocol-specifications-v2`](https://github.com/beckn/protocol-specifications-v2/blob/core-v2.0.0-lts/api/v2.0.0/beckn.yaml) +at the `core-v2.0.0-lts` tag, pinned in this repo as [`tests/testdata/beckn-v2.0.0.yaml`](../tests/testdata/beckn-v2.0.0.yaml). That document *is* the validator: the service loads it and validates every request against it, rather than restating the protocol in Go. @@ -470,7 +469,7 @@ matches no field fails the boot, so a typo cannot silently do nothing. | `EMBEDDING_MODEL` / `_ENDPOINT` / `_DIMENSIONS` | `nomic-embed-text` / `http://localhost:11434` / `768` | the Ollama seam | | `RATE_LIMIT_RPS` / `_BURST` | `20` / `40` | per-client limit; `burst >= rps` is enforced | | `VALIDATION_ENABLE_L1_SCHEMA` | `true` | Beckn schema validation of the payload | -| `VALIDATION_SPEC_URL` | the OAN profile's raw URL at commit `dfc1097` | where to fetch the spec; a commit, so it cannot move under a running deployment | +| `VALIDATION_SPEC_URL` | the `core-v2.0.0-lts` raw URL | where to fetch the spec; a tag, so it cannot move under a running deployment | | `VALIDATION_SPEC_CACHE_PATH` | `.cache/beckn/beckn.yaml` | where the spec is read from | | `AUTH_ENABLE_SIGNATURE_VERIFICATION` | `false` | `true` refuses the boot — see below | | `EXT_ALLOW_NETWORK_FETCH` | `false` | may a URL from a *request body* be fetched | diff --git a/docs/quickstart.md b/docs/quickstart.md index 389532f..77ab38c 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -270,22 +270,20 @@ does not remove the requirement. It is fetched from `VALIDATION_SPEC_URL`, which now defaults to ``` -https://raw.githubusercontent.com/OpenAgriNet/protocol-specifications-v2/dfc109759b3781fde74539adc79df63fccbadb83/api/v2.0.0/beckn.yaml +https://raw.githubusercontent.com/beckn/protocol-specifications-v2/refs/tags/core-v2.0.0-lts/api/v2.0.0/beckn.yaml ``` and cached under `VALIDATION_SPEC_CACHE_PATH` (`.cache/beckn/beckn.yaml`). That is why running the binary from the host needs no spec configuration at all: it fetches once and reads the cache afterwards. -A **commit**, not a branch, which is the whole reason there can be a default. A +A **tag**, not a branch, which is the whole reason there can be a default. A branch would let an upstream merge change the validator under a running -deployment without a deploy; a commit cannot move. It is OpenAgriNet's profile -of the Beckn spec rather than beckn's `core-v2.0.0-lts` tag because the profile -adds discover pagination (A28), and it becomes a tag when one is cut. A network +deployment without a deploy; `refs/tags/core-v2.0.0-lts` cannot move. A network that trusts a different document still overrides it. Compose additionally mounts `tests/testdata/beckn-v2.0.0.yaml` straight into -the cache path. That file is byte-identical to that commit, so the two paths agree +the cache path. That file is byte-identical to the tag, so the two paths agree — and the mount is what lets the container stack come up with no network at all. If the fetch fails, the boot logs one loud warning and falls back to the cache; that warning is accurate and is not a failure. 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 caea0e8..809867e 100644 --- a/src/beckn/actions.go +++ b/src/beckn/actions.go @@ -134,16 +134,12 @@ type CatalogStats struct { CategoryCount int `json:"categoryCount"` } -// DiscoverAction is the `message` of a discover request. -// -// Limit and Offset are the OAN profile's addition (A28): the page rides in the -// signed body rather than the query string, which the request signature does -// not cover. Pointers, because absent and zero differ — an absent field falls -// back to the query string, and the schema refuses a zero limit outright. +// DiscoverAction is the `message` of a discover request. Limit and Offset are +// the page; zero means "the default". type DiscoverAction struct { Intent Intent `json:"intent"` - Limit *int `json:"limit,omitempty"` - Offset *int `json:"offset,omitempty"` + Limit int `json:"limit,omitempty"` + Offset int `json:"offset,omitempty"` } // OnDiscoverAction is the `message` of the discover response. @@ -158,12 +154,8 @@ type OnDiscoverAction struct { Pagination *Pagination `json:"pagination,omitempty"` } -// Pagination is the page a discover answer APPLIED — after the default and the -// clamp, not as requested — so the caller computes the next offset from what -// it actually got. -// -// Total is a pointer and omitted when nil: it is sent only when exact (A28). -// Offset is never omitted, because zero is the first page and not "unset". +// Pagination is the page the answer applied, after default and clamp. Total is +// declared by the schema and not yet set. type Pagination struct { Limit int `json:"limit"` Offset int `json:"offset"` diff --git a/src/discover/controller.go b/src/discover/controller.go index 57d2a98..a205691 100644 --- a/src/discover/controller.go +++ b/src/discover/controller.go @@ -2,10 +2,7 @@ package discover import ( "encoding/json" - "fmt" "net/http" - "net/url" - "strconv" "strings" "time" @@ -68,11 +65,7 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { observeIntent(ctx, envelope.Context, action.Intent) - page, err := pageFrom(action, r.URL.Query()) - if err != nil { - httpx.WriteNack(ctx, w, c.errors, envelope.Context.MessageID, err) - return - } + page := Page{Limit: action.Limit, Offset: action.Offset} answer, degraded, err := c.service.Discover(ctx, envelope.Context, action.Intent, page) if err != nil { @@ -97,52 +90,6 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { } } -// pageFrom reads the page off the body, field by field falling back to the -// query string (A28). -// -// The body is the signed half, so a field it names wins. The query string is -// the deprecated route and stays only so that callers written before the OAN -// profile added limit and offset to DiscoverAction keep working; the schema -// already bounds the body's values, which is why only the query's are parsed -// here. -// -// An unreadable query 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(action beckn.DiscoverAction, 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 - } - if action.Limit != nil { - limit = *action.Limit - } - if action.Offset != nil { - offset = *action.Offset - } - return Page{Limit: limit, Offset: offset}, 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 -} - // responseContext turns the request's envelope into the response's. The // correlation handles are echoed rather than regenerated: they are how the caller // ties this answer to the request it made. diff --git a/src/discover/controller_test.go b/src/discover/controller_test.go index 11e6aee..72e0d11 100644 --- a/src/discover/controller_test.go +++ b/src/discover/controller_test.go @@ -312,14 +312,13 @@ 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 +// Pagination arrives in the body beside 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) { +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 { @@ -336,7 +335,7 @@ func TestLimitAndOffsetAreReadFromTheQueryString(t *testing.T) { 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 +344,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) } @@ -412,100 +410,3 @@ func TestARefusedOperatorIsA400(t *testing.T) { t.Errorf("the backend was searched %d times; a refused intent runs no query", repo.calls) } } - -// The page travels in the SIGNED body now (A28). The OAN profile of the schema -// declares limit and offset on DiscoverAction, so a caller no longer needs the -// query string — which the request signature does not cover. -func TestLimitAndOffsetAreReadFromTheBody(t *testing.T) { - repo := &stubRepo{capabilities: everything()} - - recorder := post(t, mount(t, repo, settings()), "/discover", - `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"limit":5,"offset":10}}`) - if recorder.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body: %s", recorder.Code, recorder.Body) - } - if repo.gotQuery.Limit != 5 || repo.gotQuery.Offset != 10 { - t.Errorf("page = limit %d offset %d, want the body's 5 and 10", repo.gotQuery.Limit, repo.gotQuery.Offset) - } -} - -// Each field the body names wins over the same field in the query string, and a -// field the body omits still falls back to it. The body is the signed half, so -// it is the one to believe when the two disagree. -func TestTheBodyWinsOverTheQueryStringFieldByField(t *testing.T) { - repo := &stubRepo{capabilities: everything()} - - recorder := post(t, mount(t, repo, settings()), "/discover?limit=7&offset=40", - `{"context":{"action":"discover"},"message":{"intent":{"textSearch":"wheat"},"offset":10}}`) - if recorder.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body: %s", recorder.Code, recorder.Body) - } - if repo.gotQuery.Offset != 10 { - t.Errorf("Offset = %d, want the body's 10 over the query's 40", repo.gotQuery.Offset) - } - if repo.gotQuery.Limit != 7 { - t.Errorf("Limit = %d, want the query's 7 — the body named none", repo.gotQuery.Limit) - } -} - -// The answer says which page it is: the limit and offset actually APPLIED, after -// the default and the clamp, and the total when the backend could count it. -func TestTheResponseReportsThePageItApplied(t *testing.T) { - total := 3 - repo := &stubRepo{ - capabilities: everything(), - result: domain.SearchResult{Catalogs: []domain.Catalog{{ID: "c1"}}, Total: &total}, - } - - message := onDiscover(t, post(t, mount(t, repo, settings()), "/discover?limit=1000&offset=4", wheat)) - - want := beckn.Pagination{Limit: settings().Search.MaxPageSize, Offset: 4} - if message.Pagination == nil { - t.Fatal("pagination is absent, want the applied page") - } - if got := *message.Pagination; got.Limit != want.Limit || got.Offset != want.Offset || - got.Total == nil || *got.Total != total { - t.Errorf("pagination = %+v (total %v), want limit %d offset %d total %d", - got, got.Total, want.Limit, want.Offset, total) - } -} - -// A total the backend could not count exactly is left OUT, not sent as zero or -// as a guess. A caller reads its absence as "more matched than a page walk can -// reach"; a zero would read as "nothing matched". -func TestAnUncountedTotalIsAbsentFromTheWire(t *testing.T) { - repo := &stubRepo{capabilities: everything(), result: domain.SearchResult{Total: nil}} - - recorder := post(t, mount(t, repo, settings()), "/discover", wheat) - envelope := decodeResponse(t, recorder) - - var message struct { - Pagination map[string]any `json:"pagination"` - } - if err := json.Unmarshal(envelope.Message, &message); err != nil { - t.Fatalf("decoding the message: %v", err) - } - pagination := message.Pagination - if pagination == nil { - t.Fatal("pagination is absent, want the applied page without a total") - } - if _, ok := pagination["total"]; ok { - t.Errorf("pagination = %v, want no total key", pagination) - } - if offset, ok := pagination["offset"]; !ok || offset != float64(0) { - t.Errorf("pagination = %v, want offset 0 present — zero is a real offset", pagination) - } -} - -func onDiscover(t *testing.T, recorder *httptest.ResponseRecorder) beckn.OnDiscoverAction { - t.Helper() - - if recorder.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body: %s", recorder.Code, recorder.Body) - } - var message beckn.OnDiscoverAction - if err := json.Unmarshal(decodeResponse(t, recorder).Message, &message); err != nil { - t.Fatalf("decoding the message: %v", err) - } - return message -} diff --git a/src/discover/intent_mapper.go b/src/discover/intent_mapper.go index d23f76a..5390f09 100644 --- a/src/discover/intent_mapper.go +++ b/src/discover/intent_mapper.go @@ -14,9 +14,8 @@ import ( "github.com/OpenAgriNet/discovery-service/src/platform/jsonpath" ) -// Page is the request's pagination: DiscoverAction's limit and offset, each -// falling back to the deprecated query parameter of the same name (A28). Zero -// means "the default". A struct rather than two adjacent ints, because +// Page is the request's pagination, which arrives as HTTP query parameters +// rather than inside the intent. A struct rather than two adjacent ints, because // `limit, offset` and `offset, limit` compile identically. type Page struct { Limit int @@ -230,7 +229,7 @@ func mapPage(page Page, search config.Search) (limit, offset int, faults []domai if offset+limit > search.MaxCandidatesPerMode { faults = append(faults, domain.Fault{ - Path: "$['message']['offset']", + Path: "$['offset']", Code: string(beckn.CodeSchemaInvalidFormat), Message: fmt.Sprintf( "offset %d plus limit %d passes the retrieval depth of %d", diff --git a/src/discover/intent_mapper_test.go b/src/discover/intent_mapper_test.go index 2f512f8..08e8550 100644 --- a/src/discover/intent_mapper_test.go +++ b/src/discover/intent_mapper_test.go @@ -324,10 +324,6 @@ func TestLimitIsClampedAndAPagePastTheRetrievalDepthIsRefused(t *testing.T) { if !strings.Contains(fatal[0].Message, "500") { t.Errorf("Message = %q, want it to name the boundary", fatal[0].Message) } - // The page is a body field now (A28), so the fault points into the body. - if fatal[0].Path != "$['message']['offset']" { - t.Errorf("Path = %q, want $['message']['offset']", fatal[0].Path) - } } // NetworkID is NOT set here. The service sets it from the envelope, and empty diff --git a/src/discover/service.go b/src/discover/service.go index 2fc0e0a..38e728b 100644 --- a/src/discover/service.go +++ b/src/discover/service.go @@ -73,7 +73,7 @@ func (s *Service) Discover( Catalogs: render(result.Catalogs), // What MapIntent applied, not what was asked: a caller that sent 5000 // must read back the 100 it got, or its next offset skips results. - Pagination: &beckn.Pagination{Limit: query.Limit, Offset: query.Offset, Total: result.Total}, + Pagination: &beckn.Pagination{Limit: query.Limit, Offset: query.Offset}, }, degraded, nil } @@ -90,7 +90,7 @@ func typedSearchFailure(ctx context.Context, err error) error { if errors.Is(err, domain.ErrRetrievalDepth) { return apperrors. Schema(beckn.CodeSchemaInvalidFormat, err.Error()). - At(jsonpath.Dot("$['message']['offset']")) + At(jsonpath.Dot("$['offset']")) } // An expression the store's own parser refused is the caller's mistake too, diff --git a/src/discover/service_test.go b/src/discover/service_test.go index 4dfecf9..87f2472 100644 --- a/src/discover/service_test.go +++ b/src/discover/service_test.go @@ -101,12 +101,13 @@ func TestAModeTheBackendLacksIsNamedRatherThanDropped(t *testing.T) { 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) } - if len(answer.Catalogs) != 1 { - t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(answer.Catalogs)) + if len(catalogs) != 1 { + t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(catalogs)) } if !slices.Equal(degraded, []string{string(domain.CapabilitySemantic)}) { t.Errorf("degraded = %v, want [semantic]", degraded) @@ -188,12 +189,13 @@ func TestSpatialStillAnswersWithTextSearchOff(t *testing.T) { Geometry: bengaluru(), }}}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } - if len(answer.Catalogs) != 1 { - t.Errorf("catalogs = %d, want the one the backend found", len(answer.Catalogs)) + if len(catalogs) != 1 { + t.Errorf("catalogs = %d, want the one the backend found", len(catalogs)) } if hasMode(repo.gotModes, domain.CapabilityLexical) { t.Errorf("modes = %v, want no ranked text mode — nothing asked for one", repo.gotModes) @@ -220,12 +222,13 @@ func TestAFilterABackendCannotRunIsDegradedRatherThanIgnored(t *testing.T) { Type: "jsonpath", Expression: `$.catalogs[*].resources[*] ? (@.grade == "A")`, }}, discover.Page{}) + catalogs := answer.Catalogs if err != nil { t.Fatalf("Discover: %v", err) } - if len(answer.Catalogs) != 1 { - t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(answer.Catalogs)) + if len(catalogs) != 1 { + t.Errorf("catalogs = %d, want the one the backend found — degrading is not failing", len(catalogs)) } if !slices.Equal(degraded, []string{string(domain.CapabilityJSONPath)}) { t.Errorf("degraded = %v, want [jsonpath] — a filter this backend cannot run and "+ @@ -514,10 +517,10 @@ func TestARenderedCatalogCarriesItsDocumentsVerbatim(t *testing.T) { 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) } - catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Offers) != 1 { t.Fatalf("rendered = %+v, want one catalog carrying one offer", catalogs) } @@ -579,10 +582,10 @@ func TestNoOfferWhoseResourcesAreAllOffThePageIsRendered(t *testing.T) { 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) } - catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Resources) != 1 || catalogs[0].Resources[0].ID != "wheat" { t.Fatalf("rendered = %+v, want only the matched resource", catalogs) @@ -661,10 +664,10 @@ func TestACatalogWithAnUnreadableDocumentIsDropped(t *testing.T) { 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) } - catalogs := answer.Catalogs if len(catalogs) != 0 { t.Errorf("catalogs = %+v, want none", catalogs) } @@ -684,10 +687,10 @@ func TestAResourceWithAnUnreadableDocumentIsDropped(t *testing.T) { 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) } - catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Resources) != 1 || catalogs[0].Resources[0].ID != "r2" { t.Fatalf("resources = %+v, want only r2 — r1's document could not be read", catalogs[0].Resources) } @@ -706,10 +709,10 @@ func TestAnOfferWithAnUnreadableDocumentIsDropped(t *testing.T) { 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) } - catalogs := answer.Catalogs if len(catalogs) != 1 || len(catalogs[0].Offers) != 1 || catalogs[0].Offers[0].ID != "o2" { t.Fatalf("offers = %+v, want only o2 — o1's document could not be read", catalogs[0].Offers) } @@ -813,32 +816,3 @@ func TestAMalformedFilterExpressionIsTheCallersFaultNotAFiveHundred(t *testing.T "internal that must not reach the caller", fault.Message, want) } } - -// The answer carries the page the service APPLIED — the default when none was -// asked for, the clamp when too much was — beside the backend's total. A caller -// that sent limit 5000 must read 100 back, or it will compute the next offset -// from a page size it never got. -func TestTheAnswerReportsTheAppliedPageAndTheBackendsTotal(t *testing.T) { - total := 42 - repo := &stubRepo{capabilities: everything(), result: domain.SearchResult{Total: &total}} - cfg := settings() - - answer, _, err := discover.NewService(repo, cfg).Discover( - t.Context(), beckn.Context{}, beckn.Intent{TextSearch: "wheat"}, - discover.Page{Limit: 5000, Offset: 20}) - if err != nil { - t.Fatalf("Discover: %v", err) - } - - page := answer.Pagination - if page == nil { - t.Fatal("pagination is absent") - } - if page.Limit != cfg.Search.MaxPageSize || page.Offset != 20 { - t.Errorf("pagination = limit %d offset %d, want the clamped %d and 20", - page.Limit, page.Offset, cfg.Search.MaxPageSize) - } - if page.Total == nil || *page.Total != total { - t.Errorf("total = %v, want the backend's %d", page.Total, total) - } -} diff --git a/src/domain/query.go b/src/domain/query.go index c9938f0..f6270e4 100644 --- a/src/domain/query.go +++ b/src/domain/query.go @@ -125,17 +125,11 @@ 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). -// -// Total is the size of the fused candidate set the page was sliced from, and -// nil whenever that size is not the number that matched — when a retrieval mode -// returned as many ids as its cap, it may have stopped early (A28). It is never -// a count query: A19 deleted that for costing 100x the search it accompanied, -// and this is the number the search already holds. +// caller as the X-Beckn-Degraded header, not a body key (C11). There is no +// Total (A19). 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 2a9ad7e..0d3bb45 100644 --- a/src/platform/config/config.go +++ b/src/platform/config/config.go @@ -219,19 +219,14 @@ 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 IMMUTABLE, 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, + // The default is a TAG, 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. A tag or a commit cannot move, so the default - // names one 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. - // - // It is the OAN profile at a COMMIT, not beckn's core-v2.0.0-lts tag (A28): - // the profile adds limit/offset to DiscoverAction and pagination to - // OnDiscoverAction, and its branch carries no tag yet. Swap the SHA for a - // tag when one is cut — never for the branch name. + // deployment without a deploy. A pinned commit of the OAN profile (which adds + // discover pagination) 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. // // The cost is that boot now makes a network call before falling back to // SpecCachePath. LoadSpecIndex already warns loudly and falls back, so an @@ -638,19 +633,9 @@ func validateDatabase(database Database) error { ) } -// schemaMaxPageSize is PaginationParams.limit's `maximum` in the OAN profile of -// the spec (A28). A copy rather than a read of the document, because config -// validates before the spec index is built; the two drifting apart shows up as -// a boot refusal here or as a schema failure on the response, never silently. -const schemaMaxPageSize = 100 - func validateSearch(search Search) error { return errors.Join( require(search.DefaultPageSize > 0, "search.defaultPageSize %d is not positive", search.DefaultPageSize), - require(search.MaxPageSize <= schemaMaxPageSize, - "search.maxPageSize %d is above the schema's limit maximum of %d — a body asking for more is refused "+ - "before the clamp, and a query-string caller would be answered with a pagination.limit that fails its own schema", - search.MaxPageSize, schemaMaxPageSize), require(search.MaxPageSize >= search.DefaultPageSize, "search.maxPageSize %d is below search.defaultPageSize %d — a ceiling under the value it clamps", search.MaxPageSize, search.DefaultPageSize), diff --git a/src/platform/config/config_test.go b/src/platform/config/config_test.go index ac33bf2..6e93eb5 100644 --- a/src/platform/config/config_test.go +++ b/src/platform/config/config_test.go @@ -377,13 +377,6 @@ func TestValidateRejects(t *testing.T) { "a candidate pool smaller than one page": { "search:\n maxPageSize: 100\n maxCandidatesPerMode: 50\n", "maxCandidatesPerMode", }, - // The schema caps a page at 100 (A28). Above it, a body asking for more - // is refused before the clamp is reached, and a query-string caller is - // answered with a pagination.limit that fails the response's own - // schema. - "a max page size above the schema's maximum": { - "search:\n maxPageSize: 101\n maxCandidatesPerMode: 500\n", "maxPageSize", - }, "an unloadable timezone": { "app:\n defaultTimezone: Mars/Olympus_Mons\n", "defaultTimezone", }, diff --git a/src/platform/validation/spec_index.go b/src/platform/validation/spec_index.go index 3368ef2..eb7cc84 100644 --- a/src/platform/validation/spec_index.go +++ b/src/platform/validation/spec_index.go @@ -241,7 +241,7 @@ func loadFromCache(path string) (*SpecIndex, error) { // ages with the image), so an air-gapped deploy supplies one at // VALIDATION_SPEC_CACHE_PATH, and mounting it read-only is the right way to // supply a file the service must not be able to rewrite. docker-compose.yml does -// exactly that with a copy byte-identical to VALIDATION_SPEC_URL's default, and every +// exactly that with a copy byte-identical to the core-v2.0.0-lts tag, and every // boot with a reachable registry warned that it could not overwrite those bytes // with themselves. A warning that fires when nothing is wrong is one nobody reads // on the boot when something is. diff --git a/src/storage/conformance/discover.go b/src/storage/conformance/discover.go index 2041a33..5d97b95 100644 --- a/src/storage/conformance/discover.go +++ b/src/storage/conformance/discover.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "slices" - "strconv" "testing" "time" @@ -215,15 +214,6 @@ func pageOrder(result domain.SearchResult) []string { return ids } -// totalOf renders a result's total for a failure message, where a nil pointer -// would print as an address. -func totalOf(result domain.SearchResult) string { - if result.Total == nil { - return "absent" - } - return strconv.Itoa(*result.Total) -} - // offerIDs is every offer hydrated onto the page, sorted. func offerIDs(result domain.SearchResult) []string { ids := make([]string, 0) @@ -652,17 +642,10 @@ func pageTwoDoesNotOverlapPageOne() Case { t.Errorf("page two is %v, want [r-02 r-03]", got) } - // The offset SLICES a stable order rather than re-ranking it, so - // page two holds the next two and not two of the same. - - // And both pages report the same exact total (A28): five matched, - // none of them capped, so the count is the size of the set being - // paged — whichever page asked. - for name, result := range map[string]domain.SearchResult{"one": first, "two": second} { - if result.Total == nil || *result.Total != len(resources) { - t.Errorf("page %s reports total %v, want %d", name, totalOf(result), len(resources)) - } - } + // 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. }, } } diff --git a/src/storage/memory/search.go b/src/storage/memory/search.go index e62efb0..1063c9e 100644 --- a/src/storage/memory/search.go +++ b/src/storage/memory/search.go @@ -62,13 +62,9 @@ func (r *Repository) Search( page = nil } - // This backend retrieves without a cap, so the ranked list is every match - // and its length is always the exact total. - total := len(page) return domain.SearchResult{ Catalogs: r.hydrate(pageOf(page, query.Offset, query.Limit), scope), Degraded: degraded, - Total: &total, }, nil } diff --git a/src/storage/postgres/read_path_test.go b/src/storage/postgres/read_path_test.go index cb54192..aee05d2 100644 --- a/src/storage/postgres/read_path_test.go +++ b/src/storage/postgres/read_path_test.go @@ -380,33 +380,6 @@ func TestPageTwoDoesNotOverlapPageOne(t *testing.T) { } } -// A mode that returned as many ids as its cap may have stopped early, so the -// fused list is a truncation and its length is not the number that matched. -// The total is then left OUT (A28) rather than reported short — this is the -// only backend with a cap, which is why the case lives here and not in the -// conformance suite. searchConfig's cap is 8; ten lots match. -func TestATotalIsReportedOnlyWhenNoModeHitItsCap(t *testing.T) { - repository, _ := publish(t, nil, readFixture{catalog: "cat-capped", resources: kharifLots(10)}) - - capped := search(t, repository, domain.SearchQuery{Text: "kharif", Limit: 4}) - if capped.Total != nil { - t.Errorf("total = %d with every mode at its cap of %d, want it absent", - *capped.Total, searchConfig().MaxCandidatesPerMode) - } -} - -func TestATotalUnderTheCapIsExact(t *testing.T) { - repository, _ := publish(t, nil, readFixture{catalog: "cat-counted", resources: kharifLots(6)}) - - result := search(t, repository, domain.SearchQuery{Text: "kharif", Limit: 4}) - if result.Total == nil || *result.Total != 6 { - t.Errorf("total = %v, want exactly 6", result.Total) - } - if got := len(matchedIDs(result)); got != 4 { - t.Errorf("the page holds %d, want 4 — the total counts the set, not the page", got) - } -} - // A mode that ERRORS is degraded, and the page is what the other modes found. // // The vectors are 768 wide because the column is, and the repository embeds at diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index 8375759..8ba65af 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -128,29 +128,7 @@ func (s *SearchRepository) Search( if err != nil { return domain.SearchResult{}, err } - return domain.SearchResult{ - Catalogs: catalogs, - Degraded: degraded, - Total: s.exactTotal(lists, fused), - }, nil -} - -// exactTotal is len(fused) when that is the number that matched, and nil when -// it is not (A28). -// -// A list as long as MaxCandidatesPerMode may have been cut by its LIMIT, so the -// fusion of it is a truncation. A list of exactly the cap that happened to be -// complete is reported as uncounted too — the one false negative, and the safe -// direction: an absent total says "narrow the query", a short one says "this is -// everything" when it is not. -func (s *SearchRepository) exactTotal(lists [][]string, fused []string) *int { - for _, ids := range lists { - if len(ids) >= s.search.MaxCandidatesPerMode { - return nil - } - } - total := len(fused) - return &total + return domain.SearchResult{Catalogs: catalogs, Degraded: degraded}, nil } // withinRetrievalDepth refuses a page past what any mode retrieves. Checked diff --git a/tests/acceptance/pagination_test.go b/tests/acceptance/pagination_test.go deleted file mode 100644 index a11ca6c..0000000 --- a/tests/acceptance/pagination_test.go +++ /dev/null @@ -1,98 +0,0 @@ -package acceptance - -import ( - "fmt" - "net/http" - "slices" - "testing" - - "github.com/OpenAgriNet/discovery-service/src/beckn" -) - -// fiveAtMajestic publishes five resources a spatial intent around Majestic -// matches, so a page size of two has to walk three pages to see them all. -func fiveAtMajestic(t *testing.T) *service { - t.Helper() - - list := make([]map[string]any, 0, 5) - for index := range 5 { - list = append(list, aResource(fmt.Sprintf("r-%d", index), fmt.Sprintf("Lot %d", index))) - } - svc := newService(t) - svc.publishCatalogs(t, aCatalog("c-lots", availableAt(majestic), resources(list...))) - return svc -} - -// pageAt asks for one page with limit and offset in the SIGNED body (A28), and -// returns the message so the scenario can read the pagination beside the -// catalogs. -func (s *service) pageAt(t *testing.T, limit, offset int) beckn.OnDiscoverAction { - t.Helper() - - answer := s.post(t, "/discover", envelope(beckn.ActionDiscover, map[string]any{ - "intent": spatial(dwithin(providerGeoPath, majestic, 5000)), - "limit": limit, - "offset": offset, - })) - if answer.status != http.StatusOK { - t.Fatalf("POST /discover limit %d offset %d = %d, want 200 — the OAN profile "+ - "declares both on DiscoverAction\nbody: %s", limit, offset, answer.status, answer.body) - } - - var body struct { - Message beckn.OnDiscoverAction `json:"message"` - } - answer.decode(t, &body) - return body.Message -} - -// A caller walking pages in the body reaches every match exactly once, and -// every page says where it is: the page it applied, and a total that tells the -// last page from a short one. Before A28 the body could not carry the page at -// all — the schema refused the key — and the answer carried nothing but -// catalogs, so a short page and the end were the same thing. -func TestTheBodyPagesThroughEveryMatchAndTheTotalMarksTheEnd(t *testing.T) { - svc := fiveAtMajestic(t) - - var seen []string - for offset := 0; offset < 5; offset += 2 { - message := svc.pageAt(t, 2, offset) - - page := message.Pagination - if page == nil { - t.Fatalf("offset %d: no pagination in the answer", offset) - } - if page.Limit != 2 || page.Offset != offset { - t.Errorf("offset %d: pagination reports limit %d offset %d, want 2 and %d", - offset, page.Limit, page.Offset, offset) - } - if page.Total == nil || *page.Total != 5 { - t.Errorf("offset %d: total = %v, want 5", offset, page.Total) - } - seen = append(seen, resourceIDs(message.Catalogs)...) - } - - slices.Sort(seen) - if want := []string{"r-0", "r-1", "r-2", "r-3", "r-4"}; !slices.Equal(seen, want) { - t.Errorf("walking the pages saw %v, want each of %v exactly once", seen, want) - } -} - -// The schema bounds the body's page, so a limit above its maximum is refused -// by L1 validation before the service clamps anything. The query string still -// clamps — it is outside the schema — which is one more reason it is the -// deprecated route. -func TestALimitAboveTheSchemaMaximumIsRefused(t *testing.T) { - svc := fiveAtMajestic(t) - - answer := svc.post(t, "/discover", envelope(beckn.ActionDiscover, map[string]any{ - "intent": spatial(dwithin(providerGeoPath, majestic, 5000)), - "limit": 101, - })) - if answer.status != http.StatusBadRequest { - t.Fatalf("limit 101 answered %d, want 400\nbody: %s", answer.status, answer.body) - } - if code := answer.nack(t).Message.Error.Code; code != beckn.CodeSchemaValidationFailed { - t.Errorf("the refusal is %s, want %s", code, beckn.CodeSchemaValidationFailed) - } -} diff --git a/tests/acceptance/suite_test.go b/tests/acceptance/suite_test.go index 701d7f0..905fa7d 100644 --- a/tests/acceptance/suite_test.go +++ b/tests/acceptance/suite_test.go @@ -350,9 +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 member of DiscoverAction, beside the intent rather than inside -// it (A28). It moved there from the query string, and this was the 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 From 47a8714bf6ccae4ba3c6efb81ebbad30e84fe00d Mon Sep 17 00:00:00 2001 From: Prajwal TD <157502711+tdprajwal@users.noreply.github.com> Date: Mon, 26 Oct 2026 11:26:40 +0530 Subject: [PATCH 3/9] fix: refuse discover pagination left on the query string [#51] 0.1.0 read ?limit=/?offset=; ignoring them now would silently return the default page. Refuse with SCH_INVALID_FORMAT naming the new location. BREAKING CHANGE: discover pagination moved from ?limit=/?offset= to message.limit/message.offset; the query parameters are now refused with 400. --- src/discover/controller.go | 20 +++++++++++++++++++- src/discover/controller_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/discover/controller.go b/src/discover/controller.go index a205691..04de76e 100644 --- a/src/discover/controller.go +++ b/src/discover/controller.go @@ -3,6 +3,7 @@ package discover import ( "encoding/json" "net/http" + "net/url" "strings" "time" @@ -65,7 +66,11 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { observeIntent(ctx, envelope.Context, action.Intent) - page := Page{Limit: action.Limit, Offset: action.Offset} + page, err := pageFrom(r.URL.Query(), action) + if err != nil { + httpx.WriteNack(ctx, w, c.errors, envelope.Context.MessageID, err) + return + } answer, degraded, err := c.service.Discover(ctx, envelope.Context, action.Intent, page) if err != nil { @@ -90,6 +95,19 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { } } +// pageFrom reads the page off the body. +// +// 0.1.0 read it off the query string. Ignoring a leftover parameter would +// answer the default page with a 200, and a client walking offsets would loop +// on page one; refusing it says where the page lives now. +func pageFrom(query url.Values, action beckn.DiscoverAction) (Page, error) { + if query.Has("limit") || query.Has("offset") { + return Page{}, apperrors.Schema(beckn.CodeSchemaInvalidFormat, + "pagination moved from the query string to message.limit and message.offset").At(messageRoot) + } + return Page{Limit: action.Limit, Offset: action.Offset}, nil +} + // responseContext turns the request's envelope into the response's. The // correlation handles are echoed rather than regenerated: they are how the caller // ties this answer to the request it made. diff --git a/src/discover/controller_test.go b/src/discover/controller_test.go index 72e0d11..3fadd17 100644 --- a/src/discover/controller_test.go +++ b/src/discover/controller_test.go @@ -329,6 +329,32 @@ func TestLimitAndOffsetAreReadFromTheBody(t *testing.T) { } } +// The query string carried pagination in 0.1.0. Ignoring it would answer the +// default page with a 200, and a client walking offsets would loop on page one, +// so a leftover parameter is refused and the refusal names the new location. +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. From 8ecd0c4f2b772930d1563f83768a7c9f7ffe0c79 Mon Sep 17 00:00:00 2001 From: Prajwal TD <157502711+tdprajwal@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:16:15 +0530 Subject: [PATCH 4/9] feat: return the capped result total in discover pagination [#51] Total is the ranked list's length before paging, capped at the retrieval depth so it never points past the last page a caller can fetch. --- src/beckn/actions.go | 4 ++-- src/discover/service.go | 6 +++++- src/discover/service_test.go | 17 +++++++++++++++++ src/domain/query.go | 5 +++-- src/storage/conformance/discover.go | 10 ++++++---- src/storage/memory/search.go | 1 + src/storage/postgres/search_repository.go | 2 +- 7 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/beckn/actions.go b/src/beckn/actions.go index 809867e..548cda0 100644 --- a/src/beckn/actions.go +++ b/src/beckn/actions.go @@ -154,8 +154,8 @@ type OnDiscoverAction struct { Pagination *Pagination `json:"pagination,omitempty"` } -// Pagination is the page the answer applied, after default and clamp. Total is -// declared by the schema and not yet set. +// Pagination is the page the answer applied, after default and clamp, and the +// number of results that can be paged through. type Pagination struct { Limit int `json:"limit"` Offset int `json:"offset"` diff --git a/src/discover/service.go b/src/discover/service.go index 38e728b..cca9134 100644 --- a/src/discover/service.go +++ b/src/discover/service.go @@ -69,11 +69,15 @@ func (s *Service) Discover( degraded = append(degraded, result.Degraded...) observeRetrieval(ctx, modes, degraded) + // Capped at the retrieval depth, the last page a caller can fetch; + // a true count past it is a count query per request (A19). + total := min(result.Total, s.cfg.Search.MaxCandidatesPerMode) + return beckn.OnDiscoverAction{ Catalogs: render(result.Catalogs), // What MapIntent applied, not what was asked: a caller that sent 5000 // must read back the 100 it got, or its next offset skips results. - Pagination: &beckn.Pagination{Limit: query.Limit, Offset: query.Offset}, + Pagination: &beckn.Pagination{Limit: query.Limit, Offset: query.Offset, Total: &total}, }, degraded, nil } diff --git a/src/discover/service_test.go b/src/discover/service_test.go index 87f2472..543a1cf 100644 --- a/src/discover/service_test.go +++ b/src/discover/service_test.go @@ -397,6 +397,23 @@ func TestAnOversizeLimitIsClampedAndAnUnreachablePageIsRefused(t *testing.T) { } } +// Total is what the backend matched, capped at the retrieval depth: past it no +// page can be fetched, so a larger number would point the caller at a refusal. +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 == nil || *got != want { + t.Errorf("matched %d: Total = %v, 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. diff --git a/src/domain/query.go b/src/domain/query.go index f6270e4..8beaee6 100644 --- a/src/domain/query.go +++ b/src/domain/query.go @@ -125,11 +125,12 @@ 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 counts +// the ranked ids before paging, not a separate count query (A19). type SearchResult struct { Catalogs []Catalog Degraded []string + Total int } // Scope is the request-wide gate every retrieval mode reads (A6). diff --git a/src/storage/conformance/discover.go b/src/storage/conformance/discover.go index 5d97b95..cbabfad 100644 --- a/src/storage/conformance/discover.go +++ b/src/storage/conformance/discover.go @@ -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..0e88f94 100644 --- a/src/storage/memory/search.go +++ b/src/storage/memory/search.go @@ -65,6 +65,7 @@ func (r *Repository) Search( return domain.SearchResult{ Catalogs: r.hydrate(pageOf(page, query.Offset, query.Limit), scope), Degraded: degraded, + Total: len(page), }, nil } diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index 8ba65af..e7cb62e 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -128,7 +128,7 @@ func (s *SearchRepository) Search( 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 From ff7b9fd2bffd0b3f44efffab92239e86ec553f11 Mon Sep 17 00:00:00 2001 From: Prajwal TD <157502711+tdprajwal@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:39:48 +0530 Subject: [PATCH 5/9] fix: align discover pagination with the OAN spec at a8451c4 [#51] The pinned dfc1097 capped the request limit at 100, so the validator refused an oversize limit before the clamp could run. Pin to a8451c4, bind the response to DiscoverPagination with a required total, and move the pagination decisions from code comments into ADR-0017. --- docs/adr/0017-discover-pagination.md | 50 +++++++ docs/adr/README.md | 1 + src/beckn/actions.go | 28 ++-- src/beckn/schema_conformance_test.go | 38 +----- src/discover/controller.go | 6 +- src/discover/controller_test.go | 7 +- src/discover/intent_mapper.go | 9 +- src/discover/service.go | 20 ++- src/discover/service_test.go | 7 +- src/domain/query.go | 2 +- src/platform/config/config.go | 10 +- src/storage/conformance/discover.go | 4 +- src/storage/postgres/fusion.go | 6 +- tests/testdata/beckn-v2.0.0.yaml | 189 +++++++++++++++++++++------ 14 files changed, 238 insertions(+), 139 deletions(-) create mode 100644 docs/adr/0017-discover-pagination.md diff --git a/docs/adr/0017-discover-pagination.md b/docs/adr/0017-discover-pagination.md new file mode 100644 index 0000000..3e810c4 --- /dev/null +++ b/docs/adr/0017-discover-pagination.md @@ -0,0 +1,50 @@ +# 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. **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. 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/src/beckn/actions.go b/src/beckn/actions.go index 548cda0..dc6a8dd 100644 --- a/src/beckn/actions.go +++ b/src/beckn/actions.go @@ -134,8 +134,7 @@ type CatalogStats struct { CategoryCount int `json:"categoryCount"` } -// DiscoverAction is the `message` of a discover request. Limit and Offset are -// the page; zero means "the default". +// DiscoverAction is the `message` of a discover request. type DiscoverAction struct { Intent Intent `json:"intent"` Limit int `json:"limit,omitempty"` @@ -144,20 +143,19 @@ type DiscoverAction struct { // OnDiscoverAction is the `message` of the discover response. // -// It must NOT grow a Degraded field. The schema is additionalProperties:false -// with `catalogs` and `pagination` as its only properties, 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). +// It must NOT grow a Degraded field. The v2.0.0 schema is +// 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"` - Pagination *Pagination `json:"pagination,omitempty"` + Catalogs []Catalog `json:"catalogs"` + Pagination *DiscoverPagination `json:"pagination,omitempty"` } -// Pagination is the page the answer applied, after default and clamp, and the -// number of results that can be paged through. -type Pagination struct { - Limit int `json:"limit"` - Offset int `json:"offset"` - Total *int `json:"total,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 9ec5ef5..8574c43 100644 --- a/src/beckn/schema_conformance_test.go +++ b/src/beckn/schema_conformance_test.go @@ -69,7 +69,7 @@ func bindings() []binding { {"DiscoverAction", DiscoverAction{}, schemaPath("DiscoverAction")}, {"OnDiscoverAction", OnDiscoverAction{}, schemaPath("OnDiscoverAction")}, - {"Pagination", Pagination{}, schemaPath("Pagination")}, + {"DiscoverPagination", DiscoverPagination{}, schemaPath("DiscoverPagination")}, {"Nack", Nack{}, schemaPath("NackBadRequest")}, {"NackMessage", NackMessage{}, schemaPath("NackBadRequest", "properties", "message")}, @@ -135,7 +135,7 @@ func TestWireStructsMatchTheSpecification(t *testing.T) { var found []deviation for _, b := range bindings() { - found = append(found, deviations(t, b, flattenAllOf(t, spec, resolve(t, spec, b.path)))...) + found = append(found, deviations(t, b, resolve(t, spec, b.path))...) } sort.Slice(found, func(i, j int) bool { return found[i].key < found[j].key }) @@ -158,40 +158,6 @@ func TestWireStructsMatchTheSpecification(t *testing.T) { } } -// flattenAllOf merges the properties of an allOf node's members into one -// properties map, following each member's local $ref. Pagination is the first -// bound schema composed this way — allOf[PaginationParams, {total}] in the OAN -// profile — and a walk that read only the top-level `properties` would find -// none and fail the binding rather than check it. A node without allOf comes -// back unchanged. -func flattenAllOf(t *testing.T, spec map[string]any, node map[string]any) map[string]any { - t.Helper() - - members, ok := node["allOf"].([]any) - if !ok { - return node - } - - merged := map[string]any{} - for _, member := range members { - part, ok := member.(map[string]any) - if !ok { - t.Fatalf("an allOf member is not an object: %v", member) - } - if ref, isRef := part["$ref"].(string); isRef { - part = resolve(t, spec, strings.Split(strings.TrimPrefix(ref, "#/"), "/")) - } - properties, ok := flattenAllOf(t, spec, part)["properties"].(map[string]any) - if !ok { - t.Fatalf("an allOf member declares no properties: %v", member) - } - for name, schema := range properties { - merged[name] = schema - } - } - return map[string]any{"properties": merged} -} - // deviations compares one struct's JSON tags against its schema node's // properties, in both directions. An extra tag ships a key the schema does not // declare; a missing one drops a key the schema does, which for a service that diff --git a/src/discover/controller.go b/src/discover/controller.go index 04de76e..77b84ac 100644 --- a/src/discover/controller.go +++ b/src/discover/controller.go @@ -95,11 +95,7 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { } } -// pageFrom reads the page off the body. -// -// 0.1.0 read it off the query string. Ignoring a leftover parameter would -// answer the default page with a 200, and a client walking offsets would loop -// on page one; refusing it says where the page lives now. +// pageFrom reads the page from the body; ?limit= is refused (ADR-0017). func pageFrom(query url.Values, action beckn.DiscoverAction) (Page, error) { if query.Has("limit") || query.Has("offset") { return Page{}, apperrors.Schema(beckn.CodeSchemaInvalidFormat, diff --git a/src/discover/controller_test.go b/src/discover/controller_test.go index 3fadd17..a15c1b7 100644 --- a/src/discover/controller_test.go +++ b/src/discover/controller_test.go @@ -312,8 +312,7 @@ func TestAnUnavailableModeIsA400WhenTheDeploymentAsksToBe(t *testing.T) { } } -// Pagination arrives in the body beside the intent, and an oversize limit is clamped rather than refused (C11's neighbour: the caller -// still gets the results they asked about). +// 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()) @@ -329,9 +328,7 @@ func TestLimitAndOffsetAreReadFromTheBody(t *testing.T) { } } -// The query string carried pagination in 0.1.0. Ignoring it would answer the -// default page with a 200, and a client walking offsets would loop on page one, -// so a leftover parameter is refused and the refusal names the new location. +// 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) { diff --git a/src/discover/intent_mapper.go b/src/discover/intent_mapper.go index 5390f09..0231f24 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 { diff --git a/src/discover/service.go b/src/discover/service.go index cca9134..cf081bb 100644 --- a/src/discover/service.go +++ b/src/discover/service.go @@ -33,10 +33,9 @@ func NewService(repo domain.SearchRepository, cfg config.Config) *Service { return &Service{repo: repo, cfg: cfg} } -// Discover answers one intent with one page of catalogs, the page it applied, -// and the retrieval modes that did not contribute. The degraded list is -// returned beside the message rather than inside it, because it reaches the -// caller as a header (C11). +// Discover answers one intent with one page of catalogs and the retrieval modes +// that did not contribute. The degraded list is returned beside the catalogs +// 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.OnDiscoverAction, []string, error) { @@ -69,15 +68,14 @@ func (s *Service) Discover( degraded = append(degraded, result.Degraded...) observeRetrieval(ctx, modes, degraded) - // Capped at the retrieval depth, the last page a caller can fetch; - // a true count past it is a count query per request (A19). - total := min(result.Total, s.cfg.Search.MaxCandidatesPerMode) - return beckn.OnDiscoverAction{ Catalogs: render(result.Catalogs), - // What MapIntent applied, not what was asked: a caller that sent 5000 - // must read back the 100 it got, or its next offset skips results. - Pagination: &beckn.Pagination{Limit: query.Limit, Offset: query.Offset, Total: &total}, + // The applied page, and total capped at the retrieval depth (ADR-0017). + Pagination: &beckn.DiscoverPagination{ + Limit: query.Limit, + Offset: query.Offset, + Total: min(result.Total, s.cfg.Search.MaxCandidatesPerMode), + }, }, degraded, nil } diff --git a/src/discover/service_test.go b/src/discover/service_test.go index 543a1cf..aa67b29 100644 --- a/src/discover/service_test.go +++ b/src/discover/service_test.go @@ -397,8 +397,7 @@ func TestAnOversizeLimitIsClampedAndAnUnreachablePageIsRefused(t *testing.T) { } } -// Total is what the backend matched, capped at the retrieval depth: past it no -// page can be fetched, so a larger number would point the caller at a refusal. +// 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}} @@ -408,8 +407,8 @@ func TestTheTotalIsReturnedAndCappedAtTheRetrievalDepth(t *testing.T) { if err != nil { t.Fatalf("Discover: %v", err) } - if got := answer.Pagination.Total; got == nil || *got != want { - t.Errorf("matched %d: Total = %v, want %d", matched, got, want) + if got := answer.Pagination.Total; got != want { + t.Errorf("matched %d: Total = %d, want %d", matched, got, want) } } } diff --git a/src/domain/query.go b/src/domain/query.go index 8beaee6..f6b3b55 100644 --- a/src/domain/query.go +++ b/src/domain/query.go @@ -126,7 +126,7 @@ type SearchQuery struct { // // Degraded names the retrieval modes that did not contribute and reaches the // caller as the X-Beckn-Degraded header, not a body key (C11). Total counts -// the ranked ids before paging, not a separate count query (A19). +// the ranked ids before paging (ADR-0017). type SearchResult struct { Catalogs []Catalog Degraded []string diff --git a/src/platform/config/config.go b/src/platform/config/config.go index 0d3bb45..d98d2f6 100644 --- a/src/platform/config/config.go +++ b/src/platform/config/config.go @@ -219,19 +219,19 @@ 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. A pinned commit of the OAN profile (which adds - // discover pagination) 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 + // 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. // // 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/OpenAgriNet/protocol-specifications-v2/dfc109759b3781fde74539adc79df63fccbadb83/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 cbabfad..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 diff --git a/src/storage/postgres/fusion.go b/src/storage/postgres/fusion.go index 787495f..90cf920 100644 --- a/src/storage/postgres/fusion.go +++ b/src/storage/postgres/fusion.go @@ -36,11 +36,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/tests/testdata/beckn-v2.0.0.yaml b/tests/testdata/beckn-v2.0.0.yaml index 72a8386..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' @@ -2291,9 +2357,23 @@ components: intent: $ref: '#/components/schemas/Intent' limit: - $ref: '#/components/schemas/PaginationParams/properties/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: - $ref: '#/components/schemas/PaginationParams/properties/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 @@ -2380,7 +2460,7 @@ components: items: $ref: '#/components/schemas/Catalog' pagination: - $ref: '#/components/schemas/Pagination' + $ref: '#/components/schemas/DiscoverPagination' additionalProperties: false OnInitAction: @@ -2989,22 +3069,12 @@ components: type: string format: uuid networkId: - description: "The unique identifier(s) of the Beckn subnet(s) within which\ - \ this transaction is occurring. Actors use this value to resolve the\ - \ subnet's registry URL before performing identity and address lookups.\ - \ The RECOMMENDED format is namespace_id followed by a slash followed\ - \ by registry_id, for example acmenet.org/charge-net.\n\nMAY be a single\ - \ string (single-network transaction) or an array of strings (multi-network\ - \ transaction). When an array is provided, order is significant: actors\ - \ MUST attempt registry resolution starting with the first entry, treating\ - \ it as the primary network, and MAY fall back to subsequent entries if\ - \ resolution against the primary fails." - oneOf: - - type: string - - type: array - items: - type: string - minItems: 1 + description: "The unique identifier of the Beckn subnet within which this\ + \ transaction is occurring. Actors use this value to resolve the subnet's\ + \ registry URL before performing identity and address lookups. The RECOMMENDED\ + \ format is namespace_id followed by a slash followed by registry_id, for\ + \ example acmenet.org/charge-net." + type: string timestamp: description: The date and time at which this message was generated by the sender, expressed in RFC 3339 date-time format. The receiving actor MAY @@ -4516,32 +4586,55 @@ components: ISO 8601 timestamp set by the CS each time the subscription record is modified. - PaginationParams: + DiscoverPagination: type: object - description: >- - Reusable offset-based pagination fields shared between paginated - list requests and the Pagination response metadata that describes - them. On a request, limit/offset specify the caller's desired page; - on a response, they reflect the page actually applied by the server. + 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 - maximum: 100 description: >- - Maximum number of records in this page. On a request, the - caller's desired page size; if omitted, the server applies a - default. On a response, the page size actually applied. + 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 - default: 0 description: >- - Zero-based index of the first record in this page relative to - the full result set. On a request, specifies where to start; - on a response, reflects the offset actually applied. + 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\ \ the size, position, and count of the returned result set, enabling callers\ \ to navigate through large result sets using offset-based pagination.\n\n\ @@ -4550,16 +4643,26 @@ components: \ or PN consumes it to determine whether additional pages are available and\ \ to construct subsequent requests.\n\nRelationship: Composed within the response\ \ message objects of /catalog/subscription and /catalog/search endpoints." - allOf: - - $ref: '#/components/schemas/PaginationParams' - - type: object - properties: - total: - type: integer - minimum: 0 - description: >- - Total number of matching records across the entire result - set, not the count in this page. Assigned by the CS. + properties: + total: + type: integer + minimum: 0 + description: >- + Total number of matching records across the entire result set, + not the count in this page. Assigned by the CS. + limit: + type: integer + minimum: 1 + description: >- + Maximum number of records returned in this page. Assigned by + the CS based on the request parameter or CS policy. + offset: + type: integer + minimum: 0 + description: >- + Zero-based index of the first record in this page relative to + the full result set. Assigned by the CS based on the request + parameter. CatalogSearchAction: type: object From 72afbaac4b06da5342cd0fa0921cde2d26063415 Mon Sep 17 00:00:00 2001 From: Manjunath DR Date: Tue, 27 Oct 2026 20:41:20 +0530 Subject: [PATCH 6/9] fix: let the last page be reached at any limit [#51] Both depth guards refused a page whose tail crossed the retrieval depth, but the page slice already truncates that tail. At depth 500, limit 30 refused offset 480 with 30 results still unread, and the caller could never satisfy the spec's end-of-results test (offset + count == total). It only looked right at limit 20, which divides 500 evenly. The guard now reads the offset alone: a page that starts past the depth is refused, one that starts inside it is answered, short if need be. --- src/discover/intent_mapper.go | 6 +++--- src/discover/intent_mapper_test.go | 14 ++++++++------ src/discover/service_test.go | 2 +- src/storage/postgres/search_repository.go | 6 +++--- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/discover/intent_mapper.go b/src/discover/intent_mapper.go index 0231f24..141deff 100644 --- a/src/discover/intent_mapper.go +++ b/src/discover/intent_mapper.go @@ -222,13 +222,13 @@ func mapPage(page Page, search config.Search) (limit, offset int, faults []domai offset = 0 } - if offset+limit > search.MaxCandidatesPerMode { + if offset >= search.MaxCandidatesPerMode { 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_test.go b/src/discover/service_test.go index aa67b29..61bf06e 100644 --- a/src/discover/service_test.go +++ b/src/discover/service_test.go @@ -392,7 +392,7 @@ 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") } } diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index e7cb62e..97605b1 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -135,10 +135,10 @@ func (s *SearchRepository) Search( // 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 { + if query.Offset >= s.search.MaxCandidatesPerMode { 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 } From 2751b88814f3d5e79c094fd423f2ee5c019987da Mon Sep 17 00:00:00 2001 From: Manjunath DR Date: Tue, 27 Oct 2026 21:13:36 +0530 Subject: [PATCH 7/9] fix: keep the page inside the total it is paged against [#51] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The depth counts ids PER MODE, so fusing two modes yields more ids than either retrieved — 14 from a depth of 8 on the test corpus. Only `total` was trimmed to the depth; the page was still cut from the untrimmed list, so the last page carried more resources than `total` left room for and offset + count == total never came true. Trim the fused list once, before the page is cut, so the page and the total come from the same list. --- src/storage/postgres/read_path_test.go | 52 +++++++++++++++++++++++ src/storage/postgres/search_repository.go | 7 +++ 2 files changed, 59 insertions(+) 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 97605b1..b99880d 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -121,7 +121,14 @@ func (s *SearchRepository) Search( lists = append(lists, ids) } + // Trimmed to the depth BEFORE the page is cut, because the cap counts ids + // per mode and fusing several of them yields more than any one retrieved. + // Total is this list's length, so a page cut from an untrimmed one would run + // past the total the caller pages against. fused := RRF(lists...) + if len(fused) > s.search.MaxCandidatesPerMode { + fused = fused[:s.search.MaxCandidatesPerMode] + } page := pageOf(fused, query.Offset, query.Limit) catalogs, err := s.hydrator.Hydrate(ctx, page, scope) From 8b5facfdb5bd690260583ca03b5642371bc61b2e Mon Sep 17 00:00:00 2001 From: Manjunath DR Date: Tue, 27 Oct 2026 22:01:57 +0530 Subject: [PATCH 8/9] refactor: give the retrieval depth one owner [#51] The depth rule was written in two places and the page was read by a function doing two jobs. - Search.OffsetBeyondDepth and Search.CapToDepth hold the comparison once; the request layer and the storage layer both call them. - pageFrom reads the signed body; rejectQueryStringPagination refuses a page the signature does not cover. - SearchResult.Total says what it holds and where it is capped. - ADR-0017 records why the cap is applied before the page is cut, and why the refusal tests the offset rather than offset + limit. --- docs/adr/0017-discover-pagination.md | 13 ++++++++++++- src/discover/controller.go | 18 ++++++++++++------ src/discover/intent_mapper.go | 2 +- src/discover/service.go | 6 ++++-- src/discover/service_test.go | 8 ++++---- src/domain/query.go | 9 +++++++-- src/platform/config/config.go | 22 +++++++++++++++++++--- src/storage/memory/search.go | 2 ++ src/storage/postgres/search_repository.go | 11 +++-------- 9 files changed, 64 insertions(+), 27 deletions(-) diff --git a/docs/adr/0017-discover-pagination.md b/docs/adr/0017-discover-pagination.md index 3e810c4..f69eb4e 100644 --- a/docs/adr/0017-discover-pagination.md +++ b/docs/adr/0017-discover-pagination.md @@ -25,7 +25,13 @@ signature does not cover. The OAN profile of the spec moves them into still reports it. 5. **`total` is the ranked list's length, capped at `maxCandidatesPerMode`** — the last page a caller can fetch. -6. **Order is stable across pages.** RRF breaks score ties by id. +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 @@ -48,3 +54,8 @@ signature does not cover. The OAN profile of the spec moves them into 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/src/discover/controller.go b/src/discover/controller.go index 77b84ac..b57da02 100644 --- a/src/discover/controller.go +++ b/src/discover/controller.go @@ -66,11 +66,11 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { observeIntent(ctx, envelope.Context, action.Intent) - page, err := pageFrom(r.URL.Query(), action) - 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) answer, degraded, err := c.service.Discover(ctx, envelope.Context, action.Intent, page) if err != nil { @@ -95,13 +95,19 @@ func (c *Controller) Discover(w http.ResponseWriter, r *http.Request) { } } -// pageFrom reads the page from the body; ?limit= is refused (ADR-0017). -func pageFrom(query url.Values, action beckn.DiscoverAction) (Page, error) { +// 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 Page{}, apperrors.Schema(beckn.CodeSchemaInvalidFormat, + return apperrors.Schema(beckn.CodeSchemaInvalidFormat, "pagination moved from the query string to message.limit and message.offset").At(messageRoot) } - return Page{Limit: action.Limit, Offset: action.Offset}, nil + return 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/intent_mapper.go b/src/discover/intent_mapper.go index 141deff..0f935a1 100644 --- a/src/discover/intent_mapper.go +++ b/src/discover/intent_mapper.go @@ -222,7 +222,7 @@ func mapPage(page Page, search config.Search) (limit, offset int, faults []domai offset = 0 } - if offset >= search.MaxCandidatesPerMode { + if search.OffsetBeyondDepth(offset) { faults = append(faults, domain.Fault{ Path: "$['offset']", Code: string(beckn.CodeSchemaInvalidFormat), diff --git a/src/discover/service.go b/src/discover/service.go index cf081bb..2b697ae 100644 --- a/src/discover/service.go +++ b/src/discover/service.go @@ -70,11 +70,13 @@ func (s *Service) Discover( return beckn.OnDiscoverAction{ Catalogs: render(result.Catalogs), - // The applied page, and total capped at the retrieval depth (ADR-0017). + // 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: min(result.Total, s.cfg.Search.MaxCandidatesPerMode), + Total: s.cfg.Search.CapToDepth(result.Total), }, }, degraded, nil } diff --git a/src/discover/service_test.go b/src/discover/service_test.go index 61bf06e..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 diff --git a/src/domain/query.go b/src/domain/query.go index f6b3b55..bf5e486 100644 --- a/src/domain/query.go +++ b/src/domain/query.go @@ -125,8 +125,13 @@ 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). Total counts -// the ranked ids before paging (ADR-0017). +// 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 diff --git a/src/platform/config/config.go b/src/platform/config/config.go index d98d2f6..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), diff --git a/src/storage/memory/search.go b/src/storage/memory/search.go index 0e88f94..ec545a4 100644 --- a/src/storage/memory/search.go +++ b/src/storage/memory/search.go @@ -62,6 +62,8 @@ 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, diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index b99880d..a733888 100644 --- a/src/storage/postgres/search_repository.go +++ b/src/storage/postgres/search_repository.go @@ -121,14 +121,9 @@ func (s *SearchRepository) Search( lists = append(lists, ids) } - // Trimmed to the depth BEFORE the page is cut, because the cap counts ids - // per mode and fusing several of them yields more than any one retrieved. - // Total is this list's length, so a page cut from an untrimmed one would run - // past the total the caller pages against. + // Capped before the page is cut, not after (ADR-0017). fused := RRF(lists...) - if len(fused) > s.search.MaxCandidatesPerMode { - fused = fused[:s.search.MaxCandidatesPerMode] - } + fused = fused[:s.search.CapToDepth(len(fused))] page := pageOf(fused, query.Offset, query.Limit) catalogs, err := s.hydrator.Hydrate(ctx, page, scope) @@ -142,7 +137,7 @@ func (s *SearchRepository) Search( // 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 >= s.search.MaxCandidatesPerMode { + if s.search.OffsetBeyondDepth(query.Offset) { return fmt.Errorf( "%w: offset %d starts past the %d ids a mode retrieves", domain.ErrRetrievalDepth, query.Offset, s.search.MaxCandidatesPerMode) From 0e32b2232381ddfd3c60eb09654fcf3e33a2412d Mon Sep 17 00:00:00 2001 From: Manjunath DR Date: Tue, 27 Oct 2026 22:34:08 +0530 Subject: [PATCH 9/9] refactor: name the two kinds of ranked list apart [#51] `ranked` meant the ranked modes in one function and the ranked id lists in another, in the same file, and `fold` said nothing about what it folded into. - negotiate returns rankedModes, partitionOutcomes returns rankedIDs. - withinRetrievalDepth reads as a predicate but returns an error, so it is ensureWithinRetrievalDepth. - pageOf's manual clamp is min(). - RRF presizes for the fused worst case: 6 allocations and 79KB against 32 and 169KB, at the same wall time. --- src/storage/memory/search.go | 6 +--- src/storage/postgres/fusion.go | 12 ++++++-- src/storage/postgres/search_repository.go | 36 ++++++++++------------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/storage/memory/search.go b/src/storage/memory/search.go index ec545a4..e8c904e 100644 --- a/src/storage/memory/search.go +++ b/src/storage/memory/search.go @@ -288,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 90cf920..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 { diff --git a/src/storage/postgres/search_repository.go b/src/storage/postgres/search_repository.go index a733888..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,24 +105,24 @@ 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) } // Capped before the page is cut, not after (ADR-0017). - fused := RRF(lists...) + fused := RRF(rankedIDs...) fused = fused[:s.search.CapToDepth(len(fused))] page := pageOf(fused, query.Offset, query.Limit) @@ -133,10 +133,10 @@ func (s *SearchRepository) Search( 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 { +func (s *SearchRepository) ensureWithinRetrievalDepth(query domain.SearchQuery) error { if s.search.OffsetBeyondDepth(query.Offset) { return fmt.Errorf( "%w: offset %d starts past the %d ids a mode retrieves", @@ -161,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 @@ -175,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 @@ -225,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 { @@ -241,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 @@ -280,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))] }