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

🐛 bug: fix EnableSplittingOnParsers is not functional #3231

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

🐛 bug: fix EnableSplittingOnParsers is not functional #3231

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
725855d
:bug: bug: fix EnableSplittingOnParsers is not functional
efectn Dec 5, 2024
2260663
remove wrong testcase
efectn Dec 5, 2024
a56943d
add support for external xml decoders
efectn Dec 5, 2024
8ce1fb4
improve test coverage
efectn Dec 20, 2024
043dd20
fix linter
efectn Dec 20, 2024
d2d2ab7
update
efectn Dec 20, 2024
6dea77f
Merge branch 'main' into enablesplitting-fix
efectn Dec 20, 2024
e2e93a4
add reset methods
efectn Dec 23, 2024
47a8f32
improve test coverage
efectn Dec 23, 2024
e89943b
merge Form and MultipartForm methods
efectn Dec 23, 2024
182d53a
fix linter
efectn Dec 23, 2024
7d50265
split reset and putting steps
efectn Dec 24, 2024
2ac8bbf
fix linter
efectn Dec 24, 2024
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
110 changes: 100 additions & 10 deletions bind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package fiber

import (
"encoding/xml"

"github.com/gofiber/fiber/v3/binder"
"github.com/gofiber/utils/v2"
)
Expand Down Expand Up @@ -77,7 +79,16 @@ func (b *Bind) Custom(name string, dest any) error {

// Header binds the request header strings into the struct, map[string]string and map[string][]string.
func (b *Bind) Header(out any) error {
if err := b.returnErr(binder.HeaderBinder.Bind(b.ctx.Request(), out)); err != nil {
bind := binder.GetFromThePool[*binder.HeaderBinding](&binder.HeaderBinderPool)
bind.EnableSplitting = b.ctx.App().config.EnableSplittingOnParsers

// Reset & put binder
defer func() {
bind.EnableSplitting = false
binder.PutToThePool(&binder.HeaderBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil {
return err
}

Expand All @@ -86,7 +97,16 @@ func (b *Bind) Header(out any) error {

// RespHeader binds the response header strings into the struct, map[string]string and map[string][]string.
func (b *Bind) RespHeader(out any) error {
if err := b.returnErr(binder.RespHeaderBinder.Bind(b.ctx.Response(), out)); err != nil {
bind := binder.GetFromThePool[*binder.RespHeaderBinding](&binder.RespHeaderBinderPool)
bind.EnableSplitting = b.ctx.App().config.EnableSplittingOnParsers

// Reset & put binder
defer func() {
bind.EnableSplitting = false
efectn marked this conversation as resolved.
Show resolved Hide resolved
binder.PutToThePool(&binder.RespHeaderBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.Response(), out)); err != nil {
return err
}

Expand All @@ -96,7 +116,16 @@ func (b *Bind) RespHeader(out any) error {
// Cookie binds the request cookie strings into the struct, map[string]string and map[string][]string.
// NOTE: If your cookie is like key=val1,val2; they'll be binded as an slice if your map is map[string][]string. Else, it'll use last element of cookie.
func (b *Bind) Cookie(out any) error {
if err := b.returnErr(binder.CookieBinder.Bind(b.ctx.RequestCtx(), out)); err != nil {
bind := binder.GetFromThePool[*binder.CookieBinding](&binder.CookieBinderPool)
bind.EnableSplitting = b.ctx.App().config.EnableSplittingOnParsers

// Reset & put binder
defer func() {
bind.EnableSplitting = false
binder.PutToThePool(&binder.CookieBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.RequestCtx(), out)); err != nil {
efectn marked this conversation as resolved.
Show resolved Hide resolved
return err
}

Expand All @@ -105,7 +134,16 @@ func (b *Bind) Cookie(out any) error {

// Query binds the query string into the struct, map[string]string and map[string][]string.
func (b *Bind) Query(out any) error {
if err := b.returnErr(binder.QueryBinder.Bind(b.ctx.RequestCtx(), out)); err != nil {
bind := binder.GetFromThePool[*binder.QueryBinding](&binder.QueryBinderPool)
bind.EnableSplitting = b.ctx.App().config.EnableSplittingOnParsers

// Reset & put binder
defer func() {
bind.EnableSplitting = false
binder.PutToThePool(&binder.QueryBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.RequestCtx(), out)); err != nil {
return err
}

Expand All @@ -114,7 +152,16 @@ func (b *Bind) Query(out any) error {

// JSON binds the body string into the struct.
func (b *Bind) JSON(out any) error {
if err := b.returnErr(binder.JSONBinder.Bind(b.ctx.Body(), b.ctx.App().Config().JSONDecoder, out)); err != nil {
bind := binder.GetFromThePool[*binder.JSONBinding](&binder.JSONBinderPool)
bind.JSONDecoder = b.ctx.App().Config().JSONDecoder

// Reset & put binder
defer func() {
bind.JSONDecoder = nil
binder.PutToThePool(&binder.JSONBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.Body(), out)); err != nil {
return err
}

Expand All @@ -123,15 +170,33 @@ func (b *Bind) JSON(out any) error {

// CBOR binds the body string into the struct.
func (b *Bind) CBOR(out any) error {
if err := b.returnErr(binder.CBORBinder.Bind(b.ctx.Body(), b.ctx.App().Config().CBORDecoder, out)); err != nil {
bind := binder.GetFromThePool[*binder.CBORBinding](&binder.CBORBinderPool)
bind.CBORDecoder = b.ctx.App().Config().CBORDecoder

// Reset & put binder
defer func() {
bind.CBORDecoder = nil
binder.PutToThePool(&binder.CBORBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.Body(), out)); err != nil {
return err
}
return b.validateStruct(out)
}

// XML binds the body string into the struct.
func (b *Bind) XML(out any) error {
if err := b.returnErr(binder.XMLBinder.Bind(b.ctx.Body(), out)); err != nil {
bind := binder.GetFromThePool[*binder.XMLBinding](&binder.XMLBinderPool)
bind.XMLDecoder = xml.Unmarshal

// Reset & put binder
defer func() {
bind.XMLDecoder = nil
binder.PutToThePool(&binder.XMLBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.Body(), out)); err != nil {
return err
}

Expand All @@ -140,7 +205,16 @@ func (b *Bind) XML(out any) error {

// Form binds the form into the struct, map[string]string and map[string][]string.
func (b *Bind) Form(out any) error {
if err := b.returnErr(binder.FormBinder.Bind(b.ctx.RequestCtx(), out)); err != nil {
bind := binder.GetFromThePool[*binder.FormBinding](&binder.FormBinderPool)
bind.EnableSplitting = b.ctx.App().config.EnableSplittingOnParsers

// Reset & put binder
defer func() {
bind.EnableSplitting = false
binder.PutToThePool(&binder.FormBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.RequestCtx(), out)); err != nil {
return err
}

Expand All @@ -149,7 +223,14 @@ func (b *Bind) Form(out any) error {

// URI binds the route parameters into the struct, map[string]string and map[string][]string.
func (b *Bind) URI(out any) error {
if err := b.returnErr(binder.URIBinder.Bind(b.ctx.Route().Params, b.ctx.Params, out)); err != nil {
bind := binder.GetFromThePool[*binder.URIBinding](&binder.URIBinderPool)

// Reset & put binder
defer func() {
binder.PutToThePool(&binder.URIBinderPool, bind)
}()

if err := b.returnErr(bind.Bind(b.ctx.Route().Params, b.ctx.Params, out)); err != nil {
return err
}

Expand All @@ -158,7 +239,16 @@ func (b *Bind) URI(out any) error {

// MultipartForm binds the multipart form into the struct, map[string]string and map[string][]string.
func (b *Bind) MultipartForm(out any) error {
if err := b.returnErr(binder.FormBinder.BindMultipart(b.ctx.RequestCtx(), out)); err != nil {
bind := binder.GetFromThePool[*binder.FormBinding](&binder.FormBinderPool)
bind.EnableSplitting = b.ctx.App().config.EnableSplittingOnParsers

// Reset & put binder
defer func() {
bind.EnableSplitting = false
binder.PutToThePool(&binder.FormBinderPool, bind)
}()

if err := b.returnErr(bind.BindMultipart(b.ctx.RequestCtx(), out)); err != nil {
return err
}

Expand Down
38 changes: 25 additions & 13 deletions bind_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ func Test_returnErr(t *testing.T) {
// go test -run Test_Bind_Query -v
func Test_Bind_Query(t *testing.T) {
t.Parallel()
app := New()
app := New(Config{
EnableSplittingOnParsers: true,
})
c := app.AcquireCtx(&fasthttp.RequestCtx{})

type Query struct {
Expand Down Expand Up @@ -111,7 +113,9 @@ func Test_Bind_Query(t *testing.T) {
func Test_Bind_Query_Map(t *testing.T) {
t.Parallel()

app := New()
app := New(Config{
EnableSplittingOnParsers: true,
})
c := app.AcquireCtx(&fasthttp.RequestCtx{})

c.Request().SetBody([]byte(``))
Expand Down Expand Up @@ -318,13 +322,13 @@ func Test_Bind_Header(t *testing.T) {
c.Request().Header.Add("Hobby", "golang,fiber")
q := new(Header)
require.NoError(t, c.Bind().Header(q))
require.Len(t, q.Hobby, 2)
require.Len(t, q.Hobby, 1)

c.Request().Header.Del("hobby")
c.Request().Header.Add("Hobby", "golang,fiber,go")
q = new(Header)
require.NoError(t, c.Bind().Header(q))
require.Len(t, q.Hobby, 3)
require.Len(t, q.Hobby, 1)

empty := new(Header)
c.Request().Header.Del("hobby")
Expand Down Expand Up @@ -357,7 +361,7 @@ func Test_Bind_Header(t *testing.T) {
require.Equal(t, "go,fiber", h2.Hobby)
require.True(t, h2.Bool)
require.Equal(t, "Jane Doe", h2.Name) // check value get overwritten
require.Equal(t, []string{"milo", "coke", "pepsi"}, h2.FavouriteDrinks)
require.Equal(t, []string{"milo,coke,pepsi"}, h2.FavouriteDrinks)
var nilSlice []string
require.Equal(t, nilSlice, h2.Empty)
require.Equal(t, []string{""}, h2.Alloc)
Expand Down Expand Up @@ -386,13 +390,13 @@ func Test_Bind_Header_Map(t *testing.T) {
c.Request().Header.Add("Hobby", "golang,fiber")
q := make(map[string][]string, 0)
require.NoError(t, c.Bind().Header(&q))
require.Len(t, q["Hobby"], 2)
require.Len(t, q["Hobby"], 1)

c.Request().Header.Del("hobby")
c.Request().Header.Add("Hobby", "golang,fiber,go")
q = make(map[string][]string, 0)
require.NoError(t, c.Bind().Header(&q))
require.Len(t, q["Hobby"], 3)
require.Len(t, q["Hobby"], 1)

empty := make(map[string][]string, 0)
c.Request().Header.Del("hobby")
Expand Down Expand Up @@ -543,7 +547,9 @@ func Test_Bind_Header_Schema(t *testing.T) {
// go test -run Test_Bind_Resp_Header -v
func Test_Bind_RespHeader(t *testing.T) {
t.Parallel()
app := New()
app := New(Config{
EnableSplittingOnParsers: true,
})
c := app.AcquireCtx(&fasthttp.RequestCtx{})

type Header struct {
Expand Down Expand Up @@ -627,13 +633,13 @@ func Test_Bind_RespHeader_Map(t *testing.T) {
c.Response().Header.Add("Hobby", "golang,fiber")
q := make(map[string][]string, 0)
require.NoError(t, c.Bind().RespHeader(&q))
require.Len(t, q["Hobby"], 2)
require.Len(t, q["Hobby"], 1)

c.Response().Header.Del("hobby")
c.Response().Header.Add("Hobby", "golang,fiber,go")
q = make(map[string][]string, 0)
require.NoError(t, c.Bind().RespHeader(&q))
require.Len(t, q["Hobby"], 3)
require.Len(t, q["Hobby"], 1)

empty := make(map[string][]string, 0)
c.Response().Header.Del("hobby")
Expand Down Expand Up @@ -751,7 +757,9 @@ func Benchmark_Bind_Query_WithParseParam(b *testing.B) {
func Benchmark_Bind_Query_Comma(b *testing.B) {
var err error

app := New()
app := New(Config{
EnableSplittingOnParsers: true,
})
c := app.AcquireCtx(&fasthttp.RequestCtx{})

type Query struct {
Expand Down Expand Up @@ -1341,7 +1349,9 @@ func Benchmark_Bind_URI_Map(b *testing.B) {
func Test_Bind_Cookie(t *testing.T) {
t.Parallel()

app := New()
app := New(Config{
EnableSplittingOnParsers: true,
})
c := app.AcquireCtx(&fasthttp.RequestCtx{})

type Cookie struct {
Expand Down Expand Up @@ -1414,7 +1424,9 @@ func Test_Bind_Cookie(t *testing.T) {
func Test_Bind_Cookie_Map(t *testing.T) {
t.Parallel()

app := New()
app := New(Config{
EnableSplittingOnParsers: true,
})
c := app.AcquireCtx(&fasthttp.RequestCtx{})

c.Request().SetBody([]byte(``))
Expand Down
79 changes: 67 additions & 12 deletions binder/binder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package binder

import (
"errors"
"sync"
)

// Binder errors
Expand All @@ -10,15 +11,69 @@ var (
ErrMapNotConvertable = errors.New("binder: map is not convertable to map[string]string or map[string][]string")
)

// Init default binders for Fiber
var (
HeaderBinder = &headerBinding{}
RespHeaderBinder = &respHeaderBinding{}
CookieBinder = &cookieBinding{}
QueryBinder = &queryBinding{}
FormBinder = &formBinding{}
URIBinder = &uriBinding{}
XMLBinder = &xmlBinding{}
JSONBinder = &jsonBinding{}
CBORBinder = &cborBinding{}
)
var HeaderBinderPool = sync.Pool{
New: func() any {
return &HeaderBinding{}
},
}

var RespHeaderBinderPool = sync.Pool{
New: func() any {
return &RespHeaderBinding{}
},
}

var CookieBinderPool = sync.Pool{
New: func() any {
return &CookieBinding{}
},
}

var QueryBinderPool = sync.Pool{
New: func() any {
return &QueryBinding{}
},
}

var FormBinderPool = sync.Pool{
New: func() any {
return &FormBinding{}
},
}

var URIBinderPool = sync.Pool{
New: func() any {
return &URIBinding{}
},
}

var XMLBinderPool = sync.Pool{
New: func() any {
return &XMLBinding{}
},
}

var JSONBinderPool = sync.Pool{
New: func() any {
return &JSONBinding{}
},
}

var CBORBinderPool = sync.Pool{
New: func() any {
return &CBORBinding{}
},
}
Comment on lines +14 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Initialize decoder fields in binder instances to avoid nil pointer dereferences

The JSONDecoder, XMLDecoder, and CBORDecoder fields in JSONBinding, XMLBinding, and CBORBinding are not initialized, which may lead to nil pointer dereferences when their Bind methods are called. Ensure these fields are properly initialized when creating binder instances.

For example, initialize JSONDecoder in JSONBinding:

return &JSONBinding{
+   JSONDecoder: utils.JSONUnmarshalDefault,
}

Similarly, initialize XMLDecoder and CBORDecoder:

// For XMLBinding
return &XMLBinding{
+   XMLDecoder: utils.XMLUnmarshalDefault,
}

// For CBORBinding
return &CBORBinding{
+   CBORDecoder: utils.CBORUnmarshalDefault,
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var HeaderBinderPool = sync.Pool{
New: func() any {
return &HeaderBinding{}
},
}
var RespHeaderBinderPool = sync.Pool{
New: func() any {
return &RespHeaderBinding{}
},
}
var CookieBinderPool = sync.Pool{
New: func() any {
return &CookieBinding{}
},
}
var QueryBinderPool = sync.Pool{
New: func() any {
return &QueryBinding{}
},
}
var FormBinderPool = sync.Pool{
New: func() any {
return &FormBinding{}
},
}
var URIBinderPool = sync.Pool{
New: func() any {
return &URIBinding{}
},
}
var XMLBinderPool = sync.Pool{
New: func() any {
return &XMLBinding{}
},
}
var JSONBinderPool = sync.Pool{
New: func() any {
return &JSONBinding{}
},
}
var CBORBinderPool = sync.Pool{
New: func() any {
return &CBORBinding{}
},
}
var HeaderBinderPool = sync.Pool{
New: func() any {
return &HeaderBinding{}
},
}
var RespHeaderBinderPool = sync.Pool{
New: func() any {
return &RespHeaderBinding{}
},
}
var CookieBinderPool = sync.Pool{
New: func() any {
return &CookieBinding{}
},
}
var QueryBinderPool = sync.Pool{
New: func() any {
return &QueryBinding{}
},
}
var FormBinderPool = sync.Pool{
New: func() any {
return &FormBinding{}
},
}
var URIBinderPool = sync.Pool{
New: func() any {
return &URIBinding{}
},
}
var XMLBinderPool = sync.Pool{
New: func() any {
return &XMLBinding{
XMLDecoder: utils.XMLUnmarshalDefault,
}
},
}
var JSONBinderPool = sync.Pool{
New: func() any {
return &JSONBinding{
JSONDecoder: utils.JSONUnmarshalDefault,
}
},
}
var CBORBinderPool = sync.Pool{
New: func() any {
return &CBORBinding{
CBORDecoder: utils.CBORUnmarshalDefault,
}
},
}


func GetFromThePool[T any](pool *sync.Pool) T {
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
binder, ok := pool.Get().(T)
if !ok {
panic(errors.New("failed to type-assert to T"))
}

return binder
}
Comment on lines +68 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid panicking on type assertion failure in GetFromThePool

Using panic on type assertion failures can lead to unexpected crashes. Consider returning an error instead to handle such cases gracefully.

Suggested change:

-func GetFromThePool[T any](pool *sync.Pool) T {
+func GetFromThePool[T any](pool *sync.Pool) (T, error) {
    binder, ok := pool.Get().(T)
    if !ok {
-       panic(errors.New("failed to type-assert to T"))
+       var zero T
+       return zero, errors.New("failed to type-assert to T")
    }

-   return binder
+   return binder, nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func GetFromThePool[T any](pool *sync.Pool) T {
binder, ok := pool.Get().(T)
if !ok {
panic(errors.New("failed to type-assert to T"))
}
return binder
}
func GetFromThePool[T any](pool *sync.Pool) (T, error) {
binder, ok := pool.Get().(T)
if !ok {
var zero T
return zero, errors.New("failed to type-assert to T")
}
return binder, nil
}


func PutToThePool[T any](pool *sync.Pool, binder T) {
pool.Put(binder)
}
Loading
Loading