Skip to content

Commit f6a8c1c

Browse files
author
Sylvain Lebresne
authored
Fix cases where fragment could be reused but weren't (#2541)
The main case that was handled properly was the case where a fragment that is on an abstract type is applied somewhere where the "current type" is an object type. In that case, some sub-parts (the one that don't match the "curren type") of the fragment are effectively "dead branches", but the code was still trying to matching those in the result, preventing some reuse of the fragment. Another smaller issue was that we sometimes weren't correctly reusing fragments at the very top of a "selection set". This didn't really matter too much for queries in practice, but this was also impacting some case where fragments where use at the top level of some other fragments, also preventing a few reuse. Lastly, fixing this highlighted the fact that the we were always using fragments from the original query, so against the API schema, even when building subgraph queries, and this was creating issues (the code was trying to compensate for this is a few places, but this was confusing and was not covering all cases correctly). So the patch clean this up too, and rebased fragments on subgraph before trying to use them for optimizing said subgraph fetches, ensuring that the code don't mix element from different schema.
1 parent ec1ac9c commit f6a8c1c

File tree

4 files changed

+394
-89
lines changed

4 files changed

+394
-89
lines changed

.changeset/six-ants-poke.md

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
"@apollo/query-planner": patch
3+
"@apollo/federation-internals": patch
4+
---
5+
6+
Improves the heuristics used to try to reuse the query named fragments in subgraph fetches. Said fragment will be reused
7+
more often, which can lead to smaller subgraph queries (and hence overall faster processing).
8+

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

+175-10
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,6 @@ describe('fragments optimization', () => {
154154
`);
155155

156156
const optimized = withoutFragments.optimize(operation.selectionSet.fragments!);
157-
// Note that while we didn't use `onU` for `t` in the query, it's technically ok to use
158-
// it and it makes the query smaller, so it gets used.
159157
expect(optimized.toString()).toMatchString(`
160158
fragment OnT1 on T1 {
161159
a
@@ -171,17 +169,15 @@ describe('fragments optimization', () => {
171169
b
172170
}
173171
174-
fragment OnU on U {
175-
...OnI
176-
...OnT1
177-
...OnT2
178-
}
179-
180172
{
181173
t {
182-
...OnU
174+
...OnI
175+
...OnT1
176+
...OnT2
183177
u {
184-
...OnU
178+
...OnI
179+
...OnT1
180+
...OnT2
185181
}
186182
}
187183
}
@@ -579,6 +575,175 @@ describe('fragments optimization', () => {
579575
});
580576
});
581577

578+
test('handles fragment matching at the top level of another fragment', () => {
579+
const schema = parseSchema(`
580+
type Query {
581+
t: T
582+
}
583+
584+
type T {
585+
a: String
586+
u: U
587+
}
588+
589+
type U {
590+
x: String
591+
y: String
592+
}
593+
`);
594+
595+
testFragmentsRoundtrip({
596+
schema,
597+
query: `
598+
fragment Frag1 on T {
599+
a
600+
}
601+
602+
fragment Frag2 on T {
603+
u {
604+
x
605+
y
606+
}
607+
...Frag1
608+
}
609+
610+
fragment Frag3 on Query {
611+
t {
612+
...Frag2
613+
}
614+
}
615+
616+
{
617+
...Frag3
618+
}
619+
`,
620+
expanded: `
621+
{
622+
t {
623+
u {
624+
x
625+
y
626+
}
627+
a
628+
}
629+
}
630+
`,
631+
});
632+
});
633+
634+
test('handles fragments used in a context where they get trimmed', () => {
635+
const schema = parseSchema(`
636+
type Query {
637+
t1: T1
638+
}
639+
640+
interface I {
641+
x: Int
642+
}
643+
644+
type T1 implements I {
645+
x: Int
646+
y: Int
647+
}
648+
649+
type T2 implements I {
650+
x: Int
651+
z: Int
652+
}
653+
`);
654+
655+
testFragmentsRoundtrip({
656+
schema,
657+
query: `
658+
fragment FragOnI on I {
659+
... on T1 {
660+
y
661+
}
662+
... on T2 {
663+
z
664+
}
665+
}
666+
667+
{
668+
t1 {
669+
...FragOnI
670+
}
671+
}
672+
`,
673+
expanded: `
674+
{
675+
t1 {
676+
y
677+
}
678+
}
679+
`,
680+
});
681+
});
682+
683+
test('handles fragments used in the context of non-intersecting abstract types', () => {
684+
const schema = parseSchema(`
685+
type Query {
686+
i2: I2
687+
}
688+
689+
interface I1 {
690+
x: Int
691+
}
692+
693+
interface I2 {
694+
y: Int
695+
}
696+
697+
interface I3 {
698+
z: Int
699+
}
700+
701+
type T1 implements I1 & I2 {
702+
x: Int
703+
y: Int
704+
}
705+
706+
type T2 implements I1 & I3 {
707+
x: Int
708+
z: Int
709+
}
710+
`);
711+
712+
testFragmentsRoundtrip({
713+
schema,
714+
query: `
715+
fragment FragOnI1 on I1 {
716+
... on I2 {
717+
y
718+
}
719+
... on I3 {
720+
z
721+
}
722+
}
723+
724+
{
725+
i2 {
726+
...FragOnI1
727+
}
728+
}
729+
`,
730+
expanded: `
731+
{
732+
i2 {
733+
... on I1 {
734+
... on I2 {
735+
y
736+
}
737+
... on I3 {
738+
z
739+
}
740+
}
741+
}
742+
}
743+
`,
744+
});
745+
});
746+
582747
describe('applied directives', () => {
583748
test('reuse fragments with directives on the fragment, but only when there is those directives', () => {
584749
const schema = parseSchema(`

0 commit comments

Comments
 (0)