Rejects composition when the options for a path have different runtim… #1556

pcmanus · 2022-03-02T19:17:04Z

…e types

During composition validation, a path of the supergraph can have
multiple valid options in subgraphs due to shared fields. When
that happens, this patch validates that the possible runtime types
of the various options are the same. The rational being that
shared fields should resolve the same way in all subgraphs, so
allowing 2 differen solution of shared field to have different set
of possible runtime types is a dangerous proposition: to be correct,
users would have make sure they only ever return value for that field
that are in the intersection of those possible runtime types.
Instead of asking users to reason about this, forcing the sets of
possible runtime types to always be the same (for a given shared field)
ensures any returned value is always in the intersection (since the
intersection of a set with itself is itself).

Fixes #1375

codesandbox-ci · 2022-03-02T19:17:59Z

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 · 2022-03-02T19:18:40Z

composition-js/src/validate.ts

  const witness = buildWitnessOperation(unsatisfiablePath);
-


Remark: removed that TODO because it is outdated, we do have more detailed error messages now. Technically unrelated to that PR though.

pcmanus · 2022-03-02T19:20:36Z

composition-js/src/validate.ts

+  return types;
+}
+
+export function extractValidationError(error: any): ValidationError | undefined {


Remark: this method is not used within federation itself, but it's useful in some tooling (planning to use it in the update to https://github.com/apollographql/composer-tool after this patch) so I'd prefer keeping it.

pcmanus · 2022-03-02T19:23:38Z

query-planner-js/src/buildPlan.ts

@@ -177,7 +177,7 @@ class QueryPlanningTaversal<RV extends Vertex> {
        // Do note that we'll only need that `__typename` if there is no other selections inside `foo`, and so we might include
        // it unecessarally in practice: it's a very minor inefficiency though.
        if (operation.kind === 'FragmentElement') {
-          this.closedBranches.push([option.paths.map(p => terminateWithNonRequestedTypenameField(p))]);
+          this.closedBranches.push(options.map((o) => o.paths.map(p => terminateWithNonRequestedTypenameField(p))));


Remark: this change is technically not really necessary to this patch, and in fact I'm not entirely sure if this whole branch of query planner can still be hit with the validation added (but I'm unsure and would rather keep it for now), but I noticed this while working on this patch and the new version is "more correct" (mostly, it avoids producing plans that are more inefficient than necessary if this code path is triggered).

clenfest · 2022-03-03T20:43:52Z

internals-js/src/error.ts

@@ -301,6 +301,11 @@ const INVALID_LINK_DIRECTIVE_USAGE = makeCodeDefinition(
  'An application of the @link directive is invalid/does not respect the specification.'
 );

+const SHAREABLE_WITH_MISMATCHED_RUNTIME_TYPES = makeCodeDefinition(


I would change this to SHAREABLE_HAS_MISMATCHED_RUNTIME_TYPES as it's a little more clear.

clenfest · 2022-03-03T20:44:28Z

internals-js/src/error.ts

@@ -301,6 +301,11 @@ const INVALID_LINK_DIRECTIVE_USAGE = makeCodeDefinition(
  'An application of the @link directive is invalid/does not respect the specification.'
 );

+const SHAREABLE_WITH_MISMATCHED_RUNTIME_TYPES = makeCodeDefinition(
+  'SHAREABLE_WITH_MISMATCHED_RUNTIME_TYPES',
+  'A shareable field return type have mismatched possible runtime types in the subgraphs in which the field is declared. As shared fields must resolve the same way in all subgraphs, this is almost surely a mistake.'


change 'have' to 'has'

clenfest · 2022-03-07T10:07:42Z

composition-js/src/validate.ts

@@ -257,6 +277,19 @@ function initialSubgraphPaths(kind: SchemaRootKind, subgraphs: QueryGraph): Root
  return subgraphs.outEdges(root).map(e => initialState.add(subgraphEnteringTransition, e, noConditionsResolution));
 }

+function possibleRuntimeTypeNamesSorted(path: RootPath<Transition>): string[] {
+  const types = path.tailPossibleRuntimeTypes().map((o) => o.name);


Is this already deduped?

It is. And it does take a small effort to make sure so I commented as much on the method. But essentially, tailPossibleRuntimeTypes in GraphPath is only updated through the (private) updateRuntimeTypes method in graphPaths.ts, and the only place we "push" there is guarded by I first check to ensure the element is not already there.

clenfest · 2022-03-07T10:26:35Z

composition-js/src/__tests__/validation_errors.test.ts

@@ -112,3 +112,57 @@ describe('@requires', () => {
    ]);
  });
 });
+
+


In the comments of validation.ts you have a union as an example, did you want to have that as an explicit test as well or are the interface / union examples sufficently similar?

