From 2b0cd348f527fe6e6b122db85f5778d449f60095 Mon Sep 17 00:00:00 2001 From: Nicholas Jackson Date: Thu, 21 Dec 2023 18:31:06 -0800 Subject: [PATCH] refactor: update based on code review feedback - Rename Fmt to ResFmt - Add comments in several places - Return errors instead of panicking in Format - Add 'Accept' to the Vary header in Format to match res.format --- ctx.go | 25 +++++++++++++++---------- ctx_interface.go | 2 +- ctx_test.go | 29 ++++++++++++++--------------- docs/api/ctx.md | 8 ++++---- error.go | 10 ++++++++++ 5 files changed, 44 insertions(+), 30 deletions(-) diff --git a/ctx.go b/ctx.go index f2fec623bb..56dfa6510e 100644 --- a/ctx.go +++ b/ctx.go @@ -8,7 +8,6 @@ import ( "bytes" "context" "crypto/tls" - "errors" "fmt" "io" "mime/multipart" @@ -102,6 +101,12 @@ type Views interface { Render(io.Writer, string, any, ...string) error } +// ResFmt associates a Content Type to a fiber.Handler for c.Format +type ResFmt struct { + MediaType string + Handler func(Ctx) error +} + // Accepts checks if the specified extensions or content types are acceptable. func (c *DefaultCtx) Accepts(offers ...string) string { return getOffer(c.Get(HeaderAccept), acceptsOfferType, offers...) @@ -371,27 +376,28 @@ func (c *DefaultCtx) Response() *fasthttp.Response { return &c.fasthttp.Response } -type Fmt struct { - MediaType string - Handler func(Ctx) error -} - // Format performs content-negotiation on the Accept HTTP header. // It uses Accepts to select a proper format and calls the matching // user-provided handler function. // If no accepted format is found, and a format with MediaType "default" is given, // that default handler is called. If no format is found and no default is given, // StatusNotAcceptable is sent. -func (c *DefaultCtx) Format(handlers ...Fmt) error { +func (c *DefaultCtx) Format(handlers ...ResFmt) error { if len(handlers) == 0 { - panic("Format requires at least one handler") + return ErrNoHandlers } + c.Vary(HeaderAccept) + if c.Get(HeaderAccept) == "" { c.Response().Header.SetContentType(handlers[0].MediaType) return handlers[0].Handler(c) } + // Using an int literal as the slice capacity allows for the slice to be + // allocated on the stack. The number was chosen arbitrarily as an + // approximation of the maximum number of content types a user might handle. + // If the user goes over, it just causes allocations, so it's not a problem. types := make([]string, 0, 8) var defaultHandler Handler for _, h := range handlers { @@ -417,8 +423,7 @@ func (c *DefaultCtx) Format(handlers ...Fmt) error { } } - // unreachable code - panic(errors.New("fiber: an Accept was found but no handler was called - please file a bug report")) + return fmt.Errorf("%w: format: an Accept was found but no handler was called", errUnreachable) } // AutoFormat performs content-negotiation on the Accept HTTP header. diff --git a/ctx_interface.go b/ctx_interface.go index 9ff6b72339..8619f449e4 100644 --- a/ctx_interface.go +++ b/ctx_interface.go @@ -95,7 +95,7 @@ type Ctx interface { // If no accepted format is found, and a format with MediaType "default" is given, // that default handler is called. If no format is found and no default is given, // StatusNotAcceptable is sent. - Format(handlers ...Fmt) error + Format(handlers ...ResFmt) error // AutoFormat performs content-negotiation on the Accept HTTP header. // It uses Accepts to select a proper format. diff --git a/ctx_test.go b/ctx_test.go index 3cf543e617..02ecf1f5fb 100644 --- a/ctx_test.go +++ b/ctx_test.go @@ -719,11 +719,11 @@ func Test_Ctx_Format(t *testing.T) { // set `accepted` to whatever media type was chosen by Format var accepted string - formatHandlers := func(types ...string) []Fmt { - fmts := []Fmt{} + formatHandlers := func(types ...string) []ResFmt { + fmts := []ResFmt{} for _, t := range types { t := utils.CopyString(t) - fmts = append(fmts, Fmt{t, func(c Ctx) error { + fmts = append(fmts, ResFmt{t, func(c Ctx) error { accepted = t return nil }}) @@ -745,11 +745,11 @@ func Test_Ctx_Format(t *testing.T) { require.NotEqual(t, StatusNotAcceptable, c.Response().StatusCode()) myError := errors.New("this is an error") - err = c.Format(Fmt{"text/html", func(c Ctx) error { return myError }}) + err = c.Format(ResFmt{"text/html", func(c Ctx) error { return myError }}) require.ErrorIs(t, err, myError) c.Request().Header.Set(HeaderAccept, "application/json") - err = c.Format(Fmt{"text/html", func(c Ctx) error { return c.SendStatus(StatusOK) }}) + err = c.Format(ResFmt{"text/html", func(c Ctx) error { return c.SendStatus(StatusOK) }}) require.Equal(t, StatusNotAcceptable, c.Response().StatusCode()) require.NoError(t, err) @@ -758,9 +758,8 @@ func Test_Ctx_Format(t *testing.T) { require.Equal(t, "text/html", c.GetRespHeader(HeaderContentType)) require.NoError(t, err) - require.Panics(t, func() { - err = c.Format() - }) + err = c.Format() + require.ErrorIs(t, err, ErrNoHandlers) } func Benchmark_Ctx_Format(b *testing.B) { @@ -780,17 +779,17 @@ func Benchmark_Ctx_Format(b *testing.B) { b.Run("with arg allocation", func(b *testing.B) { for n := 0; n < b.N; n++ { err = c.Format( - Fmt{"application/xml", fail}, - Fmt{"text/html", fail}, - Fmt{"text/plain;format=fixed", fail}, - Fmt{"text/plain;format=flowed", ok}, + ResFmt{"application/xml", fail}, + ResFmt{"text/html", fail}, + ResFmt{"text/plain;format=fixed", fail}, + ResFmt{"text/plain;format=flowed", ok}, ) } require.NoError(b, err) }) b.Run("pre-allocated args", func(b *testing.B) { - offers := []Fmt{ + offers := []ResFmt{ {"application/xml", fail}, {"text/html", fail}, {"text/plain;format=fixed", fail}, @@ -804,7 +803,7 @@ func Benchmark_Ctx_Format(b *testing.B) { c.Request().Header.Set("Accept", "text/plain") b.Run("text/plain", func(b *testing.B) { - offers := []Fmt{ + offers := []ResFmt{ {"application/xml", fail}, {"text/plain", ok}, } @@ -816,7 +815,7 @@ func Benchmark_Ctx_Format(b *testing.B) { c.Request().Header.Set("Accept", "json") b.Run("json", func(b *testing.B) { - offers := []Fmt{ + offers := []ResFmt{ {"xml", fail}, {"html", fail}, {"json", ok}, diff --git a/docs/api/ctx.md b/docs/api/ctx.md index 3cef3429a3..51278698e1 100644 --- a/docs/api/ctx.md +++ b/docs/api/ctx.md @@ -546,7 +546,7 @@ If the Accept header is **not** specified, the first handler will be used. ::: ```go title="Signature" -func (c *Ctx) Format(handlers ...Fmt) error +func (c *Ctx) Format(handlers ...ResFmt) error ``` ```go title="Example" @@ -555,13 +555,13 @@ func (c *Ctx) Format(handlers ...Fmt) error // Accept: application/xml => Not Acceptable app.Get("/no-default", func(c fiber.Ctx) error { return c.Format( - fiber.Fmt{"application/json", func(c fiber.Ctx) error { + fiber.ResFmt{"application/json", func(c fiber.Ctx) error { return c.JSON(fiber.Map{ "command": "eat", "subject": "fruit", }) }}, - fiber.Fmt{"text/plain", func(c fiber.Ctx) error { + fiber.ResFmt{"text/plain", func(c fiber.Ctx) error { return c.SendString("Eat Fruit!") }}, ) @@ -575,7 +575,7 @@ app.Get("/default", func(c fiber.Ctx) error { return c.SendString("Eat Fruit!") } - handlers := []fiber.Fmt{ + handlers := []fiber.ResFmt{ {"application/json", func(c fiber.Ctx) error { return c.JSON(fiber.Map{ "command": "eat", diff --git a/error.go b/error.go index 66b431cb0f..ea7a96d389 100644 --- a/error.go +++ b/error.go @@ -7,6 +7,10 @@ import ( "github.com/gofiber/fiber/v3/internal/schema" ) +// Wrap and return this for unreachable code if panicking is undesirable (i.e., in a handler). +// Unexported because users will hopefully never need to see it. +var errUnreachable = stdErrors.New("fiber: unreachable code, please create an issue at github.com/gofiber/fiber") + // Graceful shutdown errors var ( ErrGracefulTimeout = stdErrors.New("shutdown: graceful timeout has been reached, exiting") @@ -26,6 +30,12 @@ var ( // Binder errors var ErrCustomBinderNotFound = stdErrors.New("binder: custom binder not found, please be sure to enter the right name") +// Format errors +var ( + // ErrNoHandlers is returned when c.Format is called with no arguments. + ErrNoHandlers = stdErrors.New("format: at least one handler is required, but none were set") +) + // gorilla/schema errors type ( // ConversionError Conversion error exposes the internal schema.ConversionError for public use.