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

Define 'execution' as in 'before execution begins' #894

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Oct 18, 2021

UPDATE: 2025-04-03

There are various parts of the spec that relate to a concept of "execution":

  • "before execution begins" defines the boundary between request error/field error
  • "the request must fail without execution" indicates that no data should be produced
  • "validation is performed in the context of a request immediately before execution"

All of these things actually happen during ExecuteRequest(), which makes it confusing to the reader that ExecuteRequest() is not itself considered execution despite the name.

I've thus defined execution, and indicated that ExecuteRequest() contains the preamble for "execution", followed by "execution" itself.

See also: #1154 which renames these algorithms for further clarity.

Previous description

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
Copy link

netlify bot commented Oct 18, 2021

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit d60481a
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/67ee61f29fbe64000835c5ff
😎 Deploy Preview https://deploy-preview-894--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@yaacovCR
Copy link
Contributor

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
Copy link
Contributor

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

@benjie
Copy link
Member Author

benjie commented Oct 19, 2021

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

IvanGoncharov commented Jun 13, 2022

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
Copy link
Member Author

benjie commented Jun 13, 2022

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.)

@benjie benjie force-pushed the before-execution-begins-note branch from 18b38cd to 0e175cb Compare March 7, 2025 17:16
Comment on lines 61 to 62
Note: Request errors (including those raised during {ExecuteRequest()}) occur
before execution begins; when a request error is raised the `data` entry should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very hard for my brain to process. I may already be in ExecuteRequest() but still before execution begins?This feels weird.

Do we have a formal definition of what "execution" is and when it starts that we could link to maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the purpose of this PR is 💯

Execution begins the moment that the data property should be added to the response, which is when the root selection set is executed (after validating variables, establishing a stream in the case of subscriptions, etc).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a different tact by defining execution, see what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 We define _execution_ as the process of executing the operation's root
selection set through {ExecuteSelectionSet()}, thus _execution_ begins when
{ExecuteSelectionSet()} is called for the first time in a request. The
{ExecuteRequest()} algorithm is a preamble for _execution_.

This is clarifying but also also adding more language to processs. I think a rename of ExecuteRequest() would be even more clarifying.

Copy link
Member Author

@benjie benjie Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not both? We use the term "execution" thoughout the spec, it makes sense to have a definition for it IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@benjie benjie changed the title Clarify 'before execution begins' in response Define 'execution' as in 'before execution begins' Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants