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

WIP: GraphQL 4 #266

Merged
merged 107 commits into from
Oct 7, 2020
Merged

Conversation

unclecheese
Copy link

@unclecheese unclecheese commented Jul 13, 2020

GraphQL v4: Let's scale this thing

Note: This document will continue to be updated as changes evolve. It is not a comprehensive view of everything that will be included in v4, but rather an overview of where we want to be and how things are going.

This pull request represents a large body of changes to the GraphQL module that enhance its performance and developer experience on a very fundamental level. It is a complete rewrite of the entire schema-facing APIs including scaffolding, type creators, middleware, and more.

High-level goals/changes

This section describes the overarching intention of the changes. For information on upgrading, see the "Upgrading" section below.

Drastically improved performance

Some rough testing showed plenty of merit to the chosen approach:

No. Types/QueriesTime to response (current)Time to response (proposed)
10/20~3.5s~120ms
20/40~8s~120ms
50/100~25s~150ms
250/500~25s~150ms
  • Vagrant stack, no major performance optimisations such as opcode caching

While there appears to be some variance in the results, it may very well be possible that the response times do not scale with the number of types queries -- in fact, due to the new lazy loading approach, we would expect this to be the case. That the response times seem to increase with types could just be noise in the averaging of response times that varied quite a bit.

tl;dr this needs more testing, but we know we're on the right path.

No more dynamic schema generation

Generating the schema at runtime was the biggest performance bottleneck for this module and the primary reason why it could not be scaled out for heavy production use. This could not go on.

Instead of a dynamically generated schema, we build one in a new task, e.g. /dev/graphql/build with code generation. This will be a major change to the developer experience, but we believe the tradeoffs are worth it. It means that any additions of fields or changes to types, resolvers, etc., will require a separate build. It's possible that we could bundle this with dev/build, however it should be noted that while the two tasks are often interdependent, there are many cases where you need one and not the other. At the moment, the build schema task appears to be many times faster than dev/build.

Manager has been removed broken up into multiple concerns

The Manager class was becoming a catch-all that handled registration of types, execution of scaffolding, running queries/ middleware, error handling, and more. This has been broken up into a few new classes, primarily:

  • SchemaBuilder <-- the new config namespace for your schema
  • QueryHandlerInterface

3: Much more generous configuration API

In GraphQL 4, we'll aim to build schemas with config by default, and code will fill in the gaps for parts of the schema that can only be computed at build time. A thorough review of the current "TypeCreator" APIs, with the exception of their resolvers, are just value objects that map to graphql-php primitives. This can be done declaratively.

4: Resolvers use a new discovery pattern

Types can be created declaratively, but what about resolvers? The key to caching the schema has always been moving to static resolvers rather than context-aware class members. To solve this problem, we'll use a ResolverRegistry class where the developer can register any number of ResolverProvider implementations. Those implementations must provide logic on how to locate the static method given a type name and field name. Below is a simple resolver provider that ships with the module:

    public static function getResolverMethod(?string $typeName = null, ?string $fieldName = null): ?string
    {
        $candidates = array_filter([
            // resolveMyTypeMyField()
            $typeName && $fieldName ?
                sprintf('resolve%s%s', ucfirst($typeName), ucfirst($fieldName)) :
                null,
            // resolveMyType()
            $typeName ? sprintf('resolve%s', ucfirst($typeName)) : null,
            // resolveMyField()
            $fieldName ? sprintf('resolve%s', ucfirst($fieldName)) : null,
            // resolve()
            'resolve',
        ]);

        foreach ($candidates as $method) {
            $callable = [static::class, $method];
            $isCallable = is_callable($callable, false);
            if ($isCallable) {
                return $method;
            }
        }

        return null;
    }

A type named Product with a field named Price will look in the resolver registry in this order:

  • resolveProductPrice
  • resolveProduct
  • resolvePrice
  • resolve

An important note here is that this computation is not happening at query time. It solely exists to determine what static method will be added to the generated schema PHP code.

Non-dataobject types are just... config

With resolvers relying on their own discovery logic, writing new types becomes exceedingly simple:

