Docserver: Fix Redirector to return relative paths when relative redirects are

specified, rather than always absolute redirects. Also fix it not to crash when
directories without redirects are fetched.

R=yoz@chromium.org

Review URL: https://codereview.chromium.org/174393003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@252432 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 40 insertions, 13 deletions

diff --git a/chrome/common/extensions/docs/server2/app.yaml b/chrome/common/extensions/docs/server2/app.yaml
index 3685aea..76bd151 100644
--- a/chrome/common/extensions/docs/server2/app.yaml
+++ b/chrome/common/extensions/docs/server2/app.yaml
@@ -1,5 +1,5 @@
 application: chrome-apps-doc
-version: 3-12-2
+version: 3-13-0
 runtime: python27
 api_version: 1
 threadsafe: false
diff --git a/chrome/common/extensions/docs/server2/content_providers_test.py b/chrome/common/extensions/docs/server2/content_providers_test.py
index 48461af..267119d 100755
--- a/chrome/common/extensions/docs/server2/content_providers_test.py
+++ b/chrome/common/extensions/docs/server2/content_providers_test.py
@@ -145,6 +145,11 @@ class ContentProvidersTest(unittest.TestCase):
     self.assertEqual('apples-dir', serve_from)
     self.assertEqual('', path)
     provider, serve_from, path = self._content_providers.GetByServeFrom(
+        'apples-dir/')
+    self.assertEqual('apples', provider.name)
+    self.assertEqual('apples-dir', serve_from)
+    self.assertEqual('', path)
+    provider, serve_from, path = self._content_providers.GetByServeFrom(
         'apples-dir/are/forever')
     self.assertEqual('apples', provider.name)
     self.assertEqual('apples-dir', serve_from)
diff --git a/chrome/common/extensions/docs/server2/cron.yaml b/chrome/common/extensions/docs/server2/cron.yaml
index 53751371..e496ef0 100644
--- a/chrome/common/extensions/docs/server2/cron.yaml
+++ b/chrome/common/extensions/docs/server2/cron.yaml
@@ -2,4 +2,4 @@ cron:
 - description: Repopulates all cached data.
   url: /_cron
   schedule: every 5 minutes
-  target: 3-12-2
+  target: 3-13-0
diff --git a/chrome/common/extensions/docs/server2/redirector.py b/chrome/common/extensions/docs/server2/redirector.py
index acbc160..cd54d75 100644
--- a/chrome/common/extensions/docs/server2/redirector.py
+++ b/chrome/common/extensions/docs/server2/redirector.py
@@ -42,7 +42,7 @@ class Redirector(object):
         urlsplit(redirect).scheme in ('http', 'https')):
       return redirect
 
