Make HTTP middleware easier to use

This adds a new router package that wraps around the previously used router (julienschmidt/httprouter). The new package adds support to register HTTP middleware linearly via the Use function or when registering handlers. The new package also allows creating subrouters that carry middleware over from the router they are derived from.

The source code and tests are updated to use the new router package. Noteworthy points from the refactoring process:
 * InstallHandlers accept router.MiddlewareChain as argument instead of the previous middleware.Base.
 * auth.Autologin middleware is no longer coupled with admin.adminOnly.
 * gaetesting.BaseTest has been removed. Use gaetesting.TestingContext directly instead.

BUG=617774
Committed: https://github.com/luci/luci-go/commit/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95

[nishanths, 2016-06-09 04:17:54]

Description was changed from

==========
router: Improve type name


Add comments

Also, add a few convenience functions to middleware and router.

Add license header to new router.go file


Add router package

Also, rejig middleware chain types

BUG=

middleware: Add chained types
==========

to

==========
Make HTTP middleware easier to use.

Also, introduce new router package.

BUG=617774
==========

[nishanths, 2016-06-09 04:17:54]

nishanths@google.com changed reviewers:
+ estaab@chromium.org, iannucci@chromium.org, vadim@chromium.org

[nishanths, 2016-06-09 04:21:22]

On 2016/06/09 04:17:54, nishanths1 wrote:
> mailto:nishanths@google.com changed reviewers:
> + mailto:estaab@chromium.org, mailto:iannucci@chromium.org,
mailto:vadim@chromium.org

Thoughts so far? 
Will add tests and begin editing existing code to use the new middleware API
soon.

[nishanths, 2016-06-09 04:23:48]

https://codereview.chromium.org/2043423004/diff/1/server/middleware/middlewar...
File server/middleware/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/1/server/middleware/middlewar...
server/middleware/middleware.go:22: type ChainedHandler func(context.Context,
http.ResponseWriter, *http.Request, httprouter.Params, Handler)
ChainedHandler can eventually be renamed to Handler and the previous Handler
type can be removed, if necessary.

[iannucci, 2016-06-09 21:30:08]

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
File server/middleware/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
server/middleware/middleware.go:22: type ChainedHandler func(context.Context,
http.ResponseWriter, *http.Request, httprouter.Params, Handler)
Since we have all the middleware in this repo, it would probably be best to just
atomically change everything once we converge on the design particulars I think
:)

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
server/middleware/middleware.go:32: }
Why not just a []Handler? I think it could simplify basically all of the
functions you have below :-)

[Vadim Sh., 2016-06-09 21:34:48]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org
- vadim@chromium.org

[Vadim Sh., 2016-06-09 21:45:12]

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
File server/middleware/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
server/middleware/middleware.go:22: type ChainedHandler func(context.Context,
http.ResponseWriter, *http.Request, httprouter.Params, Handler)
On 2016/06/09 21:30:08, iannucci wrote:
> Since we have all the middleware in this repo, it would probably be best to
just
> atomically change everything once we converge on the design particulars I
think
> :)

+1

Having both Handler and ChainedHandler is confusing.

While at it... Maybe hide r, rw, p, and next inside single struct? Robbie, what
do you think?

type Context {
  context.Context

  Response http.ResponseWriter
  Request *http.Request
  Params httprouter.Params

  Next Handler
}

type Handler func(Context)

or maybe keep Next outside the context, since it's sort of changing all the time
(not really a stable during execution of the request, like other stuff in the
context).

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
server/middleware/middleware.go:32: }
On 2016/06/09 21:30:08, iannucci wrote:
> Why not just a []Handler? I think it could simplify basically all of the
> functions you have below :-)

+1

since chains are append-only, it is even fine to reuse underlying array for
storage

[nishanths, 2016-06-13 17:30:29]

I've added new Patch Sets. Here's a description on the contents.

Patch Set 1-5: New router API. 
Patch Set 6: Update source code to use the new router API.
Patch Set 7: Update tests

What's left to do:

- Update documentation
- Write tests for the router package
- Remove (now unused) server/router/middleware.go file
- Internal code still uses old router
(https://chrome-internal.googlesource.com/infra/infra_internal/+/master/go/src...)

Thoughts?

[nishanths, 2016-06-13 17:30:44]

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
File server/middleware/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
server/middleware/middleware.go:22: type ChainedHandler func(context.Context,
http.ResponseWriter, *http.Request, httprouter.Params, Handler)
On 2016/06/09 21:45:12, Vadim Sh. wrote:
> On 2016/06/09 21:30:08, iannucci wrote:
> > Since we have all the middleware in this repo, it would probably be best to
> just
> > atomically change everything once we converge on the design particulars I
> think
> > :)
> 
> +1
> 
> Having both Handler and ChainedHandler is confusing.
> 
> While at it... Maybe hide r, rw, p, and next inside single struct? Robbie,
what
> do you think?
> 
> type Context {
>   context.Context
> 
>   Response http.ResponseWriter
>   Request *http.Request
>   Params httprouter.Params
> 
>   Next Handler
> }
> 
> type Handler func(Context)
> 
> or maybe keep Next outside the context, since it's sort of changing all the
time
> (not really a stable during execution of the request, like other stuff in the
> context).

I like this too. I've changed the router package to use the Context-style Vadim
describes with a few changes. Next is provided as a method on *Context. Instead,
the struct contains a field for the list of handlers.

https://codereview.chromium.org/2043423004/diff/40001/server/middleware/middl...
server/middleware/middleware.go:32: }
On 2016/06/09 21:45:12, Vadim Sh. wrote:
> On 2016/06/09 21:30:08, iannucci wrote:
> > Why not just a []Handler? I think it could simplify basically all of the
> > functions you have below :-)
> 
> +1
> 
> since chains are append-only, it is even fine to reuse underlying array for
> storage

Good point. The API primariy now uses router.Handler and []router.Handler.

[hinoka, 2016-06-13 18:31:20]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org

[hinoka, 2016-06-13 18:31:21]

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/set...
File appengine/cmd/milo/settings/themes.go (left):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/set...
appengine/cmd/milo/settings/themes.go:191: return Base(hx)
Don't we still need to install the base middlewares?

[dnj (Google), 2016-06-13 18:50:41]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2016-06-13 18:50:42]

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:19: // the router, a list of handlers, and
configuration.
nit: Note that Router shouldn't be instantiated by users, and that they should
use New() instead.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:44: _ http.Handler = New()
Use:
var _ http.Handler = (*Router)(nil)