types:
  Product:
    fields:
      price: Int!
      reviews: '[ProductReview]'
      image(Size: ImageSize!): Image

Notice the ability to inline arguments.

Goodbye, scaffolders, hello models

This version decouples DataObjects from the module and abstracts them away into a concept called "models." A model is any class that can express awareness of its schema. It should be able to answer questions like:

  • Do you have field X?
  • What type is field Y?
  • What are all the fields you offer?
  • What operations do you provide?
  • Do you require any extra types to be added to the schema?

By default, we ship a DataObject implementation of a model. The configuration looks like this:

models:
  MyDataObjectType:
    class: My\DataObject
    fields:
      # infer type
      title: true
      # explicit type
      myGetter: String
      # add an arg!
      date(Format: String): true
    operations:
      read: true
      update:
        exclude: sensitiveField

Note that dataobjects are no longer one-to-one with their types. You can have one dataobject map to multiple types.

Resolver contexts

For scaffolded operations, context is critical to the resolvers. They need to know what class to get in the ORM, for instance. For this, you can use resolver context. Fields accept resolver context and pass it as the argument to the resolver function.

public static function resolve(array $context)
{
	
	return static function ($obj, $args = [])
	{
		return DataList::create($context['dataClass']);
	}
}

Enums are first-class citizens

You can now define enums as plain config:

      enums:
        SortDirection:
          values:
            - ASC
            - DESC
        ProductStatus:
          values:
            - PENDING
            - CANCELLED
            - SHIPPED

Lowercase field names are encouraged and supported

The current pattern of mapping field names to their UpperCase counterparts in the ORM is a bit unconventional. If you express them in lowercase, they'll just work.

New GraphQL developer tools

The developer API for GraphQL has expanded quite a bit in the lifetime of this module, and with these new changes, we'll now have three dev tools all with their own endpoints:

  • GraphQL IDE (graphiql)
  • Show schema (print the schema in type language)
  • Build schema
  • Debug resolvers (show all types and where their fields resolve)

In light of this, it makes sense to give these tools their own namespace in dev/graphql. We'll have:

  • dev/graphql/ide
  • dev/graphql/schema
  • dev/graphql/build
  • dev/graphql/resolvers

New performance middleware

Since the primary focus of this version is performance improvements, a performance middleware will ship by default, enabled in non-live modes:

