Support of embeded *Struct for form binding

[tpng]

type Embed struct {
    Test string `form:"test" binding:"required"`
}

type Outer struct {
    *Embed
}

Currently, if I bind to Outer{}, *Embed is nil after binding even if I pass the "test" parameter in the request.

[dvliman]

I have similar question. Is it possible for binding to work on nested / embedded structs?

[mholt]

@tpng I started investigating this a while back -- but I've been swamped by other things lately. Sorry this is taking a while.

@dvliman Yes, it is possible to bind both nested and embedded structs. See this file: https://github.com/martini-contrib/binding/blob/master/common_test.go and the associated tests (like form_test.go). It looks like pointers to embedded structs are the issue being reported here.

[dvliman]

thanks for quick response! @mholt

[mholt]

@tpng Okay, I might be missing something or making this harder than it needs to be, but so far I haven't been able to find a solid way to bind to embedded pointer to struct. The problem is that the embedded pointer is always nil because this package makes a new, empty instance of the parent struct, putting all its fields at the zero value by default. I can make an instance of the embedded struct and even populate it, however, it's nearly impossible to put under test because the struct's address is entirely variable, and comparing them isn't possible the way these tests are structured.

Plus it breaks nearly every other test with panics. But! Here's the change I made to at least get your scenario to work. I replaced a few lines starting with binding.go:213 with this:

for i := 0; i < typ.NumField(); i++ {
        typeField := typ.Field(i)
        structField := formStruct.Field(i)

        if typeField.Type.Kind() == reflect.Ptr {
            structField.Set(reflect.New(typeField.Type.Elem()))
        }

        if typeField.Type.Kind() == reflect.Struct || (structField.Type().Kind() == reflect.Ptr && structField.Type().Elem().Kind() == reflect.Struct) {
            mapForm(structField, form, formfile, errors)
// ...

to get it to work. It's a mess but it was just for exploring. Again, unfortunately, it breaks many other tests so don't use this... I'm posting it merely for documenting what I tried. Notice that there's a new if block that was added and also an or clause to the last if statement.

(Update: We can limit the damage to other tests by adding another condition to see if the field that is a pointer to a struct also has typeField.Anonymous as true, meaning that it's embedded.)

Because of the complexity, I'm going to electing not to support this kind of binding for now. Sorry. I guess if you have an embedded struct, don't use a pointer to it if at all possible. I'll clarify this in the readme.

If somebody can figure out how to do this elegantly and without breaking existing tests, I'd be all for it: but I wasn't smart enough to figure it out.

[tpng]

Thanks for trying!

Guess they would have to implement the Validate function to check if the embedded field is nil or not. I am using the json binding for this use case and it can bind embedded struct pointer without problems. I will take some time to investigate the encoding/json package and see how they do it.

[mholt]

It's quite likely I'm doing it in a complicated way. This package was my foray into Go reflection, and though I've read lots about it, it's still tricky.

Looking at encoding/json is a great idea that, for some reason, I never considered this time. I will also take a look there when I have a chance. Let me know if you find anything useful/interesting!

[tpng]

Just taken a look in encoding/json, they are doing the same thing, i.e. structField.Set(reflect.New(typeField.Type.Elem())) but additionally they update structField to the struct value, i.e. structField = structField.Elem() before proceeding.

The following modification does not break any tests.

for i := 0; i < typ.NumField(); i++ {
        typeField := typ.Field(i)
        structField := formStruct.Field(i)

        embeddedStructPtr := false
        if typeField.Type.Kind() == reflect.Ptr && typeField.Anonymous {
            structField.Set(reflect.New(typeField.Type.Elem()))
            embeddedStructPtr = true
            structField = structField.Elem()
        }

        if typeField.Type.Kind() == reflect.Struct || embeddedStructPtr {
            mapForm(structField, form, formfile, errors)

[mholt]

Ah, okay, cool. That's what happens when I'm up too late coding.

Question, though: how do we put this under test? Right now, comparing

expString := fmt.Sprintf("%+v", testCase.expected)
actString := fmt.Sprintf("%+v", actual)

is failing because the memory address of the embedded pointer is always different. We'd have to somehow check to see if that pointer is nil or not, rather than comparing actual values. I'm still thinking of an elegant way to do this.

[tpng]

I think we could in addition use reflect.DeepEqual() to test if the values are the same. maybe something like expString != actString && !reflect.DeepEqual(actual, testCase.expected)

[mholt]

Hm, the problem now is that the embedded struct pointer is always non-nil. When I'm home from work I'll have to look at encoding/json more, but it's almost as if we have to look to see if that embedded struct pointer is needed in the first place, and only then do we we create and populate it.

[mholt]

Okay. The more I look at this and think about it, the uglier it becomes. * sigh * How badly is this needed?

I don't have more time to invest in this, so rather than keep people hopeful, I invite anyone else to contribute an elegant fix if they can, just be sure to add tests and make sure everything still passes.

[tpng]

I leave my attempt as a pull request, there are some limitation on the solution.

It checks if the struct is empty or not after mapping (i.e. nothing bound), then reset the embedded pointer to nil if it is an empty struct.

[mholt]

I'm glad you came up with a pretty good fix; I wasn't sure about wanting to have those limitations but it's a great start, and we can improve it from here. Thanks for doing it!