[milo] support viewing more /ui/... routes without the /ui/ prefix.
1. implement createInterpolator to support more flexible redirect path
templates.
2. Support redirecting /inv/... to /ui/inv/...
3. Support redirecting /artifact/... to /ui/artifact/...
4. Add unit tests for createInterpolator.
R=chanli, nodir
Change-Id: I4e8b4b930fcee5b4c414e8f1d7b396ad8713755c
Bug: 1189310
Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/2812659
Commit-Queue: Weiwei Lin <weiweilin@google.com>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
diff --git a/milo/frontend/resultui/assets/redirect-sw.js b/milo/frontend/resultui/assets/redirect-sw.js
index 8ff7be4..8e43cad 100644
--- a/milo/frontend/resultui/assets/redirect-sw.js
+++ b/milo/frontend/resultui/assets/redirect-sw.js
@@ -14,7 +14,7 @@
/**
* @fileoverview
- * This service worker redirects old build page links to the new build page.
+ * This service worker redirects milo links to ResultUI links.
* This is 200-400ms faster than redirecting on the server side.
*/
@@ -25,12 +25,18 @@
self.addEventListener('fetch', (event) => {
const url = new URL(event.request.url);
- const isOldBuildPage =
+
+ const isResultUI =
// Short build link.
url.pathname.match(/^\/b\//) ||
// Long build link.
- url.pathname.match(/^\/p\/[^/]+\/builders\/[^/]+\/[^/]+\//);
- if (isOldBuildPage) {
+ url.pathname.match(/^\/p\/[^/]+\/builders\/[^/]+\/[^/]+\//) ||
+ // Invocation link.
+ url.pathname.match(/^\/inv\//) ||
+ // Artifact link.
+ url.pathname.match(/^\/artifact\//);
+
+ if (isResultUI) {
url.pathname = '/ui' + url.pathname;
event.respondWith(Response.redirect(url.toString()));
return;
diff --git a/milo/frontend/routes.go b/milo/frontend/routes.go
index ba1cfe8..b2837a2 100644
--- a/milo/frontend/routes.go
+++ b/milo/frontend/routes.go
@@ -19,9 +19,11 @@
"fmt"
"html/template"
"net/http"
+ "net/url"
"strings"
"time"
+ "github.com/julienschmidt/httprouter"
"go.chromium.org/luci/appengine/gaeauth/server"
"go.chromium.org/luci/appengine/gaemiddleware"
"go.chromium.org/luci/appengine/gaemiddleware/standard"
@@ -79,6 +81,12 @@
r.GET("/internal/cron/update-config", cronMW, UpdateConfigHandler)
r.GET("/internal/cron/update-pools", cronMW, cronHandler(buildbucket.UpdatePools))
+ // Artifacts.
+ r.GET("/artifact/:view_type/*_artifact_name", baseMW, redirect("/ui/artifact/:view_type/*_artifact_name", http.StatusFound))
+
+ // Invocations.
+ r.GET("/inv/*path", baseMW, redirect("/ui/inv/*path", http.StatusFound))
+
// Builds.
r.GET("/b/:id", htmlMW, handleError(redirectLUCIBuild))
r.GET("/p/:project/builds/b:id", baseMW, movedPermanently("/b/:id"))
@@ -189,16 +197,63 @@
panic("pathTemplate must start with /")
}
+ interpolator := createInterpolator(pathTemplate)
return func(c *router.Context) {
- parts := strings.Split(pathTemplate, "/")
- for i, p := range parts {
+ path := interpolator(c.Params)
+ http.Redirect(c.Writer, c.Request, path, status)
+ }
+}
+
+// createInterpolator returns a function that can replace the variables in the
+// pathTemplate with the provided params.
+func createInterpolator(pathTemplate string) func(params httprouter.Params) string {
+ templateParts := strings.Split(pathTemplate, "/")
+
+ return func(params httprouter.Params) string {
+ components := make([]string, 0, len(templateParts))
+
+ for _, p := range templateParts {
if strings.HasPrefix(p, ":") {
- parts[i] = c.Params.ByName(p[1:])
+ components = append(components, params.ByName(p[1:]))
+ } else if strings.HasPrefix(p, "*_") {
+ // httprouter uses the decoded URL path to perform routing
+ // (which defeats the whole purpose of encoding), so we have to
+ // use '*' to capture a path component containing %2F.
+ // "*_" is a special syntax to signal that although we are
+ // capturing all characters till the end of the path, the
+ // captured value should be treated as a single path component,
+ // therefore '/' should also be encoded.
+ //
+ // Caveat: because '*' is used, this hack only works for the
+ // last path component.
+ //
+ // https://github.com/julienschmidt/httprouter/issues/284
+ component := params.ByName(p[1:])
+ component = strings.TrimPrefix(component, "/")
+ components = append(components, component)
+ } else if strings.HasPrefix(p, "*") {
+ path := params.ByName(p[1:])
+ path = strings.TrimPrefix(path, "/")
+
+ // Split the path into components before passing them to
+ // url.PathEscape. Otherwise url.PathEscape will encode "/" into
+ // "%2F" because it escapes all non-safe characters in a path
+ // component (it should be renamed to url.PathComponentEscape).
+ components = append(components, strings.Split(path, "/")...)
+ } else {
+ components = append(components, p)
}
}
- u := *c.Request.URL
- u.Path = strings.Join(parts, "/")
- http.Redirect(c.Writer, c.Request, u.String(), status)
+
+ // Escape the path components ourselves.
+ // url.URL.String() should not be used because it escapes everything
+ // automatically except '/' making it impossible to have %2F (encoded
+ // '/') in a path component ('%2F' will be double encoded to '%252F'
+ // while '/' won't be encoded at all).
+ for i, p := range components {
+ components[i] = url.PathEscape(p)
+ }
+ return strings.Join(components, "/")
}
}
diff --git a/milo/frontend/routes_test.go b/milo/frontend/routes_test.go
index 11e55ad..cc53334 100644
--- a/milo/frontend/routes_test.go
+++ b/milo/frontend/routes_test.go
@@ -32,6 +32,7 @@
"github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/ptypes"
"github.com/golang/protobuf/ptypes/timestamp"
+ "github.com/julienschmidt/httprouter"
"go.chromium.org/luci/auth/identity"
buildbucketpb "go.chromium.org/luci/buildbucket/proto"
@@ -495,3 +496,31 @@
result := &buildbucketpb.Build{}
return result, jsonpb.Unmarshal(f, result)
}
+
+func TestCreateInterpolator(t *testing.T) {
+ Convey("Test createInterpolator", t, func() {
+ Convey("Should encode params", func() {
+ params := httprouter.Params{httprouter.Param{Key: "component2", Value: ":? +"}}
+ interpolator := createInterpolator("/component1/:component2")
+
+ path := interpolator(params)
+ So(path, ShouldEqual, "/component1/"+url.PathEscape(":? +"))
+ })
+
+ Convey("Should support catching path segments with *", func() {
+ params := httprouter.Params{httprouter.Param{Key: "component2", Value: "/:?/ +"}}
+ interpolator := createInterpolator("/component1/*component2")
+
+ path := interpolator(params)
+ So(path, ShouldEqual, "/component1/"+url.PathEscape(":?")+"/"+url.PathEscape(" +"))
+ })
+
+ Convey("Should support encoding / with *_", func() {
+ params := httprouter.Params{httprouter.Param{Key: "_component2", Value: "/:?/ +"}}
+ interpolator := createInterpolator("/component1/*_component2")
+
+ path := interpolator(params)
+ So(path, ShouldEqual, "/component1/"+url.PathEscape(":?/ +"))
+ })
+ })
+}
