Skip to content

Commit

Permalink
refactor: update based on code review feedback
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
nickajacks1 committed Dec 22, 2023
1 parent aa86933 commit 2b0cd34
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 30 deletions.
25 changes: 15 additions & 10 deletions ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"bytes"
"context"
"crypto/tls"
"errors"
"fmt"
"io"
"mime/multipart"
Expand Down Expand Up @@ -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...)
Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion ctx_interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
29 changes: 14 additions & 15 deletions ctx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}})
Expand All @@ -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)

Expand All @@ -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) {
Expand All @@ -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},
Expand All @@ -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},
}
Expand All @@ -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},
Expand Down
8 changes: 4 additions & 4 deletions docs/api/ctx.md
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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!")
}},
)
Expand All @@ -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",
Expand Down
10 changes: 10 additions & 0 deletions error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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.
Expand Down

0 comments on commit 2b0cd34

Please sign in to comment.