Skip to content

Commit

Permalink
Fix __typename rebasing for interface objects (#2886)
Browse files Browse the repository at this point in the history
We should never query `__typename` from a subgraph for an object type
marked `@interfaceObject`, as the value we get back will always be
wrong. Normally, we prevent this by not having a `__typename` edge in
the federated query graph.

However, during the optimization where we try to reuse existing named
fragments, we rebase them from the API schema onto the subgraph schemas.
We usually ignore selections where this leads to invalidity (e.g. fields
or types that don't exist in the subgraph schema), but the code doesn't
currently view the `__typename` field being rebased onto an interface
object as invalid. This has led to bugs where named fragments containing
`__typename` accidentally cause it to be queried on interface objects in
subgraph queries.

This PR changes `Field.rebaseOn()` such that `__typename` being rebased
onto a parent type that is an interface object is considered invalid.
Additionally, if the parent type is an abstract type, and that abstract
type could possibly be an interface object type at runtime, then this is
additionally considered invalid.
  • Loading branch information
sachindshinde authored Dec 8, 2023
1 parent 7b5b836 commit 74ca7dd
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 2 deletions.
5 changes: 5 additions & 0 deletions .changeset/dry-seahorses-grin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/federation-internals": patch
---

Fix query planning bug where `__typename` on interface object types in named fragments can cause query plan execution to fail. ([#2886](https://github.com/apollographql/federation/issues/2886))
60 changes: 59 additions & 1 deletion internals-js/src/__tests__/operations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
SchemaRootKind,
} from '../../dist/definitions';
import { buildSchema } from '../../dist/buildSchema';
import { FederationBlueprint } from '../../dist/federation';
import { FragmentRestrictionAtType, MutableSelectionSet, NamedFragmentDefinition, Operation, operationFromDocument, parseOperation } from '../../dist/operations';
import './matchers';
import { DocumentNode, FieldNode, GraphQLError, Kind, OperationDefinitionNode, OperationTypeNode, parse, SelectionNode, SelectionSetNode, validate } from 'graphql';
Expand Down Expand Up @@ -2798,7 +2799,7 @@ describe('basic operations', () => {
`);
});
});

describe('same fragment merging', () => {
test('do merge when same fragment and no directive', () => {
const operation = operationFromDocument(schema, gql`
Expand Down Expand Up @@ -3514,6 +3515,63 @@ describe('named fragment rebasing on subgraphs', () => {
`);
});

test('it skips __typename field for types that are potentially interface objects at runtime', () => {
const schema = parseSchema(`
type Query {
i: I
}
interface I {
id: ID!
x: String!
}
`);

const operation = parseOperation(schema, `
query {
i {
...FragOnI
}
}
fragment FragOnI on I {
__typename
id
x
}
`);

const fragments = operation.fragments;
assert(fragments, 'Should have some fragments');

const subgraph = buildSchema(`
extend schema
@link(
url: "https://specs.apollo.dev/federation/v2.5",
import: [{ name: "@interfaceObject" }, { name: "@key" }]
)
type Query {
i: I
}
type I @interfaceObject @key(fields: "id") {
id: ID!
x: String!
}
`,
{ blueprint: new FederationBlueprint(true) },
);

const rebased = fragments.rebaseOn(subgraph);
expect(rebased?.toString('')).toMatchString(`
fragment FragOnI on I {
id
x
}
`);
});

test('it skips fragments with no selection or trivial ones applying', () => {
const schema = parseSchema(`
type Query {
Expand Down
10 changes: 9 additions & 1 deletion internals-js/src/operations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,15 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
}

if (this.name === typenameFieldName) {
return this.withUpdatedDefinition(parentType.typenameField()!);
if (possibleRuntimeTypes(parentType).some((runtimeType) => isInterfaceObjectType(runtimeType))) {
validate(
!errorIfCannotRebase,
() => `Cannot add selection of field "${this.definition.coordinate}" to selection set of parent type "${parentType}" that is potentially an interface object type at runtime`
);
return undefined;
} else {
return this.withUpdatedDefinition(parentType.typenameField()!);
}
}

const fieldDef = parentType.field(this.name);
Expand Down

0 comments on commit 74ca7dd

Please sign in to comment.