Skip to content

Commit

Permalink
Fixes for issue 1753. Adds ability to limit errors in getVariableValu…
Browse files Browse the repository at this point in the history
…es() and coerceValue(). Errors are statically capped at 50
  • Loading branch information
SoyYoRafa committed Jul 14, 2019
1 parent 49d86bb commit 597e4f5
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 8 deletions.
26 changes: 26 additions & 0 deletions src/execution/__tests__/variables-test.js
Expand Up @@ -882,6 +882,32 @@ describe('Execute: Handles inputs', () => {
});
});

it('Limits errors when there are more than 50 errors of does not allow non-null lists of non-nulls to contain null.', () => {
const doc = `
query ($input: [String!]!) {
nnListNN(input: $input)
}
`;
const largeList = new Array(100).fill(null);
const result = executeQuery(doc, { input: largeList });

const expectedErrors = [];
for (let idx = 0; idx < 50; ++idx) {
expectedErrors.push({
message: `Variable "$input" got invalid value [null, null, null, null, null, null, null, null, null, null, ... 90 more items]; Expected non-nullable type String! not to be null at value[${idx}].`,
locations: [{ line: 2, column: 16 }],
});
}
expectedErrors.push({
message:
'Too many errors processing variables, error limit reached. Execution aborted.',
});

expect(result).to.deep.equal({
errors: expectedErrors,
});
});

