Skip to content

allow formatErrorMessage callback #3

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

Merged
merged 9 commits into from
Jun 27, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const ComplexityLimitRule = createComplexityLimitRule(1000, {
scalarCost: 1,
objectCost: 10, // Default is 0.
listFactor: 20, // Default is 10.
onCost: (cost) => console.log('total'),
formatErrorMessage: (cost) => 'Bad Query',
Copy link
Contributor

Choose a reason for hiding this comment

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

these examples are sort of weak (plus don't follow eslint guidelines). also the graf above needs to explain these options.

});
```

Expand Down
9 changes: 7 additions & 2 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,14 @@ export function createComplexityLimitRule(maxCost, options = {}) {
}

if (node.kind === 'Document') {
if (visitor.getCost() > maxCost) {
const cost = visitor.getCost();
if (options.onCost) options.onCost(cost);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe also pass the query as arg to onCost?

Copy link
Contributor Author

@chirag04 chirag04 Jun 22, 2017

Choose a reason for hiding this comment

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

@taion I still think passing the document node might be useful to both these functions. we maybe want to log queries that fail in prod as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

split out these properties before passing down to visitor

if (cost > maxCost) {
const errorMessage = options.formatErrorMessage ?
Copy link
Contributor

Choose a reason for hiding this comment

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

just default formatErrorMessage to soemthing

options.formatErrorMessage(cost) :
`query exceeds complexity limit. Cost: ${cost}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should absolutely not always return the cost here; the calculation may be a private detail

context.reportError(new GraphQLError(
'query exceeds complexity limit', [node],
errorMessage, [node],
));
}
}
Expand Down
36 changes: 35 additions & 1 deletion test/createComplexityLimitRule.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,41 @@ describe('createComplexityLimitRule', () => {
const errors = validate(schema, ast, [createComplexityLimitRule(9)]);
expect(errors).toHaveLength(1);
expect(errors[0]).toMatchObject({
message: 'query exceeds complexity limit',
message: 'query exceeds complexity limit. Cost: 10',
});
});

it('should call onCost with complexity score', () => {
const ast = parse(`
query {
list {
name
}
}
`);

const onCost = jest.fn();
const errors = validate(schema, ast, [
createComplexityLimitRule(9, { onCost }),
]);
expect(onCost).toBeCalledWith(10);
expect(errors).toHaveLength(1);
});

it('should call formatErrorMessage with complexity score', () => {
const ast = parse(`
query {
list {
name
}
}
`);

const formatErrorMessage = jest.fn();
const errors = validate(schema, ast, [
createComplexityLimitRule(9, { formatErrorMessage }),
]);
expect(formatErrorMessage).toBeCalledWith(10);
expect(errors).toHaveLength(1);
});
});