rather than call New()

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:67: func (r *Router) Group(relativePath string) *Router
{
We should probably clean leading/trailing "/" from relativePath, and explicitly
add a "/" when generating the next BasePath.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:132: merged := make([]Handler, totalLen)
nit: probably cleaner to just allocate size 0, capacity "totalLen" and have two
normal "append(merged, m...)". Also just use "len(m) + len(n)" directly in the
allocation, no need to store it in its own variable.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:147: handlers: copyHandlers(r.handlers, handlers),
Do we really need to allocate/copy handlers each time we handle an HTTP call? It
would be better to just store "adaptHandlers", and resolve them to an index
during execution (e.g.,

handlers := r.handlers
if i >= len(handlers) {
  handlers, i = r.adaptHandlers, (i - len(handlers))
}

... in Next.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:155: // handler, the next registered handler begins
execution  immdediately.
nit: two spaces.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:166: c.aborted = true
Just thinking here, but maybe we want "Abort()" to abort the current handler? We
could do that via panic(), then catch at middleware root and handle.

[iannucci, 2016-06-13 19:23:29]

generally lg, but I'm increasingly uneasy about Abort. It feels like the handler
that calls Abort does so without actually knowing what might come next (and we
have some applications for middleware like monitoring which would definitely
want to run even when the request 'fails').

I also think this may break paniccatcher?

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/fro...
File appengine/cmd/cron/frontend/handler.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/fro...
appengine/cmd/cron/frontend/handler.go:138: h(&requestContext{c.Context,
c.Writer, c.Request, c.Params})
hm... at this point, can't we just use router.Handler directly instead of having
cron's `handler` type? cron's *requestContext looks almost exactly like
*router.Context now.

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/fro...
appengine/cmd/cron/frontend/handler.go:214: router.GET("/_ah/warmup",
append(base(), wrap(warmupHandler))...)
er... can't this be

  router.GET("/_ah/warmup", base(), wrap(warmupHandler))

? It looks like you're doing an append and then unwrapping the result
immediately.

edit: Oh, it looks like router.GET and friends take ...router.Handler. Why don't
we make this take ...[]router.Handler instead?

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/ui/...
File appengine/cmd/cron/ui/common.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/ui/...
appengine/cmd/cron/ui/common.go:46: r.GET("/jobs/:ProjectID/:JobID/:InvID",
append(wrap(), invocationPage)...)
same here?

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/ui/...
File appengine/cmd/cron/ui/index.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/ui/...
appengine/cmd/cron/ui/index.go:14: jobs, err :=
config(c.Context).Engine.GetAllCronJobs(c.Context)
wdyt about making router.Context embed context.Context anonymously? e.g.

type Context struct {
  context.Context
  Writer http.ResponseWriter
  Request *http.Request
  Params httprouter.Params
}

Then you can just pass c here.

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/ui/...
File appengine/cmd/cron/ui/invocation.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/cron/ui/...
appengine/cmd/cron/ui/invocation.go:27: c.Abort()
probably-unpopular-idea: instead of adding a new method to indicate that the
chain is aborted, why not have the Context's ResponseWriter be a tiny wrapper
that looks for calls to WriteHeader? That way this sort of code wouldn't need to
change, and all existing mechanisms for terminating the http request (e.g.
writing an status code) continue do the right thing?

That would also let us capture the actually-sent status code for e.g.
monitoring.

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/bui...
File appengine/cmd/milo/buildbot/pubsub.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/bui...
appengine/cmd/milo/buildbot/pubsub.go:218: h.WriteHeader(200)
missed one?

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/bui...
File appengine/cmd/milo/buildbot/pubsub_test.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/bui...
appengine/cmd/milo/buildbot/pubsub_test.go:170:
PubSubHandler(&router.Context{Context: c, Writer: h, Request: r, Params: p})
(I know you didn't write this :)), but it probably would be (a lot) cleaner to
have a helper function that prepares this, since it's copy-paste in every test
case.

https://codereview.chromium.org/2043423004/diff/120001/appengine/tumble/tumbl...
File appengine/tumble/tumbletest.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/tumble/tumbl...
appengine/tumble/tumbletest.go:152: Params: nil,
can omit nil-initializers for structs.

https://codereview.chromium.org/2043423004/diff/120001/server/middleware/pani...
File server/middleware/paniccatcher.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/middleware/pani...
server/middleware/paniccatcher.go:28: }
does this still work? I don't think this works any more...

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:44: _ http.Handler = New()
Do:

  _ http.Handler = (*Router)(nil)

As it will avoid the function call at runtime and just be statically checked by
the compiler.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:161: }
Is the intended usecase of this to allow a middleware that 'wraps' the entire
rest of the chain? If so, I would add a comment in the docstring to that effect:

"This is useful if you need to execute some code before+after the rest of the
chain. Most middleware implementations will not call this."

or something.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:167: }
Yeah, I'm not convinced this is a good/safe api :/. I think capturing + exposing
the status code would be much better, as it allows existing handler code to be
used unmodified. For example, right now if we had a monitoring middleware that
came after the main handler, any calls to Abort would just point-blank skip that
processing.

If we capture the status code and expose it as c.StatusCode (or something), then
middlewares could make a choice about whether or not they want to execute anyway
(e.g. if c.StatusCode != 0 { return }).

[nodir, 2016-06-13 20:20:51]

nodir@chromium.org changed reviewers:
+ nodir@chromium.org

[nodir, 2016-06-13 20:20:52]

https://codereview.chromium.org/2043423004/diff/120001/server/router/doc.go
File server/router/doc.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/doc.go#n...
server/router/doc.go:5: /*
I think we use // everywhere

https://codereview.chromium.org/2043423004/diff/120001/server/router/doc.go#n...
server/router/doc.go:6: Package router provides a HTTP router with support for
middleware and groups.
s/a HTTP/an HTTP/

https://codereview.chromium.org/2043423004/diff/120001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/handler....
server/router/handler.go:18: Middleware func() Handler
I think this type is unnecessary

https://codereview.chromium.org/2043423004/diff/120001/server/router/handler....
server/router/handler.go:22: // TODO(nishanths): It may not be necessary to have
a specific type for
"//\n" before TODO

https://codereview.chromium.org/2043423004/diff/120001/server/router/handler....
server/router/handler.go:24: Base func() []Handler
why Base is a function that returns []Handler rather than []Handler? This
function does not accept arguments, so either it returns same handlers all the
time, or it depends on global state (which is bad)

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:166: c.aborted = true
On 2016/06/13 18:50:42, dnj (Google) wrote:
> Just thinking here, but maybe we want "Abort()" to abort the current handler?
We
> could do that via panic(), then catch at middleware root and handle.

perhaps it may be simple to remove Abort and make Handler return bool. If
handler returns false, then no need to continue

[nodir, 2016-06-13 20:26:48]

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:166: c.aborted = true
On 2016/06/13 20:20:52, nodir wrote:
> On 2016/06/13 18:50:42, dnj (Google) wrote:
> > Just thinking here, but maybe we want "Abort()" to abort the current
handler?
> We
> > could do that via panic(), then catch at middleware root and handle.
> 
> perhaps it may be simple to remove Abort and make Handler return bool. If
> handler returns false, then no need to continue

and if you do that, replace "index" field in Context with "i" variable inside
"Context.Next". And then rename Context.Next to Run.
Unless it breaks some use case that I am not aware of

[nodir, 2016-06-13 20:29:36]

one more simplification idea, sorry for many messages

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:166: c.aborted = true
On 2016/06/13 20:26:48, nodir wrote:
> On 2016/06/13 20:20:52, nodir wrote:
> > On 2016/06/13 18:50:42, dnj (Google) wrote:
> > > Just thinking here, but maybe we want "Abort()" to abort the current
> handler?
> > We
> > > could do that via panic(), then catch at middleware root and handle.
> > 
> > perhaps it may be simple to remove Abort and make Handler return bool. If
> > handler returns false, then no need to continue
> 
> and if you do that, replace "index" field in Context with "i" variable inside
> "Context.Next". And then rename Context.Next to Run.
> Unless it breaks some use case that I am not aware of

or rather remove handlers from Context (because really, they are not contextual
information) and change "Context.Run" to global "Run" function that accepts
handlers and the Context. I would be simpler

[nishanths, 2016-06-15 18:25:08]

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/bui...
File appengine/cmd/milo/buildbot/pubsub_test.go (right):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/bui...
appengine/cmd/milo/buildbot/pubsub_test.go:170:
PubSubHandler(&router.Context{Context: c, Writer: h, Request: r, Params: p})
On 2016/06/13 19:23:29, iannucci wrote:
> (I know you didn't write this :)), but it probably would be (a lot) cleaner to
> have a helper function that prepares this, since it's copy-paste in every test
> case.

Will do!

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/set...
File appengine/cmd/milo/settings/themes.go (left):

https://codereview.chromium.org/2043423004/diff/120001/appengine/cmd/milo/set...
appengine/cmd/milo/settings/themes.go:191: return Base(hx)
On 2016/06/13 18:31:20, hinoka wrote:
> Don't we still need to install the base middlewares?

You're right. :) But they were now being added at milo.go instead:
https://codereview.chromium.org/2043423004/patch/120001/130016

https://codereview.chromium.org/2043423004/diff/120001/server/router/doc.go
File server/router/doc.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/doc.go#n...
server/router/doc.go:5: /*
On 2016/06/13 20:20:52, nodir wrote:
> I think we use // everywhere

Done.

https://codereview.chromium.org/2043423004/diff/120001/server/router/doc.go#n...
server/router/doc.go:6: Package router provides a HTTP router with support for
middleware and groups.
On 2016/06/13 20:20:52, nodir wrote:
> s/a HTTP/an HTTP/

Done.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:132: merged := make([]Handler, totalLen)
On 2016/06/13 18:50:42, dnj (Google) wrote:
> nit: probably cleaner to just allocate size 0, capacity "totalLen" and have
two
> normal "append(merged, m...)". Also just use "len(m) + len(n)" directly in the
> allocation, no need to store it in its own variable.

Acknowledged.

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:147: handlers: copyHandlers(r.handlers, handlers),
On 2016/06/13 18:50:42, dnj (Google) wrote:
> Do we really need to allocate/copy handlers each time we handle an HTTP call?
It
> would be better to just store "adaptHandlers", and resolve them to an index
> during execution (e.g.,
> 
> handlers := r.handlers
> if i >= len(handlers) {
>   handlers, i = r.adaptHandlers, (i - len(handlers))
> }
> 
> ... in Next.

Thanks for catching this. Done.

[nishanths, 2016-06-15 18:25:20]

Thanks for the feedback. 
Uploaded few patch sets based on the discussion at
https://docs.google.com/document/d/1daiHkDisRor0tX0KZyhRAL4h7qgF0aYydIEixI-fT...

Summary:

Patch Set 8: Reset refactored source files to versions at origin/master (so I
can refactor from a clean slate for the updated API).
Patch Set 9-12: Updates to router package API.
Patch Set 13: Tests for router package.
Patch Set 14: Minor router package changes.

[nodir, 2016-06-15 20:14:44]

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:67: func (r *Router) Group(relativePath string) *Router
{
I think other routers use method names Path and Subrouter. Consider one of these
names

https://codereview.chromium.org/2043423004/diff/260001/server/router/doc.go
File server/router/doc.go (right):

https://codereview.chromium.org/2043423004/diff/260001/server/router/doc.go#n...
server/router/doc.go:30: //
it may be (or may be not) better to move this example to example_test.go, see
"Larger examples" section in https://blog.golang.org/examples

this way we will be sure it compiles and it may be nicely formatted on godoc

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:15: // that handles a HTTP request, it must obey the
following rules.
an HTTP

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:20: // to the Context after c.Written()==true.
Middleware may
"after next is called and c.Written()==true"
otherwise, strictly speaking, it means it can write to context only once

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:21: // modify the embedded context before calling 
next.
use "a" and "the" consistently across paragraph (Middleware, A Middleware, call
next, call the next function)

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:37: if len(m) > 0 {
use `switch {`

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:38: m[0](c, Handler(func(ctx *Context) {
`Handler(` is unnecessary because func is a literal

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:42: n[0](c, Handler(func(ctx *Context) {
same here

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:57: final = make(MiddlewareChain, 0, finalLen)
optional: return if finalLen == 0

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:66: func MiddlewareWithContext(c context.Context)
Middleware {
I am not sure we need this. odds are there will be only one usage in the
luci-common-base middleware sets up all services. dunno.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:67: return Middleware(func(ctx *Context, next Handler)
{
perhaps `Middleware(` is unnecessary

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:20: parent     *Router
why do you need parent? is not used and seems unnecessary because BasePath
already contains basepath of the parent

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:49: // all handlers registered on the router and to all
handlers handlers registered on
s/handlers handlers/handlers/

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:50: // routers derived from the router (using Group).
s/derived/that will be derived/ because Group copies middleware
alternatively do not mention derived routers

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:51: func (r *Router) Use(mc MiddlewareChain) {
consider `...Middleware`

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:61: BasePath: httprouter.CleanPath(r.BasePath +
relativePath),
this line assumes that BasePath ends with "/"
at the same time this line may break this assumption if relativePath does not
end with "/"

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:126: // NewContext creates a Context that with the given
parameters.
s/that //

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:136: // Header is used to implement http.ResponseWriter
interface on Context.
s/ on Context//g

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:145: c.status = http.StatusOK
do it only if c.status == 0

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:153: c.status = code
do it only if c.status == 0

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:157: // StatusCode returns the written HTTP status code.
" or 0 if it was not written"

[nishanths, 2016-06-16 00:14:19]

If this looks alright, I can begin refactoring the source code to use this new
API.

https://codereview.chromium.org/2043423004/diff/260001/server/router/doc.go
File server/router/doc.go (right):

https://codereview.chromium.org/2043423004/diff/260001/server/router/doc.go#n...
server/router/doc.go:30: //
On 2016/06/15 20:14:43, nodir wrote:
> it may be (or may be not) better to move this example to example_test.go, see
> "Larger examples" section in https://blog.golang.org/examples
> 
> this way we will be sure it compiles and it may be nicely formatted on godoc

Moved to example_test.go and removed this file. Compilation checking huge plus.
:)

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:15: // that handles a HTTP request, it must obey the
following rules.
On 2016/06/15 20:14:43, nodir wrote:
> an HTTP

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:20: // to the Context after c.Written()==true.
Middleware may
On 2016/06/15 20:14:43, nodir wrote:
> "after next is called and c.Written()==true"
> otherwise, strictly speaking, it means it can write to context only once

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:21: // modify the embedded context before calling 
next.
On 2016/06/15 20:14:43, nodir wrote:
> use "a" and "the" consistently across paragraph (Middleware, A Middleware,
call
> next, call the next function)

My bad. Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:37: if len(m) > 0 {
On 2016/06/15 20:14:43, nodir wrote:
> use `switch {`

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:38: m[0](c, Handler(func(ctx *Context) {
On 2016/06/15 20:14:43, nodir wrote:
> `Handler(` is unnecessary because func is a literal

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:42: n[0](c, Handler(func(ctx *Context) {
On 2016/06/15 20:14:43, nodir wrote:
> same here

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:57: final = make(MiddlewareChain, 0, finalLen)
On 2016/06/15 20:14:43, nodir wrote:
> optional: return if finalLen == 0

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:66: func MiddlewareWithContext(c context.Context)
Middleware {
On 2016/06/15 20:14:43, nodir wrote:
> I am not sure we need this. odds are there will be only one usage in the
> luci-common-base middleware sets up all services. dunno.

I've left it in (for now). We can see how it turns out when we refactor the code
for the new API.

https://codereview.chromium.org/2043423004/diff/260001/server/router/handler....
server/router/handler.go:67: return Middleware(func(ctx *Context, next Handler)
{
On 2016/06/15 20:14:43, nodir wrote:
> perhaps `Middleware(` is unnecessary

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:20: parent     *Router
On 2016/06/15 20:14:43, nodir wrote:
> why do you need parent? is not used and seems unnecessary because BasePath
> already contains basepath of the parent

Done. Remnants of old stuff I was trying, thanks for catching.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:49: // all handlers registered on the router and to all
handlers handlers registered on
On 2016/06/15 20:14:43, nodir wrote:
> s/handlers handlers/handlers/

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:50: // routers derived from the router (using Group).
On 2016/06/15 20:14:44, nodir wrote:
> s/derived/that will be derived/ because Group copies middleware
> alternatively do not mention derived routers

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:51: func (r *Router) Use(mc MiddlewareChain) {
On 2016/06/15 20:14:43, nodir wrote:
> consider `...Middleware`

+1 Will reconsider while refactoring the rest of the code to use the new API.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:61: BasePath: httprouter.CleanPath(r.BasePath +
relativePath),
On 2016/06/15 20:14:44, nodir wrote:
> this line assumes that BasePath ends with "/"
> at the same time this line may break this assumption if relativePath does not
> end with "/"

Fixed now. See the makeBasePath function. I'm still a bit hazy about how much to
clean vs. letting the user be careful.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:126: // NewContext creates a Context that with the given
parameters.
On 2016/06/15 20:14:43, nodir wrote:
> s/that //

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:136: // Header is used to implement http.ResponseWriter
interface on Context.
On 2016/06/15 20:14:43, nodir wrote:
> s/ on Context//g

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:145: c.status = http.StatusOK
On 2016/06/15 20:14:44, nodir wrote:
> do it only if c.status == 0

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:153: c.status = code
On 2016/06/15 20:14:44, nodir wrote:
> do it only if c.status == 0

Done.

https://codereview.chromium.org/2043423004/diff/260001/server/router/router.g...
server/router/router.go:157: // StatusCode returns the written HTTP status code.
On 2016/06/15 20:14:43, nodir wrote:
> " or 0 if it was not written"

Done.

[nishanths, 2016-06-16 00:18:09]

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/120001/server/router/router.g...
server/router/router.go:67: func (r *Router) Group(relativePath string) *Router
{
On 2016/06/15 20:14:43, nodir wrote:
> I think other routers use method names Path and Subrouter. Consider one of
these
> names

Chose 'Subrouter'. Open to changing the name if anyone has thoughts.

[iannucci, 2016-06-16 00:54:25]

neat :)

https://codereview.chromium.org/2043423004/diff/280001/server/router/example_...
File server/router/example_test.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/example_...
server/router/example_test.go:74: log.Fatal(http.ListenAndServe(":8080", nil))
I think you need

  // Output:
  // something
  // something else

For this to actually run as an example test. (go silently ignores it otherwise
:()

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:21: // may modify the embedded context before calling
next.
let's make these bullets:

   //   - Middleware must call next if it has not written to the Context
   //     by the end of the function.
   //   - Middleware must not call next if it has written to the Context.
   //   - Middleware must not write to the Context after next is called 
   //     and c.Written()==true. 
   //   - Middleware may modify the embedded context before calling next.

Check with the godoc command-line tool to make sure this is properly formatted
in the ui (e.g. `godoc -http=:6060`, http://localhost:6060/...)

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:53: func MergeMiddlewareChains(mchains
...MiddlewareChain) (final MiddlewareChain) {
Maybe JoinMiddlewareChains? Or ConcatMiddlewareChains? It's not really a merge
(e.g. not the 'merge' interleaving action ala
https://en.wikipedia.org/wiki/Merge_sort)

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:71: func MiddlewareWithContext(c context.Context)
Middleware {
hm... this seems a bit dangerous: this /replaces/ the context with the provided
one and calls the next handler. This means if this is done in the middle of a
chain, all the previous context is lost. What about

func SetContext(c context.Context) Middleware {
  ...
}

Then when you see:

Handle("PUT", "/something", MiddlewareChain{SetContext(c), Something,
Something}, h)

it makes sense, and if you saw it in the middle, it would immediately look
suspicious

Handle("PUT", "/something", MiddlewareChain{Something, SetContext(c),
Something}, h)

wdyt?

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:53: r.middleware = append(r.middleware, mc...)
should you use your join function above for this?

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:71: // GET is a shortcut for router.Handle("GET", mc, h)
nit: missing 'path' in these comments

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:107: // path. len(mc)==0 is allowed.
may want to add a pointer to the format of path in the original router's
documentation (See godoc.org/......./httprouter#Path for documentation on how
the path may be formatted)

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:128: // After CleanPath, len(path) >= 0.
what if base and relative are both ""?

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:136: func NewContext(c context.Context, rw
http.ResponseWriter, req *http.Request, p httprouter.Params) *Context {
is this for testing, or are users expected to call this?

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:154: if c.status == 0 {
if !c.Written()

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:164: if c.status == 0 {
if !c.Written()

[nishanths, 2016-06-16 03:43:29]

https://codereview.chromium.org/2043423004/diff/280001/server/router/example_...
File server/router/example_test.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/example_...
server/router/example_test.go:74: log.Fatal(http.ListenAndServe(":8080", nil))
On 2016/06/16 00:54:24, iannucci wrote:
> I think you need
> 
>   // Output:
>   // something
>   // something else
> 
> For this to actually run as an example test. (go silently ignores it otherwise
> :()
> 

Cool, done. :) 
If you have time, please check to make sure it's along the lines of what you
expected.

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:21: // may modify the embedded context before calling
next.
On 2016/06/16 00:54:24, iannucci wrote:
> let's make these bullets:
> 
>    //   - Middleware must call next if it has not written to the Context
>    //     by the end of the function.
>    //   - Middleware must not call next if it has written to the Context.
>    //   - Middleware must not write to the Context after next is called 
>    //     and c.Written()==true. 
>    //   - Middleware may modify the embedded context before calling next.
> 
> Check with the godoc command-line tool to make sure this is properly formatted
> in the ui (e.g. `godoc -http=:6060`, http://localhost:6060/...)

+1 Done. With aligned dashes inside a <pre> block. (This is how the standard lib
does it too: https://golang.org/pkg/encoding/xml/#Marshal)

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:53: func MergeMiddlewareChains(mchains
...MiddlewareChain) (final MiddlewareChain) {
On 2016/06/16 00:54:24, iannucci wrote:
> Maybe JoinMiddlewareChains? Or ConcatMiddlewareChains? It's not really a merge
> (e.g. not the 'merge' interleaving action ala
> https://en.wikipedia.org/wiki/Merge_sort)

Good point. But, removing this func as it is unused, since a few commits ago
when we added the run method. Previously it was used in adapt.

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:71: func MiddlewareWithContext(c context.Context)
Middleware {
On 2016/06/16 00:54:24, iannucci wrote:
> hm... this seems a bit dangerous: this /replaces/ the context with the
provided
> one and calls the next handler. This means if this is done in the middle of a
> chain, all the previous context is lost. What about
> 
> func SetContext(c context.Context) Middleware {
>   ...
> }
> 
> Then when you see:
> 
> Handle("PUT", "/something", MiddlewareChain{SetContext(c), Something,
> Something}, h)
> 
> it makes sense, and if you saw it in the middle, it would immediately look
> suspicious
> 
> Handle("PUT", "/something", MiddlewareChain{Something, SetContext(c),
> Something}, h)
> 
> wdyt?

Yep, SetContext is a far better name. 
Nevertheless, I am removing this function because, as Nodir noted, there are not
many places it will be used.

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:53: r.middleware = append(r.middleware, mc...)
On 2016/06/16 00:54:24, iannucci wrote:
> should you use your join function above for this?

No, because there is no need to force the allocation of a new array each time.

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:71: // GET is a shortcut for router.Handle("GET", mc, h)
On 2016/06/16 00:54:25, iannucci wrote:
> nit: missing 'path' in these comments

Done!

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:107: // path. len(mc)==0 is allowed.
On 2016/06/16 00:54:24, iannucci wrote:
> may want to add a pointer to the format of path in the original router's
> documentation (See godoc.org/......./httprouter#Path for documentation on how
> the path may be formatted)

Done. There is no separate section on path formatting, unfortunately.

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:128: // After CleanPath, len(path) >= 0.
On 2016/06/16 00:54:25, iannucci wrote:
> what if base and relative are both ""?

Currently, makeBasePath("", "") => "/".
We have a local test for this router_test.go testing the above.

There is also a related httprouter test:
https://github.com/julienschmidt/httprouter/blob/master/path_test.go#L24

Here's the implementation:
https://github.com/julienschmidt/httprouter/blob/master/path.go#L22

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:136: func NewContext(c context.Context, rw
http.ResponseWriter, req *http.Request, p httprouter.Params) *Context {
On 2016/06/16 00:54:25, iannucci wrote:
> is this for testing, or are users expected to call this?

Removing for now. I could not think of users wanting to call this or for a
pressing need for this in tests. But I might be missing something (see Nodir's
last comment in this thread:
https://docs.google.com/document/d/1daiHkDisRor0tX0KZyhRAL4h7qgF0aYydIEixI-fT...)

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:154: if c.status == 0 {
On 2016/06/16 00:54:24, iannucci wrote:
> if !c.Written()

Done.

https://codereview.chromium.org/2043423004/diff/280001/server/router/router.g...
server/router/router.go:164: if c.status == 0 {
On 2016/06/16 00:54:24, iannucci wrote:
> if !c.Written()

Done.

[nodir, 2016-06-16 04:20:26]

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/280001/server/router/handler....
server/router/handler.go:21: // may modify the embedded context before calling
next.
On 2016/06/16 00:54:24, iannucci wrote:
> let's make these bullets:
> 
>    //   - Middleware must call next if it has not written to the Context
>    //     by the end of the function.
>    //   - Middleware must not call next if it has written to the Context.
>    //   - Middleware must not write to the Context after next is called 
>    //     and c.Written()==true. 
>    //   - Middleware may modify the embedded context before calling next.
> 
> Check with the godoc command-line tool to make sure this is properly formatted
> in the ui (e.g. `godoc -http=:6060`, http://localhost:6060/...)

+1

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
File server/router/example_test.go (right):

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:23: durations  []time.Duration
consider removing, see below

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:24: client     *http.Client
unsed

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:25: testServer *httptest.Server
unused

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:36: durations = append(durations,
time.Since(start))
maybe just print it to stdout? it will be more visible in output and will
simplify code

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:91: return Response{status, body}
to simplify, I'd return sprintf right here

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:94: func makeRequests(server *httptest.Server) {
perhaps "url string" would be enough here

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:96: c.Timeout = 10 * time.Second
why this is needed?

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:108: r.GET("/hello/:name", nil, Hello)
consider inlining Root, Hello and Random to make example simpler

please remember that example has educational purpose and has higher requirements
for simplicity

https://codereview.chromium.org/2043423004/diff/320001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/320001/server/router/router.g...
server/router/router.go:135: if path[len(path)-1] != '/' {
I am not really sure this is correct because the arg in string indexer is a byte
offset, not index of character, and not all chars are 1 byte. Please use
strings.HasSuffix

https://codereview.chromium.org/2043423004/diff/320001/server/router/router.g...
server/router/router.go:136: path = path + "/"
`+=`?

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
File server/router/router_test.go (right):

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:44: b := func(_ *Context, _ Handler) {}
I don't know if you do it intentionally or not, but let me write it anyway

Conveys can be nested, e.g.

Convey("Router", t, func() {
  a := ...
  b := ...
  
  Convey("foo", func() { /* you can use a and b here */})
  Convey("bar", func() { /* you can use a and b here */})
})

