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

Replace graphene-django with Strawberry #9856

Closed
jeremystretch opened this issue Jul 27, 2022 · 7 comments · Fixed by #15141
Closed

Replace graphene-django with Strawberry #9856

jeremystretch opened this issue Jul 27, 2022 · 7 comments · Fixed by #15141
Assignees
Labels
breaking change This change modifies or removes some previously documented functionality status: accepted This issue has been accepted for implementation topic: GraphQL type: feature Introduction of new functionality to the application
Milestone

Comments

@jeremystretch
Copy link
Member

jeremystretch commented Jul 27, 2022

NetBox version

v3.2.7

Feature type

Change to existing functionality

Proposed functionality

Replace NetBox's existing GraphQL implementation, which was built using graphene-django, with Strawberry. graphene-django unfortunately is no longer being maintained, and is no longer compatible with Django 4.0 and later releases (something which we currently work around via a monkey patch).

Use case

The goal here will be to keep the new GraphQL API as backward-compatible as possible, and to leverage any new features of Strawberry where they might be beneficial. Further research is needed to determine the migration strategy.

Database changes

No changes to the database schema are anticipated

External dependencies

Replace graphene-django with strawberry

@jeremystretch jeremystretch added type: feature Introduction of new functionality to the application status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Jul 27, 2022
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Aug 22, 2022
@jeremystretch jeremystretch added this to the v3.4 milestone Aug 22, 2022
@arthanson arthanson self-assigned this Sep 7, 2022
arthanson added a commit that referenced this issue Sep 8, 2022
arthanson added a commit that referenced this issue Sep 8, 2022
@jeremystretch
Copy link
Member Author

jeremystretch commented Sep 27, 2022

Given the recent release of graphene-django 3.0, we've decided to put on hold the exploration of alternative GraphQL API implementations and proceed with upgrading. (#10472 has been opened to track this work.) I'm going to close this issue with the assumption that moving to graphene-django 3.0 is tenable. If we discover otherwise, this will need to be reopened.

@jeremystretch jeremystretch closed this as not planned Won't fix, can't repro, duplicate, stale Sep 27, 2022
@jeremystretch jeremystretch removed the status: accepted This issue has been accepted for implementation label Sep 27, 2022
@jeremystretch jeremystretch removed this from the v3.4 milestone Sep 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2022
@arthanson
Copy link
Collaborator

Reopening issue - Graphene Django is still pretty much dead. strawberry-django meanwhile has quite a bit of activity, they have also released https://github.com/blb-ventures/strawberry-django-plus which makes it potentially more viable.

@arthanson arthanson reopened this Mar 30, 2023
@netbox-community netbox-community unlocked this conversation Mar 30, 2023
@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Mar 30, 2023
@arthanson arthanson removed their assignment Apr 24, 2023
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels May 2, 2023
@jeremystretch jeremystretch added this to the v4.0 milestone May 2, 2023
@arthanson
Copy link
Collaborator

arthanson commented Aug 25, 2023

Investigating this, one issue is that the filtering syntax is different between the two libraries.

Graphene

user_list(is_superuser: true) {
    id
}

Strawberry

user_list(filters: {is_superuser: true}) {
   id
}

The filtering lookup syntax (greater than, less than, case-insensitive compare) is also different:
Graphene

{
  user_list(username__ic: "bob") {
    username
  }
}

Strawberry

{
  user_list(filters: {username: {i_contains: "bob"}}) {
    username
  }
}

For stawberry, allowing lookups on fields forces the lookup to be present, so it forces this somewhat awkward syntax:
Graphene

{
  user_list(id: "3") {
    id
  }
}

Strawberry

{
  user_list(filters: {id: {exact: 3}}) {
    id
  }
}

@ryanmerolle
Copy link
Contributor

I think it's a reality of switch and frankly it's not that crazy.

@arthanson
Copy link
Collaborator

arthanson commented Jan 17, 2024

After further research some issues came up with strawberry. Here are some pros, cons and more detailed explanation for discussion:

