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

Invalid argument values handling, error reporting in getComplexity #62

Merged
merged 2 commits into from Oct 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions .circleci/config.yml
Expand Up @@ -12,7 +12,7 @@ workflows:
# definitions for field extensions in older @types/graphql versions
matrix:
parameters:
graphql-version: ["~0.13", "~14.0", "~14.5", "~14.6", "~15.0"]
graphql-version: ['~14.5', '~14.6', '~15.0']
- test-and-build:
# Leave graphql-version unspecified to respect the lockfile and also run tsc
name: test-and-build-with-typecheck
Expand All @@ -22,7 +22,7 @@ jobs:
parameters:
graphql-version:
type: string
default: ""
default: ''

docker:
# specify the version you desire here
Expand All @@ -36,9 +36,9 @@ jobs:
# Download and cache dependencies
- restore_cache:
keys:
- v1-dependencies-{{ checksum "package.json" }}-<< parameters.graphql-version >>
# fallback to using the latest cache if no exact match is found
- v1-dependencies-
- v1-dependencies-{{ checksum "package.json" }}-<< parameters.graphql-version >>
# fallback to using the latest cache if no exact match is found
- v1-dependencies-

- when:
condition: << parameters.graphql-version >>
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Expand Up @@ -2,3 +2,4 @@
node_modules
.DS_Store
.idea
.vscode
25 changes: 15 additions & 10 deletions README.md
Expand Up @@ -171,16 +171,21 @@ const query = parse(`
}
`);

const complexity = getComplexity({
estimators: [simpleEstimator({ defaultComplexity: 1 })],
schema,
query,
variables: {
count: 10,
},
});

console.log(complexity); // Output: 3
try {
const complexity = getComplexity({
estimators: [simpleEstimator({ defaultComplexity: 1 })],
schema,
query,
variables: {
count: 10,
},
});

console.log(complexity); // Output: 3
} catch (e) {
// Log error in case complexity cannot be calculated (invalid query, misconfiguration, etc.)
console.error('Could not calculate complexity', e.message);
}
```

## Prior Art
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -24,7 +24,7 @@
"lodash.get": "^4.4.2"
},
"peerDependencies": {
"graphql": "^0.13.0 || ^14.0.0 || ^15.0.0"
"graphql": "^14.5.0 || ^15.0.0"
},
"files": [
"dist",
Expand Down
14 changes: 11 additions & 3 deletions src/QueryComplexity.ts
Expand Up @@ -96,11 +96,12 @@ export function getComplexity(options: {
}): number {
const typeInfo = new TypeInfo(options.schema);

const errors: GraphQLError[] = [];
const context = new ValidationContext(
options.schema,
options.query,
typeInfo,
() => null
(error) => errors.push(error)
);
const visitor = new QueryComplexity(context, {
// Maximum complexity does not matter since we're only interested in the calculated complexity.
Expand All @@ -111,6 +112,12 @@ export function getComplexity(options: {
});

visit(options.query, visitWithTypeInfo(typeInfo, visitor));

// Throw first error if any
if (errors.length) {
throw errors.pop();
}

return visitor.complexity;
}

Expand Down Expand Up @@ -244,7 +251,7 @@ export default class QueryComplexity {
(
complexities: ComplexityMap,
childNode: FieldNode | FragmentSpreadNode | InlineFragmentNode
) => {
): ComplexityMap => {
// let nodeComplexity = 0;
let innerComplexities = complexities;

Expand Down Expand Up @@ -297,7 +304,8 @@ export default class QueryComplexity {
this.variableValues || {}
);
} catch (e) {
return this.context.reportError(e);
this.context.reportError(e);
return complexities;
}

// Check if we have child complexity
Expand Down
52 changes: 44 additions & 8 deletions src/__tests__/QueryComplexity-test.ts
Expand Up @@ -2,7 +2,14 @@
* Created by Ivo Meißner on 28.07.17.
*/

import { parse, TypeInfo, visit, visitWithTypeInfo } from 'graphql';
import {
parse,
TypeInfo,
visit,
visitWithTypeInfo,
validate,
specifiedRules,
} from 'graphql';

import { expect } from 'chai';

Expand Down Expand Up @@ -523,7 +530,7 @@ describe('QueryComplexity analysis', () => {
);
});

it('should return NaN when no astNode available on field when using directiveEstimator', () => {
it('should throw error when no astNode available on field when using directiveEstimator', () => {
const ast = parse(`
query {
_service {
Expand All @@ -532,12 +539,13 @@ describe('QueryComplexity analysis', () => {
}
`);

const complexity = getComplexity({
estimators: [directiveEstimator()],
schema,
query: ast,
});
expect(Number.isNaN(complexity)).to.equal(true);
expect(() => {
getComplexity({
estimators: [directiveEstimator()],
schema,
query: ast,
});
}).to.throw(/No complexity could be calculated for field Query._service/);
});

it('should skip complexity calculation by directiveEstimator when no astNode available on field', () => {
Expand Down Expand Up @@ -796,4 +804,32 @@ describe('QueryComplexity analysis', () => {
});
expect(complexity).to.equal(30); // 3 fields on nonNullItem * 10
});

it('should handle invalid argument values for multiple query fields', () => {
const ast = parse(`
query {
requiredArgs(count: x) {
scalar
complexScalar
}
nonNullItem {
scalar
complexScalar
variableScalar(count: 10)
}
}
`);

validate(schema, ast, [
...specifiedRules,
createComplexityRule({
maximumComplexity: 1000,
estimators: [
simpleEstimator({
defaultComplexity: 1,
}),
],
}),
]);
});
});