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.

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

Filtering & Ordering overhaul #399

Closed
Kitefiko opened this issue Oct 23, 2023 · 11 comments
Closed

Filtering & Ordering overhaul #399

Kitefiko opened this issue Oct 23, 2023 · 11 comments

Comments

@Kitefiko
Copy link
Contributor

Kitefiko commented Oct 23, 2023

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 information

There 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.

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 value

Since 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 be list -> 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 keyword ordering should be used and developed as ordering v2?


So here it is. @bellini666 what do you think?

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@bellini666
Copy link
Member

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 information

There 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 value

Since 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 be list -> 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 keyword ordering 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?

@Kitefiko
Copy link
Contributor Author

Fix: Custom filter method on nested object missing prefix information

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.

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 make info/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 value

Since 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 be list -> 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

  • same GQL api for filters and orders
  • Current solution and objects can be mostly used
  • Easier for development (recursion etc.) ?

Disadvantages

  • Runtime check - type is not completely strict

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

I had this approach on graphene project, but direction was encoded in enum -> NAME_ASC

Advantages

  • Strict type

Disadvantages

  • Not as "clean" API IMO
  • Nested orders would most likery require explicit definition on enum and could not be "composed" as now

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?

Order custom method "issue"

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"

@bellini666
Copy link
Member

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! :)

@Kitefiko
Copy link
Contributor Author

Kitefiko commented Nov 9, 2023

To summarize:

  • thanks to Ordering order being ignored #150 (good job btw.), order refactoring is no longer needed
  • custom ordering method via order_field returning tuple[QuerySet, list["str of order args"]] was agreed upon
  • custom filtering method redo via filter_field was agreed upon

What 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/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.

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.

@bellini666
Copy link
Member

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.

@iamcrookedman
Copy link

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

@iamcrookedman
Copy link

iamcrookedman commented Nov 30, 2023

I have quite a silly and naive proposal for ordering customization that covers most of the needs:

  • Possibility to annotate values whose logic may be hidden within the encapsulation
  • Sorting by several fields even if one is passed (sometimes a business requires quite complex ordering logic)
  • Ability to change set 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.

@bellini666
Copy link
Member

@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!

@OdysseyJ
Copy link
Contributor

OdysseyJ commented Dec 15, 2023

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

@Kitefiko
Copy link
Contributor Author

Kitefiko commented Feb 9, 2024

Hello @bellini666,
found some time and created draft PR #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

  • I made filter changes backwards compatible with and default set to new version, however I would love to rip them out completely due to their potential undefined behavior (not using prefix etc.)
  • We could ignore null even for exact, iexact, make user use isnull (I would argue, this is better anyway). This was mentioned here - lookups doesn't accept null #459
  • ability to map/remap lookups for types, ability to choose custom Ordering enum?

EDIT:
I would consider current version of PR to be fully done.

@bellini666
Copy link
Member

Fixed by #478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants