From aece0818c42ab154f78bd88327a7ca9f11d5f158 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Thu, 4 Jan 2018 16:47:23 -0600 Subject: [PATCH] Don't cache duplicate plugin names (#34420) * Don't cache duplicate plugin names. Fixes #33484 * Add tests for duplicate plugin filenames, to showcase what happens --- lib/ansible/plugins/loader.py | 9 +++- .../units/plugins/loader_fixtures/__init__.py | 0 .../plugins/loader_fixtures/import_fixture.py | 8 ++++ test/units/plugins/test_plugins.py | 45 +++++++++++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 test/units/plugins/loader_fixtures/__init__.py create mode 100644 test/units/plugins/loader_fixtures/import_fixture.py diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py index 211dd4c6de8..4177138b16f 100644 --- a/lib/ansible/plugins/loader.py +++ b/lib/ansible/plugins/loader.py @@ -424,7 +424,14 @@ class PluginLoader: continue if path not in self._module_cache: - self._module_cache[path] = self._load_module_source(name, path) + 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 try: diff --git a/test/units/plugins/loader_fixtures/__init__.py b/test/units/plugins/loader_fixtures/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/units/plugins/loader_fixtures/import_fixture.py b/test/units/plugins/loader_fixtures/import_fixture.py new file mode 100644 index 00000000000..95209f65e9b --- /dev/null +++ b/test/units/plugins/loader_fixtures/import_fixture.py @@ -0,0 +1,8 @@ +# Nothing to see here, this file is just empty to support a imp.load_source +# without doing anything +__metaclass__ = type + + +class test: + def __init__(self, *args, **kwargs): + pass diff --git a/test/units/plugins/test_plugins.py b/test/units/plugins/test_plugins.py index def2544ea8a..8ede0d4d27a 100644 --- a/test/units/plugins/test_plugins.py +++ b/test/units/plugins/test_plugins.py @@ -91,3 +91,48 @@ class TestErrors(unittest.TestCase): def test_plugin__init_config_none(self): self.assertPluginLoaderConfigBecomes(None, []) + + def test__load_module_source_no_duplicate_names(self): + ''' + This test simulates importing 2 plugins with the same name, + and validating that the import is shortcirtuited if a file with the same name + has already been imported + ''' + + fixture_path = os.path.join(os.path.dirname(__file__), 'loader_fixtures') + + pl = PluginLoader('test', '', 'test', 'test_plugin') + one = pl._load_module_source('import_fixture', os.path.join(fixture_path, 'import_fixture.py')) + # This line wouldn't even succeed if we didn't short cirtuit on finding a duplicate name + two = pl._load_module_source('import_fixture', '/path/to/import_fixture.py') + + self.assertEqual(one, two) + + @patch('ansible.plugins.loader.glob') + @patch.object(PluginLoader, '_get_paths') + def test_all_no_duplicate_names(self, gp_mock, glob_mock): + ''' + This test goes along with ``test__load_module_source_no_duplicate_names`` + and ensures that we ignore duplicate imports on multiple paths + ''' + + fixture_path = os.path.join(os.path.dirname(__file__), 'loader_fixtures') + + gp_mock.return_value = [ + fixture_path, + '/path/to' + ] + + glob_mock.glob.side_effect = [ + [os.path.join(fixture_path, 'import_fixture.py')], + ['/path/to/import_fixture.py'] + ] + + pl = PluginLoader('test', '', 'test', 'test_plugin') + # Aside from needing ``list()`` so we can do a len, ``PluginLoader.all`` returns a generator + # so ``list()`` actually causes ``PluginLoader.all`` to run. + plugins = list(pl.all()) + self.assertEqual(len(plugins), 1) + + self.assertIn(os.path.join(fixture_path, 'import_fixture.py'), pl._module_cache) + self.assertNotIn('/path/to/import_fixture.py', pl._module_cache)