refactor: standardize API error handling (#1635)

backend/internal/httpserver/binding.go

@@ -0,0 +1,69 @@
+package httpserver
+
+import (
+	"errors"
+	"io"
+	"mime/multipart"
+	"net/http"
+
+	"github.com/gin-gonic/gin"
+	"github.com/gin-gonic/gin/binding"
+	"github.com/go-playground/validator/v10"
+
+	"github.com/pocket-id/pocket-id/backend/internal/apperror"
+	"github.com/pocket-id/pocket-id/backend/internal/dto"
+)
+
+// BindJSON binds and normalizes a JSON request while distinguishing invalid input from internal failures
+func BindJSON(c *gin.Context, value any) error {
+	err := classifyBindingError(c.ShouldBindJSON(value))
+	if err != nil {
+		return err
+	}
+
+	dto.Normalize(value)
+	return nil
+}
+
+// BindOptionalJSON accepts an empty body while normalizing valid input and classifying malformed JSON as invalid input
+func BindOptionalJSON(c *gin.Context, value any) error {
+	err := c.ShouldBindJSON(value)
+	if errors.Is(err, io.EOF) {
+		return nil
+	}
+	if err = classifyBindingError(err); err != nil {
+		return err
+	}
+
+	dto.Normalize(value)
+	return nil
+}
+
+// FormFile returns an uploaded file while classifying a missing field as request validation
+func FormFile(c *gin.Context, field string) (*multipart.FileHeader, error) {
+	file, err := c.FormFile(field)
+	if errors.Is(err, http.ErrMissingFile) {
+		return nil, apperror.MissingField(field)
+	}
+	if err != nil {
+		return nil, apperror.InvalidRequestBody(err)
+	}
+
+	return file, nil
+}
+
+func classifyBindingError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	if _, ok := errors.AsType[validator.ValidationErrors](err); ok {
+		return err
+	}
+
+	if _, ok := errors.AsType[binding.SliceValidationError](err); ok {
+		return err
+	}
+
+	return apperror.InvalidRequestBody(err)
+}

backend/internal/httpserver/binding_test.go

@@ -0,0 +1,85 @@
+package httpserver
+
+import (
+	"bytes"
+	"mime/multipart"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+	"github.com/pocket-id/pocket-id/backend/internal/apperror"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/text/unicode/norm"
+)
+
+func TestBindJSONClassifiesMalformedBody(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+	c.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", strings.NewReader(`{"name":`))
+	c.Request.Header.Set("Content-Type", "application/json")
+
+	var input struct {
+		Name string `json:"name"`
+	}
+	err := BindJSON(c, &input)
+
+	require.True(t, apperror.IsCode(err, apperror.CodeInvalidRequestBody))
+	require.Contains(t, err.Error(), "unexpected EOF")
+	var appErr *apperror.Error
+	require.ErrorAs(t, err, &appErr)
+	require.NotContains(t, appErr.ClientMessage(), "unexpected end")
+}
+
+func TestBindJSONNormalizesTaggedFieldsRecursively(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+	c.Request = httptest.NewRequestWithContext(
+		t.Context(),
+		http.MethodPost,
+		"/",
+		strings.NewReader(`{"name":"Cafe\u0301","email":"user@cafe\u0301.example","items":[{"label":"Re\u0301sume\u0301"}]}`),
+	)
+	c.Request.Header.Set("Content-Type", "application/json")
+
+	type embeddedInput struct {
+		Name string `json:"name" unorm:"nfc"`
+	}
+	type itemInput struct {
+		Label string `json:"label" unorm:"nfc"`
+	}
+	var input struct {
+		embeddedInput
+		Email *string     `json:"email" unorm:"nfc"`
+		Items []itemInput `json:"items"`
+	}
+	err := BindJSON(c, &input)
+
+	require.NoError(t, err)
+	require.Equal(t, norm.NFC.String("Café"), input.Name)
+	require.NotNil(t, input.Email)
+	require.Equal(t, norm.NFC.String("user@café.example"), *input.Email)
+	require.Equal(t, norm.NFC.String("Résumé"), input.Items[0].Label)
+}
+
+func TestFormFileClassifiesMissingField(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+	var body bytes.Buffer
+	writer := multipart.NewWriter(&body)
+	require.NoError(t, writer.Close())
+	c.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", &body)
+	c.Request.Header.Set("Content-Type", writer.FormDataContentType())
+
+	_, err := FormFile(c, "file")
+
+	require.True(t, apperror.IsCode(err, apperror.CodeValidationFailed))
+	var appErr *apperror.Error
+	require.ErrorAs(t, err, &appErr)
+	require.Equal(t, []apperror.FieldError{{
+		Field:   "file",
+		Code:    "required",
+		Message: "is required",
+	}}, appErr.Fields())
+}

backend/internal/httpserver/handler.go

@@ -0,0 +1,46 @@
+package httpserver
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net/http"
+	"runtime/debug"
+
+	"github.com/gin-gonic/gin"
+)
+
+// HandlerFunc is an application HTTP handler that returns failures to the shared error middleware
+type HandlerFunc func(*gin.Context) error
+
+// Handle adapts an error-returning application handler to Gin
+func Handle(handler HandlerFunc) gin.HandlerFunc {
+	return func(c *gin.Context) {
+		defer func() {
+			recovered := recover()
+			if recovered == nil {
+				return
+			}
+
+			// Preserve net/http's intentional request-abort behavior without logging it as an application panic
+			if err, ok := recovered.(error); ok && errors.Is(err, http.ErrAbortHandler) {
+				panic(recovered)
+			}
+
+			// Keep recovered errors out of the unwrap chain so every panic is reported as an internal failure
+			err := fmt.Errorf("panic in HTTP handler (%T): %v\n%s", recovered, recovered, debug.Stack())
+			_ = c.Error(err)
+			c.Abort()
+		}()
+
+		if err := handler(c); err != nil {
+			if errors.Is(err, context.Canceled) {
+				c.Abort()
+				return
+			}
+
+			_ = c.Error(err)
+			c.Abort()
+		}
+	}
+}

backend/internal/httpserver/handler_test.go

@@ -0,0 +1,64 @@
+package httpserver
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+	"github.com/stretchr/testify/require"
+)
+
+func TestHandleAttachesAndAbortsOnError(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+	expected := errors.New("request failed")
+
+	Handle(func(*gin.Context) error {
+		return expected
+	})(c)
+
+	require.True(t, c.IsAborted())
+	require.Len(t, c.Errors, 1)
+	require.ErrorIs(t, c.Errors[0], expected)
+}
+
+func TestHandleIgnoresCanceledRequests(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+
+	Handle(func(*gin.Context) error {
+		return context.Canceled
+	})(c)
+
+	require.True(t, c.IsAborted())
+	require.Empty(t, c.Errors)
+}
+
+func TestHandleRecoversPanicsWithStack(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+
+	Handle(func(*gin.Context) error {
+		panic("private panic details")
+	})(c)
+
+	require.True(t, c.IsAborted())
+	require.Len(t, c.Errors, 1)
+	require.ErrorContains(t, c.Errors[0], "panic in HTTP handler (string): private panic details")
+	require.ErrorContains(t, c.Errors[0], "TestHandleRecoversPanicsWithStack")
+}
+
+func TestHandlePreservesAbortHandlerPanic(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+
+	require.PanicsWithValue(t, http.ErrAbortHandler, func() {
+		Handle(func(*gin.Context) error {
+			panic(http.ErrAbortHandler)
+		})(c)
+	})
+	require.Empty(t, c.Errors)
+}