New "binding" package replaces "Form" middleware #34

mholt · 2013-12-02T17:53:35Z

Package "binding" combines the Form middleware with new Json, Bind, and Validate middleware.

Bind

Bind is the highest-level middleware. It takes a request with a Form or JSON payload and converts it to a new struct you pass in. If a Content-Type is specified, it immediately deserializes the request into that struct according to that content type. Otherwise, it guesses until it finds one that works. (Right now, it only supports Form and JSON; XML could be added later if desired.) Bind also performs validation on the request so the application only sees the request, with the populated struct, if deserialization and validation succeeded. It works like shown here. It wraps up the functionality of the rest of the package so you have very little boilerplate to worry about.

Form

Form middleware works like it did before, deserializing form-urlencoded payload or query string into a struct, except it does not validate that the "required" attribute is satisfied. It only populates a struct and ~~performs no validation~~. (Validation is now performed with the Validate middleware.) performs validation by invoking the Validate middleware, but it does not write a 400 response if there are errors: you have to do that yourself. You get more control this way if there are errors in the request.

JSON

Json middleware works like Form except it deserializes a JSON payload in the request body into the struct. ~~It performs no validation.~~ It performs validation by invoking the Validate middleware, and as Form, does not write a 400 response if there are errors: you'd have to do that. You get more control this way for error handling.

Validate

Validate middleware only performs validation. It does not write a response if there are errors. The struct passed in is presumed to be populated from the request already. It first checks each field for the "required" attribute, and if it is required, ensures that it is a non-zero value for that field's type. (That means if you want to accept 0 in int fields, or false in bool fields, etc, don't mark it as required.)

After the required property is validated, it checks to see if the struct implements the Validator interface:

Validator interface {
    Validate(*Errors, *http.Request)
}

If so, the Validate() method is called on that struct. This comment on issue 5 also describes how that works.

This PR should effectively fix #5. It's a massive change so feel free to make tweaks, and feedback is welcome. I strongly suspect we'll be using this internally at work so it's in our best interest to make this good, too.

XML would probably be a piece of cake…

… JSON first.

