Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(query-planner): handle multiple __typename selections #3164

Merged
merged 4 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
// 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)));

Check warning on line 2120 in internals-js/src/operations.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/operations.ts#L2120

Added line #L2120 was not covered by tests
} else {
return this._selections;
}
Expand Down Expand Up @@ -3053,6 +3053,13 @@
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;

Check warning on line 3060 in internals-js/src/operations.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/operations.ts#L3059-L3060

Added lines #L3059 - L3060 were not covered by tests
}

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