Ensure data entry exists for execution errors

[swalkinshaw]

According to the spec, the data entry should always exist once execution starts. Only if an error is raised before execution starts should the data key be omitted.

The only test that needed updating was when an ExecutionError was raised during the execute_query trace hook. Despite this running before super, this still happens "during execution" and should have the data key present.

Ref: https://spec.graphql.org/draft/#sec-Data

If an error was raised before execution begins, the response must be a request error result which will result in no response data.

If an error was raised during the execution that prevented a valid response, the "data" entry in the response should be null.

Arguably, this should have a better test case to show the issue beyond just a trace hook. In our case, we raised an ExecutionError within a root mutation's authorized? method and it resulted in no data key which is clearly incorrect.

@rmosolgo any idea where that test case should go?

[swalkinshaw]

I'm not 100% sure on this solution but this rescue wraps execution so any exception rescued here needs to result in the data key being present.

[gmac]

Good catch. Looks like the correct fix to me – we should be keying nil as the multiplex result.

[rmosolgo]

LGTM 👍

It looks like rails_master is failing because Rails master is version 8.2.x, and it's missing a snapshot. I'll update that when I get a chance.

that test case

I don't have a particularly good idea... seems like updating the existing test case is a good start. Is there another real-life case that you encountered that we could include in the test suite?

[swalkinshaw]

Turns out there was an existing test case for this but the assertion used assert_nil which passed either way. I've updated it to assert on the full result.