You're right, it's worth having both and pulled the example from validation.ts into a proper test (which also ensure the comment is actually correct, which it wasn't cause I had forgotten some @shareable :)).

clenfest · 2022-03-07T10:34:48Z

composition-js/src/validate.ts

+      for (const path of newSubgraphPaths) {
+        // Note: we add spacing between elements because we're going use it if we display an error. This doesn't impact our set equality though
+        // since type names can have neither comma or space in graphQL.
+        runtimeTypesToSubgraphs.add(possibleRuntimeTypeNamesSorted(path).join(', '), path.tail.source);


Trying to follow this, but I'm not sure if I'm getting it right. For the following example:

# subgraph 1 Query { a: A! } interface A { x: Int } type B implements A { x: Int } #subgraph 2 Query { a: A! } interface A { x: Int } type C implements A { x: Int }

The things that are being compared are the internals of types B and C to make sure they match, so the string you're building will look something like "x:Int"? Is that right? If so, I think it looks good, if otherwise I may have more questions.

So no. In that example, that code path will kick in after we've look at field Query.a, and we will essentially have 2 "options" (2 newSubgraphPaths), one per subgraph, but each one "tail" will be on interface A. So for the 1st path, possibleRuntimeTypeNamesSorted will return B, while it will return C. And because those are different, we'll return the error. Does that help?

ok, we want that case to error because the types are named differently, so it doesn't matter.

netlify · 2022-12-05T15:07:54Z

✅ Deploy Preview for apollo-federation-docs ready!

Name	Link
🔨 Latest commit	`b195e46`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/63b690279baeb200080ee42b
😎 Deploy Preview	https://deploy-preview-1556--apollo-federation-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

pcmanus · 2022-12-05T15:19:51Z

Unfortunately, in the lead up to federation 2.0, this PR fall through the crack and never got finished/committed.

I'd like to resurrect this and get it committed ASAP however, because I think this can really help people not getting @shareable on entity and root fields wrong. #2228 is a recent example of this can easily trip people up at the moment, but there is some evidence that this is not at all the only case.

That said, upon further reflexion (and a bit influenced by the fact this missed 2.0), I changed the check a bit to make it a bit more permissive. That is the rebased version:

error composition if the runtime types (for a given shareable field) have no intersection whatsoever, as this essentially is guaranteed to be incorrect for @shareable. This would catch example like the on in Strange query plan with @shareable root fields #2228, or users attempting to do the "node relay API" with @shareable (which cannot work; it's absolutely important we improve how easy it is to implement the "node relay API", but @shareable definitively isn't it and we should help user realise it quickly).
warn (issue a hint with WARN level) if the runtime types intersects, but are not equal. The reasoning for a warn is that it's possible to implement such a case correctly, and one could imagine specific case where you could end up in this situation (one might be when you evolve 2 subgraphs at a different pace), but it's still a bit of a red flag so a warning seem appropriate.

…e types During composition validation, a path of the supergraph can have multiple valid options in subgraphs due to shared fields. When that happens, this patch validates that the possible runtime types of the various options are the same. The rational being that shared fields should resolve the same way in all subgraphs, so allowing 2 differen solution of shared field to have different set of possible runtime types is a dangerous proposition: to be correct, users would have make sure they only ever return value for that field that are in the intersection of those possible runtime types. Instead of asking users to reason about this, forcing the sets of possible runtime types to always be the same (for a given shared field) ensures any returned value is always in the intersection (since the intersection of a set with itself is itself). Fixes #1375

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.

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 #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.

pcmanus commented Mar 2, 2022

View reviewed changes

pcmanus force-pushed the 1375-shared-field-with-interfaces branch from 6a74f17 to 39d8302 Compare March 2, 2022 19:24

clenfest reviewed Mar 7, 2022

View reviewed changes

pcmanus mentioned this pull request Jun 21, 2022

Apollo Gateway Query Plan incorrectly handles union/fragment of sharable Interfaces #1917

Open

pcmanus mentioned this pull request Nov 18, 2022

Strange query plan with @shareable root fields #2228

Closed

pcmanus force-pushed the 1375-shared-field-with-interfaces branch from 39d8302 to 38ecf4a Compare December 5, 2022 15:07

pcmanus requested a review from StephenBarlow as a code owner December 5, 2022 15:07

pcmanus self-assigned this Dec 5, 2022

pcmanus force-pushed the 1375-shared-field-with-interfaces branch from 38ecf4a to fcda450 Compare December 5, 2022 15:13

korinne added this to the 2.3.0 milestone Dec 5, 2022

pcmanus force-pushed the 1375-shared-field-with-interfaces branch from fcda450 to ffd2943 Compare December 14, 2022 14:10

pcmanus changed the base branch from main to next December 14, 2022 14:10

clenfest approved these changes Jan 5, 2023

View reviewed changes

pcmanus changed the base branch from next to main January 5, 2023 08:53

pcmanus force-pushed the 1375-shared-field-with-interfaces branch from ffd2943 to b195e46 Compare January 5, 2023 08:53

pcmanus merged commit 73eb02d into main Jan 5, 2023

pcmanus deleted the 1375-shared-field-with-interfaces branch January 5, 2023 09:08

pcmanus mentioned this pull request Jan 9, 2023

Fixes composition issues with @interfaceObject #2318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rejects composition when the options for a path have different runtim… #1556

Rejects composition when the options for a path have different runtim… #1556

pcmanus commented Mar 2, 2022

codesandbox-ci bot commented Mar 2, 2022 •

edited

Loading

pcmanus Mar 2, 2022

pcmanus Mar 2, 2022

pcmanus Mar 2, 2022

clenfest Mar 3, 2022

clenfest Mar 3, 2022

clenfest Mar 7, 2022

pcmanus Mar 7, 2022

clenfest Mar 7, 2022

pcmanus Mar 7, 2022

clenfest Mar 7, 2022

pcmanus Mar 7, 2022

clenfest Mar 7, 2022

netlify bot commented Dec 5, 2022 •

edited

Loading

pcmanus commented Dec 5, 2022

@@ @@ -112,3 +112,57 @@ describe('@requires', () => { @@
                   ]);
                 });
               });

Rejects composition when the options for a path have different runtim… #1556

Rejects composition when the options for a path have different runtim… #1556

Conversation

pcmanus commented Mar 2, 2022

codesandbox-ci bot commented Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Dec 5, 2022 • edited Loading

✅ Deploy Preview for apollo-federation-docs ready!

pcmanus commented Dec 5, 2022

codesandbox-ci bot commented Mar 2, 2022 •

edited

Loading

netlify bot commented Dec 5, 2022 •

edited

Loading