Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new ordering method allowing ordering by multiple fields #679

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add new ordering method allowing ordering by multiple fields #679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a721659
Add new ordering method allowing ordering by multiple fields
diesieben07 Dec 22, 2024
cb815bd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions strawberry_django/fields/field.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,7 @@ def field(
pagination: bool | UnsetType = UNSET,
filters: type | UnsetType | None = UNSET,
order: type | UnsetType | None = UNSET,
ordering: type | UnsetType | None = UNSET,
only: TypeOrSequence[str] | None = None,
select_related: TypeOrSequence[str] | None = None,
prefetch_related: TypeOrSequence[PrefetchType] | None = None,
Expand Down Expand Up @@ -576,6 +577,7 @@ def field(
pagination: bool | UnsetType = UNSET,
filters: type | UnsetType | None = UNSET,
order: type | UnsetType | None = UNSET,
ordering: type | UnsetType | None = UNSET,
only: TypeOrSequence[str] | None = None,
select_related: TypeOrSequence[str] | None = None,
prefetch_related: TypeOrSequence[PrefetchType] | None = None,
Expand Down Expand Up @@ -604,6 +606,7 @@ def field(
pagination: bool | UnsetType = UNSET,
filters: type | UnsetType | None = UNSET,
order: type | UnsetType | None = UNSET,
ordering: type | UnsetType | None = UNSET,
only: TypeOrSequence[str] | None = None,
select_related: TypeOrSequence[str] | None = None,
prefetch_related: TypeOrSequence[PrefetchType] | None = None,
Expand Down Expand Up @@ -631,6 +634,7 @@ def field(
pagination: bool | UnsetType = UNSET,
filters: type | UnsetType | None = UNSET,
order: type | UnsetType | None = UNSET,
ordering: type | UnsetType | None = UNSET,
only: TypeOrSequence[str] | None = None,
select_related: TypeOrSequence[str] | None = None,
prefetch_related: TypeOrSequence[PrefetchType] | None = None,
Expand Down Expand Up @@ -672,6 +676,7 @@ def field(
filters=filters,
pagination=pagination,
order=order,
ordering=ordering,
extensions=extensions,
only=only,
select_related=select_related,
Expand Down
164 changes: 161 additions & 3 deletions strawberry_django/ordering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from graphql.language.ast import ObjectValueNode
from strawberry import UNSET
from strawberry.types import has_object_definition
from strawberry.types.base import WithStrawberryObjectDefinition
from strawberry.types.base import StrawberryOptional, WithStrawberryObjectDefinition
from strawberry.types.field import StrawberryField, field
from strawberry.types.unset import UnsetType
from strawberry.utils.str_converters import to_camel_case
Expand All @@ -30,6 +30,7 @@
from strawberry_django.utils.typing import is_auto

from .arguments import argument
from .settings import strawberry_django_settings

if TYPE_CHECKING:
from collections.abc import Collection, Sequence
Expand Down Expand Up @@ -194,17 +195,102 @@
return queryset.order_by(*args)


def process_ordering_default(
ordering: Collection[WithStrawberryObjectDefinition] | None,
info: Info | None,
queryset: _QS,
prefix: str = "",
) -> tuple[_QS, Collection[F | OrderBy | str]]:
args = []

for o in ordering:

Check failure on line 206 in strawberry_django/ordering.py

View workflow job for this annotation

GitHub Actions / Typing 

Object of type "None" cannot be used as iterable value (reportOptionalIterable)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: probably need an early return for when ordering is None

for f in o.__strawberry_definition__.fields:
f_value = getattr(o, f.name, UNSET)
if f_value is UNSET or (
f_value is None and not f.metadata.get(WITH_NONE_META)
):
continue

if isinstance(f, FilterOrderField) and f.base_resolver:
res = f.base_resolver(
o,
info,
value=f_value,
queryset=queryset,
prefix=prefix,
)
if isinstance(res, tuple):
queryset, subargs = res
else:
subargs = res
args.extend(subargs)
elif isinstance(f_value, Ordering):
args.append(f_value.resolve(f"{prefix}{f.name}"))
else:
ordering_cls = f.type
if isinstance(ordering_cls, StrawberryOptional):
ordering_cls = ordering_cls.of_type
assert isinstance(ordering_cls, type) and has_object_definition(
ordering_cls
)
queryset, subargs = process_ordering(
ordering_cls,
(f_value,),
info,
queryset,
prefix=f"{prefix}{f.name}__",
)
args.extend(subargs)

return queryset, args


def process_ordering(
ordering_cls: type[WithStrawberryObjectDefinition],
ordering: Collection[WithStrawberryObjectDefinition] | None,
info: Info | None,
queryset: _QS,
prefix: str = "",
) -> tuple[_QS, Collection[F | OrderBy | str]]:
if callable(
order_method := getattr(ordering_cls, "process_ordering", None),
):
return order_method(order, info, queryset=queryset, prefix=prefix)

Check failure on line 258 in strawberry_django/ordering.py

View workflow job for this annotation

GitHub Actions / Typing 

Type "object" is not assignable to return type "tuple[_QS@process_ordering, Collection[F | OrderBy | str]]"
  "object" is not assignable to "tuple[_QS@process_ordering, Collection[F | OrderBy | str]]" (reportReturnType)
return process_ordering_default(ordering, info, queryset, prefix)


def apply_ordering(
ordering_cls: type[WithStrawberryObjectDefinition],
ordering: Collection[WithStrawberryObjectDefinition] | None,
info: Info | None,
queryset: _QS,
) -> _QS:
queryset, args = process_ordering(ordering_cls, ordering, info, queryset)
if args:
queryset = queryset.order_by(*args)
return queryset


class StrawberryDjangoFieldOrdering(StrawberryDjangoFieldBase):
def __init__(self, order: type | UnsetType | None = UNSET, **kwargs):
def __init__(
self,
order: type | UnsetType | None = UNSET,
ordering: type | UnsetType | None = UNSET,
Comment on lines +277 to +278
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I think it's worth to add a check/raise here in case both are defined

Copy link
Member

Choose a reason for hiding this comment

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

Also raise a deprecation warning when using order, suggesting to use ordering instead (same for the type definition)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to be able to define both so that users can update their APIs in a backwards compatible way.

**kwargs,
):
if order and not has_object_definition(order):
raise TypeError("order needs to be a strawberry type")
if ordering and not has_object_definition(ordering):
raise TypeError("ordering needs to be a strawberry type")

self.order = order
self.ordering = ordering
super().__init__(**kwargs)

def __copy__(self) -> Self:
new_field = super().__copy__()
new_field.order = self.order
new_field.ordering = self.ordering
return new_field

@property
Expand All @@ -214,6 +300,12 @@
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:
Copy link
Member

Choose a reason for hiding this comment

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

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?

ordering = self.get_ordering()
if ordering is not None:
arguments.append(
argument("ordering", ordering, is_list=True, default=[])
)
return super().arguments + arguments

@arguments.setter
Expand All @@ -236,16 +328,82 @@

return order if order is not UNSET else None

def get_ordering(self) -> type[WithStrawberryObjectDefinition] | None:
ordering = self.ordering
if ordering is None:
return None

if ordering is UNSET:
django_type = self.django_type
ordering = (
django_type.__strawberry_django_definition__.ordering
if django_type not in (None, UNSET)
else None
)

return ordering

Check failure on line 344 in strawberry_django/ordering.py

View workflow job for this annotation

GitHub Actions / Typing 

Type "type | UnsetType | type[WithStrawberryObjectDefinition] | None" is not assignable to return type "type[WithStrawberryObjectDefinition] | None"
  Type "type | UnsetType | type[WithStrawberryObjectDefinition] | None" is not assignable to type "type[WithStrawberryObjectDefinition] | None"
    Type "UnsetType" is not assignable to type "type[WithStrawberryObjectDefinition] | None"
      Type "UnsetType" is not assignable to type "type[WithStrawberryObjectDefinition]"
      "UnsetType" is not assignable to "None" (reportReturnType)

def get_queryset(
self,
queryset: _QS,
info: Info,
*,
order: WithStrawberryObjectDefinition | None = None,
ordering: list[WithStrawberryObjectDefinition] | None = None,
**kwargs,
) -> _QS:
queryset = super().get_queryset(queryset, info, **kwargs)
return apply(order, queryset, info=info)
queryset = apply(order, queryset, info=info)
if ordering_cls := self.get_ordering():
queryset = apply_ordering(ordering_cls, ordering, info, queryset)
return queryset


@dataclass_transform(
order_default=True,
field_specifiers=(
StrawberryField,
field,
),
)
def ordering(
model: type[Model],
*,
name: str | None = None,
one_of: bool | None = None,
description: str | None = None,
directives: Sequence[object] | None = (),
) -> Callable[[_T], _T]:
def wrapper(cls):
nonlocal one_of
try:
cls.__annotations__ # noqa: B018
except AttributeError:
# FIXME: Manual creation for python 3.9 (remove when 3.9 is dropped)
cls.__annotations__ = {}

for fname, type_ in cls.__annotations__.items():
if is_auto(type_):
type_ = Ordering # noqa: PLW2901

cls.__annotations__[fname] = Optional[type_]

field_ = cls.__dict__.get(fname)
if not isinstance(field_, StrawberryField):
setattr(cls, fname, UNSET)

if one_of is None:
one_of = strawberry_django_settings()["ORDERING_DEFAULT_ONE_OF"]

return strawberry.input(
cls,
name=name,
one_of=one_of,
description=description,
directives=directives,
)

return wrapper


@dataclass_transform(
Copy link
Member

Choose a reason for hiding this comment

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

todo: worth decorating this with @deprecated and alerting to use ordering instead

Expand Down
4 changes: 4 additions & 0 deletions strawberry_django/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ class StrawberryDjangoSettings(TypedDict):
#: to set it to unlimited.
PAGINATION_DEFAULT_LIMIT: Optional[int]

#: Whether ordering inputs are marked with oneOf directive by default.
ORDERING_DEFAULT_ONE_OF: bool
Comment on lines +49 to +50
Copy link
Member

Choose a reason for hiding this comment

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

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)



DEFAULT_DJANGO_SETTINGS = StrawberryDjangoSettings(
FIELD_DESCRIPTION_FROM_HELP_TEXT=False,
Expand All @@ -57,6 +60,7 @@ class StrawberryDjangoSettings(TypedDict):
DEFAULT_PK_FIELD_NAME="pk",
USE_DEPRECATED_FILTERS=False,
PAGINATION_DEFAULT_LIMIT=100,
ORDERING_DEFAULT_ONE_OF=False,
)


Expand Down
5 changes: 5 additions & 0 deletions strawberry_django/type.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ def _process_type(
field_cls: type[StrawberryDjangoField] = StrawberryDjangoField,
filters: Optional[type] = None,
order: Optional[type] = None,
ordering: Optional[type] = None,
pagination: bool = False,
partial: bool = False,
is_filter: Union[Literal["lookups"], bool] = False,
Expand Down Expand Up @@ -133,6 +134,7 @@ def _process_type(
is_filter=is_filter,
filters=filters,
order=order,
ordering=ordering,
pagination=pagination,
disable_optimization=disable_optimization,
store=OptimizerStore.with_hints(
Expand Down Expand Up @@ -409,6 +411,7 @@ class StrawberryDjangoDefinition(Generic[_O, _M]):
is_filter: Union[Literal["lookups"], bool] = False
filters: Optional[type] = None
order: Optional[type] = None
ordering: Optional[type] = None
pagination: bool = False
field_cls: type[StrawberryDjangoField] = StrawberryDjangoField
disable_optimization: bool = False
Expand All @@ -434,6 +437,7 @@ def type( # noqa: A001
extend: bool = False,
filters: Optional[type] = None,
order: Optional[type] = None,
ordering: Optional[type] = None,
pagination: bool = False,
only: Optional[TypeOrSequence[str]] = None,
select_related: Optional[TypeOrSequence[str]] = None,
Expand Down Expand Up @@ -471,6 +475,7 @@ def wrapper(cls: _T) -> _T:
filters=filters,
pagination=pagination,
order=order,
ordering=ordering,
only=only,
select_related=select_related,
prefetch_related=prefetch_related,
Expand Down
Loading
Loading