feat(toolkit)!: bind $top/$skiptoken, reject other $ options - #4422
Merged
MikeFalcon77 merged 1 commit intoAug 11, 2026
Conversation
📝 WalkthroughWalkthroughOData extraction now accepts ChangesOData query validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request as Raw query request
participant Extractor as OData extractor
participant Params as ODataParams
Request->>Extractor: Provide raw query pairs
Extractor->>Extractor: Reject unsupported dollar-prefixed options
Extractor->>Params: Deserialize accepted aliases
Params-->>Extractor: Return bound OData parameters
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
code-ranker report for this PR (built on fork): https://reports.code-ranker.com/HFjerQ5owagxuTPLMc9q2w/ |
capybutler
force-pushed
the
fix/odata-top-alias
branch
from
August 5, 2026 14:29
ddabffe to
6a74bb6
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
capybutler
marked this pull request as ready for review
August 6, 2026 04:33
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/toolkit/src/api/odata_tests.rs (1)
267-295: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete error contract in both tests.
The
$top=0test checks the status and field, but not theINVALID_LIMITreason. The conflict test checks the reason, but its comment promises thequeryfield without asserting it.Suggested assertions
assert_eq!( violations[0].get("field").and_then(|f| f.as_str()), Some("$top"), "violation was {:?}", violations[0] ); + assert_eq!( + violations[0].get("reason").and_then(|r| r.as_str()), + Some("INVALID_LIMIT"), + "violation was {:?}", + violations[0] + ); assert_eq!( violations[0].get("reason").and_then(|r| r.as_str()), Some("INVALID_QUERY_PARAMS"), "violation was {:?}", violations[0] ); + assert_eq!( + violations[0].get("field").and_then(|f| f.as_str()), + Some("query"), + "violation was {:?}", + violations[0] + );Also applies to: 297-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/toolkit/src/api/odata_tests.rs` around lines 267 - 295, Complete the error-contract assertions in test_extract_odata_query_top_zero_error and the related conflict test: verify the $top=0 violation includes the INVALID_LIMIT reason, and assert the conflict violation contains the promised query field. Preserve the existing status, field, and conflict assertions while validating these additional fields in the parsed field_violations entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/toolkit_unified_system/07_odata_pagination_select_filter.md`:
- Around line 377-379: Update the InvalidLimit entry in the error table to
document the current $top=0 behavior: HTTP 400 with field_violations[0].field
set to $top, replacing the stale HTTP 422 status and description. Keep the table
consistent with the pagination guidance near the $top validation section.
In `@libs/toolkit/src/api/odata.rs`:
- Around line 25-34: Add endpoint-specific maximum page-size validation in
ODataParams before calling ODataQuery::with_limit(limit), applying the same cap
to both limit and $top aliases through ODataQuery.limit. Add an integration test
covering values above the endpoint limit for both parameter spellings and verify
the expected rejection or capped behavior.
---
Nitpick comments:
In `@libs/toolkit/src/api/odata_tests.rs`:
- Around line 267-295: Complete the error-contract assertions in
test_extract_odata_query_top_zero_error and the related conflict test: verify
the $top=0 violation includes the INVALID_LIMIT reason, and assert the conflict
violation contains the promised query field. Preserve the existing status,
field, and conflict assertions while validating these additional fields in the
parsed field_violations entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef85d23a-cbc1-4227-97e6-1874211012c9
📒 Files selected for processing (6)
docs/toolkit_unified_system/07_odata_pagination_select_filter.mdlibs/toolkit-odata/src/lib.rslibs/toolkit-odata/src/limits.rslibs/toolkit-odata/src/problem_mapping.rslibs/toolkit/src/api/odata.rslibs/toolkit/src/api/odata_tests.rs
capybutler
force-pushed
the
fix/odata-top-alias
branch
2 times, most recently
from
August 6, 2026 10:26
d77d021 to
c19ea5d
Compare
MikeFalcon77
reviewed
Aug 7, 2026
|
|
||
| - OpenAPI cannot express one parameter under two names. Publish the spelling | ||
| your gear treats as canonical and mention the other in its `description`. | ||
| - `$top=0` is rejected (`InvalidLimit` → `400`, `field_violations[0].field` is |
Collaborator
There was a problem hiding this comment.
This section needs a third bullet about $skip, and probably a code change too.
OData clients send $top and $skip as a pair. Before this PR both were dropped, so a client that tried to paginate got the same page every time and noticed on request two. After this PR $top works and $skip is still dropped, so the page size changes, the offset never does, and the client gets page one forever while believing it is paging through the set. That is harder to debug than the old behavior, not easier.
Two options:
Reject $skip with 400 InvalidArgument, field $skip, detail pointing at cursor. Costs one more field on ODataParams and one check in the extractor. A client that sends it finds out immediately.
Document it here: state that $skip is not supported, that offset paging is not the model, and that cursor is the replacement.
I would do both. Option 2 alone still leaves the silent-drop behavior in place for anyone who doesn't read the docs, and the whole point of binding $top is that clients now expect canonical OData paging to work.
Same question applies to $count, though it's less urgent — it has no working equivalent under another name, so nobody is half-served by it.
Contributor
Author
There was a problem hiding this comment.
Both, and the reject generalized past $skip / $count.
ODataParams claimed the whole $ namespace but bound four options, so every other $ key was accepted and discarded — not only $skip and $count, but $expand, $search, $format, miscased $Top, and typos like $filtre (a dropped $filtre returns the unfiltered collection with a 200). A dedicated $skip field would have closed two holes out of a class. extract_odata_query now refuses any $ key outside ACCEPTED_SYSTEM_QUERY_OPTIONS — $filter, $orderby, $select, $top, $skiptoken — before any value parsing: 400, one UNSUPPORTED_QUERY_PARAM violation per offending key, deduplicated, in wire order, so one round trip names every mistake.
The spec makes this obligatory rather than a preference, which is worth having in the PR — OASIS OData 4.01 Part 1: Protocol, §6.1 "Query Option Extensibility":
OData services SHOULD NOT require any query options to be specified in a request. Services SHOULD fail any request that contains query options that they do not understand and MUST fail any request that contains unsupported OData query options defined in the version of this specification supported by the service.
$skip and $count are OData-defined and unsupported here, so refusing them is the MUST; $filtre is the SHOULD case. §6.1 also reserves the $ and @ prefixes for OData, and that is what keeps this guard off the unprefixed namespace: q, context, offset belong to the handler's own params struct, with a test pinning that they still pass. Refusing unprefixed accidents like ?status=approved stays each gear's business — AM's reject_non_odata_params.
One correction to the framing, which does not change the conclusion. The observable symptom is the same before and after: identical rows on request two. What binding $top alone changed is the signal — pre-PR a wrong page size was an immediate tell that the server ignored OData, and post-PR the page size looks right, so the repeated page is the only tell left. Worse signal, not a new failure mode.
$skip is refused rather than implemented because offset paging is not the model at any layer: ODataQuery carries no offset field and paginate_odata is keyset-only, so honoring it means adding a pagination mode, not binding a parameter. Its violation detail points at $skiptoken / cursor.
On $count: same treatment, and not planned. PageInfo is {next_cursor, prev_cursor, limit}, so there is no total to report, and adding one means the COUNT(*) per page that keyset pagination exists to avoid. The violation detail says exactly that.
Two things that came out of this and are worth flagging:
$skiptokenis now acursoralias, same one-slot-two-spellings shape as$top. It is OData's opaque continuation token for server-driven paging, which is whatPageInfo.next_cursoralready is, andresource-group's own docs advertise the spelling — without the alias the new catch-all would have started 400ing it.- AM's comment was already wrong about this.
reject_non_odata_paramsdocumented that "$-prefixed keys are intentionally out of scope: theODataextractor parses them and rejects unknown ones ($filtreetc.)". That was false when written, and the gear built its non-$guard on top of the assumption; its test pinned$skip/$countas keys AM accepts. Both now match the extractor.
Two decisions I would rather you weigh in on than settle myself:
- Binding stays case-sensitive. Part 2 §5.1 says a 4.01 service MUST accept system query option names case-insensitively and with or without the
$prefix — so strictly,$Topand an unprefixedtopshould both bind. Neither does; both now answer 400 naming the spelling that binds, which is at least not a silent drop. Full conformance (normalize casing, accepttop/filter/orderby/select) is a larger change and is documented as a separate one. Reasonable line to draw? - Cursor violations are still keyed to
cursor, not$skiptoken, while page-size violations are keyed to the canonical$topwith both spellings in the description. Renaming the cursor key would change error payloads for every existing cursor caller with no functional gain, so I left the asymmetry. Say if you would rather it match$top's treatment.
Also worth knowing: chat-engine's /search binds its own $top / $skip in SearchQuery outside this extractor, so $skip keeps working there and answers 400 everywhere else. The PR documents that inconsistency without resolving it.
Twelve extractor tests cover this (each watched fail first), including one that pins the non-$ namespace as untouched. The third bullet you asked for plus a continuation-token section and an "Unsupported system query options" section — accepted set, refusal table, the §6.1 quote, and the two boundaries above — are in docs/toolkit_unified_system/07_odata_pagination_select_filter.md.
`$top` is canonical OData (OASIS OData 4.01 Part 2: URL Conventions, §5.1.6 "System Query Options $top and $skip"), and this repo already documented it, tested it, and named it in error payloads. Nothing bound it. The wire binding for the OData family lives in exactly one place, `toolkit::api::odata::ODataParams`, which declared an unprefixed `limit` with no `$top` field and no alias; `toolkit-odata` does no HTTP query parsing at all. So `?$top=<n>` was accepted and discarded on every endpoint using the `OData` extractor, and `limit` was the only page-size parameter that worked. Bind it as `#[serde(alias = "$top")]` on `ODataParams.limit` — one slot, two accepted spellings. This is the alias option from constructorfabric#4413: `limit` keeps working for every current caller, the platform matches canonical OData, and the `$top` field violation in `problem_mapping.rs` becomes truthful without a rename. Binding `$top` alone would have made the `$` namespace worse rather than better, which is the second half of this change. `ODataParams` claims every `$`-prefixed key by convention but bound four of them, and axum drops query keys no extractor claimed. A client paginating the canonical way sends `$top` with `$skip`: before, both were dropped and the wrong page size was an immediate tell; with `$top` bound and `$skip` still dropped, the page size is honored, the offset silently is not, and the client re-reads page one while believing it advanced. OData does not leave this to taste. Part 1: Protocol, §6.1 "Query Option Extensibility": services "SHOULD fail any request that contains query options that they do not understand and MUST fail any request that contains unsupported OData query options defined in the version of this specification supported by the service". The same section reserves the `$` and `@` prefixes for OData, which is why this guard polices `$` keys only — an unprefixed key belongs to the handler's own params struct (chat-engine's `q`, file-storage's `offset`), and refusing those stays each gear's business (account-management has `reject_non_odata_params`). The extractor now binds five options and refuses every other `$` key with 400, before parsing any value: * `$filter`, `$orderby`, `$select`, `$top`, `$skiptoken`, listed in `ACCEPTED_SYSTEM_QUERY_OPTIONS`. * `$skiptoken` is a second alias in the same shape as `$top`, folding onto `ODataParams.cursor`. It is OData's opaque continuation token for server-driven paging, which is what `PageInfo.next_cursor` already is, and resource-group's own docs advertise the spelling. * One `UNSUPPORTED_QUERY_PARAM` field violation per offending key, deduplicated, in wire order, so one round trip names every mistake. * The description separates the three cases a caller can hit: `$skip` / `$count` / `$format` are OData-defined but unsupported and each says what to send instead; `$Top` is a supported option in a spelling that does not bind, and names the one that does; `$filtre` is not an OData option at all and reads as unknown, not unsupported. `$skip` is refused rather than implemented because offset paging is not this platform's model: `ODataQuery` carries no offset field and `paginate_odata` is keyset-only, so honoring it would mean a new pagination mode rather than a bound parameter. `$count` has no truthful answer either — `PageInfo` is `{next_cursor, prev_cursor, limit}`, and serving a total would mean the `COUNT(*)` per page that keyset pagination exists to avoid. Binding stays case-sensitive, which Part 2 §5.1 asks a 4.01 service not to do: it wants option names accepted case-insensitively and with or without the `$` prefix. Neither `$Top` nor an unprefixed `top` binds here, and both now answer 400 naming the spelling that does instead of being dropped. Closing that gap is a separate conformance change and is documented as one. Docs in the same crates stopped describing things as they were not: * `toolkit-odata::limits` documented `ODataLimits::max_top` as an enforced "maximum $top value (default 1000)". Nothing in the repo constructs `ODataLimits`, so nothing enforced it. The module docs now state that it is an opt-in validator and point at what does cap each request. Deleting the type was the alternative and was rejected: it is a `pub` re-export, so removal would break downstream compilation for no gain. * The toolkit OData guide documents both page-size spellings, the ambiguity rule, the continuation token, and the refused `$` options with the §6.1 requirement behind them. * `account-management`'s `reject_non_odata_params` claimed `$`-prefixed keys were "out of scope: the `OData` extractor parses them and rejects unknown ones (`$filtre` etc.)". That was false when written — it is the hole this change closes — and its test pinned `$skip` / `$count` as keys AM accepts. Both now match the extractor. Also corrects that guide's error table, which listed HTTP 422 for every `OData` error variant. All of them except `Db` map through `OdataError::invalid_argument()`, which renders as 400, and the canonical error taxonomy has no 422 status at all — so no `OData` error could ever have produced one. This is a pre-existing documentation error independent of `$top`; five of the six corrected rows concern `$filter`, `$orderby`, and cursors. Fixed here because the same table names `InvalidLimit`. Scope is the toolkit crates plus the one account-management comment this change falsifies. The gear-side items in constructorfabric#4413 — the vacuous `$top` clamp test in account-management and the chat-engine `SearchQuery` doc comment that credits `toolkit-odata` for its own serde renames — are left to a separate change. BREAKING CHANGE: two wire-behavior changes on every endpoint using the `OData` extractor. `?$top=<n>` now sets the page size instead of being silently ignored. A caller that sent `$top` and got the endpoint default now gets the page size it asked for; gears that reject an out-of-range page size rather than clamping (usage-collector, `$top > 1000`) now answer 400 where they answered 200. Sending `$top` and `limit` in one request is ambiguous and answers 400 `INVALID_QUERY_PARAMS`. Any `$`-prefixed query key outside `$filter`, `$orderby`, `$select`, `$top` and `$skiptoken` now answers 400 `UNSUPPORTED_QUERY_PARAM` where it previously answered 200 with the key discarded: `$skip`, `$count`, `$expand`, `$search`, `$format`, miscased spellings such as `$Top`, and typos such as `$filtre`. Callers must drop the parameter, or replace `$skip` with the previous page's `next_cursor` sent as `$skiptoken` or `cursor`. Unprefixed parameters are unaffected, and chat-engine's `/search` binds its own `$top` / `$skip` outside this extractor and is unchanged. Refs constructorfabric#4413 Signed-off-by: capybutler <capybutler@gmail.com>
capybutler
force-pushed
the
fix/odata-top-alias
branch
from
August 10, 2026 02:16
c19ea5d to
25f6a71
Compare
capybutler
changed the title
Merged
capybutler
added a commit
to capybutler/gears-rust
that referenced
this pull request
Aug 11, 2026
…tered routes The gear published `docs/usage-collector-v1.yaml` by hand, so the document and the code could drift apart without anything failing. A new in-tree test builds the route registry from the `OperationBuilder` registrars. It needs no server, no database, and no `Service`. The test fails when the document disagrees with the code. It compares the set of operations, methods, operation ids, tags, parameters, request and response bodies, status codes, the canonical error surface, and the authentication posture. It also checks that every published component is documented and that every `$ref` resolves. Descriptions, summaries, and examples stay under manual control, and the module header states which fields the test enforces. The test reads the document with `serde-saphyr`, which is already in the lockfile, so no new crate enters the tree. The test proved that the document was wrong. Schema names now match the code (`CreateUsageRecords*`, `QueryAggregatedUsageRecordsRequest`, `Page_UsageType`). The document now lists the `metadata.<key>`, `$filter`, `$orderby`, and `$select` parameters on the routes that accept them. The `MetadataFilter` schema is gone, because no route takes that body. Page size is published under both spellings the endpoint accepts. constructorfabric#4422 bound `$top` — canonical `OData` (OASIS `OData` 4.01 Part 2, section 5.1.6) — as `#[serde(alias = "$top")]` over `ODataParams.limit`, so the extractor folds `$top` and `limit` onto one `ODataQuery.limit` slot and rejects a request carrying both as a duplicate field. The document and the `OperationBuilder` registry therefore declare both: publishing either alone would under-report the accepted surface, which is the drift class this test exists to catch. The list allowlist keeps admitting both spellings, the aggregate allowlist keeps rejecting both (aggregation is not paginated), and the `MAX_PAGE_SIZE` violation names `$top` with the alias in its detail — the parsed `ODataQuery` does not carry the spelling the caller sent, and this is the convention `toolkit_odata` already uses for `InvalidLimit`. The TimescaleDB plugin's `effective_page_size` documented its floor-to-1 as a guard against a `$top=0` "the core gateway passes through unclamped". The gateway does not: the toolkit `OData` extractor rejects a zero page size with `InvalidLimit` before any handler runs. The floor still earns its place — an in-process SDK caller builds its own `ODataQuery` and reaches neither that check nor `prepare_list_query` — so the docs and tests now state the reason that holds. `DECOMPOSITION.md`, `DESIGN.md`, `domain-model.md`, `ADR-0012`, and the usage-emission and usage-query features get the same corrections. Signed-off-by: capybutler <capybutler@gmail.com>
16 tasks
capybutler
added a commit
to capybutler/gears-rust
that referenced
this pull request
Aug 11, 2026
…tered routes The gear published `docs/usage-collector-v1.yaml` by hand, so the document and the code could drift apart without anything failing. A new in-tree test builds the route registry from the `OperationBuilder` registrars. It needs no server, no database, and no `Service`. The test fails when the document disagrees with the code. It compares the set of operations, methods, operation ids, tags, parameters, request and response bodies, status codes, the canonical error surface, and the authentication posture. It also checks that every published component is documented and that every `$ref` resolves. Descriptions, summaries, and examples stay under manual control, and the module header states which fields the test enforces. The test reads the document with `serde-saphyr`, which is already in the lockfile, so no new crate enters the tree. The test proved that the document was wrong. Schema names now match the code (`CreateUsageRecords*`, `QueryAggregatedUsageRecordsRequest`, `Page_UsageType`). The document now lists the `metadata.<key>`, `$filter`, `$orderby`, and `$select` parameters on the routes that accept them. The `MetadataFilter` schema is gone, because no route takes that body. Page size is published under both spellings the endpoint accepts. constructorfabric#4422 bound `$top` — canonical `OData` (OASIS `OData` 4.01 Part 2, section 5.1.6) — as `#[serde(alias = "$top")]` over `ODataParams.limit`, so the extractor folds `$top` and `limit` onto one `ODataQuery.limit` slot and rejects a request carrying both as a duplicate field. The document and the `OperationBuilder` registry therefore declare both: publishing either alone would under-report the accepted surface, which is the drift class this test exists to catch. The list allowlist keeps admitting both spellings, the aggregate allowlist keeps rejecting both (aggregation is not paginated), and the `MAX_PAGE_SIZE` violation names `$top` with the alias in its detail — the parsed `ODataQuery` does not carry the spelling the caller sent, and this is the convention `toolkit_odata` already uses for `InvalidLimit`. The TimescaleDB plugin's `effective_page_size` documented its floor-to-1 as a guard against a `$top=0` "the core gateway passes through unclamped". The gateway does not: the toolkit `OData` extractor rejects a zero page size with `InvalidLimit` before any handler runs. The floor still earns its place — an in-process SDK caller builds its own `ODataQuery` and reaches neither that check nor `prepare_list_query` — so the docs and tests now state the reason that holds. `DECOMPOSITION.md`, `DESIGN.md`, `domain-model.md`, `ADR-0012`, and the usage-emission and usage-query features get the same corrections. Signed-off-by: capybutler <capybutler@gmail.com>
capybutler
added a commit
to capybutler/gears-rust
that referenced
this pull request
Aug 11, 2026
…tered routes The gear published `docs/usage-collector-v1.yaml` by hand, so the document and the code could drift apart without anything failing. A new in-tree test builds the route registry from the `OperationBuilder` registrars. It needs no server, no database, and no `Service`. The test fails when the document disagrees with the code. It compares the set of operations, methods, operation ids, tags, parameters, request and response bodies, status codes, the canonical error surface, and the authentication posture. It also checks that every published component is documented and that every `$ref` resolves. Descriptions, summaries, and examples stay under manual control, and the module header states which fields the test enforces. The test reads the document with `serde-saphyr`, which is already in the lockfile, so no new crate enters the tree. The test proved that the document was wrong. Schema names now match the code (`CreateUsageRecords*`, `QueryAggregatedUsageRecordsRequest`, `Page_UsageType`). The document now lists the `metadata.<key>`, `$filter`, `$orderby`, and `$select` parameters on the routes that accept them. The `MetadataFilter` schema is gone, because no route takes that body. Page size is published under both spellings the endpoint accepts. constructorfabric#4422 bound `$top` — canonical `OData` (OASIS `OData` 4.01 Part 2, section 5.1.6) — as `#[serde(alias = "$top")]` over `ODataParams.limit`, so the extractor folds `$top` and `limit` onto one `ODataQuery.limit` slot and rejects a request carrying both as a duplicate field. The document and the `OperationBuilder` registry therefore declare both: publishing either alone would under-report the accepted surface, which is the drift class this test exists to catch. The list allowlist keeps admitting both spellings, the aggregate allowlist keeps rejecting both (aggregation is not paginated), and the `MAX_PAGE_SIZE` violation names `$top` with the alias in its detail — the parsed `ODataQuery` does not carry the spelling the caller sent, and this is the convention `toolkit_odata` already uses for `InvalidLimit`. The TimescaleDB plugin's `effective_page_size` documented its floor-to-1 as a guard against a `$top=0` "the core gateway passes through unclamped". The gateway does not: the toolkit `OData` extractor rejects a zero page size with `InvalidLimit` before any handler runs. The floor still earns its place — an in-process SDK caller builds its own `ODataQuery` and reaches neither that check nor `prepare_list_query` — so the docs and tests now state the reason that holds. `DECOMPOSITION.md`, `DESIGN.md`, `domain-model.md`, `ADR-0012`, and the usage-emission and usage-query features get the same corrections. Signed-off-by: capybutler <capybutler@gmail.com>
capybutler
added a commit
to capybutler/gears-rust
that referenced
this pull request
Aug 11, 2026
…tered routes The gear published `docs/usage-collector-v1.yaml` by hand, so the document and the code could drift apart without anything failing. A new in-tree test builds the route registry from the `OperationBuilder` registrars. It needs no server, no database, and no `Service`. The test fails when the document disagrees with the code. It compares the set of operations, methods, operation ids, tags, parameters, request and response bodies, status codes, the canonical error surface, and the authentication posture. It also checks that every published component is documented and that every `$ref` resolves. Descriptions, summaries, and examples stay under manual control, and the module header states which fields the test enforces. The test reads the document with `serde-saphyr`, which is already in the lockfile, so no new crate enters the tree. The test proved that the document was wrong. Schema names now match the code (`CreateUsageRecords*`, `QueryAggregatedUsageRecordsRequest`, `Page_UsageType`). The document now lists the `metadata.<key>`, `$filter`, `$orderby`, and `$select` parameters on the routes that accept them. The `MetadataFilter` schema is gone, because no route takes that body. Page size is published under both spellings the endpoint accepts. constructorfabric#4422 bound `$top` — canonical `OData` (OASIS `OData` 4.01 Part 2, section 5.1.6) — as `#[serde(alias = "$top")]` over `ODataParams.limit`, so the extractor folds `$top` and `limit` onto one `ODataQuery.limit` slot and rejects a request carrying both as a duplicate field. The document and the `OperationBuilder` registry therefore declare both: publishing either alone would under-report the accepted surface, which is the drift class this test exists to catch. The list allowlist keeps admitting both spellings, the aggregate allowlist keeps rejecting both (aggregation is not paginated), and the `MAX_PAGE_SIZE` violation names `$top` with the alias in its detail — the parsed `ODataQuery` does not carry the spelling the caller sent, and this is the convention `toolkit_odata` already uses for `InvalidLimit`. The TimescaleDB plugin's `effective_page_size` documented its floor-to-1 as a guard against a `$top=0` "the core gateway passes through unclamped". The gateway does not: the toolkit `OData` extractor rejects a zero page size with `InvalidLimit` before any handler runs. The floor still earns its place — an in-process SDK caller builds its own `ODataQuery` and reaches neither that check nor `prepare_list_query` — so the docs and tests now state the reason that holds. `DECOMPOSITION.md`, `DESIGN.md`, `domain-model.md`, `ADR-0012`, and the usage-emission and usage-query features get the same corrections. Signed-off-by: capybutler <capybutler@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
$topis canonical OData (OASIS OData 4.01 Part 2: URL Conventions, §5.1.6 "System Query Options$top and $skip"), and this repo already documented it, tested it, and named it in error payloads.
Nothing bound it. The wire binding for the whole OData family lives in exactly one place,
toolkit::api::odata::ODataParams, which declared an unprefixedlimitwith no$topfield and noalias — and
toolkit-odatadoes no HTTP query parsing at all. So?$top=<n>was accepted anddiscarded on every endpoint using the
ODataextractor, andlimitwas the only page-sizeparameter that actually worked.
This binds
$topas#[serde(alias = "$top")]onODataParams.limit: one slot, two acceptedspellings.
limitkeeps working for every current caller, the platform matches canonical OData, andthe
$topfield violation already emitted bytoolkit-odata::problem_mappingbecomes truthfulwithout a rename.
Binding
$topalone would have made the$namespace worse, not better — raised in review, andthe second half of this PR.
ODataParamsclaims every$-prefixed query key by convention butbound four of them, and axum drops query keys no extractor claimed. A client paginating the
canonical way sends
$topwith$skip: before, both were dropped and the wrong page size was animmediate tell; with
$topbound and$skipstill dropped, the page size is honored, the offsetsilently is not, and the client re-reads page one while believing it advanced.
OData does not leave this to taste. Part 1: Protocol, §6.1 "Query Option Extensibility":
$skipand$countare OData-defined and unsupported here, so refusing them is a MUST; a typolike
$filtreis the SHOULD case. The same section reserves the$and@prefixes for OData,which is why this guard polices
$keys only: an unprefixed key belongs to the handler's own paramsstruct (chat-engine's
q, file-storage'soffset), and refusing those stays each gear's business(account-management has
reject_non_odata_params).Changes
libs/toolkit/src/api/odata.rs#[serde(alias = "$top")]onODataParams.limit, plus rustdoc on the struct stating that thisis the single wire-binding point for OData query parameters.
#[serde(alias = "$skiptoken")]onODataParams.cursor— the same one-slot-two-spellings shapeas
$top.$skiptokenis OData's opaque continuation token for server-driven paging, which iswhat
PageInfo.next_cursoralready is, and resource-group's own docs advertise the spelling.ACCEPTED_SYSTEM_QUERY_OPTIONS($filter,$orderby,$select,$top,$skiptoken) and aguard that answers
400for every other$-prefixed key, before any value parsing. OneUNSUPPORTED_QUERY_PARAMfield violation per offending key, deduplicated, in wire order, so oneround trip names every mistake.
$skip/$count/$formatare OData-defined but unsupported and each says what to send instead;$Topis asupported option in a spelling that does not bind, and names the one that does;
$filtreis notan OData option at all and reads as
unknown query option, notunsupported.libs/toolkit-odata/src/problem_mapping.rs— theInvalidLimitviolation keeps$topas thefield name and now describes both spellings:
"Invalid page size parameter ($top, alias limit)".libs/toolkit-odata/src/limits.rs— docs only.ODataLimits::max_topwas documented as anenforced "maximum $top value (default 1000)"; nothing in the repo constructs
ODataLimits, sonothing enforced it. The module docs now say it is an opt-in validator and point at what actually
caps each request. Deleting the type was the alternative and was rejected: it is a
pubre-export, so removal breaks downstream compilation for no gain.
docs/toolkit_unified_system/07_odata_pagination_select_filter.md— the page-size section (bothspellings, the ambiguity rule,
$top=0, and the note that upper-bound capping is the handler'sjob), a continuation-token section, and an Unsupported system query options section: the
accepted set, the refusal table, the §6.1 quote behind it, and the two boundaries it implies —
unprefixed keys are out of scope, and case-sensitivity is a stated deviation from Part 2 §5.1
(which wants option names accepted case-insensitively and with or without
$) rather than anoversight. Also corrects that file's error table, which listed HTTP
422for everyODataerrorvariant: all of them except
Dbmap throughOdataError::invalid_argument(), which renders as400, and the canonical error taxonomy has no422status at all — so noODataerror could everhave produced one. That is a pre-existing documentation error independent of
$top; five of thesix corrected rows concern
$filter,$orderby, and cursors. Fixed here (raised in review)because the same table names
InvalidLimit.gears/system/account-management/.../handlers/common.rs—reject_non_odata_paramsclaimed$-prefixed keys were "out of scope: theODataextractor parses them and rejects unknown ones(
$filtreetc.)". That was false when written — it is exactly the hole this PR closes — and thegear built its non-
$guard on top of the assumption. The comment now points atACCEPTED_SYSTEM_QUERY_OPTIONS, and its test no longer pins$skip/$countas keys AM accepts.Why
$skipis refused rather than implemented. Offset paging is not this platform's model:ODataQuerycarries no offset field andpaginate_odatais keyset-only, so honoring$skipwouldmean adding a pagination mode, not binding a parameter.
$counthas no truthful answer either —PageInfois{next_cursor, prev_cursor, limit}, and serving a total would mean theCOUNT(*)perpage that keyset pagination exists to avoid.
Out of scope. Toolkit crates plus the one account-management comment this PR falsifies. The
gear-side items from #4413 — the vacuous
$topclamp test in account-management, and thechat-engine
SearchQuerydoc comment that creditstoolkit-odatafor its own serde renames — areleft to a separate change. Accepting
$Topcase-insensitively and the unprefixed canonical spellings(
top,filter) that Part 2 §5.1 asks for is a separate conformance change; this PR answers400naming the spelling that binds instead of dropping the request.
Type of Change
Breaking change details
Two wire-behavior changes, both on every endpoint using the
ODataextractor (ledger, chat-engine,mini-chat, account-management, resource-group, usage-collector).
1.
?$top=<n>now sets the page size instead of being silently ignored.$topand received the endpoint default now receives the page size it askedfor.
400where theyanswered
200. Concretely: usage-collector rejects$top > 1000(
gears/system/usage-collector/.../usage_records.rs) — that branch was unreachable from the wirebefore and is reachable now.
$topandlimitin one request is ambiguous and answers400 INVALID_QUERY_PARAMS(serde reports the second spelling as a duplicate field).
2. Unsupported
$-prefixed keys now answer400where they previously answered200with thekey discarded. Anything outside
$filter,$orderby,$select,$top,$skiptoken:$skip,$count,$expand,$search,$format,$compute,$index,$schemaversion, miscasedspellings such as
$Top, and typos such as$filtre. The response is400 UNSUPPORTED_QUERY_PARAM, one violation per offending key, each naming the key and what tosend instead.
Migration:
limitandcursor— none.$topexpecting it to be ignored must stop sending it, or send a value withinthe endpoint's cap. Do not send both spellings in one request.
$skipmust drop it and page with the previous response'spage_info.next_cursor, sent as$skiptokenorcursor.$option must drop it.Not affected: unprefixed query parameters of any name, and chat-engine's
/search, which binds itsown
$top/$skipinSearchQueryoutside this extractor and is unchanged. That leaves$skipworking on
/searchand refused elsewhere — a platform inconsistency this PR documents but does notresolve.
Version bump justification
Required by CONTRIBUTING §Enforcement. No
Cargo.tomlversions are touched in this PR —release-plz derives them from the conventional commit (
feat(toolkit)!:+BREAKING CHANGE:footer).
cf-gears-toolkit0.(x+1).0per the CONTRIBUTING table. No Rust-API break: theODataParamsfield set is unchanged (serde aliases only) andACCEPTED_SYSTEM_QUERY_OPTIONSis additive, sosemver_checkhas nothing to flag and downstream compilation is unaffected.cf-gears-toolkit-odataODataLimitsintentionally kept.cf-gears-account-management!marker describes the toolkit wire contract — if it proposes a breaking bump for AM, that is the marker leaking, not a real break.Testing
cargo test -p cf-gears-toolkit --lib odata→ 37 passed, 0 failedcargo test -p cf-gears-toolkit --lib→ 261 passed, 0 failedcargo test -p cf-gears-toolkit-odata --lib→ 111 passed, 0 failedcf-gears-account-management→ 800 passed,cf-gears-usage-collector→ 368 passedtests/api_*.rsneed Postgres). All sixextractor gears were grepped for requests carrying a now-rejected option; there are none, so
no integration expectation flips. Left to CI.
axumrequests, so no manual pass was made.Twelve tests in
libs/toolkit/src/api/odata_tests.rs, each watched fail before the code existed:test_extract_odata_query_top_binds_limit?$top=10reachesODataQuery.limittest_extract_odata_query_top_zero_error?$top=0→400,field_violations[0].field == "$top", codeINVALID_LIMITtest_extract_odata_query_top_and_limit_conflict?$top=20&limit=50→400 INVALID_QUERY_PARAMStest_odata_extractor_top_aliasODataextractor with$filter+$toptest_extract_odata_query_skiptoken_binds_cursor?$skiptoken=<encoded CursorV1>reachesODataQuery.cursortest_extract_odata_query_skip_rejected?$skip=20→400, field$skip, reasonUNSUPPORTED_QUERY_PARAM, detail names$skiptokentest_extract_odata_query_count_rejected?$count=true→400, field$count, detail says a page carries no totaltest_extract_odata_query_misspelled_option_rejected?$filtre=…→400, described asunknown, notunsupportedtest_extract_odata_query_miscased_option_rejected?$Top=10→400, detail names the spelling that bindstest_extract_odata_query_reports_every_unsupported_option?$skip&$count&$expand→ three violations, in wire ordertest_extract_odata_query_repeated_unsupported_option_reported_once?$skip=20&$skip=40→ one violation, not twotest_extract_odata_query_non_odata_params_pass_through?q=needle&context=2&limit=5→200; the guard must not police the non-$namespaceThe last one is the regression guard that matters most: it pins that handlers sharing the query
string with the extractor (chat-engine's
q/context, file-storage'soffset) keep working.Documentation
ODataParams(wire-binding role),ODataParams.limitand.cursor(both spellings each, ambiguity rule),ACCEPTED_SYSTEM_QUERY_OPTIONSandODATA_SYSTEM_QUERY_OPTIONS(why a$key is refused asunsupported vs unknown), the guard itself (§6.1, and what a silent drop looks like to a
caller),
ODataLimits/max_top/validate_top(opt-in, not automatic), and theInvalidLimitarm inproblem_mapping.rsdocs/toolkit_unified_system/07_odata_pagination_select_filter.md: page size, continuationtoken, unsupported system query options, plus the error-table status correction (
422→400)described under Changes. The correction changes no behavior and no crate version — the file
lives in
docs/, outside both crates.Note for gear owners: OpenAPI cannot express one parameter under two names — publish the
spelling your gear treats as canonical and mention the other in its
description. A gear thatwants unprefixed accidents like
?status=approvedrefused rather than ignored needs its ownguard; the extractor only polices the
$namespace.Checklist
cargo clippy -p cf-gears-toolkit -p cf-gears-toolkit-odata -p cf-gears-account-management --all-targets→ clean)cargo fmt -p cf-gears-toolkit -p cf-gears-toolkit-odata -p cf-gears-account-management -- --check→ clean)Summary by CodeRabbit
Summary by CodeRabbit
New Features
$topalongsidelimit.$skiptokenalongsidecursor.Documentation