Skip to content

Completed analysis of include and skip directives #106

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Completed analysis of include and skip directives #106

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fc3d616
completed the analysis of includes and skip directives on objects and…
evanmcneely Aug 18, 2022
fddd78a
updated QueryParser flow chart
evanmcneely Aug 18, 2022
9d01f85
updated directiveCheck in complexity analysis to consider all directi…
evanmcneely Aug 20, 2022
f135be4
added a test for multiple directives on one field
evanmcneely Aug 20, 2022
cb261a4
updated tests for directives in complexity analysis to add differenti…
evanmcneely Aug 20, 2022
0ccd91e
refactored directive check in complexity analysis to simplyify code
evanmcneely Aug 20, 2022
9efe334
added a test for skip directive and other directives on the same field
evanmcneely Aug 20, 2022
3401795
updated comments on changes
evanmcneely Aug 20, 2022
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
82 changes: 45 additions & 37 deletions src/analysis/QueryParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
* | |
* |<------------------|
Expand Down Expand Up @@ -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 =
Expand All @@ -173,6 +178,7 @@ class QueryParser {
(directive.name.value === 'skip' && directiveArgumentValue === false)
);
}
// return true to process all queried fields without/other directives
return true;
}

Expand All @@ -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])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also handle cases where there are multiple directives attached to this node rather than just checking the first directive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this. What is your take on this check:

if (node.directives && !this.directiveExcludeField([...node.directives])) {
    //code
}

node.directives is always truthy but TS requires the check in order to access the array value at node.directives.

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this TODO

// 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;
}

Expand Down
48 changes: 40 additions & 8 deletions test/analysis/typeComplexityAnalysis.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ describe('Test getQueryTypeComplexity function', () => {
});

// TODO: refine complexity analysis to consider directives includes and skip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this TODO

xdescribe('with directives @includes and @skip', () => {
describe('with directives @includes and @skip', () => {
test('@includes on interfaces', () => {
query = `
query {
Expand All @@ -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) {
Expand All @@ -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);
});
Expand All @@ -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) {
Expand Down Expand Up @@ -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!){
Expand All @@ -1008,17 +1009,48 @@ describe('Test getQueryTypeComplexity function', () => {
hero(episode: EMPIRE) {
id, name
}
human(id: 1) @includes(if: $directive) {
human(id: 1) @skip(if: $directive) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The weight of Human and hero are both 1. Can you add something that differentiates the weights so we know that this is working.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. Differentiate the weights of here and human

id, name
}
human(id: 1) @include(if: $directive) {
id,
name,
homePlanet
}
}`;
// 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

here as well

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Differentiate weights here as well

id, name
Expand Down