Skip to content

Commit

Permalink
fix(federation): prevent execution if query plan fails
Browse files Browse the repository at this point in the history
  • Loading branch information
ardatan committed Jul 23, 2024
1 parent d46f41b commit 33e8146
Show file tree
Hide file tree
Showing 9 changed files with 112 additions and 35 deletions.
6 changes: 6 additions & 0 deletions .changeset/cool-maps-shop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@graphql-tools/federation': patch
'@graphql-tools/delegate': patch
---

Fail on query planning phase if the query plan is not successful before the actual execution
44 changes: 44 additions & 0 deletions .changeset/great-toys-joke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
---
'@graphql-tools/executor': minor
---

Ability to create critical errors that prevents to return a partial results

```ts
import { CRITICAL_ERROR } from '@graphql-tools/executor';

const schema = makeExecutableSchema({
typeDefs: `
type Query {
hello: String
}
`,
resolvers: {
Query: {
hello: () => new GraphQLError('Critical error', {
extensions: {
[CRITICAL_ERROR]: true
}
})
}
}
});
```

This will prevent to return a partial results and will return an error instead.

```ts
const result = await execute({
schema,
document: parse(`{ hello }`)
});

expect(result).toEqual({
errors: [
{
message: 'Critical error',
}
],
data: null, // Instead of { hello: null }
});
```
33 changes: 30 additions & 3 deletions packages/delegate/src/finalizeGatewayRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ import {
visit,
visitWithTypeInfo,
} from 'graphql';
import { CRITICAL_ERROR } from '@graphql-tools/executor';
import {
ASTVisitorKeyMap,
createGraphQLError,
createVariableNameGenerator,
ExecutionRequest,
getDefinedRootType,
Expand Down Expand Up @@ -139,6 +141,31 @@ export function finalizeGatewayRequest<TContext>(
operations,
);

// Fail if the query is only the root __typename field

if (
newDocument.definitions.length === 1 &&
newDocument.definitions[0].kind === Kind.OPERATION_DEFINITION
) {
const operation = newDocument.definitions[0] as OperationDefinitionNode;
if (
operation.selectionSet.selections.length === 1 &&
operation.selectionSet.selections[0].kind === Kind.FIELD
) {
const field = operation.selectionSet.selections[0] as any;
if (field.name.value === '__typename' && operation.operation === 'query') {
throw createGraphQLError(
'Failed to create a gateway request. The query must contain at least one selection.',
{
extensions: {
[CRITICAL_ERROR]: true,
},
},
);
}
}
}

const newVariables: Record<string, any> = {};
if (variables != null) {
for (const variableName of usedVariables) {
Expand Down Expand Up @@ -405,9 +432,9 @@ function finalizeSelectionSet(
leave: node => {
const type = typeInfo.getType();
if (type == null) {
console.warn(
`Invalid type for node: ${typeInfo.getParentType()?.name}.${node.name.value}`,
);
// console.warn(
// `Invalid type for node: ${typeInfo.getParentType()?.name}.${node.name.value}`,
// );
return null;
}
const namedType = getNamedType(type);
Expand Down
6 changes: 6 additions & 0 deletions packages/executor/src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,8 @@ export function buildResolveInfo(
};
}

export const CRITICAL_ERROR = 'CRITICAL_ERROR' as const;

function handleFieldError(
error: GraphQLError,
returnType: GraphQLOutputType,
Expand All @@ -770,6 +772,10 @@ function handleFieldError(
throw error;
}

if (error.extensions?.[CRITICAL_ERROR]) {
throw error;
}

// Otherwise, error protection is applied, logging the error and resolving
// a null value for this field if one is encountered.
errors.push(error);
Expand Down
5 changes: 5 additions & 0 deletions packages/federation/src/supergraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,11 @@ export function getStitchingOptionsFromSupergraphSdl(
) {
throw createGraphQLError(
`Was not able to find any options for ${node.name.value}: This shouldn't have happened.`,
{
extensions: {
CRITICAL_ERROR: true,
},
},
);
}
}
Expand Down
44 changes: 17 additions & 27 deletions packages/federation/test/federation-compatibility.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,35 +163,25 @@ describe('Federation Compatibility', () => {
}
});
it('gives the correct result', () => {
if (test.expected.errors) {
expect(result.errors).toBeInstanceOf(Array);
} else {
result.errors?.forEach(e => console.error(e));
expect(result.errors).toBeFalsy();
}
if (test.expected.data) {
expect(result.data).toEqual(test.expected.data);
const received = {
data: result.data ?? null,
errors: !!result.errors?.length,
};

const expected = {
data: test.expected.data ?? null,
errors: test.expected.errors ?? false,
};

try {
expect(received).toEqual(expected);
} catch (e) {
result.errors?.forEach(err => {
console.error(err);
});
throw e;
}
});
/*
if (!process.env['LEAK_TEST']) {
it('calls the subgraphs at the same number or less than Apollo GW', async () => {
try {
await apolloExecutor({
document: parse(test.query, { noLocation: true }),
});
} catch (e) {}
for (const subgraphName in apolloSubgraphCalls) {
if (stitchingSubgraphCalls[subgraphName] != null) {
expect(stitchingSubgraphCalls[subgraphName]).toBeLessThanOrEqual(
apolloSubgraphCalls[subgraphName],
);
}
// If never called, that's better
}
});
}
*/
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@
{
"query": "\n query {\n a {\n field\n }\n }\n ",
"expected": {
"data": {
"a": null
}
"data": null,
"errors": true
},
"plan": "\n # NOTE\n # Query.a is only available in subgraph A.\n # Query.a resolves Node interface, but none of the object types in subgraph A implement Node.\n QueryPlan {}\n "
},
Expand Down Expand Up @@ -69,4 +68,4 @@
},
"plan": "\n QueryPlan {\n Sequence {\n Fetch(service: \"a\") {\n {\n product {\n __typename\n id\n }\n }\n },\n Flatten(path: \"product\") {\n Fetch(service: \"b\") {\n {\n ... on Product {\n __typename\n id\n }\n } =>\n {\n ... on Product {\n __typename\n }\n }\n },\n },\n },\n }\n "
}
]
]
2 changes: 1 addition & 1 deletion packages/federation/test/managed-federation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ describe('Managed Federation', () => {
});

expect(result).toMatchObject({
supergraphSdl: supergraphSdl,
supergraphSdl,
id: 'test-id-1',
minDelaySeconds: 0.1,
});
Expand Down

0 comments on commit 33e8146

Please sign in to comment.