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

ID! validation is returning an error of type string. #798

Closed
thedodd opened this issue Jul 19, 2019 · 7 comments
Closed

ID! validation is returning an error of type string. #798

thedodd opened this issue Jul 19, 2019 · 7 comments

Comments

@thedodd
Copy link

thedodd commented Jul 19, 2019

What happened?

When submitting a request where a literal integer is supplied by the client instead of a string literal for the ID! type, the framework's error handling is quite different. The error is presented to the error handler configured by the user, instead of just being flagged as a malformed request and returned as a 400 to the caller automatically.

There appears to be a difference between the type validation of the high-level request object vs the validation of field parameters. This inconsistency complicates error handling, such that it is unclear where in the stack the error is coming from. Perhaps it would be good to use a more identifiable error type so that we can handle it appropriately in the error handler logic.

The question needs to be asked: "is this some unhandled error coming from bad business logic in my code (an ISE), or is this an error which can be presented as-is to the caller?"

What did you expect?

I expected consistency. This is a type validation error. The gqlgen stack should, arguably, be handling this error during the main validation phase and return an error before it ever leaves the gqlgen stack.

Minimal graphql.schema and models to reproduce

type Query {
    getProject(id: ID!): String!
}

Then just query that field with an int instead of a string.

versions

  • gqlgen version? v0.9.1
  • go version? 1.12.6
  • dep or go modules? go modules
@thedodd
Copy link
Author

thedodd commented Aug 21, 2019

@vektah &| @neelance just wanted to see if either of you have any thoughts on the best way to deal with this. Thanks for all of the work that you've put into this project!

@stale
Copy link

stale bot commented Oct 20, 2019

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 stale and removed stale labels Oct 20, 2019
@thedodd
Copy link
Author

thedodd commented Oct 21, 2019

Just adding another heartbeat here (to keep the stale bot reaper from killing this issue).

@cfilby
Copy link

cfilby commented Oct 25, 2019

@thedodd I had a PR that was related to this, #902 - does that resolve your issue?

@thedodd
Copy link
Author

thedodd commented Oct 29, 2019

@cfilby that probably will address the issue. Have those changes been released/tagged? I'll test and report back.

@cfilby
Copy link

cfilby commented Oct 29, 2019

@thedodd it's merged into master but hasn't yet been included in a tagged release. You could test it directly by using the latest master commit instead of a versioned release?

@vvakame
Copy link
Collaborator

vvakame commented Dec 25, 2019

I close this issue. if its still haven't solved, please mention to me.

@vvakame vvakame closed this as completed Dec 25, 2019
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