-
-
Notifications
You must be signed in to change notification settings - Fork 96
Nulls Last and Nulls First Parameters Sorting #769
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
d4ad40f
fcb2254
f3133d2
90e16c5
685c495
d6d4a65
cdb6468
945d165
f89631e
81e8c32
cce4a62
5730ae8
324a165
5f9d231
355b841
c8d69d6
be783d1
2920100
3e986ed
a72579c
bb02502
ee4328a
d41131d
9690176
93d520d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| from typing import Any, TYPE_CHECKING, Type, cast | ||
| from typing import Any, Optional, TYPE_CHECKING, Type, cast | ||
|
|
||
| from ormar.queryset.actions import OrderAction | ||
| from ormar.queryset.actions.filter_action import METHODS_TO_OPERATORS | ||
| from ormar.queryset.clause import FilterGroup | ||
| from ormar.queryset.clause import FilterGroup, NullsOrdering | ||
|
|
||
| if TYPE_CHECKING: # pragma: no cover | ||
| from ormar import BaseField, Model | ||
|
|
@@ -268,22 +268,40 @@ def isnull(self, other: Any) -> FilterGroup: | |
| """ | ||
| return self._select_operator(op="isnull", other=other) | ||
|
|
||
| def asc(self) -> OrderAction: | ||
| def asc(self, nulls_ordering: Optional[NullsOrdering] = None) -> OrderAction: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also be able to pass this param in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do not understand this
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order by you can pass OrderActions or strings too. If you pass |
||
| """ | ||
| works as sql `column asc` | ||
|
|
||
| :param nulls_ordering: nulls ordering option first or last, defaults to None | ||
| :type nulls_ordering: Optional[NullsOrdering], optional | ||
| :raises ValueError: if nulls_ordering is not None or NullsOrdering Enum | ||
| :return: OrderGroup for operator | ||
| :rtype: ormar.queryset.actions.OrderGroup | ||
| """ | ||
| return OrderAction(order_str=self._access_chain, model_cls=self._source_model) | ||
| if nulls_ordering is not None and not isinstance(nulls_ordering, NullsOrdering): | ||
| raise ValueError("Invalid option for ordering nulls values.") | ||
|
|
||
| def desc(self) -> OrderAction: | ||
| return OrderAction( | ||
| order_str=self._access_chain, | ||
| model_cls=self._source_model, | ||
| nulls_ordering=nulls_ordering.value if nulls_ordering is not None else None, | ||
| ) | ||
|
|
||
| def desc(self, nulls_ordering: Optional[NullsOrdering] = None) -> OrderAction: | ||
| """ | ||
| works as sql `column desc` | ||
|
|
||
| :param nulls_ordering: nulls ordering option first or last, defaults to None | ||
| :type nulls_ordering: Optional[NullsOrdering], optional | ||
| :raises ValueError: if nulls_ordering is not None or NullsOrdering Enum | ||
| :return: OrderGroup for operator | ||
| :rtype: ormar.queryset.actions.OrderGroup | ||
| """ | ||
| if nulls_ordering is not None and not isinstance(nulls_ordering, NullsOrdering): | ||
| raise ValueError("Invalid option for ordering nulls values.") | ||
|
|
||
| return OrderAction( | ||
| order_str="-" + self._access_chain, model_cls=self._source_model | ||
| order_str="-" + self._access_chain, | ||
| model_cls=self._source_model, | ||
| nulls_ordering=nulls_ordering.value if nulls_ordering is not None else None, | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you split this condition into two the result now lacks prefix and will not work with nested models.
You would have in example
{prefix}{table_name}.{field_name} is null, {field_name} {self.direction}.The second field_name does not have the prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I must mention that without this case, the test related to nested models will also pass correctly. Can you give an example of a possible error that may occur? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you have a parent model with two relations to the child model. So like items with
created_byandmodified_bythat lead to the sameUsermodel. Each of those relations in joins has to have different prefixes as otherwise, you would have ambiguous column names.