From 963602aee829abfeb6d16d2538cf533e878e4dc5 Mon Sep 17 00:00:00 2001 From: Fotis Papadopoulos Date: Thu, 24 Jan 2019 16:35:20 +0200 Subject: [PATCH] Support for HTTP errors payload (fixes #264) (#265) * Move errors to http package & update tests * Introduce http Error with code & payload data * Return encoded error response payload Since the request has Accept/Content-Type headers we should use the encoder on the response payload, just like handleSuccess does. * Refactor http errors file This commit introduces more functions for creating custom errors, which use default payload content (provided by Golang's http package). All existing functions have been renamed to New**ErrorWithPayload indicating that the caller can provide a specific payload for the error. Also a general purpose NewError method has been added, allowing the caller to create Internal Server and other errors. Signed-off-by: Fotis Papadopoulos --- sync/errors.go | 71 --------------------------------- sync/errors_test.go | 31 --------------- sync/http/errors.go | 80 +++++++++++++++++++++++++++++++++++++ sync/http/errors_test.go | 84 +++++++++++++++++++++++++++++++++++++++ sync/http/handler.go | 29 +++++++------- sync/http/handler_test.go | 17 ++++---- 6 files changed, 188 insertions(+), 124 deletions(-) delete mode 100644 sync/errors.go delete mode 100644 sync/errors_test.go create mode 100644 sync/http/errors.go create mode 100644 sync/http/errors_test.go diff --git a/sync/errors.go b/sync/errors.go deleted file mode 100644 index 6ba20bc57..000000000 --- a/sync/errors.go +++ /dev/null @@ -1,71 +0,0 @@ -package sync - -// ValidationError defines a validation error. -type ValidationError struct { - err string -} - -func (e *ValidationError) Error() string { - return e.err -} - -// NewValidationError creates a new validation error. -func NewValidationError(msg string) *ValidationError { - return &ValidationError{err: msg} -} - -// UnauthorizedError defines a authorization error. -type UnauthorizedError struct { - err string -} - -func (e *UnauthorizedError) Error() string { - return e.err -} - -// NewUnauthorizedError creates a new unauthorized error. -func NewUnauthorizedError(msg string) *UnauthorizedError { - return &UnauthorizedError{err: msg} -} - -// ForbiddenError defines a access error. -type ForbiddenError struct { - err string -} - -func (e *ForbiddenError) Error() string { - return e.err -} - -// NewForbiddenError creates a new forbidden error. -func NewForbiddenError(msg string) *ForbiddenError { - return &ForbiddenError{err: msg} -} - -// NotFoundError defines a not found error. -type NotFoundError struct { - err string -} - -func (e *NotFoundError) Error() string { - return e.err -} - -// NewNotFoundError creates a new not found error. -func NewNotFoundError(msg string) *NotFoundError { - return &NotFoundError{err: msg} -} - -// ServiceUnavailableError defines a service unavailable error. -type ServiceUnavailableError struct { - err string -} - -func (e *ServiceUnavailableError) Error() string { - return e.err -} - -// NewServiceUnavailableError creates a new service unavailable error. -func NewServiceUnavailableError(msg string) *ServiceUnavailableError { - return &ServiceUnavailableError{err: msg} -} diff --git a/sync/errors_test.go b/sync/errors_test.go deleted file mode 100644 index 29959d543..000000000 --- a/sync/errors_test.go +++ /dev/null @@ -1,31 +0,0 @@ -package sync - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestValidationError_Error(t *testing.T) { - v := NewValidationError("TEST") - assert.Equal(t, "TEST", v.Error()) -} -func TestUnauthorizedError_Error(t *testing.T) { - v := NewUnauthorizedError("TEST") - assert.Equal(t, "TEST", v.Error()) -} - -func TestForbiddenError_Error(t *testing.T) { - v := NewForbiddenError("TEST") - assert.Equal(t, "TEST", v.Error()) -} - -func TestNotFoundError_Error(t *testing.T) { - v := NewNotFoundError("TEST") - assert.Equal(t, "TEST", v.Error()) -} - -func TestServiceUnavailableError_Error(t *testing.T) { - v := NewServiceUnavailableError("TEST") - assert.Equal(t, "TEST", v.Error()) -} diff --git a/sync/http/errors.go b/sync/http/errors.go new file mode 100644 index 000000000..43649d66e --- /dev/null +++ b/sync/http/errors.go @@ -0,0 +1,80 @@ +package http + +import ( + "fmt" + "net/http" +) + +// Error defines an abstract struct that can represent several types of HTTP errors. +type Error struct { + code int + payload interface{} +} + +// Error returns the actual message of the error. +func (e *Error) Error() string { + if e.payload == nil { + return fmt.Sprintf("HTTP error with code: %d", e.code) + } + return fmt.Sprintf("HTTP error with code: %d payload: %v", e.code, e.payload) +} + +// NewValidationError creates a new validation error with default payload. +func NewValidationError() *Error { + return &Error{http.StatusBadRequest, http.StatusText(http.StatusBadRequest)} +} + +// NewValidationErrorWithPayload creates a new validation error with the specified payload. +func NewValidationErrorWithPayload(payload interface{}) *Error { + return &Error{http.StatusBadRequest, payload} +} + +// NewUnauthorizedError creates a new validation error with default payload. +func NewUnauthorizedError() *Error { + return &Error{http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)} +} + +// NewUnauthorizedErrorWithPayload creates a new unauthorized error with the specified payload. +func NewUnauthorizedErrorWithPayload(payload interface{}) *Error { + return &Error{http.StatusUnauthorized, payload} +} + +// NewForbiddenError creates a new forbidden error with default payload. +func NewForbiddenError() *Error { + return &Error{http.StatusForbidden, http.StatusText(http.StatusForbidden)} +} + +// NewForbiddenErrorWithPayload creates a new forbidden error with the specified payload. +func NewForbiddenErrorWithPayload(payload interface{}) *Error { + return &Error{http.StatusForbidden, payload} +} + +// NewNotFoundError creates a new not found error with default payload. +func NewNotFoundError() *Error { + return &Error{http.StatusNotFound, http.StatusText(http.StatusNotFound)} +} + +// NewNotFoundErrorWithPayload creates a new not found error with the specified payload. +func NewNotFoundErrorWithPayload(payload interface{}) *Error { + return &Error{http.StatusNotFound, payload} +} + +// NewServiceUnavailableError creates a new service unavailable error with default payload. +func NewServiceUnavailableError() *Error { + return &Error{http.StatusServiceUnavailable, http.StatusText(http.StatusServiceUnavailable)} +} + +// NewServiceUnavailableErrorWithPayload creates a new service unavailable error with the specified payload. +func NewServiceUnavailableErrorWithPayload(payload interface{}) *Error { + return &Error{http.StatusServiceUnavailable, payload} +} + +// NewError creates a new error with default Internal Server Error payload. +func NewError() *Error { + return &Error{http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)} +} + +// NewErrorWithCodeAndPayload creates a fully customizable error with the specified status code and payload. +func NewErrorWithCodeAndPayload(code int, payload interface{}) *Error { + return &Error{code, payload} +} diff --git a/sync/http/errors_test.go b/sync/http/errors_test.go new file mode 100644 index 000000000..42048ee95 --- /dev/null +++ b/sync/http/errors_test.go @@ -0,0 +1,84 @@ +package http + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidationError(t *testing.T) { + err := NewValidationError() + assert.EqualError(t, err, "HTTP error with code: 400 payload: Bad Request") + assert.Equal(t, 400, err.code) +} + +func TestValidationErrorWithPayload(t *testing.T) { + err := NewValidationErrorWithPayload("test") + assert.EqualError(t, err, "HTTP error with code: 400 payload: test") + assert.Equal(t, 400, err.code) +} +func TestUnauthorizedError(t *testing.T) { + err := NewUnauthorizedError() + assert.EqualError(t, err, "HTTP error with code: 401 payload: Unauthorized") + assert.Equal(t, 401, err.code) +} + +func TestUnauthorizedErrorWithPayload(t *testing.T) { + err := NewUnauthorizedErrorWithPayload("test") + assert.EqualError(t, err, "HTTP error with code: 401 payload: test") + assert.Equal(t, 401, err.code) +} + +func TestForbiddenError(t *testing.T) { + err := NewForbiddenError() + assert.EqualError(t, err, "HTTP error with code: 403 payload: Forbidden") + assert.Equal(t, 403, err.code) +} + +func TestForbiddenErrorWithPayload(t *testing.T) { + err := NewForbiddenErrorWithPayload("test") + assert.EqualError(t, err, "HTTP error with code: 403 payload: test") + assert.Equal(t, 403, err.code) +} + +func TestNotFoundError(t *testing.T) { + err := NewNotFoundError() + assert.EqualError(t, err, "HTTP error with code: 404 payload: Not Found") + assert.Equal(t, 404, err.code) +} + +func TestNotFoundErrorWithPayload(t *testing.T) { + err := NewNotFoundErrorWithPayload("test") + assert.EqualError(t, err, "HTTP error with code: 404 payload: test") + assert.Equal(t, 404, err.code) +} + +func TestServiceUnavailableError(t *testing.T) { + err := NewServiceUnavailableError() + assert.EqualError(t, err, "HTTP error with code: 503 payload: Service Unavailable") + assert.Equal(t, 503, err.code) +} + +func TestServiceUnavailableErrorWithPayload(t *testing.T) { + err := NewServiceUnavailableErrorWithPayload("test") + assert.EqualError(t, err, "HTTP error with code: 503 payload: test") + assert.Equal(t, 503, err.code) +} + +func TestNewError(t *testing.T) { + err := NewError() + assert.EqualError(t, err, "HTTP error with code: 500 payload: Internal Server Error") + assert.Equal(t, 500, err.code) +} + +func TestNewErrorWithCodeAndPayload(t *testing.T) { + err := NewErrorWithCodeAndPayload(409, "Conflict") + assert.EqualError(t, err, "HTTP error with code: 409 payload: Conflict") + assert.Equal(t, 409, err.code) +} + +func TestNewErrorWithCodeAndNoPayload(t *testing.T) { + err := NewErrorWithCodeAndPayload(409, nil) + assert.EqualError(t, err, "HTTP error with code: 409") + assert.Equal(t, 409, err.code) +} diff --git a/sync/http/handler.go b/sync/http/handler.go index cb3f2aec0..44301b292 100644 --- a/sync/http/handler.go +++ b/sync/http/handler.go @@ -33,7 +33,7 @@ func handler(hnd sync.ProcessorFunc) http.HandlerFunc { req := sync.NewRequest(f, r.Body, dec) rsp, err := hnd(ctx, req) if err != nil { - handleError(w, err) + handleError(w, enc, err) return } @@ -122,21 +122,20 @@ func handleSuccess(w http.ResponseWriter, r *http.Request, rsp *sync.Response, e return err } -func handleError(w http.ResponseWriter, err error) { - switch err.(type) { - case *sync.ValidationError: - http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest) - case *sync.UnauthorizedError: - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) - case *sync.ForbiddenError: - http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) - case *sync.NotFoundError: - http.Error(w, "", http.StatusNotFound) - case *sync.ServiceUnavailableError: - http.Error(w, http.StatusText(http.StatusServiceUnavailable), http.StatusServiceUnavailable) - default: - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) +func handleError(w http.ResponseWriter, enc encoding.EncodeFunc, err error) { + // Assert error to type Error in order to leverage the code and payload values that such errors contain. + if err, ok := err.(*Error); ok { + p, encErr := enc(err.payload) + if encErr != nil { + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + return + } + w.WriteHeader(err.code) + w.Write(p) + return } + // Using http.Error helper hijacks the content type header of the response returning plain text payload. + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) } func prepareResponse(w http.ResponseWriter, ct string) { diff --git a/sync/http/handler_test.go b/sync/http/handler_test.go index 45a0c1eea..e80e2e947 100644 --- a/sync/http/handler_test.go +++ b/sync/http/handler_test.go @@ -129,23 +129,26 @@ func Test_handleSuccess(t *testing.T) { func Test_handleError(t *testing.T) { type args struct { err error + enc encoding.EncodeFunc } tests := []struct { name string args args expectedCode int }{ - {"bad request", args{err: &sync.ValidationError{}}, http.StatusBadRequest}, - {"unauthorized request", args{err: &sync.UnauthorizedError{}}, http.StatusUnauthorized}, - {"forbidden request", args{err: &sync.ForbiddenError{}}, http.StatusForbidden}, - {"not found error", args{err: &sync.NotFoundError{}}, http.StatusNotFound}, - {"service unavailable error", args{err: &sync.ServiceUnavailableError{}}, http.StatusServiceUnavailable}, - {"default error", args{err: errors.New("Test")}, http.StatusInternalServerError}, + {"bad request", args{err: NewValidationError(), enc: json.Encode}, http.StatusBadRequest}, + {"unauthorized request", args{err: NewUnauthorizedError(), enc: json.Encode}, http.StatusUnauthorized}, + {"forbidden request", args{err: NewForbiddenError(), enc: json.Encode}, http.StatusForbidden}, + {"not found error", args{err: NewNotFoundError(), enc: json.Encode}, http.StatusNotFound}, + {"service unavailable error", args{err: NewServiceUnavailableError(), enc: json.Encode}, http.StatusServiceUnavailable}, + {"internal server error", args{err: NewError(), enc: json.Encode}, http.StatusInternalServerError}, + {"default error", args{err: errors.New("Test"), enc: json.Encode}, http.StatusInternalServerError}, + {"payload encoding error", args{err: NewErrorWithCodeAndPayload(http.StatusBadRequest, make(chan int)), enc: json.Encode}, http.StatusInternalServerError}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { rsp := httptest.NewRecorder() - handleError(rsp, tt.args.err) + handleError(rsp, tt.args.enc, tt.args.err) assert.Equal(t, tt.expectedCode, rsp.Code) }) }