9 files changed, 214 insertions, 70 deletions

diff --git a/src/mailman/app/commands.py b/src/mailman/app/commands.py
index 9983a25e5..7cf3bc4c5 100644
--- a/src/mailman/app/commands.py
+++ b/src/mailman/app/commands.py
@@ -19,18 +19,11 @@
 
 from mailman.config import config
 from mailman.interfaces.command import IEmailCommand
-from mailman.utilities.modules import find_components
+from mailman.utilities.modules import add_components
 from public import public
-from zope.interface.verify import verifyObject
 
 
 @public
 def initialize():
     """Initialize the email commands."""
-    for command_class in find_components('mailman.commands', IEmailCommand):
-        command = command_class()
-        verifyObject(IEmailCommand, command)
-        assert command_class.name not in config.commands, (
-            'Duplicate email command "{}" found in {}'.format(
-                command_class.name, command_class.__module__))
-        config.commands[command_class.name] = command_class()
+    add_components('mailman.commands', IEmailCommand, config.commands)
diff --git a/src/mailman/bin/mailman.py b/src/mailman/bin/mailman.py
index eea3603a8..62066175e 100644
--- a/src/mailman/bin/mailman.py
+++ b/src/mailman/bin/mailman.py
@@ -28,7 +28,6 @@ from mailman.interfaces.command import ICLISubCommand
 from mailman.utilities.modules import find_components
 from mailman.version import MAILMAN_VERSION_FULL
 from public import public
-from zope.interface.verify import verifyObject
 
 
 # --help should display the subcommands by alphabetical order, except that
@@ -76,9 +75,7 @@ def main():
     # the plugins.  Punt on this for now.
     subparser = parser.add_subparsers(title='Commands')
     subcommands = []
-    for command_class in find_components('mailman.commands', ICLISubCommand):
-        command = command_class()
-        verifyObject(ICLISubCommand, command)
+    for command in find_components('mailman.commands', ICLISubCommand):
         subcommands.append(command)
     subcommands.sort(key=cmp_to_key(_help_sorter))
     for command in subcommands:
diff --git a/src/mailman/chains/base.py b/src/mailman/chains/base.py
index fdf9477da..59125ba69 100644
--- a/src/mailman/chains/base.py
+++ b/src/mailman/chains/base.py
@@ -21,6 +21,7 @@ from mailman.config import config
 from mailman.interfaces.chain import (
     IChain, IChainIterator, IChainLink, IMutableChain, LinkAction)
 from mailman.interfaces.rules import IRule
+from mailman.utilities.modules import abstract_component
 from public import public
 from zope.interface import implementer
 
@@ -55,6 +56,7 @@ class Link:
 
 
 @public
