Repository navigation
feat: read discover pagination from the request body [#51] - #57
Conversation
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.
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.
🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)Go dependenciesNo findings at CRITICAL,HIGH,MEDIUM,LOW. Container imageNo findings at CRITICAL,HIGH,MEDIUM,LOW. |
|
📊 Test Coverage: ✅ Passed — 97% of changed lines covered, min 80% |
5658fef to
3b765b3
Compare
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.
3b765b3 to
47a8714
Compare
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.
| // 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"` |
There was a problem hiding this comment.
We also need to update this url in the adapter configuration
There was a problem hiding this comment.
Pinned to a8451c4, the head of protocol-specifications-v2#1, before it merges: DiscoverPagination only exists there, so binding the schema test to it and removing flattenAllOf both need it.
Will re-pin to the merged commit when it lands. The adapter config needs the same URL.
There was a problem hiding this comment.
Second review pass, after protocol-specifications-v2#1 was updated. Three spec changes affect this PR: DiscoverPagination is now a separate schema from the CS Pagination, all three of its fields are required, and maximum: 100 is gone from the request limit.
1. The spec pin stops the clamp from ever running
Problem
src/platform/config/config.go:234 and tests/testdata/beckn-v2.0.0.yaml both pin spec commit dfc1097. That commit still puts maximum: 100 on the request limit, and that has since been reverted on the spec branch.
So a request of {"intent": {"textSearch": "wheat"}, "limit": 5000} is rejected by the pinned spec with 5000 is greater than the maximum of 100, and accepted by the current spec.
SchemaValidator runs before the Discover controller (src/app/router.go:125) and EnableL1Schema is true by default. On the pinned spec the request is refused before it reaches the clamp at src/discover/intent_mapper.go:221, so the clamp never runs in production. The clamp tests at src/discover/service_test.go:375 and src/discover/controller_test.go:324 pass only because mount() does not wire the validator.
Recommendation
Re-pin SpecURL and re-vendor tests/testdata/beckn-v2.0.0.yaml once protocol-specifications-v2#1 merges. The clamp logic itself is correct and matches CON-013-31 — only the pin needs to move.
2. The conformance test checks the wrong schema
Problem
src/beckn/schema_conformance_test.go:72 maps the Go Pagination struct to schemaPath("Pagination"). In the updated spec, #/components/schemas/Pagination is the CS catalog schema, and on_discover now uses DiscoverPagination.
The test passes either way, so nothing tells you it is testing a contract this service does not serve. The two schemas really do differ: {"limit": 10, "offset": 0} is accepted by Pagination and rejected by DiscoverPagination with 'total' is a required property.
Recommendation
Bind the struct to DiscoverPagination, and rename the Go type to DiscoverPagination too. Then the next spec rename fails the test instead of quietly pointing it somewhere else.
3. Total *int with omitempty can produce an invalid response
Problem
src/beckn/actions.go:162. total is a required field now, so if Total is ever nil the response omits it and fails validation. There is one construction site today (src/discover/service.go:80) and it always sets the value, so this is a trap for later rather than a live bug. But a pointer with omitempty says the field is optional, and the contract says it is required.
Recommendation
Change it to Total int with json:"total".
4. flattenAllOf is dead code
Problem
src/beckn/schema_conformance_test.go:167. Its comment describes Pagination as allOf[PaginationParams, {total}]. That composition was reverted on the spec branch, and no schema the test binds uses allOf any more.
Recommendation
Delete the helper and the comment.
Resolved since the last pass
- Capping
totalatMaxCandidatesPerModeis fine now. The spec definestotalas how far a caller can page, which a DS may cap at its retrieval depth. CON-013-30asks for a stable order within one result set, with no resource repeated or skipped across its pages.RRFalready breaks ties by id atsrc/storage/postgres/fusion.go:44, and the comment above it gives the same page-slice reasoning. No change needed.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Three more findings, on process and readability rather than behaviour.
5. No self review
Problem
The PR is authored by @tdprajwal, and the only review activity on it is mine. The author has not gone through their own diff and left comments on it.
Recommendation
Self review the diff before this goes further. Walk the change, comment on anything a reviewer would ask about, and resolve what you can yourself. A PR without a self review should not be taken up for line-by-line review.
6. The PR description says too much
Problem
The description carries What, Why and a Testing section that lists each test that runs. Most of it restates the diff, which a reviewer can read.
Recommendation
Cut it to two things: what the problem is, and what this PR does about it. Something like:
Discover took
limitandoffsetfrom the query string, which the request signature does not cover. This moves them into the request body and returns the applied page asmessage.pagination.
Drop the Testing list. CI reports whether tests pass.
7. Decisions are written as code comments instead of ADRs
Problem
43 of the 168 added lines in src/ are comments, and the valuable ones record decisions rather than explain code:
src/discover/intent_mapper.go:213— why an oversize limit is clamped and not refused.src/discover/service.go:78— why the applied page is echoed and not the requested one.src/storage/postgres/fusion.go:39— why RRF ties break by id, and what happens to a page if they do not.src/platform/config/config.go:640— whymaxPageSizecannot sit belowdefaultPageSize.
These are design decisions. In a comment they are invisible to anyone who is not already reading that file, and they go stale when the decision changes.
The repo already keeps ADRs in docs/adr/, and none of them covers pagination.
Recommendation
Write one ADR, docs/adr/00NN-discover-pagination.md, holding these decisions: pagination is read from the signed body, counted in resources, clamped rather than refused, total capped at retrieval depth, and order made stable by the id tiebreak. Then cut the comments down to a line each, pointing at the ADR.
Keep the short comments that explain something local and non-obvious. Remove the ones that narrate what the code already says.
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.
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.
|
Pushed Problem Both depth guards refused a page whose end crossed the retrieval depth: if offset+limit > search.MaxCandidatesPerMode {But That also means the end-of-results test in the spec — Recommendation Read the offset alone. A page that starts past the depth is refused; a page that starts inside it is answered, short if need be. if offset >= search.MaxCandidatesPerMode {Same change in Checked against the docker stack with
After the change
|
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.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Design-principles pass over the full branch diff (20 files, +386/−113).
1. SearchResult.Total does not say what it holds, and the depth cap is applied twice
Problem
src/domain/query.go:128 says "Total counts the ranked ids before paging". That is no longer true for the Postgres backend — src/storage/postgres/search_repository.go:130 now trims the fused list to MaxCandidatesPerMode before counting it, so Total already comes back capped. src/discover/service.go:77 then caps it a second time with min(result.Total, s.cfg.Search.MaxCandidatesPerMode).
Two layers owning the same rule means a backend can disagree with the doc and nothing catches it. The memory backend already does: src/storage/memory/search.go:68 sets Total: len(page) and never mentions MaxCandidatesPerMode. It is test-only today, so no caller sees it, but it is the same interface returning a different number under the same field name.
Recommendation
Make the contract explicit on the type: change the comment at src/domain/query.go:128 to "Total is the number of ranked ids a caller can page to — the ranked list capped at the retrieval depth. Every backend caps it." Then drop the min() at src/discover/service.go:77 and pass result.Total straight through, and fix src/storage/memory/search.go:68 to return the capped list length so the two backends honour one contract.
2. The retrieval-depth guard lives in two layers
Problem
The same rule — refuse an offset at or past MaxCandidatesPerMode — is written twice, once at src/discover/intent_mapper.go:225 and once at src/storage/postgres/search_repository.go:145. They are not a validation plus a cheap assertion; they are the same comparison with two different messages.
The evidence is this PR: fixing the off-by-one needed both to change in lockstep. Miss one and the service either refuses a page the storage layer would have served, or serves a page the mapper thought it had rejected.
Recommendation
Put the comparison in one place — a small func OffsetBeyondDepth(offset, depth int) bool next to the search config — and have both call sites use it. Keep the two different error types (one is a client fault, one is an internal guard); share only the condition.
3. pageFrom takes a parameter it does not use for its stated job
Problem
src/discover/controller.go:99 is pageFrom(query url.Values, action beckn.DiscoverAction) (Page, error). The page is built entirely from action. query exists only so the function can reject a caller that passed ?limit= or ?offset=. The name says it reads a page; it also enforces a transport rule.
Recommendation
Split them. Keep pageFrom(action beckn.DiscoverAction) Page with no error return, and move the query-string check into its own rejectQueryStringPagination(query url.Values) error called before it in the handler. Each then does one thing and the signature stops lying.
4. The reason for the pre-page trim is a code comment, not the ADR
Problem
src/storage/postgres/search_repository.go:124-127 carries a four-line comment explaining why the fused list is trimmed before the page is cut — that the cap counts ids per mode, so fusing several modes yields more than any one retrieved. That is a design decision, and it sits in the only place a reader will not look for one.
ADR-0017 decision 5 states the outcome ("total is the ranked list's length, capped at maxCandidatesPerMode") but not that the cap must be applied before the slice, which is the part that was actually wrong.
Recommendation
Add to docs/adr/0017-discover-pagination.md under Decision: "The cap is applied to the fused list before the page is cut. maxCandidatesPerMode counts ids per retrieval mode, so a fused list is longer than any one mode returned; slicing first produces a page that runs past the total the caller pages against." Then cut the comment at search_repository.go:124-127 down to one line pointing at the ADR.
Resolved
- The four findings from the earlier pass are fixed in
ff7b9fd. - The PR description is down to two lines and no longer restates the diff.
- ADR-0017 is added and covers the signed-body page, the clamp, the applied-page echo and the capped
total. - A self review is present, though a single comment across a 20-file diff is thin.
Correction to my earlier comment
I wrote that go test ./... and make lint are both clean. The test claim stands; the lint claim does not. make lint reports 10 staticcheck SA5011 issues on this branch and 12 on origin/development. All of them are in test files this PR does not touch, and all are the usual false positive where staticcheck does not know t.Fatalf ends the test. The PR lowers the count rather than raising it, but the gate is red on both branches and I should not have said otherwise.
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.
`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.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Pagination verified end to end against the running stack at two retrieval depths: no overlap or gaps across limits 1-20, every page matches the canonical slice for its offset, the short last page satisfies offset + count == total, and an offset at or past the depth is refused. 26/26 test packages pass and CI is green.
Two notes for the record, neither blocking: make lint fails with 5 pre-existing staticcheck SA5011 in untouched test files (development has 12), and the in-memory backend leans on the service to cap Total rather than capping it itself, which is documented on domain.SearchResult.Total.
Discover took limit and offset from the query string, which the request signature does not cover. This moves them into the request body and returns the applied page, with a total capped at the retrieval depth, as message.pagination.
Breaking: ?limit= and ?offset= are now refused with a 400.
Closes #51