Fix merging of Input objects and enum types #1672
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. |
pcmanus
force-pushed
the
input-merge-updates
branch
from
e0d6fa4
to
584b1aa
Compare
| @@ -226,11 +226,11 @@ export function computeSubgraphPaths(supergraphPath: RootPath<Transition>, subgr | |||
| assert(!supergraphPath.hasAnyEdgeConditions(), () => `A supergraph path should not have edge condition paths (as supergraph edges should not have conditions): ${supergraphPath}`); | |||
| const supergraphSchema = firstOf(supergraphPath.graph.sources.values())!; | |||
| const conditionResolver = new ConditionValidationResolver(supergraphSchema, subgraphs); | |||
| const initialState = ValidationState.initial({supergraph: supergraphPath.graph, supergraphSchema, kind: supergraphPath.root.rootKind, subgraphs, conditionResolver}); | |||
| const initialState = ValidationState.initial({supergraph: supergraphPath.graph, kind: supergraphPath.root.rootKind, subgraphs, conditionResolver}); | |||
There was a problem hiding this comment.
Note to reviewers: the change to this file, to graphPath.ts and the removal of structuralSubtyping.ts are all knock-on effects (cleanups) from the change to matchesSupergraphTransition in queryGraph.ts (see comment there).
| const transition = this.transition; | ||
| switch (transition.kind) { | ||
| case 'FieldCollection': |
There was a problem hiding this comment.
What this method was previously trying to do is enforce the "intersection" semantic for input types, but to do that during composition validation. It is, in a way, the part that was broken and is fixed in this patch.
The idea of this code was to merge inputs with a union (so, say, keeping any input type field defined in any subgraph), but then when doing composition validation for a field, to ignore any definition of that field that don't accept the "full" input object as merged in the supergraph API. Which really ends up requiring (in 99.99% of the case) that all the inputs are the same in all subgraphs, but is, in hindsight, an overly complex and confusing way to do this. Which is why this patch removes this and instead deal with input types during merging, which is easier to do.
Now, you can ignore this code since it's removed, but if you're wondering "I can admit this wasn't idea, but why wasn't this working?" then the answer is that there is a typo in the isAccessible method of structuralSubtyping.ts (a missing negation), which essentially made all this code a no-op. What happens if we fix that typo? Well, I tried and run into some assertion errors and I'm sure that assertion could been fixed easily enough but I didn't looked more closely because even if this code ended up working as I intended it to 6+ months ago when I wrote it, it would end up in pretty confusing composition validation error messages and we would have to work hard to make those errors understandable. That's one of the reason why dealing with this during merging is likely way better for input: it's much much easier to provide good error messages.
| @@ -129,13 +129,14 @@ class Validator { | |||
| return this.errors; | |||
| } | |||
|
|
|||
| private validateHasType(elt: { type?: Type, coordinate: string, sourceAST?: ASTNode }) { | |||
| private validateHasType(elt: { type?: Type, coordinate: string, sourceAST?: ASTNode }): boolean { | |||
There was a problem hiding this comment.
Note to reviewers: the changes to this file are completely unrelated to the rest of the patch. It's just that when I was working on this, I run into this and that was easy enough to fix, so I did.
Namely, this method validates that we don't have some types unset in the schema (which can't really happen when we parse a schema because it's invalid syntax, but if one build a schema programmatically, this could happen), but in a few cases, we were following it by other validations that assumed the type was set. So the change here simply have the method return whether the type is set or not and the other changes in this file simply skip the dependent validation when the type is indeed unset. Concretely, this ensures we get good errors instead of some random "undefined"-access one thrown.
|
Before I get started at a high level, I have no problem with the idea that output types are unions, that makes sense to me. But for input types being intersections, it seems a little weird to me that a subgraph author can define a value that they may expect and to have composition silently remove the value they specify in an enum simply because it's not available elsewhere. Edit: I see that there is a hint. I think this is the best we can do. |
composition-js/src/merging/merge.ts
Outdated
| @@ -356,13 +374,13 @@ class Merger { | |||
| // Then, for object and interface types, we merge the 'implements' relationship, and we merge the unions. | |||
| // We do this first because being able to know if a type is a subtype of another one (which relies on those | |||
| // 2 things) is used when merging fields. | |||
| for (const objectType of this.merged.types<ObjectType>('ObjectType')) { | |||
| for (const objectType of this.typesToMerge<ObjectType>('ObjectType')) { | |||
There was a problem hiding this comment.
These should probably be separate functions. this.typesToMerge<ObjectType>('InterfaceType'); is valid code, but could cause typing errors.
There was a problem hiding this comment.
Oh yeah, the fact that we're casting inside of Schema::types before returning is definitely a code smell.
There was a problem hiding this comment.
this.typesToMerge<ObjectType>('InterfaceType');is valid code
I'll note that this isn't really true, that code did not compile (the kind argument has to match the kind of the type parameter because it's type is T['kind']).
That said, that method (and schema::types) was indeed a bit error prone as this.typesToMerge<ObjectType>() was valid code, yet didn't return only the object types. I initially went with it as I figured it would obvious that simply passing a type argument couldn't influence runtime behaviour and it would obvious you should do this, but admittedly not my best moment in hindsight.
Anyway, I cleaned this up a bit to avoid that potential problem, but the code in merge.ts still uses a single filterTypesOfKind<ObjectType>(typesToMerge, 'ObjectType') method because it is type safe now (the kind argument is not optional anymore) and it didn't felt cleaner adding a bunch of single-usage methods.
At a high level, the merging guiding principle is meant to be:
- output types are merged by "union": meaning an element of an output
types can (in general) be in the supergraph as soon as it is in any
subgraph. The idea being that it's ok (and desirable) that different
subgraph can return different "sub-parts" of an output type and the
query planner is here direct queries appropriately. Of course, in
practice, the "composition validation" algorithm imposes some limits
to this based on the reachability of the type.
- input types are merged by "intersection": meaning that an element
an input type is only in the supergraph if it is in all the subgraph
defining the type in question. The idea being that the gateway don't
control the inputs it gets, and so in general, all subgraphs needs
be able to handle all the inputs exposed by the supergraph API.
This guiding principle was not implemented properly for both Input
Object types and Enum types and this is fixed by this commit.
Note that:
- for input object types, this has the consequence of aligning the
merging of input fields with that of (output) field arguments. Indeed,
passing multiple arguments is extremely similar semantically to
passing a single input object having the same fields, so it does not
make sense for the behiavour of those to differ, and it doesn't
anymore after this patch.
- enum types are a special case because they can be use as both output
types and input types (this is true for scalar too, but scalar merging
is trivial). The patch makes the behaviour depend on how the enum
type is used:
- if it is only ever used as an input type, then we merge it by
"intersection".
- if it is only ever used as an output type, then we merge it by
"union".
- otherwise, if it used as both, then merging is essentially
"equality", meaning that we have a composition error unless all
the subgraphs defined the same values.
pcmanus
force-pushed
the
input-merge-updates
branch
from
584b1aa
to
54591ad
Compare
I noticed a few spelling changes we could make from #1672
I noticed a few spelling changes we could make from #1672
At a high level, the merging guiding principle is meant to be:
types can (in general) be in the supergraph as soon as it is in any
subgraph. The idea being that it's ok (and desirable) that different
subgraph can return different "sub-parts" of an output type and the
query planner is here direct queries appropriately. Of course, in
practice, the "composition validation" algorithm imposes some limits
to this based on the reachability of the type.
an input type is only in the supergraph if it is in all the subgraph
defining the type in question. The idea being that the gateway don't
control the inputs it gets, and so in general, all subgraphs needs
be able to handle all the inputs exposed by the supergraph API.
This guiding principle was not implemented properly for both Input
Object types and Enum types and this is fixed by this commit.
Note that:
merging of input fields with that of (output) field arguments. Indeed,
passing multiple arguments is extremely similar semantically to
passing a single input object having the same fields, so it does not
make sense for the behiavour of those to differ, and it doesn't
anymore after this patch.
types and input types (this is true for scalar too, but scalar merging
is trivial). The patch makes the behaviour depend on how the enum
type is used:
"intersection".
"union".
"equality", meaning that we have a composition error unless all
the subgraphs defined the same values.