Skip to content

Commit

Permalink
Fix bug in patch for apollographql#2137 (apollographql#2140)
Browse files Browse the repository at this point in the history
The code from apollographql#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 apollographql#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).
  • Loading branch information
Sylvain Lebresne authored Sep 13, 2022
1 parent c99de57 commit 4064c07
Show file tree
Hide file tree
Showing 4 changed files with 223 additions and 20 deletions.
5 changes: 4 additions & 1 deletion query-planner-js/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

## vNext

- Fix issue with path #2137 (optimization for `__typename`) [PR #2140](https://github.com/apollographql/federation/pull/2140).

## 2.1.2-alpha.1
- Fix potential inefficient planning due to __typename [PR #2137](https://github.com/apollographql/federation/pull/2137).

- Fix potential inefficient planning due to `__typename` [PR #2137](https://github.com/apollographql/federation/pull/2137).
- Fix potential assertion during query planning [PR #2133](https://github.com/apollographql/federation/pull/2133).

## 2.1.2-alpha.0
Expand Down
153 changes: 153 additions & 0 deletions query-planner-js/src/__tests__/buildPlan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4055,4 +4055,157 @@ describe('__typename handling', () => {
}
`);
});

it('does not needlessly consider options for __typename', () => {
const subgraph1 = {
name: 'Subgraph1',
typeDefs: gql`
type Query {
s: S
}
type S @key(fields: "id") {
id: ID
}
`
}

const subgraph2 = {
name: 'Subgraph2',
typeDefs: gql`
type S @key(fields: "id") {
id: ID
t: T @shareable
}
type T @key(fields: "id") {
id: ID!
x: Int
}
`
}

const subgraph3 = {
name: 'Subgraph3',
typeDefs: gql`
type S @key(fields: "id") {
id: ID
t: T @shareable
}
type T @key(fields: "id") {
id: ID!
y: Int
}
`
}

const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3);
// This tests the patch from https://github.com/apollographql/federation/pull/2137.
// Namely, the schema is such that `x` can only be fetched from one subgraph, but
// technically __typename can be fetched from 2 subgraphs. However, the optimization
// we test for is that we actually don't consider both choices for __typename and
// instead only evaluate a single query plan (the assertion on `evaluatePlanCount`)
let operation = operationFromDocument(api, gql`
query {
s {
t {
__typename
x
}
}
}
`);

let plan = queryPlanner.buildQueryPlan(operation);
expect(queryPlanner.lastGeneratedPlanStatistics()?.evaluatedPlanCount).toBe(1);
expect(plan).toMatchInlineSnapshot(`
QueryPlan {
Sequence {
Fetch(service: "Subgraph1") {
{
s {
__typename
id
}
}
},
Flatten(path: "s") {
Fetch(service: "Subgraph2") {
{
... on S {
__typename
id
}
} =>
{
... on S {
t {
__typename
x
}
}
}
},
},
},
}
`);

// Almost the same test, but we artificially create a case where the result set
// for `s` has a __typename alongside just an inline fragments. This should
// change nothing to the example (the __typename on `s` is trivially fetched
// from the 1st subgraph and does not create new choices), but an early bug
// in the implementation made this example forgo the optimization of the
// __typename within `t`. We make sure this is not case (that we still only
// consider a single choice of plan).
operation = operationFromDocument(api, gql`
query {
s {
__typename
... on S {
t {
__typename
x
}
}
}
}
`);

plan = queryPlanner.buildQueryPlan(operation);
expect(queryPlanner.lastGeneratedPlanStatistics()?.evaluatedPlanCount).toBe(1);
expect(plan).toMatchInlineSnapshot(`
QueryPlan {
Sequence {
Fetch(service: "Subgraph1") {
{
s {
__typename
id
}
}
},
Flatten(path: "s") {
Fetch(service: "Subgraph2") {
{
... on S {
__typename
id
}
} =>
{
... on S {
t {
__typename
x
}
}
}
},
},
},
}
`);
});
});
Loading

0 comments on commit 4064c07

Please sign in to comment.