Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #7598: GraphQL Filter Redesign #18618

Open
wants to merge 5 commits into
base: feature
Choose a base branch
from

Conversation

jeremypng
Copy link

Fixes: #7598

GraphQL rewrite with statically defined filters for each GraphQL Filter. This moves away from the DEPRECATED_FILTERS flag and gets rid of the autotype_decorator and allows strongly typed filters, enums, and custom filters like JSON and TreeNode searches. I've tried to follow idiomatic design from a GraphQL perspective. This attempts to be feature complete from a pure data model perspective, but there is plenty of room to add more business logic filters similar to the parent filters on IPRange and IPAddress that are part of this PR. It allows you to walk the data model, but does not try to replace the full functionality of the DRF API.

New query syntax for custom fields:

query GetTenants {
  tenant_list (filters: {custom_field_data: {path: "cust_id", lookup: {string_lookup: {exact:"YKY01"}}}}) {
    id
    name
    custom_field_data
  }
}

results:

{
  "data": {
    "tenant_list": [
      {
        "id": "9",
        "name": "Nakatomi Corportation",
        "custom_field_data": {
          "cust_id": "YKY01"
        }
      }
    ]
  }

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Great work @jeremypng! I have some questions and some minor cleanup, but I'd also like @arthanson to review this as he was responsible for much of the initial Strawberry implementation.

Comment on lines +18 to +25
@strawberry.enum
class CircuitStatusEnum(Enum):
STATUS_DEPROVISIONING = 'deprovisioning'
STATUS_ACTIVE = 'active'
STATUS_PLANNED = 'planned'
STATUS_PROVISIONING = 'provisioning'
STATUS_OFFLINE = 'offline'
STATUS_DECOMMISSIONED = 'decommissioned'
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to generate these enums programmatically from the ChoiceSet subclasses NetBox already defines. That would save us quite a bit of duplication. I'm happy to tackle this work if you're in agreement with the approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I second the vote for auto-generating the enums, much easier to maintain.

Copy link
Author

Choose a reason for hiding this comment

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

I’m good with either one of us tackling it. It will make the code harder to validate in your IDE because the type checking won’t have anything to match against. But that shouldn’t affect the GraphQL schema generation which is the most important part.

Copy link
Author

Choose a reason for hiding this comment

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

What do you think about a Django command to generate the enum files from the choice modules? That keeps the type checking in place for development, but would allow them to be easily updated with each release.

netbox/circuits/graphql/filters.py Show resolved Hide resolved
netbox/core/graphql/types.py Show resolved Hide resolved
netbox/core/graphql/filter_lookups.py Outdated Show resolved Hide resolved
netbox/netbox/graphql/schema.py Outdated Show resolved Hide resolved
netbox/netbox/tests/test_graphql.py Outdated Show resolved Hide resolved
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.

@jeremypng awesome work! Thank you. Looks really good, couple minor issues - I think getting the enums auto-generated would be the biggest change.

The query works but the query { part is required, is it possible to make the query optional? Otherwise we should document it as it is a change to require it. It looks like it should be optional (see: https://graphql.org/learn/queries/#operation-type-and-name)

Only other is I think the documentation strings can be internationalized (not 100% sure, but I don't see why it wouldn't work).

netbox/netbox/graphql/enums.py Outdated Show resolved Hide resolved
netbox/netbox/graphql/schema.py Outdated Show resolved Hide resolved
netbox/circuits/graphql/filters.py Show resolved Hide resolved
Comment on lines +18 to +25
@strawberry.enum
class CircuitStatusEnum(Enum):
STATUS_DEPROVISIONING = 'deprovisioning'
STATUS_ACTIVE = 'active'
STATUS_PLANNED = 'planned'
STATUS_PROVISIONING = 'provisioning'
STATUS_OFFLINE = 'offline'
STATUS_DECOMMISSIONED = 'decommissioned'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I second the vote for auto-generating the enums, much easier to maintain.

netbox/ipam/graphql/enums.py Outdated Show resolved Hide resolved
netbox/netbox/settings.py Outdated Show resolved Hide resolved
netbox/wireless/graphql/enums.py Outdated Show resolved Hide resolved
@jeremypng
Copy link
Author

The query works but the query { part is required, is it possible to make the query optional? Otherwise we should document it as it is a change to require it. It looks like it should be optional (see: https://graphql.org/learn/queries/#operation-type-and-name)

Hmm… I can query like this:

{
    device_list {
        id
        name
    }
}

Does that work for either of you @jeremystretch, @arthanson?

@jeremystretch
Copy link
Member

Yes, that works me (omitting the query prefix) in the branch currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants