Skip to content

Pagination args#255

Merged
Ckk3 merged 22 commits intostrawberry-graphql:mainfrom
davidroeca:pagination-args
Aug 7, 2025
Merged

Pagination args#255
Ckk3 merged 22 commits intostrawberry-graphql:mainfrom
davidroeca:pagination-args

Conversation

@davidroeca
Copy link
Copy Markdown
Contributor

@davidroeca davidroeca commented Jul 19, 2025

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_sync to run appropriately so I wrapped the synchronous tests in asyncio.run for 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_sync fails on the main branch too.

Hoping for some initial thoughts before I go deeper. Let me know what you think!

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

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:

  • Support cursor-based pagination (first, after, last, before) on GraphQL relationship fields
  • Introduce PaginatedLoader to manage DataLoader instances per pagination configuration

Enhancements:

  • Extend connection resolvers to compute pageInfo fields (hasNextPage, hasPreviousPage, totalCount) and handle forward and backward pagination
  • Add utilities for cursor encoding/decoding and relationship key extraction

Tests:

  • Add comprehensive tests for forward and backward pagination scenarios in both synchronous and asynchronous execution contexts

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:

  • Support cursor-based pagination (first, after, last, before) on GraphQL connection fields for SQLAlchemy relationships
  • Introduce PaginatedLoader to scope DataLoader instances per pagination configuration

Enhancements:

  • Extend connection and relationship resolvers to propagate pagination parameters and compute pageInfo (hasNextPage, hasPreviousPage, totalCount)
  • Implement offset and limit logic in loader implementation, including validation of invalid argument combinations
  • Add utilities for encoding and decoding array connection cursors

Documentation:

  • Add RELEASE.md with release notes and GraphQL pagination examples

Tests:

  • Add unit tests for valid and invalid pagination scenarios in DataLoader
  • Update schema generation tests to include pagination arguments on connection fields
  • Add integration tests for forward and backward pagination in synchronous and asynchronous execution contexts

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai bot commented Jul 19, 2025

Reviewer's Guide

This 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

Change Details Files
Add cursor encoding/decoding utilities for Relay-style pagination.
  • Introduce decode_cursor_index and encode_cursor_index functions
src/strawberry_sqlalchemy_mapper/pagination_cursor_utils.py
Extend connection resolvers to handle pagination arguments and compute pageInfo.
  • Update make_connection_wrapper_resolver to accept first/after/last/before and compute hasNextPage, hasPreviousPage, totalCount
  • Extend _resolve_connection_edges signature to process pagination args, calculate start_index, build edges, and populate pageInfo
src/strawberry_sqlalchemy_mapper/mapper.py
Modify relationship resolvers to pass pagination parameters to data loader.
  • Wrap list relationships in resolve_list accepting pagination args and invoking loader.load with those args
  • Adjust connection_resolver_for to pass RelationshipProperty instead of type_name
src/strawberry_sqlalchemy_mapper/mapper.py
Introduce PaginatedLoader and update loader_for to support pagination-scoped DataLoaders.
  • Add PaginatedLoader class managing DataLoader instances keyed by pagination args
  • Change StrawberrySQLAlchemyLoader.loader_for to return PaginatedLoader and implement load_fn with argument validation, offset/limit calculation, and record counting
src/strawberry_sqlalchemy_mapper/loader.py
Revise and expand test suite to cover pagination scenarios.
  • Update existing loader tests to use PaginatedLoader.loader_for()
  • Adjust mapper inheritance and association proxy tests to include pagination args in field definitions
  • Add tests for forward/backward pagination and invalid argument combinations in a new test_relationship_pagination.py
tests/test_loader.py
tests/test_mapper_inheritance.py
tests/test_association_proxy.py
tests/test_mapper.py
tests/test_relationship_pagination.py
Add release notes documenting pagination feature and usage examples.
  • Create RELEASE.md summarizing new pagination features, enhancements, and example queries
RELEASE.md

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@botberry
Copy link
Copy Markdown
Member

botberry commented Jul 19, 2025

Thanks for adding the RELEASE.md file!

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:

  • Support cursor-based pagination (first, after, last, before) on GraphQL relationship fields
  • Introduce PaginatedLoader to manage DataLoader instances per pagination configuration

Enhancements:

  • Extend connection resolvers to compute pageInfo fields (hasNextPage, hasPreviousPage, totalCount) and handle forward and backward pagination
  • Add utilities for cursor encoding/decoding and relationship key extraction

Tests:

  • Add comprehensive tests for forward and backward pagination scenarios in both synchronous and asynchronous execution contexts

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
      }
    }
  }
}

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 19, 2025

Codecov Report

❌ Patch coverage is 94.87179% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.31%. Comparing base (1a2db70) to head (a813be6).
⚠️ Report is 1 commits behind head on main.

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Jul 19, 2025

CodSpeed Performance Report

Merging #255 will not alter performance

Comparing davidroeca:pagination-args (a813be6) with main (1a2db70)

Summary

✅ 1 untouched benchmarks

@Ckk3
Copy link
Copy Markdown
Contributor

Ckk3 commented Jul 21, 2025

Hi, @davidroeca , thanks soo much for your contribution! This feature was in our radar since a while.
I'll need some time to review it but I will try to find some time to see it ASAP!

@Ckk3 Ckk3 marked this pull request as draft July 21, 2025 19:44
@Ckk3
Copy link
Copy Markdown
Contributor

Ckk3 commented Jul 21, 2025

@davidroeca Since its WIP I changed to a draft PR.
I see that some tests are failling, please focus on find a solution to python 3.9, dont need to make it work on python 3.8 because we have plans to remove the support to it soon (since it already reach the EOL)

@davidroeca
Copy link
Copy Markdown
Contributor Author

@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 schema.execute_sync seems to fail for more complex use cases, and that seems to be a problem on the main branch as well. Calling asyncio.run in the tests is a bit of a workaround. Happy to open a separate issue to track this, but I'm thinking it could potentially be related to #244

@davidroeca davidroeca marked this pull request as ready for review July 21, 2025 22:24
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'] 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): Extract duplicate code into function (extract-duplicate-method)

},
)
)
assert result.errors is None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): Extract duplicate code into function (extract-duplicate-method)

@davidroeca davidroeca changed the title WIP: Pagination args Pagination args Jul 21, 2025
@Ckk3
Copy link
Copy Markdown
Contributor

Ckk3 commented Jul 21, 2025

Oh I see, @davidroeca, Thanks for checking♥️!
I will look investigate this too when I review it.
I also had some problems with different python versions on #253 but I think that don't apply to this

Copy link
Copy Markdown
Contributor

@Ckk3 Ckk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your contribuition, @davidroeca
LGTM!
I will only run the pre-commit.

@Ckk3 Ckk3 merged commit fc1eae8 into strawberry-graphql:main Aug 7, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pagination args to generated Connections

3 participants