+@abstract_component
 @implementer(IChain, IChainIterator)
 class TerminalChainBase:
     """A base chain that always matches and executes a method.
@@ -85,6 +87,7 @@ class TerminalChainBase:
 
 
 @public
+@abstract_component
 @implementer(IMutableChain)
 class Chain:
     """Generic chain base class."""
diff --git a/src/mailman/core/chains.py b/src/mailman/core/chains.py
index 0eedd8b8c..c251d38bc 100644
--- a/src/mailman/core/chains.py
+++ b/src/mailman/core/chains.py
@@ -17,12 +17,10 @@
 
 """Application support for chain processing."""
 
-from mailman.chains.base import Chain, TerminalChainBase
 from mailman.config import config
 from mailman.interfaces.chain import IChain, LinkAction
-from mailman.utilities.modules import find_components
+from mailman.utilities.modules import add_components
 from public import public
-from zope.interface.verify import verifyObject
 
 
 @public
@@ -91,19 +89,5 @@ def process(mlist, msg, msgdata, start_chain='default-posting-chain'):
 @public
 def initialize():
     """Set up chains, both built-in and from the database."""
-    for chain_class in find_components('mailman.chains', IChain):
-        # FIXME 2010-12-28 barry: We need a generic way to disable automatic
-        # instantiation of discovered classes.  This is useful not just for
-        # chains, but also for rules, handlers, etc.  Ideally it should be
-        # part of find_components().  For now, hard code the ones we do not
-        # want to instantiate.
-        if chain_class in (Chain, TerminalChainBase):
-            continue
-        chain = chain_class()
-        verifyObject(IChain, chain)
-        assert chain.name not in config.chains, (
-            'Duplicate chain "{}" found in {} (previously: {}'.format(
-                chain.name, chain_class, config.chains[chain.name]))
-        config.chains[chain.name] = chain
+    add_components('mailman.chains', IChain, config.chains)
     # XXX Read chains from the database and initialize them.
-    pass
diff --git a/src/mailman/core/pipelines.py b/src/mailman/core/pipelines.py
index 07780dfeb..15e23a022 100644
--- a/src/mailman/core/pipelines.py
+++ b/src/mailman/core/pipelines.py
@@ -25,10 +25,9 @@ from mailman.core.i18n import _
 from mailman.interfaces.handler import IHandler
 from mailman.interfaces.pipeline import (
     DiscardMessage, IPipeline, RejectMessage)
-from mailman.utilities.modules import find_components
+from mailman.utilities.modules import add_components
 from public import public
 from zope.interface import implementer
-from zope.interface.verify import verifyObject
 
 
 dlog = logging.getLogger('mailman.debug')
@@ -139,13 +138,7 @@ class VirginPipeline(BasePipeline):
 def initialize():
     """Initialize the pipelines."""
     # Find all handlers in the registered plugins.
-    for handler_class in find_components('mailman.handlers', IHandler):
-        handler = handler_class()
-        verifyObject(IHandler, handler)
-        assert handler.name not in config.handlers, (
-            'Duplicate handler "{}" found in {}'.format(
-                handler.name, handler_class))
-        config.handlers[handler.name] = handler
+    add_components('mailman.handlers', IHandler, config.handlers)
     # Set up some pipelines.
     for pipeline_class in (OwnerPipeline, PostingPipeline, VirginPipeline):
         pipeline = pipeline_class()
diff --git a/src/mailman/core/rules.py b/src/mailman/core/rules.py
index ce89b06ba..8e0d9197c 100644
--- a/src/mailman/core/rules.py
+++ b/src/mailman/core/rules.py
@@ -19,18 +19,12 @@
 
 from mailman.config import config
 from mailman.interfaces.rules import IRule
-from mailman.utilities.modules import find_components
+from mailman.utilities.modules import add_components
 from public import public
-from zope.interface.verify import verifyObject
 
 
 @public
 def initialize():
     """Find and register all rules in all plugins."""
     # Find rules in plugins.
-    for rule_class in find_components('mailman.rules', IRule):
-        rule = rule_class()
-        verifyObject(IRule, rule)
-        assert rule.name not in config.rules, (
-            'Duplicate rule "{}" found in {}'.format(rule.name, rule_class))
-        config.rules[rule.name] = rule
+    add_components('mailman.rules', IRule, config.rules)
diff --git a/src/mailman/styles/manager.py b/src/mailman/styles/manager.py
index 2954d8e8b..1ec83b2a9 100644
--- a/src/mailman/styles/manager.py
+++ b/src/mailman/styles/manager.py
@@ -20,7 +20,7 @@
 from mailman.interfaces.configuration import ConfigurationUpdatedEvent
 from mailman.interfaces.styles import (
     DuplicateStyleError, IStyle, IStyleManager)
-from mailman.utilities.modules import find_components
+from mailman.utilities.modules import add_components
 from public import public
 from zope.component import getUtility
 from zope.interface import implementer
@@ -44,13 +44,7 @@ class StyleManager:
         paths = filter(None, (path.strip()
                               for path in config.styles.paths.splitlines()))
         for path in paths:
-            for style_class in find_components(path, IStyle):
-                style = style_class()
-                verifyObject(IStyle, style)
-                assert style.name not in self._styles, (
-                    'Duplicate style "{}" found in {}'.format(
-                        style.name, style_class))
-                self._styles[style.name] = style
+            add_components(path, IStyle, self._styles)
 
     def get(self, name):
         """See `IStyleManager`."""
diff --git a/src/mailman/utilities/modules.py b/src/mailman/utilities/modules.py
index 7aa2ac009..83a4384b7 100644
--- a/src/mailman/utilities/modules.py
+++ b/src/mailman/utilities/modules.py
@@ -25,6 +25,19 @@ from public import public
 
 
 @public
+def abstract_component(cls):
+    """Decorator preventing `find_components()` from instantiating the class.
+
+    Normally, `find_components()` instantiates any component class that
+    it finds matching the given interface.  Some component classes must not be
+    instantiated though, because they act as base classes.  Put this decorator
+    on the class definition to prevent instantiation.
+    """
+    cls.__abstract_component__ = True
+    return cls
+
+
+@public
 def find_name(dotted_name):
     """Import and return the named object in package space.
 
@@ -55,24 +68,36 @@ def call_name(dotted_name, *args, **kws):
     return named_callable(*args, **kws)
 
 
-@public
 def scan_module(module, interface):
-    """Return all the items in a module that conform to an interface.
+    """Return all the object in a module that conform to an interface.
+
+    Scan every item named in the module's `__all__`.  If that item conforms to
+    the given interface, *and* the item is not declared as an
+    `@abstract_component`, then instantiate the item and return the resulting
+    instance.
 
