-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
🔥 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
base: main
Are you sure you want to change the base?
Changes from 4 commits
97da803
0c41334
32dd82b
4799c50
e6ff948
81bcf95
87a8dea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" | ||||||
) | ||||||
|
@@ -273,3 +275,58 @@ | |||||
// No suitable content type found | ||||||
return ErrUnprocessableEntity | ||||||
} | ||||||
|
||||||
func (b *Bind) All(out any) error { | ||||||
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() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of |
||||||
|
||||||
// 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 { | ||||||
|
||||||
if err := bindFunc(tempStruct); err != nil { | ||||||
return err | ||||||
} | ||||||
|
||||||
tempStructVal := reflect.ValueOf(tempStruct).Elem() | ||||||
mergeStruct(outElem, tempStructVal) | ||||||
} | ||||||
|
||||||
return nil | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle “no body” and similar benign errors so
- 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 🧰 Tools🪛 GitHub Check: lint[failure] 301-301: 🪛 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()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Copilot is powered by AI, so mistakes are possible. Review output carefully before use. Positive FeedbackNegative Feedback |
||||||
if dstField.CanSet() && srcField.IsValid() { | ||||||
dstField.Set(srcField) | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
func isZero(value any) bool { | ||||||
v := reflect.ValueOf(value) | ||||||
return v.IsZero() | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
) | ||
|
@@ -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 { | ||
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) { | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t.Parallel() | ||
type User struct { | ||
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 { | ||
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: 2, | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - assert.Error(t, err)
+ require.Error(t, err)
🧰 Tools🪛 GitHub Check: lint[failure] 2027-2027: |
||
|
||
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) | ||
} | ||
}) | ||
} | ||
} | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// go test -run Test_Bind_All_Uri_Precedence | ||
func Test_Bind_All_Uri_Precedence(t *testing.T) { | ||
t.Parallel() | ||
type User struct { | ||
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 { | ||
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) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add function to docs