Abstraction for reusable schema @directive implementations. #640

benjamn · 2018-02-16T16:41:01Z

This PR implements a class called SchemaDirectiveVisitor that enables writing reusable implementations of any @directives that appear in a GraphQL schema written in Schema Definition Language.

Note: I would not recommend reviewing this PR commit-by-commit. Instead, just have a look at the combined changes, which are amply commented. There were a lot of detours as I figured out exactly which abstractions I needed.

By overriding one or more visit{Object,Union,...} methods, a subclass of SchemaDirectiveVisitor registers interest in certain schema types, such as GraphQLObjectType, GraphQLUnionType, etc. When SchemaDirectiveVisitor.visitSchemaDirectives is called with a GraphQLSchema object and a map of visitor subclasses, the overridden visitor methods of those subclasses are able to obtain references to any schema type objects that have @directives attached to them, enabling the visitors to inspect or modify the schema as appropriate.

For example, if a directive called @rest(url: "...") appears after a field definition, a SchemaDirectiveVisitor subclass could provide meaning to that directive by overriding the visitFieldDefinition method (which receives a GraphQLField parameter), and then the body of that visitor method could manipulate the field's resolver function to fetch data from a REST endpoint:

const typeDefs = `
type Query {
  people: [Person] @rest(url: "/api/v1/people")
}`;

const schema = makeExecutableSchema({ typeDefs });

SchemaDirectiveVisitor.visitSchemaDirectives(schema, {
  rest: class extends SchemaDirectiveVisitor {
    public visitFieldDefinition(field: GraphQLField<any, any>) {
      const { url } = this.args;
      field.resolve = () => fetch(url);
    }
  }
});

The subclass in this example is defined as an anonymous class expression, for brevity. A truly reusable SchemaDirectiveVisitor would most likely be defined in a library using a named class declaration, and then exported for consumption by other modules and packages.

See my comments in the code below for more explanation of how this all works.

Next steps:

Update the schema directive docs to describe this new abstraction (not just directiveResolvers).
Comment on issues in this repo related to schema directives.
Add more examples of specific use cases to the tests, e.g. Support input field directives #638 (comment).
Work with @freiksenet to align our schema-wrangling efforts.
Write a blog post about this functionality.

benjamn · 2018-02-16T16:47:40Z

src/schemaVisitor.ts

+  public visitInputObject(object: GraphQLInputObjectType) {}
+  public visitInputFieldDefinition(field: GraphQLInputField, details: {
+    objectType: GraphQLInputObjectType,
+  }) {}


Much of the complexity of this implementation was necessary so that these visit* methods could have meaningful names (compared to just visit) and concrete parameter types, rather than a generic type like GraphQLNamedType (which would not even be generic enough for GraphQLField<any, any> or GraphQLEnumValue). This makes writing SchemaDirectiveVisitor subclasses a breeze in an editor with TypeScript auto-completion, yet also works just fine if you're consuming the SchemaDirectiveVisitor class in pure JavaScript.

benjamn · 2018-02-16T16:51:22Z

src/schemaVisitor.ts

+  visitorSelector: (
+    type: VisitableSchemaType,
+    methodName: string,
+  ) => SchemaVisitor[],


After digesting the comment above this visitorSelector parameter, I would suggest checking out this very simple visitorSelector implementation and then having a look at the more powerful visitorSelector used by SchemaDirectiveVisitor.visitSchemaDirectives.

benjamn · 2018-02-16T16:54:50Z

src/schemaVisitor.ts