-    :param module: A module object.  The module's `__all__` will be scanned.
+    :param module: A module object.
     :type module: module
     :param interface: The interface that returned objects must conform to.
     :type interface: `Interface`
-    :return: The sequence of matching components.
-    :rtype: objects implementing `interface`
+    :return: The sequence of instantiated matching components.
+    :rtype: instantiated objects implementing `interface`
     """
     missing = object()
     for name in module.__all__:
         component = getattr(module, name, missing)
         assert component is not missing, (
             '%s has bad __all__: %s' % (module, name))   # pragma: no cover
-        if interface.implementedBy(component):
-            yield component
+        if (interface.implementedBy(component)
+                # We cannot use getattr() here because that will return True
+                # for all subclasses.  __abstract_component__ should *not* be
+                # inherited, meaning subclasses must declare themselves to be
+                # abstract if they also don't want to be instantiated.  Only
+                # by looking at the component's __dict__ can we know for sure
+                # where the marker has been placed.  The value of
+                # __abstract_component__ doesn't matter, only its presence.
+                and '__abstract_component__' not in component.__dict__):
+            yield component()
 
 
 @public
@@ -80,14 +105,15 @@ def find_components(package, interface):
     """Find components which conform to a given interface.
 
     Search all the modules in a given package, returning an iterator over all
-    objects found that conform to the given interface.
+    objects found that conform to the given interface, unless that object is
+    decorated with `@abstract_component`.
 
     :param package: The package path to search.
     :type package: string
     :param interface: The interface that returned objects must conform to.
     :type interface: `Interface`
-    :return: The sequence of matching components.
-    :rtype: objects implementing `interface`
+    :return: The sequence of instantiated matching components.
+    :rtype: instantiated objects implementing `interface`
     """
     for filename in resource_listdir(package, ''):
         basename, extension = os.path.splitext(filename)
@@ -102,6 +128,35 @@ def find_components(package, interface):
 
 
 @public
+def add_components(package, interface, mapping):
+    """Add components to a given mapping.
+
+    Similarly to `find_components()` this inspects all modules in a given
+    package looking for objects that conform to a given interface.  All such
+    found objects (unless decorated with `@abstract_component`) are added to
+    the given mapping, keyed by the object's `.name` attribute, which is
+    required.  It is a fatal error if that key already exists in the mapping.
+
+    :param package: The package path to search.
+    :type package: string
+    :param interface: The interface that returned objects must conform to.
+        Objects found must have a `.name` attribute containing a unique
+        string.
+    :type interface: `Interface`
+    :param mapping: The mapping to add the found components to.
+    :type mapping: A dict-like mapping.  This only needs to support
+        containment tests (e.g. `in` and `not in`) and `__setitem__()`.
+    :raises RuntimeError: when a duplicate key is found.
+    """
+    for component in find_components(package, interface):
+        if component.name in mapping:
+            raise RuntimeError(
+                'Duplicate key "{}" found in {}; previously {}'.format(
+                    component.name, component, mapping[component.name]))
+        mapping[component.name] = component
+
+
+@public
 def expand_path(url):
     """Expand a python: path, returning the absolute file system path."""
     # Is the context coming from a file system or Python path?
diff --git a/src/mailman/utilities/tests/test_modules.py b/src/mailman/utilities/tests/test_modules.py
index 068dee2cb..20735486c 100644
--- a/src/mailman/utilities/tests/test_modules.py
+++ b/src/mailman/utilities/tests/test_modules.py
@@ -23,7 +23,7 @@ import unittest
 
 from contextlib import ExitStack, contextmanager
 from mailman.interfaces.styles import IStyle
-from mailman.utilities.modules import find_components
+from mailman.utilities.modules import add_components, find_components
 from pathlib import Path
 from tempfile import TemporaryDirectory
 
@@ -50,8 +50,8 @@ class TestModuleImports(unittest.TestCase):
     def test_find_modules_with_dotfiles(self):
         # Emacs creates lock files when a single file is opened by more than
         # one user. These files look like .#<filename>.py because of which
-        # find_components tries to import them but fails. All such files should
-        # be ignored by default.
+        # find_components() tries to import them but fails. All such files
+        # should be ignored by default.
         with ExitStack() as resources:
             # Creating a temporary directory and adding it to sys.path.
             temp_package = resources.enter_context(TemporaryDirectory())
@@ -62,9 +62,9 @@ class TestModuleImports(unittest.TestCase):
             module_path = os.path.join(temp_package, 'mypackage')
             os.mkdir(module_path)
             init_file = os.path.join(module_path, '__init__.py')
+            Path(init_file).touch()
             good_file = os.path.join(module_path, 'goodfile.py')
             bad_file = os.path.join(module_path, '.#badfile.py')
-            Path(init_file).touch()
             with open(good_file, 'w', encoding='utf-8') as fp:
                 print("""\
 from public import public
