Added feature to map url params to a struct with the default binder #1165
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1165 +/- ##
==========================================
- Coverage 84.28% 81.66% -2.63%
==========================================
Files 27 26 -1
Lines 2049 1936 -113
==========================================
- Hits 1727 1581 -146
- Misses 209 247 +38
+ Partials 113 108 -5
Continue to review full report at Codecov.
|
bind.go
Outdated
| paramValues := c.ParamValues() | ||
| paramVars := make(map[string][]string) | ||
| for in, name := range paramNames { | ||
| // Fix for an echo bug where a param name would show up which dont exist if its name contains "id" e.g. "userid" |
There was a problem hiding this comment.
When the param name is defined as somethingid, echo sometimes puts this as id,somethingid (one string with a comma) into c.ParamNames(). I've tried to track this down and fix it, but I wasn't successful which is why I put that workaround here. The bug seems to occur pretty randomly. Or is that a feature?
I know it's probably not the best place to do this here.
There was a problem hiding this comment.
Can you provide steps to reproduces?
There was a problem hiding this comment.
I've registered a route to /thing/:somethingid and printed out ParamNames in the route handler after calling the default binder (before my modifications).
I wasn't able to reproduce this, with some tests right now, if you were able to, this should probably be fixed in a seperate pr before this one. If not, I'd better remove my workaround as in that case it probably was an issue with my setup.
bind.go
Outdated
| params := make(map[string][]string) | ||
| for i, name := range paramNames { | ||
| // Fix for an echo bug where a param name would show up which dont exist if its name contains "id" e.g. "userid" | ||
| names := strings.Split(name, ",") |
There was a problem hiding this comment.
// BindUnmarshaler is the interface used to wrap the UnmarshalParam method.
BindUnmarshaler interface {
// UnmarshalParam decodes and assigns a value from an form or query param.
UnmarshalParam(param string) error
}and example:
type StringArray []string
func (a *StringArray) UnmarshalParam(src string) error {
*a = StringArray(strings.Split(src, ","))
return nil
}There was a problem hiding this comment.
Do you mean I should implement the bind param method via UnmarshalParam?
Or do you mean I should run the map of mapped params through UnmarshalParam to fix that bug I described?
bind.go
Outdated
| paramValues := c.ParamValues() | ||
| params := make(map[string][]string) | ||
| for i, name := range paramNames { | ||
| // Fix for an echo bug where a param name would show up which dont exist if its name contains "id" e.g. "userid" |
There was a problem hiding this comment.
// id-named fields
type testUser struct {
ID int `param:"userid"`
NameID int `param:"nameid"`
GroupID int `param:"groupid"`
SameID int `param:"sameid"`
}
...
t.Run("should be ok for id-named fields", func(t *testing.T) {
req := httptest.NewRequest(echo.GET, "/1/2/3/4", nil)
testHandler := func(ctx echo.Context) error {
u := new(testUser)
err := ctx.Bind(u)
if assert.NoError(t, err) {
assert.Equal(t, 1, u.ID)
assert.Equal(t, 2, u.NameID)
assert.Equal(t, 3, u.GroupID)
assert.Equal(t, 4, u.SameID)
}
return nil
}
e.GET("/:userid/:nameid/:groupid/:sameid", testHandler)
e.ServeHTTP(rec, req)
})can't reproduce this bug
There was a problem hiding this comment.
Did you test with the pr in place? Or without the dirty hack I put in to prevent it...
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@vishr @im-kulikov So, should I remove my workaround? Since its not reproducable, I could just remove it and look a bit more in my application where the bug comes from. |
|
@vishr any thoughts? |
|
So, what else needs to be done to get this merged? What can I do about this? @vishr |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@vishr anything else that needs to be done here to get this merged? |
|
@kolaente I will look into this today. We want this feature and I marked it for v5 but could be sooner. |
|
If the bug isn't reproducible, remove the fix. |
|
Oh, thanks! I just wanted to fix that failing test first... |
|
You can fix and send a PR.
… On Jun 21, 2019, at 6:15 AM, kolaente ***@***.***> wrote:
Oh, thanks! I just wanted to fix that failing test first...
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub <#1165?email_source=notifications&email_token=AACMVNCBBG5IIKYVELBT22TP3TH5JA5CNFSM4FL3KKH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYINWZI#issuecomment-504421221>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACMVNGXFO3TJ57FQKVK36LP3TH5JANCNFSM4FL3KKHQ>.
|
Resolves #1162.