[proposal] Map URL params to struct

[kolaente]

First of all, thank you very much for echo, I've used it in several projects and really like it so far!

For my current project I have an URL structure like this: /some/:id/and/:more/to/:come for some endpoints and something like /another/:id/ where both of these endpoints use the same interface handler on a struct which then calls some functions on that struct and so on. As the handler is abstract, I couldn't set the url params directly in the handler, so I started implementing a custom binder, which would enable you to define a struct like so:

type Test struct {
    ID int64 `param:"id"`
    Thing string `param:"more"`
    Other string `param:"come"`
    // ... and other struct fields.
}

The binder would then map the values of the url params in /some/:id/and/:more/to/:come to their respective values in the struct.

When I was about half way through implementing this as a custom binder, I realized 95% of my custom binder was already done with (b *DefaultBinder) bindData(), so I tried to implement this directly into the standard binder and it worked pretty well. I did something like this (at the end of bindData(), right before L75):

paramNames := c.ParamNames()
paramValues := c.ParamValues()
paramVars := make(map[string][]string)
for in, name := range paramNames {
	paramVars[name] = append(paramVars[name], paramValues[in])
}
b.bindData(i, paramVars, "param")

No need to reinvent the wheel, I thought.

My question now is: Should I send a PR to "really" implement this feature? Is the way I implemented it pure garbage? Or would it be better to implement it separately on my own as a custom binder?

I thought this can be useful for others, so I shared it. This issue is a general one to discuss, as the feature is probably a pretty big one, so I thought it'd be better to create an issue first and discuss a bit instead of sending a pr right away.

[vishr]

@kolaente I believe this came up earlier as well. Do you mind looking at the existing PR/issues? I don't remember what we concluded but I am open for PR and any discussion around that - if it make sense.

[kolaente]

@vishr There was a pr #973 which got reverted until issues with #980 and #989 get addressed. #980 seems to be merged, #989 was merged and then reverted.

I've implemented the feature into my application using echo's functions and it seems to work without any issues. (The code is under https://git.kolaente.de/vikunja/api/src/branch/master/routes/crud/paramBinder.go#L15)
As #973 is now almost a year old, maybe its time to try and add this to echo and see if it fails the tests?

Concerning #989: As far as I tested it, it binds only if it finds params in the url and doesn't error out if it doesn't (The way you'd expect it). Also as my implementation calls the default binder, all values are bound to their struct methods.

[vishr]

@kolaente Why don't you send a PR?

[kolaente]

@vishr I thought discussing this first would be better, I'll send one right away!

[kolaente]

@vishr pr is up: #1165

[vishr]

@kolaente Does it work with older issues related to a similar PR?

[kolaente]

@vishr I've added a test case to check for a mix of bound params and POST data (this was the issue in #989 if I understood correctly). #980 is apparantly not related to the feature.

Please tell me if I need to add other tests.

[masterada]

I belive there was an issue with earlier implementation when binding to map or slice instead of struct. Eg: map[string]interface{}, []user. I think you should add these test cases. I also could not find a conclusion as to how to handle the later (slice of struct). Maybe it's worth checking how query parameters are handled in this case.

[stale]

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]

Fixed via #1165