Skip to content
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

Allow binders to be chained together to create multi-source binder #2311

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Oct 23, 2022

There are (rare) cases when you would want to bind value from different sources. So this PR allows binders to be chained together to create multi-source binder.

This has few use-cases because we have ValueBinders only for Path/Query/Form and most of the time if multi-source binding is needed we talk about body (ala JSON) + some other source (ala query).

Example:

// bound query params should have priority over path params
b := QueryParamsBinder(c).UseBefore(PathParamsBinder(c))

or

// bound params priority:
// 1. Path params
// 2. Query params
// 3. Form fields
b := PathParamsBinder(c).UseBefore(QueryParamsBinder(c)).UseBefore(FormFieldBinder(c))

I am not sure if this name UseBefore is best. I considered Combine, CombineWith, CombineBefore, Use, Chain

*Before is added as suffix to make it clear there is order of binding. *Before indicates that left side binder value is used before right side binder value. But I am at the moment no even sure that people would read/understand it like I do.

I am open to suggestions for naming it.

p.s. this is low priority, low value change

@codecov
Copy link

codecov bot commented Oct 23, 2022

Codecov Report

Base: 92.35% // Head: 92.38% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (510f12d) compared to base (8f2bf82).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2311      +/-   ##
==========================================
+ Coverage   92.35%   92.38%   +0.02%     
==========================================
  Files          37       37              
  Lines        4436     4450      +14     
==========================================
+ Hits         4097     4111      +14     
  Misses        247      247              
  Partials       92       92              
Impacted Files Coverage Δ
binder.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aldas
Copy link
Contributor Author

aldas commented Oct 23, 2022

@lammel when you have time. This is low priority PR, so no pressure here :)

@lammel lammel self-requested a review December 7, 2022 11:15
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my only concerns are about naming of the chaining.
The code changes LGTM!

As I have a hard time myself to find a suitable name here are some additional possibilities:

PathParamsBinder(c).UseBefore(QueryParamsBinder(c)).UseBefore(FormFieldBinder(c))
b := PathParamsBinder(c).Before(QueryParamsBinder(c)).Before(FormFieldBinder(c))
b := PathParamsBinder(c).OrElse(QueryParamsBinder(c)).OrElse(FormFieldBinder(c))
b := PathParamsBinder(c).Chain(QueryParamsBinder(c)).Chain(FormFieldBinder(c))
b := ChainBinders(PathParamsBinder(c), QueryParamsBinder(c), FormFieldBinder(c))

As the binders are processing individual fieldnames in the chaining order some params might be taken from path some from the query. So considering documentation it might be an advantage to have a ChainBinders that can be documented individually.

For a real world code example taken from our docs and slightly modified with a :

// url =  "/api/search?active=true&id=1&id=2&id=3&length=25"
var opts struct {
  IDs []int64
  Active bool
}
length := int64(50) // default length is 50

// creates binders that stops binding at first error
qb := echo.QueryParamsBinder(c).
  Int64("length", &length).
  Int64s("ids", &opts.IDs).
  Bool("active", &opts.Active).
fb := echo.FormFieldBinder(c).
  Int64s("ids", &opts.IDs).
  Bool("active", &opts.Active)
err := echo.ChainBinders(qb, fb).BindError()

(Chain could be replaced with MultiBinder or OrderedBinder, but Chain is quite precise here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants