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

Fluent Binder for Query/Path/Form binding (#1717) #1736

Merged
merged 3 commits into from
Jan 7, 2021

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Dec 28, 2020

Proposed implementation for #1717


Following functions provide handful of methods for binding to Go native types from request query or path parameters.

  • QueryParamsBinder(c) - binds query parameters (source URL)
  • PathParamsBinder(c) - binds path parameters (source URL)
  • FormFieldBinder(c) - binds form fields (source URL + body)

Example:

var opts struct {
	IDs []int64
	Active bool
}
length := int64(50) // default length is 50

// creates query params binder that stops binding at first error
err := echo.QueryParamsBinder(c).
	Int64("length", &length).
	Int64s("ids", &opts.IDs).
	Bool("active", &opts.Active).
	BindError() // returns first binding error

For every supported type there are following methods:

  • <Type>("param", &destination) - if parameter value exists then binds it to given destination of that type i.e Int64(...).
  • Must<Type>("param", &destination) - parameter value is required to exist, binds it to given destination of that type i.e MustInt64(...).
  • <Type>s("param", &destination) - (for slices) if parameter values exists then binds it to given destination of that type i.e Int64s(...).
  • Must<Type>s("param", &destination) - (for slices) parameter value is required to exist, binds it to given destination of that type i.e MustInt64s(...).

for some slice types BindWithDelimiter("param", &dest, ",") supports slitting parameter values before type conversion is done
i.e. URL /api/search?id=1,2,3&id=1 can be bind to []int64{1,2,3,1}

FailFast flags binder to stop binding after first bind error during binder call chain. Enabled by default.
BindError() returns first bind error from binder and resets errors in binder. Useful along with FailFast() method
to do binding and returns on first problem
BindErrors() returns all bind errors from binder and resets errors in binder.

Types that are supported:

  • bool
  • float32
  • float64
  • int
  • int8
  • int16
  • int32
  • int64
  • uint
  • uint8/byte (does not support bytes(). Use BindUnmarshaler/CustomFunc to convert value from base64 etc to []byte{})
  • uint16
  • uint32
  • uint64
  • string
  • time
  • duration
  • BindUnmarshaler() interface
  • UnixTime() - converts unix time (integer) to time.Time
  • CustomFunc() - callback function for your custom conversion logic. Signature func(values []string) []error

Added some naive benchmarks to compare different implementations (query params)

BenchmarkDefaultBinder_BindInt64_single-6        4744263               252 ns/op              16 B/op          2 allocs/op
BenchmarkValueBinder_BindInt64_single-6         43042522                27.9 ns/op             0 B/op          0 allocs/op
BenchmarkRawFunc_Int64_single-6                 83911664                14.2 ns/op             0 B/op          0 allocs/op

BenchmarkDefaultBinder_BindInt64_10_fields-6      490051              2319 ns/op             224 B/op         13 allocs/op
BenchmarkValueBinder_BindInt64_10_fields-6       3793227               319 ns/op               0 B/op          0 allocs/op
  • BenchmarkDefaultBinder_BindInt64_single uses DefaultBinder to bind single int64 value
  • BenchmarkValueBinder_BindInt64_single uses new binder to bind single int64 value
  • BenchmarkRawFunc_Int64_single just gets param from contexts, parses it with utility method and sets to variable
  • BenchmarkDefaultBinder_BindInt64_10_fields uses DefaultBinder to bind 10 fields to struct
  • BenchmarkValueBinder_BindInt64_10_fields uses new binder to bind 10 fields to struct

@lammel @pafuent what do you think?

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #1736 (fc6dde1) into master (6119aec) will increase coverage by 2.76%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
+ Coverage   86.66%   89.43%   +2.76%     
==========================================
  Files          29       31       +2     
  Lines        2063     2594     +531     
==========================================
+ Hits         1788     2320     +532     
  Misses        175      175              
+ Partials      100       99       -1     
Impacted Files Coverage Δ
binder.go 100.00% <100.00%> (ø)
middleware/static.go 67.16% <0.00%> (-1.50%) ⬇️
bind.go 91.25% <0.00%> (-0.06%) ⬇️
router.go 97.05% <0.00%> (ø)
middleware/timeout.go 100.00% <0.00%> (ø)
echo.go 91.53% <0.00%> (+0.05%) ⬆️
middleware/csrf.go 80.28% <0.00%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6119aec...fc6dde1. Read the comment docs.

@aldas
Copy link
Contributor Author

aldas commented Dec 28, 2020

weird test failures. somewhere between Go1.13 and 1.14/1.15 time package error messages have changed. I will fix that

@lammel
Copy link
Contributor

lammel commented Jan 1, 2021

Looks great @aldas . Going to review soon.

CI: report coverage for latest go (1.15) version
Copy link
Contributor

@pafuent pafuent left a comment

Choose a reason for hiding this comment

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

I didn't reviewed the UT yet (it's quite late here 😉)

binder.go Show resolved Hide resolved
binder.go Outdated Show resolved Hide resolved
binder.go Outdated Show resolved Hide resolved
binder.go Show resolved Hide resolved
binder.go Show resolved Hide resolved
binder_external_test.go Show resolved Hide resolved
Copy link
Contributor

@pafuent pafuent left a comment

Choose a reason for hiding this comment

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

This new way of Binding is awesome!!!
Thanks also for the effort on writing that amount of tests

@aldas
Copy link
Contributor Author

aldas commented Jan 6, 2021

When reviews are done, issues fixed and it is time to merge - please merge with squashing commits.

@lammel lammel merged commit 9b0e630 into labstack:master Jan 7, 2021
pafuent pushed a commit to labstack/echox that referenced this pull request Feb 2, 2021
* Improve request binding documentation. See Echo pull request #1736 (labstack/echo#1736)

* Split binding to dedicated guide, some wording improvements

* Bind example more alike with other router examples.
Mention `UnixTimeNano()` as supported fluent binder function

Co-authored-by: Roland Lammel <rl@neotel.at>
@aldas aldas deleted the idea_for_binder_fluent branch May 23, 2021 06:06
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.

3 participants