Aliased slices should be consistent with builtin slices

[ready4god2513]

In working with an API at work we had the use case on query params to accept the following syntaxes:

• ?foo=1&foo=2&foo=3
• ?foo=1,2,3

Initially we were using only the former syntax, but then a new library was developed that sent the latter syntax. When we switched from defining our field as []int to a custom type Ints that implemented UnmarshalParam we found that when there were multiple values for the same key, the UnmarshalParam would only receive the first value and others would be lost.

This change brings
consistency between the builtin slices and the aliased slices for query parameters.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.16%. Comparing base (fa70db8) to head (84fc44e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2602      +/-   ##
==========================================
+ Coverage   93.09%   93.16%   +0.06%     
==========================================
  Files          39       39              
  Lines        4676     4679       +3     
==========================================
+ Hits         4353     4359       +6     
+ Misses        234      232       -2     
+ Partials       89       88       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ready4god2513]

Is it possible to get some eyes/thoughts on this one?

[aldas]

This does not feel right. Take for an example. I modified your type to parse and append

type IntArray []int

// UnmarshalParam converts value to *Int64Slice.  This allows the API to accept
// a comma-separated list of integers as a query parameter.
func (i *IntArray) UnmarshalParam(value string) error {
	if value == "" {
		*i = IntArray([]int{})
		return nil
	}
	var values = strings.Split(value, ",")
	var numbers = make([]int, 0, len(values))

	for _, v := range values {
		n, err := strconv.ParseInt(v, 10, 64)
		if err != nil {
			return fmt.Errorf("'%s' is not an integer", v)
		}

		numbers = append(numbers, int(n))
	}

	*i = append(*i, numbers...)
	//*i = IntArray(numbers)
	return nil
}

Now this does not work anymore.

func TestBindUnmarshalParamMultiple(t *testing.T) {
	e := New()
	req := httptest.NewRequest(http.MethodGet, "/?a=1,2,3&a=4,5,6&b=1&b=2", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)
	result := struct {
		IA IntArray `query:"a"`
		IB IntArray `query:"b"`
	}{}
	err := c.Bind(&result)
	assert.NoError(t, err)
	assert.Equal(t, IntArray([]int{1, 2, 3, 4, 5, 6}), result.IA)
	assert.Equal(t, IntArray([]int{1, 2}), result.IB)
}

I think adding additional (lets start of as private) interface for unmarshalling that takes in all input values would be better

type bindMultipleUnmarshaler interface {
	UnmarshalParams(params []string) error
}

This way we preserve current behavior - when there are multiple input values for BindUnmarshaler the first one is bound. This PR implementation potentially breaks this with code like that *i = append(*i, numbers...)

or at least change loop to continue with next field if it was able to unmarshal that field

		wasUnmarshalled := false
		for _, v := range inputValue {
			if ok, err := unmarshalField(typeField.Type.Kind(), v, structField); ok {
				if err != nil {
					return err
				}
				wasUnmarshalled = true
			}
		}
		if wasUnmarshalled {
			continue
		}

[aldas]

I am closing this PR. #2607 adds support for UnmarshalParams(params []string) error interface

[aldas]

@ready4god2513 thanks for raising this PR!

[ready4god2513]

Perfect. Thanks for the solution. That works well

…

On Mon, Mar 11, 2024 at 1:52 PM Martti T. ***@***.***> wrote: @ready4god2513 <https://github.com/ready4god2513> thanks for raising this PR! — Reply to this email directly, view it on GitHub <#2602 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5WUVGTCEPOI5TM5M2TTTYXYKQHAVCNFSM6AAAAABECDCVGOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBZGQZDENZYGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>