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
+1,339 −152
Open

🐛 bug: fix EnableSplittingOnParsers is not functional #3231

Show file tree
Hide file tree
Changes from 8 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
10 changes: 10 additions & 0 deletions app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,13 @@ type Config struct { //nolint:govet // Aligning the struct fields is not necessa
// Default: xml.Marshal
XMLEncoder utils.XMLMarshal `json:"-"`

// XMLDecoder set by an external client of Fiber it will use the provided implementation of a
// XMLUnmarshal
//
// Allowing for flexibility in using another XML library for decoding
// Default: xml.Unmarshal
XMLDecoder utils.XMLUnmarshal `json:"-"`

// If you find yourself behind some sort of proxy, like a load balancer,
// then certain header information may be sent to you using special X-Forwarded-* headers or the Forwarded header.
// For example, the Host HTTP header is usually used to return the requested host.
Expand Down Expand Up @@ -560,6 +567,9 @@ func New(config ...Config) *App {
if app.config.XMLEncoder == nil {
app.config.XMLEncoder = xml.Marshal
}
if app.config.XMLDecoder == nil {
app.config.XMLDecoder = xml.Unmarshal
}
if len(app.config.RequestMethods) == 0 {
app.config.RequestMethods = DefaultMethods
}
Expand Down
99 changes: 89 additions & 10 deletions bind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,15 @@

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

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

Expand All @@ -86,7 +94,15 @@

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

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

Expand All @@ -96,7 +112,15 @@
// 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.Reset()
}()

if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil {
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

Use Request() method for type safety.

For consistency and type safety, use the abstracted Request() method instead of directly accessing RequestCtx().Request.

Apply this change to all affected methods:

-if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil {
+if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil {

Also applies to: 144-144, 218-218

return err
}

Expand All @@ -105,7 +129,15 @@

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

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

Expand All @@ -114,7 +146,15 @@

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

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

Expand All @@ -123,15 +163,31 @@

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

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 = b.ctx.App().config.XMLDecoder

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

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

Expand All @@ -140,7 +196,15 @@

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

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

Expand All @@ -149,7 +213,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() {
bind.Reset()
}()

Check warning on line 221 in bind.go

View check run for this annotation

Codecov / codecov/patch

bind.go#L216-L221 

Added lines #L216 - L221 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 223 in bind.go

View check run for this annotation

Codecov / codecov/patch

bind.go#L223 

Added line #L223 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

URI binding has test coverage, but pool and Reset behavior needs testing

The codebase already has URI binding tests in binder/uri_test.go including both unit tests (Test_URIBinding_Bind) and benchmarks (Benchmark_URIBinding_Bind). However, the specific pool usage (GetFromThePool) and Reset() behavior in the reviewed code segment are not explicitly tested.

  • Add test cases in bind_test.go to verify:
    • Proper acquisition of binder from the pool
    • Reset is called via defer as expected
    • Binder is properly returned to the pool after use
🔗 Analysis chain

Add test coverage for URI binding

The URI binding implementation looks correct but lacks test coverage. Please add tests to verify the behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check existing test coverage for URI binding

# Look for existing tests
rg -l "TestBind.*URI" .

# Look for URI binding usage patterns to help with test cases
ast-grep --pattern 'func $test($t *testing.T) {
  $$$
  Bind().URI($$$)
  $$$
}'

Length of output: 108

Script:

#!/bin/bash
# Let's try a broader search for URI binding tests and implementation

# Search for URI binding test files
rg -l "Test.*URI.*Bind|Bind.*URI" .

# Search for URI binding implementation and usage
rg -l "URIBinding|URIBinder" .

# Look for specific test patterns with URIBinding
ast-grep --pattern 'func Test$_($_) {
  $$$
  URIBinding
  $$$
}'

# Check for any test files that might test URI binding functionality
fd ".*_test.go$" --exec grep -l "URI.*Bind\|Bind.*URI" {}

Length of output: 483

Script:

#!/bin/bash
# Let's examine the actual URI binding test content

# Check URI binding test content
rg "func Test.*URI.*Bind|func.*TestBind.*URI" -A 10 ./binder/uri_test.go ./bind_test.go

# Look for specific URI binding test cases
rg "URIBinding" -B 2 -A 5 ./binder/uri_test.go

# Check test coverage for the specific pool usage and Reset functionality
rg "GetFromThePool.*URIBinding|Reset\(\)" -B 2 -A 5 ./binder/uri_test.go ./bind_test.go

Length of output: 2096

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 216-221: bind.go#L216-L221
Added lines #L216 - L221 were not covered by tests

[warning] 223-223: bind.go#L223
Added line #L223 was not covered by tests

return err
}

Expand All @@ -158,7 +229,15 @@

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

if err := b.returnErr(bind.BindMultipart(&b.ctx.RequestCtx().Request, 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
Loading
Loading