feature(dynamic-context): Execute function for context

[matthewerwin]

Provides the ability to do post-AST validation (e.g. security) as well as to build the context dynamically with a user object or other information.

Seems often all this work is expected in the resolvers and that adds a lot of extra code to every resolver to verify AST only has the fields permissible for the user. Root context doesn't have the AST information and there don't seem to be any other useful injection points between the inbound request and the resolver.

With this setup we elegantly handle errors can provide a rich security context and AST-management toolset to the resolvers from a centralized location.

e.g. consider as follows:

router.use('/graphql', graphqlExpress((req,res)=> {
  return {
    schema,
    context: async ({ast, args, operationName}) :Promise<RequestContext> => {
      const jwt:JwtContext = (<any>req).jwt_context;
      let user:ExtendedUser|null = null;
      if( jwt.access_token ) {
        user = await SecurityDb.AuthenticatedUser_Get(jwt.access_token, settings.server.jwt.expiresInSeconds);
      }
      else throw new NoAccessTokenError("Unauthorized");

      //class which can do AST-level access checks and provide request helper methods
      return new RequestContext( req, res, ast, user );
    }  };
}));

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[IvanGoncharov]

@matthewerwin Cool PR! Previously I made very similar one but instead of context, I used rootValue, see #253. One problem that both proposals have in common is that we change symantic of values, you can't pass function as context to execute anymore.

Second problem is that with such approach we need to add callbacks for everything: context, rootValue, documentAST, etc. I think we need more flexiable solution which allow you to inject any pre-execute and even post-execute code. That's why I submited #391 which allow you wrap standard execute in any code you want.

Would be great to hear you opinion about #391?

[IvanGoncharov]

@matthewerwin To provide more context for discussion here is your example converted to use #391:

router.use('/graphql', graphqlExpress((req,res)=> {
  return {
    schema,
    execute: async (executeArgs) :Promise<ExecutionResult> => {
      const jwt:JwtContext = (<any>req).jwt_context;
      let user:ExtendedUser|null = null;
      if( jwt.access_token ) {
        user = await SecurityDb.AuthenticatedUser_Get(jwt.access_token, settings.server.jwt.expiresInSeconds);
      }
      else throw new NoAccessTokenError("Unauthorized");

      //class which can do AST-level access checks and provide request helper methods
      return execute({
        ...executeArgs,
        context: new RequestContext( req, res, executeArgs.document, user );
      });
    }
  };
}));

[matthewerwin]

@IvanGoncharov thanks for the commentary and making me aware of your changes! One thing that is unclear to me is how to invoke the default execute function. I looked at your PR -- so is the internal execute({...executeArgs}) calling the default GraphQL execute function? Seems you would need to pass that internal function into the executeFn wrapper.

Providing an override option of the internal execute function is certainly powerful for mocking/faking aspect -- but this is already possible by passing in a mock set of resolvers rather than overriding the execute function. Even for the 3rd party API calls you mention in your PR it seems those would also be possible from the resolver vs as a "middleware" unless the calls need to be made for all resolvers and in that case why not just use the dynamic context here with post-AST parsing to make your calls and put the results in the context. That approach also avoids future function signature issues that may come up by exposing an override to the internal execute().

To directly address your two points above:

One problem that both proposals have in common is that we change symantic of values, you can't pass function as context to execute anymore.

This PR doesn't pass a function to execute() -- it evaluates context prior to execution of the resolver.

Second problem is that with such approach we need to add callbacks for everything: context, rootValue, documentAST, etc. I think we need more flexiable solution which allow you to inject any pre-execute and even post-execute code. That's why I submited #391 which allow you wrap standard execute in any code you want.

If I understand your suggestion on a more flexible solution...you're relating it to a pre/post execution hook, which in a formal middleware sense is very different than letting someone overwrite the execute( ) function -- i.e. middleware is a flexible pipeline, not an override of internals. A true middleware implementation would allow modification of the context, AST, rootValue in a generic fashion that you mention -- however, that's not the current architecture in the library and for middleware extensibility requires a lot more of a design change than is present in either of our PR's.

Let me know your thoughts!

[IvanGoncharov]

@matthewerwin I forget to mention that execute function is part of public API of graphql-js:
http://graphql.org/graphql-js/execution/#execute
So to get it you just need to do:

import { execute } from 'graphql-js'

That approach also avoids future function signature issues that may come up by exposing an override to the internal execute().

This function is part of public API of graphql-js library so changing it is a breaking change.

This PR doesn't pass a function to execute() -- it evaluates context prior to execution of the resolver.

Sorry for confusing explanation. I meant different situation when a user wants's to pass some function as the context for his resolvers. Since both express-graphql and graphql-js doesn't enforce any constraints on possible values of context that means some server can pass a function as context value. So technically this PR introduce breaking change into the public API.
For example:

router.use('/graphql', graphqlExpress((req,res)=> {
  return {
    schema,
    context: (sql) => db.execute(sql)
  };
}));

You can address this issue by making separate preExecute function which accepts ExecutionArguments object and returns an object with the same type. This will allows you to modify not only context but any argument of execute and doesn't introduce breaking changes into existing API.

Note: I still think overriding execute is better since it allows both preExecute and postExecute functionality inside one function and doesn't complicate Promise chain inside this lib.

Even for the 3rd party API calls you mention in your PR it seems those would also be possible from the resolver

It's extremely complicated to implement GraphQL proxy inside resolver functions. Inside resolvers, you only get arguments of a certain field and almost nothing else. It's impossible to construct query for 3rd-party server based on just the name of field and args, you need to know entire query like above fields and also subselection. If you also take into account abstract types like interfaces and unions you need to resolveType even before you field resolver are called. There is a ton of problem with proxying through resolvers and even if you solve them it will require more than 200 lines of js code.

But I think technical details of implementing GraphQL proxy is unrelated to this discussion, let's focus on making express-graphql flexible to accommodate the majority of use-cases.

If I understand your suggestion on a more flexible solution...

I agree that my use case is pretty unique and shouldn't be a strong argument for adding features into this lib. But you need exactly the same functionality for logging queries and their results or for measuring the performance of execute. Or implementing custom directives that affect query execution like @skip and @include.

you're relating it to a pre/post execution hook, which in a formal middleware sense is very different than letting someone overwrite the execute( ) function

I don't see a lot of difference here since execute is part of public API so its input and output parameters are well defined. So it's pretty easy to create your own wrapper.

[matthewerwin]

@IvanGoncharov thank you for shedding light on the public API method and excellent references -- influences my thoughts substantially around this topic. Under that light my preference would be to see #391 merged...what's been the delay?

[IvanGoncharov]

what's been the delay?

@matthewerwin I think it should be reviewed by someone from Facebook preferably @leebyron.

[IvanGoncharov]

Fixed in #394