From 4f2f4fc40d778b870bac901fe1b34e89121e3cf0 Mon Sep 17 00:00:00 2001 From: George Blue Date: Sun, 10 Sep 2023 15:48:13 +0100 Subject: [PATCH] fix: NewFailureResponse() should return type error (#257) 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 https://github.com/pivotal-cf/brokerapi/pull/252 Resolves https://github.com/pivotal-cf/brokerapi/issues/256 --- domain/apiresponses/failure_responses.go | 4 ++-- domain/apiresponses/failure_responses_test.go | 16 +++++++++++----- failure_response.go | 6 +++--- failure_response_test.go | 19 +++++++++++++------ 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/domain/apiresponses/failure_responses.go b/domain/apiresponses/failure_responses.go index 436d43b4..cce32839 100644 --- a/domain/apiresponses/failure_responses.go +++ b/domain/apiresponses/failure_responses.go @@ -18,11 +18,11 @@ type FailureResponse struct { errorKey string } -// NewFailureResponse returns a pointer to a new instance of FailureResponse. +// NewFailureResponse returns an error of type FailureResponse. // 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 { +func NewFailureResponse(err error, statusCode int, loggerAction string) error { return &FailureResponse{ error: err, statusCode: statusCode, diff --git a/domain/apiresponses/failure_responses_test.go b/domain/apiresponses/failure_responses_test.go index ef6d1af3..90b76b8c 100644 --- a/domain/apiresponses/failure_responses_test.go +++ b/domain/apiresponses/failure_responses_test.go @@ -15,7 +15,7 @@ import ( var _ = Describe("FailureResponse", func() { Describe("ErrorResponse", func() { It("returns a ErrorResponse containing the error message", func() { - failureResponse := apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key") + failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key")) Expect(failureResponse.ErrorResponse()).To(Equal(apiresponses.ErrorResponse{ Description: "my error message", })) @@ -71,7 +71,7 @@ var _ = Describe("FailureResponse", func() { Describe("ValidatedStatusCode", func() { It("returns the status code that was passed in", func() { - failureResponse := apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key") + failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key")) Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden)) }) @@ -82,7 +82,7 @@ var _ = Describe("FailureResponse", func() { Context("when the status code is invalid", func() { It("returns 500", func() { - failureResponse := apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key") + failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key")) Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusInternalServerError)) }) @@ -90,7 +90,7 @@ var _ = Describe("FailureResponse", func() { log := gbytes.NewBuffer() logger := lager.NewLogger("test") logger.RegisterSink(lager.NewWriterSink(log, lager.DEBUG)) - failureResponse := apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key") + failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key")) failureResponse.ValidatedStatusCode(logger) Expect(log).To(gbytes.Say("Invalid failure http response code: 600, expected 4xx or 5xx, returning internal server error: 500.")) }) @@ -104,8 +104,14 @@ var _ = Describe("FailureResponse", func() { }) It("when error key is provided it returns the logger action that was passed in", func() { - failureResponse := apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key") + failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key")) Expect(failureResponse.LoggerAction()).To(Equal("log-key")) }) }) }) + +func asFailureResponse(err error) *apiresponses.FailureResponse { + GinkgoHelper() + Expect(err).To(BeAssignableToTypeOf(&apiresponses.FailureResponse{})) + return err.(*apiresponses.FailureResponse) +} diff --git a/failure_response.go b/failure_response.go index b2dd463c..11bcb600 100644 --- a/failure_response.go +++ b/failure_response.go @@ -17,12 +17,12 @@ import ( type FailureResponse = apiresponses.FailureResponse // Deprecated: Use github.com/pivotal-cf/brokerapi/domain/apiresponses -// NewFailureResponse returns a pointer to a new instance of FailureResponse. +// NewFailureResponse returns an error of type FailureResponse. // 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)) +func NewFailureResponse(err error, statusCode int, loggerAction string) error { + return apiresponses.NewFailureResponse(err, statusCode, loggerAction) } // Deprecated: Use github.com/pivotal-cf/brokerapi/domain/apiresponses diff --git a/failure_response_test.go b/failure_response_test.go index 7e1661c9..65e3a0fa 100644 --- a/failure_response_test.go +++ b/failure_response_test.go @@ -17,6 +17,7 @@ package brokerapi_test import ( "github.com/pivotal-cf/brokerapi/v10" + "github.com/pivotal-cf/brokerapi/v10/domain/apiresponses" "errors" @@ -31,7 +32,7 @@ import ( var _ = Describe("FailureResponse", func() { Describe("ErrorResponse", func() { It("returns a ErrorResponse containing the error message", func() { - failureResponse := brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key") + failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key")) Expect(failureResponse.ErrorResponse()).To(Equal(brokerapi.ErrorResponse{ Description: "my error message", })) @@ -39,7 +40,7 @@ var _ = Describe("FailureResponse", func() { Context("when the error key is provided", func() { It("returns a ErrorResponse containing the error message and the error key", func() { - failureResponse := brokerapi.NewFailureResponseBuilder(errors.New("my error message"), http.StatusForbidden, "log-key").WithErrorKey("error key").Build() + failureResponse := asFailureResponse(brokerapi.NewFailureResponseBuilder(errors.New("my error message"), http.StatusForbidden, "log-key").WithErrorKey("error key").Build()) Expect(failureResponse.ErrorResponse()).To(Equal(brokerapi.ErrorResponse{ Description: "my error message", Error: "error key", @@ -87,7 +88,7 @@ var _ = Describe("FailureResponse", func() { Describe("ValidatedStatusCode", func() { It("returns the status code that was passed in", func() { - failureResponse := brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key") + failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key")) Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden)) }) @@ -98,7 +99,7 @@ var _ = Describe("FailureResponse", func() { Context("when the status code is invalid", func() { It("returns 500", func() { - failureResponse := brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key") + failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key")) Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusInternalServerError)) }) @@ -106,7 +107,7 @@ var _ = Describe("FailureResponse", func() { log := gbytes.NewBuffer() logger := lager.NewLogger("test") logger.RegisterSink(lager.NewWriterSink(log, lager.DEBUG)) - failureResponse := brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key") + failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key")) failureResponse.ValidatedStatusCode(logger) Expect(log).To(gbytes.Say("Invalid failure http response code: 600, expected 4xx or 5xx, returning internal server error: 500.")) }) @@ -120,8 +121,14 @@ var _ = Describe("FailureResponse", func() { }) It("when error key is provided it returns the logger action that was passed in", func() { - failureResponse := brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key") + failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key")) Expect(failureResponse.LoggerAction()).To(Equal("log-key")) }) }) }) + +func asFailureResponse(err error) *apiresponses.FailureResponse { + GinkgoHelper() + Expect(err).To(BeAssignableToTypeOf(&apiresponses.FailureResponse{})) + return err.(*apiresponses.FailureResponse) +}