Extensions Docs Server: BranchUtility not fetching branch numbers correctly

BranchUtility was crashing when it tried to fetch the branch numbers for the
dev, beta, and stable channels. There was also a problem using the branch number
where the name should have been used.

ServerInstances are no longer leaked.

BUG=141909

Review URL: https://chromiumcodereview.appspot.com/10831269

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

chrome/common/extensions/docs/server2/appengine_url_fetcher.py

@@ -23,11 +23,17 @@ class AppEngineUrlFetcher(object):
   def Fetch(self, url):
     """Fetches a file synchronously.
     """
-    return urlfetch.fetch(self._base_path + '/' + url)
+    if self._base_path is not None:
+      return urlfetch.fetch(self._base_path + '/' + url)
+    else:
+      return urlfetch.fetch(url)
 
   def FetchAsync(self, url):
     """Fetches a file asynchronously, and returns a Future with the result.
     """
     rpc = urlfetch.create_rpc()
-    urlfetch.make_fetch_call(rpc, self._base_path + '/' + url)
+    if self._base_path is not None:
+      urlfetch.make_fetch_call(rpc, self._base_path + '/' + url)
+    else:
+      urlfetch.make_fetch_call(rpc, url)
     return Future(delegate=_AsyncFetchDelegate(rpc))

chrome/common/extensions/docs/server2/appengine_wrappers.py

@@ -13,6 +13,59 @@ try:
   import google.appengine.api.memcache as memcache
   import google.appengine.api.urlfetch as urlfetch
 except ImportError:
+  import re
+
+  FAKE_URL_FETCHER_CONFIGURATION = None
+
+  def ConfigureFakeUrlFetch(configuration):
+    """|configuration| is a dictionary mapping strings to fake urlfetch classes.
+    A fake urlfetch class just needs to have a fetch method. The keys of the
+    dictionary are treated as regex, and they are matched with the URL to
+    determine which fake urlfetch is used.
+    """
+    global FAKE_URL_FETCHER_CONFIGURATION
+    FAKE_URL_FETCHER_CONFIGURATION = dict(
+        (re.compile(k), v) for k, v in configuration.iteritems())
+
+  def _GetConfiguration(key):
+    if not FAKE_URL_FETCHER_CONFIGURATION:
+      raise ValueError('No fake fetch paths have been configured. '
+                       'See ConfigureFakeUrlFetch in appengine_wrappers.py.')
+    for k, v in FAKE_URL_FETCHER_CONFIGURATION.iteritems():
+      if k.match(key):
+        return v
+    return None
+
+  class FakeUrlFetch(object):
+    """A fake urlfetch module that uses the current
+    |FAKE_URL_FETCHER_CONFIGURATION| to map urls to fake fetchers.
+    """
+    class _Response(object):
+      def __init__(self, content):
+        self.content = content
+        self.headers = { 'content-type': 'none' }
+        self.status_code = 200
+
+    class _RPC(object):
+      def __init__(self):
+        self.result = None
+
+      def wait(self):
+        pass
+
+      def get_result(self):
+        return self.result
+
+    def fetch(self, url):
+      return self._Response(_GetConfiguration(url).fetch(url))
+
+    def create_rpc(self):
+      return self._RPC()
+
+    def make_fetch_call(self, rpc, url):
+      rpc.result = self.fetch(url)
+  urlfetch = FakeUrlFetch()
+
   class NotImplemented(object):
     def __getattr__(self, attr):
       raise NotImplementedError()
@@ -42,28 +95,6 @@ except ImportError:
         self._cache[namespace].pop(key)
   memcache = InMemoryMemcache()
 
-  class FakeUrlfetch(object):
-    class _RPC(object):
-      def wait(self):
-        pass
-
-      def get_result(self):
-        return FakeUrlfetch._Result()
-
-    class _Result(object):
-      def __init__(self):
-        self.content = '{ "commit": { "tree": { "sha": 0 } } }'
-
-    def fetch(self, url):
-      return self._Result()
-
-    def create_rpc(self):
-      return self._RPC()
-
-    def make_fetch_call(self, rpc, url):
-      pass
-  urlfetch = FakeUrlfetch()
-
   # A fake webapp.RequestHandler class for Handler to extend.
   class webapp(object):
     class RequestHandler(object):

chrome/common/extensions/docs/server2/branch_utility.py

@@ -14,6 +14,10 @@ class BranchUtility(object):
     self._fetcher = fetcher
     self._memcache = memcache
 
+  def GetAllBranchNumbers(self):
+    return [self.GetBranchNumberForChannelName(branch)
+            for branch in ['dev', 'beta', 'stable', 'trunk', 'local']]
+
   def SplitChannelNameFromPath(self, path):
     try:
       first, second = path.split('/', 1)
@@ -26,8 +30,11 @@ class BranchUtility(object):
       return (self._default_branch, path)
 
   def GetBranchNumberForChannelName(self, channel_name):
