Skip to content

Commit

Permalink
Fix potential bug when an @interfaceObject has a @requires (#2524)
Browse files Browse the repository at this point in the history 
When an `@interfaceObject` type has a field with a `@requires` and the
query requests that field only for some specific implementations of the
corresponding interface, then the generated query plan was sometimes
invalid and could result in an invalid query to a subgraph (against a
subgraph that rely on `@apollo/subgraph`, this lead the subgraph to
produce an error message looking like `"The _entities resolver tried to
load an entity for type X, but no object or interface type of that name
was found in the schema"`).

The underlying reason is that the plan was mistakenly requesting the
required fields for any object of the interface (corresponding to the
`@interfaceObject` in question), instead of only requesting them only
for only the implementation types it needed to. Not only is that
inefficient in principle, but this lead to invalid fetches because the
"rewrite" logic used to fixup the `__typename` for `@interfaceObject`
under the hood was only rewriting the types of the implementation types
in question (only expecting those to need rewrite, which technically was
correct) but was mistakenly getting values for other implementation
types.
  • Loading branch information
Sylvain Lebresne authored Apr 12, 2023
1 parent 449351d commit 179b460
Show file tree
Hide file tree
Showing 4 changed files with 299 additions and 50 deletions.
13 changes: 13 additions & 0 deletions .changeset/spotty-monkeys-clean.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
"@apollo/query-planner": patch
"@apollo/federation-internals": patch
"@apollo/gateway": patch
---

Fix potential bug when an `@interfaceObject` type has a `@requires`. When an `@interfaceObject` type has a field with a
`@requires` and the query requests that field only for some specific implementations of the corresponding interface,
then the generated query plan was sometimes invalid and could result in an invalid query to a subgraph (against a
subgraph that rely on `@apollo/subgraph`, this lead the subgraph to produce an error message looking like `"The
_entities resolver tried to load an entity for type X, but no object or interface type of that name was found in the
schema"`).

245 changes: 245 additions & 0 deletions gateway-js/src/__tests__/executeQueryPlan.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4863,6 +4863,251 @@ describe('executeQueryPlan', () => {
}
`);
});

test('handles @requires on @interfaceObject that applies to only one of the queried implementation', async () => {
// The case this test is that where the @interfaceObject in s2 has a @requires, but the query we send requests the field on which
// there is this @require only for one of the implementation type, which it request another field with no require for another implementation.
// And we're making sure the requirements only get queried for T1, the first type.
const s1 = {
name: 's1',
typeDefs: gql`
extend schema
@link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key"])
type Query {
is: [I!]!
}
interface I @key(fields: "id") {
id: ID!
name: String
req: Req
}
type T1 implements I @key(fields: "id") {
id: ID!
name: String
req: Req
}
type T2 implements I @key(fields: "id") {
id: ID!
name: String
req: Req
}
type Req {
id: ID!
}
`,
resolvers: {
Query: {
is: () => [
{ __typename: 'T1', id: '2', name: 'e2', req: { id: 'r1'} },
{ __typename: 'T2', id: '4', name: 'e4', req: { id: 'r2'} },
{ __typename: 'T1', id: '1', name: 'e1', req: { id: 'r3'} }
]
},
}
}

const s2 = {
name: 's2',
typeDefs: gql`
extend schema
@link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key", "@interfaceObject", "@external", "@requires"])
type I @key(fields: "id") @interfaceObject {
id: ID!
req: Req @external
v: String! @requires(fields: "req { id }")
}
type Req {
id: ID! @external
}
`,
resolvers: {
I: {
__resolveReference(ref: any) {
return {
...ref,
v: `req=${ref.req.id}`
};
},
}
}
}

const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2 ]);

let operation = parseOp(`
{
is {
... on T1 {
v
}
... on T2 {
name
}
}
}
`, schema);

let queryPlan = buildPlan(operation, queryPlanner);
expect(queryPlan).toMatchInlineSnapshot(`
QueryPlan {
Sequence {
Fetch(service: "s1") {
{
is {
__typename
... on T1 {
__typename
id
req {
id
}
}
... on T2 {
name
}
}
}
},
Flatten(path: "is.@") {
Fetch(service: "s2") {
{
... on T1 {
__typename
id
}
... on I {
__typename
req {
id
}
}
} =>
{
... on I {
v
}
}
},
},
},
}
`);

let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
expect(response.errors).toBeUndefined();
expect(response.data).toMatchInlineSnapshot(`
Object {
"is": Array [
Object {
"v": "req=r1",
},
Object {
"name": "e4",
},
Object {
"v": "req=r3",
},
],
}
`);

// Sanity checking that if we ask for `v` (the field with @requires), then everything still works.
operation = parseOp(`
{
is {
... on T1 {
v
}
... on T2 {
v
name
}
}
}
`, schema);

global.console = require('console');
queryPlan = buildPlan(operation, queryPlanner);
expect(queryPlan).toMatchInlineSnapshot(`
QueryPlan {
Sequence {
Fetch(service: "s1") {
{
is {
__typename
... on T1 {
__typename
id
req {
id
}
}
... on T2 {
__typename
id
req {
id
}
name
}
}
}
},
Flatten(path: "is.@") {
Fetch(service: "s2") {
{
... on T1 {
__typename
id
}
... on I {
__typename
req {
id
}
}
... on T2 {
__typename
id
}
} =>
{
... on I {
v
}
}
},
},
},
}
`);

response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
expect(response.errors).toBeUndefined();
expect(response.data).toMatchInlineSnapshot(`
Object {
"is": Array [
Object {
"v": "req=r1",
},
Object {
"name": "e4",
"v": "req=r2",
},
Object {
"v": "req=r3",
},
],
}
`);
});
});

describe('fields with conflicting types needing aliasing', () => {
Expand Down
18 changes: 14 additions & 4 deletions internals-js/src/operations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ import {
} from "./definitions";
import { isInterfaceObjectType } from "./federation";
import { ERRORS } from "./error";
import { sameType } from "./types";
import { isSubtype, sameType } from "./types";
import { assert, isDefined, mapEntries, mapValues, MapWithCachedArrays, MultiMap, SetMultiMap } from "./utils";
import { argumentsEquals, argumentsFromAST, isValidValue, valueToAST, valueToString } from "./values";
import { v1 as uuidv1 } from 'uuid';
Expand Down Expand Up @@ -678,12 +678,12 @@ function isUselessFollowupElement(first: OperationElement, followup: OperationEl
: first.typeCondition;

// The followup is useless if it's a fragment (with no directives we would want to preserve) whose type
// is already that of the first element.
// is already that of the first element (or a supertype).
return !!typeOfFirst
&& followup.kind === 'FragmentElement'
&& !!followup.typeCondition
&& (followup.appliedDirectives.length === 0 || isDirectiveApplicationsSubset(conditionals, followup.appliedDirectives))
&& sameType(typeOfFirst, followup.typeCondition);
&& isSubtype(followup.typeCondition, typeOfFirst);
}

export type RootOperationPath = {
Expand Down Expand Up @@ -1598,9 +1598,19 @@ function addOneToKeyedUpdates(keyedUpdates: MultiMap<string, SelectionUpdate>, s
}
}

function maybeRebaseOnSchema(toRebase: CompositeType, schema: Schema): CompositeType {
if (toRebase.schema() === schema) {
return toRebase;
}

const rebased = schema.type(toRebase.name);
assert(rebased && isCompositeType(rebased), () => `Expected ${toRebase} to exists and be composite in the rebased schema, but got ${rebased?.kind}`);
return rebased;
}

function isUnecessaryFragment(parentType: CompositeType, fragment: FragmentSelection): boolean {
return fragment.element.appliedDirectives.length === 0
&& (!fragment.element.typeCondition || sameType(parentType, fragment.element.typeCondition));
&& (!fragment.element.typeCondition || isSubtype(maybeRebaseOnSchema(fragment.element.typeCondition, parentType.schema()), parentType));
}

function withUnecessaryFragmentsRemoved(
Expand Down
Loading

0 comments on commit 179b460

Please sign in to comment.