New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 617774 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Make HTTP middlewares in luci-go easier to use

Project Member Reported by vadimsh@chromium.org, Jun 6 2016

Issue description

luci-go is using https://github.com/julienschmidt/httprouter as its base HTTP router.

Since it doesn't supports middleware natively, we apply them "manually" to each route.

It usually looks like this:
https://github.com/luci/luci-go/blob/master/appengine/cmd/milo/frontend/milo.go

where "manual" bit is wrapping done on each individual route, and wrappers often look very confusing (callbacks within callbacks or due to reverse order of application), e.g:

https://github.com/luci/luci-go/blob/master/appengine/cmd/milo/settings/themes.go#L167
https://chrome-internal.googlesource.com/infra/infra_internal/+/master/go/src/infra_internal/puppet/service/frontend/main.go#67 (my favorite example so far)
https://github.com/luci/luci-go/blob/master/appengine/cmd/cron/ui/common.go#L35

----

We should try to simplify it by implementing API similar to gin's one: https://github.com/gin-gonic/gin#using-middleware

It should hide:
1) Weird order of application of middlewares.
2) Their explicit chaining (users shouldn't have to write callbacks that call other callbacks).
3) Support "groups" of routes: all routes in a group share an URL prefix and get applied same middleware chain.

 
Owner: nishanths@google.com
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/external/github.com/luci/luci-go.git/+/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95

commit d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95
Author: nishanths <nishanths@google.com>
Date: Wed Jun 22 04:34:30 2016

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 

Review-Url: https://codereview.chromium.org/2043423004

[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/cron/frontend/handler.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/cron/ui/common.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/cron/ui/index.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/cron/ui/invocation.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/cron/ui/job.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/cron/ui/project.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/dm/distributor/handlers.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/dm/distributor/pubsub.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/dm/distributor/tq_handler.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/dm/frontend/init.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/helloworld/frontend/handler.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/helloworld_mvm/backend/main.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/helloworld_mvm/frontend/main.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/logdog_coordinator/backend/main.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/logdog_coordinator/services/main.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/logdog_coordinator/vmuser/main.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/milo/buildbot/pubsub.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/milo/buildbot/pubsub_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/milo/frontend/milo.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/milo/settings/settings.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/milo/settings/themes.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/cmd/tokenserver/frontend/main.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaeauth/server/default.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaeauth/server/internal/authdb/handlers.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaeauth/server/internal/authdb/handlers_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaemiddleware/appengine.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaemiddleware/appengine_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaemiddleware/context.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaemiddleware/routes.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/gaetesting/middleware.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/logdog/coordinator/config/middleware.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/logdog/coordinator/service.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tsmon/global_callback_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tsmon/handler.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tsmon/handler_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tsmon/middleware.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tsmon/middleware_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tumble/service.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/appengine/tumble/tumbletest.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/common/prpc/talk/buildbot/frontend/handler.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/common/prpc/talk/buildbot/frontend/prpc.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/common/prpc/talk/helloworld/frontend/handler.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/common/prpc/talk/helloworld/frontend/prpc.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/common/testing/prpctest/server.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/context.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/context_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/db.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/handlers.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/info/info.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/info/info_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/openid/method.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/openid/method_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/signing/context.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/signing/context_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/xsrf/xsrf.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/auth/xsrf/xsrf_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/middleware/paniccatcher.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/prpc/server.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/prpc/server_test.go
[add] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/router/example_test.go
[add] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/router/handler.go
[add] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/router/router.go
[add] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/router/router_test.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/settings/admin/handlers.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/settings/admin/index.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/settings/admin/settings.go
[modify] https://crrev.com/d1ceacb1d00aa31b8fbd37a13c24c28f3650bd95/server/templates/middleware.go

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 22 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/infra_internal.git/+/5165e6772a77dff7340b3b1073c0f9fb85d34e8f

commit 5165e6772a77dff7340b3b1073c0f9fb85d34e8f
Author: nishanths <nishanths@google.com>
Date: Wed Jun 22 21:27:21 2016

Status: Fixed (was: Assigned)

Sign in to add a comment