Mailing-List: contact allura-commits-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: allura-dev@incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: johnsca@apache.org
To: allura-commits@incubator.apache.org
Date: Tue, 04 Jun 2013 22:17:26 -0000
Message-Id: <6d71c014c9cf488e9dd91a61242cb682@git.apache.org>
In-Reply-To: <b69cb6b111a34b7693e804963f7c2f20@git.apache.org>
References: <b69cb6b111a34b7693e804963f7c2f20@git.apache.org>
Subject: [22/26] git commit: [#6325] Added tests for PPL, refactored,
 and fixed issue with partial ordering of EP overrides

[#6325] Added tests for PPL, refactored, and fixed issue with partial ordering of EP overrides


Project: http://git-wip-us.apache.org/repos/asf/incubator-allura/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-allura/commit/af325d92
Tree: http://git-wip-us.apache.org/repos/asf/incubator-allura/tree/af325d92
Diff: http://git-wip-us.apache.org/repos/asf/incubator-allura/diff/af325d92

Branch: refs/heads/cj/6325
Commit: af325d9223adc5238a953bdba4a0ef279705f2bd
Parents: b2093db
Author: Cory Johns <cjohns@slashdotmedia.com>
Authored: Mon Jun 3 14:23:32 2013 +0000
Committer: Cory Johns <cjohns@slashdotmedia.com>
Committed: Tue Jun 4 20:51:31 2013 +0000

----------------------------------------------------------------------
 Allura/allura/lib/helpers.py                       |   65 +++++
 Allura/allura/lib/package_path_loader.py           |  215 +++++++++------
 .../allura/tests/unit/test_package_path_loader.py  |  203 ++++++++++++++
 requirements-common.txt                            |    2 +-
 4 files changed, 395 insertions(+), 90 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/af325d92/Allura/allura/lib/helpers.py
----------------------------------------------------------------------
diff --git a/Allura/allura/lib/helpers.py b/Allura/allura/lib/helpers.py
index c821301..6c0a863 100644
--- a/Allura/allura/lib/helpers.py
+++ b/Allura/allura/lib/helpers.py
@@ -28,6 +28,7 @@ import logging
 import cPickle as pickle
 from hashlib import sha1
 from datetime import datetime, timedelta
+from collections import defaultdict
 
 import tg
 import genshi.template
@@ -725,3 +726,67 @@ def log_output(log):
     finally:
         sys.stdout = _stdout
         sys.stderr = _stderr
+
+def topological_sort(items, partial_order):
+    """Perform topological sort.
+       items is a list of items to be sorted.
+       partial_order is a list of pairs. If pair (a,b) is in it, it means
+       that item a should appear before item b.
+       Returns a list of the items in one of the possible orders, or None
+       if partial_order contains a loop.
+
+       Modified from: http://www.bitformation.com/art/python_toposort.html
+    """
+
+    def add_arc(graph, fromnode, tonode):
+        """Add an arc to a graph. Can create multiple arcs.
+           The end nodes must already exist."""
+        graph[fromnode].append(tonode)
+        # Update the count of incoming arcs in tonode.
+        graph[tonode][0] = graph[tonode][0] + 1
+
+    # step 1 - create a directed graph with an arc a->b for each input
+    # pair (a,b).
+    # The graph is represented by a dictionary. The dictionary contains
+    # a pair item:list for each node in the graph. /item/ is the value
+    # of the node. /list/'s 1st item is the count of incoming arcs, and
+    # the rest are the destinations of the outgoing arcs. For example:
+    #           {'a':[0,'b','c'], 'b':[1], 'c':[1]}
+    # represents the graph:   c <-- a --> b
+    # The graph may contain loops and multiple arcs.
+    # Note that our representation does not contain reference loops to
+    # cause GC problems even when the represented graph contains loops,
+    # because we keep the node names rather than references to the nodes.
+    graph = defaultdict(lambda:[0])
+    for a,b in partial_order:
+        add_arc(graph, a, b)
+
+    # Step 2 - find all roots (nodes with zero incoming arcs).
+    roots = [n for n in items if graph[n][0] == 0]
+    roots.reverse()  # keep sort stable
+
+    # step 3 - repeatedly emit a root and remove it from the graph. Removing
+    # a node may convert some of the node's direct children into roots.
+    # Whenever that happens, we append the new roots to the list of
+    # current roots.
+    sorted = []
+    while roots:
+        # If len(roots) is always 1 when we get here, it means that
+        # the input describes a complete ordering and there is only
+        # one possible output.
+        # When len(roots) > 1, we can choose any root to send to the
+        # output; this freedom represents the multiple complete orderings
+        # that satisfy the input restrictions. We arbitrarily take one of
+        # the roots using pop(). Note that for the algorithm to be efficient,
+        # this operation must be done in O(1) time.
+        root = roots.pop()
+        sorted.append(root)
+        for child in graph[root][1:]:
+            graph[child][0] = graph[child][0] - 1
+            if graph[child][0] == 0:
+                roots.append(child)
+        del graph[root]
+    if len(graph) > 0:
+        # There is a loop in the input.
+        return None
+    return sorted

http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/af325d92/Allura/allura/lib/package_path_loader.py
----------------------------------------------------------------------
diff --git a/Allura/allura/lib/package_path_loader.py b/Allura/allura/lib/package_path_loader.py
index 53b6fb7..0efde59 100644
--- a/Allura/allura/lib/package_path_loader.py
+++ b/Allura/allura/lib/package_path_loader.py
@@ -1,3 +1,19 @@
+#       Licensed to the Apache Software Foundation (ASF) under one
+#       or more contributor license agreements.  See the NOTICE file
+#       distributed with this work for additional information
+#       regarding copyright ownership.  The ASF licenses this file
+#       to you 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.
 '''
 A Jinja template loader which allows for:
  - dotted-notation package loading
@@ -93,8 +109,12 @@ The positioners are:
 '''
 import pkg_resources
 import os
+from collections import defaultdict
 
 import jinja2
+from ming.utils import LazyProperty
+
+from allura.lib.helpers import topological_sort
 
 
 class PackagePathLoader(jinja2.BaseLoader):
@@ -113,7 +133,7 @@ class PackagePathLoader(jinja2.BaseLoader):
         # TODO: How does one handle project-theme?
         if default_paths is None:
             default_paths = [
-                    #['projec-theme', None],
+                    #['project-theme', None],
                     ['site-theme', None],
                     ['allura', '/'],
                 ]
@@ -122,72 +142,108 @@ class PackagePathLoader(jinja2.BaseLoader):
         self.default_paths = default_paths
         self.override_root = override_root
 
-        # Finally instantiate the loader
-        self.fs_loader = jinja2.FileSystemLoader(self.init_paths())
+    @LazyProperty
+    def fs_loader(self):
+        return jinja2.FileSystemLoader(self.init_paths())
+
+    def _load_paths(self):
+        """
+        Load all the paths to be processed, including defaults, in the default order.
+        """
+        paths = self.default_paths[:]  # copy default_paths
+        paths[-1:0] = [  # insert all eps just before last item, by default
+                [ep.name, pkg_resources.resource_filename(ep.module_name, "")]
+                for ep in pkg_resources.iter_entry_points(self.override_entrypoint)
+            ]
+        return paths
+
+    def _load_rules(self):
+        """
+        Load and pre-process the rules from the entry points.
+
+        Rules are specified per-tool as a list of the form:
+
+            template_path_rules = [
+                    ['>', 'tool1'],  # this tool must be resolved before tool1
+                    ['<', 'tool2'],  # this tool must be resolved after tool2
+                    ['=', 'tool3'],  # this tool replaces all of tool3's templates
+                ]
+
+        Returns two lists of rules, order_rules and replacement_rules.
+
+        order_rules represents all of the '>' and '<' rules and are returned
+        as a list of pairs of the form ('a', 'b') indicating that path 'a' must
+        come before path 'b'.
+
+        replacement_rules represent all of the '=' rules and are returned as
+        a dictionary mapping the paths to replace to the paths to replace with.
+        """
+        order_rules = []
+        replacement_rules = {}
+        for ep in pkg_resources.iter_entry_points(self.override_entrypoint):
+            for rule in getattr(ep.load(), 'template_path_rules', []):
+                if rule[0] == '>':
+                    order_rules.append((ep.name, rule[1]))
+                elif rule[0] == '=':
+                    replacement_rules[rule[1]] = ep.name
+                elif rule[0] == '<':
+                    order_rules.append((rule[1], ep.name))
+                else:
+                    raise jinja2.TemplateError(
+                        'Unknown template path rule in %s: %s' % (
+                            ep.name, ' '.join(rule)))
+        return order_rules, replacement_rules
+
+    def _sort_paths(self, paths, rules):
+        """
+        Process all '>' and '<' rules, providing a partial ordering
+        of the paths based on the given rules.
+
+        The rules should already have been pre-processed by _load_rules
+        to a list of partial ordering pairs ('a', 'b') indicating that
+        path 'a' should come before path 'b'.
+        """
+        names = [p[0] for p in paths]
+        # filter rules that reference non-existent paths to prevent "loops" in the graph
+        rules = [r for r in rules if r[0] in names and r[1] in names]
+        ordered_paths = topological_sort(names, rules)
+        if ordered_paths is None:
+            raise jinja2.TemplateError(
+                'Loop detected in ordering of overrides')
+        return paths.sort(key=lambda p: ordered_paths.index(p[0]))
+
+    def _replace_signposts(self, paths, rules):
+        """
+        Process all '=' rules, replacing the rule target's path value with
+        the rule's entry's path value.
+
+        Multiple entries replacing the same signpost can cause indeterminate
+        behavior, as the order of the entries is not entirely defined.
+        However, if _sort_by_rules is called first, the partial ordering is
+        respected.
+
+        This mutates paths.
+        """
+        p_idx = lambda n: [e[0] for e in paths].index(n)
+        for target, replacement in rules.items():
+            try:
+                removed = paths.pop(p_idx(replacement))
+                paths[p_idx(target)][1] = removed[1]
+            except ValueError:
+                # target or replacement missing (may not be installed)
+                pass
 
     def init_paths(self):
         '''
         Set up the setuptools entry point-based paths.
         '''
-        paths = self.default_paths[:]
-
-        '''
-        Iterate through the overriders.
-        TODO: Can this be moved to allura.app_globals.Globals, or is this
-              executed before that is available?
-        '''
-        epoints = pkg_resources.iter_entry_points(self.override_entrypoint)
-        for epoint in epoints:
-            overrider = epoint.load()
-            # Get the path of the module
-            tmpl_path = pkg_resources.resource_filename(
-                overrider.__module__,
-                ""
-            )
-            # Default insert position is right before allura(/)
-            insert_position = len(paths) - 1
-
-            rules = getattr(overrider, 'template_path_rules', [])
-
-            # Check each of the rules for this overrider
-            for direction, signpost in rules:
-                sp_location = None
-
-                # Find the signpost
-                try:
-                    sp_location = [path[0] for path in paths].index(signpost)
-                except ValueError:
-                    # Couldn't find it, hope they specified another one, or
-                    # that the default is ok.
-                    continue
-
-                if direction == '=':
-                    # Set a signpost. Behavior if already set is undetermined,
-                    # as entry point ordering is undetermined
-                    paths[sp_location][1] = tmpl_path
-                    # already inserted! our work is done here
-                    insert_position = None
-                    break
-                elif direction == '>':
-                    # going to put it right before the signpost
-                    insert_position = min(sp_location, insert_position)
-                elif direction == '<':
-                    # going to put it right after the signpost
-                    insert_position = min(sp_location + 1, insert_position)
-                else:
-                    # don't know what that is!
-                    raise jinja2.TemplateError(
-                        'Unknown template path rule in %s: %s' % (
-                            overrider, direction))
+        paths = self._load_paths()
+        order_rules, repl_rules = self._load_rules()
 
-            # in the case that we've already replaced a signpost, carry on
-            if insert_position is not None:
-                # TODO: wouldn't OrderedDict be better? the allura.lib one
-                #       doesn't support ordering like the markdown one
-                paths.insert(insert_position, (epoint.name, tmpl_path))
+        self._sort_paths(paths, order_rules)
+        self._replace_signposts(paths, repl_rules)
 
-        # Get rid of None paths... not useful
-        return [path for name, path in paths if path is not None]
+        return [p[1] for p in paths if p[1] is not None]
 
     def get_source(self, environment, template):
         '''
@@ -195,40 +251,21 @@ class PackagePathLoader(jinja2.BaseLoader):
         - path/to/template.html
         - module:path/to/template.html
         '''
-        package, path = None, None
         src = None
-        bits = template.split(':')
-
-        if len(bits) == 2:
-            # splitting out the Python module name from the template string...
-            # the default allura behavior
-            package, path = template.split(':')
-            # TODO: is there a better way to do this?
-            path_fragment = os.path.join(self.override_root, package, path)
-        elif len(bits) == 1:
-            # TODO: is this even useful?
-            path = bits[0]
-            path_fragment = os.path.join(self.override_root, path)
-        else:
-            raise jinja2.TemplateNotFound(template)
 
         # look in all of the customized search locations...
         try:
-            src = self.fs_loader.get_source(environment, path_fragment)
+            parts = [self.override_root] + template.split(':')
+            if len(parts) > 2:
+                parts[1:2] = parts[1].split('.')
+            return self.fs_loader.get_source(environment, os.path.join(*parts))
         except jinja2.TemplateNotFound:
-            # ...this doesn't mean it's really not found... it's probably
-            # just specified in the Allura-normal manner
+            # fall-back to attempt non-override loading
             pass
 
-        # ...but if you don't find anything, fall back to the explicit package
-        # approach
-        if src is None and package is not None:
-            # gets the absolute filename of the template
+        if ':' in template:
+            package, path = template.split(':', 2)
             filename = pkg_resources.resource_filename(package, path)
-            # get the filename relative to the root: (default '/').. if this
-            # fails this error is not caught, so should get propagated
-            # normally
-            src = self.fs_loader.get_source(environment, filename)
-        elif src is None:
-            raise jinja2.TemplateNotFound(template)
-        return src
+            return self.fs_loader.get_source(environment, filename)
+        else:
+            return self.fs_loader.get_source(environment, template)

http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/af325d92/Allura/allura/tests/unit/test_package_path_loader.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/unit/test_package_path_loader.py b/Allura/allura/tests/unit/test_package_path_loader.py
new file mode 100644
index 0000000..a5055ac
--- /dev/null
+++ b/Allura/allura/tests/unit/test_package_path_loader.py
@@ -0,0 +1,203 @@
+#       Licensed to the Apache Software Foundation (ASF) under one
+#       or more contributor license agreements.  See the NOTICE file
+#       distributed with this work for additional information
+#       regarding copyright ownership.  The ASF licenses this file
+#       to you 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.
+
+from unittest import TestCase
+from collections import defaultdict
+
+import jinja2
+from nose.tools import assert_equal, assert_in, assert_raises
+import mock
+
+from allura.lib.package_path_loader import PackagePathLoader
+
+
+class TestPackagePathLoader(TestCase):
+
+    @mock.patch('pkg_resources.resource_filename')
+    @mock.patch('pkg_resources.iter_entry_points')
+    def test_load_paths(self, iter_entry_points, resource_filename):
+        eps = iter_entry_points.return_value.__iter__.return_value = [
+            mock.Mock(ep_name='ep0', module_name='eps.ep0'),
+            mock.Mock(ep_name='ep1', module_name='eps.ep1'),
+            mock.Mock(ep_name='ep2', module_name='eps.ep2'),
+        ]
+        for ep in eps:
+            ep.name = ep.ep_name
+        resource_filename.side_effect = lambda m, r: 'path:'+m
+
+        paths = PackagePathLoader()._load_paths()
+
+        assert_equal(paths, [
+                ['site-theme', None],
+                ['ep0', 'path:eps.ep0'],
+                ['ep1', 'path:eps.ep1'],
+                ['ep2', 'path:eps.ep2'],
+                ['allura', '/'],
+            ])
+        assert_equal(type(paths[0]), list)
+        assert_equal(resource_filename.call_args_list, [
+                mock.call('eps.ep0', ''),
+                mock.call('eps.ep1', ''),
+                mock.call('eps.ep2', ''),
+            ])
+
+    @mock.patch('pkg_resources.iter_entry_points')
+    def test_load_rules(self, iter_entry_points):
+        eps = iter_entry_points.return_value.__iter__.return_value = [
+                mock.Mock(ep_name='ep0', rules=[('>', 'allura')]),
+                mock.Mock(ep_name='ep1', rules=[('=', 'allura')]),
+                mock.Mock(ep_name='ep2', rules=[('<', 'allura')]),
+            ]
+        for ep in eps:
+            ep.name = ep.ep_name
+            ep.load.return_value.template_path_rules = ep.rules
+
+        order_rules, replacement_rules = PackagePathLoader()._load_rules()
+
+        assert_equal(order_rules, [('ep0', 'allura'), ('allura', 'ep2')])
+        assert_equal(replacement_rules, {'allura': 'ep1'})
+
+        eps = iter_entry_points.return_value.__iter__.return_value = [
+                mock.Mock(ep_name='ep0', rules=[('?', 'allura')]),
+            ]
+        for ep in eps:
+            ep.name = ep.ep_name
+            ep.load.return_value.template_path_rules = ep.rules
+        assert_raises(jinja2.TemplateError, PackagePathLoader()._load_rules)
+
+    def test_replace_signposts(self):
+        ppl = PackagePathLoader()
+        ppl._replace_signpost = mock.Mock()
+        paths = [
+                ['site-theme', None],
+                ['ep0', '/ep0'],
+                ['ep1', '/ep1'],
+                ['ep2', '/ep2'],
+                ['allura', '/'],
+            ]
+        rules = {
+                'allura': 'ep2',
+                'site-theme': 'ep1',
+                'foo': 'ep1',
+                'ep0': 'bar',
+            }
+
+        ppl._replace_signposts(paths, rules)
+
+        assert_equal(paths, [
+                ['site-theme', '/ep1'],
+                ['ep0', '/ep0'],
+                ['allura', '/ep2'],
+            ]);
+
+    def test_sort_paths(self):
+        paths = [
+                ['site-theme', None],
+                ['ep0', '/ep0'],
+                ['ep1', '/ep1'],
+                ['ep2', '/ep2'],
+                ['ep3', '/ep3'],
+                ['allura', '/'],
+            ]
+        rules = [
+                ('allura', 'ep0'),
+                ('ep3', 'ep1'),
+                ('ep2', 'ep1'),
+                ('ep4', 'ep1'),  # rules referencing missing paths
+                ('ep2', 'ep5'),
+            ]
+
+        PackagePathLoader()._sort_paths(paths, rules)
+
+        assert_equal(paths, [
+                ['site-theme', None],
+                ['ep2', '/ep2'],
+                ['ep3', '/ep3'],
+                ['ep1', '/ep1'],
+                ['allura', '/'],
+                ['ep0', '/ep0'],
+            ])
+
+    def test_init_paths(self):
+        paths =  [
+                ['root', '/'],
+                ['none', None],
+                ['tail', '/tail'],
+            ]
+        ppl = PackagePathLoader()
+        ppl._load_paths = mock.Mock(return_value=paths)
+        ppl._load_rules = mock.Mock(return_value=('order_rules','repl_rules'))
+        ppl._replace_signposts = mock.Mock()
+        ppl._sort_paths = mock.Mock()
+
+        output = ppl.init_paths()
+
+        ppl._load_paths.assert_called_once_with()
+        ppl._load_rules.assert_called_once_with()
+        ppl._sort_paths.assert_called_once_with(paths, 'order_rules')
+        ppl._replace_signposts.assert_called_once_with(paths, 'repl_rules')
+
+        assert_equal(output, ['/', '/tail'])
+
+    @mock.patch('jinja2.FileSystemLoader')
+    def test_fs_loader(self, FileSystemLoader):
+        ppl = PackagePathLoader()
+        ppl.init_paths = mock.Mock(return_value=['path1', 'path2'])
+        FileSystemLoader.return_value = 'fs_loader'
+
+        output1 = ppl.fs_loader
+        output2 = ppl.fs_loader
+
+        ppl.init_paths.assert_called_once_with()
+        FileSystemLoader.assert_called_once_with(['path1', 'path2'])
+        assert_equal(output1, 'fs_loader')
+        assert output1 is output2
+
+    @mock.patch('jinja2.FileSystemLoader')
+    def test_get_source(self, fs_loader):
+        ppl = PackagePathLoader()
+        ppl.init_paths = mock.Mock()
+        fs_loader().get_source.return_value = 'fs_load'
+
+        # override exists
+        output = ppl.get_source('env', 'allura.ext.admin:templates/audit.html')
+
+        assert_equal(output, 'fs_load')
+        fs_loader().get_source.assert_called_once_with('env', 'override/allura/ext/admin/templates/audit.html')
+
+        fs_loader().get_source.reset_mock()
+        fs_loader().get_source.side_effect = [jinja2.TemplateNotFound('test'), 'fs_load']
+
+        with mock.patch('pkg_resources.resource_filename') as rf:
+            rf.return_value = 'resource'
+            # no override, ':' in template
+            output = ppl.get_source('env', 'allura.ext.admin:templates/audit.html')
+            rf.assert_called_once_with('allura.ext.admin', 'templates/audit.html')
+
+        assert_equal(output, 'fs_load')
+        assert_equal(fs_loader().get_source.call_count, 2)
+        fs_loader().get_source.assert_called_with('env', 'resource')
+
+        fs_loader().get_source.reset_mock()
+        fs_loader().get_source.side_effect = [jinja2.TemplateNotFound('test'), 'fs_load']
+
+        # no override, ':' not in template
+        output = ppl.get_source('env', 'templates/audit.html')
+
+        assert_equal(output, 'fs_load')
+        assert_equal(fs_loader().get_source.call_count, 2)
+        fs_loader().get_source.assert_called_with('env', 'templates/audit.html')

http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/af325d92/requirements-common.txt
----------------------------------------------------------------------
diff --git a/requirements-common.txt b/requirements-common.txt
index f80e240..7bb0de3 100644
--- a/requirements-common.txt
+++ b/requirements-common.txt
@@ -67,7 +67,7 @@ smmap==0.8.1
 # testing & development
 datadiff==1.1.5
 ipython==0.11
-mock==0.8.0
+mock==1.0.1
 nose==1.1.2
 pyflakes==0.5.0
 WebTest==1.4.0