Skip to content

Conversation

@jeremystretch
Copy link
Member

@jeremystretch jeremystretch commented Feb 21, 2025

Closes: #7598

This PR serves to capture @jeremypng's recent work on improving the GraphQL API. Although this work primarily targets FR #7598, it includes many foundational improvements:

  • Discontinuing the use of deprecated filters (USE_DEPRECATED_FILTERS = False)
  • Defining standalone filters for GraphQL
  • Reusable mixin classes for filters
  • Adds the as_enum() method to ChoiceSet for use in GraphQL filters

jeremypng and others added 6 commits February 19, 2025 10:11
* Add as_num() method to ChoiceSet

* Update circuits enums

* Update DCIM enums

* Update extras enums

* Update IPAM enums

* Add tenancy enums

* Update virtualization enums

* Update VPN enums

* Update wireless enums

* Update netbox enums

* Permit TenantType.group to be null
@jeremystretch jeremystretch added this to the v4.3 milestone Feb 21, 2025
@jeremystretch
Copy link
Member Author

We still need to update the documentation to cover:

  • The new filtering syntax for GraphQL queries
  • Developer instructions for adding & updating GraphQL filters

@jeremystretch
Copy link
Member Author

@jeremypng
Copy link

jeremypng commented Feb 27, 2025

We have continued using this graphql filtering system on 4.0.11 and have reduced nested queries that were taking 36s with pynetbox and the DRF API to 400ms with graphql.

The additional changes you have made look good.

upstream_speed: Annotated['IntegerLookup', strawberry.lazy('netbox.graphql.filter_lookups')] | None = (
strawberry_django.filter_field()
)
xconnect_id: FilterLookup[str] | None = strawberry_django.filter_field()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremypng do you know whether it's necessary to define these filters explicitly? Setting them to auto seems to yield the same schema, but I wanted to check if you had run into any problems with this approach.

Suggested change
xconnect_id: FilterLookup[str] | None = strawberry_django.filter_field()
xconnect_id: strawberry.auto

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can successfully infer the filter type from the field type it will work. From what I saw, this generally works for model fields that are physical database fields but not computed fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thanks!

@arthanson
Copy link
Collaborator

arthanson commented Mar 6, 2025

Adding a couple sample queries here for anyone testing to play with:

{
  site_list(
    filters: {status: STATUS_PLANNED, OR: {tenant: {name: {exact: "Foo"}}}}
  ) {
    name
  }
}
{
  ip_address_list(
    filters: {parent:["172.16.0.1"]}
  ) {
    address
    status
  }
}

arthanson
arthanson previously approved these changes Mar 7, 2025
Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple small suggestions, but it could go in even without those.

@arthanson arthanson merged commit c35f5f8 into feature Mar 7, 2025
6 checks passed
@jeremystretch jeremystretch deleted the 7598-graphql-custom-fields branch March 10, 2025 13:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants