Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1402543003:
Add initial middleware package. (Closed)
|
Created:
5 years, 2 months ago by iannucci Modified:
5 years, 2 months ago CC:
andrew.wang, chromium-reviews, infra-reviews+luci-go_chromium.org, M-A Ruel, tandrii(chromium), todd Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-go@master Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
DescriptionAdd initial middleware package.
We can have compatible middleware functions in other packages, but this will be
a place to put generally applicable middlewares.
R=dnj@chromium.org, estaab@chromium.org, maruel@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=
Committed: https://github.com/luci/luci-go/commit/b86a389f3d25326e71e27dc7e801df56b828e372
Patch Set 1 #Patch Set 2 : swap args to prevent ugly chains #
Total comments: 18
Patch Set 3 : fixit #Patch Set 4 : comments #Patch Set 5 : nits #
Dependent Patchsets: Messages
Total messages: 19 (2 generated)
lgtm with nits https://codereview.chromium.org/1402543003/diff/20001/appengine/middleware/ap... File appengine/middleware/appengine.go (right): https://codereview.chromium.org/1402543003/diff/20001/appengine/middleware/ap... appengine/middleware/appengine.go:19: if r.Header.Get("X-Appengine-Cron") != "true" { consider adding "&& !IsDevAppServer()" it simplifies local manual testing https://codereview.chromium.org/1402543003/diff/20001/appengine/middleware/co... File appengine/middleware/context.go (right): https://codereview.chromium.org/1402543003/diff/20001/appengine/middleware/co... appengine/middleware/context.go:19: // same has httprouder.Handle, except that it also has a context parameter. typo: same as?
https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/appengine.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:19: if r.Header.Get("X-Appengine-Cron") != "true" { On 2015/10/13 at 01:29:14, Vadim Sh. wrote: > consider adding "&& !IsDevAppServer()" > > it simplifies local manual testing WDYT about just adding this header in BaseTest? https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/context.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/context.go:19: // same has httprouder.Handle, except that it also has a context parameter. On 2015/10/13 at 01:29:14, Vadim Sh. wrote: > typo: same as? yup
https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/appengine.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:19: if r.Header.Get("X-Appengine-Cron") != "true" { On 2015/10/13 01:37:17, iannucci wrote: > On 2015/10/13 at 01:29:14, Vadim Sh. wrote: > > consider adding "&& !IsDevAppServer()" > > > > it simplifies local manual testing > > WDYT about just adding this header in BaseTest? What do you mean? By "local manual testing" I meant when running an app on devserver for real (not in unit tests). BaseProd is used in that case. There's no UI on devserver to trigger cron jobs. And doing GET's with headers is trickier than just hitting an URL in browser.
On 2015/10/13 at 01:40:26, vadimsh wrote: > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > File appengine/middleware/appengine.go (right): > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > appengine/middleware/appengine.go:19: if r.Header.Get("X-Appengine-Cron") != "true" { > On 2015/10/13 01:37:17, iannucci wrote: > > On 2015/10/13 at 01:29:14, Vadim Sh. wrote: > > > consider adding "&& !IsDevAppServer()" > > > > > > it simplifies local manual testing > > > > WDYT about just adding this header in BaseTest? > > What do you mean? > > By "local manual testing" I meant when running an app on devserver for real (not in unit tests). BaseProd is used in that case. There's no UI on devserver to trigger cron jobs. And doing GET's with headers is trickier than just hitting an URL in browser. Ah, got it. Yeah, OK. Same bypass for taskqueue check?
FTR: I don't like the 4-argument case. IMO at least the params should be wrapped in a Context. Explicit is well and good, but users are explicitly aware that they're using the middleware package. Just MHO :) https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/appengine.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:21: fmt.Fprint(rw, "error: must be run from cron") Worth logging, too. https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:37: if qName == "" || (queue != "" && queue != qName) { Why have "" be special? IMO should be simple: if queue != r.Header.Get("X-AppEngine-QueueName") {...} https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:42: fmt.Fprintf(rw, "error: must be run from the taskqueue %q", queue) This could leak internal task queue names. Maybe log the task queue name instead. https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/context.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/context.go:24: func Base(h Handler) httprouter.Handle { Make this take context.Context. context.Background is easy enough to supply, and this lets a user define their own prod/etc. stack.
On 2015/10/13 01:41:38, iannucci wrote: > On 2015/10/13 at 01:40:26, vadimsh wrote: > > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > > File appengine/middleware/appengine.go (right): > > > > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > > appengine/middleware/appengine.go:19: if r.Header.Get("X-Appengine-Cron") != > "true" { > > On 2015/10/13 01:37:17, iannucci wrote: > > > On 2015/10/13 at 01:29:14, Vadim Sh. wrote: > > > > consider adding "&& !IsDevAppServer()" > > > > > > > > it simplifies local manual testing > > > > > > WDYT about just adding this header in BaseTest? > > > > What do you mean? > > > > By "local manual testing" I meant when running an app on devserver for real > (not in unit tests). BaseProd is used in that case. There's no UI on devserver > to trigger cron jobs. And doing GET's with headers is trickier than just hitting > an URL in browser. > > Ah, got it. Yeah, OK. Same bypass for taskqueue check? For task queues it is less helpful: 1) Queued tasks usually have payload with state. 2) They must be produced by something. 3) There's UI to launch tasks. Actually, there's UI to run cron jobs too... I remember having issues with it... Whatever, as you prefer.
PTAL
On 2015/10/13 at 01:44:49, dnj wrote: > FTR: I don't like the 4-argument case. IMO at least the params should be wrapped in a Context. Explicit is well and good, but users are explicitly aware that they're using the middleware package. Just MHO :) > Yeah, I tried that, but I think I like the 4-clause one better since it's explicit. Putting everything into the context felt weird, since it introduced a bunch of new potential error cases (what if ResponseWriter is not set? Can I call it with any context anywhere?), and basically required every middleware to just pull everything back out anyway (at minimum they need the request and the responsewriter. params is debatable, but I have some app-specific middleware in mind which would use it (e.g. VerifyAndLoadAttempt type of things)). > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > File appengine/middleware/appengine.go (right): > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > appengine/middleware/appengine.go:21: fmt.Fprint(rw, "error: must be run from cron") > Worth logging, too. > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > appengine/middleware/appengine.go:37: if qName == "" || (queue != "" && queue != qName) { > Why have "" be special? IMO should be simple: > if queue != r.Header.Get("X-AppEngine-QueueName") {...} > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > appengine/middleware/appengine.go:42: fmt.Fprintf(rw, "error: must be run from the taskqueue %q", queue) > This could leak internal task queue names. Maybe log the task queue name instead. > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > File appengine/middleware/context.go (right): > > https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... > appengine/middleware/context.go:24: func Base(h Handler) httprouter.Handle { > Make this take context.Context. context.Background is easy enough to supply, and this lets a user define their own prod/etc. stack.
https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/appengine.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:21: fmt.Fprint(rw, "error: must be run from cron") On 2015/10/13 at 01:44:49, dnj wrote: > Worth logging, too. Ah, right, meant to do that. Done. https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:37: if qName == "" || (queue != "" && queue != qName) { On 2015/10/13 at 01:44:49, dnj wrote: > Why have "" be special? IMO should be simple: > if queue != r.Header.Get("X-AppEngine-QueueName") {...} Docs explain it pretty well I think? https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:42: fmt.Fprintf(rw, "error: must be run from the taskqueue %q", queue) On 2015/10/13 at 01:44:49, dnj wrote: > This could leak internal task queue names. Maybe log the task queue name instead. done https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/context.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/context.go:24: func Base(h Handler) httprouter.Handle { On 2015/10/13 at 01:44:49, dnj wrote: > Make this take context.Context. context.Background is easy enough to supply, and this lets a user define their own prod/etc. stack. I think you misunderstand the point of this. This is to adapt middleware style functions to be accepted by httprouter.
Dan, did you have more comments?
lgtm w/ nits/explanation. https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/appengine.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/appengine.go:37: if qName == "" || (queue != "" && queue != qName) { On 2015/10/13 01:56:34, iannucci wrote: > On 2015/10/13 at 01:44:49, dnj wrote: > > Why have "" be special? IMO should be simple: > > if queue != r.Header.Get("X-AppEngine-QueueName") {...} > > Docs explain it pretty well I think? Ah okay, makes sense. Sorry was on a bus. https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/context.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/context.go:24: func Base(h Handler) httprouter.Handle { On 2015/10/13 01:56:34, iannucci wrote: > On 2015/10/13 at 01:44:49, dnj wrote: > > Make this take context.Context. context.Background is easy enough to supply, > and this lets a user define their own prod/etc. stack. > > I think you misunderstand the point of this. This is to adapt middleware style > functions to be accepted by httprouter. Context generation shouldn't have to start at the middleware layer. I would much prefer: router.GET(middleware.Base(context.Background(), h) For example, maybe I want to do: ctx := log.SetField(context.Background(), "apiVersion", "1") router.GET(middleware.BaseProd(ctx, ...)) The setup right now basically says, "if you use middleware, you must use more middleware handlers to generate the actual context." That's fine, I guess; I just prefer the option of supplying my own. https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/doc.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/doc.go:17: // import ( WDYT about moving this into a test example?
https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/context.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/context.go:24: func Base(h Handler) httprouter.Handle { On 2015/10/13 at 04:39:10, dnj wrote: > On 2015/10/13 01:56:34, iannucci wrote: > > On 2015/10/13 at 01:44:49, dnj wrote: > > > Make this take context.Context. context.Background is easy enough to supply, > > and this lets a user define their own prod/etc. stack. > > > > I think you misunderstand the point of this. This is to adapt middleware style > > functions to be accepted by httprouter. > > Context generation shouldn't have to start at the middleware layer. I would much prefer: > router.GET(middleware.Base(context.Background(), h) > > For example, maybe I want to do: > ctx := log.SetField(context.Background(), "apiVersion", "1") > router.GET(middleware.BaseProd(ctx, ...)) > > The setup right now basically says, "if you use middleware, you must use more middleware handlers to generate the actual context." That's fine, I guess; I just prefer the option of supplying my own. Ah, ah, ok... I misunderstood you. The amount of context construction you can do in init() is extremely limited though. Note that you could also do: myBase := func(h middleware.Handler) httprouter.Handle { return middleware.Base(func(c, rw, req, p) { // add lots of default stuff to c, but realistically using req and p h(c, rw, req, p) }) } I'm not sure what the benefit of doing it at the Base layer is, except that it forces the majority users (who will not want to put init()-static stuff in the context) to have an extra empty parameter (context.Background())? One thing that COULD be useful would be that you could add a memory logger to the context... but then if you use BaseTest, it would be overwritten because context is dumb :) (I guess we could add UseIfNotSet methods to all the service packages... ugh). This is solved pretty easily in the tests, however, by just capturing it in a middleware closure though :) https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... File appengine/middleware/doc.go (right): https://chromiumcodereview.appspot.com/1402543003/diff/20001/appengine/middle... appengine/middleware/doc.go:17: // import ( On 2015/10/13 at 04:39:10, dnj wrote: > WDYT about moving this into a test example? I don't think it would work? Is there a way to do an example with data types and init functions?
> > WDYT about moving this into a test example? > > I don't think it would work? Is there a way to do an example with data types and > init functions? Data types sure, init functions I think not. However, middleware example doesn't have to use init(), nor does it need to have test output. It can just be an example function for the purpose of generating the dox. If you want to make a working example, you can always play a client/server interaction via https://golang.org/pkg/net/http/httptest/ Or just leave it in the docstring *shrug*
On 2015/10/13 at 04:57:58, dnj wrote: > > > WDYT about moving this into a test example? > > > > I don't think it would work? Is there a way to do an example with data types and > > init functions? > > Data types sure, init functions I think not. However, middleware example doesn't have to use init(), nor does it need to have test output. It can just be an example function for the purpose of generating the dox. > > If you want to make a working example, you can always play a client/server interaction via https://golang.org/pkg/net/http/httptest/ > > Or just leave it in the docstring *shrug* /me opts for docstring for now... we may rip this out for a megamicroframework or some such anyway.
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1402543003/#ps80001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1402543003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1402543003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/luci/luci-go/commit/b86a389f3d25326e71e27dc7e801df56b828e372 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
