Skip to content

🔥 feat: Add All method to Bind #3373

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.

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
57 changes: 57 additions & 0 deletions bind.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package fiber

import (
"reflect"

"github.com/gofiber/fiber/v3/binder"
"github.com/gofiber/utils/v2"
)
Expand Down Expand Up @@ -273,3 +275,58 @@
// No suitable content type found
return ErrUnprocessableEntity
}

func (b *Bind) All(out any) error {
Copy link
Member

Choose a reason for hiding this comment

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

Add function to docs

outVal := reflect.ValueOf(out)
if outVal.Kind() != reflect.Ptr || outVal.Elem().Kind() != reflect.Struct {
return ErrUnprocessableEntity
}

outElem := outVal.Elem()

// Precedence: URL Params -> Body -> Query -> Headers -> Cookies
sources := []func(any) error{
b.URI,
b.Body,
b.Query,
b.Header,
b.Cookie,
}

tempStruct := reflect.New(outElem.Type()).Interface()
Copy link
Preview

Copilot AI Apr 21, 2025

Choose a reason for hiding this comment

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

The temporary struct (tempStruct) is created once outside the loop and then reused for each binding source. This may cause previous binding values to carry over into subsequent bindings. Consider reinitializing tempStruct inside the loop for each source to ensure isolated binding.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of tempStruct is to act as a temporary holder for each source binding (such as URI, Body, Query, etc.) and merge it into the final outElem struct.


// TODO: Support custom precedence with an optional binding_source tag
// TODO: Create WithOverrideEmptyValues
// Bind from each source, but only update unset fields
for _, bindFunc := range sources {

Check failure on line 301 in bind.go

View workflow job for this annotation

GitHub Actions / lint

empty-lines: extra empty line at the start of a block (revive)

if err := bindFunc(tempStruct); err != nil {
return err
}

tempStructVal := reflect.ValueOf(tempStruct).Elem()
mergeStruct(outElem, tempStructVal)
}

return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Handle “no body” and similar benign errors so All doesn’t abort early

b.Body() (and the other single‑source helpers) can legitimately return
ErrUnprocessableEntity, ErrEmptyBody, etc. when the incoming request
simply doesn’t contain data for that source.
All should treat those as non‑fatal and continue with the next source;
otherwise a perfectly valid request that, say, only has query params will
fail the whole bind.

-		if err := bindFunc(tempStruct); err != nil {
-			return err
-		}
+		if err := bindFunc(tempStruct); err != nil {
+			// Skip “nothing to bind”‑style errors and continue.
+			if errors.Is(err, ErrUnprocessableEntity) ||
+				errors.Is(err, binder.ErrEmptyBody) {
+				continue
+			}
+			return err
+		}

(You may need to import errors and, if available, the concrete “empty body”
error from the binder package.)

🧰 Tools
🪛 GitHub Check: lint

[failure] 301-301:
empty-lines: extra empty line at the start of a block (revive)

🪛 GitHub Actions: golangci-lint

[error] 301-301: golangci-lint (revive): extra empty line at the start of a block (empty-lines)


func mergeStruct(dst, src reflect.Value) {
dstFields := dst.NumField()
for i := 0; i < dstFields; i++ {
dstField := dst.Field(i)
srcField := src.Field(i)

// Skip if the destination field is already set
if isZero(dstField.Interface()) {
Copy link
Preview

Copilot AI Apr 21, 2025

Choose a reason for hiding this comment

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

Using isZero to decide whether to update a field might unintentionally overwrite valid zero values (e.g., 0 for an int). Reevaluate the logic for what constitutes an 'unset' field, possibly by using pointer types or an explicit flag.

Suggested change
if isZero(dstField.Interface()) {
if (dstField.Kind() == reflect.Ptr && dstField.IsNil()) || (dstField.Kind() != reflect.Ptr && isZero(dstField.Interface())) {

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

if dstField.CanSet() && srcField.IsValid() {
dstField.Set(srcField)
}
}
}
}

func isZero(value any) bool {
v := reflect.ValueOf(value)
return v.IsZero()
}
225 changes: 225 additions & 0 deletions bind_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@
"mime/multipart"
"net/http/httptest"
"reflect"
"strings"
"testing"
"time"

"github.com/fxamacker/cbor/v2"
"github.com/gofiber/fiber/v3/binder"
"github.com/stretchr/testify/assert"
Copy link
Member

Choose a reason for hiding this comment

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

Use testify.require() instead of assert()

"github.com/stretchr/testify/require"
"github.com/valyala/fasthttp"
)
Expand Down Expand Up @@ -1885,3 +1887,226 @@
testDecodeParser(MIMEApplicationForm, "body_param=body_param")
testDecodeParser(MIMEMultipartForm+`;boundary="b"`, "--b\r\nContent-Disposition: form-data; name=\"body_param\"\r\n\r\nbody_param\r\n--b--")
}

type RequestConfig struct {

Check failure on line 1891 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

fieldalignment: struct with 64 pointer bytes could be 56 (govet)
ContentType string
Body []byte
Headers map[string]string
Cookies map[string]string
Query string
}

func (rc *RequestConfig) ApplyTo(ctx Ctx) {
if rc.Body != nil {
ctx.Request().SetBody(rc.Body)
ctx.Request().Header.SetContentLength(len(rc.Body))
}
if rc.ContentType != "" {
ctx.Request().Header.SetContentType(rc.ContentType)
}
for k, v := range rc.Headers {
ctx.Request().Header.Set(k, v)
}
for k, v := range rc.Cookies {
ctx.Request().Header.SetCookie(k, v)
}
if rc.Query != "" {
ctx.Request().URI().SetQueryString(rc.Query)
}
}

// go test -run Test_Bind_All
func Test_Bind_All(t *testing.T) {

Check failure on line 1919 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

Test_Bind_All's subtests should call t.Parallel (tparallel)
t.Parallel()
type User struct {

Check failure on line 1921 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

fieldalignment: struct with 72 pointer bytes could be 64 (govet)
ID int `param:"id" query:"id" json:"id" form:"id"`
Avatar *multipart.FileHeader `form:"avatar"`
Name string `query:"name" json:"name" form:"name"`
Email string `json:"email" form:"email"`
Role string `header:"x-user-role"`
SessionID string `json:"session_id" cookie:"session_id"`
}
newBind := func(app *App) *Bind {
return &Bind{
ctx: app.AcquireCtx(&fasthttp.RequestCtx{}),
}
}

defaultConfig := func() *RequestConfig {
return &RequestConfig{
ContentType: MIMEApplicationJSON,
Body: []byte(`{"name":"john", "email": "[email protected]", "session_id": "abc1234", "id": 1}`),
Headers: map[string]string{
"x-user-role": "admin",
},
Cookies: map[string]string{
"session_id": "abc123",
},
Query: "id=1&name=john",
}
}

tests := []struct {

Check failure on line 1949 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

fieldalignment: struct with 48 pointer bytes could be 40 (govet)
name string
out any
expected *User
config *RequestConfig
wantErr bool
}{
{
name: "Invalid output type",
out: 123,
wantErr: true,
},
{
name: "Successful binding",
out: new(User),
config: defaultConfig(),
expected: &User{
ID: 1,
Name: "john",
Email: "[email protected]",
Role: "admin",
SessionID: "abc1234",
},
},
{
name: "Missing fields (partial JSON only)",
out: new(User),
config: &RequestConfig{
ContentType: MIMEApplicationJSON,
Body: []byte(`{"name":"partial"}`),
},
expected: &User{
Name: "partial",
},
},
{
name: "Override query with JSON",
out: new(User),
config: &RequestConfig{
ContentType: MIMEApplicationJSON,
Body: []byte(`{"name":"fromjson", "id": 99}`),
Query: "id=1&name=queryname",
},
expected: &User{
Name: "fromjson",
ID: 99,
},
},
{
name: "Form binding",
out: new(User),
config: &RequestConfig{
ContentType: MIMEApplicationForm,
Body: []byte("id=2&name=formname&[email protected]"),
},
expected: &User{
ID: 1,
Name: "formname",
Email: "[email protected]",
},
},
}

app := New()

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
bind := newBind(app)

if tt.config != nil {
tt.config.ApplyTo(bind.ctx)
}

err := bind.All(tt.out)
if tt.wantErr {
assert.Error(t, err)
return
}
assert.NoError(t, err)

Check failure on line 2027 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

require-error: for error assertions use require (testifylint)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use require for error assertions instead of assert.

The codebase consistently uses require for error assertions to stop test execution on failure.

-			assert.Error(t, err)
+			require.Error(t, err)

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: lint

[failure] 2027-2027:
require-error: for error assertions use require (testifylint)


if tt.expected != nil {
actual, ok := tt.out.(*User)
assert.True(t, ok)

assert.Equal(t, tt.expected.ID, actual.ID)
assert.Equal(t, tt.expected.Name, actual.Name)
assert.Equal(t, tt.expected.Email, actual.Email)
assert.Equal(t, tt.expected.Role, actual.Role)
assert.Equal(t, tt.expected.SessionID, actual.SessionID)
}
})
}
}

// go test -run Test_Bind_All_Uri_Precedence
func Test_Bind_All_Uri_Precedence(t *testing.T) {
t.Parallel()
type User struct {

Check failure on line 2046 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

fieldalignment: struct with 32 pointer bytes could be 24 (govet)
ID int `param:"id" json:"id" query:"id" form:"id"`
Name string `json:"name"`
Email string `json:"email"`
}

app := New()

app.Post("/test1/:id", func(c Ctx) error {
d := new(User)
if err := c.Bind().All(d); err != nil {
t.Fatal(err)
}

require.Equal(t, 111, d.ID)
require.Equal(t, "john", d.Name)
require.Equal(t, "[email protected]", d.Email)
return nil
})

body := strings.NewReader(`{"id": 999, "name": "john", "email": "[email protected]"}`)
req := httptest.NewRequest(MethodPost, "/test1/111?id=888", body)
req.Header.Set("Content-Type", "application/json")
res, err := app.Test(req)
require.NoError(t, err)
assert.Equal(t, 200, res.StatusCode)
}

// go test -v -run=^$ -bench=Benchmark_Bind_All -benchmem -count=4
func BenchmarkBind_All(b *testing.B) {
type User struct {

Check failure on line 2076 in bind_test.go

View workflow job for this annotation

GitHub Actions / lint

fieldalignment: struct with 72 pointer bytes could be 64 (govet)
ID int `param:"id" query:"id" json:"id" form:"id"`
Avatar *multipart.FileHeader `form:"avatar"`
Name string `query:"name" json:"name" form:"name"`
Email string `json:"email" form:"email"`
Role string `header:"x-user-role"`
SessionID string `json:"session_id" cookie:"session_id"`
}

app := New()
c := app.AcquireCtx(&fasthttp.RequestCtx{})

config := &RequestConfig{
ContentType: MIMEApplicationJSON,
Body: []byte(`{"name":"john", "email": "[email protected]", "session_id": "abc1234", "id": 1}`),
Headers: map[string]string{
"x-user-role": "admin",
},
Cookies: map[string]string{
"session_id": "abc123",
},
Query: "id=1&name=john",
}

bind := &Bind{
ctx: c,
}

b.ResetTimer()
for i := 0; i < b.N; i++ {
user := &User{}
config.ApplyTo(c)
if err := bind.All(user); err != nil {
b.Fatalf("unexpected error: %v", err)
}
}
}
Loading