for v11: NewFailureResponse() should return error not *apiresponses.FailureResponse #256
Labels
Comments
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/185344128 The labels on this github issue will be updated when the story is started. |
|
Delay until v11 |
blgm
added a commit
that referenced
this issue
Jun 7, 2023
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
|
Decided not to wait until v11. On reflection, this is unlikely to break many users, as anyone using an apiresponses.FailureResponse pointer is likely to be seeing failures. See PR #257 for resolution. |
zucchinidev
pushed a commit
that referenced
this issue
Sep 10, 2023
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
See: #252 (comment)
Reproduction steps
See: #252 (comment)
Expected behavior
A
New...function arguably should return the type that it mentions, but in this case returning anerrortype may make more sense as it would encourage the use ofvar errorrather thanvar *apierrors.FailureResponsewhich is not idiomatic Go, and can lead to strange situations.Additional context
Would be a breaking change, so wait until v11
The text was updated successfully, but these errors were encountered: