Conversation
not the long term fix
Reviewer's GuideThis PR implements Relay-style cursor pagination for SQLAlchemy relationships by extending connection and relationship resolvers to accept pagination arguments, introducing a PaginatedLoader to scope DataLoader instances per pagination configuration, adding cursor encoding/decoding utilities, and updating tests to verify forward and backward pagination behavior. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for adding the Here's a preview of the changelog: Implement Relay-style cursor pagination for SQLAlchemy relationships by extending the connection resolver and DataLoader to accept pagination arguments, computing pageInfo metadata, and introducing cursor utilities. Add PaginatedLoader to scope DataLoader instances per pagination parameters and update tests to verify pagination behavior. New Features:
Enhancements:
Tests:
Examples: Get the first three books for a specific author: query {
author(id: 1) {
id
name
books(first: 3) {
edges {
node {
id
title
}
}
pageInfo {
hasNextPage
hasPreviousPage
startCursor
endCursor
}
}
}
}Get all books after a specific book's cursor: query($afterBook: String) {
author(id: 1) {
id
name
books(after: $afterBook) {
edges {
node {
id
title
}
}
pageInfo {
hasNextPage
hasPreviousPage
startCursor
endCursor
}
}
}
}Get the first three books for a specific author after a specific book's cursor: query($afterBook: String) {
author(id: 1) {
id
name
books(first: 3, after: $afterBook) {
edges {
node {
id
title
}
}
pageInfo {
hasNextPage
hasPreviousPage
startCursor
endCursor
}
}
}
}Get the last three books for a specific author: query {
author(id: 1) {
id
name
books(last: 3) {
edges {
node {
id
title
}
}
pageInfo {
hasNextPage
hasPreviousPage
startCursor
endCursor
}
}
}
}Get all books before a specific book's cursor: query($beforeBook: String) {
author(id: 1) {
id
name
books(before: $beforeBook) {
edges {
node {
id
title
}
}
pageInfo {
hasNextPage
hasPreviousPage
startCursor
endCursor
}
}
}
}Get the last three books for a specific author before a specific book's cursor: query($beforeBook: String) {
author(id: 1) {
id
name
books(last: 3, before: $beforeBook) {
edges {
node {
id
title
}
}
pageInfo {
hasNextPage
hasPreviousPage
startCursor
endCursor
}
}
}
} |
There was a problem hiding this comment.
Hey @davidroeca - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/strawberry_sqlalchemy_mapper/mapper.py:568` </location>
<code_context>
+ edges.append(
+ edge_type.resolve_edge(
+ related_object,
+ cursor=max(0, decoded_before_index - total_count + i),
+ )
+ )
</code_context>
<issue_to_address>
Cursor calculation for before pagination may be incorrect.
Please review the logic for calculating the cursor in the 'before' branch, as it may produce incorrect values when total_count is less than decoded_before_index. Ensure the cursor calculation is accurate for all cases.
</issue_to_address>
### Comment 2
<location> `src/strawberry_sqlalchemy_mapper/mapper.py:576` </location>
<code_context>
+ edges.append(
+ edge_type.resolve_edge(
+ related_object,
+ cursor=max(0, total_count - last),
+ )
+ )
</code_context>
<issue_to_address>
Cursor value for 'last' pagination is constant for all edges.
Using the same cursor for all edges can break pagination. Each edge should have a unique cursor reflecting its position.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
+ Coverage 91.56% 92.31% +0.75%
==========================================
Files 17 19 +2
Lines 2038 2395 +357
Branches 159 188 +29
==========================================
+ Hits 1866 2211 +345
- Misses 92 98 +6
- Partials 80 86 +6 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #255 will not alter performanceComparing Summary
|
|
Hi, @davidroeca , thanks soo much for your contribution! This feature was in our radar since a while. |
|
@davidroeca Since its WIP I changed to a draft PR. |
|
@Ckk3 found the issue causing <= python3.9 to fail -- a misplaced setattr. Still unclear to me why 3.10+ worked. I think this PR is ready for review -- just a reminder of the one callout that |
There was a problem hiding this comment.
Hey @davidroeca - I've reviewed your changes - here's some feedback:
- The pagination logic in loader_for is quite dense—consider extracting the offset/limit calculation and argument validation into standalone helper functions to improve readability and testability.
- There’s duplicated code for resolving
info.context['sqlalchemy_loader']versusinfo.context.sqlalchemy_loader; you could introduce a small accessor/helper to DRY this lookup and reduce branching. - The
_resolve_connection_edgesmethod has grown large with pagination logic—extracting pageInfo and edge slicing into a dedicated paginator class or module would make both the mapper and loader code easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pagination logic in loader_for is quite dense—consider extracting the offset/limit calculation and argument validation into standalone helper functions to improve readability and testability.
- There’s duplicated code for resolving `info.context['sqlalchemy_loader']` versus `info.context.sqlalchemy_loader`; you could introduce a small accessor/helper to DRY this lookup and reduce branching.
- The `_resolve_connection_edges` method has grown large with pagination logic—extracting pageInfo and edge slicing into a dedicated paginator class or module would make both the mapper and loader code easier to follow.
## Individual Comments
### Comment 1
<location> `src/strawberry_sqlalchemy_mapper/loader.py:133` </location>
<code_context>
"""
- _loaders: Dict[RelationshipProperty, DataLoader]
+ _loaders: Dict[RelationshipProperty, PaginatedLoader] = {}
def __init__(
</code_context>
<issue_to_address>
Class-level _loaders dictionary may cause cross-instance data leakage.
Since all instances share the same _loaders cache, this can cause unintended data sharing. Recommend moving _loaders to an instance variable to avoid this issue.
</issue_to_address>
### Comment 2
<location> `tests/test_loader.py:198` </location>
<code_context>
+ Employee(name="e4"),
+ ],
+ )
+ d_empty = Department(name="empty")
+ session.add(d1)
+ session.add(d_empty)
+ session.flush()
+ session.commit()
</code_context>
<issue_to_address>
Consider adding assertions for empty result sets.
Please add assertions to verify that loading employees for d_empty returns an empty list, ensuring correct loader behavior for departments without employees.
</issue_to_address>
### Comment 3
<location> `RELEASE.md:119` </location>
<code_context>
+}
+```
+
+Get all books before a spcific book's cursor:
+
+```gql
</code_context>
<issue_to_address>
Typo: 'spcific' should be 'specific'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Get all books before a spcific book's cursor:
=======
Get all books before a specific book's cursor:
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `RELEASE.md:144` </location>
<code_context>
+}
+```
+
+Get the last three books for a specific author before a spcific book's cursor:
+
+```gql
</code_context>
<issue_to_address>
Typo: 'spcific' should be 'specific'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Get the last three books for a specific author before a spcific book's cursor:
=======
Get the last three books for a specific author before a specific book's cursor:
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }, | ||
| ) | ||
| ) | ||
| assert result.errors is None |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
| }, | ||
| ) | ||
| ) | ||
| assert result.errors is None |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
|
Oh I see, @davidroeca, Thanks for checking |
Ckk3
left a comment
There was a problem hiding this comment.
Thanks so much for your contribuition, @davidroeca
LGTM!
I will only run the pre-commit.

I'm currently looking into pagination arguments in this project. It's working here, but one thing to call out are the synchronous tests -- I wasn't able to get
schema.run_syncto run appropriately so I wrapped the synchronous tests inasyncio.runfor now. There might be a deeper bug to look into here. EDIT: I've tested this on the current main branch with comparable tests without pagination, and.execute_syncfails on the main branch too.Hoping for some initial thoughts before I go deeper. Let me know what you think!
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Implement Relay-style cursor pagination for SQLAlchemy relationships by extending the connection resolver and DataLoader to accept pagination arguments, computing pageInfo metadata, and introducing cursor utilities. Add PaginatedLoader to scope DataLoader instances per pagination parameters and update tests to verify pagination behavior.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Add Relay-style cursor pagination support to Strawberry SQLAlchemy mapper by extending resolvers and data loaders to accept and handle first, after, last, and before arguments
New Features:
Enhancements:
Documentation:
Tests: