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

Completed analysis of include and skip directives #106

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

Conversation

evanmcneely
Copy link
Contributor

Summary

  1. adds support for @skip and @include directives for more accurate cost analysis for queries using these directives
  2. adds tests for these directives on objects, interfaces/fragments

Type of Change

  • New feature (non-breaking change which adds functionality)

Issues

closes #95

Evidence

Screen Shot 2022-08-18 at 2 01 59 PM
Screen Shot 2022-08-18 at 2 05 54 PM

@evanmcneely evanmcneely added the enhancement New feature or request label Aug 18, 2022
Copy link
Collaborator

@shalarewicz shalarewicz left a comment

Choose a reason for hiding this comment

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

Awesome job! This really starts to round out the tooling and provides much more support!
Minor comments on tests mostly.

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

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.

@@ -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

@@ -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.

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

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

});

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

Copy link
Collaborator

@shalarewicz shalarewicz left a comment

Choose a reason for hiding this comment

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

Requesting changes per previous comment.

@evanmcneely
Copy link
Contributor Author

evanmcneely commented Aug 20, 2022

I ended up refactoring things a bit to check all directives in the array.

Screen Shot 2022-08-20 at 10 31 24 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account for include and skip directives
2 participants