Handling of @defer in the query planner
#1958
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
An oversight from a previous change ensured that when adding a field selection to another selection set, we made sure the "rebase" the added selection on the proper schema, but forgot to handle fragment elements. This created issues in some cases when building `@defer` subselection in the query plans, though isn't defer-specific so may have created issue in other non-defer cases (?). In any case, this commit fix that issue, but that actually encovered another issue which is that when we extract subgraph from the supergraph, we forget to add executable directives from the supergraph. Meaning that the extract subgraph may not have the definition for custom executable directive that _are_ copied to subgraph fetches. But someone, the previous was hidding this problem _for the unit test that tests execute directives_ (again, this probably created other issues but ...).
…hQL-js The `@defer` directive has been added as a built-in in the internal `Schema` abstraction, but that means it doesn't get included when converting to a graphQL-js `GraphQLSchema` object. And at least until we link against a version of graphQL-js with `@defer` support, we should have a way to ship the definition. This commit adds an option to `Schema.toGraphQLJSSchema` to do just that. Note that this is an option because whether the definition of `@defer` is desirable depends on what the caller do: if the resulting schema is meant to be used by a server that does not handle `@defer` (either because it does not support it, or because it's explicitely disabled by configuration), then it does not make sense to add the `@defer` definition, this would be misleading.
Notice a slightly inefficiencies with direct nesting for entity types and added a TODO. Will fix that later, but it's only an inefficiency and the generated plan will technically return the proper result.
There was a problem hiding this comment.
I did a high level review of this (not going into the depths of the query planner, which I'm not familiar with) to see if I could bring anything helpful to the table and to help me understand the overall query plan structure for this defer implementation. Hope this helps!
| @@ -1072,8 +1099,50 @@ const graphQLBuiltInDirectivesSpecifications: readonly DirectiveSpecification[] | |||
| locations: [DirectiveLocation.SCALAR], | |||
| argumentFct: (schema) => ({ args: [{ name: 'url', type: new NonNullType(schema.stringType()) }], errors: [] }) | |||
| }), | |||
| // TODO: currently inconditionally adding @defer as the list of built-in. It's probably fine, but double check if we want to not do so when @defer-support is | |||
There was a problem hiding this comment.
The general concept is that if @defer is "part of the schema" (visible via introspection if introspection is on, passes validation when included in an operation) then the server should "support defer" (though it could choose to not defer particular deferred fragments anyway).
|
Oh one other note. There are subtleties in the current spec around mutations. Normal GraphQL serial execution says you completely finish everything rooted at one root mutation field before moving on to the next one. (I hope Router does this correctly already? I actually don't see any tests of mutations in I think your mutation tests cover this correctly but I'm not entirely sure I'm mentally parsing the plans correctly to verify :) |
This address most of the comment from @glasser on apollographql#1958, namely: - it removes/update some outdated TODOs. - it fixup the definition of @defer/@stream to match current spec. - makes serialized query plan for condition hopefully a tad easier to parse (solely impact unit tests readability). - adds a test to ensure that if the same field is queries both "normally" and deferred, we do query it twice (that's what spec specifies).
…2093) An optimisation recently made to the handling of fragments was mistakenly forgetting to look for directive applications on fragments and as a result those application were lost when that optimisation kicked in. In particular, this could make some `@defer` on a named fragments be completely ignored. This patch fixes this by simply avoiding the optimisation in question when there is applied directives, thus preserving them. Additionally, this commit addresses comments from @glasser on #1958, namely: - it removes/update some outdated TODOs. - it fixes up the definition of @defer/@stream to match current spec. - makes serialized query plan for condition hopefully a tad easier to parse (solely impact unit tests readability). - adds a test to ensure that if the same field is queries both "normally" and deferred, we do query it twice (that's what spec specifies).
Experimental support for
@deferin the query planner