Docserver: Properly qualify sidebar paths so that _patch/12345667 links are

maintained. Make the SidenavDataSource tests slightly more sensible.

BUG=304302
R=jyasskin@chromium.org
NOTRY=true

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

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

11 files changed, 132 insertions, 102 deletions

diff --git a/chrome/common/extensions/docs/server2/app.yaml b/chrome/common/extensions/docs/server2/app.yaml
index e2813f2..6a325609 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: 2-31-0
+version: 2-31-1
 runtime: python27
 api_version: 1
 threadsafe: false
diff --git a/chrome/common/extensions/docs/server2/cron.yaml b/chrome/common/extensions/docs/server2/cron.yaml
index 001f549..2fd2f74 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: 2-31-0
+  target: 2-31-1
diff --git a/chrome/common/extensions/docs/server2/cron_servlet.py b/chrome/common/extensions/docs/server2/cron_servlet.py
index 328ef20..1051ba4 100644
--- a/chrome/common/extensions/docs/server2/cron_servlet.py
+++ b/chrome/common/extensions/docs/server2/cron_servlet.py
@@ -269,7 +269,6 @@ class CronServlet(Servlet):
     return ServerInstance(object_store_creator,
                           host_file_system,
                           app_samples_file_system,
-                          '',
                           compiled_host_fs_factory,
                           branch_utility,
                           host_file_system_creator)
diff --git a/chrome/common/extensions/docs/server2/instance_servlet.py b/chrome/common/extensions/docs/server2/instance_servlet.py
index 47a9be4..c39ffab 100644
--- a/chrome/common/extensions/docs/server2/instance_servlet.py
+++ b/chrome/common/extensions/docs/server2/instance_servlet.py
@@ -46,7 +46,6 @@ class OfflineRenderServletDelegate(RenderServlet.Delegate):
     return ServerInstance(object_store_creator,
                           host_file_system,
                           app_samples_file_system,
-                          '',
                           compiled_host_fs_factory,
                           branch_utility,
                           host_file_system_creator)
diff --git a/chrome/common/extensions/docs/server2/patch_servlet.py b/chrome/common/extensions/docs/server2/patch_servlet.py
index a394a17..5680adc 100644
--- a/chrome/common/extensions/docs/server2/patch_servlet.py
+++ b/chrome/common/extensions/docs/server2/patch_servlet.py
@@ -60,10 +60,10 @@ class _PatchServletDelegate(RenderServlet.Delegate):
                           patched_file_system,
                           self._delegate.CreateAppSamplesFileSystem(
                               object_store_creator),
-                          '/_patch/%s' % self._issue,
                           compiled_fs_factory,
                           branch_utility,
