Skip to content

Commit

Permalink
fix(query-planner): handle multiple __typename selections (#3164)
Browse files Browse the repository at this point in the history
### Problem
The sibling typename optimization treated `__typename` and `__typename
@Skip(if:$X)` interchangeably. Thus, one could become a sibling of the
other, depending on the order they appear in query. That is problematic,
since sibling typename should not have directives or aliases.

Also, there was another bug in `selectionsInPrintOrder` function where
some __typename selections with directives could be dropped unexpectedly
if there are multiple __typename selections in the same selection set.

### Fix Summary
- fixed sibling typename optimization to only fold plain __typename
without alias/directives.
- fixed `selectionsInPrintOrder` to not drop duplicate __typename
selections with directives.
  • Loading branch information
duckki authored Oct 14, 2024
1 parent e00e1c9 commit 1c99cb0
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 4 deletions.
8 changes: 8 additions & 0 deletions .changeset/wise-yaks-switch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@apollo/federation-internals": patch
"@apollo/query-planner": patch
---

Fixed a bug that `__typename` with applied directives gets lost in fetch operations.

The sibling typename optimization used by query planner simplifies operations by folding `__typename` selections into their sibling selections. However, that optimization does not account for directives or aliases. The bug was applying the optimization even if the `__typename` has directives on it, which caused the selection to lose its directives. Now, `__typename` with directives (or aliases) are excluded from the optimization.
13 changes: 10 additions & 3 deletions internals-js/src/operations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2114,10 +2114,10 @@ export class SelectionSet {
// By default, we will print the selection the order in which things were added to it.
// If __typename is selected however, we put it first. It's a detail but as __typename is a bit special it looks better,
// and it happens to mimic prior behavior on the query plan side so it saves us from changing tests for no good reasons.
const isNonAliasedTypenameSelection = (s: Selection) => s.kind === 'FieldSelection' && !s.element.alias && s.element.name === typenameFieldName;
const typenameSelection = this._selections.find((s) => isNonAliasedTypenameSelection(s));
const isPlainTypenameSelection = (s: Selection) => s.kind === 'FieldSelection' && s.isPlainTypenameField();
const typenameSelection = this._selections.find((s) => isPlainTypenameSelection(s));
if (typenameSelection) {
return [typenameSelection].concat(this.selections().filter(s => !isNonAliasedTypenameSelection(s)));
return [typenameSelection].concat(this.selections().filter(s => !isPlainTypenameSelection(s)));
} else {
return this._selections;
}
Expand Down Expand Up @@ -3053,6 +3053,13 @@ export class FieldSelection extends AbstractSelection<Field<any>, undefined, Fie
return this.element.definition.name === typenameFieldName;
}

// Is this a plain simple __typename without any directive or alias?
isPlainTypenameField(): boolean {
return this.element.definition.name === typenameFieldName
&& this.element.appliedDirectives.length == 0
&& !this.element.alias;
}

withAttachment(key: string, value: string): FieldSelection {
const updatedField = this.element.copy();
updatedField.addAttachment(key, value);
Expand Down
69 changes: 69 additions & 0 deletions query-planner-js/src/__tests__/buildPlan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5749,6 +5749,75 @@ describe('__typename handling', () => {
`);
});

it('preserves __typename with a directive', () => {
const subgraph1 = {
name: 'Subgraph1',
typeDefs: gql`
type Query {
t: T
}
type T @key(fields: "id") {
id: ID!
}
`,
};

const [api, queryPlanner] = composeAndCreatePlanner(subgraph1);
// Especially, when there are multiple __typename selections.
let operation = operationFromDocument(
api,
gql`
query ($v: Boolean!) {
t {
__typename
__typename @skip(if: $v)
}
}
`,
);

let plan = queryPlanner.buildQueryPlan(operation);
expect(plan).toMatchInlineSnapshot(`
QueryPlan {
Fetch(service: "Subgraph1") {
{
t {
__typename
__typename @skip(if: $v)
}
}
},
}
`);

operation = operationFromDocument(
api,
gql`
query ($v: Boolean!) {
t {
__typename @skip(if: $v)
__typename
}
}
`,
);

plan = queryPlanner.buildQueryPlan(operation);
expect(plan).toMatchInlineSnapshot(`
QueryPlan {
Fetch(service: "Subgraph1") {
{
t {
__typename
__typename @skip(if: $v)
}
}
},
}
`);
});

it('does not needlessly consider options for __typename', () => {
const subgraph1 = {
name: 'Subgraph1',
Expand Down
2 changes: 1 addition & 1 deletion query-planner-js/src/buildPlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3425,7 +3425,7 @@ export class QueryPlanner {
if (
!typenameSelection
&& selection.kind === 'FieldSelection'
&& selection.element.name === typenameFieldName
&& selection.isPlainTypenameField()
&& !parentMaybeInterfaceObject
) {
// The reason we check for `!typenameSelection` is that due to aliasing, there can be more than one __typename selection
Expand Down

0 comments on commit 1c99cb0

Please sign in to comment.