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() - return decorator + 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) + + # 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):