+          args = getArgumentValues(decl, directiveNode);
+        } else {
+          // If this directive was not explicitly declared, just convert the
+          // argument nodes to their corresponding JavaScript values.


Note that the SchemaDirectiveVisitor abstraction works even for @directives that have not been declared in the schema with the syntax

directive @myDirective(param: Type = defaultValue) on OBJECT | UNION | ...

However, declaring your directives using SDL might still be a good idea, since the parameter types and default values will be enforced by this code.

Hmmmm, if I import a directive from somewhere, will its arguments/locations get enforced?

This is actually super useful info for third party packages that may not have access to the sdl!

@stubailo Yes, if a GraphQLDirective node with the same name is in the schema at the time visitSchemaDirectives happens (usually at the end of makeExecutableSchema), then it will be enforced. Regarding locations, though, we only enforce that the SchemaDirectiveVisitor actually implements appropriate visitor methods for the locations declared by the declaration. It's ok if the visitor implements additional locations not mentioned by the declaration.

benjamn · 2018-02-16T16:58:46Z

src/schemaVisitor.ts

+  | GraphQLArgument
+  | GraphQLUnionType
+  | GraphQLEnumType
+  | GraphQLEnumValue;


There was no meaningful supertype of this hodgepodge of types that SchemaVisitor can visit, hence the union.

benjamn · 2018-02-16T17:00:04Z

src/schemaGenerator.ts


+  Object.keys(directiveResolvers).forEach(directiveName => {
+    directiveVisitors[directiveName] = class extends SchemaDirectiveVisitor {


The directiveResolvers option supported by makeExecutableSchema works exactly as it did before, but is now implemented in terms of SchemaDirectiveVisitor.visitSchemaDirectives.

benjamn · 2018-02-16T17:01:40Z

src/Interfaces.ts

@@ -78,6 +82,7 @@ export interface IExecutableSchemaDefinition {
  allowUndefinedInResolve?: boolean;
  resolverValidationOptions?: IResolverValidationOptions;
  directiveResolvers?: IDirectiveResolvers<any, any>;
+  directiveVisitors?: { [name: string]: typeof SchemaDirectiveVisitor };


I wasn't sure whether folks would want to pass directiveVisitors to makeExecutableSchema, a la directiveResolvers, or just call SchemaDirectiveVisitor.visitSchemaDirectives(schema, directiveVisitors) after creating the schema, so both styles are supported.

Yeah I think we want to have it here. The way I'm imagining the most common case is something like:

// We should come up with a naming convention for these import { adminOnlyDirective } from 'graphql-tools-directive-admin-only'; // ... makeExecutableSchema({ typeDefs, resolvers, directives: [adminOnlyDirective] // could also be an object? })

I definitely do think that we shouldn't use the word "visitor" in the name of the option though, since probably most users won't be aware of the fact that the implementation is a visitor pattern (and knowing that doesn't really help)

@benjamn I think for the most part people would pass them in when creating the schema. Are there any added usecases to delaying that addition? Like create the schema, do something async, add directives?

I just thought of a potential use case here: if you're stitching schemas together, you might want to apply directives to one or both of the schemas before you merge them, or you might want to wait until after merging, so the directives get to operate on the combined schema. In the second case, you'd probably want to call visitSchemaDirectives yourself, after the merge, rather than passing directiveVisitors to makeExecutableSchema.

@stubailo I'm open to naming the option directives, but the value does have to be an object, since the keys of the object specify the names of the directives.

benjamn · 2018-02-16T17:06:13Z

src/schemaVisitor.ts

+}
+
+// Generic function for visiting GraphQLSchema objects.
+export function visitSchema(


I believe this visitSchema function is useful for any kind of schema visitor pattern, not just visiting @directive syntax. Since reinventing schema visitor abstractions seems pretty common (e.g. this PR and #527, and probably elsewhere), I'm hopeful that we can consolidate our efforts into one shared abstraction. That said, I don't think consolidation should be a blocker for shipping developer-facing features like SchemaDirectiveVisitor or schema stitching / field renaming.

@freiksenet Looking forward to chatting about this with you and @stubailo!

benjamn · 2018-02-16T17:09:10Z

src/test/testDirectives.ts

+        // More complicated visitor implementations might use the
+        // visitorSelector function more selectively, but this SimpleVisitor
+        // class always volunteers itself to visit any schema type.
+        visitSchema(this.schema, () => [this]);


This test demonstrates how easy it is to use the visitSchema function even if you don't need to do something as complicated as what SchemaDirectiveVisitor does.

benjamn · 2018-02-16T17:11:20Z

src/schemaVisitor.ts

+  // SchemaDirectiveVisitor can be instantiated multiple times to visit
+  // directives of different names. In other words, SchemaDirectiveVisitor
+  // implementations are effectively anonymous, and it's up to the caller of
+  // SchemaDirectiveVisitor.visitSchemaDirectives to assign names to them.


This is a pretty important comment. Essentially, implementors of SchemaDirectiveVisitors don't have to commit to any specific names for the directives they're implementing.

zionts · 2018-02-17T01:10:11Z

src/schemaVisitor.ts

+export abstract class SchemaVisitor {
+  // All SchemaVisitor instances are created while visiting a specific
+  // GraphQLSchema object, so this property holds a reference to that object,
+  // in case a vistor method needs to refer to this.schema.


typo vistor

giautm · 2018-02-17T02:07:15Z

cool! 😄

freiksenet · 2018-02-19T07:53:01Z

Check out https://github.com/apollographql/graphql-tools/pull/527/files#diff-57e653512b90ee48bd7b35c45cd51747R1

There is some overlapping work here I guess.

jbaxleyiii

@benjamn still reviewing the tests, but here are a few comments

jbaxleyiii · 2018-02-20T15:14:04Z

src/Interfaces.ts

@@ -78,6 +82,7 @@ export interface IExecutableSchemaDefinition {
  allowUndefinedInResolve?: boolean;
  resolverValidationOptions?: IResolverValidationOptions;
  directiveResolvers?: IDirectiveResolvers<any, any>;
+  directiveVisitors?: { [name: string]: typeof SchemaDirectiveVisitor };


@benjamn I think for the most part people would pass them in when creating the schema. Are there any added usecases to delaying that addition? Like create the schema, do something async, add directives?

jbaxleyiii · 2018-02-20T15:17:38Z

src/schemaGenerator.ts

+      public visitFieldDefinition(field: GraphQLField<any, any>) {
+        const resolver = directiveResolvers[directiveName];
+        const originalResolver = field.resolve || defaultFieldResolver;
+        const directiveArgs = this.args;
        field.resolve = (...args: any[]) => {
          const [source, , context, info] = args;


what is with the extra comma here? I'm assuming that for the arguments, but we should probably name it or note what the empty space is doing to split the args

jbaxleyiii · 2018-02-20T16:37:26Z

src/schemaVisitor.ts

+  }
+
+  function visit(type: VisitableSchemaType) {
+    if (type instanceof GraphQLSchema) {


so right now I think this is fine since graphql-js does this, but in practice I really hate using instanceof since it makes dependency trees harder.

Agreed! Not sure what to do instead, though. I wish graphql-js tagged prototypes like GraphQLObjectType.prototype with a Symbol that was unique to that class, but identical across all copies of the package.

jbaxleyiii · 2018-02-20T16:57:49Z

src/schemaVisitor.ts

+        }
+
+        const decl = declaredDirectives[directiveName];
+        let args: { [key: string]: any };


In some places we use Record<string, any> vs this syntax. I'm honestly not sure of the difference though

jbaxleyiii · 2018-02-20T16:58:42Z

src/schemaVisitor.ts

+          args = getArgumentValues(decl, directiveNode);
+        } else {
+          // If this directive was not explicitly declared, just convert the
+          // argument nodes to their corresponding JavaScript values.


This is actually super useful info for third party packages that may not have access to the sdl!

jbaxleyiii · 2018-02-20T17:02:31Z

src/schemaVisitor.ts

+  // subclasses (not instances) to visitSchemaDirectives.
+  protected constructor(config: {
+    name: string
+    args: { [name: string]: any }


It would be nice for args and context to be type parameters on this so they can be validated in the implementing functions

Ah, then they should probably be type parameters on the SchemaDirectiveVisitor class itself, right?

jbaxleyiii · 2018-02-20T17:03:18Z

src/test/testDirectives.ts

+}
+
+enum Gender @enumTypeDirective {
+  NONBINARY @enumValueDirective


#640 (comment)

benjamn · 2018-02-27T03:47:42Z

docs/source/directive-resolvers.md

@@ -0,0 +1,175 @@
+
+## Directive example


This is the old documentation, just to be clear. The new docs are in schema-directives.md.

stubailo

outdated comments from a week ago

stubailo · 2018-02-16T21:54:01Z

src/Interfaces.ts

@@ -78,6 +82,7 @@ export interface IExecutableSchemaDefinition {
  allowUndefinedInResolve?: boolean;
  resolverValidationOptions?: IResolverValidationOptions;
  directiveResolvers?: IDirectiveResolvers<any, any>;
+  directiveVisitors?: { [name: string]: typeof SchemaDirectiveVisitor };


Yeah I think we want to have it here. The way I'm imagining the most common case is something like:

// We should come up with a naming convention for these import { adminOnlyDirective } from 'graphql-tools-directive-admin-only'; // ... makeExecutableSchema({ typeDefs, resolvers, directives: [adminOnlyDirective] // could also be an object? })

I definitely do think that we shouldn't use the word "visitor" in the name of the option though, since probably most users won't be aware of the fact that the implementation is a visitor pattern (and knowing that doesn't really help)

stubailo · 2018-02-16T21:57:02Z

src/schemaVisitor.ts

+
+  // Determine if this SchemaVisitor (sub)class implements a particular
+  // visitor method.
+  public static implementsVisitorMethod(methodName: string) {


any reason to have these be static members rather than functions exported by this module separately? I don't have anything against it, but there doesn't seem to be any benefit of attaching them here.

This method refers to this.prototype below, so it's important that it's a static method inherited by all subclasses. That's the benefit.

stubailo · 2018-02-16T21:59:00Z

src/schemaVisitor.ts

+  // Determine if this SchemaVisitor (sub)class implements a particular
+  // visitor method.
+  public static implementsVisitorMethod(methodName: string) {
+    if (! methodName.startsWith('visit')) {


What's the goal of this return false? Should it throw an error maybe?

It's just a shortcut because we know this method name can't correspond to a visitor method that this class implements.

Sure - I just think this would be more like "you used this function wrong probably" and not "nope we don't implement this"

stubailo · 2018-02-16T22:05:32Z

src/schemaVisitor.ts

+          args = getArgumentValues(decl, directiveNode);
+        } else {
+          // If this directive was not explicitly declared, just convert the
+          // argument nodes to their corresponding JavaScript values.


Hmmmm, if I import a directive from somewhere, will its arguments/locations get enforced?

stubailo

Some suggestions

stubailo · 2018-02-27T18:58:56Z