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

Handle unmarshalling errors better #137

Closed
markphelps opened this issue Sep 22, 2019 · 6 comments
Closed

Handle unmarshalling errors better #137

markphelps opened this issue Sep 22, 2019 · 6 comments
Assignees

Comments

@markphelps
Copy link
Collaborator

It would be great if we could handle unmarshalling errors more gracefully during evaluation.

Example

Screen Shot 2019-09-22 at 11 08 31 AM

Ideas

  1. Catch unmarshalling errors and return a nicer error that informs the user that values must be strings
  2. Infer "true" from true and 'autobox' the type into a string
@markphelps markphelps self-assigned this Sep 22, 2019
@markphelps markphelps removed their assignment Oct 4, 2019
@markphelps
Copy link
Collaborator Author

This is handled by grpc-gateway, so we should implement our own error handler per: https://mycodesmells.com/post/grpc-gateway-error-handler

https://grpc-ecosystem.github.io/grpc-gateway/docs/customizingyourgateway.html

@aaronraff
Copy link
Contributor

I can take a a look into this one. I'm leaning towards starting with idea 1, if that sounds like a good plan to you.

@aaronraff
Copy link
Contributor

I started taking a look into this over the weekend. I took a look at the blog post that you linked in this issue. After implementing that, this is the error that is made available to us through the custom handler: rpc error: code = InvalidArgument desc = json: cannot unmarshal bool into Go value of type string.

It seems like we would probably need to add some custom logic when decoding the json (https://golang.org/pkg/encoding/json/#Decoder). Is this desired, or did you have a simpler solution in mind for this? Maybe I am missing something here.

@markphelps
Copy link
Collaborator Author

Thanks for taking a look @aaronraff!

My initial thoughts were to be able to return something like context values must be strings or something similar if we failed to unmarshal those values, however after thinking about it some more, it may not be worth the trouble at the moment to implement a custom decoder just to do that.

What do you think?

@aaronraff
Copy link
Contributor

I don't mind trying it out, but if there are other issues that you feel are more important, I can jump on one of those.

Sounds like a cool idea for a library though!

@markphelps
Copy link
Collaborator Author

I think im gonna close this since it's not really critical. There's another issue that I could use your help on re the automatic 'slugifying' of flag/segment keys from the name that was added in #155. I'll create an issue for that

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

2 participants