this will save you from writing same a, b, `r = New()` many times

usually we have only one top-level func that has a hierarchy of conveys

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:70: Convey("No side-effects on r.middleware", t,
func() {
convey usually panics on identical names. make them different

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:82: io.WriteString(c, "1")
consider replacing digits with something that indicates which middleware printed
it, e.g. "a"

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:84: io.WriteString(c, "7")
hm.. this test violates middleware rule that it should not write to c if next
wrote something

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:108: run(&Context{writer: rec}, m, n, handler)
you could just pass nils instead as middleware chains. no need for m and n

here and everywhere

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:138: }
setup and cleanup can be inlined if you use hierarchy of conveys

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:272: assert.ShouldBeFalse(c.Written())
why not

So(c.Written(), ShouldBeFalse)

this is more conventional in this codebase

[nishanths, 2016-06-16 22:11:27]

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
File server/router/example_test.go (right):

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:23: durations  []time.Duration
On 2016/06/16 04:20:25, nodir wrote:
> consider removing, see below

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:24: client     *http.Client
On 2016/06/16 04:20:25, nodir wrote:
> unsed

Removed.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:25: testServer *httptest.Server
On 2016/06/16 04:20:25, nodir wrote:
> unused

Removed.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:36: durations = append(durations,
time.Since(start))
On 2016/06/16 04:20:25, nodir wrote:
> maybe just print it to stdout? it will be more visible in output and will
> simplify code

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:91: return Response{status, body}
On 2016/06/16 04:20:25, nodir wrote:
> to simplify, I'd return sprintf right here

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:94: func makeRequests(server *httptest.Server) {
On 2016/06/16 04:20:25, nodir wrote:
> perhaps "url string" would be enough here

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:96: c.Timeout = 10 * time.Second
On 2016/06/16 04:20:25, nodir wrote:
> why this is needed?

If Timeout==0, the client waits infinitely for a response
(https://golang.org/src/net/http/client.go#L290). So, for instance, if the run
function had a bug, the tests would not finish running.

func run(...) {
       for {
              // infinite loop     
       }
}

But now that I think of it, the tests not finishing running holds true for any
such poorly written functions. 
Removed it. Also removed from router_test.go.

https://codereview.chromium.org/2043423004/diff/320001/server/router/example_...
server/router/example_test.go:108: r.GET("/hello/:name", nil, Hello)
On 2016/06/16 04:20:25, nodir wrote:
> consider inlining Root, Hello and Random to make example simpler
> 
> please remember that example has educational purpose and has higher
requirements
> for simplicity

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/320001/server/router/router.g...
server/router/router.go:135: if path[len(path)-1] != '/' {
On 2016/06/16 04:20:25, nodir wrote:
> I am not really sure this is correct because the arg in string indexer is a
byte
> offset, not index of character, and not all chars are 1 byte. Please use
> strings.HasSuffix

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router.g...
server/router/router.go:136: path = path + "/"
On 2016/06/16 04:20:25, nodir wrote:
> `+=`?

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
File server/router/router_test.go (right):

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:44: b := func(_ *Context, _ Handler) {}
On 2016/06/16 04:20:25, nodir wrote:
> I don't know if you do it intentionally or not, but let me write it anyway
> 
> Conveys can be nested, e.g.
> 
> Convey("Router", t, func() {
>   a := ...
>   b := ...
>   
>   Convey("foo", func() { /* you can use a and b here */})
>   Convey("bar", func() { /* you can use a and b here */})
> })
> 
> this will save you from writing same a, b, `r = New()` many times
> 
> usually we have only one top-level func that has a hierarchy of conveys

Thanks. Rewritten.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:70: Convey("No side-effects on r.middleware", t,
func() {
On 2016/06/16 04:20:25, nodir wrote:
> convey usually panics on identical names. make them different

Thanks for the tip. Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:82: io.WriteString(c, "1")
On 2016/06/16 04:20:25, nodir wrote:
> consider replacing digits with something that indicates which middleware
printed
> it, e.g. "a"

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:84: io.WriteString(c, "7")
On 2016/06/16 04:20:25, nodir wrote:
> hm.. this test violates middleware rule that it should not write to c if next
> wrote something

Done, using context.Context instead.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:108: run(&Context{writer: rec}, m, n, handler)
On 2016/06/16 04:20:25, nodir wrote:
> you could just pass nils instead as middleware chains. no need for m and n
> 
> here and everywhere

Done. For some reason, I was thinking we would need typed nils in the args.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:138: }
On 2016/06/16 04:20:25, nodir wrote:
> setup and cleanup can be inlined if you use hierarchy of conveys

Done.

https://codereview.chromium.org/2043423004/diff/320001/server/router/router_t...
server/router/router_test.go:272: assert.ShouldBeFalse(c.Written())
On 2016/06/16 04:20:25, nodir wrote:
> why not
> 
> So(c.Written(), ShouldBeFalse)
> 
> this is more conventional in this codebase

Done. Earlier I used assertions because the tests errored as a result of So
being called without a Convey func on the stack (requests are handled in
goroutines). But looking back, I was wrong anyway because I don't think the test
suite waited for the assertions to finish.

I've fixed by using the async style.

[nodir, 2016-06-16 22:19:00]

lgtm

https://codereview.chromium.org/2043423004/diff/360001/server/router/router_t...
File server/router/router_test.go (right):

https://codereview.chromium.org/2043423004/diff/360001/server/router/router_t...
server/router/router_test.go:106: So(ctx.Value(outputKey).([]string),
ShouldResemble, []string{"handler"})
ctx.Value(outputKey).([]string) casts the value from interface{} to []string,
but first parameter of So is interface{}, so conversion is noop, thus
unnecessary

[nishanths, 2016-06-16 22:24:22]

https://codereview.chromium.org/2043423004/diff/360001/server/router/router_t...
File server/router/router_test.go (right):

https://codereview.chromium.org/2043423004/diff/360001/server/router/router_t...
server/router/router_test.go:106: So(ctx.Value(outputKey).([]string),
ShouldResemble, []string{"handler"})
On 2016/06/16 22:19:00, nodir wrote:
> ctx.Value(outputKey).([]string) casts the value from interface{} to []string,
> but first parameter of So is interface{}, so conversion is noop, thus
> unnecessary

Thanks. Done.

[Vadim Sh., 2016-06-16 22:47:14]

https://codereview.chromium.org/2043423004/diff/400001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/400001/server/router/handler....
server/router/handler.go:7: type (
nit: it's the first chunk of code that uses type ( .... ) construct. I'd prefer
each type has it's own "type" keyword. That makes them standout visually more
when glancing over the file.

type Handler ...

type Middleware ...

....

https://codereview.chromium.org/2043423004/diff/400001/server/router/handler....
server/router/handler.go:43: run(ctx, m, n[1:], h)
nit: run(ctx, nil, n[1:], h)

(to make it clear that we are done with 'm')

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:17: type (
same here

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:29: writer  http.ResponseWriter
I think it should be mockable (e.g. be "Writer http.ResponseWriter").

Monitoring middleware will need to replace the writer. I think it's more
important than exposing StatusCode() and Written().

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:123: // TODO(maybe): Use a free list for making
Contexts.
IMHO, not worth it, it's lightweight object and its created only once per
request

[nodir, 2016-06-16 23:04:47]

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:29: writer  http.ResponseWriter
On 2016/06/16 22:47:14, Vadim Sh. wrote:
> I think it should be mockable (e.g. be "Writer http.ResponseWriter").
> 
> Monitoring middleware will need to replace the writer. I think it's more
> important than exposing StatusCode() and Written().

+1, I didn't notice there is no NewContext that accepts writer

[nishanths, 2016-06-16 23:33:01]

https://codereview.chromium.org/2043423004/diff/400001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/400001/server/router/handler....
server/router/handler.go:7: type (
On 2016/06/16 22:47:14, Vadim Sh. wrote:
> nit: it's the first chunk of code that uses type ( .... ) construct. I'd
prefer
> each type has it's own "type" keyword. That makes them standout visually more
> when glancing over the file.
> 
> type Handler ...
> 
> type Middleware ...
> 
> ....

Done.

https://codereview.chromium.org/2043423004/diff/400001/server/router/handler....
server/router/handler.go:43: run(ctx, m, n[1:], h)
On 2016/06/16 22:47:14, Vadim Sh. wrote:
> nit: run(ctx, nil, n[1:], h)
> 
> (to make it clear that we are done with 'm')

Done.

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:17: type (
On 2016/06/16 22:47:14, Vadim Sh. wrote:
> same here

Done.

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:29: writer  http.ResponseWriter
On 2016/06/16 22:47:14, Vadim Sh. wrote:
> I think it should be mockable (e.g. be "Writer http.ResponseWriter").
> 
> Monitoring middleware will need to replace the writer. I think it's more
> important than exposing StatusCode() and Written().

Done.

https://codereview.chromium.org/2043423004/diff/400001/server/router/router.g...
server/router/router.go:123: // TODO(maybe): Use a free list for making
Contexts.
On 2016/06/16 22:47:14, Vadim Sh. wrote:
> IMHO, not worth it, it's lightweight object and its created only once per
> request

Cool. Removed the TODO comment.

[Vadim Sh., 2016-06-16 23:46:38]

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.g...
server/router/router.go:27: context.Context
nit: new line after context.Context to separate embedded struct from regular
fields

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.g...
server/router/router.go:138: // Header is used to implement http.ResponseWriter
interface.
IMHO, it's better to remove this wrapping, since now users can trivially bypass
it by using c.Writer directly. Is StatusCode() or Written() used in any
meaningful way currently?

Better to embed ResponseWriter too:

type Context {
  context.Context
  http.ResponseWriter

  ...
}

[Vadim Sh., 2016-06-17 00:05:35]

(Sorry this review is taking so long... The important lesson here: pick small
set of reviewers, ideally <=2 people. It's hard to satisfy everyone. Code
reviews often are subjective, unfortunately).

[nodir, 2016-06-17 00:25:04]

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.g...
server/router/router.go:138: // Header is used to implement http.ResponseWriter
interface.
On 2016/06/16 23:46:37, Vadim Sh. wrote:
> IMHO, it's better to remove this wrapping, since now users can trivially
bypass
> it by using c.Writer directly. Is StatusCode() or Written() used in any
> meaningful way currently?
> 
> Better to embed ResponseWriter too:
> 
> type Context {
>   context.Context
>   http.ResponseWriter
> 
>   ...
> }

(Chatted with Vadim)

Indeed. If a middleware needs to know whether next wrote something, of need to
know the status code, it can replace ctx.ResponseWriter  with one that records
those

[nishanths, 2016-06-17 00:48:24]

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.g...
server/router/router.go:27: context.Context
On 2016/06/16 23:46:37, Vadim Sh. wrote:
> nit: new line after context.Context to separate embedded struct from regular
> fields

Done.

https://codereview.chromium.org/2043423004/diff/420001/server/router/router.g...
server/router/router.go:138: // Header is used to implement http.ResponseWriter
interface.
On 2016/06/17 00:25:04, nodir wrote:
> On 2016/06/16 23:46:37, Vadim Sh. wrote:
> > IMHO, it's better to remove this wrapping, since now users can trivially
> bypass
> > it by using c.Writer directly. Is StatusCode() or Written() used in any
> > meaningful way currently?
> > 
> > Better to embed ResponseWriter too:
> > 
> > type Context {
> >   context.Context
> >   http.ResponseWriter
> > 
> >   ...
> > }
> 
> (Chatted with Vadim)
> 
> Indeed. If a middleware needs to know whether next wrote something, of need to
> know the status code, it can replace ctx.ResponseWriter  with one that records
> those

Sounds good. Done.

[Vadim Sh., 2016-06-17 00:57:31]

this looks good, thanks

next steps:
1) Convert common middlewares in server/* and appengine/*, but do not yet touch
appengine/cmd/* or other non-generic app code.
2) Once reviewed, convert the rest of the repo to use new package.
3) Land.
4) (Here starts the "fun" part you probably didn't expect). Update luci-go
pinned revision in infra.git (ask me for details).
5) Fix all infra.git code to use new package.
6) Fix all infra_internal.git code to use new package.

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.g...
server/router/router.go:32: status  int
not needed anymore

[Vadim Sh., 2016-06-17 01:07:52]

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.g...
server/router/router.go:72: func (r *Router) GET(path string, mc
MiddlewareChain, h Handler) {
also, what do you feel about adding a bit more magic here:

func (r *Router) GET(path string, args ...interface{}) {
  // type cast first len(args) - 1 args to Middleware or MiddlewareChain, merge
  // type cast the last one to Handler
  ...
}

r.GET("/path", Logging, Auth, handler)

?

[nishanths, 2016-06-17 01:48:52]

https://codereview.chromium.org/2043423004/diff/460001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/460001/server/router/handler....
server/router/handler.go:27: func (mc MiddlewareChain) Run(c *Context, h
Handler) {
Removing this since it is unused. I cannot see users calling this directly.

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.g...
server/router/router.go:32: status  int
On 2016/06/17 00:57:31, Vadim Sh. wrote:
> not needed anymore

Sorry I missed that. Done.

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.g...
server/router/router.go:72: func (r *Router) GET(path string, mc
MiddlewareChain, h Handler) {
On 2016/06/17 01:07:52, Vadim Sh. wrote:
> also, what do you feel about adding a bit more magic here:
> 
> func (r *Router) GET(path string, args ...interface{}) {
>   // type cast first len(args) - 1 args to Middleware or MiddlewareChain,
merge
>   // type cast the last one to Handler
>   ...
> }
> 
> r.GET("/path", Logging, Auth, handler)
> 
> ?

panic if we cannot coerce to the types we expect? (Nevertheless, the panic will
happen as soon as the server is started.) I'm open to changing if you want to,
however I prefer having the types mentioned because it is better documented and
checked at compile time.

[Vadim Sh., 2016-06-17 01:58:15]

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.g...
server/router/router.go:72: func (r *Router) GET(path string, mc
MiddlewareChain, h Handler) {
On 2016/06/17 01:48:51, nishanths wrote:
> On 2016/06/17 01:07:52, Vadim Sh. wrote:
> > also, what do you feel about adding a bit more magic here:
> > 
> > func (r *Router) GET(path string, args ...interface{}) {
> >   // type cast first len(args) - 1 args to Middleware or MiddlewareChain,
> merge
> >   // type cast the last one to Handler
> >   ...
> > }
> > 
> > r.GET("/path", Logging, Auth, handler)
> > 
> > ?
> 
> panic if we cannot coerce to the types we expect? (Nevertheless, the panic
will
> happen as soon as the server is started.) I'm open to changing if you want to,
> however I prefer having the types mentioned because it is better documented
and
> checked at compile time.

Yeah... Explicit types are probably better than magic that saves few keystrokes.

[nodir, 2016-06-17 02:00:09]

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.go
File server/router/router.go (right):

https://codereview.chromium.org/2043423004/diff/460001/server/router/router.g...
server/router/router.go:72: func (r *Router) GET(path string, mc
MiddlewareChain, h Handler) {
On 2016/06/17 01:48:51, nishanths wrote:
> On 2016/06/17 01:07:52, Vadim Sh. wrote:
> > also, what do you feel about adding a bit more magic here:
> > 
> > func (r *Router) GET(path string, args ...interface{}) {
> >   // type cast first len(args) - 1 args to Middleware or MiddlewareChain,
> merge
> >   // type cast the last one to Handler
> >   ...
> > }
> > 
> > r.GET("/path", Logging, Auth, handler)
> > 
> > ?
> 
> panic if we cannot coerce to the types we expect? 

yes

> (Nevertheless, the panic will
> happen as soon as the server is started.) I'm open to changing if you want to,
> however I prefer having the types mentioned because it is better documented
and
> checked at compile time.

fwiw i am ok with ...interface{}
if user makes a mistake, server won't start

[nishanths, 2016-06-17 21:44:27]

I've un-embedded context.Context and http.ResponseWriter from Context because of
https://docs.google.com/a/google.com/document/d/1QUK_-oF3wCOgim7_941C1yk8edIy...

[iannucci, 2016-06-18 00:16:05]

On 2016/06/17 21:44:27, nishanths wrote:
> I've un-embedded context.Context and http.ResponseWriter from Context because
of
>
https://docs.google.com/a/google.com/document/d/1QUK_-oF3wCOgim7_941C1yk8edIy...

Ok, seems fine, but how is a Middleware supposed to know if the Writer has been
written to already now?

[Vadim Sh., 2016-06-18 00:21:40]

On 2016/06/18 00:16:05, iannucci wrote:
> On 2016/06/17 21:44:27, nishanths wrote:
> > I've un-embedded context.Context and http.ResponseWriter from Context
because
> of
> >
>
https://docs.google.com/a/google.com/document/d/1QUK_-oF3wCOgim7_941C1yk8edIy...
> 
> Ok, seems fine, but how is a Middleware supposed to know if the Writer has
been
> written to already now?

If middlware cares about it, it can replace the writer before calling next(),
and checking the result after next(). The assumption is that middlewares always
starts with response writer clean. I think its fine. A middleware that writes
something to response and still calls next() is evil.

[nishanths, 2016-06-18 01:13:34]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-18 01:13:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/520001

[commit-bot: I haz the power, 2016-06-18 01:19:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-18 01:19:58]

Dry run: Try jobs failed on following builders:
  Luci-go Mac Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Mac%20Tester/...)

[nishanths, 2016-06-18 03:05:26]

Added Patch Set 28 that addresses step 1 in Vadim's comment: "Convert common
middlewares in server/* and appengine/*, but do not yet touch
appengine/cmd/* or other non-generic app code."

pcg pre-submit checks fail (when checked across the whole repo), but the touched
packages should still build individually without errors:

- appengine/gaeauth/server/...
- appengine/gaemiddleware/...
- appengine/gaetesting/...
- appengine/logdog/....
- appengine/tsmon/...
- appengine/tumble/...
- server/auth/...
- server/middleware/...
- server/prpc/...
- server/router/...
- server/settings/...
- server/templates/...

PTAL (I'll look at the changes once again also to make sure I haven't missed
anything) :)

[Vadim Sh., 2016-06-18 16:57:20]

looks good

two main comments (see inline comments for details):

1) We now should be careful about possibility of *Context being mutated in
next(). It's important for middlewares that do something after next() (tsmon and
panic catcher currently).

2) Order of middlewares in settings/admin/handlers.go.

https://codereview.chromium.org/2043423004/diff/560001/appengine/gaemiddlewar...
File appengine/gaemiddleware/context.go (right):

https://codereview.chromium.org/2043423004/diff/560001/appengine/gaemiddlewar...
appengine/gaemiddleware/context.go:82: 
nit: remove this new line :)

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
File appengine/tsmon/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
appengine/tsmon/middleware.go:86: // Middleware returns a middleware that must
be inserted into the middleware
nit: "is a middleware"

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
appengine/tsmon/middleware.go:105: s.flushIfNeeded(c.Context, state, settings)
I think we need to preserve the original c.Context and use it here.

next(c) can mutate c.Context now, replacing it with something unusable.

Same for defer: keep a copy of c.Context and grab c.Request.ContentLength before
calling next().

https://codereview.chromium.org/2043423004/diff/560001/server/middleware/pani...
File server/middleware/paniccatcher.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/middleware/pani...
server/middleware/paniccatcher.go:21: }.Errorf(c.Context, "Caught panic during
handling of %q:\n%s", c.Request.RequestURI, p.Stack)
same as in tsmon middleware: better to preserve the current state before calling
next(), since c is mutable now.

ctx := *c
defer .... {
  // use ctx, not c
}
next(c)

https://codereview.chromium.org/2043423004/diff/560001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/router/handler....
server/router/handler.go:26: var VoidMiddleware = func(c *Context, next Handler)
{
what do you think about teaching 'run' to recognize nil as a void middleware
instead?

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
File server/settings/admin/handlers.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
server/settings/admin/handlers.go:60: rr.Use(append(
I think it should be in reverse now?

base, // basic setup #1
templates.WithTemplates(tmpl), // basic setup #2
auth.Use(auth.Authenticator{adminAuth}), // auth setup #1
auth.WithDB{...}, // auth setup #2
auth.Autologin, // actual auth step
adminOnly // additional auth step

[nishanths, 2016-06-19 02:47:03]

https://codereview.chromium.org/2043423004/diff/560001/appengine/gaemiddlewar...
File appengine/gaemiddleware/context.go (right):

https://codereview.chromium.org/2043423004/diff/560001/appengine/gaemiddlewar...
appengine/gaemiddleware/context.go:82: 
On 2016/06/18 16:57:20, Vadim Sh. wrote:
> nit: remove this new line :)

Done :)

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
File appengine/tsmon/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
appengine/tsmon/middleware.go:86: // Middleware returns a middleware that must
be inserted into the middleware
On 2016/06/18 16:57:20, Vadim Sh. wrote:
> nit: "is a middleware"

Done.

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
appengine/tsmon/middleware.go:105: s.flushIfNeeded(c.Context, state, settings)
On 2016/06/18 16:57:20, Vadim Sh. wrote:
> I think we need to preserve the original c.Context and use it here.
> 
> next(c) can mutate c.Context now, replacing it with something unusable.
> 
> Same for defer: keep a copy of c.Context and grab c.Request.ContentLength
before
> calling next().

Done. With Patch Set 39 applied, it matches behavior in origin/master, except
for:

- In Patch Set 29 we are grabbing c.Request.ContentLength (int64). 
- In origin/master, we only grab r (pointer to http.Request). 

What should we do here? (I'm unsure because I don't understand the expected
behavior of the monitoring code completely yet.)

https://codereview.chromium.org/2043423004/diff/560001/server/middleware/pani...
File server/middleware/paniccatcher.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/middleware/pani...
server/middleware/paniccatcher.go:21: }.Errorf(c.Context, "Caught panic during
handling of %q:\n%s", c.Request.RequestURI, p.Stack)
On 2016/06/18 16:57:20, Vadim Sh. wrote:
> same as in tsmon middleware: better to preserve the current state before
calling
> next(), since c is mutable now.
> 
> ctx := *c
> defer .... {
>   // use ctx, not c
> }
> next(c)

Done. Now matches behavior at origin/master.

https://codereview.chromium.org/2043423004/diff/560001/server/router/handler.go
File server/router/handler.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/router/handler....
server/router/handler.go:26: var VoidMiddleware = func(c *Context, next Handler)
{
On 2016/06/18 16:57:20, Vadim Sh. wrote:
> what do you think about teaching 'run' to recognize nil as a void middleware
> instead?

Good idea; not too surprising to users either. Done.

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
File server/settings/admin/handlers.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
server/settings/admin/handlers.go:60: rr.Use(append(
On 2016/06/18 16:57:20, Vadim Sh. wrote:
> I think it should be in reverse now?
> 
> base, // basic setup #1
> templates.WithTemplates(tmpl), // basic setup #2
> auth.Use(auth.Authenticator{adminAuth}), // auth setup #1
> auth.WithDB{...}, // auth setup #2
> auth.Autologin, // actual auth step
> adminOnly // additional auth step
> 

You're right. Done.

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
server/settings/admin/handlers.go:71: rr.GET("/", nil, indexPage)
This change expects that clients follow redirects. 

Previously: /admin/settings
After CL: /admin/settings/

Is this alright?

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
server/settings/admin/handlers.go:113: if !auth.CurrentUser(c.Context).Superuser
{
Note: auth.Autologin is not coupled with adminOnly any longer.

[nishanths, 2016-06-20 15:14:29]

Updated remaining source files and the tests!

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
File appengine/tsmon/middleware.go (right):

https://codereview.chromium.org/2043423004/diff/560001/appengine/tsmon/middle...
appengine/tsmon/middleware.go:105: s.flushIfNeeded(c.Context, state, settings)
On 2016/06/19 02:47:02, nishanths wrote:
> On 2016/06/18 16:57:20, Vadim Sh. wrote:
> > I think we need to preserve the original c.Context and use it here.
> > 
> > next(c) can mutate c.Context now, replacing it with something unusable.
> > 
> > Same for defer: keep a copy of c.Context and grab c.Request.ContentLength
> before
> > calling next().
> 
> Done. With Patch Set 39 applied, it matches behavior in origin/master, except
> for:
> 
> - In Patch Set 29 we are grabbing c.Request.ContentLength (int64). 
> - In origin/master, we only grab r (pointer to http.Request). 
> 
> What should we do here? (I'm unsure because I don't understand the expected
> behavior of the monitoring code completely yet.)

typo: meant Patch Set 30 at both spots above

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
File server/settings/admin/handlers.go (right):

https://codereview.chromium.org/2043423004/diff/560001/server/settings/admin/...
server/settings/admin/handlers.go:71: rr.GET("/", nil, indexPage)
On 2016/06/19 02:47:02, nishanths wrote:
> This change expects that clients follow redirects. 
> 
> Previously: /admin/settings
> After CL: /admin/settings/
> 
> Is this alright?

Changed in Patch Set 33.

https://codereview.chromium.org/2043423004/diff/620001/appengine/cmd/milo/set...
File appengine/cmd/milo/settings/themes.go (right):

https://codereview.chromium.org/2043423004/diff/620001/appengine/cmd/milo/set...
appengine/cmd/milo/settings/themes.go:160: m = append(m, withNamedBundle(nb))
The bundles are now in reverse order in the middleware chain; as far as I could
tell it does not make a difference.

[nishanths, 2016-06-20 15:28:24]

Description was changed from

==========
Make HTTP middleware easier to use.

Also, introduce new router package.

BUG=617774
==========

to

==========
This adds a new router package that wraps around the previously used router
(julienschmidt/httprouter). The new package adds support to register HTTP
middleware linearly via the Use function or when registering handlers. The new
package also allows creating subrouters that carry middleware over from the
router they are derived from.

The source code and tests are updated to use the new router package. Noteworthy
points from the refactoring process:
 * InstallHandlers accept router.MiddlewareChain as argument instead of the
previous middleware.Base.
 * auth.Autologin middleware is no longer coupled with admin.adminOnly.
 * gaetesting.BaseTest has been removed. Use gaetesting.TestingContext directly
instead.

BUG=617774
==========

[nishanths, 2016-06-20 17:52:57]

https://codereview.chromium.org/2043423004/diff/660001/appengine/gaeauth/serv...
File appengine/gaeauth/server/default.go (left):

https://codereview.chromium.org/2043423004/diff/660001/appengine/gaeauth/serv...
appengine/gaeauth/server/default.go:39: func InstallWebHandlers(r
*httprouter.Router, base middleware.Base) {
Rename to InstallHandlers?

[Vadim Sh., 2016-06-20 19:06:04]

https://codereview.chromium.org/2043423004/diff/660001/appengine/gaeauth/serv...
File appengine/gaeauth/server/default.go (left):

https://codereview.chromium.org/2043423004/diff/660001/appengine/gaeauth/serv...
appengine/gaeauth/server/default.go:39: func InstallWebHandlers(r
*httprouter.Router, base middleware.Base) {
On 2016/06/20 17:52:57, nishanths wrote:
> Rename to InstallHandlers?

Yes, please.

[nishanths, 2016-06-20 19:38:19]

Patchset #35 (id:680001) has been deleted

[nishanths, 2016-06-20 19:52:04]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-20 19:52:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/700001

[commit-bot: I haz the power, 2016-06-20 20:13:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-20 20:13:35]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-20 20:42:31]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-20 20:42:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/700001

[commit-bot: I haz the power, 2016-06-20 21:03:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-20 21:03:50]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-21 00:02:11]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 00:02:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/700001

[commit-bot: I haz the power, 2016-06-21 00:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 00:23:30]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-21 17:07:29]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 17:07:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/720001

[commit-bot: I haz the power, 2016-06-21 17:28:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 17:28:52]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-21 18:30:50]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 18:31:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/720001

[commit-bot: I haz the power, 2016-06-21 18:51:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 18:51:40]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-21 19:09:54]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 19:10:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/720001

[commit-bot: I haz the power, 2016-06-21 19:30:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 19:30:53]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-21 19:58:51]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 19:59:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/720001

[commit-bot: I haz the power, 2016-06-21 20:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 20:20:06]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-21 23:08:13]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-06-21 23:08:15]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2043423004/#ps720001
(title: "gaemiddleware: add middleware func for WithProd")

[nishanths, 2016-06-21 23:08:17]

The CQ bit was unchecked by nishanths@google.com

[nishanths, 2016-06-22 02:16:08]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-06-22 02:16:21]

The CQ bit was checked by nishanths@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 02:16:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/720001

[commit-bot: I haz the power, 2016-06-22 02:53:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 02:53:16]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[nishanths, 2016-06-22 04:24:03]

The CQ bit was checked by nishanths@google.com

[commit-bot: I haz the power, 2016-06-22 04:24:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043423004/720001

[commit-bot: I haz the power, 2016-06-22 04:34:33]

Description was changed from

==========
This adds a new router package that wraps around the previously used router
(julienschmidt/httprouter). The new package adds support to register HTTP
middleware linearly via the Use function or when registering handlers. The new
package also allows creating subrouters that carry middleware over from the
router they are derived from.

The source code and tests are updated to use the new router package. Noteworthy
points from the refactoring process:
 * InstallHandlers accept router.MiddlewareChain as argument instead of the
previous middleware.Base.
 * auth.Autologin middleware is no longer coupled with admin.adminOnly.
 * gaetesting.BaseTest has been removed. Use gaetesting.TestingContext directly
instead.

BUG=617774
==========

to

==========
This adds a new router package that wraps around the previously used router
(julienschmidt/httprouter). The new package adds support to register HTTP
middleware linearly via the Use function or when registering handlers. The new
package also allows creating subrouters that carry middleware over from the
router they are derived from.

The source code and tests are updated to use the new router package. Noteworthy
points from the refactoring process:
 * InstallHandlers accept router.MiddlewareChain as argument instead of the
previous middleware.Base.
 * auth.Autologin middleware is no longer coupled with admin.adminOnly.
 * gaetesting.BaseTest has been removed. Use gaetesting.TestingContext directly
instead.

BUG=617774

Committed:
https://github.com/luci/luci-go/commit/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95
==========

[commit-bot: I haz the power, 2016-06-22 04:34:36]

Committed patchset #36 (id:720001) as
https://github.com/luci/luci-go/commit/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95

[chromium-reviews, 2016-06-22 16:28:10]

http://67.media.tumblr.com/tumblr_m1powhRVdz1rqnp16o2_500.gif

On Tue, Jun 21, 2016 at 9:34 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Committed patchset #36 (id:720001) as
>
>
https://github.com/luci/luci-go/commit/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95
>
> https://codereview.chromium.org/2043423004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nodir, 2016-06-22 16:36:04]

thanks for doing this and for bearing with us