-                          host_file_system_creator)
+                          host_file_system_creator,
+                          base_path='/_patch/%s/' % self._issue)
 
 class PatchServlet(Servlet):
   '''Servlet which renders patched docs.
diff --git a/chrome/common/extensions/docs/server2/patch_servlet_test.py b/chrome/common/extensions/docs/server2/patch_servlet_test.py
index 70a6f2f..5ca3bb2 100755
--- a/chrome/common/extensions/docs/server2/patch_servlet_test.py
+++ b/chrome/common/extensions/docs/server2/patch_servlet_test.py
@@ -45,12 +45,16 @@ class PatchServletTest(unittest.TestCase):
                          _RenderServletDelegate()).Get()
 
   def _RenderAndCheck(self, path, issue, expected_equal):
+    '''Renders |path| with |issue| patched in and asserts that the result is
+    the same as |expected_equal| modulo any links that get rewritten to
+    "_patch/issue".
+    '''
     patched_response = self._RenderWithPatch(path, issue)
     unpatched_response = self._RenderWithoutPatch(path)
     patched_response.headers.pop('cache-control', None)
     unpatched_response.headers.pop('cache-control', None)
     patched_content = patched_response.content.ToString().replace(
-        '/_patch/%s/' % issue, '/')
+        '/_patch/%s' % issue, '')
     unpatched_content = unpatched_response.content.ToString()
 
     self.assertEqual(patched_response.status, unpatched_response.status)
diff --git a/chrome/common/extensions/docs/server2/server_instance.py b/chrome/common/extensions/docs/server2/server_instance.py
index 4b8cea8..259187f 100644
--- a/chrome/common/extensions/docs/server2/server_instance.py
+++ b/chrome/common/extensions/docs/server2/server_instance.py
@@ -30,10 +30,32 @@ class ServerInstance(object):
                object_store_creator,
                host_file_system,
                app_samples_file_system,
-               base_path,
                compiled_fs_factory,
                branch_utility,
-               host_file_system_creator):
+               host_file_system_creator,
+               base_path='/'):
+    '''
+    |object_store_creator|
+        The ObjectStoreCreator used to create almost all caches.
+    |host_file_system|
+        The main FileSystem instance which hosts the server, its templates, and
+        most App/Extension content. Probably a SubversionFileSystem.
+    |app_samples_file_system|
+        The FileSystem instance which hosts the App samples.
+    |compiled_fs_factory|
+        Factory used to create CompiledFileSystems, a higher-level cache type
+        than ObjectStores. This can usually be derived from
+        |object_store_creator| and |host_file_system| but under special
+        circumstances a different implementation needs to be passed in.
+    |branch_utility|
+        Has knowledge of Chrome branches, channels, and versions.
+    |host_file_system_creator|
+        Creates FileSystem instances which host the server at alternative
+        revisions.
+    |base_path|
+        The path which all HTML is generated relative to. Usually this is /
+        but some servlets need to override this.
+    '''
     self.object_store_creator = object_store_creator
 
     self.host_file_system = host_file_system
@@ -44,6 +66,9 @@ class ServerInstance(object):
 
     self.host_file_system_creator = host_file_system_creator
 
+    assert base_path.startswith('/') and base_path.endswith('/')
+    self.base_path = base_path
+
     self.host_file_system_iterator = HostFileSystemIterator(
         host_file_system_creator,
         host_file_system,
@@ -120,7 +145,6 @@ class ServerInstance(object):
         svn_constants.PUBLIC_TEMPLATE_PATH)
 
     self.strings_json_path = svn_constants.STRINGS_JSON_PATH
-    self.sidenav_json_base_path = svn_constants.JSON_PATH
     self.manifest_json_path = svn_constants.MANIFEST_JSON_PATH
     self.manifest_features_path = svn_constants.MANIFEST_FEATURES_PATH
 
@@ -142,17 +166,17 @@ class ServerInstance(object):
         self.template_data_source_factory)
 
   @staticmethod
-  def ForTest(file_system):
+  def ForTest(file_system, base_path='/'):
     object_store_creator = ObjectStoreCreator.ForTest()
     return ServerInstance(object_store_creator,
                           file_system,
                           EmptyDirFileSystem(),
-                          '',
                           CompiledFileSystem.Factory(file_system,
                                                      object_store_creator),
                           TestBranchUtility.CreateWithCannedData(),
                           HostFileSystemCreator.ForTest(file_system,
-                                                        object_store_creator))
+                                                        object_store_creator),
+                          base_path=base_path)
 
   @staticmethod
   def ForLocal():
@@ -165,7 +189,6 @@ class ServerInstance(object):
         object_store_creator,
         trunk_file_system,
         EmptyDirFileSystem(),
-        '',
         CompiledFileSystem.Factory(trunk_file_system, object_store_creator),
         TestBranchUtility.CreateWithCannedData(),
         host_file_system_creator)
diff --git a/chrome/common/extensions/docs/server2/sidenav_data_source.py b/chrome/common/extensions/docs/server2/sidenav_data_source.py
index 6d09093..7574e98 100644
--- a/chrome/common/extensions/docs/server2/sidenav_data_source.py
+++ b/chrome/common/extensions/docs/server2/sidenav_data_source.py
@@ -7,6 +7,7 @@ import logging
 
 from data_source import DataSource
 from third_party.json_schema_compiler.json_parse import Parse
+from svn_constants import JSON_PATH
 
 
 def _AddLevels(items, level):
@@ -25,7 +26,7 @@ def _AddSelected(items, path):
   True if an item was marked 'selected'.
   '''
   for item in items:
-    if item.get('href', '') == '/' + path:
+    if item.get('href', '') == path:
       item['selected'] = True
       return True
     if 'items' in item:
@@ -36,47 +37,47 @@ def _AddSelected(items, path):
   return False
 
 
-def _QualifyHrefs(items):
-  '''Force hrefs in |items| to either be absolute (http://...) or qualified
-  (begins with a slash (/)). Other hrefs emit a warning and should be updated.
-  '''
-  for item in items:
-    if 'items' in item:
-      _QualifyHrefs(item['items'])
-
-    href = item.get('href')
-    if href is not None and not href.startswith(('http://', 'https://')):
-      if not href.startswith('/'):
-        logging.warn('Paths in sidenav must be qualified. %s is not.' % href)
-        href = '/' + href
-      item['href'] = href
-
-
-def _CreateSidenavDict(_, content):
-  items = Parse(content)
-  # Start at level 2, the top <ul> element is level 1.
-  _AddLevels(items, level=2)
-  _QualifyHrefs(items)
-  return items
-
-
 class SidenavDataSource(DataSource):
   '''Provides templates with access to JSON files used to create the side
   navigation bar.
   '''
   def __init__(self, server_instance, request):
     self._cache = server_instance.compiled_host_fs_factory.Create(
-        _CreateSidenavDict, SidenavDataSource)
-    self._json_path = server_instance.sidenav_json_base_path
+        self._CreateSidenavDict, SidenavDataSource)
+    self._server_instance = server_instance
     self._request = request
 
+  def _CreateSidenavDict(self, _, content):
+    items = Parse(content)
+    # Start at level 2, the top <ul> element is level 1.
+    _AddLevels(items, level=2)
+    self._QualifyHrefs(items)
+    return items
+
+  def _QualifyHrefs(self, items):
+    '''Force hrefs in |items| to either be absolute (http://...) or qualified
+    (beginning with /, in which case it will be moved relative to |base_path|).
+    Relative hrefs emit a warning and should be updated.
+    '''
+    for item in items:
+      if 'items' in item:
+        self._QualifyHrefs(item['items'])
+
+      href = item.get('href')
+      if href is not None and not href.startswith(('http://', 'https://')):
+        if not href.startswith('/'):
+          logging.warn('Paths in sidenav must be qualified. %s is not.' % href)
+        else:
+          href = href.lstrip('/')
+        item['href'] = self._server_instance.base_path + href
+
   def Cron(self):
-    for platform in ['apps', 'extensions']:
+    for platform in ('apps', 'extensions'):
       self._cache.GetFromFile(
-          '%s/%s_sidenav.json' % (self._json_path, platform))
+          '%s/%s_sidenav.json' % (JSON_PATH, platform))
 
   def get(self, key):
     sidenav = copy.deepcopy(self._cache.GetFromFile(
-        '%s/%s_sidenav.json' % (self._json_path, key)))
-    _AddSelected(sidenav, self._request.path)
+        '%s/%s_sidenav.json' % (JSON_PATH, key)))
+    _AddSelected(sidenav, self._server_instance.base_path + self._request.path)
     return sidenav
diff --git a/chrome/common/extensions/docs/server2/sidenav_data_source_test.py b/chrome/common/extensions/docs/server2/sidenav_data_source_test.py
index 50df92b..322752f 100755
--- a/chrome/common/extensions/docs/server2/sidenav_data_source_test.py
+++ b/chrome/common/extensions/docs/server2/sidenav_data_source_test.py
@@ -8,20 +8,13 @@ import unittest
 
 from compiled_file_system import CompiledFileSystem
 from object_store_creator import ObjectStoreCreator
+from server_instance import ServerInstance
 from servlet import Request
-from sidenav_data_source import (
-    SidenavDataSource, _AddLevels, _AddSelected, _QualifyHrefs)
+from sidenav_data_source import SidenavDataSource, _AddLevels, _AddSelected
 from test_file_system import TestFileSystem
 from test_util import CaptureLogging
 
 