-    """Returns an empty string if the branch number cannot be found.
-    Throws exception on network errors.
+    """Returns the branch number for a channel name. If the |channel_name| is
+    'trunk' or 'local', then |channel_name| will be returned unchanged. These
+    are returned unchanged because 'trunk' has a separate URL from the other
+    branches and should be handled differently. 'local' is also a special branch
+    for development that should be handled differently.
     """
     if channel_name in ['trunk', 'local']:
       return channel_name
@@ -59,6 +66,6 @@ class BranchUtility(object):
     self._memcache.Set(channel_name + '.' + self._base_path,
                        sorted_branches[0][0],
                        memcache.MEMCACHE_BRANCH_UTILITY,
-                       86400)
+                       time=86400)
 
     return sorted_branches[0][0]

chrome/common/extensions/docs/server2/handler.py

@@ -36,7 +36,7 @@ DEFAULT_BRANCH = 'local'
 BRANCH_UTILITY_MEMCACHE = AppEngineMemcache('branch_utility')
 BRANCH_UTILITY = BranchUtility(url_constants.OMAHA_PROXY_URL,
                                DEFAULT_BRANCH,
-                               AppEngineUrlFetcher(''),
+                               AppEngineUrlFetcher(None),
                                BRANCH_UTILITY_MEMCACHE)
 
 GITHUB_FILE_SYSTEM = GithubFileSystem(
@@ -59,7 +59,8 @@ FULL_EXAMPLES_PATH = DOCS_PATH + '/' + EXAMPLES_PATH
 # Global cache of instances because Handler is recreated for every request.
 SERVER_INSTANCES = {}
 
-def _GetInstanceForBranch(branch, local_path):
+def _GetInstanceForBranch(channel_name, local_path):
+  branch = BRANCH_UTILITY.GetBranchNumberForChannelName(channel_name)
   if branch in SERVER_INSTANCES:
     return SERVER_INSTANCES[branch]
   if branch == 'local':
@@ -87,7 +88,7 @@ def _GetInstanceForBranch(branch, local_path):
                                                   API_PATH,
                                                   samples_data_source_factory)
   template_data_source_factory = TemplateDataSource.Factory(
-      branch,
+      channel_name,
       api_data_source_factory,
       api_list_data_source,
       intro_data_source,
@@ -110,6 +111,12 @@ def _GetURLFromBranch(branch):
       return url_constants.SVN_TRUNK_URL + '/src'
     return url_constants.SVN_BRANCH_URL + '/' + branch + '/src'
 
+def _CleanBranches():
+  numbers = BRANCH_UTILITY.GetAllBranchNumbers()
+  for key in SERVER_INSTANCES.keys():
+    if key not in numbers:
+      SERVER_INSTANCES.pop(key)
+
 class Handler(webapp.RequestHandler):
   def __init__(self, request, response, local_path=EXTENSIONS_PATH):
     self._local_path = local_path
@@ -117,23 +124,21 @@ class Handler(webapp.RequestHandler):
 
   def _NavigateToPath(self, path):
     channel_name, real_path = BRANCH_UTILITY.SplitChannelNameFromPath(path)
-    branch = BRANCH_UTILITY.GetBranchNumberForChannelName(channel_name)
     # TODO: Detect that these are directories and serve index.html out of them.
     if real_path.strip('/') == 'apps':
       real_path = 'apps/index.html'
     if real_path.strip('/') == 'extensions':
       real_path = 'extensions/index.html'
-    # TODO: This leaks Server instances when branch bumps.
-    _GetInstanceForBranch(branch, self._local_path).Get(real_path,
-                                                        self.request,
-                                                        self.response)
+    _CleanBranches()
+    _GetInstanceForBranch(channel_name, self._local_path).Get(real_path,
+                                                              self.request,
+                                                              self.response)
 
   def get(self):
     path = self.request.path
     if '_ah/warmup' in path:
       logging.info('Warmup request.')
-      if DEFAULT_BRANCH != 'local':
-        self._NavigateToPath('trunk/extensions/samples.html')
+      self._NavigateToPath('trunk/extensions/samples.html')
       self._NavigateToPath('dev/extensions/samples.html')
       self._NavigateToPath('beta/extensions/samples.html')
       self._NavigateToPath('stable/extensions/samples.html')

chrome/common/extensions/docs/server2/integration_test.py

@@ -5,12 +5,12 @@
 
 import logging
 import os
+import re
 from StringIO import StringIO
 import unittest
 
 import appengine_memcache as memcache
-import handler
-from handler import Handler
+import appengine_wrappers
 import url_constants
 
 KNOWN_FAILURES = [
@@ -19,6 +19,46 @@ KNOWN_FAILURES = [
   'apps/samples.html',
 ]
 
+def _ReadFile(path):
+  with open(path, 'r') as f:
+    return f.read()
+
+class FakeOmahaProxy(object):
+  def fetch(self, url):
+    return _ReadFile(os.path.join('test_data', 'branch_utility', 'first.json'))
+
+class FakeSubversionServer(object):
+  def __init__(self):
+    self._base_pattern = re.compile(r'.*chrome/common/extensions/(.*)')
+
+  def fetch(self, url):
+    path = os.path.join(
+        os.pardir, os.pardir, self._base_pattern.match(url).group(1))
+    if os.path.isdir(path):
+      html = ['<html>Revision 000000']
+      for f in os.listdir(path):
+        if os.path.isdir(os.path.join(path, f)):
+          html.append('<a>' + f + '/</a>')
+        else:
+          html.append('<a>' + f + '</a>')
+      html.append('</html>')
+      return '\n'.join(html)
+    return _ReadFile(path)
+
+class FakeGithub(object):
+  def fetch(self, url):
+    return '{ "commit": { "tree": { "sha": 0} } }'
+
+appengine_wrappers.ConfigureFakeUrlFetch({
+  url_constants.OMAHA_PROXY_URL: FakeOmahaProxy(),
+  '%s/.*' % url_constants.SVN_URL: FakeSubversionServer(),
+  '%s/.*' % url_constants.GITHUB_URL: FakeGithub()
+})
+
+# Import Handler later because it immediately makes a request to github. We need
+# the fake urlfetch to be in place first.
+from handler import Handler
+
 class _MockResponse(object):
   def __init__(self):
     self.status = 200
@@ -67,11 +107,6 @@ class IntegrationTest(unittest.TestCase):
       self.assertTrue(response.out.getvalue())
 
   def testWarmupRequest(self):
-    for branch in ['dev', 'trunk', 'beta', 'stable']:
-      handler.BRANCH_UTILITY_MEMCACHE.Set(
-          branch + '.' +  url_constants.OMAHA_PROXY_URL,
-          'local',
-          memcache.MEMCACHE_BRANCH_UTILITY)
     request = _MockRequest('_ah/warmup')
     response = _MockResponse()
     Handler(request, response, local_path='../..').get()

chrome/common/extensions/docs/server2/template_data_source.py

@@ -10,16 +10,16 @@ from third_party.handlebar import Handlebar
 
 EXTENSIONS_URL = '/chrome/extensions'
 
-def _MakeBranchDict(branch):
+def _MakeChannelDict(channel_name):
   return {
-    'showWarning': branch != 'stable',
-    'branches': [
+    'showWarning': channel_name != 'stable',
+    'channels': [
       { 'name': 'Stable', 'path': 'stable' },
       { 'name': 'Dev',    'path': 'dev' },
       { 'name': 'Beta',   'path': 'beta' },
       { 'name': 'Trunk',  'path': 'trunk' }
     ],
-    'current': branch
+    'current': channel_name
   }
 
 class TemplateDataSource(object):
@@ -39,7 +39,7 @@ class TemplateDataSource(object):
     individual Requests.
     """
     def __init__(self,
-                 branch,
+                 channel_name,
                  api_data_source_factory,
                  api_list_data_source,
                  intro_data_source,
@@ -47,9 +47,7 @@ class TemplateDataSource(object):
                  cache_builder,
                  public_template_path,
                  private_template_path):
-      self._branch_info = _MakeBranchDict(branch)
-      self._static_resources = ((('/' + branch) if branch != 'local' else '') +
-                                '/static')
+      self._branch_info = _MakeChannelDict(channel_name)
       self._api_data_source_factory = api_data_source_factory
       self._api_list_data_source = api_list_data_source
       self._intro_data_source = intro_data_source
@@ -57,13 +55,14 @@ class TemplateDataSource(object):
       self._cache = cache_builder.build(Handlebar)
       self._public_template_path = public_template_path
       self._private_template_path = private_template_path
+      self._static_resources = (
+          (('/' + channel_name) if channel_name != 'local' else '') + '/static')
 
     def Create(self, request):
       """Returns a new TemplateDataSource bound to |request|.
       """
       return TemplateDataSource(
           self._branch_info,
-          self._static_resources,
           self._api_data_source_factory.Create(request),
           self._api_list_data_source,
           self._intro_data_source,
@@ -71,11 +70,11 @@ class TemplateDataSource(object):
           self._cache,
           self._public_template_path,
           self._private_template_path,
+          self._static_resources,
           request)
 
   def __init__(self,
                branch_info,
-               static_resources,
                api_data_source,
                api_list_data_source,
                intro_data_source,
@@ -83,9 +82,9 @@ class TemplateDataSource(object):
                cache,
                public_template_path,
                private_template_path,
+               static_resources,
                request):
     self._branch_info = branch_info
-    self._static_resources = static_resources
     self._api_list_data_source = api_list_data_source
     self._intro_data_source = intro_data_source
     self._samples_data_source = samples_data_source
@@ -93,6 +92,7 @@ class TemplateDataSource(object):
     self._cache = cache
     self._public_template_path = public_template_path
     self._private_template_path = private_template_path
+    self._static_resources = static_resources
     self._request = request
 
   def Render(self, template_name):

chrome/common/extensions/docs/server2/templates/private/header_body.html

@@ -3,7 +3,7 @@
   <span>WARNING: this is the {{branchInfo.current}} documentation. It may not work with the stable release of Chrome.</span>
   <select id="branchChooser">
     <option value="">Choose a different version...</option>
-    {{#branchInfo.branches}}
+    {{#branchInfo.channels}}
     <option value="{{path}}">{{name}}</option>
     {{/}}
   </select>