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

9856 Replace graphene with Strawberry #15141

Merged
merged 107 commits into from
Mar 22, 2024
Merged

9856 Replace graphene with Strawberry #15141

merged 107 commits into from
Mar 22, 2024

Conversation

arthanson
Copy link
Collaborator

@arthanson arthanson commented Feb 13, 2024

Fixes: #9856

Replaces django-graphene with strawberry-django a new engine for handling GraphQL queries. Notable changes:

  • Converts all GraphQL types to Strawberry Types
  • Includes much more updated GraphiQL browser
  • Decorator to auto-generate Strawberry required data-classes for filters as Strawberry doesn't support django-filter currently.

Example Queries:

{
  circuit_termination_list(filters: {cabled: false}) {
    id
    display
    cable {
      id
    }
  }
}

and

{
  location_list {
    image_attachments {
      id
    }
    contacts {
      id
    }
    changelog {
      id
    }
    display
    class_type
    tags {
      id
    }
    custom_fields
    id
    created
    last_updated
    custom_field_data
    name
    slug
    description
    site {
      id
    }
    status
    tenant {
      id
    }
    lft
    rght
    tree_id
    level
    children {
      id
    }
    powerpanel_set {
      id
    }
    cabletermination_set {
      id
    }
    racks {
      id
    }
    vlan_groups {
      id
    }
    parent {
      id
    }
    devices {
      id
    }
  }
}

Filtering lookups are 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
  }
}

Performance optimizer

Below is a ridiculous nested query that can be used to show optimization

old graphene: 55 queries
strawberry: 24 queries

{
  location_list {
    image_attachments {
      id
    }
    contacts {
      id
    }
    changelog {
      id
    }
    display
    class_type
    tags {
      id
    }
    custom_fields
    id
    created
    last_updated
    custom_field_data
    name
    slug
    description
    site {
      id
      name
      locations {
        id
        name
      }
    }
    status
    tenant {
      id
      sites {
        id
        name
      }
    }
    lft
    rght
    tree_id
    level
    children {
      id
    }
    powerpanel_set {
      id
    }
    cabletermination_set {
      id
    }
    racks {
      id
      site {
        id
        name
      }
    }
    vlan_groups {
      id
    }
    parent {
      id
    }
    devices {
      id
    }
  }
}

@arthanson arthanson changed the base branch from develop to feature February 13, 2024 16:35
@arthanson
Copy link
Collaborator Author

arthanson commented Feb 13, 2024

Remaining work:

  • fix type definitions
  • fix type data types
  • fix filtersets
  • Auto generate Filtersets definitions
  • Choice field handling
  • Fix GraphQL tests auto-discovery of fields
  • Update netbox/graphql/views.py GraphQLView (GraphiQL)
  • netbox/netbox/graphql/fields.py (ObjectField, ObjectListfield)? - I think this is obsolete from optimizer
  • netbox/netbox/graphql/scalars.py (BigInt)?
  • netbox/netbox/graphql/utils.py (get_graphene_type)?
  • netbox/dcim/graphql/gfk_mixins.py (gfk mixins)?

@jeremystretch jeremystretch added this to the v4.0 milestone Feb 14, 2024
@arthanson arthanson marked this pull request as ready for review March 13, 2024 22:35
@arthanson arthanson changed the title DRAFT: 9856 Replace graphene with Strawberry 9856 Replace graphene with Strawberry Mar 13, 2024
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.

Excellent work! Noticed a few minor cleanup items on initial review and had some general questions.

base_requirements.txt Outdated Show resolved Hide resolved
netbox/circuits/graphql/schema.py Outdated Show resolved Hide resolved
netbox/circuits/graphql/types.py Show resolved Hide resolved
netbox/netbox/graphql/types.py Show resolved Hide resolved
netbox/utilities/testing/api.py Outdated Show resolved Hide resolved
netbox/netbox/graphql/filter_mixins.py Outdated Show resolved Hide resolved
netbox/netbox/graphql/filter_mixins.py Outdated Show resolved Hide resolved
netbox/tenancy/graphql/types.py Outdated Show resolved Hide resolved
netbox/users/graphql/types.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
base_requirements.txt Outdated Show resolved Hide resolved
netbox/circuits/graphql/types.py Show resolved Hide resolved
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.

The GraphiQL view isn't using our custom template, and static resources are being pulled from unpkg.com rather than being served locally.

Also, it looks like maybe we should swap django-graphiql-debug-toolbar for django-graphiql-strawberry-debug-toolbar, although AFAICT the current middleware seems functional.

@arthanson
Copy link
Collaborator Author

Fixed up the GraphiQL view, the old method of bundling the assets wouldn't work as the new GraphiQL has some include packages that ESBuild can't translate to ES6, see: (graphql/graphiql#2525) - we could switch to webpack which does handle it, but as it is just for GraphiQL it seemed fine to just statically link the separate files. Changed the ESBuild script to copy the files out of the node-packages.

django-graphical-debug-toolbar is no longer needed, so removed that. The strawberry one is handled by middleware already.

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.

Almost there!

netbox/netbox/settings.py Outdated Show resolved Hide resolved
netbox/project-static/bundle.js Outdated Show resolved Hide resolved
netbox/project-static/bundle.js Outdated Show resolved Hide resolved
netbox/project-static/bundle.js Outdated Show resolved Hide resolved
@jeremystretch jeremystretch merged commit 45c99e4 into feature Mar 22, 2024
6 checks passed
@jeremystretch jeremystretch deleted the 9856-strawberry-2 branch March 22, 2024 16:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2024
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.

Replace graphene-django with Strawberry
2 participants