-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix incorrectly rejected composition and subgraph issues with `@inter… #2494
Fix incorrectly rejected composition and subgraph issues with `@inter… #2494
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: c907e94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
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. |
892f37e
to
40b6467
Compare
@@ -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[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
internals-js/src/operations.ts
Outdated
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing it, you don't specify the third case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.changeset/hungry-berries-destroy.md
Outdated
"@apollo/gateway": patch | ||
--- | ||
|
||
Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occurs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that it doesn't declare @interfaceObject
for I
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
query-graphs-js/src/querygraph.ts
Outdated
const otherVertice = otherVertices[0]; | ||
// The edge goes from the otherSubgraph to the current one. | ||
const head = copyPointers[j].copiedVertex(otherVertice); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
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`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that these get wrapped in an Error further up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
return; | ||
} | ||
|
||
if (typeof value !== 'object') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
4a3b499
to
2360d3c
Compare
…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
2360d3c
to
c907e94
Compare
…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
…faceObject`
There is 2 main issues with the handling of
@interfaceObject
that this patch fixes:@interfaceObject
typeI
in some subgraphS1
, if another subgraphS2
was declaring an implementation typeT
ofI
but without declaringI
, then the code missed creating an "edge" betweenT
inS2
andI
inS1
, which resulted in composition rejecting such cases, even though it should work.@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
, ifI
was an interface, then we would no select objects that implementsI
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