@@ -97,3 +97,134 @@ class BadStyle:
                      for component
                      in find_components('mypackage', IStyle)]
             self.assertEqual(names, ['good-style'])
+
+    def test_find_components_no_instantiate(self):
+        # find_components() now instantiates the class unless it's been
+        # decorated with the @abstract_component decorator.
+        with ExitStack() as resources:
+            # Creating a temporary directory and adding it to sys.path.
+            temp_package = resources.enter_context(TemporaryDirectory())
+            resources.enter_context(hack_syspath(0, temp_package))
+            resources.callback(clean_mypackage)
+            # Create a module inside the above package along with an
+            # __init__.py file so that we can import from it.
+            module_path = os.path.join(temp_package, 'mypackage')
+            os.mkdir(module_path)
+            init_file = os.path.join(module_path, '__init__.py')
+            Path(init_file).touch()
+            component_file = os.path.join(module_path, 'components.py')
+            with open(component_file, 'w', encoding='utf-8') as fp:
+                print("""\
+from mailman.interfaces.styles import IStyle
+from mailman.utilities.modules import abstract_component
+from public import public
+from zope.interface import implementer
+
+@public
+@implementer(IStyle)
+class ConcreteStyle:
+    name = 'concrete-style'
+    def apply(self):
+        pass
+
+@public
+@implementer(IStyle)
+@abstract_component
+class AbstractStyle:
+    name = 'abstract-style'
+    def apply(self):
+        pass
+""", file=fp)
+            names = [component.name
+                     for component
+                     in find_components('mypackage', IStyle)]
+            self.assertEqual(names, ['concrete-style'])
+
+    def test_add_components(self):
+        with ExitStack() as resources:
+            # Creating a temporary directory and adding it to sys.path.
+            temp_package = resources.enter_context(TemporaryDirectory())
+            resources.enter_context(hack_syspath(0, temp_package))
+            resources.callback(clean_mypackage)
+            # Create a module inside the above package along with an
+            # __init__.py file so that we can import from it.
+            module_path = os.path.join(temp_package, 'mypackage')
+            os.mkdir(module_path)
+            init_file = os.path.join(module_path, '__init__.py')
+            Path(init_file).touch()
+            component_file = os.path.join(module_path, 'components.py')
+            with open(component_file, 'w', encoding='utf-8') as fp:
+                print("""\
+from mailman.interfaces.styles import IStyle
+from mailman.utilities.modules import abstract_component
+from public import public
+from zope.interface import implementer
+
+@public
+@implementer(IStyle)
+class StyleA:
+    name = 'styleA'
+    def apply(self):
+        pass
+
+@public
+@implementer(IStyle)
+@abstract_component
+class StyleB:
+    name = 'styleB'
+    def apply(self):
+        pass
+
+@public
+@implementer(IStyle)
+class StyleC:
+    name = 'styleC'
+    def apply(self):
+        pass
+""", file=fp)
+            styles = {}
+            add_components('mypackage', IStyle, styles)
+            self.assertEqual(set(styles), {'styleA', 'styleC'})
+
+    def test_add_components_duplicates(self):
+        # A duplicate name exists, so a RuntimeError is raised.
+        with ExitStack() as resources:
+            # Creating a temporary directory and adding it to sys.path.
+            temp_package = resources.enter_context(TemporaryDirectory())
+            resources.enter_context(hack_syspath(0, temp_package))
+            resources.callback(clean_mypackage)
+            # Create a module inside the above package along with an
+            # __init__.py file so that we can import from it.
+            module_path = os.path.join(temp_package, 'mypackage')
+            os.mkdir(module_path)
+            init_file = os.path.join(module_path, '__init__.py')
+            Path(init_file).touch()
+            component_file = os.path.join(module_path, 'components.py')
+            with open(component_file, 'w', encoding='utf-8') as fp:
+                print("""\
+from mailman.interfaces.styles import IStyle
+from mailman.utilities.modules import abstract_component
+from public import public
+from zope.interface import implementer
+
+@public
+@implementer(IStyle)
+class StyleA1:
+    name = 'styleA'
+    def apply(self):
+        pass
+
+@public
+@implementer(IStyle)
+class StyleA2:
+    name = 'styleA'
+    def apply(self):
+        pass
+""", file=fp)
+            with self.assertRaisesRegex(
+                    RuntimeError,
+                    'Duplicate key "styleA" found in '
+                    '<mypackage.components.StyleA2 object at .*>; '
+                    'previously <mypackage.components.StyleA1 object at '
+                    '.*>'):
+                add_components('mypackage', IStyle, {})