Fix loader for filters (#37748)

* Fix loading of filter and test plugins

Filter and test plugins are different than other plugins in that they
can have many plugins in a single file.  Therefore they need to operate
a little differently.  They need to have all of the potential files
returned.  Then the caller takes care of passing those onto jinja2 in
order for jinja2 to make use of them.

This problem was (most recently) introduced with f921369445

This commit also restructures how we deduplicate plugins to take paths
into account.  If we want to start scoping which set of modules are
loaded (due to roles, for instance) we'll need to hang on to the path
information.

* add integration test for override

* Fix style checks for bcoca code

* Implement jinja2 plugin loader as a subclass

Having a subclass allows us to customize the overriding of jinja
plugins.  We can then move common parts of common code into the Loader.

lib/ansible/plugins/loader.py

@@ -35,13 +35,11 @@ def get_all_plugin_loaders():
 
 
 class PluginLoader:
-
     '''
     PluginLoader loads plugins from the configured plugin directories.
 
-    It searches for plugins by iterating through the combined list of
-    play basedirs, configured paths, and the python path.
-    The first match is used.
+    It searches for plugins by iterating through the combined list of play basedirs, configured
+    paths, and the python path.  The first match is used.
     '''
 
     def __init__(self, class_name, package, config, subdir, aliases=None, required_base_class=None):
@@ -410,36 +408,72 @@ class PluginLoader:
         display.debug(msg)
 
     def all(self, *args, **kwargs):
-        ''' instantiates all plugins with the same arguments '''
+        '''
+        Iterate through all plugins of this type
+
+        A plugin loader is initialized with a specific type.  This function is an iterator returning
+        all of the plugins of that type to the caller.
+
+        :kwarg path_only: If this is set to True, then we return the paths to where the plugins reside
+            instead of an instance of the plugin.  This conflicts with class_only and both should
+            not be set.
+        :kwarg class_only: If this is set to True then we return the python class which implements
+            a plugin rather than an instance of the plugin.  This conflicts with path_only and both
+            should not be set.
+        :kwarg _dedupe: By default, we only return one plugin per plugin name.  Deduplication happens
+            in the same way as the :meth:`get` and :meth:`find_plugin` methods resolve which plugin
+            should take precedence.  If this is set to False, then we return all of the plugins
+            found, including those with duplicate names.  In the case of duplicates, the order in
+            which they are returned is the one that would take precedence first, followed by the
+            others  in decreasing precedence order.  This should only be used by subclasses which
+            want to manage their own deduplication of the plugins.
+        :*args: Any extra arguments are passed to each plugin when it is instantiated.
+        :**kwargs: Any extra keyword arguments are passed to each plugin when it is instantiated.
+        '''
+        # TODO: Change the signature of this method to:
+        # def all(return_type='instance', args=None, kwargs=None):
+        #     if args is None: args = []
+        #     if kwargs is None: kwargs = {}
+        #     return_type can be instance, class, or path.
+        #     These changes will mean that plugin parameters won't conflict with our params and
+        #     will also make it impossible to request both a path and a class at the same time.
+        #
+        #     Move _dedupe to be a class attribute, CUSTOM_DEDUPE, with subclasses for filters and
+        #     tests setting it to True
 
         global _PLUGIN_FILTERS
 
+        dedupe = kwargs.pop('_dedupe', True)
         path_only = kwargs.pop('path_only', False)
         class_only = kwargs.pop('class_only', False)
+        # Having both path_only and class_only is a coding bug
+        if path_only and class_only:
+            raise AnsibleError('Do not set both path_only and class_only when calling PluginLoader.all()')
+
         all_matches = []
         found_in_cache = True
 
         for i in self._get_paths():
             all_matches.extend(glob.glob(os.path.join(i, "*.py")))
 
+        loaded_modules = set()
         for path in sorted(all_matches, key=os.path.basename):
-            name = os.path.basename(os.path.splitext(path)[0])
+            name = os.path.splitext(path)[0]
+            basename = os.path.basename(name)
 
-            if '__init__' in name or name in _PLUGIN_FILTERS[self.package]:
+            if basename == '__init__' or basename in _PLUGIN_FILTERS[self.package]:
                 continue
 
+            if dedupe and basename in loaded_modules:
+                continue
+            loaded_modules.add(basename)
+
             if path_only:
                 yield path
                 continue
 
             if path not in self._module_cache:
                 module = self._load_module_source(name, path)
-                if module in self._module_cache.values():
-                    # In ``_load_module_source`` if a plugin has a duplicate name, we just return the
-                    # previously matched plugin from sys.modules, which means you are never getting both,
-                    # just one, but cached for both paths, this isn't normally a problem, except with callbacks
-                    # where it will run that single callback twice. This rejects duplicates.
-                    continue
                 self._module_cache[path] = module
                 found_in_cache = False
 
@@ -461,7 +495,7 @@ class PluginLoader:
                 if not issubclass(obj, plugin_class):
                     continue
 
-            self._display_plugin_load(self.class_name, name, self._searched_paths, path, found_in_cache=found_in_cache, class_only=class_only)
+            self._display_plugin_load(self.class_name, basename, self._searched_paths, path, found_in_cache=found_in_cache, class_only=class_only)
             if not class_only:
                 try:
                     obj = obj(*args, **kwargs)
@@ -470,12 +504,58 @@ class PluginLoader:
 
             # load plugin config data
             if not found_in_cache:
-                self._load_config_defs(name, path)
+                self._load_config_defs(basename, path)
 
             self._update_object(obj, name, path)
             yield obj
 
 
+class Jinja2Loader(PluginLoader):
+    """
+    PluginLoader optimized for Jinja2 plugins
+
+    The filter and test plugins are Jinja2 plugins encapsulated inside of our plugin format.
+    The way the calling code is setup, we need to do a few things differently in the all() method
+    """
+    def find_plugin(self, name):
+        # Nothing using Jinja2Loader use this method.  We can't use the base class version because
+        # we deduplicate differently than the base class
+        raise AnsibleError('No code should call find_plugin for Jinja2Loaders (Not implemented)')
+
+    def get(self, name, *args, **kwargs):
+        # Nothing using Jinja2Loader use this method.  We can't use the base class version because
+        # we deduplicate differently than the base class
+        raise AnsibleError('No code should call find_plugin for Jinja2Loaders (Not implemented)')
+
+    def all(self, *args, **kwargs):
+        """
+        Differences with :meth:`PluginLoader.all`:
+
+        * We do not deduplicate ansible plugin names.  This is because we don't care about our
+          plugin names, here.  We care about the names of the actual jinja2 plugins which are inside
+          of our plugins.
+        * We reverse the order of the list of plugins compared to other PluginLoaders.  This is
+          because of how calling code chooses to sync the plugins from the list.  It adds all the
+          Jinja2 plugins from one of our Ansible plugins into a dict.  Then it adds the Jinja2
+          plugins from the next Ansible plugin, overwriting any Jinja2 plugins that had the same
+          name.  This is an encapsulation violation (the PluginLoader should not know about what
+          calling code does with the data) but we're pushing the common code here.  We'll fix
+          this in the future by moving more of the common code into this PluginLoader.
+        * We return a list.  We could iterate the list instead but that's extra work for no gain because
+          the API receiving this doesn't care.  It just needs an iterable
+        """
+        # We don't deduplicate ansible plugin names.  Instead, calling code deduplicates jinja2
+        # plugin names.
+        kwargs['_dedupe'] = False
+
+        # We have to instantiate a list of all plugins so that we can reverse it.  We reverse it so
+        # that calling code will deduplicate this correctly.
+        plugins = [p for p in super(Jinja2Loader, self).all(*args, **kwargs)]
+        plugins.reverse()
+
+        return plugins
+
+
 def _load_plugin_filter():
     filters = defaultdict(frozenset)
 
@@ -610,14 +690,14 @@ lookup_loader = PluginLoader(
     required_base_class='LookupBase',
 )
 
-filter_loader = PluginLoader(
+filter_loader = Jinja2Loader(
     'FilterModule',
     'ansible.plugins.filter',
     C.DEFAULT_FILTER_PLUGIN_PATH,
     'filter_plugins',
 )
 
-test_loader = PluginLoader(
+test_loader = Jinja2Loader(
     'TestModule',
     'ansible.plugins.test',
     C.DEFAULT_TEST_PLUGIN_PATH,

lib/ansible/template/__init__.py

@@ -300,16 +300,15 @@ class Templar:
         if self._filters is not None:
             return self._filters.copy()
 
-        plugins = [x for x in self._filter_loader.all()]
-
         self._filters = dict()
-        for fp in plugins:
-            self._filters.update(fp.filters())
 
         # TODO: Remove registering tests as filters in 2.9
         for name, func in self._get_tests().items():
             self._filters[name] = tests_as_filters_warning(name, func)
 
+        for fp in self._filter_loader.all():
+            self._filters.update(fp.filters())
+
         return self._filters.copy()
 
     def _get_tests(self):
@@ -320,10 +319,8 @@ class Templar:
         if self._tests is not None:
             return self._tests.copy()
 
-        plugins = [x for x in self._test_loader.all()]
-
         self._tests = dict()
-        for fp in plugins:
+        for fp in self._test_loader.all():
             self._tests.update(fp.tests())
 
         return self._tests.copy()

lib/ansible/template/safe_eval.py

@@ -91,8 +91,8 @@ def safe_eval(expr, locals=None, include_exceptions=False):
         )
 
     filter_list = []
-    for filter in filter_loader.all():
-        filter_list.extend(filter.filters().keys())
+    for filter_ in filter_loader.all():
+        filter_list.extend(filter_.filters().keys())
 
     test_list = []
     for test in test_loader.all():

test/integration/targets/plugin_loader/aliases

@@ -0,0 +1 @@
+posix/ci/group3

test/integration/targets/plugin_loader/normal/filters.yml

@@ -0,0 +1,13 @@
+- hosts: testhost
+  gather_facts: false
+  tasks:
+    - name: ensure filters work as shipped from core
+      assert:
+         that:
+             - a|flatten == [1, 2, 3, 4, 5]
+             - a|ternary('yes', 'no') == 'yes'
+      vars:
+        a:
+         - 1
+         - 2
+         - [3, 4, 5]

test/integration/targets/plugin_loader/override/filter_plugins/core.py

@@ -0,0 +1,18 @@
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+
+def do_flag(myval):
+    return 'flagged'
+
+
+class FilterModule(object):
+    ''' Ansible core jinja2 filters '''
+
+    def filters(self):
+        return {
+            # jinja2 overrides
+            'flag': do_flag,
+            'flatten': do_flag,
+        }

test/integration/targets/plugin_loader/override/filters.yml

@@ -0,0 +1,15 @@
+- hosts: testhost
+  gather_facts: false
+  tasks:
+    - name: ensure local 'flag' filter works, 'flatten' is overriden and 'ternary' is still from core
+      assert:
+         that:
+             - a|flag == 'flagged'
+             - a|flatten != [1, 2, 3, 4, 5]
+             - a|flatten == "flagged"
+             - a|ternary('yes', 'no') == 'yes'
+      vars:
+        a:
+         - 1
+         - 2
+         - [3, 4, 5]

test/integration/targets/plugin_loader/runme.sh

@@ -0,0 +1,24 @@
+#!/usr/bin/env bash
+
+set -ux
+
+
+# check normal execution
+for myplay in normal/*.yml
+do
+	ansible-playbook "${myplay}" -i ../../inventory -vvv "$@"
+	if test $? != 0 ; then
+		echo "### Failed to run ${myplay} normally"
+		exit 1
+	fi
+done
+
+# check overrides
+for myplay in override/*.yml
+do
+	ansible-playbook "${myplay}" -i ../../inventory -vvv "$@"
+	if test $? != 0 ; then
+		echo "### Failed to run ${myplay} override"
+		exit 1
+	fi
+done