Merge pull request #1029 from octodns/error-context

config/validation errors with source context

CHANGELOG.md

@@ -1,3 +1,10 @@
+## v1.1.0 - 2023-??-?? - ???
+
+* Add context to general configuration and Record validation, e.g.
+  Some problem at filename.yaml, line 42, column 14. Our custom Yaml Loaders
+  attach this context information, arbitrary string. Other providers may do so
+  by creating ContextDict to pass as `data` into Record.new.
+
 ## v1.0.0 - 2023-07-30 - The One
 
 1.0 marks a point at which we can formally deprecate things that will be

octodns/context.py

@@ -0,0 +1,20 @@
+#
+#
+#
+
+
+class ContextDict(dict):
+    '''
+    This is used by things that call `Record.new` to pass in a `data`
+    dictionary that includes some context as to where the data came from to be
+    printed along with exceptions or validations of the record.
+
+    It breaks lots of stuff if we stored the context in an extra key and the
+    python `dict` object doesn't allow you to set attributes on the object so
+    this is a very thin wrapper around `dict` that allows us to have a context
+    attribute.
+    '''
+
+    def __init__(self, *args, context=None, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.context = context

octodns/manager.py

@@ -190,9 +190,11 @@ class Manager(object):
             except KeyError:
                 self.log.exception('Invalid provider class')
                 raise ManagerException(
-                    f'Provider {provider_name} is missing class'
+                    f'Provider {provider_name} is missing class, {provider_config.context}'
                 )
-            _class, module, version = self._get_named_class('provider', _class)
+            _class, module, version = self._get_named_class(
+                'provider', _class, provider_config.context
+            )
             kwargs = self._build_kwargs(provider_config)
             try:
                 providers[provider_name] = _class(provider_name, **kwargs)
@@ -205,7 +207,7 @@ class Manager(object):
             except TypeError:
                 self.log.exception('Invalid provider config')
                 raise ManagerException(
-                    'Incorrect provider config for ' + provider_name
+                    f'Incorrect provider config for {provider_name}, {provider_config.context}'
                 )
 
         return providers
@@ -218,9 +220,11 @@ class Manager(object):
             except KeyError:
                 self.log.exception('Invalid processor class')
                 raise ManagerException(
-                    f'Processor {processor_name} is missing class'
+                    f'Processor {processor_name} is missing class, {processor_config.context}'
                 )
-            _class, module, version = self._get_named_class('processor', _class)
+            _class, module, version = self._get_named_class(
+                'processor', _class, processor_config.context
+            )
             kwargs = self._build_kwargs(processor_config)
             try:
                 processors[processor_name] = _class(processor_name, **kwargs)
@@ -233,22 +237,23 @@ class Manager(object):
             except TypeError:
                 self.log.exception('Invalid processor config')
                 raise ManagerException(
-                    'Incorrect processor config for ' + processor_name
+                    f'Incorrect processor config for {processor_name}, {processor_config.context}'
                 )
         return processors
 
     def _config_plan_outputs(self, plan_outputs_config):
         plan_outputs = {}
         for plan_output_name, plan_output_config in plan_outputs_config.items():
+            context = getattr(plan_output_config, 'context', '')
             try:
                 _class = plan_output_config.pop('class')
             except KeyError:
                 self.log.exception('Invalid plan_output class')
                 raise ManagerException(
-                    f'plan_output {plan_output_name} is missing class'
+                    f'plan_output {plan_output_name} is missing class, {context}'
                 )
             _class, module, version = self._get_named_class(
-                'plan_output', _class
+                'plan_output', _class, context
             )
             kwargs = self._build_kwargs(plan_output_config)
             try:
@@ -266,8 +271,9 @@ class Manager(object):
             except TypeError:
                 self.log.exception('Invalid plan_output config')
                 raise ManagerException(
-                    'Incorrect plan_output config for ' + plan_output_name
+                    f'Incorrect plan_output config for {plan_output_name}, {context}'
                 )
+
         return plan_outputs
 
     def _try_version(self, module_name, module=None, version=None):
@@ -298,7 +304,7 @@ class Manager(object):
             version = self._try_version(current)
         return module, version or 'n/a'
 
-    def _get_named_class(self, _type, _class):
+    def _get_named_class(self, _type, _class, context):
         try:
             module_name, class_name = _class.rsplit('.', 1)
             module, version = self._import_module(module_name)
@@ -306,7 +312,9 @@ class Manager(object):
             self.log.exception(
                 '_get_{}_class: Unable to import module %s', _class
             )
-            raise ManagerException(f'Unknown {_type} class: {_class}')
+            raise ManagerException(
+                f'Unknown {_type} class: {_class}, {context}'
+            )
 
         try:
             return getattr(module, class_name), module_name, version
@@ -316,7 +324,9 @@ class Manager(object):
                 class_name,
                 module,
             )
