plugable: switch API to Registry-based plugin discovery

Merge Registrar into Registry. Use the Registry instance of each plugin
module to discover plugins in the module instead of the global Registrar
instance.

This removes the side-effect of all plugins in a module being re-registered
every time the module is imported.

https://fedorahosted.org/freeipa/ticket/4739

Reviewed-By: David Kupka <dkupka@redhat.com>

3 files changed, 62 insertions, 82 deletions

diff --git a/ipalib/errors.py b/ipalib/errors.py
index 52fa25f02..93333c2aa 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -236,6 +236,14 @@ class PluginsPackageError(PrivateError):
     format = '%(name)s must be sub-package, not module: %(file)r'
 
 
+class PluginModuleError(PrivateError):
+    """
+    Raised when a module is not a valid plugin module.
+    """
+
+    format = '%(name)s is not a valid plugin module'
+
+
 ##############################################################################
 # Public errors:
 
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index ac28e57e2..0ed8f4fdb 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -92,12 +92,32 @@ class Registry(object):
     For forward compatibility, make sure that the module-level instance of
     this object is named "register".
     """
-    def __call__(self):
-        def decorator(cls):
-            _register(cls)
-            return cls
+    def __init__(self):
+        self.__registry = collections.OrderedDict()
+
+    def __call__(self, **kwargs):
+        def register(klass):
+            """
+            Register the plugin ``klass``.
+
+            :param klass: A subclass of `Plugin` to attempt to register.
+            """
+            if not inspect.isclass(klass):
+                raise TypeError('plugin must be a class; got %r' % klass)
 
-        return decorator
+            # Raise DuplicateError if this exact class was already registered:
+            if klass in self.__registry:
+                raise errors.PluginDuplicateError(plugin=klass)
+
+            # The plugin is okay, add to __registry:
+            self.__registry[klass] = dict(kwargs, klass=klass)
+
+            return klass
+
+        return register
+
+    def __iter__(self):
+        return iter(self.__registry.values())
 
 
 class Plugin(ReadOnly):
@@ -248,48 +268,6 @@ class Plugin(ReadOnly):
         )
 
 
-class Registrar(collections.Mapping):
-    """
-    Collects plugin classes as they are registered.
-
-    The Registrar does not instantiate plugins... it only implements the
-    override logic and stores the plugins in a namespace per allowed base
-    class.
-
-    The plugins are instantiated when `API.finalize()` is called.
-    """
-    def __init__(self):
-        self.__registry = collections.OrderedDict()
-
-    def __call__(self, klass, override=False):
-        """
-        Register the plugin ``klass``.
-
-        :param klass: A subclass of `Plugin` to attempt to register.
-        :param override: If true, override an already registered plugin.
-        """
-        if not inspect.isclass(klass):
-            raise TypeError('plugin must be a class; got %r' % klass)
-
-        # Raise DuplicateError if this exact class was already registered:
-        if klass in self.__registry:
-            raise errors.PluginDuplicateError(plugin=klass)
-
-        # The plugin is okay, add to __registry:
-        self.__registry[klass] = dict(override=override)
-
-    def __getitem__(self, key):
-        return self.__registry[key]
-
-    def __iter__(self):
-        return iter(self.__registry)
-
-    def __len__(self):
-        return len(self.__registry)
-
-_register = Registrar()
-
-
 class API(ReadOnly):
     """
     Dynamic API object through which `Plugin` instances are accessed.
@@ -563,14 +541,18 @@ class API(ReadOnly):
                 module = importlib.import_module(name)
             except errors.SkipPluginModule as e:
                 self.log.debug("skipping plugin module %s: %s", name, e.reason)
+                continue
             except Exception as e:
                 if self.env.startup_traceback:
                     import traceback
                     self.log.error("could not load plugin module %s\n%s", name,
                                    traceback.format_exc())
                 raise
-            else:
+
+            try:
                 self.add_module(module)
+            except errors.PluginModuleError as e:
+                self.log.debug("%s", e)
 
     def add_module(self, module):
         """
@@ -578,14 +560,17 @@ class API(ReadOnly):
 
         :param module: A module from which to add plugins.
         """
-        for name in dir(module):
-            klass = getattr(module, name)
-            if not inspect.isclass(klass):
-                continue
-            if klass not in _register:
-                continue
-            kwargs = _register[klass]
-            self.add_plugin(klass, **kwargs)
+        try:
+            register = module.register
+        except AttributeError:
+            pass
+        else:
+            if isinstance(register, Registry):
+                for kwargs in register:
+                    self.add_plugin(**kwargs)
+                return
+
+        raise errors.PluginModuleError(name=module.__name__)
 
     def add_plugin(self, klass, override=False):
         """
@@ -606,9 +591,6 @@ class API(ReadOnly):
             sub_d = self.__plugins.setdefault(base, {})
             found = True
 
-            if sub_d.get(klass.__name__) is klass:
-                continue
-
             # Check override:
             if klass.__name__ in sub_d:
                 if not override:
diff --git a/ipatests/test_ipalib/test_plugable.py b/ipatests/test_ipalib/test_plugable.py
index 0434d7970..a618550e7 100644
--- a/ipatests/test_ipalib/test_plugable.py
+++ b/ipatests/test_ipalib/test_plugable.py
@@ -98,9 +98,9 @@ class test_Plugin(ClassChecker):
         assert o.__islocked__()
 
 
-def test_Registrar():
+def test_Registry():
     """
-    Test the `ipalib.plugable.Registrar` class
+    Test the `ipalib.plugable.Registry` class
     """
     class Base1(object):
         pass
@@ -115,59 +115,49 @@ def test_Registrar():
     class plugin3(Base3):
         pass
 
-    # Test creation of Registrar:
-    r = plugable.Registrar()
+    # Test creation of Registry:
+    r = plugable.Registry()
 
     # Check that TypeError is raised trying to register something that isn't
     # a class:
     p = plugin1()
-    e = raises(TypeError, r, p)
+    e = raises(TypeError, r(), p)
     assert str(e) == 'plugin must be a class; got %r' % p
 
     # Check that registration works
-    r(plugin1)
-    assert len(r) == 1
-    assert plugin1 in r
-    assert r[plugin1] == dict(override=False)
+    r()(plugin1)
 
     # Check that DuplicateError is raised trying to register exact class
     # again:
-    e = raises(errors.PluginDuplicateError, r, plugin1)
+    e = raises(errors.PluginDuplicateError, r(), plugin1)
     assert e.plugin is plugin1
 
     # Check that overriding works
-    orig1 = plugin1
     class base1_extended(Base1):
         pass
     class plugin1(base1_extended):  # pylint: disable=function-redefined
         pass
-    r(plugin1, override=True)
-    assert len(r) == 2
-    assert plugin1 in r
-    assert r[plugin1] == dict(override=True)
+    r(override=True)(plugin1)
 
     # Test that another plugin can be registered:
-    r(plugin2)
-    assert len(r) == 3
-    assert plugin2 in r
-    assert r[plugin2] == dict(override=False)
+    r()(plugin2)
 
     # Setup to test more registration:
     class plugin1a(Base1):
         pass
-    r(plugin1a)
+    r()(plugin1a)
 
     class plugin1b(Base1):
         pass
-    r(plugin1b)
+    r()(plugin1b)
 
     class plugin2a(Base2):
         pass
-    r(plugin2a)
+    r()(plugin2a)
 
     class plugin2b(Base2):
         pass
-    r(plugin2b)
+    r()(plugin2b)
 
 
 class test_API(ClassChecker):