martini-contrib / binding

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

Fix typo in the README #19

Closed attilaolah closed 10 years ago

attilaolah commented 10 years ago

At least I think it is a typo. If not, then there's a bug.

mholt commented 10 years ago

What kind of behavior are you seeing, that the handler is still executing when Bind() maps errors?

(If Bind() has errors like, for example, a required field is missing a value or the JSON deserialization failed, it should not execute the handler at the end.)

attilaolah commented 10 years ago

I just found it odd that the handler is executed even if there's a decoding error. However, I can see now how that might be the "intended" behaviour. I just find that a little odd.

Here's a full example:

package main

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

type example struct {
    ID int `json:"id"`
}

func main() {
    m := martini.Classic()
    m.Post("/ok", binding.Json(example{}), func(e example) string {
        return "shouldn't be called"
    })
    m.Post("/error", binding.Json(example{}), binding.ErrorHandler, func(e example) string {
        return "shouldn't be called"
    })
    m.Run()
}

Sending invalid JSON to /ok executes the handler. Sending invalid JSON to /error results in a 400 Bad Request.

attilaolah commented 10 years ago

However, with Bind, it is different. I get the error in both cases.

So it would seem that the error is only "silenced" when using JSON.

Let's close this issue then, and I'll have a look at binding.Json to see why it doesn't call the default error handler when there are errors.

mholt commented 10 years ago

Ah, yes. Bind() is the one-func-to-rule-them-all which bails early with an error HTTP code if there are problems binding the request. Your application doesn't even have to consider those kinds of errors, then. It's a convenience method, since it chooses form or JSON deserialization based on the Content-Type.

The readme says, for Bind():

"Your application (the final handler) will not even see the request if there are any errors."

Using Json() or other binders manually, however, does not invoke an error handler, since these are, in a way, "lower-level" functions for you to have more control if desired. This is intended.

Thanks for your feedback, though, I'll look into making that clearer for 1.0 later on. Or, would you rather like to see the other binders also invoke the error handler? (What do you do if you want to handle the errors yourself then?)

attilaolah commented 10 years ago

Thanks, that makes perfect sense then.