martini-contrib / binding

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

binding.Bind failed to bind query params from GET without content-type header #31

Closed tpng closed 10 years ago

tpng commented 10 years ago
package main

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

type TestStruct struct {
  Test string `form:"test"`
}

func test(t TestStruct) {

}

func main() {
  m := martini.Classic()
  // need to bind with binding.Form instead to work
  m.Get("/", binding.Bind(TestStruct{}), test)
  m.Run()
}

The case where the request is a GET with query parameters without specifying Content-Type header, the binding will fail in the form of PANIC: Value not found for type TestStruct. Need to call binding.Form explicitly in this case.

mholt commented 10 years ago

I'm not exactly sure what the reproducible case is here. I think the code snippet you provided is missing something. For example, what is t defined to be in the line with m.Get? Can you provide a more complete reproducible case?

tpng commented 10 years ago

Oops, my bad. t should be test (the handler). Updated the issue with complete program.

tpng commented 10 years ago

I think this is a regression introduced by this commit: https://github.com/martini-contrib/binding/commit/750ef0660be366c51c14bb750ff8aae4c7248520#diff-0ef9aa02a701d68edca28a8f67e34badL61 where the else clause invoking Form() for GET without Content-Type header was removed.

mholt commented 10 years ago

Ah. Now that you mention this, I think I was aware of this somehow when I made mholt/binding, the reflectionless, framework-agnostic version of this package, because it considers the case when no Content-Type is specified but there are query string parameters.

Thanks for filling out the code snippet. I'll take a look at putting that line back in the Bind function.

robgssp commented 10 years ago

Looks like the fix was just re-adding the else clause from that commit; pull request sent.

mholt commented 10 years ago

Great, thanks! I've had a busy week and didn't even get around to it. (Memo to self, add a test) -- anyway, merged in.