diff --git a/.changeset/brown-bats-float.md b/.changeset/brown-bats-float.md index 6d7cd257d..24afb1117 100644 --- a/.changeset/brown-bats-float.md +++ b/.changeset/brown-bats-float.md @@ -5,6 +5,7 @@ "@apollo/gateway": patch --- -Improves the performance of the code used to try to reuse query named fragments, thus improving query planning -performance (at least for queries having named fragments). - \ No newline at end of file +Re-work the code use to try to reuse query named fragments to improve performance (thus sometimes improving query +planning performance), to fix a possibly raised assertion error (with a message of form like `Cannot add selection of +field X to selection set of parent type Y`), and to fix a rare issue where an interface or union field was not being +queried for all the types it should be. diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 6e610cda4..a29f70bfe 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -697,7 +697,7 @@ export type RootOperationPath = { } // Computes for every fragment, which other fragments use it (so the reverse of it's dependencies, the other fragment it uses). -function computeFragmentsReverseDependencies(fragments: NamedFragments): SetMultiMap { +function computeFragmentsDependents(fragments: NamedFragments): SetMultiMap { const reverseDeps = new SetMultiMap(); for (const fragment of fragments.definitions()) { for (const dependency of fragment.fragmentUsages().keys()) { @@ -738,8 +738,8 @@ function clearKeptFragments( } // Checks, in `selectionSet`, which fragments (of `fragments`) are used at least `minUsagesToOptimize` times. -// Returns the updated set of fragments containing only the fragment definitions with usage above our treshold, -// and `undefined` or `null` if no such fragment meets said treshold. When this method returns `null`, it +// Returns the updated set of fragments containing only the fragment definitions with usage above our threshold, +// and `undefined` or `null` if no such fragment meets said threshold. When this method returns `null`, it // additionally means that no fragments are use at all in `selectionSet` (and so `undefined` means that // "some" fragments are used in `selectionSet`, but just none of them is used at least `minUsagesToOptimize` // times). @@ -793,7 +793,7 @@ function computeFragmentsToKeep( // F1 first, and then realize that this increases F2 usages to 2, which means we stop there and keep F2. // Generalizing this, it means we want to first pick up fragments to expand that are _not_ used by any // other fragments that may be expanded. - const reverseDependencies = computeFragmentsReverseDependencies(fragments); + const reverseDependencies = computeFragmentsDependents(fragments); // We'll add to `toExpand` fragment we will definitively expand. const toExpand = new Set; let shouldContinue = true; @@ -831,7 +831,7 @@ function computeFragmentsToKeep( // after that to see if something changes. shouldContinue = true; - // Now that we expand it, we should bump the usage or every fragment it uses. + // Now that we expand it, we should bump the usage for every fragment it uses. const nameUsages = fragments.get(name)!.fragmentUsages(); for (const [otherName, otherCount] of nameUsages.entries()) { const prev = usages.get(otherName); diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 46fdaa40f..166ec9231 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -2793,7 +2793,7 @@ export class QueryPlanner { } const reuseQueryFragments = this.config.reuseQueryFragments ?? true; - let fragments = operation.fragments + let fragments = operation.fragments; if (fragments && !fragments.isEmpty() && reuseQueryFragments) { // For all subgraph fetches we query `__typename` on every abstract types (see `FetchGroup.toPlanNode`) so if we want // to have a chance to reuse fragments, we should make sure those fragments also query `__typename` for every abstract type.