…s (related to #32)

codegangsta · 2013-12-02T19:21:33Z

Stoked! I will review this after work

codegangsta · 2013-12-03T04:07:19Z

This will need a readme and godoc documentation before it lands.

Looks pretty good so far. The only thing I am hung up on design-wise is the validation. Validation currently requires the struct to be passed in, which means we need a reference to it as a pointer. This situation opens the door for some major race conditions. To get around it we can copy the struct. but for that to happen we would need some way for the validation middleware to get that copied struct, so here is what I propose.

I think the Form and JSON middlewares should bind the struct and validate/map errors. The validation middleware can still exists, but it's only purpose is to throw a bad request error if there are validation errors. The validation middleware should only be a convenience if you don't want to be checking for validation errors all over the place.

So I don't know, should it still be called validation? Or something different?

codegangsta · 2013-12-03T04:07:56Z

It's looking pretty good so far @mholt. really excited to pull the json bindings into my projects

mholt · 2013-12-03T04:30:46Z

I appreciate your enthusiasm. You have a great way of motivating contributors. 👍

Okay, so basically, these lines should be moved into the Validate middleware, and the existing code in the Validate function goes into a new, un-exported func, that is called by the Json and Form middlewares (they will no longer execute the Validate middleware: only the Bind middleware will do that).

I may be misunderstanding you... if so, make sure I'm clear on things! As I understand it now, the Json and Form middlewares would still allow the application logic (the final handler) to take the request even if the validation fails: in other words, you'd have to check for errors yourself if you didn't use the Bind middleware.

Also, could you point out the race condition? Middleware is called sequentially, with one handler running at a time, isn't it? When Validate is invoked as sort-of middle-middleware like it is now, is that what would cause the race condition?

codegangsta · 2013-12-03T04:54:30Z

@mholt You are pretty much reading my mind. I think validation enforcement should be optional. Either by allowing the application code to take care of it or for the validation handler to be added to the stack, it is up to the developer.

m.Post("/form", binding.Form(BlogPost{}), binding.Validate(), func(bp BlogPost) {
   // ...
})

I was getting ahead of myself with the race condition. In this more modular model that we are talking about right now (which I think is pretty good) we could run into race conditions. When you were invoking validate yourself there was no race condition unless you referenced the struct as a pointer.

m.Post("/form", binding.Form(&BlogPost{}), ...) // the *BlogPost will be pointing to the same struct on every request! big ohnoes

That issue might be worth outlining in the documentation as it will still be possible to do.

mholt · 2013-12-03T05:14:42Z

Oh! I see your point about the pointer to the struct. The handler is only set up once (m.Post, or whatever, is only actually executed once...) thus the instantiation of a new struct only happens once and the pointer is to the same one for each request. I guess doing an in-memory copy is OK; a request wouldn't be too huge, right? Yes, that should be documented.

I also see now what you mean about adding the Validate middleware to the stack manually instead of building it in. If we do this refactor, then yes, let's rename Validate to... uh, I dunno... letsee, it responds with a certain status code and payload and then stops the request in its tracks... what do you call something like that? 😕

I'm just trying to bury as much boilerplate as possible away from the actual, final handler. I'd personally love not to have to specify that so-called-"Validate" middleware for each m.Post/m.Get/etc...

What if the "Validate" middleware executed a (optional) user-defined function for dealing with requests that don't validate (have errors)? That was kind of the idea behind providing the Validator interface, but maybe it could also provide a ErrorResponder interface or something where the user-defined ErrorResponse func is called by default, if one exists, if there are errors... follow?

codegangsta · 2013-12-03T05:32:19Z

I see where you are coming from. I think the main hangup for me is the error handling automatically being done for me. If he response is written in the bind middleware then there is no way for me to specify my own custom response body. I think it is good to remove the boilerplate with having a good default error handler but I have a spidey sense that it should be optional.

What if it was named binding.ErrorHandler() ?

Sent from my iPhone

On Dec 2, 2013, at 9:14 PM, Matt notifications@github.com wrote:

Oh! I see your point about the pointer to the struct. The handler is only set up once (m.Post, or whatever, is only actually executed once...) thus the instantiation of a new struct only happens once and the pointer is to the same one for each request. I guess doing an in-memory copy is OK; a request wouldn't be too huge, right? Yes, that should be documented.

I also see now what you mean about adding the Validate middleware to the stack manually instead of building it in. If we do this refactor, then yes, let's rename Validate to... uh, I dunno... letsee, it responds with a certain status code and payload and then stops the request in its tracks... what do you call something like that?

I'm just trying to bury as much boilerplate as possible away from the actual, final handler. I'd personally love not to have to specify that so-called-"Validate" middleware for each m.Post/m.Get/etc...

What if the "Validate" middleware executed a (optional) user-defined function for dealing with requests that don't validate (have errors)? That was kind of the idea behind providing the Validator interface, but maybe it could also provide a ErrorResponder interface or something where the user-defined ErrorResponse func is called by default, if one exists, if there are errors... follow?

—
Reply to this email directly or view it on GitHub.

codegangsta · 2013-12-03T05:34:35Z

My thought is that all the Validator would do is get the binding.Errors type injected and write a response error if any exist. Simple? Yes, but it is effective and consistent. And it also is easily swappable.

Sent from my iPhone

On Dec 2, 2013, at 9:14 PM, Matt notifications@github.com wrote:

Oh! I see your point about the pointer to the struct. The handler is only set up once (m.Post, or whatever, is only actually executed once...) thus the instantiation of a new struct only happens once and the pointer is to the same one for each request. I guess doing an in-memory copy is OK; a request wouldn't be too huge, right? Yes, that should be documented.

I also see now what you mean about adding the Validate middleware to the stack manually instead of building it in. If we do this refactor, then yes, let's rename Validate to... uh, I dunno... letsee, it responds with a certain status code and payload and then stops the request in its tracks... what do you call something like that?

I'm just trying to bury as much boilerplate as possible away from the actual, final handler. I'd personally love not to have to specify that so-called-"Validate" middleware for each m.Post/m.Get/etc...

What if the "Validate" middleware executed a (optional) user-defined function for dealing with requests that don't validate (have errors)? That was kind of the idea behind providing the Validator interface, but maybe it could also provide a ErrorResponder interface or something where the user-defined ErrorResponse func is called by default, if one exists, if there are errors... follow?

—
Reply to this email directly or view it on GitHub.

codegangsta · 2013-12-03T05:47:20Z

Now thinking about it. binding.Bind can still write an error. binding.Bind is really a composition of the smaller pieces of the package. I just wouldn't want response control flow restricting me from using binding.JSON or binding.Form

Thanks for discussing this. I know we are both wanting to nail this functinality.

Sent from my iPhone

On Dec 2, 2013, at 9:14 PM, Matt notifications@github.com wrote:

Oh! I see your point about the pointer to the struct. The handler is only set up once (m.Post, or whatever, is only actually executed once...) thus the instantiation of a new struct only happens once and the pointer is to the same one for each request. I guess doing an in-memory copy is OK; a request wouldn't be too huge, right? Yes, that should be documented.

I also see now what you mean about adding the Validate middleware to the stack manually instead of building it in. If we do this refactor, then yes, let's rename Validate to... uh, I dunno... letsee, it responds with a certain status code and payload and then stops the request in its tracks... what do you call something like that?

I'm just trying to bury as much boilerplate as possible away from the actual, final handler. I'd personally love not to have to specify that so-called-"Validate" middleware for each m.Post/m.Get/etc...

What if the "Validate" middleware executed a (optional) user-defined function for dealing with requests that don't validate (have errors)? That was kind of the idea behind providing the Validator interface, but maybe it could also provide a ErrorResponder interface or something where the user-defined ErrorResponse func is called by default, if one exists, if there are errors... follow?

—
Reply to this email directly or view it on GitHub.

mholt · 2013-12-03T06:26:41Z

Ah, yes, I keep forgetting that middleware is swappable. (Duh.) And if Bind is kind of a "catch-all" middleware that does all this boilerplate for me as discussed, I'll be happy at least. And if you want more control, use Form or Json and perform error handling yourself, too, or push the default ErrorHandler middleware onto the stack. (What if we called it binding.Checkpoint()? Is that too nebulous? I'm good with either; it's just another thought.)

Thanks for your patience as we iterate on these ideas. I'll try to work on this more tomorrow.

mholt · 2013-12-03T17:16:37Z

Woah, okay, I just looked at the whole thing from a fresh set of eyes this morning. I wonder if both our desires are already being satisfied, and I think some errors in my description from my original post are root of the confusion:

Bind is just a composition of the other parts of the package, including responding with an error code (yay for no boilerplate!)
Form and Json only deserialize and validate (map errors), they do not write a response if there are errors. That would still be up to the application. (My initial post was confusing, I now realize, because I said they perform no validation. They do actually, because they invoke the Validate middleware. But they do not check for errors and respond with 400 if len(errors) > 0. Only Bind does that.)
Validate stays the same. It simply performs validation and maps errors; it does not write a response. That could still be up to you.
If we want to do so, these lines could still be moved to their own middleware, ErrorHandler, if you wanted to do something like:

m.Post("/form", binding.Form(BlogPost{}), binding.ErrorHandler(), func(bp BlogPost) {
   // ...
})

Or you could write your own middleware handler to respond to errors, or you could do it in your application, or whatever you want.

Using Bind, which kind of wraps up all the others, you'd do only this (but you're right, you have less control):

