Skip to content

Commit

Permalink
Fix issues with named fragment handling code (#2619)
Browse files Browse the repository at this point in the history
The naming of `trimUnsatisfiableBranches` was misleading because it
does not just "remove unsatisfiable branches", it more generally remove
things that are not-very-optimal (even if they are not full branches).
So this rename this method to `normalize`, which more clearly explains
then intent (on top of being shorter).

Additionally, the method was not actually removing unsatisfiable
branches in all cases, which made it a bit error prone to use in
practice and led to some small inefficiencies in the
`NamedFragmentDefinition.selectionAtType` method. So fixed that,
making the method more regular and fixing the aforementioned
inefficiency.

This also force a normalisation after `expandAllFragments` because
we were relying on it in practice so this is less fragile this way.

Also, the QP code never truly rely on operation validation as it is assumed
the operation passed to it have already been validated prior to being
passed to the planner, but having the added validation helps a bit in
other testing context when that pre-validation hasn't happened, and
since this is fairly cheap...

Last but not least, the code for normalizing fields was not taking into account that if we
normalize on a more precise type, this may make a field definition "more
precise", which can impacts the type we should use to normalize the
sub-selection of said field.
  • Loading branch information
Sylvain Lebresne committed Jun 13, 2023
1 parent 2a97f37 commit 7f1ef73
Show file tree
Hide file tree
Showing 9 changed files with 1,140 additions and 317 deletions.
10 changes: 10 additions & 0 deletions .changeset/nice-cheetahs-yell.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"@apollo/query-planner": patch
"@apollo/federation-internals": patch
---

Fix issues in code to reuse named fragments. One of the fixed issue would manifest as an assertion error with a message
looking like `Cannot add fragment of condition X (...) to parent type Y (...)`. Another would manifest itself by
generating an invalid subgraph fetch where a field conflicts with another version of that field that is in a reused
named fragment.

0 comments on commit 7f1ef73

Please sign in to comment.