application: add redirect to ensure trailing slash (#2734)

Summary:
When using a `--path_prefix`, TensorBoard has historically required a
trailing slash after that prefix: e.g., one visits `localhost:6006/foo/`
rather than `localhost:6006/foo`. (See, for instance, #1176.) Different
versions of TensorBoard have different failure modes when the non-slash
path is loaded; currently, the TensorBoard shell loads, but the frontend
computes relative URLs incorrectly. This commit adds a redirect from the
empty path to `/` to avoid the problem.

This logic could be inlined into the `PathPrefixMiddleware`, but we’ll
soon have other middlewares with similar needs, so providing this as its
own middleware avoids duplicating the functionality.

Test Plan:
Launch TensorBoard with `--path_prefix /foo` (or `--path_prefix /foo/`;
the two are equivalent) and navigate to `/foo/` and `/foo` in a browser.
Note that they now both work and resolve to `/foo/`, while prior to this
commit navigating to `/foo` yielded a broken TensorBoard.

wchargin-branch: empty-path-redirect

tensorboard/backend/BUILD

@@ -62,6 +62,7 @@ py_library(
     srcs_version = "PY2AND3",
     visibility = ["//visibility:public"],
     deps = [
+        ":empty_path_redirect",
         ":http_util",
         ":path_prefix",
         "//tensorboard:errors",
@@ -98,6 +99,25 @@ py_test(
     ],
 )
 
+py_library(
+    name = "empty_path_redirect",
+    srcs = ["empty_path_redirect.py"],
+    srcs_version = "PY2AND3",
+)
+
+py_test(
+    name = "empty_path_redirect_test",
+    size = "small",
+    srcs = ["empty_path_redirect_test.py"],
+    srcs_version = "PY2AND3",
+    tags = ["support_notf"],
+    deps = [
+        ":empty_path_redirect",
+        "//tensorboard:test",
+        "@org_pocoo_werkzeug",
+    ],
+)
+
 py_library(
     name = "path_prefix",
     srcs = ["path_prefix.py"],

tensorboard/backend/application.py

@@ -39,6 +39,7 @@
 from werkzeug import wrappers
 
 from tensorboard import errors
+from tensorboard.backend import empty_path_redirect
 from tensorboard.backend import http_util
 from tensorboard.backend import path_prefix
 from tensorboard.backend.event_processing import db_import_multiplexer
@@ -329,6 +330,7 @@ def __init__(self, plugins, path_prefix=''):
   def _create_wsgi_app(self):
     """Apply middleware to create the final WSGI app."""
     app = self._route_request
+    app = empty_path_redirect.EmptyPathRedirectMiddleware(app)
     app = path_prefix.PathPrefixMiddleware(app, self._path_prefix)
     app = _handling_errors(app)
     return app

tensorboard/backend/application_test.py

@@ -302,9 +302,9 @@ def _get_json(self, path):
     return json.loads(response.get_data().decode('utf-8'))
 
   def testBaseUrlRequest(self):
-    """Request a page that doesn't exist; it should 404."""
+    """Base URL should redirect to "/" for proper relative URLs."""
     response = self.server.get(self.path_prefix)
-    self.assertEqual(404, response.status_code)
+    self.assertEqual(301, response.status_code)
 
   def testBaseUrlRequestNonexistentPage(self):
     """Request a page that doesn't exist; it should 404."""
@@ -680,7 +680,7 @@ def testMissingRoute(self):
     self._test_route('/data/plugin/foo/bogus', 404)
 
   def testEmptyRoute(self):
-    self._test_route('', 404)
+    self._test_route('', 301)
 
   def testSlashlessRoute(self):
     self._test_route('runaway', 404)

tensorboard/backend/empty_path_redirect.py

@@ -0,0 +1,50 @@
+# Copyright 2019 The TensorFlow Authors. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+"""Redirect from an empty path to the virtual application root.
+
+Sometimes, middleware transformations will make the path empty: for
+example, navigating to "/foo" (no trailing slash) when the path prefix
+is exactly "/foo". In such cases, relative links on the frontend would
+break. Instead of handling this special case in each relevant
+middleware, we install a top-level redirect handler from "" to "/".
+
+This middleware respects `SCRIPT_NAME` as described by the WSGI spec. If
+`SCRIPT_NAME` is set to "/foo", then an empty `PATH_INFO` corresponds to
+the actual path "/foo", and so will be redirected to "/foo/".
+"""
+
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+
+
+class EmptyPathRedirectMiddleware(object):
+  """WSGI middleware to redirect from "" to "/"."""
+
+  def __init__(self, application):
+    """Initializes this middleware.
+
+    Args:
+      application: The WSGI application to wrap (see PEP 3333).
+    """
+    self._application = application
+
+  def __call__(self, environ, start_response):
+    path = environ.get("PATH_INFO", "")
+    if path:
+      return self._application(environ, start_response)
+    location = environ.get("SCRIPT_NAME", "") + "/"
+    start_response("301 Moved Permanently", [("Location", location)])
+    return []

tensorboard/backend/empty_path_redirect_test.py

@@ -0,0 +1,72 @@
+# Copyright 2019 The TensorFlow Authors. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+"""Tests for `tensorboard.backend.empty_path_redirect`."""
+
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+
+import json
+
+import werkzeug
+
+from tensorboard import test as tb_test
+from tensorboard.backend import empty_path_redirect
+
+
+class EmptyPathRedirectMiddlewareTest(tb_test.TestCase):
+  """Tests for `EmptyPathRedirectMiddleware`."""
+
+  def setUp(self):
+    super(EmptyPathRedirectMiddlewareTest, self).setUp()
+    app = werkzeug.Request.application(lambda req: werkzeug.Response(req.path))
+    app = empty_path_redirect.EmptyPathRedirectMiddleware(app)
+    app = self._lax_strip_foo_middleware(app)
+    self.app = app
+    self.server = werkzeug.test.Client(self.app, werkzeug.BaseResponse)
+
+  def _lax_strip_foo_middleware(self, app):
+    """Strips a `/foo` prefix if it exists; no-op otherwise."""
+
+    def wrapper(environ, start_response):
+      path = environ.get("PATH_INFO", "")
+      if path.startswith("/foo"):
+        environ["PATH_INFO"] = path[len("/foo") :]
+        environ["SCRIPT_NAME"] = "/foo"
+      return app(environ, start_response)
+
+    return wrapper
+
+  def test_normal_route_not_redirected(self):
+    response = self.server.get("/foo/bar")
+    self.assertEqual(response.status_code, 200)
+
+  def test_slash_not_redirected(self):
+    response = self.server.get("/foo/")
+    self.assertEqual(response.status_code, 200)
+
+  def test_empty_redirected_with_script_name(self):
+    response = self.server.get("/foo")
+    self.assertEqual(response.status_code, 301)
+    self.assertEqual(response.headers["Location"], "/foo/")
+
+  def test_empty_redirected_with_blank_script_name(self):
+    response = self.server.get("")
+    self.assertEqual(response.status_code, 301)
+    self.assertEqual(response.headers["Location"], "/")
+
+
+if __name__ == "__main__":
+  tb_test.main()