m.Post("/form", binding.Bind(BlogPost{}), func(bp BlogPost) {
   // ...
})

Does that clear things up?

codegangsta · 2013-12-03T17:36:11Z

Hah wow. That sounds great! So just to clarify, everything is staying the
same except we will have a new handler called binding.ErrorHandler which
will write to the response if binding.Errors contains any errors.

Sounds great to me!

On Tue, Dec 3, 2013 at 9:16 AM, Matt notifications@github.com wrote:

Woah, okay, I just looked at the whole thing from a fresh set of eyes this
morning. I wonder if both our desires are already being satisfied, and I
think some errors in my description from my original post are root of the
confusion:

Bind is just a composition of the other parts of the package,
including responding with an error code (yay for no boilerplate!)

Form and Json only deserialize and validate (map errors), they do
not write a response if there are errors. That would still be up to the
application. (My initial post was confusing, I now realize, because I said
they perform no validation. They do actually, because they invoke the
Validate middleware. But they do not check for errors and respond with 400
if len(errors) > 0. Only Bind does that.)

Validate stays the same. It simply performs validation and maps
errors; it does not write a response. That could still be up to you.

If we want to do so, these lineshttps://github.com/smartystreets/martini-contrib/blob/ff9d2619e19436f4db169dfbf0dbb31b82559e6d/binding/binding.go#L36-L43could still be moved to their own middleware, ErrorHandler, if you wanted
to do something like:

