-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix: NewFailureResponse() should return type error #257
Conversation
The function NewFailureResponse() creates an error. The [Go FAQ](https://go.dev/doc/faq#nil_error) says: > It's a good idea for functions that return errors always to use the error type in their signature (as we did above) rather than a concrete type such as *MyError, to help guarantee the error is created correctly. As an example, os.Open returns an error even though, if not nil, it's always of concrete type *os.PathError. Returning the concrete type can result in subtle failures. For example, consider the code: ```go func something() error { var err *apiresponses.FailureResponse if 1 != 1 { err = apiresponses.NewFailureResponse(errors.New("something bad has happened with the universe"), http.StatusInternalServerError, "log-key") } return err } func main() { if err := something(); err != nil { fmt.Printf("bad thing: %s\n", err.Error()) } } ``` You might expect this to print nothing since 1 does not equal 1. But actually it panics. This is because the nil *apiresponses.FailureResponse is not equal to `nil` because it has a type, but it does have a nil pointer so it panics. If we replace `var err *apiresponses.FailureResponse` with `var err error` then everything works as expected. By returning an `error` type from NewFailureResponse(), the failing scenario doesn't compile. As with many fixes, this could be considered to be a breaking change. But it didn't require code changes on the brokers that I tested with, and if it does break anyone, then it's likely because they have risky code that has the potential to panic in a counterintuitive way. So overall I think the benefit of making this change it worth the risk. Resolves #252 Resolves #256
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/185353508 The labels on this github issue will be updated when the story is started. |
// err will by default be used as both a logging message and HTTP response description. | ||
// statusCode is the HTTP status code to be returned, must be 4xx or 5xx | ||
// loggerAction is a short description which will be used as the action if the error is logged. | ||
func NewFailureResponse(err error, statusCode int, loggerAction string) *FailureResponse { | ||
return (*FailureResponse)(apiresponses.NewFailureResponse(err, statusCode, loggerAction)) |
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.
praise: thanks for simplifying the code
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.
Thank you very much @blgm .
Definitely how GoLang deals with interfaces making a clear distinction between the type and the content value, is a curious and beautiful subject. A talk at the Tech Forum would be interesting
The function NewFailureResponse() creates an error. The Go FAQ says:
Returning the concrete type can result in subtle failures. For example, consider the code:
You might expect this to print nothing since 1 does not equal 1. But actually it panics. This is because the nil
*apiresponses.FailureResponse is not equal to
nil
because it has a type, but it does have a nil pointer so it panics.If we replace
var err *apiresponses.FailureResponse
withvar err error
then everything works as expected. By returning anerror
type from NewFailureResponse(), the failing scenario doesn't compile.As with many fixes, this could be considered to be a breaking change. But it didn't require code changes on the brokers that I tested with, and if it does break anyone, then it's likely because they have risky code that has the potential to panic in a counterintuitive way. So overall I think the benefit of making this change it worth the risk.
Resolves #252 Resolves #256