From 1776d558b5b361602b8ecb152ded1fa2a3cc238d Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 28 Jul 2023 17:14:01 -0700 Subject: [PATCH] POC of config/validation errors with context. Implemented by YAML for all it's cases --- octodns/manager.py | 54 ++++++++++++++++++++++------------- octodns/record/base.py | 17 ++++++++--- octodns/record/exception.py | 13 ++++++--- octodns/yaml.py | 38 ++++++++++++++++++++++-- tests/test_octodns_manager.py | 6 ++-- 5 files changed, 94 insertions(+), 34 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index b8fb0ff..a0a8b91 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -200,9 +200,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) @@ -215,7 +217,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 @@ -228,9 +230,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) @@ -243,22 +247,24 @@ 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', None) 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' - ) + msg = f'plan_output {plan_output_name} is missing class' + if context: + msg += f', {context}' + raise ManagerException(msg) _class, module, version = self._get_named_class( - 'plan_output', _class + 'plan_output', _class, context ) kwargs = self._build_kwargs(plan_output_config) try: @@ -275,9 +281,11 @@ class Manager(object): ) except TypeError: self.log.exception('Invalid plan_output config') - raise ManagerException( - 'Incorrect plan_output config for ' + plan_output_name - ) + msg = f'Incorrect plan_output config for {plan_output_name}' + if context: + msg += f', {plan_output_config.context}' + raise ManagerException(msg) + return plan_outputs def _try_version(self, module_name, module=None, version=None): @@ -308,7 +316,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) @@ -316,7 +324,10 @@ class Manager(object): self.log.exception( '_get_{}_class: Unable to import module %s', _class ) - raise ManagerException(f'Unknown {_type} class: {_class}') + msg = f'Unknown {_type} class: {_class}' + if context: + msg += f', {context}' + raise ManagerException(msg) try: return getattr(module, class_name), module_name, version @@ -326,11 +337,14 @@ 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 kwargs = {} + context = getattr(source, 'context', None) for k, v in source.items(): try: if v.startswith('env/'): @@ -339,10 +353,10 @@ class Manager(object): v = environ[env_var] except KeyError: self.log.exception('Invalid provider config') - raise ManagerException( - 'Incorrect provider config, ' - 'missing env var ' + env_var - ) + msg = f'Incorrect provider config, missing env var {env_var}' + if context: + msg += f', {context}' + raise ManagerException(msg) except AttributeError: pass kwargs[k] = v diff --git a/octodns/record/base.py b/octodns/record/base.py index 23b9a5c..ffea1b6 100644 --- a/octodns/record/base.py +++ b/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 diff --git a/octodns/record/exception.py b/octodns/record/exception.py index 34aea0b..105a43d 100644 --- a/octodns/record/exception.py +++ b/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 diff --git a/octodns/yaml.py b/octodns/yaml.py index 3021fba..714bb1a 100644 --- a/octodns/yaml.py +++ b/octodns/yaml.py @@ -10,12 +10,43 @@ from yaml.representer import SafeRepresenter _natsort_key = natsort_keygen() +# TODO: where should this live +class ContextDict(dict): + # can't assign attributes to plain dict objects and it breaks lots of stuff + # if we put the context into the dict data itself + + def __init__(self, *args, context=None, **kwargs): + super().__init__(*args, **kwargs) + self.context = context + + +class ContextLoader(SafeLoader): + def _construct(self, node): + self.flatten_mapping(node) + ret = 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(ret, context=context) + + +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): + # TODO: inheritance + def _construct(self, node): self.flatten_mapping(node) ret = self.construct_pairs(node) + + start_mark = node.start_mark + context = f'{start_mark.name}, line {start_mark.line+1}, column {start_mark.column+1}' + keys = [d[0] for d in ret] keys_sorted = sorted(keys, key=_natsort_key) for key in keys: @@ -25,9 +56,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 ContextDict(ret, context=context) SortEnforcingLoader.add_constructor( @@ -36,7 +68,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): diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 62de2c4..7d100dc 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -104,13 +104,13 @@ 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):