proposal: helper methods to bind buildin types from query params/path params

[aldas]

do continue discussion #1681 (comment)_

just finished porting older php app to go and there is one nice feature that PHP has - mapping &param[]=1&param[]=2 to array [1,2]. And looking at other binding APIs there are methods to bind params to native type ala Int64, Float64, Bool - these are also nice things that I always seems to implement as similar utility functions in my Echo apps.

ala

func ToValidInt64(input string, defaultValue int64) (int64, bool) {
	if input == "" {
		return defaultValue, true
	}
	n, err := strconv.Atoi(input)
	if err != nil {
		return 0, false
	}
	return int64(n), true
}

and use in handlers

	length, ok := sanitizer.ToValidInt64(c.QueryParam("length"), 25)
	if !ok {
		return apperror.NewValidationError("length is not valid int64 value")
	}

Maybe echo could have special func/structs for handling things like that. ala very short API for usage like that.

length := int64(25) // default value
var csv bool
var params []string

b := echo.BindQuery(c) // deals only with query params
if err := b.Int64(&length, "length"); err != nil {
    return errors.wrap(err, "length is not valid int64 value")
}
if err := b.Bool(&csv, "csv"); err != nil {
    return errors.wrap(err, "csv is not valid bool value")
}
// NB: go URL returns query params as list of values `c.Request().URL.Query()` 
// but Echo  `c.QueryParam("params[]")` returns single value  so it is not possible in Echo
if err := b.SliceStrings(&params, "params[]"); err != nil { 
    return errors.wrap(err, "params contains invalid values")
}
// or name like that
if err := b.Int64s(&ids, "ids[]"); err != nil { 
    return errors.wrap(err, "ids contains invalid values")
}

or even this - fluent APIs are not very idiomatic go but for me I usually have bunch of query params that I need to bind and need to check for error (first) and return some meaningful message

 // deals only with query params, returns on first error, error struct contains field name as separate member
err := echo.BindQuery(c).
   Int64(&length, "length").
   Bool(&csv, "csv").
   Bind() // and ala BindErrs() that tries to bind all returns all errors
if err != nil {
   return errors.Wrapf(err, "%s can not be bind", (err.(echo.BindError)).field)
}

I have not used any library for this kind of stuff but that later one is probably already implemented in some binding/validation or something-something library

this would be backwards compatible as it would not change existing interfaces (so could be released as 4.x minor version)

Originally posted by @aldas in #1681 (comment)

[aldas]

@lammel @pafuent @iambenkay I created small mvp to test this idea out (separate helper func for each type). What are your thoughts?

master...aldas:idea_for_binder

short example

		var payload User
		length := int64(50) // default length is 50

		b := echo.BindQueryParams(c)

		if err := b.Int64("length", &length); err != nil {
			return err
		}
		if err := b.Bool("active", &payload.Active); err != nil {
			return err
		}

[iambenkay]

I think this is great but should be in addition to binding all params to a struct at once.

Maybe specific params can be binded first and after that the rest can be binded to a struct...

That would have a larger use case... Restricting binding of query params for example to happening step by step would considerably create more LOCs for the dev and inconvenience even... Say there are 10 query params would be easier to bind all of them to a struct than create 10 variables to bind each of them one by one.

Apart from that this is really great 🙌🙌

[aldas]

This is definitely more lines of code than using current DefaultBinder way (binding everything in one call), but this is trade-off of doing it one by one. Doing it one by one gives you:

• reduces boilerplate code converting string(s) to type X as query/path params values are string or []string
• ability to return your own error (messages) for each field
• have different behavior - ala I really do not care when int64 fields x and y have parsing error out of 10 fields binded from query params.

shorter version would be to have similar thing with fluent API ala err := echo.BindQuery(c).Int64("length", &length).Bool("csv", &csv).Bind() but would mean that when you chain multiple field bindings together and want to know what field actually failed you would need to access error for that (error needs to contain field info).

[iambenkay]

echo/bind.go

Line 171 in 2b36b3d

