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

Operation type directive is overriding directives defined in ancestor fields #242

Open
leoloso opened this issue Apr 2, 2020 · 5 comments · May be fixed by #245
Open

Operation type directive is overriding directives defined in ancestor fields #242

leoloso opened this issue Apr 2, 2020 · 5 comments · May be fixed by #245

Comments

@leoloso
Copy link

leoloso commented Apr 2, 2020

In the following query, all directives declared in the ancestor fields are lost, only the ones in the leaf fields are parsed correctly from the AST:

query {
  ancestorField @thisDirectiveIsLost { 
    leafField @thisDirectiveWorksWell
  }
}

For instance, in this query the @include directive is lost, and title is included when it should not:

query {
  post(id:1) @include(if: false) { 
    title
  }
}

Inspecting the code, I found out that in Parser/Parser.php, function parseOperation, the operation type directive is being set into $operation:

$operation->setDirectives($directives);

However, the $operation already had its own directives, parsed through parseBodyItem. Hence, these are being overridden.

Merging the 2 sets of directives, instead, it works well:

$operation->setDirectives(array_merge(
    $directives,
    $operation->getDirectives()
));

Is that the right solution? Must directives defined next to the operation type be copied into all ancestor fields? And what about leaf fields, which currently have no problem with their own directives? (This solution seems to fix the bug, but I don't know if I may be creating another one? I can't find any reference in the GraphQL spec to what is the expected behavior for operation type directives...)

@leoloso
Copy link
Author

leoloso commented Apr 2, 2020

To clarify: this bug also happens if no operation type directives were defined in the query; in that case, the overriding $directives is an empty array

@leoloso
Copy link
Author

leoloso commented Apr 4, 2020

Ops, I closed by mistake.

This branch in my fork deals with this issue:

https://github.com/getpop/graphql-parser/tree/fix-overriding-directives

@leoloso leoloso reopened this Apr 4, 2020
@Jalle19
Copy link
Collaborator

Jalle19 commented Apr 8, 2020

Can you make a PR?

@leoloso
Copy link
Author

leoloso commented Apr 12, 2020

Alright. I just added a test, will submit the PR now.

@leoloso
Copy link
Author

leoloso commented Apr 12, 2020

The bug happens only when adding "query" at the beginning of the query:

query { 
  user @include(if:false) {
    name
   }
}

This query had no issue:

{ 
  user @include(if:false) {
    name
   }
}

@leoloso leoloso linked a pull request Apr 12, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants