strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
391 stars 115 forks source link

"OR" filter not working for OneToOne-related models #422

Closed TWeidi closed 3 months ago

TWeidi commented 7 months ago

Describe the bug

When running a query including an "OR" filter the filter gets passed as "AND" (according to the query shown in the Django Debug Toolbar)

Django Model

class SomeBaseModel(models.Model):
     some_base_attr = models.CharField(...)
    ...

class SomeModel(SomeBaseModel):
    some_other_attr = models.FloatField(...)
    ...

Types

@type(models.SomeBaseModel, filters=filters.SomeBaseFilter)
class SomeBaseNode(relay.Node):
    some_base_attr: auto
    ...

@type(models.SomeModel, filters=filters.SomeFilter)
class SomeNode(SomeBaseNode):
    some_other_attr: auto
    ...

Filters

@filter(models.SomeBaseModel, lookups=True)
class SomeBaseFilter:
    some_base_attr: auto
    id: auto
    ...

@filter(models.SomeModel, lookups=True)
class SomeFilter(SomeBaseFilter):
    some_other_attr: auto
    id: auto
    ...

Query

{
  someQuery(filters: {
    id: {exact: "U29tZU5vZGU6MjMwNg=="}
    OR: {id: {exact: "U29tZU5vZGU6MjI3NA=="}}
  }) {
    edges {
      node {
        id
      }
    }
  }
}

I guess it has to do with the build_filter_kwargs function in filters.py. Trying to figure out how the function works, but, unfortunately, it's a bit complicated ;)

System Information

Upvote & Fund

Fund with Polar

TWeidi commented 7 months ago

I played a bit around and the real problem seems to be that the build_filter_kwargs function in filters.py relies on the fields order of filters.__strawberry_definition__.fields. More precisely, the logical fields AND,OR, and NOT fields necessarily need to be the last fields. This order is not guaranteed, since subclassing another Filter (see SomeFilter in my bug report above) will append its fields AFTER the logical fields and filter_kwargs will be an empty Q()-object when entering line 226 of filters.py.

By extending line 183 from

for f in filters.__strawberry_definition__.fields:

to

if filters.__strawberry_definition__.fields:
        filters.__strawberry_definition__.fields.sort(key=lambda x: x.name, reverse=True)

for f in filters.__strawberry_definition__.fields:

seems to fix the problem. Unfortunately, I have no idea if sorting the fields inplace will impact any other code...

bellini666 commented 7 months ago

Hi @TWeidi ,

Hrm, that's really interesting!

Would you like to try to open a PR for this?

Also, I would not sort the fields inplace, but instead just call sorted() which returns a copy of it. i.e.

for f in sorted(filters.__strawberry_definition__.fields, key=lambda x: x.name, reverse=True):
TWeidi commented 7 months ago

Hi @bellini666,

sure, I can do that. Do you want me to add a dedicated test case or are you fine with the existing one ('test_or')?

bellini666 commented 7 months ago

sure, I can do that. Do you want me to add a dedicated test case or are you fine with the existing one ('test_or')?

If you can it would be awesome! I mean, the current tests did not catch this issue, so it would be nice to have something that would have caught it

bellini666 commented 3 months ago

Fixed by https://github.com/strawberry-graphql/strawberry-django/pull/478