Clarify 'before execution begins' in response

[benjie]

This is cleared up in the "Errors" -> "Request errors" section just 20 lines later, but for people dipping in to see what should happen with "data" it may not be obvious that the concept of "before execution begins" and that of entering the "Executing Requests" and/or "Executing Operations" sections differs due to the subtlety of when "request errors" can be raised.

[netlify]

✔️ Deploy Preview for graphql-spec-draft ready!

🔨 Explore the source changes: d7eaf72

🔍 Inspect the deploy log: https://app.netlify.com/sites/graphql-spec-draft/deploys/616dc0d8a0d0a900078fad6b

😎 Browse the preview: https://deploy-preview-894--graphql-spec-draft.netlify.app/draft

[yaacovCR]

I guess the question is why (or why not) some errors should be considered Request errors, for example, submitting a mutation operation when no mutation root type exists.

it is the fault of the graphql user, so should be considered a request error by the rough explanation given, but it does not occur during the executing requests section.

[yaacovCR]

But if you don’t validate, you can get user errors during field execution so ???

[benjie]

For that specific case that should be something raised during validation; the assert in the request execution phase is just to ensure that the assumption made by validation is still correct (useful if validation happened a long time previous, maybe the schema has changed since then).

[IvanGoncharov]

@benjie @yaacovCR I think we need to clean criteria for what consider executions and what not.
Otherwise, we just adding additional notes that clarify it but not fully.
I suggest simple criteria:
If error contains a non-empty path it's execution error and data is always present (maybe null).

[yaacovCR]

@IvanGoncharov that definition while clean is a bit circular, whether or not a path should be empty depends on whether we have started execution…

[IvanGoncharov]

My second attempt, to produce "non-confusing criteria" 😄
What if we just say that data should be null only due to "error propagation to the root":
https://spec.graphql.org/draft/#sec-Handling-Field-Errors
De facto it's the only algorithm in a spec that explicitly requires setting data to null.
It is very easy to check programmatically, and I actually think it matches the original intention.

[benjie]

I think that's a fair assessment: data should only be null in the event of a field error (including a non-null assertion) being triggered on a root field. In all other cases it should be either an object (map), or not be present. (This comment not intended to be taken as a strictly worded specification.)