Closed Kitefiko closed 7 months ago
Hello, I would like to use this issue to discuss and hopefully agree on filtering and ordering changes and fixes that I am currently working on.
Filtering
Fix: Custom filter method on nested object missing
prefix
informationThere is potential error or unexpected behaviour (filtering something different) when using custom filter method on nested filter - filtering on
FruitConnection
@strawberry_django.filters.filter(models.Fruit, lookups=True) class FruitFilter: id: auto name: auto color: ColorFilter @strawberry_django.filters.filter(models.Color, lookups=True) class ColorFilter: id: auto name: auto fruits: FruitFilter special: str def filter_special(self, queryset): # Incorrect -> Missing prefix information -> color__ return queryset.filter(name=self.special) def filter_special_fixed(self, queryset, info, prefix): # Correct -> Prefix is passed in and used return queryset.filter(f"{prefix}name"=self.special)
Currently
info
argument can be ommited. Here I would change it, so all must be defined.
Indeed this would be interesting.
I was already in a situation where I needed the prefix and it wasn't available for me.
The only issue I see with forcing it to be mandatory is that it is a breaking change. Maybe we can do that as long as we can validate the custom filter function at creation time? Because right now it would only be an issue at runtime, making it hard to fix. But if we can raise an issue when creating the filter class, the user can fix everything and ensure that it is working properly.
Another possibility here would be to define it like a custom strawberry field, and retrieve the type from the argument. e.g.
@strawberry_django.filter(SomeModel)
class SomeModelFilter:
foo: auto
@strawberry_django.filter_field
def special(self, queryset, value: str, info, prefix):
...
In this case, the value
's annotation would be used for the filter type itself, and now we can make info
/prefix
mandatory (or not).
What do you think?
Change: Making all Filter fields optional automatically
I don't see use-case where would someone need mandatory filter field. Bonus: This change would fix potentional recursion issues. Btw. current example project has it:
❌ Cannot reference Input Object 'FruitFilter' within itself through a series of non-null fields: 'color.fruits'.
They actually are when using auto
. But when not using the annotations are respected no matter what.
I do understand that the use-cases for this are probably minimum, but I don't like going against the user's annotation when they choose to be explicit about it (instead of using auto)
Also, regarding the example project, I really need to fix it. It was written in the past and I should have fixed it after my major revamp on v0.10.0
For a more modern demo, there's this repo here which I'm planning on merging here.
Change: Ignoring lookups with
None
valueSince there is dedicated lookup
is_null
, lookups with None value would be skipped the same way as they are now if UNSET. This would not apply for custom filter methods. This change would make usage of filters little friendlier and would get rid of errors when None cannot be used for lookup.
For lookups this indeed make sense, but when not using lookups None
can be valid in some situations.
Also, what kind of errors do you mean this change would get rid of?
Ordering
Feature: Custom ordering methods
Fix: Ordering order being ignored Issue
The new API that I would suggest here is this:
The
ordering
argument would belist
->ordering: [FruitOrder!]
Each object in this list must have at most one leaf field otherwise runtime error is thrown.{ order_by_one: fruits(ordering: [{name: ASC}]) { name }, order_by_one_short: fruits(ordering: {name: ASC}) { name }, order_by_multiple: fruits( ordering: [{name: ASC}, {color: {name: ASC}}] ) { name } runtime_error: fruits(ordering: {name: ASC, color: {name: DESC}}) { name }, }
Even tho one can use
order_by_one_short
way to order and thus this might be backwards compatible, exept for the runtime error (that I think would actually be a good thing), probably new keywordordering
should be used and developed as ordering v2?So here it is. @bellini666 what do you think?
I do think a list is the way to go here, but I don't think we can do {attr: ASC/DESC}
unfortunately. I mean, if that is an object than it needs to be properly typed in the schema, and the only way to do that would be to create a type with all the possible columns in there, meaning we can't enforce a single column for each element in the list.
What we can do is something like: [{attr: NAME, direction: DESC}]
, and that attr
would be an enum that we can automatically generate based on the available columns
What do you think?
Fix: Custom filter method on nested object missing
prefix
informationCurrently
info
argument can be ommited. Here I would change it, so all must be defined.Indeed this would be interesting.
I was already in a situation where I needed the prefix and it wasn't available for me.
The only issue I see with forcing it to be mandatory is that it is a breaking change. Maybe we can do that as long as we can validate the custom filter function at creation time? Because right now it would only be an issue at runtime, making it hard to fix. But if we can raise an issue when creating the filter class, the user can fix everything and ensure that it is working properly.
Reasonable middle groud, I like it.
Another possibility here would be to define it like a custom strawberry field, and retrieve the type from the argument. e.g.
@strawberry_django.filter(SomeModel) class SomeModelFilter: foo: auto @strawberry_django.filter_field def special(self, queryset, value: str, info, prefix): ...
In this case, the
value
's annotation would be used for the filter type itself, and now we can makeinfo
/prefix
mandatory (or not).What do you think?
Yeah, I considered this and like the API more, however since it's even less backwards compatible I did not mentioned it.
I assume the way to go about this would obviously be to forbid any extra arguments for field. Additionally this "resolver" method would have prio and current way would be unchanged and deprecated with warnings?
Yeah I like this better TBH. prefix
and info
optional with prefix
behaviour mentioned in docs would suffice too.
Change: Making all Filter fields optional automatically
I don't see use-case where would someone need mandatory filter field. Bonus: This change would fix potentional recursion issues. Btw. current example project has it:
❌ Cannot reference Input Object 'FruitFilter' within itself through a series of non-null fields: 'color.fruits'.
They actually are when using
auto
. But when not using the annotations are respected no matter what.I do understand that the use-cases for this are probably minimum, but I don't like going against the user's annotation when they choose to be explicit about it (instead of using auto)
Yeah, I was overreaching here a bit, maybe have it as a @straberry_django.filter
argument? Or just leave it since it's possible via custom Field class?
Change: Ignoring lookups with
None
valueSince there is dedicated lookup
is_null
, lookups with None value would be skipped the same way as they are now if UNSET. This would not apply for custom filter methods. This change would make usage of filters little friendlier and would get rid of errors when None cannot be used for lookup.For lookups this indeed make sense, but when not using lookups
None
can be valid in some situations.
Completely forgot about non-lookup filters... Maybe use global settings for this or skip None
if field has no method and is from FilterLookup
class?
Also, what kind of errors do you mean this change would get rid of?
Django does not like None for some lookups
>>> from app.models import Color
>>> Color.objects.filter(name__icontains=None)
Traceback (most recent call last):
File "/usr/lib/python3.10/code.py", line 90, in runcode
exec(code, self.locals)
File "<console>", line 1, in <module>
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/manager.py", line 87, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/query.py", line 1436, in filter
return self._filter_or_exclude(False, args, kwargs)
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/query.py", line 1454, in _filter_or_exclude
clone._filter_or_exclude_inplace(negate, args, kwargs)
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/query.py", line 1461, in _filter_or_exclude_inplace
self._query.add_q(Q(*args, **kwargs))
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/sql/query.py", line 1545, in add_q
clause, _ = self._add_q(q_object, self.used_aliases)
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/sql/query.py", line 1576, in _add_q
child_clause, needed_inner = self.build_filter(
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/sql/query.py", line 1491, in build_filter
condition = self.build_lookup(lookups, col, value)
File "/home/kitefiko/.cache/pypoetry/virtualenvs/strawberry-graphql-django-k3aCGng--py3.12/lib/python3.10/site-packages/django/db/models/sql/query.py", line 1323, in build_lookup
raise ValueError("Cannot use None as a query value")
ValueError: Cannot use None as a query value
Ordering
Feature: Custom ordering methods
The new API that I would suggest here is this: The
ordering
argument would belist
->ordering: [FruitOrder!]
Each object in this list must have at most one leaf field otherwise runtime error is thrown.I do think a list is the way to go here, but I don't think we can do
{attr: ASC/DESC}
unfortunately. I mean, if that is an object than it needs to be properly typed in the schema, and the only way to do that would be to create a type with all the possible columns in there, meaning we can't enforce a single column for each element in the list.
Yes the object(s) used would be the same as now.
@strawberry_django.ordering.order(models.Fruit)
class FruitOrder:
name: auto
color: ColorOrder
@strawberry_django.ordering.order(models.Color)
class ColorOrder:
name: auto
fruit: FruitOrder
The enforcement of single value per list item would have to be at runtime.
✔ FruitOrder(name: ASC) ✔ FruitOrder(color: FruitOrder(name: DESC)) ❌ FruitOrder(name: ASC, color: FruitOrder(name: ASC))
Advantages
Disadvantages
What we can do is something like:
[{attr: NAME, direction: DESC}]
, and thatattr
would be an enum that we can automatically generate based on the available columns
I had this approach on graphene project, but direction was encoded in enum -> NAME_ASC
Advantages
Disadvantages
So the actual definition of the order would be pure enum that we would at most validate that is correct against model? What would that look like?
Since django does not support order_by chaining ( maybe could be done via internals?) would it be OK to require method to return queryset & field to order by (or list of them)? Something like:
@strawberry_django.filter(User)
class SomeModelFilter:
first_name: auto
last_name: auto
@strawberry_django.filter_field
def full_name(self, queryset, value: str, info, prefix):
queryset = queryset.alias(fullname=Concat("first_name", Value(" "), "last_name")
return queryset, "fullname"
Django does not like None for some lookups
TBH, the current lookups implementation is non optimal for most cases. E.g. it allows one to use range
for strings/bools
What I would like to do here is to refactor those to make sure that at least those more basic ones (str, int, date/datetime, bool, etc) have proper options.
The enforcement of single value per list item would have to be at runtime.
Although I can see this working, I'm get really worried with something that can only be validated at runtime, because the user that is going to use the API usually is not the same one that is writing it.
One other thing we can do here is to try to find a way to check for the order the arguments were provided.
Since django does not support order_by chaining ( maybe could be done via internals?) would it be OK to require method to return queryset & field to order by (or list of them)?
I like this idea! :)
To summarize:
tuple[QuerySet, list["str of order args"]]
was agreed uponWhat remains to discuss are lookups.
First thing I would like to mention is current implementation of auto
for bool
field when lookups
are enabled for Filter class. Currently type generated is bool
not FilterLookup[bool]
as one would expect. The issue here is that at least one usefull filter is missing - filter all null values out, boolean_field__isnull=False
.
TBH, the current lookups implementation is non optimal for most cases. E.g. it allows one to use
range
for strings/boolsWhat I would like to do here is to refactor those to make sure that at least those more basic ones (str, int, date/datetime, bool, etc) have proper options.
Do we want to by default provide lookups that technically work or technically work AND make sense?
For boolean range
works, but really makes no sense.
For strings range
works and in some cases even makes sense - user_name__range=("A", "D")
For choice field we might want to discard more advanced CharField lookups (contains, startswith etc.) yet those, if used with base type (str, int), might be usefull too.
Only "exact", "iexact" & "range" lookups support None as a value. How do we solve null
here? You cant make field required. You might have custom method, but then the behaviour is not "unified"?
After thinking a bit longer and Boolean being the only real outlier, something like this might be good start?
TLDR; Ignoring null for "exact" & "iexact" with description on lookup so user knows; bool
is limited; enum
has its base type for advanced lookups; __range
is strict now.
class BooleanFilterLookup(Generic[T]):
exact: T | None = UNSET
@filter_field(description="null value is skipped")
def is_null(self, queryset, value: bool, prefix: str):
# Is this how to ignore null values?
if value is not None:
queryset = queryset.filter(**{f"{prefix}isnull":value})
return queryset
class RangeLookup(Generic[T]):
left: T | None = None
right: T | None = None
def filter(self, queryset, prefix: str):
return queryset.filter(**{f"{prefix}range": [self.left, self.right]})
class EnumFilterLookup(BooleanFilterLookup[T], Generic[T, BASE_T]):
in_list: list[T] | None = UNSET
# These might have use aswell but only with BASE TYPE (int/str)
iexact: BASE_T | None = UNSET
range: RangeLookup[BASE_T] | None = UNSET
gt: BASE_T | None = UNSET # ignores null
gte: BASE_T | None = UNSET # ignores null
lt: BASE_T | None = UNSET # ignores null
lte: BASE_T | None = UNSET # ignores null
contains: BASE_T | None = UNSET # ignores null
i_contains: BASE_T | None = UNSET # ignores null
starts_with: BASE_T | None = UNSET # ignores null
i_starts_with: T | None = UNSET # ignores null
ends_with: BASE_T | None = UNSET # ignores null
i_ends_with: BASE_T | None = UNSET # ignores null
regex: BASE_T | None = UNSET # ignores null
i_regex: BASE_T | None = UNSET # ignores null
class FilterLookup(BooleanFilterLookup[T]):
in_list: list[T] | None = UNSET
range: RangeLookup[T] | None = UNSET
gt: T | None = UNSET # ignores null
gte: T | None = UNSET # ignores null
lt: T | None = UNSET # ignores null
lte: T | None = UNSET # ignores null
contains: T | None = UNSET # ignores null
i_contains: T | None = UNSET # ignores null
starts_with: T | None = UNSET # ignores null
i_starts_with: T | None = UNSET # ignores null
ends_with: T | None = UNSET # ignores null
i_ends_with: T | None = UNSET # ignores null
regex: T | None = UNSET # ignores null
i_regex: T | None = UNSET # ignores null
# Transforms consideration ?
class DateFilterLookup(FilterLookup[T]):
year: FilterLookup[int] | None = UNSET
month: FilterLookup[int] | None = UNSET
day: FilterLookup[int] | None = UNSET
week_day: FilterLookup[int] | None = UNSET
iso_week_day: FilterLookup[int] | None = UNSET
week: FilterLookup[int] | None = UNSET
iso_year: FilterLookup[int] | None = UNSET
quarter: FilterLookup[int] | None = UNSET
iso_year: FilterLookup[int] | None = UNSET
quarter: FilterLookup[int] | None = UNSET
class DateTimeFilterLookup(DateFilterLookup[T]):
hour: FilterLookup[int] | None = UNSET
minute: FilterLookup[int] | None = UNSET
second: FilterLookup[int] | None = UNSET
date: FilterLookup[int] | None = UNSET
time: FilterLookup[int] | None = UNSET
Also I would prefer range to be named between? Makes more sense to me TBH.
First thing I would like to mention is current implementation of auto for bool field when lookups are enabled for Filter class. Currently type generated is bool not FilterLookup[bool] as one would expect. The issue here is that at least one usefull filter is missing - filter all null values out, boolean_field__isnull=False.
Oh, you are correct. I basically ignored that condition, even when I refactored that file.
And yeah, I agree with you. It is currently not possible to filter null
from boolean fields.
Do we want to by default provide lookups that technically work or technically work AND make sense?
IMO provide something that work and make sense is the best option, while also leaving room for customization (i.e. the user can subclass the lookup for extra functionality)
For boolean range works, but really makes no sense.
👍🏼 ! For bools, IMO, we want to filter for True
, False
and None
For strings range works and in some cases even makes sense - user_name__range=("A", "D")
I like to think that I have a lot of experience with sql and django lookups in general, but TIL :)
Doing some tests here, gt
/lt
/ge
/le
also works for the same reason range does
For choice field we might want to discard more advanced CharField lookups (contains, startswith etc.) yet those, if used with base type (str, int), might be usefull too.
For choices, the first thing that comes to my mind is to only allow a subset of lookups that makes sense, like exact
and in
. But when I think of integer choices, gt
/lt
/etc can be also useful in some situations where those values have meanings (i.e. a weight)
Maybe we should keep those for choices as well?
Only "exact", "iexact" & "range" lookups support None as a value. How do we solve null here? You cant make field required. You might have custom method, but then the behaviour is not "unified"?
Don't know if that is solvable, due to how GraphQL works. You can't make a field not required that doesn't accept null.
This unfortunately falls into the "we can validate this at runtime only", unless the GraphQL spec evolves to allow a solution for this in the future.
After thinking a bit longer and Boolean being the only real outlier, something like this might be good start?
I like the idea here! :)
I would just call BooleanFilterLookup
as BaseFilterLookup
, as it seems to me that it is the best candidate for a base filter lookup that should work for basically all fields, including 3rd party ones.
Hi, in addition to the custom ordering field, it would be nice to add a way to manipulate the nulls_first
and nulls_last
parameters
I have quite a silly and naive proposal for ordering customization that covers most of the needs:
nulls_first
and nulls_last
parameters (WIP)So for my current situation I use this solution:
def generate_order_args(
order: WithStrawberryObjectDefinition,
*,
queryset: _QS,
sequence: dict[str, _OrderSequence] | None = None,
prefix: str = "",
info: Info | None = None,
):
sequence = sequence or {}
args = []
def sort_key(f: StrawberryField) -> int:
if not (seq := sequence.get(f.name)):
return 0
return seq.seq
for f in sorted(order.__strawberry_definition__.fields, key=sort_key):
ordering = getattr(order, f.name, UNSET)
if ordering is UNSET:
continue
ordering_method = getattr(order, f"order_{f.name}", None)
if ordering_method:
queryset, ordering_params = ordering_method(queryset=queryset, prefix=prefix, ordering=ordering, info=info)
args.extend(ordering_params)
continue
if ordering == Ordering.ASC:
args.append(f"{prefix}{f.name}")
elif ordering == Ordering.DESC:
args.append(f"-{prefix}{f.name}")
else:
queryset, subargs = generate_order_args(
ordering,
queryset=queryset,
prefix=f"{prefix}{f.name}__",
sequence=(seq := sequence.get(f.name)) and seq.children,
info=info
)
args.extend(subargs)
return queryset, args
def apply(
order: WithStrawberryObjectDefinition | None,
queryset: _QS,
info: Info | None = None,
) -> _QS:
if order in (None, strawberry.UNSET):
return queryset
sequence: dict[str, _OrderSequence] = {}
if info is not None and info._raw_info.field_nodes: # noqa: SLF001
field_node = info._raw_info.field_nodes[0] # noqa: SLF001
for arg in field_node.arguments:
if arg.name.value != ORDER_ARG or not isinstance(arg.value, ObjectValueNode):
continue
def parse_and_fill(field: ObjectValueNode, seq: dict[str, _OrderSequence]):
for i, f in enumerate(field.fields):
f_sequence: dict[str, _OrderSequence] = {}
if isinstance(f.value, ObjectValueNode):
parse_and_fill(f.value, f_sequence)
seq[f.name.value] = _OrderSequence(seq=i, children=f_sequence)
parse_and_fill(arg.value, sequence)
queryset, args = generate_order_args(order, queryset=queryset, sequence=sequence, info=info)
if not args:
return queryset
return queryset.order_by(*args)
Usage:
@strawberry.django.order(WorkLoad)
class WorkLoadGqlOrdering:
area: auto
...
def order_area(self, queryset: QuerySet, prefix: str, ordering: Ordering, info: Info) -> (QuerySet, list[str]):
queryset = queryset.alias(area=Coalesce(Sum(f"{prefix}operations__stand__area", distinct=True), 0.0))
return queryset, ["area"] if ordering == Ordering.ASC else ["-area"]
This may look awkward but I'd love to hear comments.
@iamcrookedman I actually like you idea, specially because of the fact that ordering
cannot be applied sequentially (i.e. the next call to order_by
will replace the previous one), so your suggestion makes total sense!
Hello! Thank you for the good discussion.
Another possibility here would be to define it like a custom strawberry field, and retrieve the type from the argument. e.g.
@strawberry_django.filter(SomeModel) class SomeModelFilter: foo: auto
@strawberry_django.filter_field def special(self, queryset, value: str, info, prefix): ...
In this case, the value's annotation would be used for the filter type itself, and now we can make info/prefix mandatory (or not).
What do you think?
@strawberry_django.filters.filter(models.Fruit, lookups=True)
class FruitFilter:
id: auto
name: auto
color: ColorFilter
@strawberry_django.filters.filter(models.Color, lookups=True)
class ColorFilter:
id: auto
name: auto
fruits: FruitFilter
def filter_special(self, queryset):
# Incorrect -> Missing prefix information -> color__
return queryset.filter(name=self.special)
@strawberry_django.filter_field
def special(self, queryset, value: str, info, prefix):
return queryset.filter(f"{prefix}name"=value)
I like this idea. But, i think this cannot solve nested filter problem with NOT, AND, OR e.g.) i want to get color objects without special "red"
# this still not works
query getColors{
colors(filters: {NOT: {special: "red"}}{
...
}
}
Do you have Idea to solve this? I think just prefix is not enough. I know this is quite big change, but how about this?
@strawberry_django.filter_field
def special(self, queryset, value: str, info, prefix) -> Q | QuerySet:
return Q(f"{prefix}name"=value) # change ~Q() in my example (with NOT)
@strawberry_django.filter_field
def special(self, queryset, value: str, info, prefix) -> Q | QuerySet:
return queryset.filter(f"{prefix}name"=value) # work same as before
def q_special(self, queryset, value: str, info, prefix) -> Q:
return Q(f"{prefix}name"=value) # this also can be
Hello @bellini666, found some time and created draft PR https://github.com/strawberry-graphql/strawberry-django/pull/478
I believe it solves a lot of current known problems to me - filter bugs, errors on null, custom filters & order methods, nulls ordering, filter methods using Q object, API enforcement
I would love for you to take a look to verify overall direction (PR should be fully functional however). Verify lookups, API design etc.
Few question
null
even for exact
, iexact
, make user use isnull
(I would argue, this is better anyway). This was mentioned here - https://github.com/strawberry-graphql/strawberry-django/issues/459EDIT: I would consider current version of PR to be fully done.
Hello, I would like to use this issue to discuss and hopefully agree on filtering and ordering changes and fixes that I am currently working on.
Filtering
Fix: Custom filter method on nested object missing
prefix
informationThere is potential error or unexpected behaviour (filtering something different) when using custom filter method on nested filter - filtering on
FruitConnection
Currently
info
argument can be ommited. Here I would change it, so all must be defined.Change: Making all Filter fields optional automatically
I don't see use-case where would someone need mandatory filter field. Bonus: This change would fix potentional recursion issues. Btw. current example project has it:
❌ Cannot reference Input Object 'FruitFilter' within itself through a series of non-null fields: 'color.fruits'.
Change: Ignoring lookups with
None
valueSince there is dedicated lookup
is_null
, lookups with None value would be skipped the same way as they are now if UNSET. This would not apply for custom filter methods. This change would make usage of filters little friendlier and would get rid of errors when None cannot be used for lookup.Ordering
Feature: Custom ordering methods
Fix: Ordering order being ignored Issue
The new API that I would suggest here is this:
The
ordering
argument would belist
->ordering: [FruitOrder!]
Each object in this list must have at most one leaf field otherwise runtime error is thrown.Even tho one can use
order_by_one_short
way to order and thus this might be backwards compatible, exept for the runtime error (that I think would actually be a good thing), probably new keywordordering
should be used and developed as ordering v2?So here it is. @bellini666 what do you think?
Upvote & Fund