-            raise ManagerException(f'Unknown {_type} class: {_class}')
+            raise ManagerException(
+                f'Unknown {_type} class: {_class}, {context}'
+            )
 
     def _build_kwargs(self, source):
         # Build up the arguments we need to pass to the provider
@@ -330,8 +340,7 @@ class Manager(object):
                     except KeyError:
                         self.log.exception('Invalid provider config')
                         raise ManagerException(
-                            'Incorrect provider config, '
-                            'missing env var ' + env_var
+                            f'Incorrect provider config, missing env var {env_var}, {source.context}'
                         )
             except AttributeError:
                 pass

octodns/record/base.py

@@ -46,14 +46,21 @@ class Record(EqualityTupleMixin):
             reasons.append('invalid record, whitespace is not allowed')
 
         fqdn = f'{name}.{zone.name}' if name else zone.name
+        context = getattr(data, 'context', None)
         try:
             _type = data['type']
         except KeyError:
-            raise Exception(f'Invalid record {idna_decode(fqdn)}, missing type')
+            msg = f'Invalid record {idna_decode(fqdn)}, missing type'
+            if context:
+                msg += f', {context}'
+            raise Exception(msg)
         try:
             _class = cls._CLASSES[_type]
         except KeyError:
-            raise Exception(f'Unknown record type: "{_type}"')
+            msg = f'Unknown record type: "{_type}"'
+            if context:
+                msg += f', {context}'
+            raise Exception(msg)
         reasons.extend(_class.validate(name, fqdn, data))
         try:
             lenient |= data['octodns']['lenient']
@@ -61,9 +68,11 @@ class Record(EqualityTupleMixin):
             pass
         if reasons:
             if lenient:
-                cls.log.warning(ValidationError.build_message(fqdn, reasons))
+                cls.log.warning(
+                    ValidationError.build_message(fqdn, reasons, context)
+                )
             else:
-                raise ValidationError(fqdn, reasons)
+                raise ValidationError(fqdn, reasons, context)
         return _class(zone, name, data, source=source)
 
     @classmethod

octodns/record/exception.py

@@ -11,11 +11,16 @@ class RecordException(Exception):
 
 class ValidationError(RecordException):
     @classmethod
-    def build_message(cls, fqdn, reasons):
+    def build_message(cls, fqdn, reasons, context=None):
         reasons = '\n  - '.join(reasons)
-        return f'Invalid record "{idna_decode(fqdn)}"\n  - {reasons}'
+        msg = f'Invalid record "{idna_decode(fqdn)}"'
+        if context:
+            msg += f', {context}'
+        msg += f'\n  - {reasons}'
+        return msg
 
-    def __init__(self, fqdn, reasons):
-        super().__init__(self.build_message(fqdn, reasons))
+    def __init__(self, fqdn, reasons, context=None):
+        super().__init__(self.build_message(fqdn, reasons, context))
         self.fqdn = fqdn
         self.reasons = reasons
+        self.context = context

octodns/yaml.py

@@ -7,16 +7,35 @@ from yaml import SafeDumper, SafeLoader, dump, load
 from yaml.constructor import ConstructorError
 from yaml.representer import SafeRepresenter
 
+from .context import ContextDict
+
 _natsort_key = natsort_keygen()
 
 
+class ContextLoader(SafeLoader):
+    def _pairs(self, node):
+        self.flatten_mapping(node)
+        pairs = self.construct_pairs(node)
+        start_mark = node.start_mark
+        context = f'{start_mark.name}, line {start_mark.line+1}, column {start_mark.column+1}'
+        return ContextDict(pairs, context=context), pairs, context
+
+    def _construct(self, node):
+        return self._pairs(node)[0]
+
+
+ContextLoader.add_constructor(
+    ContextLoader.DEFAULT_MAPPING_TAG, ContextLoader._construct
+)
+
+
 # Found http://stackoverflow.com/a/21912744 which guided me on how to hook in
 # here
