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 #252
Resolves #256

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,

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,15 +82,15 @@ 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))
  })
 
  It("logs that the status has been changed", 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)
+}

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

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,15 +32,15 @@ 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",
  }))
  })
 
  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,15 +99,15 @@ 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))
  })
 
  It("logs that the status has been changed", 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)
+}