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.

Sign up for GitHub

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

Jump to bottom

Fix potential bug when an @interfaceObject has a @requires #2524

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Fix potential bug when an @interfaceObject has a @requires #2524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions .changeset/spotty-monkeys-clean.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is slightly different than I would expect. If the type is undefined in the new schema, why return the original type rather than, say, undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that was me being overly cautious and defaulting to what was essentially the prior behaviour before this method was added if something unexpected happened. But truly, we should always find the type or we're having a bug, so I ended up changing this to an assertion. Probably better to get a signal early if we introduce/have such a bug (returning undefined would push the problem to the caller, which in that case wouldn't have much better it can do than failing, so having an assertion directly kept the code simpler; this is all non-exported methods after all).

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