query {
  myQuery {

  }
  __perf {
   schema # time it took to load the schema
   query # query execution time
   middleware # middleware execution time
  }

Upgrade to graphql-php ^14.0

The new lazy loading of types makes this all possible.

Upgrading

Again, this is a work in progress, and should mostly be considered a live document that's getting updated as APIs change:

  • All config on SilverStripe\GraphQL\Manager should be migrated to SilverStripe\GraphQL\Schema\SchemaBuilder
  • SilverStripe\GraphQL\Controller implementations should be passed a SchemaBuilder instance rather than Manager.
  • scaffolding.types config should be moved to models
  • scaffolding.queries and scaffolding.mutations should be moved to queries and mutations at the top level of SchemaBuilder config.
  • Move all your custom resolvers to a ResolverProvider implementation. Recommend subclassing the abstract DefaultResolverProvider class and writing resolve{TypeName}{FieldName} methods as described above.
  • Anything in *Creator classes should be migrated to config. The resolver should be made static and moved into a ResolverProvider implementation.

Installing the WIP

Use our test project.
Or extract the relevant bits from composer.json into your own project.
Then run a sake dev/build, followed by a sake dev/tasks/build-schema.
Read the draft docs at https://pulls-master-schemageddon--ss-docs.netlify.app/en/4/developer_guides/graphql/

See also

Remaining todos:

Critical for merge

  • Documentation
  • Tests
  • Rule out security regressions
    • CSRF
    • Limit/canView check
    • Others...?
  • Upgrade guide

Nice to haves, post merge

  • DataObject wildcarding (e.g. expose all classes in one or many partial match namespaces)
  • Default args for DBFields (e.g. image sizing, date formatting, text truncation)

Compatibility

  • asset-admin (70% done)
  • versioned (90% done)
  • versioned-admin
  • snapshot-admin
  • elemental editor
  • silverstripe-gatsby
  • silverstripe-graphql-forms
  • framework test
  • cms

@unclecheese unclecheese marked this pull request as draft July 13, 2020 21:55
@unclecheese unclecheese mentioned this pull request Jul 13, 2020
3 tasks
Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

Slooowly making my way through this epic journey. I'll paste feedback in increments to avoid losing them to the browser gods at some point.

In light of this, it makes sense to give these tools their own namespace in dev/graphql.

That's not actually in the PR yet, right? Maybe start a public todo list to make it easier to review?

src/Manager.php Outdated Show resolved Hide resolved
src/Manager.php Outdated Show resolved Hide resolved
src/Dev/Benchmark.php Show resolved Hide resolved
@chillu
Copy link
Member

chillu commented Aug 13, 2020

Here's how to get this going for local testing (on silverstripe/installer:4.x-dev):

composer config repositories.graphql vcs https://github.com/unclecheese/silverstripe-graphql.git
composer config repositories.admin vcs https://github.com/unclecheese/silverstripe-admin.git
composer config repositories.asset-admin vcs https://github.com/unclecheese/silverstripe-asset-admin.git
composer config repositories.versioned vcs https://github.com/unclecheese/silverstripe-versioned.git
composer config repositories.frameworktest vcs https://github.com/unclecheese/silverstripe-frameworktest.git

composer require "silverstripe/graphql":"dev-pulls/master/schemageddon as 3.x-dev" "silverstripe/versioned":"dev-pulls/1/schemageddon as 1.x-dev" "silverstripe/admin":"dev-pulls/1/schemageddon as 1.x-dev" "silverstripe/asset-admin":"dev-pulls/1/schemageddon as 1.x-dev" "silverstripe/frameworktest":"dev-pulls/4/schemageddon as 4.x-dev" "silverstripe/registry":"~2" 

Then run a sake dev/build, followed by a sake dev/tasks/build-schema

chillu and others added 3 commits August 13, 2020 21:33
Required for dev/tasks/schema-build which will loop over multiple schemas by default.
@sminnee
Copy link
Member

sminnee commented Sep 21, 2020

Subclassing DevelopmentAdmin is only okay for a PoC IMO, unless we can guarantee that this is the only module in existence that subclasses it. Otherwise any other module that subclasses it will either

  • require this module
  • conflict with this module

Adding an additional extensibility feature to DevelopmentAdmin is probably a better approach

EDIT: oops, it's not an injector replacement so my biggest concern goes away.

@blueo
Copy link
Contributor

blueo commented Sep 28, 2020

I'm testing out a new composer install of 4.6 with this module setup and just noting down some things as I go:

  • asset upload, publishing, creating a folder, upload field just works ™️

  • moving a file by drag and drop throws:
    image

  • history viewer on a page does not load with the following graphql error:

[Emergency] Uncaught TypeError: Argument 1 passed to GraphQL\Type\Definition\Type::getNullableType() must be an instance of GraphQL\Type\Definition\Type, null given, called in /var/www/mysite/www/vendor/webonyx/graphql-php/src/Validator/Rules/ValuesOfCorrectType.php on line 75

@robbieaverill
Copy link
Contributor

Ironic error message

@unclecheese
Copy link
Author

Yup, all the modules are currently "render only". Long way to go, but the work is actually really straightforward in what I've seen so far. It's like a hackday or two.

@unclecheese
Copy link
Author

Thank you for tracking that, though. One less thing to test when we finish the asset admin upgrade. 😁

@chillu
Copy link
Member

chillu commented Oct 7, 2020

Talked to Aaron, keeping this PR open is starting to obstruct collaboration and further review. I think we've taken it far enough to prove that the API as it is designed has legs, and that the core premise (make it faster) is satisfied. So while the tests are failing, and there's still lots of work left, this can happen as follow on work in the master branch. Aaron is documenting this follow on work in tickets. MERGING! 🦄👯‍♂️🎉🌤

@chillu chillu merged commit 1c4e623 into silverstripe:master Oct 7, 2020
@tractorcow
Copy link
Contributor

Thank you for making my sites faster. Hope it works in practice!

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

Successfully merging this pull request may close these issues.