Skip to content

Commit 4064c07

Browse files
author
Sylvain Lebresne
authored
Fix bug in patch for #2137 (#2140)
The code from #2137 is optimizing some use of __typename when other fields are queried alongside it. But when __typename had only inline fragments alongside it, the logic was flawed and was discarding any potential instance of the optimization done somewhere more nested. This fixes that problem. Additionally, this patch adds a test for the optimization of #2137, but to do that, this patch adds a new behaviour to the query planner whereby along the generation of the plan, it also exposes some statistics on the plan generation. As of this commit, the only thing exposed is the number of plan that were evaluated under the hood, as that is what we care to check here (but it is also one of the main contributor to time spent query planning, so arguably an important statistic).
1 parent c99de57 commit 4064c07

File tree

4 files changed

+223
-20
lines changed

4 files changed

+223
-20
lines changed

query-planner-js/CHANGELOG.md

+4-1
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
44

55
## vNext
66

7+
- Fix issue with path #2137 (optimization for `__typename`) [PR #2140](https://github.com/apollographql/federation/pull/2140).
8+
79
## 2.1.2-alpha.1
8-
- Fix potential inefficient planning due to __typename [PR #2137](https://github.com/apollographql/federation/pull/2137).
10+
11+
- Fix potential inefficient planning due to `__typename` [PR #2137](https://github.com/apollographql/federation/pull/2137).
912
- Fix potential assertion during query planning [PR #2133](https://github.com/apollographql/federation/pull/2133).
1013

1114
## 2.1.2-alpha.0

query-planner-js/src/__tests__/buildPlan.test.ts

+153
Original file line numberDiff line numberDiff line change
@@ -4055,4 +4055,157 @@ describe('__typename handling', () => {
40554055
}
40564056
`);
40574057
});
4058+
4059+
it('does not needlessly consider options for __typename', () => {
4060+
const subgraph1 = {
4061+
name: 'Subgraph1',
4062+
typeDefs: gql`
4063+
type Query {
4064+
s: S
4065+
}
4066+
4067+
type S @key(fields: "id") {
4068+
id: ID
4069+
}
4070+
`
4071+
}
4072+
4073+
const subgraph2 = {
4074+
name: 'Subgraph2',
4075+
typeDefs: gql`
4076+
type S @key(fields: "id") {
4077+
id: ID
4078+
t: T @shareable
4079+
}
4080+
4081+
type T @key(fields: "id") {
4082+
id: ID!
4083+
x: Int
4084+
}
4085+
`
4086+
}
4087+
4088+
const subgraph3 = {
4089+
name: 'Subgraph3',
4090+
typeDefs: gql`
4091+
type S @key(fields: "id") {
4092+
id: ID
4093+
t: T @shareable
4094+
}
4095+
4096+
type T @key(fields: "id") {
4097+
id: ID!
4098+
y: Int
4099+
}
4100+
`
4101+
}
4102+
4103+
const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3);
4104+
// This tests the patch from https://github.com/apollographql/federation/pull/2137.
4105+
// Namely, the schema is such that `x` can only be fetched from one subgraph, but
4106+
// technically __typename can be fetched from 2 subgraphs. However, the optimization
4107+
// we test for is that we actually don't consider both choices for __typename and
4108+
// instead only evaluate a single query plan (the assertion on `evaluatePlanCount`)
4109+
let operation = operationFromDocument(api, gql`
4110+
query {
4111+
s {
4112+
t {
4113+
__typename
4114+
x
4115+
}
4116+
}
4117+
}
4118+
`);
4119+
4120+
let plan = queryPlanner.buildQueryPlan(operation);
4121+
expect(queryPlanner.lastGeneratedPlanStatistics()?.evaluatedPlanCount).toBe(1);
4122+
expect(plan).toMatchInlineSnapshot(`
4123+
QueryPlan {
4124+
Sequence {
4125+
Fetch(service: "Subgraph1") {
4126+
{
4127+
s {
4128+
__typename
4129+
id
4130+
}
4131+
}
4132+
},
4133+
Flatten(path: "s") {
4134+
Fetch(service: "Subgraph2") {
4135+
{
4136+
... on S {
4137+
__typename
4138+
id
4139+
}
4140+
} =>
4141+
{
4142+
... on S {
4143+
t {
4144+
__typename
4145+
x
4146+
}
4147+
}
4148+
}
4149+
},
4150+
},
4151+
},
4152+
}
4153+
`);
4154+
4155+
// Almost the same test, but we artificially create a case where the result set
4156+
// for `s` has a __typename alongside just an inline fragments. This should
4157+
// change nothing to the example (the __typename on `s` is trivially fetched
4158+
// from the 1st subgraph and does not create new choices), but an early bug
4159+
// in the implementation made this example forgo the optimization of the
4160+
// __typename within `t`. We make sure this is not case (that we still only
4161+
// consider a single choice of plan).
4162+
operation = operationFromDocument(api, gql`
4163+
query {
4164+
s {
4165+
__typename
4166+
... on S {
4167+
t {
4168+
__typename
4169+
x
4170+
}
4171+
}
4172+
}
4173+
}
4174+
`);
4175+
4176+
plan = queryPlanner.buildQueryPlan(operation);
4177+
expect(queryPlanner.lastGeneratedPlanStatistics()?.evaluatedPlanCount).toBe(1);
4178+
expect(plan).toMatchInlineSnapshot(`
4179+
QueryPlan {
4180+
Sequence {
4181+
Fetch(service: "Subgraph1") {
4182+
{
4183+
s {
4184+
__typename
4185+
id
4186+
}
4187+
}
4188+
},
4189+
Flatten(path: "s") {
4190+
Fetch(service: "Subgraph2") {
4191+
{
4192+
... on S {
4193+
__typename
4194+
id
4195+
}
4196+
} =>
4197+
{
4198+
... on S {
4199+
t {
4200+
__typename
4201+
x
4202+
}
4203+
}
4204+
}
4205+
},
4206+
},
4207+
},
4208+
}
4209+
`);
4210+
});
40584211
});

0 commit comments

Comments
 (0)