From bf1896329ba1c8f5829895ecfe71e233ca0173cc Mon Sep 17 00:00:00 2001 From: Adam Smith Date: Sun, 22 Oct 2017 22:39:18 -0700 Subject: [PATCH] validate values for empty string or None value dump does not write invalid value(s) to yaml --- octodns/record.py | 42 +++++++- tests/test_octodns_provider_dnsimple.py | 8 +- tests/test_octodns_record.py | 125 +++++++++++++++++++++++- 3 files changed, 165 insertions(+), 10 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index 2f67c60..f9812a6 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -211,9 +211,30 @@ class _ValuesMixin(object): values = [] try: values = data['values'] + if not values: + values = [] + reasons.append('missing value(s)') + else: + # loop through copy of values + # remove invalid value from values + for value in list(values): + if value is None: + reasons.append('missing value(s)') + values.remove(value) + elif len(value) == 0: + reasons.append('empty value') + values.remove(value) except KeyError: try: - values = [data['value']] + value = data['value'] + if value is None: + reasons.append('missing value(s)') + values = [] + elif len(value) == 0: + reasons.append('empty value') + values = [] + else: + values = [value] except KeyError: reasons.append('missing value(s)') @@ -238,10 +259,16 @@ class _ValuesMixin(object): def _data(self): ret = super(_ValuesMixin, self)._data() if len(self.values) > 1: - ret['values'] = [getattr(v, 'data', v) for v in self.values] - else: + values = [getattr(v, 'data', v) for v in self.values if v] + if len(values) > 1: + ret['values'] = values + elif len(values) == 1: + ret['value'] = values[0] + elif len(self.values) == 1: v = self.values[0] - ret['value'] = getattr(v, 'data', v) + if v: + ret['value'] = getattr(v, 'data', v) + return ret def __repr__(self): @@ -349,6 +376,10 @@ class _ValueMixin(object): value = None try: value = data['value'] + if value is None: + reasons.append('missing value') + elif value == '': + reasons.append('empty value') except KeyError: reasons.append('missing value') if value: @@ -366,7 +397,8 @@ class _ValueMixin(object): def _data(self): ret = super(_ValueMixin, self)._data() - ret['value'] = getattr(self.value, 'data', self.value) + if self.value: + ret['value'] = getattr(self.value, 'data', self.value) return ret def __repr__(self): diff --git a/tests/test_octodns_provider_dnsimple.py b/tests/test_octodns_provider_dnsimple.py index 950d460..b44d6ee 100644 --- a/tests/test_octodns_provider_dnsimple.py +++ b/tests/test_octodns_provider_dnsimple.py @@ -96,23 +96,23 @@ class TestDnsimpleProvider(TestCase): mock.get(ANY, text=fh.read()) zone = Zone('unit.tests.', []) - provider.populate(zone) + provider.populate(zone, lenient=True) self.assertEquals(set([ Record.new(zone, '', { 'ttl': 3600, 'type': 'SSHFP', 'values': [] - }), + }, lenient=True), Record.new(zone, '_srv._tcp', { 'ttl': 600, 'type': 'SRV', 'values': [] - }), + }, lenient=True), Record.new(zone, 'naptr', { 'ttl': 600, 'type': 'NAPTR', 'values': [] - }), + }, lenient=True), ]), zone.records) def test_apply(self): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index e7eaa61..ff57975 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -96,6 +96,57 @@ class TestRecord(TestCase): DummyRecord().__repr__() + def test_values_mixin_data(self): + # no values, no value or values in data + a = ARecord(self.zone, '', { + 'type': 'A', + 'ttl': 600, + 'values': [] + }) + self.assertNotIn('values', a.data) + + # empty value, no value or values in data + b = ARecord(self.zone, '', { + 'type': 'A', + 'ttl': 600, + 'values': [''] + }) + self.assertNotIn('value', b.data) + + # empty/None values, no value or values in data + c = ARecord(self.zone, '', { + 'type': 'A', + 'ttl': 600, + 'values': ['', None] + }) + self.assertNotIn('values', c.data) + + # empty/None values and valid, value in data + c = ARecord(self.zone, '', { + 'type': 'A', + 'ttl': 600, + 'values': ['', None, '10.10.10.10'] + }) + self.assertNotIn('values', c.data) + self.assertEqual('10.10.10.10', c.data['value']) + + def test_value_mixin_data(self): + # unspecified value, no value in data + a = AliasRecord(self.zone, '', { + 'type': 'ALIAS', + 'ttl': 600, + 'value': None + }) + self.assertNotIn('value', a.data) + + # unspecified value, no value in data + a = AliasRecord(self.zone, '', { + 'type': 'ALIAS', + 'ttl': 600, + 'value': '' + }) + self.assertNotIn('value', a.data) + def test_geo(self): geo_data = {'ttl': 42, 'values': ['5.2.3.4', '6.2.3.4'], 'geo': {'AF': ['1.1.1.1'], @@ -750,6 +801,13 @@ class TestRecordValidation(TestCase): 'ttl': 600, 'value': '1.2.3.4', }) + Record.new(self.zone, '', { + 'type': 'A', + 'ttl': 600, + 'values': [ + '1.2.3.4', + ] + }) Record.new(self.zone, '', { 'type': 'A', 'ttl': 600, @@ -759,13 +817,60 @@ class TestRecordValidation(TestCase): ] }) - # missing value(s) + # missing value(s), no value or value with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, '', { 'type': 'A', 'ttl': 600, }) self.assertEquals(['missing value(s)'], ctx.exception.reasons) + + # missing value(s), empty values + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': 600, + 'values': [] + }) + self.assertEquals(['missing value(s)'], ctx.exception.reasons) + + # missing value(s), None values + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': 600, + 'values': None + }) + self.assertEquals(['missing value(s)'], ctx.exception.reasons) + + # missing value(s) and empty value + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': 600, + 'values': [None, ''] + }) + self.assertEquals(['missing value(s)', + 'empty value'], ctx.exception.reasons) + + # missing value(s), None value + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': 600, + 'value': None + }) + self.assertEquals(['missing value(s)'], ctx.exception.reasons) + + # empty value, empty string value + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': 600, + 'value': '' + }) + self.assertEquals(['empty value'], ctx.exception.reasons) + # missing value(s) & ttl with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, '', { @@ -922,6 +1027,24 @@ class TestRecordValidation(TestCase): }) self.assertEquals(['missing value'], ctx.exception.reasons) + # missing value + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'ALIAS', + 'ttl': 600, + 'value': None + }) + self.assertEquals(['missing value'], ctx.exception.reasons) + + # empty value + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'ALIAS', + 'ttl': 600, + 'value': '' + }) + self.assertEquals(['empty value'], ctx.exception.reasons) + # missing trailing . with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, '', {