monoculum / formam

a package for decode form's values into struct in Go
Apache License 2.0
190 stars 19 forks source link

Decoding a multi-checkbox fieldset #33

Closed Ullaakut closed 4 years ago

Ullaakut commented 4 years ago

Hi! Thanks for your great library.

I have encountered an issue with decoding fieldsets:

        <fieldset>
            <legend>Select labels</legend>
            <input type="checkbox" name="labels" value="label1">label1
            <input type="checkbox" name="labels" value="label2">label2
            <input type="checkbox" name="labels" value="label3">label3
            <input type="checkbox" name="labels" value="label4">label4
        </fieldset>

When selecting multiple checkboxes, the form data is sent appropriately as expected:

INFO[2020-09-20T08:44:35+02:00] /submit/ content_type=application/x-www-form-urlencoded db=0s duration=1.374036ms form="{\"authenticity_token\":[\"MONVHBgDotwySs80XP5vV901VtmULCrk73Sxe5oYSN+hUpiIPKnGdO8DBRUlizJBmbkganMr4C6QfPNJsHnncg==\"],\"description\":[\"test\"],\"labels\":[\"label1\",\"label2\",\"label3\",\"label4\"], ...

But then when decoded into a []string only the first value is unmarshalled, and the []string results in:

BOUND ENTRY: &models.Entry{Title:"Test", Description:"Test", Labels:slices.String{"tileable"}}

For extra info, I don't think it matters but just in case, I'm using gobuffalo's Bind() function and the type I unmarshal into is not directly a []string but a wrapper around it, a slices.String from gobuffalo's pop/slices package.

emilgpa commented 4 years ago

I think this problem is related (I would say it is the same) with https://github.com/monoculum/formam/issues/28. Possibly, gobuffalo does not have the latest version that fixes this problem. To be safe, is it possible that you could include an example to reproduce with the html and Go code?

Ullaakut commented 4 years ago

Hi @emilgpa ! Thanks for the quick response :)

It indeed works with the latest version of your library:

package test

import (
    "fmt"
    "net/url"
    "testing"

    "github.com/monoculum/formam"
)

type LabelContainer struct {
    Labels []string
}

func TestDecode(t *testing.T) {
    var vals = url.Values{
        // anonymous
        "Labels": []string{"label1", "label2", "label3"},
    }

    var lc LabelContainer
    err := formam.NewDecoder(nil).Decode(vals, &lc)
    if err != nil {
        t.Error(err)
        t.FailNow()
    }

    fmt.Println(lc.Labels) // prints [label1 label2 label3]
}

This issue can then be closed and I'll open an issue on gobuffalo to update their dependency!

arp242 commented 4 years ago

This issue can then be closed and I'll open an issue on gobuffalo to update their dependency!

It looks like it already is at the latest version: https://github.com/gobuffalo/buffalo/blob/master/go.mod#L37

Maybe that's just not in the version you're using?

Ullaakut commented 4 years ago

@arp242 You're right, I noticed it before opening an issue so I ended up double-checking my test and it turns out I was wrong 😅

Actually I wasn't thorough enough in my test, the issue is with the slices.String type. When replacing the []string with a slices.String, the error is reproduced (reopening the issue).

Here is a reproducible example:

package test

import (
    "fmt"
    "net/url"
    "testing"

    "github.com/gobuffalo/pop/v5/slices"
    "github.com/monoculum/formam"
)

type LabelContainer struct {
    Labels slices.String
}

func TestDecode(t *testing.T) {
    var vals = url.Values{
        // anonymous
        "Labels": []string{"label1", "label2", "label3"},
    }

    var lc LabelContainer
    err := formam.NewDecoder(nil).Decode(vals, &lc)
    if err != nil {
        t.Error(err)
        t.FailNow()
    }

    fmt.Println(lc.Labels) // prints [label1]
}

As you can see in the link above, the slices.String type is a []string, but it probably still messes with some reflection calls?

emilgpa commented 4 years ago

The error happens because slices.String uses the UnmarshalText interface string.go#L54 and formam passes the first value always https://github.com/monoculum/formam/blob/d7a8fbd33677d897e6e2b1407a92ed2ffc2fb19c/formam.go#L672

When the field to decode the value is a slice/array then it always be the first value. The UnmarshalText is used mainly when the field to decode is a string field but I see that slices.String uses a CSV format to decode it to slice. So if the text passed to the interface was "label1,label2,label3" then it will be correctly decoded but I am afraid including this functionality may break someone's application using our library.

Another solution would be to disable the interface by field or globally.

What do you think @arp242?

emilgpa commented 4 years ago

@Ullaakut At the moment you can to pass the labels/checkboxes as a single string separated by comma from the browser or just before to decoding in Go.

emilgpa commented 4 years ago

@Ullaakut the latest version includes DisableUnmarshalText in the options. This is possibly a more elegant solution to your problem. In the future I hope to include to disable the interface per field.