netbox-community / netbox

The premier source of truth powering network automation. Open source under Apache 2. Public demo: https://demo.netbox.dev
http://netboxlabs.com/oss/netbox/
Apache License 2.0
15.68k stars 2.53k forks source link

New GraphQL filters (AND, OR and NOT) don't work as intended #16024

Open NetaliDev opened 3 months ago

NetaliDev commented 3 months ago

Deployment Type

Self-hosted

NetBox Version

v4.0.0

Python Version

3.11

Steps to Reproduce

Let's take the OR-Filter as an example: As described in the docs, I would like to filter for VMs that are in the active and in the staged status:

{
  virtual_machine_list(filters: {status: "staged", OR: {status: "active"}}) {
    name
    status
  }
}

Returns:

{
  "data": {
    "virtual_machine_list": []
  }
}

As seen, this query returns an empty result, while distinct queries for staged and active VMs return results:

{
  virtual_machine_list(filters: {status: "staged"}) {
    name
    status
  }
}

Returns:

{
  "data": {
    "virtual_machine_list": [
      {
        "name": "vm3",
        "status": "staged"
      },
      {
        "name": "vm4",
        "status": "staged"
      }
    ]
  }
}

And

{
  virtual_machine_list(filters: {status: "active"}) {
    name
    status
  }
}

Returns:

{
  "data": {
    "virtual_machine_list": [
      {
        "name": "vm1",
        "status": "active"
      },
      {
        "name": "vm2",
        "status": "active"
      }
    ]
  }
}

Or another example: the NOT-Filter: let's assume that I want to filter for all VMs that are not active:

{
  virtual_machine_list(filters: {NOT: {status: "active"}}) {
    name
    status
  }
}

But for some reason, this only returns active VMs:

{
  "data": {
    "virtual_machine_list": [
      {
        "name": "vm1",
        "status": "active"
      },
      {
        "name": "vm2",
        "status": "active"
      }
    ]
  }
}

Expected Behavior

That the OR-Filter returns active and staged VMs and that the NOT-Filter returns everything except active VMs.

Observed Behavior

Described above.

jeffgdotorg commented 3 months ago

I'm able to reproduce this problem on a fresh 4.0.0 install with two VMs with status "active" and two VMs with status "staged". My level of GraphQL sophistication is currently low, so I might have missed something subtle.

jeffgdotorg commented 3 months ago

Thanks for reporting a problem you've encountered in NetBox. My role in this moment is limited to triage, so you may get questions from the developers who understand these things more deeply.

jeremystretch commented 3 months ago

I'm not sure this filter logic is meant to be supported currently. I know we've had to do some extensive customization with the GraphQL filters to support django-filters integration. Maybe @arthanson can weigh in?

NetaliDev commented 3 months ago

In my opinion, we have a regression here if the new filter logic is not yet supported, because the support for the __n suffix for filters has been dropped in 4.0 and with the NOT filter not working, I currently don't see a way to filter for example all VMs that are not active (like the scenario described in the original bug report).

arthanson commented 3 months ago

Dug into this, the issue here is Strawberry requires Q objects to be returned from the filter functions in order to do the AND, OR, NOT operations, but when we call django-filter we get back a queryset. I don't see any built-in way to get a Q object from django-filter. It should be possible to basically duplicate some of what django-filter is doing and take a list of query-params and create a Q object from them. That should be basically what django-filter is doing internally before it uses them to generate the queryset.

NetaliDev commented 3 months ago

What is the status of this now? I would like to emphasize once again that this is a regression, as it is now no longer possible to use NOT filters, which in my view restricts the GraphQL API quite a lot.

jeremystretch commented 3 months ago

What is the status of this now?

screenshot

As you can see from the assigned labels, this issue needs an owner. @NetaliDev would you like to volunteer to own this issue?

NetaliDev commented 3 months ago

Sorry, but I don't know enough about GraphQL, Strawberry and especially the netbox codebase to fix this. I just had the task to migrate our GraphQL queries at work to netbox 4.X and stumbled over this issue.

arthanson commented 2 months ago

Note: Make sure to test below syntax as well:

{
  interface_list(filters: {device: ["fceos21", "ceos1"]}) {
    name
  }
}
OliElli commented 1 month ago

+1 here, I just found that the NOT filter doesn't work in 4.0.7

Query that works in 3.x: {device_list (status__n: "active", tenant: "tenant-name-here") {name status}} Query that does NOT work in Netbox 4.x: {device_list(filters: {tenant: "tenant-name-here", NOT: {status: "active"}}) {status name}}

Because the AND and OR args are also broken I can't even workaround by selecting all statuses but the one I want

arthanson commented 3 weeks ago

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 fair bit of work. The solutions I see:

  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.

  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(...)

  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.

First step need to get rid of Strawberry Legacy Filtering (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).