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

Allow separation of variable value coercion from the rest of execution #3679

Closed
glasser opened this issue Jul 21, 2022 · 7 comments
Closed

Comments

@glasser
Copy link
Contributor

glasser commented Jul 21, 2022

graphql-js lets you run the entire GraphQL operation with a single graphql function, or break it apart into different steps (parse, validate, execute) if you want a bit more control (for example, handling errors at different stages differently). However, the execute step is actually three steps combined into one:

  • Selecting the required operation from the document (execute is the first step that looks at the explicitly-provided operationName from the request)
  • Coercing variable values
  • Actual execution

Servers should be able to separate out these three steps and run them individually if they want. Specifically, it's likely that error handling should work differently for errors in the first two steps vs in the third. Apollo Server tries to provide a 400 HTTP status code for errors in the first two steps and a 200 for errors at execution time; this is also what the current GraphQL over HTTP spec suggests via a SHOULD, for response content-type application/graphql+json.

It's not too hard for a server to implement the first step manually, but differentiating between variable coercion errors and field errors is challenging. For example, Apollo Server has this hacky error-matching code.

Ideally, you would be able to choose between running execute and running three resolveOperation, coerceVariableValues, and executeResolvedOperation functions (passing return values from the previous functions to the next ones). This should be fully backwards-compatible since it's just adding a new API.

(I thought there was already an issue for this but I had trouble finding it.)

@yaacovCR
Copy link
Contributor

See #3644 and #3658

these PRs came from different directions to solve this issue but both include different major refactors

#3658 was started to address breaking up execution into different stages and ended up with one type of major refactor

#3644 was started to integrate execute and subscribe but also ended up exporting “helpers” to address above issue.

the basic thing is that in a pipeline that includes subscriptions, you have to find the operation to figure out how to execute, so users of graphql-js may have to peak inside the operation to figure out how to execute, which is another reason to give users greater control over the pipeline.

this may have been mitigated somewhat for setups that use different endpoints or protocols for subscriptions (or perhaps some other factor?)

anyway, thanks for raising a dedicated issue to track this

@yaacovCR
Copy link
Contributor

Also see #3215

@glasser
Copy link
Contributor Author

glasser commented Jul 21, 2022

(note that you can manage the 400 by checking for lack of data field on the return, but if you want to differentiate "couldn't select operation" from "couldn't coerce variables" it's harder.)

@glasser
Copy link
Contributor Author

glasser commented Jul 21, 2022

Thanks Yaacov, I knew there had been some recent work here but I wanted something to link in a GraphQL-over-HTTP discussion.

@yaacovCR
Copy link
Contributor

yaacovCR commented Jul 21, 2022

Sure. And thank u again @glasser for creating this dedicated issue to track these efforts. Just a note on #3215 that’s kind of a least change approach just to export the tools surrounding ExecutionContext. We should probably make sure to separate the internal errors array from the rest of the context, which is basically the sum total of my approach to this in #3644.

This issue may now be a great place for you @glasser or @IvanGoncharov or anybody interested to compare the different approaches in the PRs above, or suggest a different approach or just generally chime in on what the best way to move forward e.g. if we should solve this/refactor in stages, etc, etc.

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 2, 2024

This should be solved at least in part by #4210 and was my effort at minimum change.

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 6, 2024

I guess can only be considered closed once #4215 lands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants