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.

Sign up for GitHub

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

Jump to bottom

Run error presenter in context during unmarshal #1305

Closed
wants to merge 1 commit into from
Closed

Run error presenter in context during unmarshal #1305

wants to merge 1 commit into from

Conversation

lwc
Copy link
Member

@lwc lwc commented Aug 24, 2020

WrapErrorWithInputPath introduced in #1115 causes input error types to be clobbered (#1291).

My original fix was going to be to resurrect #1263 and push forward with go 1.13 errors, but when I was taking stock of the error flows for marshal and unmarshal cases, I noticed how asymmetrical they are.

Errors during marshal call the execution context in scope and with all necessary context, whereas errors during unmarshal flow back up the call stack and are handled at fairly high levels after a bunch of context is lost.

This patch is an attempt to bring the unmarshal flow more in line with the marshal flow, removing the need to wrap errors to attach the extra context.

Errors are still used as flow control back up the stack though, so we need to avoid over reporting errors by checking if children have errored, this isn't super elegant.

#1115 also introduced FieldInputContext separate from FieldContext, which invalidates some existing assumptions about how to extract the gql path from context. GetPathFromContext is introduced to smooth over the difference, prioritising the input context when both are present.

The default error presenter is updated to use this new path source, but custom error presenters will need to be updated.

Alternatives / thoughts / questions

  • Should FieldInputContext be separate from FieldContext? A single type with some sparse properties might provide better BC
  • Rather than make unmarshal work like marshal, we could go the other way and lean in on error wrapping

Fixes: #1291

I have:

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

@coveralls
Copy link

coveralls commented Aug 24, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 66.311% when pulling 507bfd7 on error-symmetry into 622316e on master.

@lwc lwc requested a review from vektah August 24, 2020 04:15
lwc added a commit that referenced this pull request Aug 28, 2020
@lwc
Always wrap user errors
be7c190 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
lwc added a commit that referenced this pull request Aug 28, 2020
@lwc
Always wrap user errors
1840e9e 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
lwc added a commit that referenced this pull request Aug 28, 2020
@lwc
Always wrap user errors
6a0ff1c 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
lwc added a commit that referenced this pull request Aug 28, 2020
@lwc
Always wrap user errors
a7b1525 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
@lwc lwc mentioned this pull request Aug 28, 2020
Merged
2 tasks
lwc added a commit that referenced this pull request Aug 28, 2020
@lwc
Always wrap user errors
450dc9a 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
lwc added a commit that referenced this pull request Aug 28, 2020
@lwc
Always wrap user errors
e024656 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
lwc added a commit that referenced this pull request Sep 17, 2020
@lwc
Always wrap user errors
a10a62b 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
lwc added a commit that referenced this pull request Sep 17, 2020
@lwc
Always wrap user errors
e821b97 
This is an alternative to #1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
@lwc lwc closed this Sep 21, 2020
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
@lwc
Always wrap user errors
5d50337 
This is an alternative to 99designs#1305 that leans into error wrapping instead of away from it.

Requires use of go 1.13 error unwrapping.

On measure I think I prefer this approach, even though it's a bigger BC break:
- There's less mutex juggling
- It has never felt right to me that we make the user deal with path when overriding the error presenter
- The default error presenter is now incredibly simple

Questions:
- Are we comfortable with supporting 1.13 and up?
- Should we change the signature of `ErrorPresenterFunc` to `func(ctx context.Context, err *gqlerror.Error) *gqlerror.Error`?
    - It always is now, and breaking BC will force users to address the requirement for `errors.As`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vektah vektah Awaiting requested review from vektah

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

INPUT_FIELD_DEFINITION directive handler returned errors are clobbered
2 participants
@lwc @coveralls