martini-contrib / binding

Martini handler for mapping and validating a raw request into a structure.
MIT License
140 stars 47 forks source link

Pointers to basic types do not deserialize correctly #27

Closed losinggeneration closed 10 years ago

losinggeneration commented 10 years ago

If you have a basic type that's a pointer, deserialization doesn't happen correctly. For instance:

type Post struct {
    Id      uint64
    Content *string
}

causes a panic during the ValidateStruct step. The validation incorrectly assumes if it's a pointer, it's a pointer to a struct.

A full, stand alone test would be the following

package main

import (
    "log"

    "github.com/go-martini/martini"
    "github.com/martini-contrib/binding"
)

type Post struct {
    Id      uint64
    Content *string
}

func main() {
    c := martini.Classic()
    c.Post("/", binding.Json(Post{}), func(p Post) {
        log.Println(p)
    })
    c.Run()
}

With a curl test of

curl -XPOST --data '{"content": "fubar"}' http://localhost:3000/
$GOROOT/src/pkg/reflect/type.go:654 (0x48a9cd)
        (*rtype).NumField: panic("reflect: NumField of non-struct type")
$GOPATH/src/github.com/martini-contrib/binding/binding.go:191 (0x432512)
        validateStruct: for i := 0; i < typ.NumField(); i++ {
$GOPATH/src/github.com/martini-contrib/binding/binding.go:171 (0x43372a)
        func.005: errors = validateStruct(errors, obj)

I've also done a quick JSON test case with the following commit: https://github.com/losinggeneration/binding/commit/58a8515eb2be7b5226273e09fbd660b9f8a17457

mholt commented 10 years ago

Good find, thanks for the reproducible case.

Can you confirm that changing line 200 of binding.go to this fixes the problem:

            (field.Type.Kind() == reflect.Ptr && !reflect.DeepEqual(zero, fieldValue) &&
                field.Type.Elem().Kind() == reflect.Struct) {

Notice the extra condition that the field's type's Elem() must be a Struct kind before going into validate it.

losinggeneration commented 10 years ago

That does indeed look like it fixes the problem. The test case doesn't work here because it's comparing the pointer locations and not the underlying data.

mholt commented 10 years ago

Awesome, thank you! I'll get the fix rolled out shortly.