GraphQL filters (AND, OR and NOT) don't work for custom filterset fields

[arthanson]

Deployment Type

Self-hosted

NetBox Version

v4.1.3

Python Version

3.10

Steps to Reproduce

Using a GraphQL filter with AND, OR, NOT for a field that has custom implementation in the filterset (or only appears in the filterset) for example asn_id on Site. Doesn't work

1. Create 4 sites with ID's 1 to 4
2. Create 4 ASNs and assign each to a single Site (1-4)
3. Use the following GraphQL query

{
  site_list(filters: {asn_id: "1", OR: {asn_id: "4"}}) {
    name
    asns {
      id
    }
  }
}

Expected Behavior

Will get a list of 2 sites.

Observed Behavior

Get an empty list.

[arthanson]

Copying this from issue #16024 as that was resolved for the specific case of the ChoiceField, but there is still and underling issue with Strawberry and django-filters interaction.

The basic issue is strawberry filtering uses Q objects, django-filters returns filtersets which makes them incompatible. None of the solutions I see look very good and all would take a lot of work.

Graphene interfaces with Django-filter and uses filtersets. Code is at https://github.com/graphql-python/graphene-django/tree/main/graphene_django/filter. About 8 files of code.

Strawberry is all based off of Q objects. Code is at https://github.com/strawberry-graphql/strawberry-django/blob/main/strawberry_django/filters.py. Strawberry documentation on filters is at: https://strawberry.rocks/docs/django/guide/filters

From what I can see, this is also tied into the GraphQL parsing framework, so if we tried to use Django-filter we would also probably have to make patches to that code as well.

We are also using Strawberry Legacy Filtering which needs to be removed (https://strawberry.rocks/docs/django/guide/filters#legacy-filtering) this would need changes to the auto type decorator (https://github.com/netbox-community/netbox/blob/develop/netbox/netbox/graphql/filter_mixins.py#L131).

I can think of several different potential solutions (Option 4 might potentially be the best option?)

1. Basically replicate filter handling in Django-graphene - override strawberry_django.process_filters to work with django-filters basically replacing strawberry's filter handling with a new one that is compatible with Django-filters.

   • pros: No changes to existing NetBox filter code. Plugin filtering / GraphQL code would remain compatible.
   • cons: Have to replicate all graphene filter handling code which was mentioned to be bug-prone. A lot of work needed for POC. Not compatible with any future Strawberry filtering enhancements and probably brittle for future updates to Strawberry. Lots of potential for edge-case bugs.

2. Change NetBox filterset functions to return Q objects (or provide sub-functions returning Q objects) which should make them compatible with Strawberry lookup code. (I think this might be the most straight-forward solution)

   Functions like (https://github.com/netbox-community/netbox/blob/develop/netbox/dcim/filtersets.py#L1152).
   So it is not possible to convert a QuerySet to a Q object, Django-filter is pretty much built around QuerySet. Many use Q objects - but some (https://github.com/netbox-community/netbox/blob/develop/netbox/dcim/filtersets.py#L607) are just query set - although exclude could be converted to a negative Q object ~Q(...)

   • pros: Keeps filtering code in one place. Quick to do a POC and could actually implement it filter by filter. Less potential for edge-case bugs.
   • cons: Have to modify NetBox filtering code. Plugin filtering code would need to be changed to support this.

3. Can override the default filter method (https://strawberry.rocks/docs/django/guide/filters#overriding-the-default-filter-method) to just call the filter set, but then would need to handle the (AND, OR, NOT, ...) parsing ourselves.

   • pros: No changes to NetBox filter code. Plugin filtering / GraphQL code would remain compatible.
   • cons: A lot of work needed for POC. Most brittle for future Strawberry versions. More potential for edge-case bugs.

4. [Potential Best Option?] Write custom Strawberry filters for each of the filterset methods. This is what Strawberry is sort-of designed for, but it would require replicating / duplicating all the special NetBox filter handling in a Strawberry compatible way. Similar to 2 but leaves the existing filter code (non GraphQL) untouched.

   • pros: No changes to NetBox filter code. Can just be added, no POC needed. Less potential for edge-case bugs.
   • cons: Need to duplicate filtering logic specifically for GraphQL and plugins would need to do the same. Not DRY.

   Note: Could create the Strawberry filters as Q objects as a first pass, then migrate those over to the filters code, thus doing item 2 above, but would allow time to test the filters in the real world.

Support for Django-filter has been requested in Strawberry, but it doesn't look like it will be implemented (strawberry-graphql/strawberry-django#448).

5. Move back to Graphene / abandoning Strawberry.

   • pros: Known entity and we know filtering works. Could be done fairly easily by rolling back to old code.
   • cons: graphene-django is still semi-dead with about 1 commit every 2-3 months (see: https://github.com/graphql-python/graphene-django/commits/main/). Has minor issues / lack of full support for newer versions of Django.

strawberry-django code here is where it is calling deprecated filterset (as we have the USE_DEPRECATED_FILTERS flag) (https://github.com/strawberry-graphql/strawberry-django/blob/main/strawberry_django/filters.py#L235) - this only returns the queryset and bypasses the creation of the Q object. q in this case ends up as “q: (AND: )“. So it just doesn’t work.

Docs for the new filtering code (https://strawberry.rocks/docs/django/guide/filters#custom-filter-methods) as you can see this requires returning a Q object (see: https://strawberry.rocks/docs/django/guide/filters#resolver-return) the Q object is what it uses for processing.

So, to use Strawberry as-is would need to remove the USE_DEPRECATED_FILTERS flag, then rewrite our filter_by_filterset to return a Q object (which django-filter doesn’t do).

If you put a breakpoint at the end of filter_by_filterset in netbox/graphql/filter_mixins.py you will see it is getting called from Strawberry’s process_filters function in that _process_deprecated_filter function. Can use a query like:

{
  site_list(filters: {asn_id: "1", OR: {asn_id: "4"}}) {
    name
    asns {
      id
    }
  }
}

[akarneliuk]

My humble ask is just get back to graphene and stop changing API format. Or whatever changes you do internally, api still shall stay untouched.

It is just bonkers to try to keep up with breaking changes in graphql now. So i updated netbox from 4.0 to 4.1 and now simple thing in graphql that was working is not working anymore.
I talk about status. In all filters it had type [String!], which is great and easy for filtering. Now it is String and instead of filter: {status: ["a", "b", "c"]}, which is perfectly passable via variables, i need to do filter: {status: "a", OR: {"status": "b", OR: {"status": "c"}}}. That is way less user friendly and difficult to pass programmatically properly. I can use jinja templating of requests, but that defeats the purpose of variablesin graphql.

It was massive change in gql api from 3.7 to 4.0, and now it is again from 4.0 to 4.1. It is simply not sustainable to rewrite integrations permanently.

[jeremystretch]

@akarneliuk at the time the decision was made to move to Strawberry, graphene-django was a dying project. If you'd like to propose moving back to it, I encourage you to take a stab at rewriting the GraphQL implementation using graphene-django] v3 yourself, and sharing what you learn in the process. If you're successful, we can certainly consider making the move in a future release.