Merge pull request #45 from zopefoundation/issue43

Simplify exception chaining and nested exception error messages. 

7 files changed, 196 insertions, 109 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index 93d7bef..19389a2 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -4,7 +4,8 @@ Changes
 4.2.3 (unreleased)
 ------------------
 
-- Nothing changed yet.
+- Simplify exception chaining and nested exception error messages.
+  See `issue 43 <https://github.com/zopefoundation/zope.configuration/issues/43>`_.
 
 
 4.2.2 (2018-09-27)
diff --git a/docs/narr.rst b/docs/narr.rst
index fc7d804..fa60dc4 100644
--- a/docs/narr.rst
+++ b/docs/narr.rst
@@ -1162,16 +1162,15 @@ redefine our directives:
 .. doctest::
 
    >>> from zope.configuration.xmlconfig import string
-   >>> from zope.configuration.xmlconfig import ZopeXMLConfigurationError
-   >>> try:
-   ...    v = string(
+   >>> from zope.configuration.exceptions import ConfigurationError
+   >>> v = string(
    ...      '<text xmlns="http://sample.namespaces.zope.org/schema" name="x" />',
    ...      context)
-   ... except ZopeXMLConfigurationError as e:
-   ...   v = e
-   >>> print(v)
-   File "<string>", line 1.0
-       ConfigurationError: The directive ('http://sample.namespaces.zope.org/schema', 'text') cannot be used in this context
+   Traceback (most recent call last):
+   ...
+   zope.configuration.exceptions.ConfigurationError: The directive ('http://sample.namespaces.zope.org/schema', 'text') cannot be used in this context
+     File "<string>", line 1.0
+
 
 Let's see what happens if we declare duplicate fields:
 
@@ -1187,7 +1186,7 @@ Let's see what happens if we declare duplicate fields:
    ...      </schema>
    ...      ''',
    ...      context)
-   ... except ZopeXMLConfigurationError as e:
+   ... except ConfigurationError as e:
    ...   v = e
    >>> print(v)
    File "<string>", line 5.7-5.24
diff --git a/src/zope/configuration/config.py b/src/zope/configuration/config.py
index e7ba289..0123021 100644
--- a/src/zope/configuration/config.py
+++ b/src/zope/configuration/config.py
@@ -27,6 +27,7 @@ from zope.schema import URI
 from zope.schema import ValidationError
 
 from zope.configuration.exceptions import ConfigurationError
+from zope.configuration.exceptions import ConfigurationWrapperError
 from zope.configuration.interfaces import IConfigurationContext
 from zope.configuration.interfaces import IGroupingContext
 from zope.configuration.fields import GlobalInterface
@@ -41,7 +42,6 @@ __all__ = [
     'ConfigurationContext',
     'ConfigurationAdapterRegistry',
     'ConfigurationMachine',
-    'ConfigurationExecutionError',
     'IStackItem',
     'SimpleStackItem',
     'RootStackItem',
@@ -74,10 +74,14 @@ testns = 'http://namespaces.zope.org/test'
 
 
 class ConfigurationContext(object):
-    """Mix-in that implements IConfigurationContext
+    """
+    Mix-in for implementing.
+    :class:`zope.configuration.interfaces.IConfigurationContext`.
 
-    Subclasses provide a ``package`` attribute and a ``basepath``
-    attribute.  If the base path is not None, relative paths are
+    Note that this class itself does not actually declare that it
+    implements that interface; the subclass must do that. In addition,
+    subclasses must provide a ``package`` attribute and a ``basepath``
+    attribute. If the base path is not None, relative paths are
     converted to absolute paths using the the base path. If the
     package is not none, relative imports are performed relative to
     the package.
@@ -91,34 +95,38 @@ class ConfigurationContext(object):
     attribute.
 
     The include path is appended to each action and is used when
-    resolving conflicts among actions.  Normally, only the a
+    resolving conflicts among actions. Normally, only the a
     ConfigurationMachine provides the actions attribute. Decorators
     simply use the actions of the context they decorate. The
-    ``includepath`` attribute is a tuple of names.  Each name is
+    ``includepath`` attribute is a tuple of names. Each name is
     typically the name of an included configuration file.
 
     The ``info`` attribute contains descriptive information helpful
-    when reporting errors.  If not set, it defaults to an empty string.
-
-    The actions attribute is a sequence of dictionaries where each dictionary
-    has the following keys:
+    when reporting errors. If not set, it defaults to an empty string.
 
-      - ``discriminator``, a value that identifies the action. Two actions
-        that have the same (non None) discriminator conflict.
+    The actions attribute is a sequence of dictionaries where each
+    dictionary has the following keys:
 
-      - ``callable``, an object that is called to execute the action,
+        - ``discriminator``, a value that identifies the action. Two
+          actions that have the same (non None) discriminator
+          conflict.
 
-      - ``args``, positional arguments for the action
+        - ``callable``, an object that is called to execute the
+          action,
 
-      - ``kw``, keyword arguments for the action
+        - ``args``, positional arguments for the action
 
-      - ``includepath``, a tuple of include file names (defaults to ())
+        - ``kw``, keyword arguments for the action
 
-      - ``info``, an object that has descriptive information about
-        the action (defaults to '')
+        - ``includepath``, a tuple of include file names (defaults to
+          ())
 
+        - ``info``, an object that has descriptive information about
+          the action (defaults to '')
     """
 
+    # pylint:disable=no-member
+
     def __init__(self):
         super(ConfigurationContext, self).__init__()
         self._seen_files = set()
@@ -666,8 +674,8 @@ class ConfigurationMachine(ConfigurationAdapterRegistry, ConfigurationContext):
     info = ''
 
     #: These `Exception` subclasses are allowed to be raised from `execute_actions`
-    #: without being re-wrapped into a `ConfigurationExecutionError`. (`BaseException`
-    #: instances are never wrapped.)
+    #: without being re-wrapped into a `~.ConfigurationError`. (`BaseException`
+    #: and other `~.ConfigurationError` instances are never wrapped.)
     #:
     #: Users of instances of this class may modify this before calling `execute_actions`
     #: if they need to propagate specific exceptions.
@@ -727,9 +735,8 @@ class ConfigurationMachine(ConfigurationAdapterRegistry, ConfigurationContext):
             [('f', (1,), {}), ('f', (2,), {})]
 
         If the action raises an error, we convert it to a
-        ConfigurationExecutionError.
+        `~.ConfigurationError`.
 
-            >>> from zope.configuration.config import ConfigurationExecutionError
             >>> output = []
             >>> def bad():
             ...    bad.xxx
@@ -739,22 +746,35 @@ class ConfigurationMachine(ConfigurationAdapterRegistry, ConfigurationContext):
             ...   (2, f, (2,)),
             ...   (3, bad, (), {}, (), 'oops')
             ...   ]
-            >>> try:
-            ...    v = context.execute_actions()
-            ... except ConfigurationExecutionError as e:
-            ...    v = e
-            >>> lines = str(v).splitlines()
-            >>> 'AttributeError' in lines[0]
-            True
-            >>> lines[0].endswith("'function' object has no attribute 'xxx'")
-            True
-            >>> lines[1:]
-            ['  in:', '  oops']
+
+            >>> context.execute_actions()
+            Traceback (most recent call last):
+            ...
+            zope.configuration.config.ConfigurationExecutionError: oops
+                AttributeError: 'function' object has no attribute 'xxx'
 
         Note that actions executed before the error still have an effect:
 
             >>> output
             [('f', (1,), {}), ('f', (2,), {})]
+
+        If the exception was already a `~.ConfigurationError`, it is raised
+        as-is with the action's ``info`` added.
+
+            >>> def bad():
+            ...     raise ConfigurationError("I'm bad")
+            >>> context.actions = [
+            ...   (1, f, (1,)),
+            ...   (1, f, (11,), {}, ('x', )),
+            ...   (2, f, (2,)),
+            ...   (3, bad, (), {}, (), 'oops')
+            ...   ]
+            >>> context.execute_actions()
+            Traceback (most recent call last):
+            ...
+            zope.configuration.exceptions.ConfigurationError: I'm bad
+                oops
+
         """
         pass_through_exceptions = self.pass_through_exceptions
         if testing:
@@ -769,29 +789,23 @@ class ConfigurationMachine(ConfigurationAdapterRegistry, ConfigurationContext):
                 info = action['info']
                 try:
                     callable(*args, **kw)
+                except ConfigurationError as ex:
+                    ex.add_details(info)
+                    raise
                 except pass_through_exceptions:
                     raise
                 except Exception:
-                    t, v, tb = sys.exc_info()
-                    try:
-                        reraise(ConfigurationExecutionError(t, v, info),
-                                None, tb)
-                    finally:
-                        del t, v, tb
-
+                    # Wrap it up and raise.
+                    reraise(ConfigurationExecutionError(info, sys.exc_info()[1]),
+                            None, sys.exc_info()[2])
         finally:
             if clear:
                 del self.actions[:]
 
-class ConfigurationExecutionError(ConfigurationError):
+class ConfigurationExecutionError(ConfigurationWrapperError):
     """
     An error occurred during execution of a configuration action
     """
-    def __init__(self, etype, evalue, info):
-        self.etype, self.evalue, self.info = etype, evalue, info
-
-    def __str__(self): # pragma: no cover
-        return "%s: %s\n  in:\n  %s" % (self.etype, self.evalue, self.info)
 
 ##############################################################################
 # Stack items
@@ -1671,15 +1685,16 @@ def toargs(context, schema, data):
             try:
                 args[str(name)] = field.fromUnicode(s)
             except ValidationError as v:
-                reraise(ConfigurationError("Invalid value for", n, str(v)),
+                reraise(ConfigurationError("Invalid value for %r" % (n)).add_details(v),
                         None, sys.exc_info()[2])
         elif field.required:
             # if the default is valid, we can use that:
             default = field.default
             try:
                 field.validate(default)
-            except ValidationError:
-                raise ConfigurationError("Missing parameter:", n)
+            except ValidationError as v:
+                reraise(ConfigurationError("Missing parameter: %r" % (n,)).add_details(v),
+                        None, sys.exc_info()[2])
             args[str(name)] = default
 
     if data:
@@ -1801,9 +1816,10 @@ def resolveConflicts(actions):
 class ConfigurationConflictError(ConfigurationError):
 
     def __init__(self, conflicts):
+        super(ConfigurationConflictError, self).__init__()
         self._conflicts = conflicts
 
-    def __str__(self): #pragma NO COVER
+    def _with_details(self, opening, detail_formatter):
         r = ["Conflicting configuration actions"]
         for discriminator, infos in sorted(self._conflicts.items()):
             r.append("  For: %s" % (discriminator, ))
@@ -1811,7 +1827,8 @@ class ConfigurationConflictError(ConfigurationError):
                 for line in text_type(info).rstrip().split(u'\n'):
                     r.append(u"    " + line)
 
-        return "\n".join(r)
+        opening = "\n".join(r)
+        return super(ConfigurationConflictError, self)._with_details(opening, detail_formatter)
 
 
 ##############################################################################
diff --git a/src/zope/configuration/exceptions.py b/src/zope/configuration/exceptions.py
index dce50a7..abac17a 100644
--- a/src/zope/configuration/exceptions.py
+++ b/src/zope/configuration/exceptions.py
@@ -14,6 +14,8 @@
 """Standard configuration errors
 """
 
+import traceback
+
 __all__ = [
     'ConfigurationError',
 ]
@@ -21,3 +23,48 @@ __all__ = [
 class ConfigurationError(Exception):
     """There was an error in a configuration
     """
+
+    # A list of strings or things that can be converted to strings,
+    # added by append_details as we walk back up the call/include stack.
+    # We will print them in reverse order so that the most recent detail is
+    # last.
+    _details = ()
+
+    def add_details(self, info):
+        if isinstance(info, BaseException):
+            info = traceback.format_exception_only(type(info), info)
+            # Trim trailing newline
+            info[-1] = info[-1].rstrip()
+            self._details += tuple(info)
+        else:
+            self._details += (info,)
+        return self
+
+    def _with_details(self, opening, detail_formatter):
+        lines = ['    ' + detail_formatter(detail) for detail in self._details]
+        lines.append(opening)
+        lines.reverse()
+        return '\n'.join(lines)
+
+    def __str__(self):
+        s = super(ConfigurationError, self).__str__()
+        return self._with_details(s, str)
+
+    def __repr__(self):
+        s = super(ConfigurationError, self).__repr__()
+        return self._with_details(s, repr)
+
+
+class ConfigurationWrapperError(ConfigurationError):
+
+    USE_INFO_REPR = False
+
+    def __init__(self, info, exception):
+        super(ConfigurationWrapperError, self).__init__(repr(info) if self.USE_INFO_REPR else info)
+        self.add_details(exception)
+
+        # This stuff is undocumented and not used but we store
+        # for backwards compatibility
+        self.info = info
+        self.etype = type(exception)
+        self.evalue = exception
diff --git a/src/zope/configuration/tests/test_config.py b/src/zope/configuration/tests/test_config.py
index 4bd5930..ab21589 100644
--- a/src/zope/configuration/tests/test_config.py
+++ b/src/zope/configuration/tests/test_config.py
@@ -1789,7 +1789,18 @@ class Test_toargs(unittest.TestCase):
         with self.assertRaises(ConfigurationError) as exc:
             self._callFUT(context, ISchema, {})
         self.assertEqual(exc.exception.args,
-                         ('Missing parameter:', 'no_default'))
+                         ("Missing parameter: 'no_default'",))
+
+        # It includes the details of any validation failure;
+        # The rendering of the nested exception varies by Python version,
+        # sadly.
+        exception_str = str(exc.exception)
+        self.assertTrue(exception_str.startswith(
+            "Missing parameter: 'no_default'\n"
+        ), exception_str)
+        self.assertTrue(exception_str.endswith(
+            "RequiredMissing: no_default"
+        ), exception_str)
 
     def test_w_field_missing_but_default(self):
         from zope.interface import Interface
@@ -1810,8 +1821,16 @@ class Test_toargs(unittest.TestCase):
         with self.assertRaises(ConfigurationError) as exc:
             self._callFUT(context, ISchema, {'count': '-1'})
         self.assertEqual(exc.exception.args,
-                         ('Invalid value for', 'count', '(-1, 0)'))
+                         ("Invalid value for 'count'",))
 
+        for meth in str, repr:
+            exception_str = meth(exc.exception)
+            self.assertIn(
+                "Invalid value for",
+                exception_str)
+            self.assertIn(
+                "TooSmall: (-1, 0)",
+                exception_str)
 
 class Test_expand_action(unittest.TestCase):
 
@@ -1974,6 +1993,14 @@ class Test_resolveConflicts(unittest.TestCase):
             "For: ('a', 1)\n    conflict!\n    conflict2!",
             str(exc.exception))
 
+        exc.exception.add_details('a detail')
+
+        self.assertEqual(
+            "Conflicting configuration actions\n  "
+            "For: ('a', 1)\n    conflict!\n    conflict2!\n"
+            "    a detail",
+            str(exc.exception))
+
     def test_wo_discriminators_final_sorting_order(self):
         from zope.configuration.config import expand_action
         def _a():
diff --git a/src/zope/configuration/tests/test_xmlconfig.py b/src/zope/configuration/tests/test_xmlconfig.py
index e75a8f0..3cb1270 100644
--- a/src/zope/configuration/tests/test_xmlconfig.py
+++ b/src/zope/configuration/tests/test_xmlconfig.py
@@ -29,6 +29,7 @@ BVALUE = u'bvalue'
 # pylint:disable=protected-access
 
 class ZopeXMLConfigurationErrorTests(unittest.TestCase):
+    maxDiff = None
 
     def _getTargetClass(self):
         from zope.configuration.xmlconfig import ZopeXMLConfigurationError
@@ -38,11 +39,16 @@ class ZopeXMLConfigurationErrorTests(unittest.TestCase):
         return self._getTargetClass()(*args, **kw)
 
     def test___str___uses_repr_of_info(self):
-        zxce = self._makeOne('info', Exception, 'value')
-        self.assertEqual(str(zxce), "'info'\n    Exception: value")
+        zxce = self._makeOne('info', Exception('value'))
+        self.assertEqual(
+            str(zxce),
+            "'info'\n    Exception: value"
+        )
+
 
 
 class ZopeSAXParseExceptionTests(unittest.TestCase):
+    maxDiff = None
 
     def _getTargetClass(self):
         from zope.configuration.xmlconfig import ZopeSAXParseException
@@ -52,12 +58,16 @@ class ZopeSAXParseExceptionTests(unittest.TestCase):
         return self._getTargetClass()(*args, **kw)
 
     def test___str___not_a_sax_error(self):
-        zxce = self._makeOne(Exception('Not a SAX error'))
-        self.assertEqual(str(zxce), "Not a SAX error")
+        zxce = self._makeOne("info", Exception('Not a SAX error'))
+        self.assertEqual(
+            str(zxce),
+            "info\n    Exception: Not a SAX error")
 
     def test___str___w_a_sax_error(self):
-        zxce = self._makeOne(Exception('filename.xml:24:32:WAAA'))
-        self.assertEqual(str(zxce), 'File "filename.xml", line 24.32, WAAA')
+        zxce = self._makeOne("info", Exception('filename.xml:24:32:WAAA'))
+        self.assertEqual(
+            str(zxce),
+            'info\n    Exception: filename.xml:24:32:WAAA')
 
 
 class ParserInfoTests(unittest.TestCase):
@@ -385,7 +395,7 @@ class Test_processxmlfile(unittest.TestCase):
         registerCommonDirectives(context)
         with self.assertRaises(ZopeSAXParseException) as exc:
             self._callFUT(StringIO(), context)
-        self.assertEqual(str(exc.exception._v),
+        self.assertEqual(str(exc.exception.evalue),
                          '<string>:1:0: no element found')
 
     def test_w_valid_xml_fp(self):
diff --git a/src/zope/configuration/xmlconfig.py b/src/zope/configuration/xmlconfig.py
index e979e13..4fc0d0d 100644
--- a/src/zope/configuration/xmlconfig.py
+++ b/src/zope/configuration/xmlconfig.py
@@ -40,14 +40,13 @@ from zope.configuration.config import GroupingContextDecorator
 from zope.configuration.config import GroupingStackItem
 from zope.configuration.config import resolveConflicts
 from zope.configuration.exceptions import ConfigurationError
+from zope.configuration.exceptions import ConfigurationWrapperError
 from zope.configuration.fields import GlobalObject
 from zope.configuration.zopeconfigure import IZopeConfigure
 from zope.configuration.zopeconfigure import ZopeConfigure
 from zope.configuration._compat import reraise
 
 __all__ = [
-    'ZopeXMLConfigurationError',
-    'ZopeSAXParseException',
     'ParserInfo',
     'ConfigurationHandler',
     'processxmlfile',
@@ -70,7 +69,7 @@ ZCML_NAMESPACE = "http://namespaces.zope.org/zcml"
 ZCML_CONDITION = (ZCML_NAMESPACE, u"condition")
 
 
-class ZopeXMLConfigurationError(ConfigurationError):
+class ZopeXMLConfigurationError(ConfigurationWrapperError):
     """
     Zope XML Configuration error
 
@@ -80,41 +79,28 @@ class ZopeXMLConfigurationError(ConfigurationError):
     Example
 
         >>> from zope.configuration.xmlconfig import ZopeXMLConfigurationError
-        >>> v = ZopeXMLConfigurationError("blah", AttributeError, "xxx")
+        >>> v = ZopeXMLConfigurationError("blah", AttributeError("xxx"))
         >>> print(v)
         'blah'
             AttributeError: xxx
     """
-    def __init__(self, info, etype, evalue):
-        self.info, self.etype, self.evalue = info, etype, evalue
 
-    def __str__(self):
-        # Only use the repr of the info. This is because we expect to
-        # get a parse info and we only want the location information.
-        return "%s\n    %s: %s" % (
-            repr(self.info), self.etype.__name__, self.evalue)
+    USE_INFO_REPR = True
+
 
-class ZopeSAXParseException(ConfigurationError):
+class ZopeSAXParseException(ConfigurationWrapperError):
     """
-    Sax Parser errors, reformatted in an emacs friendly way.
+    Sax Parser errors as a ConfigurationError.
 
     Example
 
         >>> from zope.configuration.xmlconfig import ZopeSAXParseException
-        >>> v = ZopeSAXParseException("foo.xml:12:3:Not well formed")
+        >>> v = ZopeSAXParseException("info", Exception("foo.xml:12:3:Not well formed"))
         >>> print(v)
-        File "foo.xml", line 12.3, Not well formed
+        info
+            Exception: foo.xml:12:3:Not well formed
     """
-    def __init__(self, v):
-        self._v = v
 
-    def __str__(self):
-        v = self._v
-        s = tuple(str(v).split(':'))
-        if len(s) == 4:
-            return 'File "%s", line %s.%s, %s' % s
-        else:
-            return str(v)
 
 class ParserInfo(object):
     r"""
@@ -238,6 +224,16 @@ class ConfigurationHandler(ContentHandler):
     def characters(self, text):
         self.context.getInfo().characters(text)
 
+    def _handle_exception(self, ex, info):
+        if self.testing:
+            raise
+        if isinstance(ex, ConfigurationError):
+            ex.add_details(repr(info))
+            raise
+
+        exc = ZopeXMLConfigurationError(info, ex)
+        reraise(exc, None, sys.exc_info()[2])
+
     def startElementNS(self, name, qname, attrs):
         if self.ignore_depth:
             self.ignore_depth += 1
@@ -268,13 +264,8 @@ class ConfigurationHandler(ContentHandler):
 
         try:
             self.context.begin(name, data, info)
-        except Exception:
-            if self.testing:
-                raise
-            reraise(ZopeXMLConfigurationError(info,
-                                              sys.exc_info()[0],
-                                              sys.exc_info()[1]),
-                    None, sys.exc_info()[2])
+        except Exception as ex:
+            self._handle_exception(ex, info)
 
         self.context.setInfo(info)
 
@@ -398,13 +389,8 @@ class ConfigurationHandler(ContentHandler):
 
         try:
             self.context.end()
-        except Exception:
-            if self.testing:
-                raise
-            reraise(ZopeXMLConfigurationError(info,
-                                              sys.exc_info()[0],
-                                              sys.exc_info()[1]),
-                    None, sys.exc_info()[2])
+        except Exception as ex:
+            self._handle_exception(ex, info)
 
 
 def processxmlfile(file, context, testing=False):
@@ -420,7 +406,7 @@ def processxmlfile(file, context, testing=False):
     try:
         parser.parse(src)
     except SAXParseException:
-        reraise(ZopeSAXParseException(sys.exc_info()[1]),
+        reraise(ZopeSAXParseException(file, sys.exc_info()[1]),
                 None, sys.exc_info()[2])