Add new ordering method allowing ordering by multiple fields #679
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new ordering method, Sequence diagram for the new ordering processsequenceDiagram
participant C as Client
participant F as Field
participant O as Ordering Processor
participant Q as QuerySet
C->>F: Query with ordering list
F->>O: process_ordering()
O->>O: process_ordering_default()
O->>Q: Apply ordering args
Q-->>F: Ordered QuerySet
F-->>C: Ordered results
Class diagram showing the new ordering functionalityclassDiagram
class StrawberryDjangoFieldOrdering {
+order: type|UnsetType|None
+ordering: type|UnsetType|None
+__init__(order, ordering, **kwargs)
+get_order()
+get_ordering()
+get_queryset(queryset, info, order, ordering, **kwargs)
}
class StrawberryDjangoSettings {
+ORDERING_DEFAULT_ONE_OF: bool
}
class OrderingDecorator {
<<function>>
+model: Model
+name: str|None
+one_of: bool|None
+description: str|None
+directives: Sequence[object]|None
}
StrawberryDjangoFieldOrdering ..> StrawberryDjangoSettings : uses
OrderingDecorator ..> StrawberryDjangoSettings : uses settings
note for StrawberryDjangoFieldOrdering "New ordering support added"
note for OrderingDecorator "New @ordering decorator"
note for StrawberryDjangoSettings "Added ORDERING_DEFAULT_ONE_OF setting"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #679 +/- ##
==========================================
+ Coverage 89.17% 89.29% +0.12%
==========================================
Files 41 41
Lines 3778 3849 +71
==========================================
+ Hits 3369 3437 +68
- Misses 409 412 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is looking great, thanks for the work @diesieben07 :)
Left some suggestions. Some things I'm missing from this (probably you already have in your TODO:
- Add a test which checks the schema output of the ordering, especially to see the
oneOfdirective being used :) - Update the documentation to reflect the new usage, and maybe keep the old one but in a
(legacy)session?
| ) -> tuple[_QS, Collection[F | OrderBy | str]]: | ||
| args = [] | ||
|
|
||
| for o in ordering: |
There was a problem hiding this comment.
suggestion: probably need an early return for when ordering is None
| order: type | UnsetType | None = UNSET, | ||
| ordering: type | UnsetType | None = UNSET, |
There was a problem hiding this comment.
suggestion: I think it's worth to add a check/raise here in case both are defined
There was a problem hiding this comment.
Also raise a deprecation warning when using order, suggesting to use ordering instead (same for the type definition)
There was a problem hiding this comment.
We need to be able to define both so that users can update their APIs in a backwards compatible way.
| @@ -214,6 +300,12 @@ def arguments(self) -> list[StrawberryArgument]: | |||
| order = self.get_order() | |||
| if order and order is not UNSET: | |||
| arguments.append(argument("order", order, is_optional=True)) | |||
| if self.base_resolver is None: | |||
There was a problem hiding this comment.
question: why is this if not the same as the above? shouldn't the logic to apply ordering be the same, only changing if we call get_order or get_ordering?
| directives=directives, | ||
| ) | ||
|
|
||
| return wrapper | ||
|
|
||
|
|
||
| @dataclass_transform( |
There was a problem hiding this comment.
todo: worth decorating this with @deprecated and alerting to use ordering instead
| #: Whether ordering inputs are marked with oneOf directive by default. | ||
| ORDERING_DEFAULT_ONE_OF: bool |
There was a problem hiding this comment.
suggestion: I don't think we need this option. IMO one_of should be True by default, and if the user wants it to be False you already added an option for that (but, in my mind, it doesn't make sense for it to not be one_of)
Add new
@strawberry_django.ordering()annotation that supersedes@strawberry_django.order()offering better support for ordering by multiple fields.This is still a draft, because it is not finished, but I have added it here for feedback.
Description
Currently the behavior is almost exactly the same as for
@strawberry_django.orderexcept for the following changes:@strawberry_django.ordering()exposes theone_ofsetting of@strawberry.input()and allows setting a default via the Strawberry Django settings. It is useful to set this to true, because sorting by multiple fields should be given as a list of objects with a single field set each. This way object property order is irrelevant for the sorting.OrderSequencetype and its associated machinery is removed fromorderingStill missing:
ordertoorderingin a schema-compatible way:This already works, you can just have both parameters, but we should validate that only one of them is set by clients and send a useful error message.
ordering.Types of Changes
Issues Fixed or Closed by This PR
Fixes #678.
Checklist
Summary by Sourcery
Add
orderingfield to allow ordering by a list of fields.New Features:
@strawberry_django.ordering()annotation to supersede@strawberry_django.order().orderingfield takes a list of orderings, allowing for more complex ordering logic such as "sort by name, then by birthdate".one_ofsetting to@strawberry_django.ordering()to allow defaulting via Django settings.Tests:
orderingfunctionality.