Avoid incomplete subgraph when extracting from supergraph #1511
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Due to imprecisions of the fed1 join spec, the code handling supergraph sometimes initially assume a type is in all subgraph to remove that type later from some subgraphs as it realises it cannot have been in that subgraph. As we remove types that way, we have to remove references to it as well, and this was done correctly for object types through `removeRecursive`, but for union types, `remove` was used instead. This fix imply ensure `removeRecursive` is used there as well.
|
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. |
abernix
approved these changes
Feb 14, 2022
There was a problem hiding this comment.
The justification here makes sense to me and this commit is also on #1510 which has been reviewed separately. Landing this will unblock a use-case in the Router and we're going to try to cut a release of that soon-ish (today?).
I will follow-up this with a release myself. (And add a CHANGELOG there)
|
|
||
| test('handles unions types having no members in a subgraph correctly', () => { | ||
| /* | ||
| * The following supergraph has been generated on version-0.x from: |
There was a problem hiding this comment.
Would this have a different comment if it were generated by main?
abernix
added a commit
to apollographql/federation-rs
that referenced
this pull request
Feb 14, 2022
This was referenced Feb 14, 2022
abernix
added a commit
to apollographql/federation-rs
that referenced
this pull request
Feb 14, 2022
* router-bridge: Update to `@apollo/query-planner@2.0.0-alpha.6` Most notably to bring in apollographql/federation#1511 Ref: apollographql/federation@ee84b3fd9df161c3a94 * tests: update snapshots to include `operationKind` This is expected since the [Ref]erenced PR introduced it! Ref: apollographql/federation#1427
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a small followup to #1351: the patch removed object/interface types recursively as it should, but typo-ed union types only calling
removeinstead ofremoveRecursive.I didn't notice it at the time because while I added a union type test, it wasn't actually exercise the part of the code I though it was.
This PR fixes that problem and adds a proper test (keeping the other union test because while it didn't test what I though it was, it still tests something useful (namely that the
removeRecursiveon object types correctly recurse to unions)).