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 2 commits
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 @@

// 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 @@

// 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 @@
// 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 @@

// 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 @@

// 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 @@

// 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 @@

// 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 @@

// 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)
}()

Check warning on line 231 in bind.go

View check run for this annotation

Codecov / codecov/patch

bind.go#L226-L231 

Added lines #L226 - L231 were not covered by tests

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

Check warning on line 233 in bind.go

View check run for this annotation

Codecov / codecov/patch

bind.go#L233 

Added line #L233 was not covered by tests
return err
}

Expand All @@ -158,7 +239,16 @@

// 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 @@

import (
"errors"
"sync"
)

// Binder errors
Expand All @@ -10,15 +11,69 @@
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{}
},

Check warning on line 17 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L15-L17 

Added lines #L15 - L17 were not covered by tests
}

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

Check warning on line 23 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L21-L23 

Added lines #L21 - L23 were not covered by tests
}

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

Check warning on line 29 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L27-L29 

Added lines #L27 - L29 were not covered by tests
}

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

Check warning on line 35 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L33-L35 

Added lines #L33 - L35 were not covered by tests
}

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

Check warning on line 41 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L39-L41 

Added lines #L39 - L41 were not covered by tests
}

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

Check warning on line 47 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L45-L47 

Added lines #L45 - L47 were not covered by tests
}

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

Check warning on line 53 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L51-L53 

Added lines #L51 - L53 were not covered by tests
}

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

Check warning on line 59 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L57-L59 

Added lines #L57 - L59 were not covered by tests
}

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

Check warning on line 65 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L63-L65 

Added lines #L63 - L65 were not covered by tests
}
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"))

Check warning on line 71 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L68-L71 

Added lines #L68 - L71 were not covered by tests
}

return binder

Check warning on line 74 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L74 

Added line #L74 was not covered by tests
}
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)

Check warning on line 78 in binder/binder.go

View check run for this annotation

Codecov / codecov/patch

binder/binder.go#L77-L78 

Added lines #L77 - L78 were not covered by tests
}
Loading
Loading