twirp.Erroer interface #320
Conversation
…t interface to return Twirp errors
|
Thanks for sending this PR! I will review this today. |
There was a problem hiding this comment.
Logic looks good to me. The main change I'd like to see is renaming the interface before we lock it in.
It would be good to update the error documentation here as well: https://twitchtv.github.io/twirp/docs/errors.html.
| // This may be useful if implementing all of the required methods for | ||
| // twirp.Error is too difficult or not possible. | ||
| // See https://github.com/twitchtv/twirp/issues/300 for reasoning. | ||
| type Erroer interface { |
There was a problem hiding this comment.
Shouldn't it be Errorer or TwirpErrorer?
| // as an alternative to implementing the Error interface. | ||
| // This may be useful if implementing all of the required methods for | ||
| // twirp.Error is too difficult or not possible. | ||
| // See https://github.com/twitchtv/twirp/issues/300 for reasoning. |
There was a problem hiding this comment.
To my mind, the key difference from the service author's perspective between this interface and Error is that the handler will attempt to unwrap an Erroer whereas the Error interface is only used if the returned handler error is an Error. I think that's the most important thing to explain.
For the reasons I described on the thread in #300, I don't think that callers should ever implement the Error interface themselves at all. So I don't think we should even mention that possibility here.
I also think that the documentation for something so fundamental should be self-contained rather than needing to refer to a whole other discussion.
Putting all that together, here's a rough suggestion of a doc comment:
// Erroer may be implemented by service errors in order to return specific
// Twirp errors from RPC handlers.
//
// If the error returned from an RPC handler is an Error, it is used as-is.
// If the error matches Erroer using errors.As, the result of calling TwirpError
// on the unwrapped Erroer is used.
// Otherwise, the error is wrapped using InternalErrorWith.
| t.P(`// newTwirpError casts an error into the twirp.Error interface,`) | ||
| t.P(`// or calls .TwirpError() if it implements twirp.Erroer.`) | ||
| t.P(`// Otherwise it wraps the error as a twirp.Internal error.`) | ||
| t.P(`func newTwirpError(err error) `, t.pkgs["twirp"], `.Error {`) |
There was a problem hiding this comment.
newTwirpError sounds like it's a normal constructor function. Perhaps asTwirpError?
| @@ -414,24 +416,37 @@ func (t *twirp) generateUtils() { | |||
| t.P(`}`) | |||
| t.P() | |||
|
|
|||
| t.P(`// newTwirpError casts an error into the twirp.Error interface,`) | |||
| t.P(`// or calls .TwirpError() if it implements twirp.Erroer.`) | |||
There was a problem hiding this comment.
Suggested rephrasing of this sentence:
newTwirpError converts err to a twirp.Error, if possible, or else calls TwirpError if err wraps Erroer.
| t.P(`// newTwirpError casts an error into the twirp.Error interface,`) | ||
| t.P(`// or calls .TwirpError() if it implements twirp.Erroer.`) | ||
| t.P(`// Otherwise it wraps the error as a twirp.Internal error.`) | ||
| t.P(`func newTwirpError(err error) `, t.pkgs["twirp"], `.Error {`) |
There was a problem hiding this comment.
As a side note, it's unfortunate that this helper function (along with many others) is duplicated in every generated twirp file. It'd be nice for generated code size (and binary size) to move those to a shared internal package (same as ctxsetters) at some point in the future.
There was a problem hiding this comment.
Twirp tries to keep as much functionality as possible on the generated file, by design. That allows to re-generate and upgrade to newer versions without having to worry about version incompatibilities, which may become complex when importing multiple clients that were generated at different times. The less dependencies between the generated code and any other package, the better. Code size is not too bad at this point.
| t.P(` twerr = `, t.pkgs["twirp"], `.InternalErrorWith(err)`) | ||
| t.P(` }`) | ||
| t.P(``) | ||
| t.P(` // Cast to twirp.Error. Non-twirp errors are wrapped as Internal errors.`) |
There was a problem hiding this comment.
This comment feels pretty unnecessary since it essentially repeats what the newTwirpError doc says and it bloats the generated code size.
|
It's not clear to me that this new interface is required, or how the patterns it enables are different or better than what errors.As provides. I gave a specific example in #300 (comment). |
|
closing in favor of #323 323 |
Issue #300
Allow errors returned by service handlers to implement a different interface
twirp.Erroerto be serialized as Twirp errors.The interface doesn't need to be exported, but having it exported documents the alternative better and makes the option more explicit.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.