-class FakeServerInstance(object):
-  def __init__(self, file_system):
-    self.compiled_host_fs_factory = CompiledFileSystem.Factory(
-        file_system, ObjectStoreCreator.ForTest())
-    self.sidenav_json_base_path = ''
-
-
 class SamplesDataSourceTest(unittest.TestCase):
   def testAddLevels(self):
     sidenav_json = [{
@@ -68,35 +61,43 @@ class SamplesDataSourceTest(unittest.TestCase):
       }
     ]
 
-    _AddSelected(sidenav_json, 'H3.html')
+    _AddSelected(sidenav_json, '/H3.html')
     self.assertEqual(expected, sidenav_json)
 
-  def testQualifyHrefs(self):
-    sidenav_json = [
-      { 'href': '/qualified/H1.html' },
-      { 'href': 'https://qualified/X1.html' },
-      {
-        'href': 'H2.html',
-        'items': [{
-          'href': 'H3.html'
-        }]
-      }
-    ]
+  def testWithDifferentBasePath(self):
+    file_system = TestFileSystem({
+      'apps_sidenav.json': json.dumps([
+        { 'href': '/H1.html' },
+        { 'href': '/H2.html' },
+        { 'href': '/base/path/H2.html' },
+        { 'href': 'https://qualified/X1.html' },
+        {
+          'href': 'H3.html',
+          'items': [{
+            'href': 'H4.html'
+          }]
+        },
+      ])
+    }, relative_to='docs/templates/json')
 
     expected = [
-      { 'href': '/qualified/H1.html' },
-      { 'href': 'https://qualified/X1.html' },
-      {
-        'href': '/H2.html',
-        'items': [{
-          'href': '/H3.html'
-        }]
-      }
+      {'href': '/base/path/H1.html', 'level': 2},
+      {'href': '/base/path/H2.html', 'level': 2, 'selected': True},
+      {'href': '/base/path/base/path/H2.html', 'level': 2},
+      {'href': 'https://qualified/X1.html', 'level': 2},
+      {'items': [
+        {'href': '/base/path/H4.html', 'level': 3}
+      ],
+      'href': '/base/path/H3.html', 'level': 2}
     ]
 
-    log_output = CaptureLogging(lambda: _QualifyHrefs(sidenav_json))
+    server_instance = ServerInstance.ForTest(file_system,
+                                             base_path='/base/path/')
+    sidenav_data_source = SidenavDataSource(server_instance,
+                                            Request.ForTest('/H2.html'))
 
-    self.assertEqual(expected, sidenav_json)
+    log_output = CaptureLogging(
+        lambda: self.assertEqual(expected, sidenav_data_source.get('apps')))
     self.assertEqual(2, len(log_output))
 
   def testSidenavDataSource(self):
@@ -109,7 +110,7 @@ class SamplesDataSourceTest(unittest.TestCase):
           'href': '/H2.html'
         }]
       }])
-    })
+    }, relative_to='docs/templates/json')
 
     expected = [{
       'level': 2,
@@ -125,7 +126,7 @@ class SamplesDataSourceTest(unittest.TestCase):
     }]
 
     sidenav_data_source = SidenavDataSource(
-        FakeServerInstance(file_system), Request.ForTest('/H2.html'))
+        ServerInstance.ForTest(file_system), Request.ForTest('/H2.html'))
 
     log_output = CaptureLogging(
         lambda: self.assertEqual(expected, sidenav_data_source.get('apps')))
@@ -138,18 +139,18 @@ class SamplesDataSourceTest(unittest.TestCase):
     file_system = TestFileSystem({
       'apps_sidenav.json': '[{ "title": "H1" }]' ,
       'extensions_sidenav.json': '[{ "title": "H2" }]'
-    })
+    }, relative_to='docs/templates/json')
 
     # Ensure Cron doesn't rely on request.
     sidenav_data_source = SidenavDataSource(
-        FakeServerInstance(file_system), request=None)
+        ServerInstance.ForTest(file_system), request=None)
     sidenav_data_source.Cron()
 
     # If Cron fails, apps_sidenav.json will not be cached, and the _cache_data
     # access will fail.
     # TODO(jshumway): Make a non hack version of this check.
     sidenav_data_source._cache._file_object_store.Get(
-        '/apps_sidenav.json').Get()._cache_data
+        'docs/templates/json/apps_sidenav.json').Get()._cache_data
 
 
 if __name__ == '__main__':
