Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor path/query Param errors to use fuego http errors #419

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+245 −178
Draft

Refactor path/query Param errors to use fuego http errors #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e27b3f9
refactor: use BadRequest err for internal query param errs
jwtly10 Feb 24, 2025
8418ceb
refactor: use BadRequest err for internal path param errs
jwtly10 Feb 24, 2025
1c4794d
improve testing around BadRequest query param validation
jwtly10 Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 4 additions & 18 deletions ctx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,19 +234,6 @@ func (e PathParamNotFoundError) Error() string {

func (e PathParamNotFoundError) StatusCode() int { return 404 }

type PathParamInvalidTypeError struct {
Err error
ParamName string
ParamValue string
ExpectedType string
}

func (e PathParamInvalidTypeError) Error() string {
return fmt.Errorf("param %s=%s is not of type %s: %w", e.ParamName, e.ParamValue, e.ExpectedType, e.Err).Error()
}

func (e PathParamInvalidTypeError) StatusCode() int { return 422 }

type ContextWithPathParam interface {
PathParam(name string) string
}
Expand All @@ -259,11 +246,10 @@ func PathParamIntErr(c ContextWithPathParam, name string) (int, error) {

i, err := strconv.Atoi(param)
if err != nil {
return 0, PathParamInvalidTypeError{
ParamName: name,
ParamValue: param,
ExpectedType: "int",
Err: err,
return 0, BadRequestError{
Title: "Invalid path parameter",
Detail: fmt.Sprintf("path parameter %s=%s is not of type int", name, param),
Err: err,
}
}

Expand Down
77 changes: 75 additions & 2 deletions ctx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package fuego
import (
"bytes"
"context"
"encoding/json"
"encoding/xml"
"errors"
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
Expand Down Expand Up @@ -85,7 +87,14 @@ func TestContext_PathParam(t *testing.T) {

s.Mux.ServeHTTP(w, r)

require.Equal(t, 422, w.Code)
require.Equal(t, http.StatusBadRequest, w.Code)

var errorResponse HTTPError
err := json.NewDecoder(w.Body).Decode(&errorResponse)
require.NoError(t, err)

require.Equal(t, "Invalid path parameter", errorResponse.Title)
require.Equal(t, "path parameter id=abc is not of type int", errorResponse.Detail)
})

t.Run("path param invalid", func(t *testing.T) {
Expand Down Expand Up @@ -143,7 +152,10 @@ func TestContext_QueryParam(t *testing.T) {

paramInt, err = c.QueryParamIntErr("other")
require.Error(t, err)
require.Contains(t, err.Error(), "param other=hello is not of type int")
var badReqErr BadRequestError
require.ErrorAs(t, err, &badReqErr)
require.Equal(t, "Invalid query parameter", badReqErr.Title)
require.Equal(t, "query parameter other=hello is not of type int", badReqErr.Detail)
require.Equal(t, 0, paramInt)
})

Expand All @@ -168,7 +180,12 @@ func TestContext_QueryParam(t *testing.T) {

paramBool, err = c.QueryParamBoolErr("other")
require.Error(t, err)
var badReqErr BadRequestError
require.ErrorAs(t, err, &badReqErr)
require.Equal(t, "Invalid query parameter", badReqErr.Title)
require.Equal(t, "query parameter other=hello is not of type bool", badReqErr.Detail)
require.Equal(t, false, paramBool)
require.Error(t, err)
})

t.Run("slice", func(t *testing.T) {
Expand All @@ -181,6 +198,62 @@ func TestContext_QueryParam(t *testing.T) {
})
}
Copy link
Collaborator

@ccoVeille ccoVeille Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting a random line that GitHub allows me to. Why are the test about jhon doe and not john doe.

Note: I know it's not related to the current PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #419 (comment)


func TestContext_QueryParamHttp(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func TestContext_QueryParamHttp(t *testing.T) {
func TestContext_QueryParamHTTP(t *testing.T) {

t.Run("reading non-int query param to int sends an error", func(t *testing.T) {
s := NewServer()
Get(s, "/foo", func(c ContextNoBody) (ans, error) {
id, err := c.QueryParamIntErr("age")
if err != nil {
return ans{}, err
}
return ans{Ans: fmt.Sprintf("%d", id)}, nil
},
OptionQueryInt("age", "Filter by age (in years)", ParamExample("3 years old", 3), ParamInteger()),
)

r := httptest.NewRequest("GET", "/foo?age=invalid", nil)
w := httptest.NewRecorder()

s.Mux.ServeHTTP(w, r)

require.Equal(t, http.StatusBadRequest, w.Code)

var errorResponse HTTPError
err := json.NewDecoder(w.Body).Decode(&errorResponse)
require.NoError(t, err)

require.Equal(t, "Invalid query parameter", errorResponse.Title)
require.Equal(t, "query parameter age=invalid is not of type int", errorResponse.Detail)
})

t.Run("reading non-bool query param to bool sends an error", func(t *testing.T) {
s := NewServer()
Get(s, "/foo", func(c ContextNoBody) (ans, error) {
boo, err := c.QueryParamBoolErr("boo")
if err != nil {
return ans{}, err
}
return ans{Ans: fmt.Sprintf("%v", boo)}, nil
},
OptionQueryBool("boo", "Set boo boolean", ParamExample("true", true), ParamBool()),
)

r := httptest.NewRequest("GET", "/foo?boo=invalid", nil)
w := httptest.NewRecorder()

s.Mux.ServeHTTP(w, r)

require.Equal(t, http.StatusBadRequest, w.Code)

var errorResponse HTTPError
err := json.NewDecoder(w.Body).Decode(&errorResponse)
require.NoError(t, err)

require.Equal(t, "Invalid query parameter", errorResponse.Title)
require.Equal(t, "query parameter boo=invalid is not of type bool", errorResponse.Detail)
})
}

func TestContext_QueryParams(t *testing.T) {
r := httptest.NewRequest("GET", "http://example.com/foo/123?id=456&other=hello", nil)
w := httptest.NewRecorder()
Expand Down
152 changes: 15 additions & 137 deletions errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,147 +2,25 @@ package fuego

import (
"errors"
"fmt"
"log/slog"
"net/http"
)

// ErrorWithStatus is an interface that can be implemented by an error to provide
// a status code
type ErrorWithStatus interface {
error
StatusCode() int
}

// ErrorWithDetail is an interface that can be implemented by an error to provide
// an additional detail message about the error
type ErrorWithDetail interface {
error
DetailMsg() string
}

// HTTPError is the error response used by the serialization part of the framework.
type HTTPError struct {
// Developer readable error message. Not shown to the user to avoid security leaks.
Err error `json:"-" xml:"-" yaml:"-"`
// URL of the error type. Can be used to lookup the error in a documentation
Type string `json:"type,omitempty" xml:"type,omitempty" yaml:"type,omitempty" description:"URL of the error type. Can be used to lookup the error in a documentation"`
// Short title of the error
Title string `json:"title,omitempty" xml:"title,omitempty" yaml:"title,omitempty" description:"Short title of the error"`
// HTTP status code. If using a different type than [HTTPError], for example [BadRequestError], this will be automatically overridden after Fuego error handling.
Status int `json:"status,omitempty" xml:"status,omitempty" yaml:"status,omitempty" description:"HTTP status code" example:"403"`
// Human readable error message
Detail string `json:"detail,omitempty" xml:"detail,omitempty" yaml:"detail,omitempty" description:"Human readable error message"`
Instance string `json:"instance,omitempty" xml:"instance,omitempty" yaml:"instance,omitempty"`
Errors []ErrorItem `json:"errors,omitempty" xml:"errors,omitempty" yaml:"errors,omitempty"`
}

type ErrorItem struct {
More map[string]any `json:"more,omitempty" xml:"more,omitempty" description:"Additional information about the error"`
Name string `json:"name" xml:"name" description:"For example, name of the parameter that caused the error"`
Reason string `json:"reason" xml:"reason" description:"Human readable error message"`
}

func (e HTTPError) Error() string {
code := e.StatusCode()
title := e.Title
if title == "" {
title = http.StatusText(code)
if title == "" {
title = "HTTP Error"
}
}
msg := fmt.Sprintf("%d %s", code, title)

detail := e.DetailMsg()
if detail == "" {
return msg
}

return fmt.Sprintf("%s: %s", msg, e.Detail)
}

func (e HTTPError) StatusCode() int {
if e.Status == 0 {
return http.StatusInternalServerError
}
return e.Status
}

func (e HTTPError) DetailMsg() string {
return e.Detail
}

func (e HTTPError) Unwrap() error { return e.Err }

// BadRequestError is an error used to return a 400 status code.
type BadRequestError HTTPError

var _ ErrorWithStatus = BadRequestError{}

func (e BadRequestError) Error() string { return e.Err.Error() }

func (e BadRequestError) StatusCode() int { return http.StatusBadRequest }

func (e BadRequestError) Unwrap() error { return HTTPError(e) }

// NotFoundError is an error used to return a 404 status code.
type NotFoundError HTTPError

var _ ErrorWithStatus = NotFoundError{}

func (e NotFoundError) Error() string { return e.Err.Error() }

func (e NotFoundError) StatusCode() int { return http.StatusNotFound }

func (e NotFoundError) Unwrap() error { return HTTPError(e) }

// UnauthorizedError is an error used to return a 401 status code.
type UnauthorizedError HTTPError

var _ ErrorWithStatus = UnauthorizedError{}

func (e UnauthorizedError) Error() string { return e.Err.Error() }

func (e UnauthorizedError) StatusCode() int { return http.StatusUnauthorized }

func (e UnauthorizedError) Unwrap() error { return HTTPError(e) }

// ForbiddenError is an error used to return a 403 status code.
type ForbiddenError HTTPError

var _ ErrorWithStatus = ForbiddenError{}

func (e ForbiddenError) Error() string { return e.Err.Error() }

func (e ForbiddenError) StatusCode() int { return http.StatusForbidden }

func (e ForbiddenError) Unwrap() error { return HTTPError(e) }

// ConflictError is an error used to return a 409 status code.
type ConflictError HTTPError

var _ ErrorWithStatus = ConflictError{}

func (e ConflictError) Error() string { return e.Err.Error() }

func (e ConflictError) StatusCode() int { return http.StatusConflict }

func (e ConflictError) Unwrap() error { return HTTPError(e) }

// InternalServerError is an error used to return a 500 status code.
type InternalServerError = HTTPError

// NotAcceptableError is an error used to return a 406 status code.
type NotAcceptableError HTTPError

var _ ErrorWithStatus = NotAcceptableError{}

func (e NotAcceptableError) Error() string { return e.Err.Error() }

func (e NotAcceptableError) StatusCode() int { return http.StatusNotAcceptable }
"github.com/go-fuego/fuego/internal"
)

func (e NotAcceptableError) Unwrap() error { return HTTPError(e) }
type (
ErrorWithStatus = internal.ErrorWithStatus
ErrorWithDetail = internal.ErrorWithDetail
HTTPError = internal.HTTPError
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure what will be provided in the public documentation.

It needs to be double checked.

Unless you want to deprecate them all, but then all of them need to be clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should provide (by copying) a documentation at this level too

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too

ErrorItem = internal.ErrorItem
BadRequestError = internal.BadRequestError
NotFoundError = internal.NotFoundError
UnauthorizedError = internal.UnauthorizedError
ForbiddenError = internal.ForbiddenError
ConflictError = internal.ConflictError
InternalServerError = internal.HTTPError
NotAcceptableError = internal.NotAcceptableError
)

// ErrorHandler is the default error handler used by the framework.
// If the error is an [HTTPError] that error is returned.
Expand Down
29 changes: 8 additions & 21 deletions internal/common_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,10 @@ func (c CommonContext[B]) QueryParamIntErr(name string) (int, error) {

i, err := strconv.Atoi(param)
if err != nil {
return 0, QueryParamInvalidTypeError{
ParamName: name,
ParamValue: param,
ExpectedType: "int",
Err: err,
return 0, BadRequestError{
Title: "Invalid query parameter",
Detail: fmt.Sprintf("query parameter %s=%s is not of type int", name, param),
Err: err,
}
}

Expand All @@ -138,17 +137,6 @@ func (e QueryParamNotFoundError) Error() string {
return fmt.Errorf("param %s not found", e.ParamName).Error()
}

type QueryParamInvalidTypeError struct {
Err error
ParamName string
ParamValue string
ExpectedType string
}

func (e QueryParamInvalidTypeError) Error() string {
return fmt.Errorf("param %s=%s is not of type %s: %w", e.ParamName, e.ParamValue, e.ExpectedType, e.Err).Error()
}

// QueryParamArr returns an slice of string from the given query parameter.
func (c CommonContext[B]) QueryParamArr(name string) []string {
_, ok := c.OpenAPIParams[name]
Expand Down Expand Up @@ -200,11 +188,10 @@ func (c CommonContext[B]) QueryParamBoolErr(name string) (bool, error) {

b, err := strconv.ParseBool(param)
if err != nil {
return false, QueryParamInvalidTypeError{
ParamName: name,
ParamValue: param,
ExpectedType: "bool",
Err: err,
return false, BadRequestError{
Title: "Invalid query parameter",
Detail: fmt.Sprintf("query parameter %s=%s is not of type bool", name, param),
Err: err,
}
}
return b, nil
Expand Down
Loading
Loading