Surface pagination-aware repository errors (backend-4.2.2)#356
Open
leynos wants to merge 6 commits into
Open
Conversation
Draft the pre-implementation plan for backend roadmap item 4.2.2. The plan captures the repository-layer pagination error gap, the hexagonal boundary constraints, validation strategy, and approval gate before any implementation work proceeds.
Contributor
Implementation of Pagination-Aware Repository Error SurfacingImplements backend roadmap item 4.2.2 to surface pagination-aware repository errors as HTTP-safe Core ChangesDomain Layer
Cursor Implementation
HTTP Error Mapping
Test CoverageAdds comprehensive rstest and rstest-bdd coverage:
Documentation
Validation
WalkthroughRefactors the pagination cursor implementation to classify direction-related failures separately from malformed tokens, then propagates these distinctions through domain error types, inbound HTTP parsing, and outbound persistence mapping to produce HTTP ChangesPagination-aware error handling
Possibly related PRs
Suggested labels
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 6 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements roadmap item 4.2.2 by introducing semantic, pagination-aware errors at the user repository port, wiring them through outbound persistence to HTTP-safe error envelopes, enhancing the shared pagination crate to classify unsupported cursor directions, and adding BDD/unit coverage and documentation describing the new invalid_cursor/unsupported_direction behaviour while keeping existing happy paths intact. Sequence diagram for users pagination error handlingsequenceDiagram
actor Client
participant UsersHttpAdapter as UsersHttpAdapter
participant Cursor as Cursor
participant UsersQueryAdapter as UsersQueryAdapter
participant ErrorMapping as ErrorMapping
rect rgb(230,230,250)
Client->>UsersHttpAdapter: GET /api/v1/users?cursor=bad
UsersHttpAdapter->>Cursor: Cursor::decode(cursor)
Cursor-->>UsersHttpAdapter: CursorError::InvalidBase64
UsersHttpAdapter->>UsersHttpAdapter: map_cursor_error(CursorError::InvalidBase64)
UsersHttpAdapter-->>Client: 400 Error { code=invalid_request, details.code=invalid_cursor }
end
rect rgb(230,250,230)
Client->>UsersHttpAdapter: GET /api/v1/users?cursor=unsupported-dir
UsersHttpAdapter->>Cursor: Cursor::decode(cursor)
Cursor-->>UsersHttpAdapter: CursorError::UnsupportedDirection
UsersHttpAdapter->>UsersHttpAdapter: map_cursor_error(CursorError::UnsupportedDirection)
UsersHttpAdapter-->>Client: 400 Error { code=invalid_request, details.code=unsupported_direction }
end
rect rgb(250,230,230)
Client->>UsersHttpAdapter: GET /api/v1/users?cursor=valid
UsersHttpAdapter->>UsersQueryAdapter: list_users_page(ListUsersPageRequest)
UsersQueryAdapter-->>UsersHttpAdapter: Err(UserPersistenceError::Pagination(UserPaginationError::InvalidCursorFormat))
UsersQueryAdapter->>ErrorMapping: map_user_persistence_error(UserPersistenceError::Pagination)
ErrorMapping-->>UsersQueryAdapter: Error { code=invalid_request, details.code=invalid_cursor }
UsersQueryAdapter-->>Client: 400 Error { code=invalid_request, details.code=invalid_cursor }
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Mark the ExecPlan as in progress after explicit implementation approval. Capture the clean branch baseline so later milestones can distinguish planned work from unrelated local state.
Add a user repository pagination error surface so invalid cursor and unsupported direction failures can travel through the port layer without being collapsed into generic persistence query failures. Classify unsupported cursor directions in the pagination crate, map both repository-originated and inbound cursor failures to HTTP 400 details, and cover the externally visible users-list behaviour with rstest-bdd.
Record the user-facing and internal pagination error contract. Mark backend roadmap item 4.2.2 done after the implementation and validation gates passed.
Update the execution plan with the final CodeRabbit result and closure status for backend roadmap item 4.2.2.
Update the execution plan with the draft PR handoff after pushing the completed backend 4.2.2 work.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
Cursor::decodeyou decode intoserde_json::Valueand then immediatelyfrom_valueintoCursor<Key>, which does a second deserialization step; consider instead deserializing into a small intermediate struct (with anOption<String>dir field) or implementing a customDeserializeto validatedirwithout double-parsing the JSON. - The construction of
invalid_cursor/unsupported_directionerror envelopes (message +details.field/details.code) is duplicated between the HTTP pagination adapter anduser_persistence_error_mapping; consider extracting small shared helpers to centralise this mapping and avoid drift if the contract changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Cursor::decode` you decode into `serde_json::Value` and then immediately `from_value` into `Cursor<Key>`, which does a second deserialization step; consider instead deserializing into a small intermediate struct (with an `Option<String>` dir field) or implementing a custom `Deserialize` to validate `dir` without double-parsing the JSON.
- The construction of `invalid_cursor` / `unsupported_direction` error envelopes (message + `details.field`/`details.code`) is duplicated between the HTTP pagination adapter and `user_persistence_error_mapping`; consider extracting small shared helpers to centralise this mapping and avoid drift if the contract changes.
## Individual Comments
### Comment 1
<location path="backend/crates/pagination/src/cursor/tests.rs" line_range="117-134" />
<code_context>
+}
+
+#[test]
+fn invalid_direction_value_returns_unsupported_direction_error() {
+ let invalid_cursor_json =
+ r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":"Sideways"}"#;
+ let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json);
+
+ let result = Cursor::<FixtureKey>::decode(&encoded);
+
+ assert!(matches!(
+ result,
+ Err(CursorError::UnsupportedDirection { direction }) if direction == "Sideways"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for unsupported direction values that are not strings
The helper has a dedicated branch for non-string `dir` values via `as_str().map_or_else(|| direction.to_string(), …)`, but the current test only covers the string case (`"Sideways"`). Please add a test where `"dir"` is non-string (e.g. `"dir": 123` or an object) and assert you still get `CursorError::UnsupportedDirection` with the expected `direction` string, so the fallback path and malformed JSON cursors are covered.
```suggestion
#[test]
fn invalid_direction_value_returns_unsupported_direction_error() {
let invalid_cursor_json =
r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":"Sideways"}"#;
let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json);
let result = Cursor::<FixtureKey>::decode(&encoded);
assert!(matches!(
result,
Err(CursorError::UnsupportedDirection { direction }) if direction == "Sideways"
));
}
#[test]
fn non_string_direction_value_returns_unsupported_direction_error() {
let invalid_cursor_json =
r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":123}"#;
let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json);
let result = Cursor::<FixtureKey>::decode(&encoded);
assert!(matches!(
result,
Err(CursorError::UnsupportedDirection { direction }) if direction == "123"
));
}
#[rstest]
#[case(Direction::Next)]
#[case(Direction::Prev)]
fn into_parts_returns_key_and_direction(#[case] direction: Direction) {
```
</issue_to_address>
### Comment 2
<location path="backend/crates/pagination/src/cursor/mod.rs" line_range="214" />
<code_context>
- /// Returns [`CursorError::InvalidBase64`] when `value` is not valid
- /// base64url and [`CursorError::Deserialize`] when the decoded JSON does
- /// not match the expected cursor shape.
- pub fn decode(value: &str) -> Result<Self, CursorError> {
- let payload = URL_SAFE_NO_PAD
- .decode(value)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `Cursor::decode` to use a dedicated wire struct and a base64 helper to streamline decoding logic and remove the JSON `Value` round-trip.
You can simplify `Cursor::decode` without changing behavior by:
1. **Avoiding the `serde_json::Value` round‑trip**
Instead of `Value` + `reject_unsupported_direction` + second `from_value`, deserialize into a small internal wire type and handle `dir` as a `String`/`Option<String>`.
```rust
#[derive(Deserialize)]
struct CursorWire<Key> {
key: Key,
#[serde(default)]
dir: Option<String>,
}
impl<Key> Cursor<Key>
where
Key: DeserializeOwned,
{
pub fn decode(value: &str) -> Result<Self, CursorError> {
let payload = decode_base64_url(value)?;
let wire: CursorWire<Key> =
serde_json::from_slice(&payload).map_err(|error| CursorError::Deserialize {
message: error.to_string(),
})?;
let dir = match wire.dir.as_deref() {
None | Some("Next") => Direction::Next,
Some("Prev") => Direction::Prev,
Some(other) => {
return Err(CursorError::UnsupportedDirection {
direction: other.to_owned(),
})
}
};
Ok(Cursor { key: wire.key, dir })
}
}
```
This:
- Keeps the `UnsupportedDirection { direction }` behavior intact.
- Removes the extra allocation and double‑decode.
- Makes the decode path linear and easier to read.
You can then delete `reject_unsupported_direction` entirely.
2. **Extracting base64 decoding into a helper**
The dual engine handling is reasonable, but inlining it makes `decode` dense. A small helper keeps behavior while improving readability:
```rust
fn decode_base64_url(value: &str) -> Result<Vec<u8>, CursorError> {
URL_SAFE_NO_PAD
.decode(value)
.or_else(|_| URL_SAFE.decode(value))
.map_err(|error| CursorError::InvalidBase64 {
message: error.to_string(),
})
}
```
Then `Cursor::decode` just does:
```rust
let payload = decode_base64_url(value)?;
```
</issue_to_address>
### Comment 3
<location path="docs/execplans/backend-4-2-2-surface-pagination-aware-errors.md" line_range="14" />
<code_context>
+
+Roadmap task 4.2.2 requires the backend to surface pagination-aware repository
+errors, such as malformed cursor tokens and unsupported pagination direction,
+as HTTP `400 Bad Request` responses instead of treating them as generic
+persistence failures. After this work, a caller of `GET /api/v1/users` can pass
+an invalid pagination cursor and receive the existing Wildside error envelope
</code_context>
<issue_to_address>
**issue (review_instructions):** HTTP is introduced without being expanded on first use in this new document.
Since this is a new ExecPlan file, Hypertext Transfer Protocol (HTTP) should be expanded on its first occurrence, for example: "as Hypertext Transfer Protocol (HTTP) `400 Bad Request` responses". Subsequent uses can then use the acronym alone.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 4
<location path="docs/execplans/backend-4-2-2-surface-pagination-aware-errors.md" line_range="645" />
<code_context>
+ HTTP `400` client errors that identify the problematic parameter.
+- RFC 9457:
+ `https://www.rfc-editor.org/rfc/rfc9457.html`. Relevant point:
+ machine-readable error details are useful for HTTP APIs, but Wildside already
+ has a stable JSON error envelope, so this work should enrich `details`
+ instead of changing media type or top-level shape.
</code_context>
<issue_to_address>
**issue (review_instructions):** API is used without being expanded on first use in this file.
Please expand API on its first appearance in this ExecPlan, for example "application programming interface (API)", then use "API" thereafter. That keeps the document self-contained for readers unfamiliar with the acronym.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 5
<location path="docs/users-guide.md" line_range="3" />
<code_context>
+# Wildside server users guide
+
+This guide records user-visible server behaviour for Wildside API consumers.
+It focuses on contracts that clients can rely on when calling the backend.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** API is not expanded on first use in this new users guide.
As this is a new standalone guide, please expand API on first use, for example: "Wildside application programming interface (API) consumers".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 6
<location path="docs/users-guide.md" line_range="33" />
<code_context>
+}
+```
+
+Pagination input errors use the standard Wildside error envelope and return
+HTTP `400 Bad Request`:
+
</code_context>
<issue_to_address>
**issue (review_instructions):** HTTP is used on the following line without being expanded on first use in this file.
On the next line, HTTP appears for the first time in this document. Please expand it as "Hypertext Transfer Protocol (HTTP)" at that first occurrence before using the acronym alone.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+117
to
+134
| #[test] | ||
| fn invalid_direction_value_returns_unsupported_direction_error() { | ||
| let invalid_cursor_json = | ||
| r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":"Sideways"}"#; | ||
| let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json); | ||
|
|
||
| let result = Cursor::<FixtureKey>::decode(&encoded); | ||
|
|
||
| assert!(matches!( | ||
| result, | ||
| Err(CursorError::UnsupportedDirection { direction }) if direction == "Sideways" | ||
| )); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case(Direction::Next)] | ||
| #[case(Direction::Prev)] | ||
| fn into_parts_returns_key_and_direction(#[case] direction: Direction) { |
There was a problem hiding this comment.
suggestion (testing): Add a test for unsupported direction values that are not strings
The helper has a dedicated branch for non-string dir values via as_str().map_or_else(|| direction.to_string(), …), but the current test only covers the string case ("Sideways"). Please add a test where "dir" is non-string (e.g. "dir": 123 or an object) and assert you still get CursorError::UnsupportedDirection with the expected direction string, so the fallback path and malformed JSON cursors are covered.
Suggested change
| #[test] | |
| fn invalid_direction_value_returns_unsupported_direction_error() { | |
| let invalid_cursor_json = | |
| r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":"Sideways"}"#; | |
| let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json); | |
| let result = Cursor::<FixtureKey>::decode(&encoded); | |
| assert!(matches!( | |
| result, | |
| Err(CursorError::UnsupportedDirection { direction }) if direction == "Sideways" | |
| )); | |
| } | |
| #[rstest] | |
| #[case(Direction::Next)] | |
| #[case(Direction::Prev)] | |
| fn into_parts_returns_key_and_direction(#[case] direction: Direction) { | |
| #[test] | |
| fn invalid_direction_value_returns_unsupported_direction_error() { | |
| let invalid_cursor_json = | |
| r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":"Sideways"}"#; | |
| let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json); | |
| let result = Cursor::<FixtureKey>::decode(&encoded); | |
| assert!(matches!( | |
| result, | |
| Err(CursorError::UnsupportedDirection { direction }) if direction == "Sideways" | |
| )); | |
| } | |
| #[test] | |
| fn non_string_direction_value_returns_unsupported_direction_error() { | |
| let invalid_cursor_json = | |
| r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":123}"#; | |
| let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json); | |
| let result = Cursor::<FixtureKey>::decode(&encoded); | |
| assert!(matches!( | |
| result, | |
| Err(CursorError::UnsupportedDirection { direction }) if direction == "123" | |
| )); | |
| } | |
| #[rstest] | |
| #[case(Direction::Next)] | |
| #[case(Direction::Prev)] | |
| fn into_parts_returns_key_and_direction(#[case] direction: Direction) { |
| /// base64url, [`CursorError::UnsupportedDirection`] when `dir` is present | ||
| /// but not one of the supported directions, and [`CursorError::Deserialize`] | ||
| /// when the decoded JSON does not match the expected cursor shape. | ||
| pub fn decode(value: &str) -> Result<Self, CursorError> { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring Cursor::decode to use a dedicated wire struct and a base64 helper to streamline decoding logic and remove the JSON Value round-trip.
You can simplify Cursor::decode without changing behavior by:
- Avoiding the
serde_json::Valueround‑trip
Instead of Value + reject_unsupported_direction + second from_value, deserialize into a small internal wire type and handle dir as a String/Option<String>.
#[derive(Deserialize)]
struct CursorWire<Key> {
key: Key,
#[serde(default)]
dir: Option<String>,
}
impl<Key> Cursor<Key>
where
Key: DeserializeOwned,
{
pub fn decode(value: &str) -> Result<Self, CursorError> {
let payload = decode_base64_url(value)?;
let wire: CursorWire<Key> =
serde_json::from_slice(&payload).map_err(|error| CursorError::Deserialize {
message: error.to_string(),
})?;
let dir = match wire.dir.as_deref() {
None | Some("Next") => Direction::Next,
Some("Prev") => Direction::Prev,
Some(other) => {
return Err(CursorError::UnsupportedDirection {
direction: other.to_owned(),
})
}
};
Ok(Cursor { key: wire.key, dir })
}
}This:
- Keeps the
UnsupportedDirection { direction }behavior intact. - Removes the extra allocation and double‑decode.
- Makes the decode path linear and easier to read.
You can then delete reject_unsupported_direction entirely.
- Extracting base64 decoding into a helper
The dual engine handling is reasonable, but inlining it makes decode dense. A small helper keeps behavior while improving readability:
fn decode_base64_url(value: &str) -> Result<Vec<u8>, CursorError> {
URL_SAFE_NO_PAD
.decode(value)
.or_else(|_| URL_SAFE.decode(value))
.map_err(|error| CursorError::InvalidBase64 {
message: error.to_string(),
})
}Then Cursor::decode just does:
let payload = decode_base64_url(value)?;|
|
||
| Roadmap task 4.2.2 requires the backend to surface pagination-aware repository | ||
| errors, such as malformed cursor tokens and unsupported pagination direction, | ||
| as HTTP `400 Bad Request` responses instead of treating them as generic |
There was a problem hiding this comment.
issue (review_instructions): HTTP is introduced without being expanded on first use in this new document.
Since this is a new ExecPlan file, Hypertext Transfer Protocol (HTTP) should be expanded on its first occurrence, for example: "as Hypertext Transfer Protocol (HTTP) 400 Bad Request responses". Subsequent uses can then use the acronym alone.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| HTTP `400` client errors that identify the problematic parameter. | ||
| - RFC 9457: | ||
| `https://www.rfc-editor.org/rfc/rfc9457.html`. Relevant point: | ||
| machine-readable error details are useful for HTTP APIs, but Wildside already |
There was a problem hiding this comment.
issue (review_instructions): API is used without being expanded on first use in this file.
Please expand API on its first appearance in this ExecPlan, for example "application programming interface (API)", then use "API" thereafter. That keeps the document self-contained for readers unfamiliar with the acronym.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| @@ -0,0 +1,44 @@ | |||
| # Wildside server users guide | |||
|
|
|||
| This guide records user-visible server behaviour for Wildside API consumers. | |||
There was a problem hiding this comment.
issue (review_instructions): API is not expanded on first use in this new users guide.
As this is a new standalone guide, please expand API on first use, for example: "Wildside application programming interface (API) consumers".
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| } | ||
| ``` | ||
|
|
||
| Pagination input errors use the standard Wildside error envelope and return |
There was a problem hiding this comment.
issue (review_instructions): HTTP is used on the following line without being expanded on first use in this file.
On the next line, HTTP appears for the first time in this document. Please expand it as "Hypertext Transfer Protocol (HTTP)" at that first occurrence before using the acronym alone.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@backend/crates/pagination/src/cursor/tests.rs`:
- Around line 20-175: Add a proptest-based property test that asserts round-trip
invariants for Cursor: generate arbitrary FixtureKey values (strings for
created_at and id) and both Direction variants, build a Cursor with
Cursor::with_direction(key, direction), encode it with Cursor::encode(), decode
it with Cursor::<FixtureKey>::decode(&encoded) and assert equality (decoded ==
original cursor). Put the test alongside the existing tests (tests.rs), import
proptest macros/strategies (proptest::proptest, proptest::prop_oneof or
prop::strategy::Strategy), and ensure the test covers both directions and
handles encode/decode Result by unwrapping or mapping errors into test failures
so failures surface as test errors.
In `@docs/users-guide.md`:
- Line 38: Update the table row string "Cursor text is not a valid users cursor"
to correct grammar by changing "users" to "user" so it reads "Cursor text is not
a valid user cursor"; keep the rest of the row (the code literals `cursor is
invalid`, `cursor`, `invalid_cursor`) unchanged to preserve identifiers.
- Line 8: The sentence describing the endpoint `GET /api/v1/users` uses the
ambiguous phrase "paginated users response"; change that fragment to a
grammatically correct form such as "paginated user-list response" (or "paginated
users-list response") to fix article–noun agreement and improve clarity in the
docs.
- Around line 14-15: Update the documentation line describing `cursor` to fix
article–noun agreement: replace "a previous users list response" with "a
previous user-list response" so the `cursor` description reads "...an opaque
base64url cursor returned by a previous user-list response." Reference the
`cursor` descriptor in the docs and ensure the hyphenated compound noun
"user-list" is used.
In `@docs/wildside-backend-architecture.md`:
- Line 2024: The Markdown paragraph containing "from generic deserialization
failures with `CursorError::UnsupportedDirection`." exceeds 80 columns; rewrap
that paragraph so the line breaks before the backtick-quoted identifier (e.g.,
split into "from generic deserialization failures with" on one line and
"`CursorError::UnsupportedDirection`." on the next) to comply with the 80-column
limit while preserving the exact identifier.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dcd1394c-adec-4782-9b84-853d1a6f36ed
📒 Files selected for processing (18)
backend/crates/pagination/src/cursor.rsbackend/crates/pagination/src/cursor/mod.rsbackend/crates/pagination/src/cursor/tests.rsbackend/crates/pagination/src/lib.rsbackend/src/domain/ports/mod.rsbackend/src/domain/ports/user_repository.rsbackend/src/inbound/http/users_pagination.rsbackend/src/outbound/persistence/diesel_users_query.rsbackend/src/outbound/persistence/user_persistence_error_mapping.rsbackend/tests/features/users_list_pagination.featurebackend/tests/users_list_pagination_bdd.rsdocs/backend-roadmap.mddocs/contents.mddocs/developers-guide.mddocs/execplans/backend-4-2-2-surface-pagination-aware-errors.mddocs/keyset-pagination-design.mddocs/users-guide.mddocs/wildside-backend-architecture.md
💤 Files with no reviewable changes (1)
- backend/crates/pagination/src/cursor.rs
Comment on lines
+20
to
+175
| #[test] | ||
| fn cursor_round_trips_through_opaque_token() { | ||
| let cursor = Cursor::new(FixtureKey { | ||
| created_at: "2026-03-22T10:30:00Z".to_owned(), | ||
| id: "8b116c56-0a58-4c55-b7d7-06ee6bbddb8c".to_owned(), | ||
| }); | ||
|
|
||
| let encoded = cursor.encode().expect("cursor encoding should succeed"); | ||
| let decoded = Cursor::<FixtureKey>::decode(&encoded).expect("cursor decoding should succeed"); | ||
|
|
||
| assert_eq!(decoded, cursor); | ||
| } | ||
|
|
||
| #[test] | ||
| fn invalid_base64_cursor_fails_decode() { | ||
| let result = Cursor::<FixtureKey>::decode("!!!"); | ||
|
|
||
| assert!(matches!(result, Err(CursorError::InvalidBase64 { .. }))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn padded_base64_cursor_decodes_successfully() { | ||
| let cursor = Cursor::new(FixtureKey { | ||
| created_at: "2026-03-22T10:30:00Z".to_owned(), | ||
| id: "8b116c56-0a58-4c55-b7d7-06ee6bbddb8c".to_owned(), | ||
| }); | ||
| let payload = serde_json::to_vec(&cursor).expect("cursor should serialize"); | ||
| let encoded = base64::engine::general_purpose::URL_SAFE.encode(payload); | ||
|
|
||
| let decoded = | ||
| Cursor::<FixtureKey>::decode(&encoded).expect("padded cursor decoding should succeed"); | ||
|
|
||
| assert_eq!(decoded, cursor); | ||
| } | ||
|
|
||
| #[test] | ||
| fn structurally_invalid_json_cursor_fails_decode() { | ||
| let invalid_payload = | ||
| base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(br#"{"unexpected":true}"#); | ||
|
|
||
| let result = Cursor::<FixtureKey>::decode(&invalid_payload); | ||
|
|
||
| assert!(matches!(result, Err(CursorError::Deserialize { .. }))); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case(Direction::Next)] | ||
| #[case(Direction::Prev)] | ||
| fn direction_round_trips_through_encoding(#[case] direction: Direction) { | ||
| let cursor = Cursor::with_direction( | ||
| FixtureKey { | ||
| created_at: "2026-03-22T10:30:00Z".to_owned(), | ||
| id: "test-id".to_owned(), | ||
| }, | ||
| direction, | ||
| ); | ||
| let encoded = cursor.encode().expect("encoding succeeds"); | ||
| let decoded = Cursor::<FixtureKey>::decode(&encoded).expect("decoding succeeds"); | ||
|
|
||
| assert_eq!(decoded.direction(), direction); | ||
| assert_eq!(decoded.key(), cursor.key()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cursor_without_direction_defaults_to_next() { | ||
| let old_cursor_json = r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"}}"#; | ||
| let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(old_cursor_json); | ||
|
|
||
| let decoded = Cursor::<FixtureKey>::decode(&encoded).expect("decoding succeeds"); | ||
|
|
||
| assert_eq!(decoded.direction(), Direction::Next); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case(Direction::Next, "Next")] | ||
| #[case(Direction::Prev, "Prev")] | ||
| fn new_cursor_includes_direction_in_json(#[case] direction: Direction, #[case] expected: &str) { | ||
| let cursor = Cursor::with_direction( | ||
| FixtureKey { | ||
| created_at: "2026-03-22T10:30:00Z".to_owned(), | ||
| id: "test-id".to_owned(), | ||
| }, | ||
| direction, | ||
| ); | ||
| let encoded = cursor.encode().expect("encoding succeeds"); | ||
| let decoded_bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD | ||
| .decode(&encoded) | ||
| .expect("base64 decoding succeeds"); | ||
| let json_value: serde_json::Value = serde_json::from_slice(&decoded_bytes).expect("valid JSON"); | ||
|
|
||
| let dir_value = json_value | ||
| .get("dir") | ||
| .and_then(|v| v.as_str()) | ||
| .expect("dir field should exist and be a string"); | ||
| assert_eq!(dir_value, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn invalid_direction_value_returns_unsupported_direction_error() { | ||
| let invalid_cursor_json = | ||
| r#"{"key":{"created_at":"2026-03-22T10:30:00Z","id":"test-id"},"dir":"Sideways"}"#; | ||
| let encoded = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(invalid_cursor_json); | ||
|
|
||
| let result = Cursor::<FixtureKey>::decode(&encoded); | ||
|
|
||
| assert!(matches!( | ||
| result, | ||
| Err(CursorError::UnsupportedDirection { direction }) if direction == "Sideways" | ||
| )); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case(Direction::Next)] | ||
| #[case(Direction::Prev)] | ||
| fn into_parts_returns_key_and_direction(#[case] direction: Direction) { | ||
| let key = FixtureKey { | ||
| created_at: "2026-03-22T10:30:00Z".to_owned(), | ||
| id: "test-id".to_owned(), | ||
| }; | ||
| let cursor = Cursor::with_direction(key.clone(), direction); | ||
|
|
||
| let (returned_key, returned_dir) = cursor.into_parts(); | ||
|
|
||
| assert_eq!(returned_key, key); | ||
| assert_eq!(returned_dir, direction); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cursor_new_uses_next_direction() { | ||
| let cursor = Cursor::new(FixtureKey { | ||
| created_at: "2026-03-22T10:30:00Z".to_owned(), | ||
| id: "test-id".to_owned(), | ||
| }); | ||
|
|
||
| assert_eq!(cursor.direction(), Direction::Next); | ||
| } | ||
|
|
||
| #[test] | ||
| fn encode_returns_serialize_error_when_key_cannot_be_serialized() { | ||
| use std::collections::HashMap; | ||
| #[derive(Hash, PartialEq, Eq)] | ||
| struct FailingKey; | ||
| impl Serialize for FailingKey { | ||
| fn serialize<S: serde::Serializer>(&self, _: S) -> Result<S::Ok, S::Error> { | ||
| Err(serde::ser::Error::custom("fail")) | ||
| } | ||
| } | ||
| let cursor = Cursor { | ||
| key: HashMap::from([(FailingKey, String::new())]), | ||
| dir: Direction::Next, | ||
| }; | ||
| let Err(CursorError::Serialize { message }) = cursor.encode() else { | ||
| panic!("expected Serialize error") | ||
| }; | ||
| assert!(message.contains("fail")); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Add property-based invariants for cursor round-tripping.
Add a proptest suite that asserts decode(encode(cursor)) == cursor across generated
keys and both directions. Lock the opaque-token contract against edge cases that
example-based tests miss.
♻️ Suggested test shape
+use proptest::prelude::*;
+
+proptest! {
+ #[test]
+ fn cursor_roundtrip_holds_for_generated_inputs(
+ created_at in "[0-9TZ:\\-]{1,40}",
+ id in "[a-zA-Z0-9\\-]{1,64}",
+ dir in prop_oneof![Just(Direction::Next), Just(Direction::Prev)],
+ ) {
+ let key = FixtureKey { created_at, id };
+ let cursor = Cursor::with_direction(key, dir);
+ let encoded = cursor.encode().expect("encode succeeds");
+ let decoded = Cursor::<FixtureKey>::decode(&encoded).expect("decode succeeds");
+ prop_assert_eq!(decoded, cursor);
+ }
+}As per coding guidelines: "**/*.{rs,py,ts,tsx}: Property tests ... should be recommended when the change introduces an invariant over a range of inputs, states, orderings, or transitions."
🤖 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 `@backend/crates/pagination/src/cursor/tests.rs` around lines 20 - 175, Add a
proptest-based property test that asserts round-trip invariants for Cursor:
generate arbitrary FixtureKey values (strings for created_at and id) and both
Direction variants, build a Cursor with Cursor::with_direction(key, direction),
encode it with Cursor::encode(), decode it with
Cursor::<FixtureKey>::decode(&encoded) and assert equality (decoded == original
cursor). Put the test alongside the existing tests (tests.rs), import proptest
macros/strategies (proptest::proptest, proptest::prop_oneof or
prop::strategy::Strategy), and ensure the test covers both directions and
handles encode/decode Result by unwrapping or mapping errors into test failures
so failures surface as test errors.
|
|
||
| ## Users list pagination | ||
|
|
||
| `GET /api/v1/users` returns a paginated users response. Clients should follow |
Contributor
There was a problem hiding this comment.
Fix grammar: use singular article-noun agreement.
"paginated users response" is grammatically ambiguous. Change to "paginated user-list response" or "paginated users-list response" for clarity.
[type:grammar]
Proposed fix
-`GET /api/v1/users` returns a paginated users response. Clients should follow
+`GET /api/v1/users` returns a paginated user-list response. Clients should follow📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `GET /api/v1/users` returns a paginated users response. Clients should follow | |
| `GET /api/v1/users` returns a paginated user-list response. Clients should follow |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...GET /api/v1/users returns a paginated users response. Clients should follow the `li...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🤖 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 `@docs/users-guide.md` at line 8, The sentence describing the endpoint `GET
/api/v1/users` uses the ambiguous phrase "paginated users response"; change that
fragment to a grammatically correct form such as "paginated user-list response"
(or "paginated users-list response") to fix article–noun agreement and improve
clarity in the docs.
Comment on lines
+14
to
+15
| - `cursor`: an opaque base64url cursor returned by a previous users list | ||
| response. |
Contributor
There was a problem hiding this comment.
Fix grammar: resolve article-noun agreement.
"a previous users list response" contains a grammatical disagreement between the article "a" and the plural noun "users". Change to "a previous user-list response" for clarity.
[type:grammar]
Proposed fix
-- `cursor`: an opaque base64url cursor returned by a previous users list
- response.
+- `cursor`: an opaque base64url cursor returned by a previous user-list
+ response.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ...lves. The endpoint accepts: - cursor: an opaque base64url cursor returned by ...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~14-~14: The plural noun “users” cannot be used with the article “a”. Did you mean “a previous user” or “previous users”?
Context: ... an opaque base64url cursor returned by a previous users list response. - limit: page size. ...
(A_NNS)
🤖 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 `@docs/users-guide.md` around lines 14 - 15, Update the documentation line
describing `cursor` to fix article–noun agreement: replace "a previous users
list response" with "a previous user-list response" so the `cursor` description
reads "...an opaque base64url cursor returned by a previous user-list response."
Reference the `cursor` descriptor in the docs and ensure the hyphenated compound
noun "user-list" is used.
|
|
||
| | Condition | Message | `details.field` | `details.code` | | ||
| |--------------------------------------------|-------------------------------------|-----------------|-------------------------| | ||
| | Cursor text is not a valid users cursor | `cursor is invalid` | `cursor` | `invalid_cursor` | |
Contributor
There was a problem hiding this comment.
Fix grammar: use singular article-noun agreement.
"a valid users cursor" contains a grammatical disagreement. Change "users" to "user" to match the singular article "a".
[type:grammar]
Proposed fix
-| Cursor text is not a valid users cursor | `cursor is invalid` | `cursor` | `invalid_cursor` |
+| Cursor text is not a valid user cursor | `cursor is invalid` | `cursor` | `invalid_cursor` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Cursor text is not a valid users cursor | `cursor is invalid` | `cursor` | `invalid_cursor` | | |
| | Cursor text is not a valid user cursor | `cursor is invalid` | `cursor` | `invalid_cursor` | |
🧰 Tools
🪛 LanguageTool
[grammar] ~38-~38: The plural noun “users” cannot be used with the article “a”. Did you mean “a valid user”?
Context: ...-----------------| | Cursor text is not a valid users cursor | cursor is invalid ...
(A_NNS)
🤖 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 `@docs/users-guide.md` at line 38, Update the table row string "Cursor text is
not a valid users cursor" to correct grammar by changing "users" to "user" so it
reads "Cursor text is not a valid user cursor"; keep the rest of the row (the
code literals `cursor is invalid`, `cursor`, `invalid_cursor`) unchanged to
preserve identifiers.
|
|
||
| Roadmap 4.2.2 adds the pagination-aware error contract for endpoint adoption. | ||
| The pagination crate now distinguishes unsupported decoded cursor directions | ||
| from generic deserialization failures with `CursorError::UnsupportedDirection`. |
Contributor
There was a problem hiding this comment.
Rewrap line 2024 to comply with the 80-column limit.
Line 2024 is 83 characters, exceeding the required 80-column wrapping for Markdown paragraphs. Break the line before the backtick-quoted identifier.
Triage: [type:syntax/md]
Proposed fix
-from generic deserialization failures with `CursorError::UnsupportedDirection`.
+from generic deserialization failures with
+`CursorError::UnsupportedDirection`.As per coding guidelines: Markdown paragraphs and bullet points must be wrapped at 80 columns.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from generic deserialization failures with `CursorError::UnsupportedDirection`. | |
| from generic deserialization failures with | |
| `CursorError::UnsupportedDirection`. |
🤖 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 `@docs/wildside-backend-architecture.md` at line 2024, The Markdown paragraph
containing "from generic deserialization failures with
`CursorError::UnsupportedDirection`." exceeds 80 columns; rewrap that paragraph
so the line breaks before the backtick-quoted identifier (e.g., split into "from
generic deserialization failures with" on one line and
"`CursorError::UnsupportedDirection`." on the next) to comply with the 80-column
limit while preserving the exact identifier.
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
1 participant
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.
Summary
400 Bad Requestwith stabledetails.codevalues ofinvalid_cursorandunsupported_direction.rstestunit coverage andrstest-bddbehavioural coverage for the users list pagination unhappy path while preserving existing happy paths.Review Walkthrough
docs/execplans/backend-4-2-2-surface-pagination-aware-errors.mdfor the approved plan, decisions, progress, and validation record.backend/src/domain/ports/user_repository.rsandbackend/src/outbound/persistence/user_persistence_error_mapping.rsfor the port-level error vocabulary and HTTP mapping.backend/crates/pagination/src/cursor/mod.rsandbackend/crates/pagination/src/cursor/tests.rsfor cursor decode classification.backend/tests/features/users_list_pagination.featureandbackend/tests/users_list_pagination_bdd.rsfor externally observable behaviour.Validation
make check-fmtmake lintmake testmake fmtmake markdownlintcoderabbit review --agentimplementation milestone: 0 findingscoderabbit review --agentdocumentation and roadmap closure: 0 findingsReferences
Summary by Sourcery
Surface pagination-aware errors for users list pagination and document the new error contract.
New Features:
Enhancements:
Documentation:
Tests: