-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolaente What is it about?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide steps to reproduces?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.