-
-
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 5 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,57 @@ | |||||
// 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 | ||||||
} | ||||||
|
||||||
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 @@ import ( | |
"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 @@ func Test_Bind_RepeatParserWithSameStruct(t *testing.T) { | |
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 { | ||
Headers map[string]string | ||
Cookies map[string]string | ||
ContentType string | ||
Query string | ||
Body []byte | ||
} | ||
|
||
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 { | ||
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"` | ||
ID int `param:"id" query:"id" json:"id" form:"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 { | ||
out any | ||
expected *User | ||
config *RequestConfig | ||
name string | ||
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) { | ||
t.Parallel() | ||
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 | ||
} | ||
require.NoError(t, err) | ||
|
||
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 { | ||
Name string `json:"name"` | ||
Email string `json:"email"` | ||
ID int `param:"id" json:"id" query:"id" form:"id"` | ||
} | ||
|
||
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 { | ||
SessionID string `json:"session_id" cookie:"session_id"` | ||
Name string `query:"name" json:"name" form:"name"` | ||
Email string `json:"email" form:"email"` | ||
Role string `header:"X-User-Role"` | ||
ID int `param:"id" query:"id" json:"id" form:"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