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

batch-execute enhancements #3588

Merged
merged 15 commits into from
Sep 29, 2021
12 changes: 12 additions & 0 deletions .changeset/pink-walls-return.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@graphql-tools/batch-execute": patch
"@graphql-tools/delegate": patch
"@graphql-tools/utils": patch
"@graphql-tools/website": patch
---

batch-execute enhancements:
- fixes bugs with batched fragment definitions
- unpathed errors are now returned for all batch results
- the "graphqlTools" prefix is simplified down to just "_"
- new tests and documentation
18 changes: 10 additions & 8 deletions packages/batch-execute/src/createBatchingExecutor.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import DataLoader from 'dataloader';

import { ExecutionRequest, Executor, ExecutionResult } from '@graphql-tools/utils';
import { Executor, ExecutionRequest, ExecutionResult } from '@graphql-tools/utils';

import { mergeRequests } from './mergeRequests';
import { splitResult } from './splitResult';
Expand Down Expand Up @@ -33,23 +33,25 @@ function createLoadFn(

const operationType = request.operationType;

if (operationType == null) {
throw new Error('could not identify operation type of document');
}

while (++index < requests.length) {
const currentOperationType = requests[index].operationType;
if (operationType == null) {
throw new Error('Could not identify operation type of document.');
}
const currentRequest = requests[index];
const currentOperationType = currentRequest.operationType;

if (operationType === currentOperationType) {
currentBatch.push(requests[index]);
currentBatch.push(currentRequest);
} else {
currentBatch = [requests[index]];
currentBatch = [currentRequest];
execBatches.push(currentBatch);
}
}

const results = await Promise.all(
execBatches.map(async execBatch => {
const mergedRequests = mergeRequests(execBatch, extensionsReducer);
const mergedRequests = mergeRequests(execBatch[0].operationType, execBatch, extensionsReducer);
const resultBatches = (await executor(mergedRequests)) as ExecutionResult;
return splitResult(resultBatches, execBatch.length);
})
Expand Down
31 changes: 24 additions & 7 deletions packages/batch-execute/src/mergeRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
Kind,
DefinitionNode,
OperationDefinitionNode,
OperationTypeNode,
DocumentNode,
FragmentDefinitionNode,
VariableDefinitionNode,
Expand Down Expand Up @@ -56,6 +57,7 @@ import { createPrefix } from './prefix';
* }
*/
export function mergeRequests(
operationType: OperationTypeNode,
requests: Array<ExecutionRequest>,
extensionsReducer: (mergedExtensions: Record<string, any>, request: ExecutionRequest) => Record<string, any>
): ExecutionRequest {
Expand All @@ -67,7 +69,7 @@ export function mergeRequests(

for (const index in requests) {
const request = requests[index];
const prefixedRequests = prefixRequest(createPrefix(index), request);
const prefixedRequests = prefixRequest(createPrefix(index), request, operationType);

for (const def of prefixedRequests.document.definitions) {
if (isOperationDefinition(def)) {
Expand All @@ -86,7 +88,7 @@ export function mergeRequests(

const mergedOperationDefinition: OperationDefinitionNode = {
kind: Kind.OPERATION_DEFINITION,
operation: requests[0].operationType,
operation: operationType,
variableDefinitions: mergedVariableDefinitions,
selectionSet: {
kind: Kind.SELECTION_SET,
Expand All @@ -103,11 +105,11 @@ export function mergeRequests(
extensions: mergedExtensions,
context: requests[0].context,
info: requests[0].info,
operationType: requests[0].operationType,
operationType,
};
}

function prefixRequest(prefix: string, request: ExecutionRequest): ExecutionRequest {
function prefixRequest(prefix: string, request: ExecutionRequest, operationType: OperationTypeNode): ExecutionRequest {
const executionVariables = request.variables ?? {};

function prefixNode(node: VariableNode | FragmentDefinitionNode | FragmentSpreadNode) {
Expand All @@ -117,12 +119,18 @@ function prefixRequest(prefix: string, request: ExecutionRequest): ExecutionRequ
let prefixedDocument = aliasTopLevelFields(prefix, request.document);

const executionVariableNames = Object.keys(executionVariables);
const hasFragmentDefinitions = request.document.definitions.some(def => isFragmentDefinition(def));
const fragmentSpreadImpl: Record<string, boolean> = {};

if (executionVariableNames.length > 0) {
if (executionVariableNames.length > 0 || hasFragmentDefinitions) {
Copy link
Contributor Author

@gmac gmac Sep 27, 2021

Choose a reason for hiding this comment

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

@yaacovCR – have a look at 44ba4cb. I've fixed a couple bugs related to fragments here:

  1. Fragments weren't getting prefixed unless there were also variables.
  2. Inlined root fragments were allowed to leave an invalid orphaned fragment definition. This now filters out fragment definitions unless they have a surviving spread implementation.

prefixedDocument = visit(prefixedDocument, {
[Kind.VARIABLE]: prefixNode,
[Kind.FRAGMENT_DEFINITION]: prefixNode,
[Kind.FRAGMENT_SPREAD]: prefixNode,
[Kind.FRAGMENT_SPREAD]: node => {
node = prefixNodeName(node, prefix);
fragmentSpreadImpl[node.name.value] = true;
return node;
},
}) as DocumentNode;
}

Expand All @@ -132,10 +140,19 @@ function prefixRequest(prefix: string, request: ExecutionRequest): ExecutionRequ
prefixedVariables[prefix + variableName] = executionVariables[variableName];
}

if (hasFragmentDefinitions) {
prefixedDocument = {
...prefixedDocument,
definitions: prefixedDocument.definitions.filter(def => {
return !isFragmentDefinition(def) || fragmentSpreadImpl[def.name.value];
}),
};
}

return {
document: prefixedDocument,
variables: prefixedVariables,
operationType: request.operationType,
operationType,
};
}

Expand Down
4 changes: 2 additions & 2 deletions packages/batch-execute/src/prefix.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// adapted from https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-graphql/src/batching/merge-queries.js

export function createPrefix(index: string): string {
return `graphqlTools${index}_`;
return `_${index}_`;
}

export function parseKey(prefixedKey: string): { index: number; originalKey: string } {
const match = /^graphqlTools([\d]+)_(.*)$/.exec(prefixedKey);
const match = /^_([\d]+)_(.*)$/.exec(prefixedKey);
if (match && match.length === 3 && !isNaN(Number(match[1])) && match[2]) {
return { index: Number(match[1]), originalKey: match[2] };
}
Expand Down
9 changes: 7 additions & 2 deletions packages/batch-execute/src/splitResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ export function splitResult({ data, errors }: ExecutionResult, numResults: numbe
const parsedKey = parseKey(error.path[0] as string);
const { index, originalKey } = parsedKey;
const newError = relocatedError(error, [originalKey, ...error.path.slice(1)]);
const errors = (splitResults[index].errors = (splitResults[index].errors || []) as GraphQLError[]);
errors.push(newError);
const resultErrors = (splitResults[index].errors = (splitResults[index].errors || []) as GraphQLError[]);
resultErrors.push(newError);
} else {
splitResults.forEach(result => {
const resultErrors = (result.errors = (result.errors || []) as GraphQLError[]);
resultErrors.push(new GraphQLError(error.message));
});
}
}
}
Expand Down
197 changes: 197 additions & 0 deletions packages/batch-execute/tests/batchExecute.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
import {
graphql,
parse,
print,
OperationDefinitionNode,
ExecutionResult
} from 'graphql';
import { makeExecutableSchema } from '@graphql-tools/schema';
import { createBatchingExecutor } from '@graphql-tools/batch-execute';
import { Executor } from '@graphql-tools/utils';

describe('batch execution', () => {
let executorCalls = 0;
let executorDocument: string | undefined;
let executorVariables: Record<string, any> | undefined;

const schema = makeExecutableSchema({
typeDefs: /* GraphQL */`
type Query {
field1: String
field2: String
field3(input: String): String
boom(message: String): String
widget: Widget
}
type Widget {
name: String
}
`,
resolvers: {
Query: {
field1: () => '1',
field2: () => '2',
field3: (_root, { input }) => String(input),
boom: (_root, { message }) => new Error(message),
widget: () => ({ name: 'wingnut' }),
},
},
});

const exec = (({ document, variables }) => {
executorCalls += 1;
executorDocument = print(document);
executorVariables = variables;
return graphql({
schema,
source: executorDocument,
variableValues: executorVariables,
});
}) as Executor;

const batchExec = createBatchingExecutor(exec);

beforeEach(() => {
executorCalls = 0;
executorDocument = undefined;
executorVariables = undefined;
});

function getRequestFields(): Array<string> {
if (executorDocument != null) {
const op = parse(executorDocument).definitions[0] as OperationDefinitionNode;
const names = op.selectionSet.selections.map(sel => 'name' in sel ? sel.name.value : undefined);
return names.filter(Boolean) as Array<string>;
}
return [];
}

it('batchs multiple executions', async () => {
const [first, second] = await Promise.all([
batchExec({ document: parse('{ field1 field2 }'), operationType: 'query' }),
batchExec({ document: parse('{ field2 field3(input: "3") }'), operationType: 'query' }),
]) as ExecutionResult[];

expect(first?.data).toEqual({ field1: '1', field2: '2' });
expect(second?.data).toEqual({ field2: '2', field3: '3' });
expect(executorCalls).toEqual(1);
expect(getRequestFields()).toEqual(['field1', 'field2', 'field2', 'field3']);
});

it('preserves root field aliases in the final result', async () => {
const [first, second] = await Promise.all([
batchExec({ document: parse('{ a: field1 b: field2 }'), operationType: 'query' }),
batchExec({ document: parse('{ c: field2 d: field3(input: "3") }'), operationType: 'query' }),
]) as ExecutionResult[];

expect(first?.data).toEqual({ a: '1', b: '2' });
expect(second?.data).toEqual({ c: '2', d: '3' });
expect(executorCalls).toEqual(1);
expect(getRequestFields()).toEqual(['field1', 'field2', 'field2', 'field3']);
});

it('renames input variables', async () => {
const [first, second] = await Promise.all([
batchExec({
document: parse('query($a: String){ field3(input: $a) }'),
variables: { a: '1' },
operationType: 'query',
}),
batchExec({
document: parse('query($a: String){ field3(input: $a) }'),
variables: { a: '2' },
operationType: 'query',
}),
]) as ExecutionResult[];

expect(first?.data).toEqual({ field3: '1' });
expect(second?.data).toEqual({ field3: '2' });
expect(executorVariables).toEqual({ _0_a: '1', _1_a: '2' });
expect(executorCalls).toEqual(1);
});

it('renames fields within inline spreads', async () => {
const [first, second] = await Promise.all([
batchExec({ document: parse('{ ...on Query { field1 } }'), operationType: 'query' }),
batchExec({ document: parse('{ ...on Query { field2 } }'), operationType: 'query' }),
]) as ExecutionResult[];

const squishedDoc = executorDocument?.replace(/\s+/g, ' ');
expect(squishedDoc).toMatch('... on Query { _0_field1: field1 }');
expect(squishedDoc).toMatch('... on Query { _1_field2: field2 }');
expect(first?.data).toEqual({ field1: '1' });
expect(second?.data).toEqual({ field2: '2' });
expect(executorCalls).toEqual(1);
});

it('renames fragment definitions and spreads', async () => {
const [first, second] = await Promise.all([
batchExec({
document: parse('fragment A on Widget { name } query{ widget { ...A } }'),
operationType: 'query',
}),
batchExec({
document: parse('fragment A on Widget { name } query{ widget { ...A } }'),
operationType: 'query',
}),
]) as ExecutionResult[];

const squishedDoc = executorDocument?.replace(/\s+/g, ' ');
expect(squishedDoc).toMatch('_0_widget: widget { ..._0_A }');
expect(squishedDoc).toMatch('_1_widget: widget { ..._1_A }');
expect(squishedDoc).toMatch('fragment _0_A on Widget');
expect(squishedDoc).toMatch('fragment _1_A on Widget');
expect(first?.data).toEqual({ widget: { name: 'wingnut' } });
expect(second?.data).toEqual({ widget: { name: 'wingnut' } });
expect(executorCalls).toEqual(1);
});

it('removes expanded root fragment definitions', async () => {
const [first, second] = await Promise.all([
batchExec({
document: parse('fragment A on Query { field1 } query{ ...A }'),
operationType: 'query',
}),
batchExec({
document: parse('fragment A on Query { field2 } query{ ...A }'),
operationType: 'query',
}),
]) as ExecutionResult[];

expect(first?.data).toEqual({ field1: '1' });
expect(second?.data).toEqual({ field2: '2' });
expect(executorCalls).toEqual(1);
});

it('preserves pathed errors in the final result', async () => {
const [first, second] = await Promise.all([
batchExec({
document: parse('{ first: boom(message: "first error") }'),
operationType: 'query',
}),
batchExec({
document: parse('{ second: boom(message: "second error") }'),
operationType: 'query',
}),
]) as ExecutionResult[];

expect(first?.errors?.[0].message).toEqual('first error');
expect(first?.errors?.[0].path).toEqual(['first']);
expect(second?.errors?.[0].message).toEqual('second error');
expect(second?.errors?.[0].path).toEqual(['second']);
expect(executorCalls).toEqual(1);
});

it('returns request-level errors to all results', async () => {
const [first, second] = await Promise.all([
batchExec({ document: parse('{ field1 field2 }'), operationType: 'query' }),
batchExec({ document: parse('{ notgonnawork }'), operationType: 'query' }),
]) as ExecutionResult[];

expect(first?.errors?.length).toEqual(1);
expect(second?.errors?.length).toEqual(1);
expect(first?.errors?.[0].message).toMatch(/notgonnawork/);
expect(second?.errors?.[0].message).toMatch(/notgonnawork/);
expect(executorCalls).toEqual(1);
});
});
Loading