diff --git a/chrome/common/extensions/docs/server2/test_file_system.py b/chrome/common/extensions/docs/server2/test_file_system.py
index 621de6e..35ae153 100644
--- a/chrome/common/extensions/docs/server2/test_file_system.py
+++ b/chrome/common/extensions/docs/server2/test_file_system.py
@@ -5,6 +5,20 @@
 from file_system import FileSystem, FileNotFoundError, StatInfo
 from future import Future
 
+
+def _MoveTo(base, obj):
+  '''Returns an object as |obj| moved to |base|. That is,
+  _MoveTo('foo/bar', {'a': 'b'}) -> {'foo': {'bar': {'a': 'b'}}}
+  '''
+  result = {}
+  leaf = result
+  for k in base.split('/'):
+    leaf[k] = {}
+    leaf = leaf[k]
+  leaf.update(obj)
+  return result
+
+
 class TestFileSystem(FileSystem):
   '''A FileSystem backed by an object. Create with an object representing file
   paths such that {'a': {'b': 'hello'}} will resolve Read('a/b') as 'hello',
@@ -12,24 +26,9 @@ class TestFileSystem(FileSystem):
   IncrementStat.
   '''
 
-  # TODO(kalman): this method would be unnecessary if we injected paths properly
-  # in ServerInstance.
-  @staticmethod
-  def MoveTo(base, obj):
-    '''Returns an object as |obj| moved to |base|. That is,
-    MoveTo('foo/bar', {'a': 'b'}) -> {'foo': {'bar': {'a': 'b'}}}
-    '''
-    result = {}
-    leaf = result
-    for k in base.split('/'):
-      leaf[k] = {}
-      leaf = leaf[k]
-    leaf.update(obj)
-    return result
-
-  def __init__(self, obj):
+  def __init__(self, obj, relative_to=None):
     assert obj is not None
-    self._obj = obj
+    self._obj = obj if relative_to is None else _MoveTo(relative_to, obj)
     self._path_stats = {}
     self._global_stat = 0
 
diff --git a/chrome/common/extensions/docs/server2/test_file_system_test.py b/chrome/common/extensions/docs/server2/test_file_system_test.py
index bca4a03..a0fba35 100755
--- a/chrome/common/extensions/docs/server2/test_file_system_test.py
+++ b/chrome/common/extensions/docs/server2/test_file_system_test.py
@@ -5,9 +5,10 @@
 
 from copy import deepcopy
 from file_system import FileNotFoundError, StatInfo
-from test_file_system import TestFileSystem
+from test_file_system import TestFileSystem, _MoveTo
 import unittest
 
+
 _TEST_DATA = {
   '404.html': '404.html contents',
   'apps': {
@@ -23,11 +24,13 @@ _TEST_DATA = {
   }
 }
 
+
 def _Get(fn):
   '''Returns a function which calls Future.Get on the result of |fn|.
   '''
   return lambda *args: fn(*args).Get()
 
+
 class TestFileSystemTest(unittest.TestCase):
   def testEmptyFileSystem(self):
     self._TestMetasyntacticPaths(TestFileSystem({}))
@@ -114,11 +117,12 @@ class TestFileSystemTest(unittest.TestCase):
 
   def testMoveTo(self):
     self.assertEqual({'foo': {'a': 'b', 'c': 'd'}},
-                    TestFileSystem.MoveTo('foo', {'a': 'b', 'c': 'd'}))
+                     _MoveTo('foo', {'a': 'b', 'c': 'd'}))
     self.assertEqual({'foo': {'bar': {'a': 'b', 'c': 'd'}}},
-                    TestFileSystem.MoveTo('foo/bar', {'a': 'b', 'c': 'd'}))
+                     _MoveTo('foo/bar', {'a': 'b', 'c': 'd'}))
     self.assertEqual({'foo': {'bar': {'baz': {'a': 'b'}}}},
-                    TestFileSystem.MoveTo('foo/bar/baz', {'a': 'b'}))
+                     _MoveTo('foo/bar/baz', {'a': 'b'}))
+
 
 if __name__ == '__main__':
   unittest.main()