Fix incorrectly rejected composition and subgraph issues with `@inter… #2494

pcmanus · 2023-03-23T10:46:50Z

…faceObject`

There is 2 main issues with the handling of @interfaceObject that this patch fixes:

given some @interfaceObject type I in some subgraph S1, if another subgraph S2 was declaring an implementation type T of I but without declaring I, then the code missed creating an "edge" between T in S2 and I in S1, which resulted in composition rejecting such cases, even though it should work.
there were an additional issue in the handling of @requires so that the "input rewrites" generated were not generic enough, leading to sometimes sending incorrect queries to subgraph (more precisely, we were sometimes not rewriting the typename when querying an @interfaceObject subgraph, leading that subgraph to complain that it is asked for type it doesn't know).

Note that fixing those 2 issues surfaced the fact that the handling of "rewrites" in the gateway was not working exactly as it should have. More precisely, hen a path had a fragment ... on I, if I was an interface, then we would no select objects that implements I correctly. As it happens, the router implementation of those rewrites was both cleaner and didn't had that issue, so this patch also updates the handling of "rewrites" to mimick what the router implementation does (fixing the issue, and overall cleaning the code).

Fixes #2485

netlify · 2023-03-23T10:46:54Z

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`c907e94`

changeset-bot · 2023-03-23T10:46:54Z

🦋 Changeset detected

Latest commit: c907e94

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/query-planner	Patch
@apollo/query-graphs	Patch
@apollo/federation-internals	Patch
@apollo/gateway	Patch
@apollo/composition	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci · 2023-03-23T10:47:33Z

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 · 2023-03-23T12:24:35Z

query-planner-js/src/QueryPlan.ts

@@ -46,29 +46,19 @@ export interface FetchNode {
  operationKind: OperationTypeNode;
  operationDocumentNode?: DocumentNode;
  // Optionally describe a number of "rewrites" that query plan executors should apply to the data that is sent as input of this fetch.
-  inputRewrites?: FetchDataInputRewrite[];


Note to reviewer(s): the context behind the changes of this file is that I always though to ultimately have just FetchDataRewrite, but I didn't do a super good job of the first implementation of those rewrites on the execution side, so that before this patch, input and output rewrites use separate code at execution, and I used this separate typing to avoid dead code/ugly assertions in the execution code.

And fwiw, when I implemented this in the router side, I realise there was no point in having that distinction, so the router don't even bother distinguishing between those 2 types. Anyway, this patch cleans up the execution on the gateway side, using the same approach than in the router, and in doing so the distinction between FetchDataOutputRewrite and FetchDataInputRewrite becomes meaningless, so I figure it's worth getting rid of.

But do note this has no impact on the router since this is just some typing that is changed here: the underlying plans we generate are unchanged.

clenfest · 2023-03-28T21:08:51Z

internals-js/src/operations.ts

@@ -304,16 +305,18 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
  }

  private canRebaseOn(parentType: CompositeType) {
-    // There is 2 valid cases we want to allow:
+    const fieldParentType = this.definition.parent
+    // There is 3 valid cases we want to allow:


Unless I'm missing it, you don't specify the third case.

Fair. I was counting 3 because there is 3 "or" conditions, but both the "interface" and "interfaceObject" are similar and described in the same point, so that's probably a bit confusing as a result.

clenfest · 2023-03-28T21:14:50Z

.changeset/hungry-berries-destroy.md

+"@apollo/gateway": patch
+---
+
+Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occurs


Suggested change

Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occurs

Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occur

clenfest · 2023-03-28T21:16:34Z

.changeset/hungry-berries-destroy.md

+Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occurs
+either due to some use of `@requires` in an `@interfaceObject` type, or when some subgraph `S` defines a type that is an
+implementation of an interface `I` in the supergraph, and there is an `@interfaceObject` for `I` in another subgraph,
+but `S` does not itself defines `I`.


Do you mean that it doesn't declare @interfaceObject for I?

No, I mean that S does not have type I at all (but it declares a type that is an implementation of I in another subgraph).

clenfest · 2023-03-28T21:20:38Z

query-graphs-js/src/querygraph.ts

+              const otherVertice = otherVertices[0];
+              // The edge goes from the otherSubgraph to the current one.
+              const head = copyPointers[j].copiedVertex(otherVertice);


Suggested change

const otherVertice = otherVertices[0];

// The edge goes from the otherSubgraph to the current one.

const head = copyPointers[j].copiedVertex(otherVertice);

const otherVertex = otherVertices[0];

// The edge goes from the otherSubgraph to the current one.

const head = copyPointers[j].copiedVertex(otherVertex);

clenfest · 2023-03-28T21:27:28Z

query-graphs-js/src/querygraph.ts

-                const implemVertice = otherSubgraph.verticesForType(implemType.name)[0];
+            if (isInterfaceObject) {
+              const typeInSupergraph = supergraph.type(type.name);
+              assert(typeInSupergraph && isInterfaceType(typeInSupergraph), () => `Type ${type} is an interfaceObject in subgraph ${i}; should be an interface in the supergraph`);


Assuming that these get wrapped in an Error further up?

I'm not sure to understand what "these" refers to. But assert throws an Error if it ever does anything, so unsure why there would need to be additional wrapping further up?

clenfest · 2023-03-28T21:39:49Z

gateway-js/src/dataRewrites.ts

+    return;
+  }
+
+  if (typeof value !== 'object') {


I'm a little confused. Further down, we seem to be treating value as a Record<string,any>. Wouldn't this cause that case to exit here? I'm probably missing something.

If value is a Record<string, any>, then it is an object, so it shouldn't exit since the condition exits if it is not an object.

…faceObject` There is 2 main issues with the handling of `@interfaceObject` that this patch fixes: 1. given some `@interfaceObject` type `I` in some subgraph `S1`, if another subgraph `S2` was declaring an _implementation_ type `T` of `I` but _without_ declaring `I`, then the code missed creating an "edge" between `T` in `S2` and `I` in `S1`, which resulted in composition rejecting such cases, even though it should work. 2. there were an additional issue in the handling of `@requires` so that the "input rewrites" generated were not generic enough, leading to sometimes sending incorrect queries to subgraph (more precisely, we were sometimes not rewriting the typename when querying an `@interfaceObject` subgraph, leading that subgraph to complain that it is asked for type it doesn't know). Note that fixing those 2 issues surfaced the fact that the handling of "rewrites" in the gateway was not working exactly as it should have. More precisely, hen a path had a fragment `... on I`, if `I` was an interface, then we would no select objects that implements `I` correctly. As it happens, the router implementation of those rewrites was both cleaner and didn't had that issue, so this patch also updates the handling of "rewrites" to mimick what the router implementation does (fixing the issue, and overall cleaning the code). Fixes apollographql#2485

…faceObject` (#2494) There is 2 main issues with the handling of `@interfaceObject` that this patch fixes: 1. given some `@interfaceObject` type `I` in some subgraph `S1`, if another subgraph `S2` was declaring an _implementation_ type `T` of `I` but _without_ declaring `I`, then the code missed creating an "edge" between `T` in `S2` and `I` in `S1`, which resulted in composition rejecting such cases, even though it should work. 2. there were an additional issue in the handling of `@requires` so that the "input rewrites" generated were not generic enough, leading to sometimes sending incorrect queries to subgraph (more precisely, we were sometimes not rewriting the typename when querying an `@interfaceObject` subgraph, leading that subgraph to complain that it is asked for type it doesn't know). Note that fixing those 2 issues surfaced the fact that the handling of "rewrites" in the gateway was not working exactly as it should have. More precisely, hen a path had a fragment `... on I`, if `I` was an interface, then we would no select objects that implements `I` correctly. As it happens, the router implementation of those rewrites was both cleaner and didn't had that issue, so this patch also updates the handling of "rewrites" to mimick what the router implementation does (fixing the issue, and overall cleaning the code). Fixes #2485

pcmanus force-pushed the f2485-composition-error-itfObject branch from 892f37e to 40b6467 Compare March 23, 2023 10:48

pcmanus commented Mar 23, 2023

View reviewed changes

pcmanus mentioned this pull request Mar 23, 2023

The following supergraph API query: ... cannot be satisfied by the subgraphs because ... there is a more direct path in ... that avoids ... altogether #2485

Closed

pcmanus self-assigned this Mar 23, 2023

korinne requested review from sachindshinde and clenfest March 24, 2023 14:53

clenfest approved these changes Mar 28, 2023

View reviewed changes

pcmanus force-pushed the f2485-composition-error-itfObject branch 2 times, most recently from 4a3b499 to 2360d3c Compare March 29, 2023 16:51

pcmanus force-pushed the f2485-composition-error-itfObject branch from 2360d3c to c907e94 Compare March 30, 2023 06:55

pcmanus merged commit 450b957 into apollographql:main Mar 30, 2023

github-actions bot mentioned this pull request Mar 30, 2023

release: on branch main #2496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrectly rejected composition and subgraph issues with `@inter… #2494

Fix incorrectly rejected composition and subgraph issues with `@inter… #2494

pcmanus commented Mar 23, 2023

netlify bot commented Mar 23, 2023 •

edited

changeset-bot bot commented Mar 23, 2023 •

edited

codesandbox-ci bot commented Mar 23, 2023 •

edited

pcmanus Mar 23, 2023

clenfest Mar 28, 2023

pcmanus Mar 29, 2023

clenfest Mar 28, 2023

clenfest Mar 28, 2023

pcmanus Mar 29, 2023

clenfest Mar 28, 2023

clenfest Mar 28, 2023

pcmanus Mar 29, 2023

clenfest Mar 28, 2023

pcmanus Mar 29, 2023

pcmanus Mar 29, 2023

	Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occurs
	Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occur

Fix incorrectly rejected composition and subgraph issues with `@inter… #2494

Fix incorrectly rejected composition and subgraph issues with `@inter… #2494

Conversation

pcmanus commented Mar 23, 2023

netlify bot commented Mar 23, 2023 • edited

👷 Deploy request for apollo-federation-docs pending review.

changeset-bot bot commented Mar 23, 2023 • edited

🦋 Changeset detected

codesandbox-ci bot commented Mar 23, 2023 • edited

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 Mar 23, 2023 •

edited

changeset-bot bot commented Mar 23, 2023 •

edited

codesandbox-ci bot commented Mar 23, 2023 •

edited