Skip to content

Commit afde315

Browse files
author
Sylvain Lebresne
authored
Fix assertion error due to not ignoring some unsatisfiable branches (#2486)
Code was added in #2449 which "optimize" selections by removing unecessary inline fragments and branches that can't be satisfied (when we cross type conditions so that the intersection of types leading here is empty). Unfortunately that code misses some cases of when a branch is impossible, which leads to trying to build a selection set that is invalid and this in turn triggers an assertion error.
1 parent 4175093 commit afde315

File tree

3 files changed

+69
-5
lines changed

3 files changed

+69
-5
lines changed

.changeset/lovely-walls-sneeze.md

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@apollo/federation-internals": patch
3+
---
4+
5+
Fix assertion error during query planning in some cases where queries has some unsatisfiable branches (a part of the
6+
query goes through type conditions that no runtime types satisfies).
7+

internals-js/src/__tests__/operations.test.ts

+57-1
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,6 @@ describe('basic operations', () => {
477477
})
478478
});
479479

480-
481480
describe('MutableSelectionSet', () => {
482481
test('memoizer', () => {
483482
const schema = parseSchema(`
@@ -546,3 +545,60 @@ describe('MutableSelectionSet', () => {
546545
expect(sets).toStrictEqual(['{}', '{ t { v1 } }', '{ t { v1 v3 } }', '{ t { v1 v3 v2 } }', '{ t { v1 v3 v4 } }']);
547546
});
548547
});
548+
549+
describe('unsatisfiable branches removal', () => {
550+
const schema = parseSchema(`
551+
type Query {
552+
i: I
553+
j: J
554+
}
555+
556+
interface I {
557+
a: Int
558+
b: Int
559+
}
560+
561+
interface J {
562+
b: Int
563+
}
564+
565+
type T1 implements I & J {
566+
a: Int
567+
b: Int
568+
c: Int
569+
}
570+
571+
type T2 implements I {
572+
a: Int
573+
b: Int
574+
d: Int
575+
}
576+
577+
type T3 implements J {
578+
a: Int
579+
b: Int
580+
d: Int
581+
}
582+
`);
583+
584+
const withoutUnsatisfiableBranches = (op: string) => {
585+
return parseOperation(schema, op).trimUnsatisfiableBranches().toString(false, false)
586+
};
587+
588+
589+
it.each([
590+
'{ i { a } }',
591+
'{ i { ... on T1 { a b c } } }',
592+
])('is identity if there is no unsatisfiable branches', (op) => {
593+
expect(withoutUnsatisfiableBranches(op)).toBe(op);
594+
});
595+
596+
it.each([
597+
{ input: '{ i { ... on I { a } } }', output: '{ i { a } }' },
598+
{ input: '{ i { ... on T1 { ... on I { a b } } } }', output: '{ i { ... on T1 { a b } } }' },
599+
{ input: '{ i { ... on I { a ... on T2 { d } } } }', output: '{ i { a ... on T2 { d } } }' },
600+
{ input: '{ i { ... on T2 { ... on I { a ... on J { b } } } } }', output: '{ i { ... on T2 { a } } }' },
601+
])('removes unsatisfiable branches', ({input, output}) => {
602+
expect(withoutUnsatisfiableBranches(input)).toBe(output);
603+
});
604+
});

internals-js/src/operations.ts

+5-4
Original file line numberDiff line numberDiff line change
@@ -2272,10 +2272,12 @@ class InlineFragmentSelection extends FragmentSelection {
22722272
// If the current type is an object, then we never need to keep the current fragment because:
22732273
// - either the fragment is also an object, but we've eliminated the case where the 2 types are the same,
22742274
// so this is just an unsatisfiable branch.
2275-
// - or it's not an object, but then the current type is more precise and no poitn in "casting" to a
2276-
// less precise interface/union.
2275+
// - or it's not an object, but then the current type is more precise and no point in "casting" to a
2276+
// less precise interface/union. And if the current type is not even a valid runtime of said interface/union,
2277+
// then we should completely ignore the branch (or, since we're eliminating `thisCondition`, we would be
2278+
// building an invalid selection).
22772279
if (isObjectType(currentType)) {
2278-
if (isObjectType(thisCondition)) {
2280+
if (isObjectType(thisCondition) || !possibleRuntimeTypes(thisCondition).includes(currentType)) {
22792281
return undefined;
22802282
} else {
22812283
const trimmed = this.selectionSet.trimUnsatisfiableBranches(currentType);
@@ -2340,7 +2342,6 @@ class InlineFragmentSelection extends FragmentSelection {
23402342
return this.selectionSet === trimmedSelectionSet ? this : this.withUpdatedSelectionSet(trimmedSelectionSet);
23412343
}
23422344

2343-
23442345
expandAllFragments(): FragmentSelection {
23452346
return this.mapToSelectionSet((s) => s.expandAllFragments());
23462347
}

0 commit comments

Comments
 (0)