From e44b7488ff6605aef50f98666be6d5f451813c2f Mon Sep 17 00:00:00 2001 From: Vincent Smith Date: Fri, 15 Jul 2022 17:49:59 -0400 Subject: [PATCH] Ignore Missing Ignore/Skip Directives Currently unused variables are ignored for the purposes of calculating complexity. However, because graphql-js's `getVariableValues` method returns either a map of coersed values, OR an array of errors, if directives are present in conjuection with unused variables then strange errors will be raised. In particular errors will state that directive variables were not present when they may well have been. This change adds fallback measures to ensure that complexity will stil be calculated if unused variables are present alonside directives. Fixes #69 --- src/QueryComplexity.ts | 54 ++++++++++------ src/__tests__/QueryComplexity-test.ts | 93 ++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 23 deletions(-) diff --git a/src/QueryComplexity.ts b/src/QueryComplexity.ts index 348c4f5..e2c4718 100644 --- a/src/QueryComplexity.ts +++ b/src/QueryComplexity.ts @@ -173,6 +173,12 @@ export default class QueryComplexity { this.options.variables ?? {} ).coerced; + // If there are no coerced variable values it is because one or all of those variables were + // absent. In this case fallback to what was provided and ignore defaults. + if (!this.variableValues) { + this.variableValues = this.options.variables; + } + switch (operation.operation) { case 'query': this.complexity += this.nodeComplexity( @@ -261,28 +267,36 @@ export default class QueryComplexity { for (const directive of childNode.directives ?? []) { const directiveName = directive.name.value; - switch (directiveName) { - case 'include': { - const values = getDirectiveValues( - this.includeDirectiveDef, - childNode, - this.variableValues || {} - ); - if (typeof values.if === 'boolean') { - includeNode = values.if; + try { + switch (directiveName) { + case 'include': { + const values = getDirectiveValues( + this.includeDirectiveDef, + childNode, + this.variableValues || {} + ); + if (typeof values.if === 'boolean') { + includeNode = values.if; + } + break; } - break; - } - case 'skip': { - const values = getDirectiveValues( - this.skipDirectiveDef, - childNode, - this.variableValues || {} - ); - if (typeof values.if === 'boolean') { - skipNode = values.if; + case 'skip': { + const values = getDirectiveValues( + this.skipDirectiveDef, + childNode, + this.variableValues || {} + ); + if (typeof values.if === 'boolean') { + skipNode = values.if; + } + break; } - break; + } + } catch (error) { + // Ignore GraphQLErrors in favor of falling back to the default values for + // includeNode and skipNode + if (!(error instanceof GraphQLError)) { + throw error; } } } diff --git a/src/__tests__/QueryComplexity-test.ts b/src/__tests__/QueryComplexity-test.ts index a0979db..ee9c033 100644 --- a/src/__tests__/QueryComplexity-test.ts +++ b/src/__tests__/QueryComplexity-test.ts @@ -271,6 +271,93 @@ describe('QueryComplexity analysis', () => { expect(visitor.complexity).to.equal(10); }); + it('should ignore unused variables with include', () => { + const ast = parse(` + query ($unusedVar: ID!, $shouldInclude: Boolean! = false) { + variableScalar(count: 100) @include(if: $shouldInclude) + } + `); + + const context = new CompatibleValidationContext(schema, ast, typeInfo); + const visitor = new ComplexityVisitor(context, { + maximumComplexity: 100, + estimators: [simpleEstimator({ defaultComplexity: 10 })], + variables: { shouldInclude: true }, + }); + + visit(ast, visitWithTypeInfo(typeInfo, visitor)); + expect(visitor.complexity).to.equal(10); + }); + + it('should ignore unused variables with skip', () => { + const ast = parse(` + query ($unusedVar: ID!, $shouldSkip: Boolean! = false) { + variableScalar(count: 100) @skip(if: $shouldSkip) + } + `); + + const context = new CompatibleValidationContext(schema, ast, typeInfo); + const visitor = new ComplexityVisitor(context, { + maximumComplexity: 100, + estimators: [simpleEstimator({ defaultComplexity: 10 })], + variables: { shouldSkip: false }, + }); + + visit(ast, visitWithTypeInfo(typeInfo, visitor)); + expect(visitor.complexity).to.equal(10); + }); + + it('should ignore unused variables with unused include', () => { + const ast = parse(` + query ($unusedVar: ID!, $shouldInclude: Boolean! = false) { + variableScalar(count: 100) @include(if: $shouldInclude) + } + `); + + const context = new CompatibleValidationContext(schema, ast, typeInfo); + const visitor = new ComplexityVisitor(context, { + maximumComplexity: 100, + estimators: [simpleEstimator({ defaultComplexity: 10 })], + }); + + visit(ast, visitWithTypeInfo(typeInfo, visitor)); + expect(visitor.complexity).to.equal(10); + }); + + it('should ignore unused variables with unused skip', () => { + const ast = parse(` + query ($unusedVar: ID!, $shouldSkip: Boolean! = false) { + variableScalar(count: 100) @skip(if: $shouldSkip) + } + `); + + const context = new CompatibleValidationContext(schema, ast, typeInfo); + const visitor = new ComplexityVisitor(context, { + maximumComplexity: 100, + estimators: [simpleEstimator({ defaultComplexity: 10 })], + }); + + visit(ast, visitWithTypeInfo(typeInfo, visitor)); + expect(visitor.complexity).to.equal(10); + }); + + it('should ignore multiple unused variables', () => { + const ast = parse(` + query ($unusedVar: ID!, $anotherUnusedVar: Boolean!) { + variableScalar(count: 100) + } + `); + + const context = new CompatibleValidationContext(schema, ast, typeInfo); + const visitor = new ComplexityVisitor(context, { + maximumComplexity: 100, + estimators: [simpleEstimator({ defaultComplexity: 10 })], + }); + + visit(ast, visitWithTypeInfo(typeInfo, visitor)); + expect(visitor.complexity).to.equal(10); + }); + it('should ignore unknown field', () => { const ast = parse(` query { @@ -639,7 +726,7 @@ describe('QueryComplexity analysis', () => { ...on Union { ...on Item { complexScalar1: complexScalar - } + } } ...on SecondItem { scalar @@ -678,7 +765,7 @@ describe('QueryComplexity analysis', () => { fragment F on Union { ...on Item { complexScalar1: complexScalar - } + } } `); @@ -759,7 +846,7 @@ describe('QueryComplexity analysis', () => { } } } - + fragment F on Query { complexScalar ...on Query {