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

INPUT_FIELD_DEFINITION directive handler returned errors are clobbered #1291

Closed
dixieflatline76 opened this issue Aug 17, 2020 · 4 comments · Fixed by #1312
Closed

INPUT_FIELD_DEFINITION directive handler returned errors are clobbered #1291

dixieflatline76 opened this issue Aug 17, 2020 · 4 comments · Fixed by #1312

Comments

@dixieflatline76
Copy link

What happened?

When using an INPUT_FIELD_DEFINITION directive, if the implemented directive handler returns a custom error (meant for leveraging custom error presenter to deliver tailored error graph style and info), it is overwritten with a gqlerror with only the error string copied over. This is a break introduced in v0.12.x, same code runs fine in v0.11.x and v0.10.x.

What did you expect?

Errors returned in any directive handlers should be propagated without lost of information until the custom error presenters are run.

Minimal graphql.schema and models to reproduce

directive @titleField(fieldName: String) on INPUT_FIELD_DEFINITION
directive @genresField(fieldName: String) on INPUT_FIELD_DEFINITION

input NewGame {
title: String! @titleField # Add directive to input type field to pattern validation and string case normalization
genres: [Genre!]! @genresField(fieldName:"genres") # Refer to demoserver.go to see we check for min 1 and max of 2 genres, and they must be unique
platforms: [NewGamePlatform!]!
}

versions

  • gqlgen version v0.12.1
  • go version? 1.14.2
  • dep or go modules?
@lwc
Copy link
Member

lwc commented Sep 21, 2020

@dixieflatline76 can you confirm that v0.13.0 fixes this for you, after swapping from type assertions to errors.As?

@dixieflatline76
Copy link
Author

@lwc thanks, I will test this out.

@dixieflatline76
Copy link
Author

@lwc just curious, looks like the default error handler just does a hard cast no test on the incoming error (i noticed it cause it crashed one of my unit tests). Wouldn't it be better to either just change the signature graphql.ErrorPresenterFunc and the default error presenter to accept a gqlerror?

@dixieflatline76
Copy link
Author

dixieflatline76 commented Sep 22, 2020

@dixieflatline76 can you confirm that v0.13.0 fixes this for you, after swapping from type assertions to errors.As?

@lwc I can confirm that the issue I reported is fixed with v0.13.0. Thank you!

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