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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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. https://github.com/labstack/echo/pull/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 https://github.com/labstack/echo/pull/2602#issuecomment-1989422781, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5WUVGTCEPOI5TM5M2TTTYXYKQHAVCNFSM6AAAAABECDCVGOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBZGQZDENZYGE . You are receiving this because you were mentioned.Message ID: @.***>