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

Add input path in unmarshaling errors #1115

Merged
merged 8 commits into from
Jul 26, 2020

Conversation

bowd
Copy link
Contributor

@bowd bowd commented Mar 20, 2020

Related to #1113

Problem summary

When relying on custom scalar, especially in mutations, you offload an aspect of validation to the unmarshaling step. This means that you ensure these custom types have the right structure in one place and then you can rely on them in the next layers.

The problem with the current implementation is that errors that happen as a result of scalar unmarshaling only have path information up to the mutation that they occur in. And it's impossible to reconstruct what field in the input is responsible for the error if you have multiple instances of the scalar in a nested input structure. And makes it impossible for the frontend to link the error to a form field, for example.

Solution

I've added a new context type FieldInputContext which acts similarly to FieldContext but is responsible for keeping track of the nesting inside of the arguments received by queries or mutations. This context is then used during unmarshaling to wrap the error returned into a gqlerror.Error if it's not already or just inject the path if it is. This lets developers return gqlerror.Error from their unmarshaling logic in order to inject additional data in extensions

See the updated docs for additional details.

Codegen touchpoints:

  • args.gotpl - created nested input context for each argument
  • input.gotpl - created nested input context for each field in an input
  • type.gotpl - treat all unmarshaling scenarios

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@bowd bowd force-pushed the add-input-path-for-unmarshaling branch from 6ac4b6b to fd615cf Compare March 20, 2020 08:19
@bowd
Copy link
Contributor Author

bowd commented Mar 20, 2020

I was thinking now that this is, in a way, a breaking change because some implementations might be expecting the old pat somewhere downstream. So this feature could be enabled through configuration so you can switch between the partial field path or the full path for the errors.

@bowd bowd changed the title Add input path for unmarshaling Add input path in unmarshaling errors Mar 20, 2020
@bowd
Copy link
Contributor Author

bowd commented Mar 20, 2020

An extension to this that I would like to make and I didn't handle in this PR would be to not fail on the first unmarshal error in the arg tree, but collect all errors in the tree using a multierror package or something similar to handle merging of errors. This way we could reduce round-trips by returning all errors that we can find in the first call.

@stale
Copy link

stale bot commented May 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 19, 2020
@stale stale bot closed this May 26, 2020
@mtibben mtibben reopened this Jun 11, 2020
@stale stale bot removed the stale label Jun 11, 2020
@coveralls
Copy link

coveralls commented Jul 26, 2020

Coverage Status

Coverage increased (+0.2%) to 66.419% when pulling bde4291 on bowd:add-input-path-for-unmarshaling into 6be2e9d on 99designs:master.

@vektah vektah force-pushed the add-input-path-for-unmarshaling branch from a8530f3 to bde4291 Compare July 26, 2020 03:38
@vektah
Copy link
Collaborator

vektah commented Jul 26, 2020

Thanks, this has annoyed me for a long time. 🎉

I've added a small commit to drop the custom context names to be consistent with the rest of the context usage (eg field context).

@vektah vektah merged commit ce964c1 into 99designs:master Jul 26, 2020
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
…haling

Add input path in unmarshaling errors
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

Successfully merging this pull request may close these issues.

4 participants