feat: New Paginated generic to be used as a wrapped for paginated results #642
+2,142
−112
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #642 +/- ##
==========================================
+ Coverage 88.77% 88.99% +0.21%
==========================================
Files 41 41
Lines 3607 3724 +117
==========================================
+ Hits 3202 3314 +112
- Misses 405 410 +5 ☔ View full report in Codecov by Sentry. |
bellini666
force-pushed
the
pagination
branch
3 times, most recently
from
253ccc7
to
2ca008c
Compare
bellini666
force-pushed
the
pagination
branch
from
e32366b
to
eda46e7
Compare
Reviewer's Guide by SourceryThis pull request introduces a new generic type User journey diagram for querying paginated resultsjourney
title User Journey for Querying Paginated Results
section Query Fruits
User -> GraphQL API: Request fruits with pagination
GraphQL API -> Database: Fetch fruits with pagination
Database -> GraphQL API: Return paginated fruits
GraphQL API -> User: Return paginated fruits with totalCount and pageInfo
section Query Colors with Nested Fruits
User -> GraphQL API: Request colors with nested fruits pagination
GraphQL API -> Database: Fetch colors and nested fruits with pagination
Database -> GraphQL API: Return paginated colors and nested fruits
GraphQL API -> User: Return paginated colors with nested fruits, totalCount, and pageInfo
Class diagram for the new Paginated generic typeclassDiagram
class Paginated {
+Optional~QuerySet~ queryset
+OffsetPaginationInput pagination
+PaginatedInfo page_info()
+int total_count()
+list~NodeType~ results()
+int get_total_count()
+Optional~QuerySet~ get_paginated_queryset()
}
class OffsetPaginationInput {
+int offset
+int limit
}
class PaginatedInfo {
+int limit
+int offset
}
Paginated --> OffsetPaginationInput
Paginated --> PaginatedInfo
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bellini666 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Great addition of the
Paginatedgeneric type. The implementation is solid, well-integrated with existing components, and thoroughly tested. The extensive test coverage is particularly commendable.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
bellini666
force-pushed
the
pagination
branch
from
ce87a26
to
4c9c068
Compare
patrick91
reviewed
Oct 19, 2024
Comment on lines
+142
to
+155
| @strawberry_django.field | ||
| def average_price(self) -> Decimal: | ||
| if self.queryset is None: | ||
| return Decimal(0) | ||
|
|
||
| return self.queryset.aggregate(Avg("price"))["price__avg"] | ||
|
|
||
| @strawberry_django.field | ||
| def paginated_average_price(self) -> Decimal: | ||
| paginated_queryset = self.get_paginated_queryset() | ||
| if paginated_queryset is None: | ||
| return Decimal(0) | ||
|
|
||
| return paginated_queryset.aggregate(Avg("price"))["price__avg"] |
There was a problem hiding this comment.
obs. this also works for relay connections (I've done some similar stuff in the past like this :)
| @@ -574,6 +574,15 @@ def _optimize_prefetch_queryset( | |||
| else: | |||
| mark_optimized = False | |||
|
|
|||
| if isinstance(field.type, type) and issubclass(field.type, Paginated): | |||
| pagination = field_kwargs.get("pagination") | |||
| qs = apply_window_pagination( | |||
There was a problem hiding this comment.
is there a way to customise the queryset? should we document it?
There was a problem hiding this comment.
Hrm I haven't thought about that... good idea! Make it similar to the Connection
There was a problem hiding this comment.
I refactored everything to be used similarly to how the Connection works, with an extension.
It is in this commit: b5a226d (#642)
bellini666
force-pushed
the
pagination
branch
from
b5a226d
to
9705ab0
Compare
thclark
reviewed
Oct 22, 2024
docs/guide/pagination.md
Outdated
|
|
||
| @strawberry.type | ||
| class Query: | ||
| @straberry.offset_paginated(OffsetPaginated[Fruit], order=order) |
There was a problem hiding this comment.
Typo here @bellini666 but this is really gorgeous, overall, I'm loving this feature.
thclark
reviewed
Oct 22, 2024
| return queryset | ||
| ``` | ||
|
|
||
| This would produce the following schema: |
There was a problem hiding this comment.
I'm a huge fan of adding the resulting schema to the docs for each of these things, it's been a big point of frustration for me with the docs not seeing how it should come out.
There was a problem hiding this comment.
Thanks for the feedback! I'm planing on doing some improvements to the docs soon and I'll do more of this!
|
@bellini666 and @patrick91 I'd love to see this released, anything I can do to help expedite? |
patrick91
approved these changes
Nov 8, 2024
|
|
||
| ### Default limit for pagination | ||
|
|
||
| The default limit for pagination is set to `100`. This can be changed in the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix #408
Paginatedgeneric typePaginatedwith StrawberryDjangoFieldSummary by Sourcery
Introduce a new Paginated generic type for handling paginated results, integrate it with StrawberryDjangoField, and update documentation and tests to support this new feature.
New Features:
Enhancements:
Documentation:
Tests: