Fix errors reading fed1 supergraphs #1351
Merged
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
queerviolet
approved these changes
Jan 6, 2022
There was a problem hiding this comment.
code looks good! would like a few more tests:
- a negative test (ensure that types with remaining fields are not removed) mainly for documentation (i suspect many things would break if that functionality were broken)
- testing the recursive removal of empty unions, fields, and the other cases we handle
| @@ -0,0 +1,110 @@ | |||
| import { buildSupergraphSchema, extractSubgraphsFromSupergraph } from ".."; | |||
|
|
|||
| test('handles types having no fields in a subgraph correctly', () => { | |||
There was a problem hiding this comment.
could we add another test which exercises the path where we remove fields in a subgraph but do not remove the whole type? mainly for documentation of the behavior—i expect this is exercised in other tests.
| ref.removeRecursive(); | ||
| break; | ||
| case 'UnionType': | ||
| if (ref.membersCount() === 0) { |
There was a problem hiding this comment.
do we have tests that exercise this?
A fed1 supergraphs doesn't always have enough information to know in which subgraph a type is defined, so when extracting a subgraph from the supergraph, we sometime "optimistically" add a type to that subgraph, but later have to remove it if it turns out the type has no fields in that subgraph. The issue was that this type was sometimes referenced and removing it left the references invalid. Instead, this patch ensures those references are also removed (we can infer those were also added mistakenly due to a lack of information). Additionally, the commit slightly improves the handling of errors happing while extracting subgraphs from the supergraph, providing clearer messages and, for debugging, allows to dump the extracted subgraph (but only when an `APOLLO_FEDERATION_DEBUG_SUBGRAPHS` env variable is set to avoid surprises).
pcmanus
force-pushed
the
fed1-supergraph-errors
branch
from
9ce7188
to
1bc13f7
Compare
|
Thanks for the review. Pushed a few more tests indeed. |
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.
A fed1 supergraphs doesn't always have enough information to know in
which subgraph a type is defined, so when extracting a subgraph from
the supergraph, we sometime "optimistically" add a type to that
subgraph, but later have to remove it if it turns out the type has
no fields in that subgraph. The issue was that this type was sometimes
referenced and removing it left the references invalid. Instead, this
patch ensures those references are also removed (we can infer those
were also added mistakenly due to a lack of information).
Additionally, the commit slightly improves the handling of errors
happing while extracting subgraphs from the supergraph, providing
clearer messages and, for debugging, allows to dump the extracted
subgraph (but only when an
APOLLO_FEDERATION_DEBUG_SUBGRAPHSenvvariable is set to avoid surprises).