it('does not allow invalid types to be used as values', () => {
const doc = `
query ($input: TestType!) {
Expand Down
31 changes: 29 additions & 2 deletions src/execution/values.js
Expand Up @@ -45,6 +45,8 @@ export function getVariableValues(
): CoercedVariableValues {
const errors = [];
const coercedValues = {};
const maxErrors = 50;
let errorLimitReached = false;
for (let i = 0; i < varDefNodes.length; i++) {
const varDefNode = varDefNodes[i];
const varName = varDefNode.variable.name.value;
Expand All @@ -61,6 +63,10 @@ export function getVariableValues(
varDefNode.type,
),
);
if (errors.length === maxErrors) {
errorLimitReached = true;
break;
}
} else {
const hasValue = hasOwnProperty(inputs, varName);
const value = hasValue ? inputs[varName] : undefined;
Expand All @@ -81,6 +87,10 @@ export function getVariableValues(
varDefNode,
),
);
if (errors.length === maxErrors) {
errorLimitReached = true;
break;
}
} else if (hasValue) {
if (value === null) {
// If the explicit value `null` was provided, an entry in the coerced
Expand All @@ -92,19 +102,36 @@ export function getVariableValues(
const coerced = coerceValue(value, varType, varDefNode);
const coercionErrors = coerced.errors;
if (coercionErrors) {
for (const error of coercionErrors) {
const reachesErrorLimit =
coercionErrors.length + errors.length >= maxErrors;
const publishedErrors = reachesErrorLimit
? coercionErrors
: coercionErrors.slice(0, maxErrors - errors.length);
for (const error of publishedErrors) {
error.message =
`Variable "$${varName}" got invalid value ${inspect(value)}; ` +
error.message;
}
errors.push(...coercionErrors);
errors.push(...publishedErrors);
if (reachesErrorLimit || coerced.errorLimitReached) {
errorLimitReached = true;
break;
}
} else {
coercedValues[varName] = coerced.value;
}
}
}
}
}
if (errorLimitReached) {
errors.push(
new GraphQLError(
'Too many errors processing variables, error limit reached. Execution aborted.',
),
);
}

return errors.length === 0
? { errors: undefined, coerced: coercedValues }
: { errors, coerced: undefined };
Expand Down
18 changes: 18 additions & 0 deletions src/utilities/__tests__/coerceValue-test.js
Expand Up @@ -16,11 +16,13 @@ import {

function expectValue(result) {
expect(result.errors).to.equal(undefined);
expect(result.errorLimitReached).to.equal(undefined);
return expect(result.value);
}

function expectErrors(result) {
expect(result.value).to.equal(undefined);
expect(result.errorLimitReached).to.not.equal(undefined);
const messages = result.errors && result.errors.map(error => error.message);
return expect(messages);
}
Expand Down Expand Up @@ -334,5 +336,21 @@ describe('coerceValue', () => {
const result = coerceValue([42, [null], null], TestNestedList);
expectValue(result).to.deep.equal([[42], [null], null]);
});

it('returns an error array limited to 50 errors and limit reached flag is true', () => {
const value = [];
const errors = [];
for (let index = 0; index < 100; ++index) {
value.push(['string']);
if (index < 50) {
errors.push(
`Expected type Int at value[${index}][0]. Int cannot represent non-integer value: "string"`,
);
}
}
const result = coerceValue(value, TestNestedList);
expectErrors(result).to.deep.equal(errors);
expect(result.errorLimitReached).to.equal(true);
});
});
});
32 changes: 26 additions & 6 deletions src/utilities/coerceValue.js
@@ -1,6 +1,6 @@
// @flow strict

import { forEach, isCollection } from 'iterall';
import { isCollection } from 'iterall';
import objectValues from '../polyfills/objectValues';
import inspect from '../jsutils/inspect';
import isInvalid from '../jsutils/isInvalid';
Expand All @@ -19,6 +19,7 @@ import {
} from '../type/definition';

type CoercedValue = {|
+errorLimitReached: boolean | void,
+errors: $ReadOnlyArray<GraphQLError> | void,
+value: mixed,
|};
Expand All @@ -38,6 +39,7 @@ export function coerceValue(
blameNode?: ASTNode,
path?: Path,
): CoercedValue {
const maxErrors = 50;
// A value must be provided if the type is non-null.
if (isNonNullType(type)) {
if (value == null) {
Expand Down Expand Up @@ -108,7 +110,9 @@ export function coerceValue(
if (isCollection(value)) {
let errors;
const coercedValue = [];
forEach((value: any), (itemValue, index) => {
const listValue: $ReadOnlyArray<mixed> = (value: any);
for (let index = 0; index < listValue.length; ++index) {
const itemValue = listValue[index];
const coercedItem = coerceValue(
itemValue,
itemType,
Expand All @@ -117,10 +121,13 @@ export function coerceValue(
);
if (coercedItem.errors) {
errors = add(errors, coercedItem.errors);
if (errors.length >= maxErrors) {
return ofErrors(errors.slice(0, maxErrors), true);
}
} else if (!errors) {
coercedValue.push(coercedItem.value);
}
});
}
return errors ? ofErrors(errors) : ofValue(coercedValue);
}
// Lists accept a non-list value as a list of one.
Expand Down Expand Up @@ -157,6 +164,9 @@ export function coerceValue(
blameNode,
),
);
if (errors.length >= maxErrors) {
return ofErrors(errors.slice(0, maxErrors), true);
}
}
} else {
const coercedField = coerceValue(
Expand All @@ -167,6 +177,9 @@ export function coerceValue(
);
if (coercedField.errors) {
errors = add(errors, coercedField.errors);
if (errors.length >= maxErrors) {
return ofErrors(errors.slice(0, maxErrors), true);
}
} else if (!errors) {
coercedValue[field.name] = coercedField.value;
}
Expand All @@ -186,6 +199,9 @@ export function coerceValue(
didYouMean(suggestions),
),
);
if (errors.length >= maxErrors) {
return ofErrors(errors.slice(0, maxErrors), true);
}
}
}

Expand All @@ -198,11 +214,15 @@ export function coerceValue(
}

function ofValue(value) {
return { errors: undefined, value };
return { errorLimitReached: undefined, errors: undefined, value };
}

function ofErrors(errors) {
return { errors, value: undefined };
function ofErrors(errors, errorLimitReached) {
return {
errorLimitReached: errorLimitReached == null ? false : errorLimitReached,
errors,
value: undefined,
};
}

function add(errors, moreErrors) {
Expand Down

0 comments on commit 597e4f5

Please sign in to comment.