youshido-php / GraphQL

Pure PHP realization of GraphQL protocol
MIT License
710 stars 107 forks source link

Scoping errors to the offending query when batching queries #208

Closed Pirokiko closed 6 years ago

Pirokiko commented 6 years ago

When multiple queries are batched together, it appears that errors in one query are retained and consequently also added to the response for the following queries. This happens even if one of the following queries is executing correctly, such that we have the situation that a query returns both a result as well as errors (which i believe is in conflict with the GraphQL spec).

Example: I used the default hello world example

[{
    "variables":{"name": "World"},
    "operationName":"hello",
    "query":"query hello($name: String){ hello(name: $name) }"
},{
    "variables":{"name": "error"},
    "operationName":"hello",
    "query":"query hello($name: String){ hello(name: $name) }"
},{
    "variables":{"name": "throw"},
    "operationName":"hello",
    "query":"query hello($name: String){ hello(name: $name) }"
},{
    "variables":{"name": "World"},
    "operationName":"hello",
    "query":"query hello($name: String){ hello(name: $name) }"
}]

For this the first and last queries work fine, while the second and third throw an error. This results in the following response

[{
    "data": { "hello": "Hello World" }
},{
    "data": { "hello": null },
    "errors": [{"message": "Problem found: error"}]
},{
    "data": { "hello": null },
    "errors": [{"message": "Problem found: error"},{"message": "Problem found: throw"}]
},{
    "data": { "hello": "Hello World" },
    "errors": [{"message": "Problem found: error"},{"message": "Problem found: throw"}]
}]

I did not expect to see the "Problem found: error" message in the third query result. The last query result appears to be in conflict with the spec as it is returning both a data response and errors (even though those errors did not occur during that query execution)

The fix appears to be quite simple: In the processPayload method of the Processor class add a line at the top to reset the errors in the execution context. The data is also reset there so it makes sense to also reset the errors at that point. This does seem to fix the issues laid above.

public function processPayload($payload, $variables = [], $reducers = [])
    {
        $this->data = [];
        //Add this line
        $this->executionContext->clearErrors();
        ......
    }

However, since i have no clue of the impact of doing this, i hope someone else can take a look at this.

fritz-gerneth commented 6 years ago

This already has been disussed here https://github.com/Youshido/GraphQL/issues/106 . As you are batching I assume you iterate over the batched array anyway so you can just call clearErrors by yourself.

Pirokiko commented 6 years ago

I guess i need to upgrade my search skills, thanks for the reference to the issue. I'll close the issue considering the discussion in #106.