-
Notifications
You must be signed in to change notification settings - Fork 393
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
oidc: use %w verb for wrapping errors #381
Conversation
I don't think we want to do this everywhere. Any wrapped errors should have an associated test, since that'd now be an API we have to ensure works for a while. Is the request here effectively, "methods should wrap context errors"? Does net/http do the same thing? |
In particular the reason that I made this change is to capture
The reason you might want to opaque an error is if the underlying code is signalling something to the caller that you don't want to signal to your caller. Sometimes error messages are fine to bubble up, but there's a specific reason why you wouldn't want the behavior of underlying errors to become the behavior of your errors. For example if there is an
Sometimes yes, sometimes no (not an exhaustive list, just grabbed a couple of each):
So why would the standard library choose one vs. the other? From that cherry-picked sample, the two examples that use |
Thanks for the reply! I'm familiar with error wrapping. The reason I was asking about net/http was to see if context.Canceled values get passed back up to go-oidc. For example, net/http docs don't claim to wrap errors: Error values we return become part of our API contract, and not wrapping errors by default is intentional. I don't want some underlying change to break clients because they were expecting ErrFoo, and we're suddenly returning ErrBar. Happy to wrap errors where its appropriate. But any wrapped errors need a test to verify that those values are actually surfaced. |
I don't think I've really ever seen functions advertise their internal errors as wrapped vs. opaque(ed?). But One thing to also point out is that not wrapping errors produces the same behavior as wrapping with the That said, I don't think an "opaque by default" strategy is unreasonable, so happy to swap most of this back to how it was. I went ahead and pared this change down to the errors that specifically bubble up from HTTP calls, since that should cover my use case, and added a test to demonstrate. A number of error code paths aren't covered yet and would be quite tricky to try to hit, but hopefully what's there now is a good coverage for what's changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! just a couple nits
Errors wrapped using %v cannot be unwrapped. This means if there is an underlying error such as `context.Canceled`, a caller cannot reliably discover that error using any method other than string comparison. By swapping to the %w verb, `errors.Is` and `errors.As` become valuable tools for error discovery and behavior differentiation. While by convention, some of the errors like `fetching keys %v` should probably be changed to `fetching keys: %w` (with the `:` character), this change opts not to do that to help preserve backward compatibility for external error handling that uses string comparison today.
@ericchiang Thanks for your time and help here! |
Errors wrapped using %v cannot be unwrapped. This means if there is an underlying error such as
context.Canceled
, a caller cannot reliably discover that error using any method other than string comparison. By swapping to the %w verb,errors.Is
anderrors.As
become valuable tools for error discovery and behavior differentiation.While by convention, some of the errors like
fetching keys %v
should probably be changed tofetching keys: %w
(with the:
character), this change opts not to do that to help preserve backward compatibility for external error handling that uses string comparison today.