-class SortEnforcingLoader(SafeLoader):
+class SortEnforcingLoader(ContextLoader):
     def _construct(self, node):
-        self.flatten_mapping(node)
-        ret = self.construct_pairs(node)
-        keys = [d[0] for d in ret]
+        ret, pairs, context = self._pairs(node)
+
+        keys = [d[0] for d in pairs]
         keys_sorted = sorted(keys, key=_natsort_key)
         for key in keys:
             expected = keys_sorted.pop(0)
@@ -25,9 +44,10 @@ class SortEnforcingLoader(SafeLoader):
                     None,
                     None,
                     'keys out of order: '
-                    f'expected {expected} got {key} at ' + str(node.start_mark),
+                    f'expected {expected} got {key} at {context}',
                 )
-        return dict(ret)
+
+        return ret
 
 
 SortEnforcingLoader.add_constructor(
@@ -36,7 +56,7 @@ SortEnforcingLoader.add_constructor(
 
 
 def safe_load(stream, enforce_order=True):
-    return load(stream, SortEnforcingLoader if enforce_order else SafeLoader)
+    return load(stream, SortEnforcingLoader if enforce_order else ContextLoader)
 
 
 class SortingDumper(SafeDumper):

tests/test_octodns_manager.py

@@ -104,13 +104,15 @@ class TestManager(TestCase):
         with self.assertRaises(ManagerException) as ctx:
             name = 'bad-plan-output-missing-class.yaml'
             Manager(get_config_filename(name)).sync()
-        self.assertEqual('plan_output bad is missing class', str(ctx.exception))
+        self.assertTrue(
+            'plan_output bad is missing class' in str(ctx.exception)
+        )
 
     def test_bad_plan_output_config(self):
         with self.assertRaises(ManagerException) as ctx:
             Manager(get_config_filename('bad-plan-output-config.yaml')).sync()
-        self.assertEqual(
-            'Incorrect plan_output config for bad', str(ctx.exception)
+        self.assertTrue(
+            'Incorrect plan_output config for bad' in str(ctx.exception)
         )
 
     def test_source_only_as_a_target(self):

tests/test_octodns_record.py

@@ -22,6 +22,7 @@ from octodns.record import (
     ValidationError,
     ValuesMixin,
 )
+from octodns.yaml import ContextDict
 from octodns.zone import Zone
 
 
@@ -572,3 +573,58 @@ class TestRecordValidation(TestCase):
             },
             lenient=True,
         )
+
+    def test_validation_context(self):
+        # fails validation, no context
+        with self.assertRaises(ValidationError) as ctx:
+            Record.new(
+                self.zone, 'www', {'type': 'A', 'ttl': -1, 'value': '1.2.3.4'}
+            )
+        self.assertFalse(', line' in str(ctx.exception))
+
+        # fails validation, with context
+        with self.assertRaises(ValidationError) as ctx:
+            Record.new(
+                self.zone,
+                'www',
+                ContextDict(
+                    {'type': 'A', 'ttl': -1, 'value': '1.2.3.4'},
+                    context='needle',
+                ),
+            )
+        self.assertTrue('needle' in str(ctx.exception))
+
+    def test_invalid_type_context(self):
+        # fails validation, no context
+        with self.assertRaises(Exception) as ctx:
+            Record.new(
+                self.zone, 'www', {'type': 'X', 'ttl': 42, 'value': '1.2.3.4'}
+            )
+        self.assertFalse(', line' in str(ctx.exception))
+
+        # fails validation, with context
+        with self.assertRaises(Exception) as ctx:
+            Record.new(
+                self.zone,
+                'www',
+                ContextDict(
+                    {'type': 'X', 'ttl': 42, 'value': '1.2.3.4'},
+                    context='needle',
+                ),
+            )
+        self.assertTrue('needle' in str(ctx.exception))
+
+    def test_missing_type_context(self):
+        # fails validation, no context
+        with self.assertRaises(Exception) as ctx:
+            Record.new(self.zone, 'www', {'ttl': 42, 'value': '1.2.3.4'})
+        self.assertFalse(', line' in str(ctx.exception))
+
+        # fails validation, with context
+        with self.assertRaises(Exception) as ctx:
+            Record.new(
+                self.zone,
+                'www',
+                ContextDict({'ttl': 42, 'value': '1.2.3.4'}, context='needle'),
+            )
+        self.assertTrue('needle' in str(ctx.exception))