m.Post("/form", binding.Form(BlogPost{}), binding.ErrorHandler(), func(bp BlogPost) {
// ...})

Or you could write your own middleware handler to respond to errors, or
you could do it in your application, or whatever you want.

Does that clear things up?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/34#issuecomment-29730292
.

mholt · 2013-12-03T17:42:07Z

Yup! I'll do that and add godocs. And if anything needs to change after the PR is merged, that's fine too.

mholt · 2013-12-03T19:59:40Z

@codegangsta Bam. See what you think.

There's still no README, but I can throw that together soon enough if you'd like me to do that.

codegangsta · 2013-12-03T20:00:52Z

Yeah. throw together a readme. People seem to like them for getting
started. I will look at this tonight!

On Tue, Dec 3, 2013 at 11:59 AM, Matt notifications@github.com wrote:

@codegangsta https://github.com/codegangsta Bam. See what you think.

There's still no README, but I can throw that together soon enough if
you'd like me to do that.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/34#issuecomment-29745511
.

codegangsta · 2013-12-04T02:16:19Z

aaaand merged.

Seriously awesome work! @mholt @mdwhatcott I'm adding you two as collaborators so you can update binding without PRs

luxifer · 2013-12-05T14:42:47Z

Hi guys. I just wanted to update the vendors I use and wait what ? the form middleware disappear in favor of this one. The fact is, if you red the go guidelines carefully, it says "never break backward compatibility". If you want to do it, create a new package.

So thanks for this middleware which support more functionality than the form one. But for the future, please never breack BC...

codegangsta · 2013-12-05T14:48:01Z

martini and martini-contrib are less than a month old. The plan is to use
semantic versioning for these packages http://semver.org/

Truth is, stuff is going to break before we stamp it as 1.0. We are all in
the early adopter phase right now, so we cannot expect things to not change.

That being said, I am conscious of API compatibility and will not break
things unless I really think it necessary.

On Thu, Dec 5, 2013 at 6:42 AM, Florent VIEL notifications@github.comwrote:

Hi guys. I just wanted to update the vendors I use and wait what ? the
form middleware disappear in favor of this one. The fact is, if you red the
go guidelines carefully, it says "never break backward compatibility". If
you want to do it, create a new package.

So thanks for this middleware which support more functionality than the
form one. But for the future, please never breack BC...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/34#issuecomment-29902500
.

luxifer · 2013-12-05T15:29:02Z

I understant martini is a new project so that things are not fixed. But how would you use martini and martini-contrib with semantic versioning because the go get command does not permit to specify branch, tag or commit sha1 ?

codegangsta · 2013-12-05T15:58:47Z

There is tooling built around managing dependencies with specific versions. Godep comes to mind. Thankfully for the majority of use cases we shouldn't run into any broken promises with the api until Martini hits 2.0.

Right now it's the pre 1.0, Wild West era of martini. Once we hit 1.0 we will be making compatibility promises until 2.0 in which I am banking some hope that dependency management in Go will be a lot better by that time.

Sent from my iPhone

On Dec 5, 2013, at 7:29 AM, Florent VIEL notifications@github.com wrote:

I understant martini is a new project so that things are not fixed. But how would you use martini and martini-contrib with semantic versioning because the go get command does not permit to specify branch, tag or commit sha1 ?

—
Reply to this email directly or view it on GitHub.

luxifer · 2013-12-05T16:02:59Z

With go you don't have to care about dependency management because you don't have to break BC.
I think for Martini2 it will be better to create a martini2 repository.

Further reading here http://www.stovepipestudios.com/blog/2013/02/go-dependency-management.html

codegangsta · 2013-12-05T16:33:53Z

Yup. Either a martini2 repository or a separate branch.

I personally like what mgo does. They have their own endpoint for different
versions:

labix.org/v2/mgo

On Thu, Dec 5, 2013 at 8:03 AM, Florent VIEL notifications@github.comwrote:

With go you don't have to care about dependency management because you
don't have to break BC.
I think for Martini2 it will be better to create a martini2 repository.

Further reading here
http://www.stovepipestudios.com/blog/2013/02/go-dependency-management.html

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/34#issuecomment-29910158
.

luxifer · 2013-12-05T21:20:23Z

My point is not to be angry against you.
I really like your work on martini.
I just want the best decisions on this framework.
By the way I also like the way mgo version its package.

codegangsta · 2013-12-05T21:21:47Z

No offense taken. :) Martini may never have a version 2. I also have no
idea how the Go community will look as far as dependency management goes in
the future. I am prepared for anything at this point.

On Thu, Dec 5, 2013 at 1:20 PM, Florent VIEL notifications@github.comwrote:

My point is not to be angry against you.
I really like your work on martini.
I just the best decisions on this framework.
By the way I also like the way mgo version its package.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/34#issuecomment-29939187
.

mdwhatcott and others added 13 commits November 19, 2013 16:44

Implemented JSON payload binder.

ecae500

XML would probably be a piece of cake…

37 new lines of panicking code

42f32bd

Passing test

140f4b0

Validate calls a method “Validate<StructName>” to perform validation

8783aa5

Started pipeline; code doesn’t compile

7a5c667

Changed the validation package to use an interface instead of reflection

4e7a23a

First passing tests of new binding package

eedda7d

Major refactoring of errors; lots of fixes; more tests; tests pass.

b2e1481

A request without form data won’t error; when we have to guess, guess…

376538c

… JSON first.

Demystifying the “required” attribute…

b097c99

Major usability fixes; could use some optimizing now probably

2fd1661

Deleted form package; new binding package ready for review

e932e46

Form deserializer now handles bool, string, int types, and float type…

ff9d261

…s (related to #32)

Godoc; new standalone ErrorHandler middleware.

50eafe0

Added readme documentation.

e94eb9b

codegangsta merged commit e94eb9b into codegangsta:master Dec 4, 2013

yosisa mentioned this pull request May 1, 2014

Map a pointer of a struct to an interface martini-contrib/binding#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New "binding" package replaces "Form" middleware #34

New "binding" package replaces "Form" middleware #34

mholt commented Dec 2, 2013

codegangsta commented Dec 2, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 4, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

New "binding" package replaces "Form" middleware #34

New "binding" package replaces "Form" middleware #34

Conversation

mholt commented Dec 2, 2013

Bind

Form

JSON

Validate

codegangsta commented Dec 2, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

mholt commented Dec 3, 2013

mholt commented Dec 3, 2013

codegangsta commented Dec 3, 2013

codegangsta commented Dec 4, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013

luxifer commented Dec 5, 2013

codegangsta commented Dec 5, 2013