func setWithProperType(valueKind reflect.Kind, val string, structField reflect.Value) error {

This function exists to do type conversion under the hood when structs are provided as the destination of the bind so it kind of already solves your first concern.
The second is a good one and is extremely useful but a solution that works with binding all at once is probably checking the struct field for a struct tag titled msg or something of your choosing. so that for fields where the validation fails it would return that message.

[aldas]

Problem with that method is that setWithProperType signature (and other methods) depends on reflect package types and need destination be of type reflect.Value

[iambenkay]

IMO I don't really see anything wrong with that..
based on the type definitions in the struct, it serializes the data from the request into structs based on their field type. Looks very sane to me.

[aldas]

to be able to use these functions you destination must be converted to reflect.Value at first seems less lines but it is quite a bit magic code inside reflect itself

func TestXX(t *testing.T) {
	value := "1"

	id := int8(1)
	val := reflect.ValueOf(&id).Elem()
	err := setIntField(value, 0, val)

	if assert.NoError(t, err) {
		assert.Equal(t, int8(1), id)
	}
}

[iambenkay]

Yeah I get that. but currently it is only used with structs and what defines the destination type is the struct it is being serialized to. so the type is taken from the struct field, used to parse the string into that type and set as the struct field's value.

Maybe I am not understanding your angle but this seems perfectly normal and logical

[aldas]

We are probably writing about different things.

For your needs Bind is capable enough and you do not mind using structs as target and not particularly concerned about control over errors when some field conversion fails.

What this proposal aims is to give user more low level/fine grained control over binding and implements helper functionality for binding/conversion methods. These functions are similar to what struct binding uses but do not use unsafe reflect package code.

[lammel]

I personally like the fluent syntax. It reminds me of zerolog field composition which also works out quite well and real code.
So we might consider to offer additionally the struct (or interface{}) binding an explizit method for single fields.

We need to check though if it helps or adds confusion for the "correct" way to do binding (I usually like that there is one way to do it and a way to make things custom).

[pafuent]

@aldas I just found that Echo supports binding to a slice using a "different" syntax. Please see this test that I wrote based on an existing one on bind_test.go

func TestBindSlices(t *testing.T) {
	e := New()
	req := httptest.NewRequest(GET, "/?str=foo&str=bar&str=foobar", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)
	result := struct {
		Str []string `query:"str"`
	}{}
	err := c.Bind(&result)
	if assert.NoError(t, err) {
		assert.Equal(t, []string{"foo", "bar", "foobar"}, result.Str)
	}
}

@iambenkay I remember that in other issue/PR you mentioned that it would be nice to have a way to fill out a []string with a csv query param, well it seems that its doable, but not against []string directly, you need a new type (yes, I know is not perfect). Check this test.

type ALotOfStrings []string

func (s *ALotOfStrings) UnmarshalParam(src string) error {
	*s = append(*s, strings.Split(src, ",")...)
	return nil
}

func TestBindSlicesFromCSV(t *testing.T) {
	e := New()
	req := httptest.NewRequest(GET, "/?str=foo,bar,foobar", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)
	result := struct {
		Str ALotOfStrings `query:"str"`
	}{}
	err := c.Bind(&result)
	if assert.NoError(t, err) {
		assert.Equal(t, ALotOfStrings([]string{"foo", "bar", "foobar"}), result.Str)
	}
}

I guess Echo documentation needs more ❤️

[iambenkay]

Yeah exactly. This is the only way I could even do it.

Your first snippet however is really great. I didn't know about that! It's a solution for me too!

[aldas]

so fluent API would be something like that

master...aldas:idea_for_binder_fluent

In case you want to fail fast

		type SearchOpts struct {
			Active bool
		}

		var payload SearchOpts
		var ids []int64
		length := int64(50) // default length is 50

		// create binder that stops binding at first error
		b := echo.BindQueryParams(c).FailFast()

		err := b.Int64("length", &length).
			Int64s("ids", &ids).
			Bool("active", &payload.Active).
			BindError() // returns first binding error
		if err != nil {
			bErr := err.(echo.BindingError)
			return fmt.Errorf("my own custom error for field: %s value: %s", bErr.Field, bErr.Value)
		}

or just gather all bind errors

		b := echo.BindQueryParams(c)

		errs := b.Int64("length", &length).
			Int64s("ids", &ids).
			Bool("active", &payload.Active).
			BindErrors() // returns all errors
		if errs != nil {
			for _, err := range errs {
				bErr := err.(echo.BindingError)
				log.Printf("in case you want to access what field: %s value: %s failed", bErr.Field, bErr.Value)
			}
			return fmt.Errorf("%v fields failed to bind", len(errs))
		}

to be honest I thought it would make single field binding messier but it does not

target := int64(100) // default value is 100
if err := b.Int64("id", &target).BindError(); err != nil {
  return err
}

[aldas]

a little different take would be to have generic method that does type switch inside and uses appropriate conversion method (to slice or not). As it is only for native types it does not require reflection.

line

but I personally feel that it would make code a little bit less self describing

var payload SearchOpts
var ids []int64
var nrs []int64
length := int64(50) // default length is 50

errs := b.Bind("length", &length).
	Bind("active", &payload.Active).
	Bind("numbers", &nrs).
	BindWithDelimiter("ids", &ids, ","). // `&ids=1,2&ids=2` -> [1,2,2]
	BindErrors()          

[pafuent]

First off all, thanks for the modifications on Makefile!!! That really helps newcomers to Echo that want to contribute to it.
The implementation of the proposal LGTM!
The only change that I would propose is related to naming: instead of BindQueryParams() I think is more descriptive QueryParamsBinder(), the method is not binding anything, it's just returning the binder.