Skip to content

Commit

Permalink
Fix cases where fragment could be reused but weren't (#2541)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Sylvain Lebresne committed May 3, 2023
1 parent ec1ac9c commit f6a8c1c
Show file tree
Hide file tree
Showing 4 changed files with 394 additions and 89 deletions.
8 changes: 8 additions & 0 deletions .changeset/six-ants-poke.md
@@ -0,0 +1,8 @@
---
"@apollo/query-planner": patch
"@apollo/federation-internals": patch
---

Improves the heuristics used to try to reuse the query named fragments in subgraph fetches. Said fragment will be reused
more often, which can lead to smaller subgraph queries (and hence overall faster processing).

185 changes: 175 additions & 10 deletions internals-js/src/__tests__/operations.test.ts
Expand Up @@ -154,8 +154,6 @@ describe('fragments optimization', () => {
`);

const optimized = withoutFragments.optimize(operation.selectionSet.fragments!);
// Note that while we didn't use `onU` for `t` in the query, it's technically ok to use
// it and it makes the query smaller, so it gets used.
expect(optimized.toString()).toMatchString(`
fragment OnT1 on T1 {
a
Expand All @@ -171,17 +169,15 @@ describe('fragments optimization', () => {
b
}
fragment OnU on U {
...OnI
...OnT1
...OnT2
}
{
t {
...OnU
...OnI
...OnT1
...OnT2
u {
...OnU
...OnI
...OnT1
...OnT2
}
}
}
Expand Down Expand Up @@ -579,6 +575,175 @@ describe('fragments optimization', () => {
});
});

test('handles fragment matching at the top level of another fragment', () => {
const schema = parseSchema(`
type Query {
t: T
}
type T {
a: String
u: U
}
type U {
x: String
y: String
}
`);

testFragmentsRoundtrip({
schema,
query: `
fragment Frag1 on T {
a
}
fragment Frag2 on T {
u {
x
y
}
...Frag1
}
fragment Frag3 on Query {
t {
...Frag2
}
}
{
...Frag3
}
`,
expanded: `
{
t {
u {
x
y
}
a
}
}
`,
});
});

test('handles fragments used in a context where they get trimmed', () => {
const schema = parseSchema(`
type Query {
t1: T1
}
interface I {
x: Int
}
type T1 implements I {
x: Int
y: Int
}
type T2 implements I {
x: Int
z: Int
}
`);

testFragmentsRoundtrip({
schema,
query: `
fragment FragOnI on I {
... on T1 {
y
}
... on T2 {
z
}
}
{
t1 {
...FragOnI
}
}
`,
expanded: `
{
t1 {
y
}
}
`,
});
});

test('handles fragments used in the context of non-intersecting abstract types', () => {
const schema = parseSchema(`
type Query {
i2: I2
}
interface I1 {
x: Int
}
interface I2 {
y: Int
}
interface I3 {
z: Int
}
type T1 implements I1 & I2 {
x: Int
y: Int
}
type T2 implements I1 & I3 {
x: Int
z: Int
}
`);

testFragmentsRoundtrip({
schema,
query: `
fragment FragOnI1 on I1 {
... on I2 {
y
}
... on I3 {
z
}
}
{
i2 {
...FragOnI1
}
}
`,
expanded: `
{
i2 {
... on I1 {
... on I2 {
y
}
... on I3 {
z
}
}
}
}
`,
});
});

describe('applied directives', () => {
test('reuse fragments with directives on the fragment, but only when there is those directives', () => {
const schema = parseSchema(`
Expand Down

0 comments on commit f6a8c1c

Please sign in to comment.