Note: If you do actively use GraphQL then please take a look at this and make comments as it will effect your code...

Pros (of using strawberry):

  • it is actively maintained
  • It has a queryset optimizer built-in so reduces n+1 queries when referencing foreign-key fields
  • GraphiQL is up-to-date

Cons:

  • Graphene works currently with bugs but nothing major
  • strawberry doesn't support django-filters (see below)
  • Not stable, pre-1.0 they make a LOT of releases and still have breaking changes fairly often

The major downside of strawberry is they don't support django-filters. They used to, but ripped it out without almost any warning on a prior release when they refactored to use data-classes. This has a couple side-effects:

  1. All filter fields have to be re-defined in the GraphQL code
  2. custom filters that we define have to have a custom wrapper in the GraphQL code to call into the filterset
  3. Lookup expressions will be different

More detail / examples

  1. Each field has to be defined on each filter, inheritance isn't supported for field definitions, so a filter example would be:
@strawberry_django.filter(models.Circuit, lookups=True)
class CircuitFilter(filtersets.CircuitFilterSet, filters.NetBoxModelFilter):
    # NetBoxModelFilterSet
    q: str | None
    # tag:
    # ChangeLoggedModelFilterSet
    created: auto
    last_updated: auto
    created_by_request: str | None
    updated_by_request: str | None
    modified_by_request: str | None

    id: auto
    cid: auto
    description: auto
    install_date: auto
    termination_date: auto
    commit_rate: auto
    provider_id: auto
    provider: auto
    ...

So the top part with the comment #NetBoxModelFilterSet would need to be on each filter definition. Type definitions (CircuitType) do support field definition inheritance so the duplication of field definitions is only needed on the filters. Note: The inheriting from the filtersets is a bit of a hack to support calling of the custom filter functions (see below).

  1. The custom filters have to have a custom wrapper, but the functional implementation for the filters do support inheritance, so for example the custom filters for created_by_request is defined in a common subclass that calls into the filterset methods:
class ChangeLoggedModelFilter:

    def created_by_request(self, queryset):
        return self.filter_by_request(queryset, "created_by_request", self.created_by_request)

    def updated_by_request(self, queryset):
        return self.filter_by_request(queryset, "updated_by_request", self.updated_by_request)

    def modified_by_request(self, queryset):
        return self.filter_by_request(queryset, "modified_by_request", self.modified_by_request)
  1. Lookups (greater-than, less-than, etc... ) are different, on the old they are added by the custom routines in base FilterSet classes, for strawberry they have their own implementation which is a slightly different set. Old:
id__n: [String]
id__lte: [String]
id__lt: [String]
id__gte: [String]
id__gt: [String]
id__empty: Boolean
cid__n: [String]
cid__ic: [String]
cid__nic: [String]
cid__iew: [String]
cid__niew: [String]
cid__isw: [String]
cid__nisw: [String]
cid__ie: [String]
cid__nie: [String]
cid__empty: Boolean

strawberry:

exact: ID
i_exact: ID
contains: ID
i_contains: ID
in_list: [ID!]
gt: ID
gte: ID
lt: ID
lte: ID
starts_with: ID
i_starts_with: ID
ends_with: ID
i_ends_with: ID
range: [ID!]
is_null: Boolean
regex: String
i_regex: String

There is no not operators (cid_n) as not is a graphql operator so you could query with {not {cid: ... which should get the same result.

@DanSheps
Copy link
Member

DanSheps commented Jan 25, 2024

strawberry-graphql-django - Issue for adding support for django_filters: strawberry-graphql/strawberry-django#448
strawberry-graphql-django - Issue for filtering overhaul: strawberry-graphql/strawberry-django#399

@jeremystretch
Copy link
Member Author

Very happy to announce this has been completed for the upcoming v4.0 release! Thank you @arthanson!

@jeremystretch jeremystretch added the breaking change This change modifies or removes some previously documented functionality label May 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This change modifies or removes some previously documented functionality status: accepted This issue has been accepted for implementation topic: GraphQL type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants