Separate validation of operation and coercion of variables into separate steps #3658
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This comment has been minimized.
This comment has been minimized.
@IvanGoncharov The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
|
It's interesting that this change made I ran it a few times and it was always faster. |
| @@ -871,7 +871,6 @@ describe('Execute: Handles basic execution tasks', () => { | |||
| expectJSON( | |||
| executeSync({ schema, document, operationName: 'Q' }), | |||
| ).toDeepEqual({ | |||
| data: null, | |||
There was a problem hiding this comment.
this change fits your proposed simplified criteria here: graphql/graphql-spec#894 (comment) but I am not sure about it
| @@ -883,7 +882,6 @@ describe('Execute: Handles basic execution tasks', () => { | |||
| expectJSON( | |||
| executeSync({ schema, document, operationName: 'M' }), | |||
| ).toDeepEqual({ | |||
| data: null, | |||
| @@ -895,7 +893,6 @@ describe('Execute: Handles basic execution tasks', () => { | |||
| expectJSON( | |||
| executeSync({ schema, document, operationName: 'S' }), | |||
| ).toDeepEqual({ | |||
| data: null, | |||
| @@ -424,6 +424,7 @@ describe('Subscription Initialization Phase', () => { | |||
| { | |||
| message: 'The subscription field "unknownField" is not defined.', | |||
| locations: [{ line: 1, column: 16 }], | |||
| path: ['unknownField'], | |||
There was a problem hiding this comment.
I am not sure about this one. There is not an exact corollary to queries/mutations, because unknown fields there are silently dropped rather than raising an error.
The spec at https://spec.graphql.org/draft/#sel-HAPHRPJABABmE-1Y says:
This field should be a list of path segments starting at the root of the response and ending with the field associated with the error. Path segments that represent fields should be strings, and path segments that represent list indices should be 0-indexed integers. If the error happens in an aliased field, the path to the error should use the aliased name, since it represents a path in the response, not in the request.
The last sentence seems odd to me as the paths in the response and the request are the same -- perhaps it should be interpreted that the path is the path in the response, not in the schema. That might argue as you have here that a field not in the schema can have a path. Although, I am not sure about this, as perforce if the field is not in the schema, it cannot be in the response. For subscriptions, of course, there is no path in the response of CreateSourceEventStream, as the response for that section of the algorithm is not a map, it's an AsyncIterable. So, overall, I think we are better off without the path, although it seems to me that it could go either way.
| errors: ReadonlyArray<GraphQLError>, | ||
| ): ExecutionResult { | ||
| return errors.length === 0 ? { data } : { errors, data }; | ||
| interface GraphQLExecutionPlanOptions { |
There was a problem hiding this comment.
These options are to be passed into the makeExecutionPlan function and are used to construct the Plan below. But they also just form part of the Plan itself, and so they are not that different in kind from the other properties of the Plan, which are all essentially options. It seems as if these options might be better named "MakeExecutionPlanOptions", see my comment below on that function.
|
|
||
| type ResultOrGraphQLErrors<T> = |
There was a problem hiding this comment.
Is this helpful in terms of #3649 ?? -- I'm afraid I did not try to reproduce that error, and have not yet hit it with my TS version.
There was a problem hiding this comment.
Yes, exactly.
I missed #3649 but I always dislike this particular check.
I think we need to have a generic convention and this one seems like one that suits our use case the best since the error response is a valid GraphQL response.
| schema: GraphQLSchema, | ||
| document: DocumentNode, | ||
| operationName: Maybe<string>, | ||
| options: GraphQLExecutionPlanOptions = {}, |
There was a problem hiding this comment.
Why are some of these named options, but not others. Is the function going to be memoized?
| | ExecutableMutationRequest | ||
| | ExecutableSubscriptionRequest; | ||
|
|
||
| export interface ExecutableQueryRequest { |
There was a problem hiding this comment.
@IvanGoncharov This is the key comment I believe in my review of this approach. It's more of a question than a comment.
I know I have asked elsewhere to export interfaces so that third parties can customize, add hooks, etc. I am generally happy to see the suggestion adopted, but I have to say that I don't know if the added complexity in this particular instance is so helpful. We currently have a pipeline of schema generation => request parse => request validate => request execute. Hidden in there also as a step is that without "assumeValid", schema validation is automatically performed.
My thinking in overall direction was to break out additional recommended steps, i.e., "validateSchema" would be its own step, request document parsing would be its own step, variable coercion would be its own step, and execute would then operation on the results of the previous stages, such that ExecutionContext would be broken down into its constituent parts, some cacheable (document), some not cacheable/not as cacheable (variables). Perhaps this interface/implementation approach has some additional benefits I am not appreciating?
There was a problem hiding this comment.
Hidden in there also as a step is that without "assumeValid", schema validation is automatically performed.
It's not hidden, it's "optional" you can perform it explicitly if you want.
All GraphQL servers I saw just call validateSchema and even our own graphql function does that, for example, https://github.com/graphql/express-graphql/blob/f4414b44996f25a5328e523d9a4b213fd1d70b16/src/index.ts#L268
But I agree it's a bad API design so I plan to work on the GraphQLExecutableSchema class to make it more explicit from an API standpoint.
I know I have asked elsewhere to export interfaces so that third parties can customize, add hooks, etc. I am generally happy to see the suggestion adopted, but I have to say that I don't know if the added complexity in this particular instance is so helpful.
I still have the same position about hooks, this PR adds interfaces that can be used by external libraries to customize executor.
Different here is that we don't have any single line of code that can run 3rd-party ExecutableRequest, so it's not a hook.
I'm for adding configuration/interfaces if the below rules are satisfied:
- Use case for which we add it is fully spec-compliant.
- We can't incorporate this use case in graphql-js. For example, there is no "one size fits all" solution.
Custom GraphQL executors satisfy both rules, JIT is a valid use case but JIT is not a universal one (not everyone trusts generated code to run through eval) it also applies some limitations: https://github.com/zalando-incubator/graphql-jit#differences-to-graphql-js
Even JIT aside, custom execute engines purposely built for complex scenarios like federation, stitching, etc. will always outperform generic ones even with all necessary hooks provided (hooks add their own cost and prevent certain JIT/compilation optimizations).
So it's a valid use case for adding interfaces.
This PR basically implements the #3314 proposal and defines clean interfaces that can be used by GraphQL servers to support specialized GraphQL executors.
I have to say that I don't know if the added complexity in this particular instance is so helpful.
Just to be clear, are you referring to interfaces specifically or your comment is about the entire approach of using class instances vs having functions accept query plan as an argument?
There was a problem hiding this comment.
I don't think we need an ExecutableSchema class. I think if validateSchema is not called, the schema should not be validated, plain and simple.
In terms of your last question -- I am not sure I understand the distinction. I commented below in terms of relationship to #3314.
Basically, if the goal is to separate out coercion, and storing result of document analysis into separate step, I think that can be done as separate exported functions, like analyzeDocument and coerceVariables, and then the user can cache them. We actually don't have to deprecate execute, we can still include it for those who want it but just be explicit about which exported pipeline steps it performs: analyzeDocument, coerceVariables, getRootType, executeOperation...
| schema: GraphQLSchema; | ||
| operation: OperationDefinitionNode; | ||
| fragments: ObjMap<FragmentDefinitionNode>; | ||
| rootType: GraphQLObjectType; |
There was a problem hiding this comment.
It's nice that this is cacheable at this point, so that we don't have to repeatedly check that it exists for the same schema/operation.
| /** | ||
| * Implements the "ExecuteQuery" algorithm described in the GraphQL specification. | ||
| */ | ||
| executeOperation: ( |
There was a problem hiding this comment.
see above, I like how executeOperation now actually executes every operation (whereas previously the term was used for retrieving the data-only for queries/mutations). I just think we can deprecate the execute stage of the pipeline and utilize executeOperation, which would take a set of cacheable parameters (schema/document) as well as the 3 arguments here).
There was a problem hiding this comment.
I just think we can deprecate the execute stage of the pipeline and utilize executeOperation, which would take a set of cacheable parameters (schema/document) as well as the 3 arguments here).
We can do that as an intermediate step.
Just to clarify you are proposing using WeakMap internally to cache stuff based on schema/document/operationName as you do in graphql-executor?
Or are you proposing to use externally catchable ExecutionPlanas a replacement for schema/document/operationName?
There was a problem hiding this comment.
But as I wrote above, although the beginnings of #3314 r here, there is very little that is being actually done and stored within the plan (afaict) besides storing the a validated root type. In terms of the exported interface, it also would not be enough (afaict) to build a customized stitching executor which needs access to later steps in the execution algorithm. Although I think I see the direction, in terms of functionality, I am not sure this as yet brings enough minimum value to be worth the complexity.
There was a problem hiding this comment.
I was constrained within graphql-executor because I wanted it to be a drop in replacement for graphql-js. So there, I used a simple memoization strategy that assumed if you have cached a document, everything about it should be cached.
If we are talking about changing the graphql-js api - I think exporting a few more low level building blocks is the low-hanging fruit.
This draft PR shows what I'm currently working on.
It passes tests but fails due to lower coverage.
I will try to split it into smaller commits with proper descriptions and separate discussions.
The purpose of this Draft PR is to show the end goal of these smaller comments.
Any feedback/review is welcome but please keep in mind this PR is in the exploration phase (and code is still raw) so please focus only on high-level stuff.