gin crashes when binding JSON array of objects because of forced validation

[oryband]

My app receives requests which are JSON arrays of objects. I'm representing these data using slices and map[string]interface{}, since the objects can have unknown fields.

I've noticed that latest changes to Gin cause are assuming the binding is using structs only and performing validation on them. This is causing gin to crash if trying to bind on something other than struct, like a slice of maps.

type Object map[string]interface{}
type MyObjects []Object

func MyHandler(context *gin.Context) {
    var objects MyObjects
    context.Bind(&objects)  // Request is `[{"key1":"value1"},{"key2":"value2"},..]`
    ...
}

2015/05/27 17:16:51 Panic recovery -> interface passed for validation is not a struct
/home/ory/.gvm/gos/go1.4.1/src/runtime/panic.go:387 (0x4168a8)
        gopanic: reflectcall(unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/home/ory/.gvm/pkgsets/go1.4.1/global/src/gopkg.in/bluesuncorp/validator.v5/validator.go:177 (0x6d3c49)
        (*Validate).structRecursive: panic("interface passed for validation is not a struct")
/home/ory/.gvm/pkgsets/go1.4.1/global/src/gopkg.in/bluesuncorp/validator.v5/validator.go:173 (0x6d3b89)
        (*Validate).structRecursive: return v.structRecursive(top, current, structValue.Elem().Interface())
/home/ory/.gvm/pkgsets/go1.4.1/global/src/gopkg.in/bluesuncorp/validator.v5/validator.go:162 (0x6d3960)
        (*Validate).Struct: return v.structRecursive(s, s, s)
/home/ory/.gvm/pkgsets/go1.4.1/global/src/github.com/gin-gonic/gin/binding/binding.go:59 (0x5e4680)
        Validate: if err := validate.Struct(obj); err != nil {
/home/ory/.gvm/pkgsets/go1.4.1/global/src/github.com/gin-gonic/gin/binding/json.go:24 (0x5e5aae)
        jsonBinding.Bind: return Validate(obj)
<autogenerated>:6 (0x5e5ff7)
/home/ory/.gvm/pkgsets/go1.4.1/global/src/github.com/gin-gonic/gin/context.go:262 (0x4f05b2)
        (*Context).BindWith: if err := b.Bind(c.Request, obj); err != nil {
/home/ory/.gvm/pkgsets/go1.4.1/global/src/github.com/gin-gonic/gin/context.go:254 (0x4f046e)
        (*Context).Bind: return c.BindWith(obj, b)
...

The question arises: Why is gin forcing me to validate my objects? More so, validate using https://github.com/bluesuncorp/validator . What if I want to validate using another preferred validator package, like https://github.com/asaskevich/govalidator ? What if I don't want to validate at all?

I feel like this type of control should be given to the developer, and not forced upon him. Maybe validation is better off as an optional (or default) middleware instead?

[bolshoy]

:+1:

[slimmy]

The question arises: Why is gin forcing me to validate my objects? More so, validate using https://github.com/bluesuncorp/validator . What if I want to validate using another preferred validator package, like https://github.com/asaskevich/govalidator ? What if I don't want to validate at all?

I feel like this type of control should be given to the developer, and not forced upon him. Maybe validation is better off as an optional (or default) middleware instead?

+1 on this. I tried updating gin the other day and this new validator is causing some problems. The main one being that I have a struct with a "required" field on a slice. It's perfectly fine that the slice is empty (in my use case) but I need it to be present in the request. This new validator wont bind the request if the slice is empty.

[larryaasen]

:+1:

[oryband]

@manucorporat wdyt? this is kind of a big deal since it suddenly broke our apps.

[manucorporat]

@slimmy @larryaasen @oryband @bolshoy try the fix.

• govalidator does not fit
• this is not a bluesuncorp/validator bug
• if you want to use govalidator, then use the "valid" tag.

[oryband]

@manucorporat thanks, will check next week and let you know

[oryband]

@manucorporat this does fix the problem, but I feel the concept is wrong:
Binding should just perform an unmarshal of the content based on Content-Type.

It should not perform implicit validations. In addition, this causes some unnecessary reflections to be executed. This shouldn't affect performance that much, but the key point we chose to use Gin here at Rounds was because Gin is fast - so let's go all the way with that and not force implicit reflections where it's not strictly necessary. Also, if I decide to validate using my own validator, that Validate() call inside Bind() is unnecessary, because I call my own Validate().

If I wish to validate I can just do so manually after binding, i.e.:

var v MyObject
if err := context.Bind(&v); err != nil {
    // Handle binding error.
}
if err := validate.Struct(&v); err != nil {  // Choose the validation library of my preference.
    // Handle validation error.
}
// Continue

At the very least, validation could be performed implicitly by setting an off-by-default flag on initialization, similar to ginmode. But I still prefer it wouldn't be there entirely.

wdyt?

[manucorporat]

@oryband I see your point and I totally agree. The concept of autovalidation was taken from martini, when Gin was created.

I have an idea, how about if the binding package has an associated "validator" that implements a binding.Validator interface.

For backcompatibity purposes I would continue setting this variable to a valid validator.
but you could set:

binding.Validator = nil

how does it sound?

[oryband]

@manucorporat that still leaves us with implicit active validation by default. Maybe go the other way around and set binding.Validator = nil by default, and let the developer set it to something else instead?

I feel keeping it like you suggested is confusing and implicit, and instead documenting this in bold and hoping people will see it, I think we're better off with turning validation off by default and letting people turn this on themselves (if it won't break anything).

And another thought, why can't we make breaking changes? Isn't gin on v1.0 rc?

[manucorporat]

@oryband in theory we can still make changes. But this change is very dangerous, since there are developers relying in a specified behaviour of Bind() and if we change it, we will be disabling silently everybody's validation. No compiler warning, nothing.

[manucorporat]

@oryband check this out: https://github.com/gin-gonic/gin/commit/fecde9fed68ad71461e90c5477172d5c7fe6fbf4

func validate(obj interface{}) error {
    if Validator == nil {
        return nil
    }
    return Validator.ValidateStruct(obj)
}

since the condition Validator == nil will be mostly be always true or always false, the overhead should be very low since the CPU will always predict the correct branch.

[oryband]

@manucorporat yeah, I understand the performance won't be hit. I was talking about the concept of Validator to be = nil by default, and a non-nil value. That's what is important to me. Are you sure there is no way to do this without serious breakage?

[manucorporat]

@oryband well, it is not really that bad:

• The validator will not validate anything unless there is a binding:"required" tag.
• Even though it is a non-nil by default, the validator is lazy initialized so it does not allocate a lot of memory.

I know this is not great for you, but I am sure many people will disagree if I disable it by default. Gin is a very fast framework, right now even faster than httpRouter. But it is a framework, it includes more features than a muxer. So built-in validation still makes sense.

As I said other times, Gin is about a well designed balance between correctness, features and performance.

Check out the framework overhead:
https://www.techempower.com/benchmarks/#section=data-r10&hw=ec2&test=plaintext

and that benchmarks used Gin 0.6, right now the overhead is even lower.

func init() {
    // shortcut for binding.Validator = nil, so you do not have to import the package.
    gin.DisableBindValidation()
    // gin.SetMode(gin.ReleaseMode)
}

[oryband]

@manucorporat ok then. That last suggestion looks good. Thanks for taking the time for explaining and fixing everything! :+1:

[manucorporat]

@oryband it is already merged in master! In fact I like it much more now. It can be disabled very easily, no code is broken and people can use different validators. Thanks for this report!

Question: are you using c.Bind() to parse JSON? You could use:

// shortcut for c.BindWith(obj, binding.JSON)
c.BindJSON()

it may be more efficient (marginally).

func (c *Context) Bind(obj interface{}) error {
    b := binding.Default(c.Request.Method, c.ContentType())
    return c.BindWith(obj, b)
}

// Shortcut for c.Bind(obj, binding.JSON)
func (c *Context) BindJSON(obj interface{}) error {
    return c.BindWith(obj, binding.JSON)
}

[bolshoy]

:+1: looks good, guys.

[oryband]

@manucorporat excellent. Will think about the BindJSON option.
Also, this validation feature should be documented in the README and godoc.

[manucorporat]

yeah! I want to spend a complete week rewriting the documentation.