From fc3d616fab1bb73f6578c34bdce02f53ff71020c Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Thu, 18 Aug 2022 14:01:29 -0700 Subject: [PATCH 1/8] completed the analysis of includes and skip directives on objects and interfaces --- src/analysis/QueryParser.ts | 74 +++++++++++--------- test/analysis/typeComplexityAnalysis.test.ts | 48 ++++++++++--- 2 files changed, 81 insertions(+), 41 deletions(-) diff --git a/src/analysis/QueryParser.ts b/src/analysis/QueryParser.ts index d2b7010..a1896bd 100644 --- a/src/analysis/QueryParser.ts +++ b/src/analysis/QueryParser.ts @@ -151,11 +151,16 @@ class QueryParser { * 1. there is no directive * 2. there is a directive named inlcude and the value is true * 3. there is a directive named skip and the value is false + * 4. there is a directive of any other form */ - // THIS IS NOT CALLED ANYWEHERE. IN PROGRESS private directiveCheck(directive: DirectiveNode): boolean { - if (directive?.arguments) { - // get the first argument + // check if there is a directive "include" or "skip" and arguments are present + // evaluate the state of the directive to ignore or calculate the compexity of this queried field + if ( + directive?.arguments && + (directive.name.value === 'include' || directive.name.value === 'skip') + ) { + // only consider the first argument const argument = directive.arguments[0]; // ensure the argument name is 'if' const argumentHasVariables = @@ -173,6 +178,7 @@ class QueryParser { (directive.name.value === 'skip' && directiveArgumentValue === false) ); } + // return true to process all queried fields without/other directives return true; } @@ -185,42 +191,44 @@ class QueryParser { * 2. there is a directive named inlcude and the value is true * 3. there is a directive named skip and the value is false */ - // const directive = node.directives; - // if (directive && this.directiveCheck(directive[0])) { - this.depth += 1; - if (this.depth > this.maxDepth) this.maxDepth = this.depth; - // the kind of a field node will either be field, fragment spread or inline fragment - if (node.kind === Kind.FIELD) { - complexity += this.fieldNode(node, parentName.toLowerCase()); - } else if (node.kind === Kind.FRAGMENT_SPREAD) { - // add complexity and depth from fragment cache - const { complexity: fragComplexity, depth: fragDepth } = - this.fragmentCache[node.name.value]; - complexity += fragComplexity; - this.depth += fragDepth; + const directive = node.directives; + if (directive && this.directiveCheck(directive[0])) { + this.depth += 1; if (this.depth > this.maxDepth) this.maxDepth = this.depth; - this.depth -= fragDepth; + // the kind of a field node will either be field, fragment spread or inline fragment + if (node.kind === Kind.FIELD) { + complexity += this.fieldNode(node, parentName.toLowerCase()); + } else if (node.kind === Kind.FRAGMENT_SPREAD) { + // add complexity and depth from fragment cache + const { complexity: fragComplexity, depth: fragDepth } = + this.fragmentCache[node.name.value]; + complexity += fragComplexity; + this.depth += fragDepth; + if (this.depth > this.maxDepth) this.maxDepth = this.depth; + this.depth -= fragDepth; + + // This is a leaf + // need to parse fragment definition at root and get the result here + } else if (node.kind === Kind.INLINE_FRAGMENT) { + const { typeCondition } = node; - // This is a leaf - // need to parse fragment definition at root and get the result here - } else if (node.kind === Kind.INLINE_FRAGMENT) { - const { typeCondition } = node; + // named type is the type from which inner fields should be take + // If the TypeCondition is omitted, an inline fragment is considered to be of the same type as the enclosing context + const namedType = typeCondition + ? typeCondition.name.value.toLowerCase() + : parentName; - // named type is the type from which inner fields should be take - // If the TypeCondition is omitted, an inline fragment is considered to be of the same type as the enclosing context - const namedType = typeCondition ? typeCondition.name.value.toLowerCase() : parentName; + // TODO: Handle directives like @include and @skip + // subtract 1 before, and add one after, entering the fragment selection to negate the additional level of depth added + this.depth -= 1; + complexity += this.selectionSetNode(node.selectionSet, namedType); + this.depth += 1; + } else { + throw new Error(`ERROR: QueryParser.selectionNode: node type not supported`); + } - // TODO: Handle directives like @include and @skip - // subtract 1 before, and add one after, entering the fragment selection to negate the additional level of depth added this.depth -= 1; - complexity += this.selectionSetNode(node.selectionSet, namedType); - this.depth += 1; - } else { - throw new Error(`ERROR: QueryParser.selectionNode: node type not supported`); } - - this.depth -= 1; - //* } return complexity; } diff --git a/test/analysis/typeComplexityAnalysis.test.ts b/test/analysis/typeComplexityAnalysis.test.ts index 4eff8ac..c18be4b 100644 --- a/test/analysis/typeComplexityAnalysis.test.ts +++ b/test/analysis/typeComplexityAnalysis.test.ts @@ -855,7 +855,7 @@ describe('Test getQueryTypeComplexity function', () => { }); // TODO: refine complexity analysis to consider directives includes and skip - xdescribe('with directives @includes and @skip', () => { + describe('with directives @includes and @skip', () => { test('@includes on interfaces', () => { query = ` query { @@ -871,10 +871,10 @@ describe('Test getQueryTypeComplexity function', () => { } } }`; - expect(mockCharacterFriendsFunction).toHaveBeenCalledTimes(0); + mockCharacterFriendsFunction.mockReturnValueOnce(3); // Query 1 + 1 hero + max(...Character 3, ...Human 0) = 5 expect(queryParser.processQuery(parse(query))).toBe(5); - + mockCharacterFriendsFunction.mockReset(); query = ` query { hero(episode: EMPIRE) { @@ -889,7 +889,7 @@ describe('Test getQueryTypeComplexity function', () => { } } }`; - mockCharacterFriendsFunction.mockReturnValueOnce(3); + expect(mockCharacterFriendsFunction).toHaveBeenCalledTimes(0); // Query 1 + 1 hero = 2 expect(queryParser.processQuery(parse(query))).toBe(2); }); @@ -912,7 +912,7 @@ describe('Test getQueryTypeComplexity function', () => { expect(mockCharacterFriendsFunction).toHaveBeenCalledTimes(0); // Query 1 + 1 hero = 2 expect(queryParser.processQuery(parse(query))).toBe(2); - + mockCharacterFriendsFunction.mockReset(); query = ` query { hero(episode: EMPIRE) { @@ -987,7 +987,8 @@ describe('Test getQueryTypeComplexity function', () => { // 1 query + 1 hero + 1 human expect(queryParser.processQuery(parse(query))).toBe(3); }); - test('with arguments and varibales', () => { + + test('@skip with arguments and varibales', () => { variables = { directive: false }; queryParser = new QueryParser(typeWeights, variables); query = `query ($directive: Boolean!){ @@ -1008,7 +1009,24 @@ describe('Test getQueryTypeComplexity function', () => { hero(episode: EMPIRE) { id, name } - human(id: 1) @includes(if: $directive) { + human(id: 1) @skip(if: $directive) { + id, + name, + homePlanet + } + }`; + // 1 query + 1 hero + expect(queryParser.processQuery(parse(query))).toBe(2); + }); + + test('@includes with arguments and varibales', () => { + variables = { directive: false }; + queryParser = new QueryParser(typeWeights, variables); + query = `query ($directive: Boolean!){ + hero(episode: EMPIRE) { + id, name + } + human(id: 1) @include(if: $directive) { id, name, homePlanet @@ -1016,9 +1034,23 @@ describe('Test getQueryTypeComplexity function', () => { }`; // 1 query + 1 hero expect(queryParser.processQuery(parse(query))).toBe(2); + variables = { directive: true }; + queryParser = new QueryParser(typeWeights, variables); + query = `query ($directive: Boolean!){ + hero(episode: EMPIRE) { + id, name + } + human(id: 1) @include(if: $directive) { + id, + name, + homePlanet + } + }`; + // 1 query + 1 hero + 1 human + expect(queryParser.processQuery(parse(query))).toBe(3); }); - xtest('and other directive are ignored', () => { + test('and other directives or arguments are ignored', () => { query = `query { hero(episode: EMPIRE) { id, name From fddd78a263850451a8f0729a7ad0f715d2583b7e Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Thu, 18 Aug 2022 14:27:22 -0700 Subject: [PATCH 2/8] updated QueryParser flow chart --- src/analysis/QueryParser.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/analysis/QueryParser.ts b/src/analysis/QueryParser.ts index a1896bd..fe4d696 100644 --- a/src/analysis/QueryParser.ts +++ b/src/analysis/QueryParser.ts @@ -22,10 +22,10 @@ import { FieldWeight, TypeWeightObject, Variables } from '../@types/buildTypeWei * |-----> Selection Set Node <-------| * | / * | Selection Node - * | (Field, Inline fragment and fragment spread) - * | | | \ - * | Field Node | fragmentCache - * | | | + * | (Field, Inline fragment, directives and fragment spread) + * | | | \ \ + * | Field Node | \ \ + * | | | directiveCheck fragmentCache * |<--calculateCast | * | | * |<------------------| From 9d01f854792f1d778a42d8a067910ad6172c073e Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Sat, 20 Aug 2022 08:45:27 -0700 Subject: [PATCH 3/8] updated directiveCheck in complexity analysis to consider all directives in the array --- src/analysis/QueryParser.ts | 63 ++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/src/analysis/QueryParser.ts b/src/analysis/QueryParser.ts index fe4d696..ccab636 100644 --- a/src/analysis/QueryParser.ts +++ b/src/analysis/QueryParser.ts @@ -148,51 +148,51 @@ class QueryParser { /** * Return true if: - * 1. there is no directive + * 1. there is no directive skip or include * 2. there is a directive named inlcude and the value is true * 3. there is a directive named skip and the value is false - * 4. there is a directive of any other form */ - private directiveCheck(directive: DirectiveNode): boolean { - // check if there is a directive "include" or "skip" and arguments are present - // evaluate the state of the directive to ignore or calculate the compexity of this queried field - if ( - directive?.arguments && - (directive.name.value === 'include' || directive.name.value === 'skip') - ) { - // only consider the first argument - const argument = directive.arguments[0]; - // ensure the argument name is 'if' - const argumentHasVariables = - argument.value.kind === Kind.VARIABLE && argument.name.value === 'if'; - // access the value of the argument depending on whether it is passed as a variable or not - let directiveArgumentValue; - if (argument.value.kind === Kind.BOOLEAN) { - directiveArgumentValue = Boolean(argument.value.value); - } else if (argumentHasVariables) { - directiveArgumentValue = Boolean(this.variables[argument.value.name.value]); - } + private directiveCheck(directives: DirectiveNode[]): boolean { + // set the default of the return value of directiveCheck to true, reset to false if the directive include or skip is found with the argument is false or true respectively + let directiveCheck = true; + directives.forEach((directive) => { + if ( + directive?.arguments && + (directive.name.value === 'include' || directive.name.value === 'skip') + ) { + // only consider the first argument + const argument = directive.arguments[0]; + // ensure the argument name is 'if' + const argumentHasVariables = + argument.value.kind === Kind.VARIABLE && argument.name.value === 'if'; + // access the value of the argument depending on whether it is passed as a variable or not + let directiveArgumentValue; + if (argument.value.kind === Kind.BOOLEAN) { + directiveArgumentValue = Boolean(argument.value.value); + } else if (argumentHasVariables) { + directiveArgumentValue = Boolean(this.variables[argument.value.name.value]); + } - return ( - (directive.name.value === 'include' && directiveArgumentValue === true) || - (directive.name.value === 'skip' && directiveArgumentValue === false) - ); - } - // return true to process all queried fields without/other directives - return true; + if ( + (directive.name.value === 'include' && directiveArgumentValue !== true) || + (directive.name.value === 'skip' && directiveArgumentValue !== false) + ) { + directiveCheck = false; + } + } + }); + return directiveCheck; } private selectionNode(node: SelectionNode, parentName: string): number { let complexity = 0; - // TODO: complete implementation of directives include and skip /** * process this node only if: * 1. there is no directive * 2. there is a directive named inlcude and the value is true * 3. there is a directive named skip and the value is false */ - const directive = node.directives; - if (directive && this.directiveCheck(directive[0])) { + if (node.directives && this.directiveCheck([...node.directives])) { this.depth += 1; if (this.depth > this.maxDepth) this.maxDepth = this.depth; // the kind of a field node will either be field, fragment spread or inline fragment @@ -218,7 +218,6 @@ class QueryParser { ? typeCondition.name.value.toLowerCase() : parentName; - // TODO: Handle directives like @include and @skip // subtract 1 before, and add one after, entering the fragment selection to negate the additional level of depth added this.depth -= 1; complexity += this.selectionSetNode(node.selectionSet, namedType); From f135be432b19e11e8c9d1e538228e801d6edfff7 Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Sat, 20 Aug 2022 08:59:02 -0700 Subject: [PATCH 4/8] added a test for multiple directives on one field --- test/analysis/typeComplexityAnalysis.test.ts | 47 +++++++++++++++++--- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/test/analysis/typeComplexityAnalysis.test.ts b/test/analysis/typeComplexityAnalysis.test.ts index c18be4b..6fe65d7 100644 --- a/test/analysis/typeComplexityAnalysis.test.ts +++ b/test/analysis/typeComplexityAnalysis.test.ts @@ -856,7 +856,7 @@ describe('Test getQueryTypeComplexity function', () => { // TODO: refine complexity analysis to consider directives includes and skip describe('with directives @includes and @skip', () => { - test('@includes on interfaces', () => { + test('@include on interfaces', () => { query = ` query { hero(episode: EMPIRE) { @@ -932,7 +932,7 @@ describe('Test getQueryTypeComplexity function', () => { expect(queryParser.processQuery(parse(query))).toBe(5); }); - test('@includes on object types', () => { + test('@include on object types', () => { query = `query { hero(episode: EMPIRE) { id, name @@ -1019,7 +1019,7 @@ describe('Test getQueryTypeComplexity function', () => { expect(queryParser.processQuery(parse(query))).toBe(2); }); - test('@includes with arguments and varibales', () => { + test('@include with arguments and varibales', () => { variables = { directive: false }; queryParser = new QueryParser(typeWeights, variables); query = `query ($directive: Boolean!){ @@ -1067,14 +1067,51 @@ describe('Test getQueryTypeComplexity function', () => { hero(episode: EMPIRE) { id, name } - human(id: 1) @includes(when: false) { + human(id: 1) @include(when: false) { id, name, homePlanet } }`; // 1 query + 1 hero - expect(queryParser.processQuery(parse(query))).toBe(3); + expect(queryParser.processQuery(parse(query))).toBe(2); + }); + + test('@include with other diretcives', () => { + query = `query { + hero(episode: EMPIRE) { + id, + name + friends(first: 3) { + name + } + } + human(id: 1) @ignore(if: true) @include(if: false) { + id, + name, + homePlanet + } + }`; + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 1 human + expect(queryParser.processQuery(parse(query))).toBe(5); + query = `query { + hero(episode: EMPIRE) { + id, + name + friends(first: 3) { + name + } + } + human(id: 1) @ignore(if: true) @include(if: true) { + id, + name, + homePlanet + } + }`; + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + expect(queryParser.processQuery(parse(query))).toBe(6); }); }); From cb261a4cce6528ad711194b1b95c1cb9ef2aea75 Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Sat, 20 Aug 2022 09:20:17 -0700 Subject: [PATCH 5/8] updated tests for directives in complexity analysis to add differentiation between fields --- test/analysis/typeComplexityAnalysis.test.ts | 116 +++++++++++++------ 1 file changed, 83 insertions(+), 33 deletions(-) diff --git a/test/analysis/typeComplexityAnalysis.test.ts b/test/analysis/typeComplexityAnalysis.test.ts index 6fe65d7..44a30c6 100644 --- a/test/analysis/typeComplexityAnalysis.test.ts +++ b/test/analysis/typeComplexityAnalysis.test.ts @@ -935,7 +935,11 @@ describe('Test getQueryTypeComplexity function', () => { test('@include on object types', () => { query = `query { hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @include(if: true) { id, @@ -943,12 +947,17 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero + 1 human - expect(queryParser.processQuery(parse(query))).toBe(3); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + 1 human + expect(queryParser.processQuery(parse(query))).toBe(6); query = `query { hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @include(if: false) { id, @@ -956,14 +965,19 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero - expect(queryParser.processQuery(parse(query))).toBe(2); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + expect(queryParser.processQuery(parse(query))).toBe(5); }); test('@skip on object types', () => { query = `query { hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @skip(if: true) { id, @@ -971,12 +985,17 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero - expect(queryParser.processQuery(parse(query))).toBe(2); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + expect(queryParser.processQuery(parse(query))).toBe(5); query = `query { hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @skip(if: false) { id, @@ -984,8 +1003,9 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero + 1 human - expect(queryParser.processQuery(parse(query))).toBe(3); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + 1 human + expect(queryParser.processQuery(parse(query))).toBe(6); }); test('@skip with arguments and varibales', () => { @@ -993,7 +1013,11 @@ describe('Test getQueryTypeComplexity function', () => { queryParser = new QueryParser(typeWeights, variables); query = `query ($directive: Boolean!){ hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @skip(if: $directive) { id, @@ -1001,13 +1025,18 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero + 1 human - expect(queryParser.processQuery(parse(query))).toBe(3); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + 1 human + expect(queryParser.processQuery(parse(query))).toBe(6); variables = { directive: true }; queryParser = new QueryParser(typeWeights, variables); query = `query ($directive: Boolean!){ hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @skip(if: $directive) { id, @@ -1015,8 +1044,9 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero - expect(queryParser.processQuery(parse(query))).toBe(2); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + expect(queryParser.processQuery(parse(query))).toBe(5); }); test('@include with arguments and varibales', () => { @@ -1024,7 +1054,11 @@ describe('Test getQueryTypeComplexity function', () => { queryParser = new QueryParser(typeWeights, variables); query = `query ($directive: Boolean!){ hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @include(if: $directive) { id, @@ -1032,13 +1066,18 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero - expect(queryParser.processQuery(parse(query))).toBe(2); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + expect(queryParser.processQuery(parse(query))).toBe(5); variables = { directive: true }; queryParser = new QueryParser(typeWeights, variables); query = `query ($directive: Boolean!){ hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @include(if: $directive) { id, @@ -1046,14 +1085,19 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero + 1 human - expect(queryParser.processQuery(parse(query))).toBe(3); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + 1 human + expect(queryParser.processQuery(parse(query))).toBe(6); }); test('and other directives or arguments are ignored', () => { query = `query { hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @ignore(if: true) { id, @@ -1061,11 +1105,16 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero + 1 human - expect(queryParser.processQuery(parse(query))).toBe(3); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3friends + 1 human + expect(queryParser.processQuery(parse(query))).toBe(6); query = `query { hero(episode: EMPIRE) { - id, name + id, + name + friends(first: 3) { + name + } } human(id: 1) @include(when: false) { id, @@ -1073,11 +1122,12 @@ describe('Test getQueryTypeComplexity function', () => { homePlanet } }`; - // 1 query + 1 hero - expect(queryParser.processQuery(parse(query))).toBe(2); + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 3 friends + 1 hero 1 human + expect(queryParser.processQuery(parse(query))).toBe(6); }); - test('@include with other diretcives', () => { + test('@include with other directives', () => { query = `query { hero(episode: EMPIRE) { id, @@ -1093,7 +1143,7 @@ describe('Test getQueryTypeComplexity function', () => { } }`; mockCharacterFriendsFunction.mockReturnValueOnce(3); - // 1 query + 1 hero + 1 human + // 1 query + 1 hero + 3 friends expect(queryParser.processQuery(parse(query))).toBe(5); query = `query { hero(episode: EMPIRE) { @@ -1110,7 +1160,7 @@ describe('Test getQueryTypeComplexity function', () => { } }`; mockCharacterFriendsFunction.mockReturnValueOnce(3); - // 1 query + 1 hero + // 1 query + 1 hero + 3 friends + 1 human expect(queryParser.processQuery(parse(query))).toBe(6); }); }); From 0ccd91e22244a0751b4f41f3a200d1187c0196a7 Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Sat, 20 Aug 2022 09:58:45 -0700 Subject: [PATCH 6/8] refactored directive check in complexity analysis to simplyify code --- src/analysis/QueryParser.ts | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/analysis/QueryParser.ts b/src/analysis/QueryParser.ts index ccab636..1be5a24 100644 --- a/src/analysis/QueryParser.ts +++ b/src/analysis/QueryParser.ts @@ -148,24 +148,22 @@ class QueryParser { /** * Return true if: - * 1. there is no directive skip or include - * 2. there is a directive named inlcude and the value is true - * 3. there is a directive named skip and the value is false + * 2. there is a directive named inlcude and the value is false + * 3. there is a directive named skip and the value is true */ - private directiveCheck(directives: DirectiveNode[]): boolean { - // set the default of the return value of directiveCheck to true, reset to false if the directive include or skip is found with the argument is false or true respectively - let directiveCheck = true; + private directiveExcludeField(directives: DirectiveNode[]): boolean { + let skipField = false; + directives.forEach((directive) => { if ( directive?.arguments && - (directive.name.value === 'include' || directive.name.value === 'skip') + (directive.name.value === 'include' || directive.name.value === 'skip') && + directive.arguments[0].name.value === 'if' ) { // only consider the first argument const argument = directive.arguments[0]; - // ensure the argument name is 'if' - const argumentHasVariables = - argument.value.kind === Kind.VARIABLE && argument.name.value === 'if'; - // access the value of the argument depending on whether it is passed as a variable or not + + const argumentHasVariables = argument.value.kind === Kind.VARIABLE; let directiveArgumentValue; if (argument.value.kind === Kind.BOOLEAN) { directiveArgumentValue = Boolean(argument.value.value); @@ -174,14 +172,15 @@ class QueryParser { } if ( - (directive.name.value === 'include' && directiveArgumentValue !== true) || - (directive.name.value === 'skip' && directiveArgumentValue !== false) + (directive.name.value === 'include' && directiveArgumentValue === false) || + (directive.name.value === 'skip' && directiveArgumentValue === true) ) { - directiveCheck = false; + skipField = true; } } }); - return directiveCheck; + + return skipField; } private selectionNode(node: SelectionNode, parentName: string): number { @@ -192,7 +191,7 @@ class QueryParser { * 2. there is a directive named inlcude and the value is true * 3. there is a directive named skip and the value is false */ - if (node.directives && this.directiveCheck([...node.directives])) { + if (node.directives && !this.directiveExcludeField([...node.directives])) { this.depth += 1; if (this.depth > this.maxDepth) this.maxDepth = this.depth; // the kind of a field node will either be field, fragment spread or inline fragment From 9efe3349e6d444d34b251ff7a019c9cc1e2a63d3 Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Sat, 20 Aug 2022 09:59:12 -0700 Subject: [PATCH 7/8] added a test for skip directive and other directives on the same field --- test/analysis/typeComplexityAnalysis.test.ts | 37 ++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/analysis/typeComplexityAnalysis.test.ts b/test/analysis/typeComplexityAnalysis.test.ts index 44a30c6..6ae4ae7 100644 --- a/test/analysis/typeComplexityAnalysis.test.ts +++ b/test/analysis/typeComplexityAnalysis.test.ts @@ -1163,6 +1163,43 @@ describe('Test getQueryTypeComplexity function', () => { // 1 query + 1 hero + 3 friends + 1 human expect(queryParser.processQuery(parse(query))).toBe(6); }); + + test('@skip with other directives', () => { + query = `query { + hero(episode: EMPIRE) { + id, + name + friends(first: 3) { + name + } + } + human(id: 1) @ignore(if: true) @skip(if: false) { + id, + name, + homePlanet + } + }`; + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + expect(queryParser.processQuery(parse(query))).toBe(6); + query = `query { + hero(episode: EMPIRE) { + id, + name + friends(first: 3) { + name + } + } + human(id: 1) @ignore(if: true) @skip(if: true) { + id, + name, + homePlanet + } + }`; + mockCharacterFriendsFunction.mockReturnValueOnce(3); + // 1 query + 1 hero + 3 friends + 1 human + expect(queryParser.processQuery(parse(query))).toBe(5); + }); }); describe('with nested lists', () => { From 340179526fa6748f2d2a45b47f3a4e239a40ab9f Mon Sep 17 00:00:00 2001 From: "[Evan McNeely]" Date: Sat, 20 Aug 2022 10:22:43 -0700 Subject: [PATCH 8/8] updated comments on changes --- src/analysis/QueryParser.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/analysis/QueryParser.ts b/src/analysis/QueryParser.ts index 1be5a24..3ecf5fc 100644 --- a/src/analysis/QueryParser.ts +++ b/src/analysis/QueryParser.ts @@ -23,9 +23,9 @@ import { FieldWeight, TypeWeightObject, Variables } from '../@types/buildTypeWei * | / * | Selection Node * | (Field, Inline fragment, directives and fragment spread) - * | | | \ \ - * | Field Node | \ \ - * | | | directiveCheck fragmentCache + * | | | \ \ + * | Field Node | \ \ + * | | | directiveExcludeField fragmentCache * |<--calculateCast | * | | * |<------------------| @@ -187,7 +187,7 @@ class QueryParser { let complexity = 0; /** * process this node only if: - * 1. there is no directive + * 1. there is no include or skip directive * 2. there is a directive named inlcude and the value is true * 3. there is a directive named skip and the value is false */