netbox-community / netbox

The premier source of truth powering network automation. Open source under Apache 2. Try NetBox Cloud free: https://netboxlabs.com/free-netbox-cloud/
http://netboxlabs.com/oss/netbox/
Apache License 2.0
15.95k stars 2.56k forks source link

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

Open arthanson opened 1 day ago

arthanson commented 1 day ago

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 commented 1 day ago

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 (https://github.com/strawberry-graphql/strawberry-django/issues/448).

  1. 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
    }
  }
}