-    return posixpath.normpath(posixpath.join('/', dirname, redirect))
+    return posixpath.normpath(posixpath.join(dirname, redirect))
 
   def _RedirectOldHosts(self, host, path):
     ''' Redirect paths from the old code.google.com to the new
diff --git a/chrome/common/extensions/docs/server2/redirector_test.py b/chrome/common/extensions/docs/server2/redirector_test.py
index 0eb404e..f80ab9b 100755
--- a/chrome/common/extensions/docs/server2/redirector_test.py
+++ b/chrome/common/extensions/docs/server2/redirector_test.py
@@ -23,7 +23,8 @@ file_system = TestFileSystem({
   'apps': {
     'redirects.json': json.dumps({
       '': '../index.html',
-      'index.html': 'about_apps.html'
+      'index.html': 'about_apps.html',
+      'foo.html': '/bar.html',
     })
   },
   'extensions': {
@@ -55,22 +56,24 @@ class RedirectorTest(unittest.TestCase):
 
   def testAbsoluteRedirection(self):
     self.assertEqual(
-        '/apps/about_apps.html',
-        self._redirector.Redirect(HOST, 'apps/index.html'))
-    self.assertEqual(
         '/index.html', self._redirector.Redirect(HOST, ''))
     self.assertEqual(
-        '/index.html', self._redirector.Redirect(HOST, 'home'))
+        '/bar.html', self._redirector.Redirect(HOST, 'apps/foo.html'))
 
   def testRelativeRedirection(self):
     self.assertEqual(
-        '/extensions/manifest.html',
+        'apps/about_apps.html',
+        self._redirector.Redirect(HOST, 'apps/index.html'))
+    self.assertEqual(
+        'extensions/manifest.html',
         self._redirector.Redirect(HOST, 'extensions/manifest/'))
     self.assertEqual(
-        '/extensions/manifest.html',
+        'extensions/manifest.html',
         self._redirector.Redirect(HOST, 'extensions/manifest'))
     self.assertEqual(
-        '/index.html', self._redirector.Redirect(HOST, 'apps/'))
+        'index.html', self._redirector.Redirect(HOST, 'apps/'))
+    self.assertEqual(
+        'index.html', self._redirector.Redirect(HOST, 'home'))
 
   def testNotFound(self):
     self.assertEqual(
diff --git a/chrome/common/extensions/docs/server2/render_servlet.py b/chrome/common/extensions/docs/server2/render_servlet.py
index a910d03..c714b5d 100644
--- a/chrome/common/extensions/docs/server2/render_servlet.py
+++ b/chrome/common/extensions/docs/server2/render_servlet.py
@@ -70,20 +70,24 @@ class RenderServlet(Servlet):
       logging.warning('No 404.html found in %s' % path)
       return Response.NotFound('Not Found', headers=_MakeHeaders('text/plain'))
 
-  def _GetSuccessResponse(self, path, server_instance):
+  def _GetSuccessResponse(self, request_path, server_instance):
     '''Returns the Response from trying to render |path| with
     |server_instance|.  If |path| isn't found then a FileNotFoundError will be
     raised, such that the only responses that will be returned from this method
     are Ok and Redirect.
     '''
     content_provider, serve_from, path = (
-        server_instance.content_providers.GetByServeFrom(path))
+        server_instance.content_providers.GetByServeFrom(request_path))
     assert content_provider, 'No ContentProvider found for %s' % path
 
     redirect = Redirector(
         server_instance.compiled_fs_factory,
         content_provider.file_system).Redirect(self._request.host, path)
     if redirect is not None:
+      # Absolute redirects stay absolute, relative redirects are relative to
+      # |serve_from|; all redirects eventually need to be *served* as absolute.
+      if not redirect.startswith('/'):
+        redirect = '/' + posixpath.join(serve_from, redirect)
       return Response.Redirect(redirect, permanent=False)
 
     canonical_path = content_provider.GetCanonicalPath(path)
@@ -91,6 +95,16 @@ class RenderServlet(Servlet):
       redirect_path = posixpath.join(serve_from, canonical_path)
       return Response.Redirect('/' + redirect_path, permanent=False)
 
+    if request_path.endswith('/'):
+      # Directory request hasn't been redirected by now. Default behaviour is
+      # to redirect as though it were a file.
+      return Response.Redirect('/' + request_path.rstrip('/'),
+                               permanent=False)
+
+    if not path:
+      # Empty-path request hasn't been redirected by now. It doesn't exist.
+      raise FileNotFoundError('Empty path')
+
     content_and_type = content_provider.GetContentAndType(path).Get()
     if not content_and_type.content:
       logging.error('%s had empty content' % path)
diff --git a/chrome/common/extensions/docs/server2/render_servlet_test.py b/chrome/common/extensions/docs/server2/render_servlet_test.py
index 0550b9e..1857028 100755
--- a/chrome/common/extensions/docs/server2/render_servlet_test.py
+++ b/chrome/common/extensions/docs/server2/render_servlet_test.py
@@ -105,6 +105,11 @@ class RenderServletTest(unittest.TestCase):
     self.assertEqual(('/apps/tags/webview', False),
                      response.GetRedirect())
 
+  def testDirectories(self):
+    # Directories should be redirected to a URL that doesn't end in a '/'
+    # whether or not that exists.
+    self.assertEqual(('/dir', False), self._Render('dir/').GetRedirect())
+
 
 if __name__ == '__main__':
   unittest.main()