Keystone: [RFC] GraphQL Provider Framework

[timleslie]

Background

One of the core responsibilities of Keystone is to generate a GraphQL API. In the common use case, this API consists of CRUD operations over a collection of lists. It also provides authentication mutations, custom types/queries/mutations, as well as an application version query.

The current implementation of each of these features is tightly woven into the core of Keystone. This tight coupling is a problem, as it blurs the lines between each of the concerns, as well as making it hard to introduce new features into the GraphQL API.

Proposal

I would like to propose the introduction of a GraphQL Provider Framework. While this sounds big and impressive, it can be easily summarised by the following class API definition:

class Provider {
  constructor() {}

  getTypes({ schemaName }) {
    return [];
  }
  getQueries({ schemaName }) {
    return [];
  }
  getMutations({ schemaName }) {
    return [];
  }

  getTypeResolvers({ schemaName }) {
    return {};
  }
  getQueryResolvers({ schemaName }) {
    return {};
  }
  getMutationResolvers({ schemaName }) {
    return {};
  }
}

Each feature of Keystone which generates graphQL API surface area would be encapsulated in a Provider class, which has methods for generating the type definitions and resolvers required by Apollo.

Keystone would keep track of an array of providers, and iterate over these to generate the combined set of type definitions and resolvers which make up the complete graphQL API.

Discussion

How would this impact the current implementation?

The code which currently lives in Keystone.getAdminSchema() and Keystone.getTypeDefs() would be greatly simplified to iteration over the list of providers. All the functionality would be shifted into four separate providers:

• ListCRUDProvider
• ListAuthProvider
• AppVersionProvider
• CustomProvider

How would I add my own Provider to Keystone?

A system writer would be able to implement their own Provider class and then call Keystone.addProvider({ provider }).

Why don't we just have getTypedefs and getResolvers as the Provider API?

The way Keystone currently combines the different pieces of the graphQL API means that the types, queries and mutations all need to be handled individually. We could definitely write some kind of BaseProvider class which had getTypedefs() and getResolvers() and leveraged the other methods to generate valid results which could be passed directly into ApolloServer. I'm 50/50 on whether we want to make this the default or not.

What benefits to we expect to get out of this?

• The existing code will be better organised. For example, the AppVersionProvider functionality will be able to live in one place in a single module, rather than being spread throughout Keystone/index.js.
• Separation of concerns will be more obvious. For example, the auth aspects of Keystone are actually quite different to the CRUD aspects, however in order to generate our graphQL API these are currently somewhat conflated.
• It will help us think about our Keystone APIs as a composition of individual providers, rather than a big mish-mash of misc things.
• It will provide an alternate way for developers to integrate new pieces of API into the system beyond the existing custom type/query/mutation API.
• It moves us closer to the goal of being able to support CRUD APIs across multiple data sources.
• It gives us a new way of thinking about the Admin UI and what it does and does not support.
• ...probably a lot of other things I haven't considered 🤷‍♂

What could possibly go wrong?

I've prototyped out an implementation of this API (#2420) and have verified that this API is sufficient to match the existing functionality. Also, the refactor does not appear to introduce any complexity beyond what already exists. I'm rather confident that this is a good design, but I'm open to questions or concerns that I may have overlooked.

How will all this get implemented.

I will implement a series of PRs, each of which introduces a single piece of the functionality required to fully convert to this API. This will hopefully make it easier to review each individual PR and also provide us with some checkpoints to identify any bugs are design issues which arise during the conversion process.

[jesstelford]

Love it! The core has evolved over time into what it is today. Bringing a proper architecture to it will be a big win, and I'm looking forward to that code being "just iterating" 👍

Thoughts

The separation of type{resolver}, query{resolver}, and mutation{resolver} makes sense for the main case of lists & versions. However, when we look at authentication, we see it also provides two other things: GraphQL Context and Express Middleware.

I'd like to propose we extend the definition of a "Provider" with a couple more methods:

class Provider {
  constructor() {}
  // ...

  getContext({ context }) {
    return context;
  }

  prepareApp({ app }) {
    // Setup the Express app with middlware, etc
    // no return necessary
  }
}

We can then iterate over the providers when preparing the middlewares and graphQL context too.

Note I haven't given any consideration to how providers are communicated across to, say, the GraphQLApp from Keystone core, etc.

Future enhancements

@mitchellhamilton brought up the npm lib merge-graphql-schemas which could be used to replace our "just iterating" part. I've used this on a project in the past and it works quite well.

It would make implementing getTypedefs() and getResolvers() even easier, plus enables some other nice things like extending pre-existing types, etc.

[timleslie]

I'd like to propose we extend the definition of a "Provider" with a couple more methods:

class Provider {
  constructor() {}
  // ...

  getContext({ context }) {
    return context;
  }

  prepareApp({ app }) {
    // Setup the Express app with middlware, etc
    // no return necessary
  }
}

We can then iterate over the providers when preparing the middlewares and graphQL context too.

I think these are both interesting ideas. Definitely the AppVersionProvider and ListAuthProvider are concerned with the context and/or middleware stacks. I'm not sure that we necessarily want to include that functionality within the Provider class itself, or whether to make that functionality part of a higher level structure which can compose (perhaps) a Provider, a ContextManager and a MiddlewareProvider (or something along those lines).

I propose we defer adding these methods to the Provider API until the core functionality is implemented and we can fully assess how each Provider couples to context and middleware and what the best way to support that might be.

[timleslie]

@mitchellhamilton brought up the npm lib merge-graphql-schemas which could be used to replace our "just iterating" part. I've used this on a project in the past and it works quite well.

It would make implementing getTypedefs() and getResolvers() even easier, plus enables some other nice things like extending pre-existing types, etc.

I think this is definitely an interesting direction to head in. One of the considerations around the current API is that I want to be able to make the changes incrementally, which necessitates following the iteration pattern until the refactoring is complete. I think that once we have finished refactoring the existing functionality we will be in a position to consider taking advantage of libraries like merge-graphql-schemas. I'm wary of explicitly making the use of that library a goal, but I think it's definitely a good direction for us to head in and I think that once we get closer we'll be better placed to make decisions on whether that, or something similar, will provide us with improved functionality.

[gautamsi]

Would this make sense to have GraphQL Subscription as part of custom provider. I know the lists don't support subscription yet but I can have custom code for subscription which I dont want to mount another graphql middleware

class Provider {
  constructor() {}
// -----
  getSubscriptions({ schemaName }) {
    return [];
  }
  getSubscriptionResolvers({ schemaName }) {
    return {};
  }
}

[VinayaSathyanarayana]

Support for Subscription should be prioritized in the roadmap

[timleslie]

Yep, this change will definitely unlock the ability to add support for Subscriptions to Keystone 👍