Operation type directive is overriding directives defined in ancestor fields
leoloso opened this issue · 5 comments
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...)
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
Ops, I closed by mistake.
This branch in my fork deals with this issue:
https://github.com/getpop/graphql-parser/tree/fix-overriding-directives
Can you make a PR?
Alright. I just added a test, will submit the PR now.
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
}
}