Fixes composition issues with @interfaceObject #2318
Conversation
This commit fixes two main problems: 1. merging a shareable field whose return type was an `@interfaceObject` was not working properly. 2. some metadata was not properly computed on a path added for `@interfaceObject` and this led to a potential assertion errors during composition validation on some cases. For the merging of shareable fields, the main issue was that the code checking if 2 types are "the same" was relying on the type "kind", which was fine before but broke with `@interfaceObject` in the sense that some type type T can be an interface type in one subgraph but an object type in another (through `@interfaceObject`) and we were not considering those "the same type". But with that fixed, a small issue remained in that the check added in apollographql#1556 had not been updated for `@interfaceObject` and needed to ignore those. For the 2nd issue, the core bug was that when building a `GraphPath`, we keep track of the edge that made us enter the current graph, and some assertion is essentially checking that there is no "key" edges between that edge and the end of the path, but that "edge entering the subgraph" info was not properly set on a path added for `@interfaceObject` and that triggered the assertion. This commit fix this issue, but this was also highlighting some ineffeciency whereby we were generating unecessary paths (path moving to a subgraph and then moving back again right away), so the patch also fix that inefficiency.
👷 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. |
| * that the structure of named types is the same. Also note that it does not check the "kind" | ||
| * of the type, which is actually relied on due to @interfaceObject (where the "same" type | ||
| * can be an interface in one subgraph but an object type in another, while fundamentally being | ||
| * the same type). | ||
| */ | ||
| export function sameType(t1: Type, t2: Type): boolean { |
There was a problem hiding this comment.
The kind was previously just used as a short-circuit to not have to recurse, but this won't change any existing behavior in theory, correct?
There was a problem hiding this comment.
I honestly didn't consider when writing this that comparing type of different kind could ever matter, so yes that was mostly a short-circuit to ensure the recursion worked. But yes, I reviewed the call site of this and I'm almost certain this won't change any behavior in an intended way (I mean, if we compare types from the same subgraph, this change makes no difference since a given name uniquely identify a type object in a given schema; so this only matter where comparing types from different schema where we check the kind of types between subgraph independently anyway, so this can't really allow things we don't want).
query-graphs-js/src/graphPath.ts
Outdated
| @@ -159,6 +159,8 @@ type PathProps<TTrigger, RV extends Vertex = Vertex, TNullEdge extends null | ne | |||
| /** If the last edge (the one getting to tail) was a DownCast, the runtime types before that edge. */ | |||
| readonly runtimeTypesBeforeTailIfLastIsCast?: readonly ObjectType[], | |||
|
|
|||
| readonly lastIsInterfaceObjectFakeCastAfterNonCollecting: boolean, | |||
There was a problem hiding this comment.
Yeah. Writing such a comment, it occurred to me that there was no need to add a new field, and that I could just reuse the subgraphEnteringEdge field we already have to implement, so update slightly. It saves the field but it's also probably a bit clearer because more explicit.
This commit fixes two main problems:
@interfaceObjectwas not working properly.@interfaceObjectand this led to a potential assertion errors during composition validation on some cases.For the merging of shareable fields, the main issue was that the code checking if 2 types are "the same" was relying on the type "kind", which was fine before but broke with
@interfaceObjectin the sense that some type type T can be an interface type in one subgraph but an object type in another (through@interfaceObject) and we were not considering those "the same type". But with that fixed, a small issue remained in that the check added in #1556 had not been updated for@interfaceObjectand needed to ignore those.For the 2nd issue, the core bug was that when building a
GraphPath, we keep track of the edge that made us enter the current graph, and some assertion is essentially checking that there is no "key" edges between that edge and the end of the path, but that "edge entering the subgraph" info was not properly set on a path added for@interfaceObjectand that triggered the assertion. This commit fix this issue, but this was also highlighting some ineffeciency whereby we were generating unecessary paths (path moving to a subgraph and then moving back again right away), so the patch also fix that inefficiency.