From 6f6cb798541ce47051d4747162e01c3d290b42e8 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 16:27:02 -0800 Subject: [PATCH 1/5] Fix bug with Record.copy when values is an empty list [] --- CHANGELOG.md | 1 + octodns/record/base.py | 5 ++++- tests/test_octodns_record.py | 18 ++++++++++++++---- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc3fef1..8e25a7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## v1.?.0 - 2023-??-?? - * Record.lenient property added similar to other common/standard _octodns data +* Fix bug with Record.copy when values is an empty list [] ## v1.3.0 - 2023-11-14 - New and improved processors diff --git a/octodns/record/base.py b/octodns/record/base.py index 6f83578..d203cea 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -296,7 +296,10 @@ class ValuesMixin(object): try: values = data['values'] except KeyError: - values = [data['value']] + try: + values = [data['value']] + except KeyError: + values = [] self.values = sorted(self._value_type.process(values)) def changes(self, other, target): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 66f0b9f..0fe3d57 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -338,6 +338,17 @@ class TestRecord(TestCase): del b._octodns['key']['second'] self.assertNotEqual(a.data, b.data) + def test_record_copy_with_no_values(self): + txt = Record.new( + self.zone, + 'txt', + {'ttl': 45, 'type': 'TXT', 'values': []}, + lenient=True, + ) + + dup = txt.copy() + self.assertEqual(txt.values, dup.values) + def test_change(self): existing = Record.new( self.zone, 'txt', {'ttl': 44, 'type': 'TXT', 'value': 'some text'} @@ -631,10 +642,9 @@ class TestRecordValidation(TestCase): lenient=True, ) - # __init__ may still blow up, even if validation is lenient - with self.assertRaises(KeyError) as ctx: - Record.new(self.zone, 'www', {'type': 'A', 'ttl': -1}, lenient=True) - self.assertEqual(('value',), ctx.exception.args) + # empty values is allowed with lenient + r = Record.new(self.zone, 'www', {'type': 'A', 'ttl': -1}, lenient=True) + self.assertEqual([], r.values) # no exception if we're in lenient mode from config Record.new( From 1a5c9a2c5e612c86c246724cb88ec51de54a13f1 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 29 Nov 2023 14:03:22 -0800 Subject: [PATCH 2/5] Flip ValuesMixin._values logic to handle/have key in all cases --- octodns/record/base.py | 14 +++++++------- tests/test_octodns_record.py | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/octodns/record/base.py b/octodns/record/base.py index d203cea..5d5d0af 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -309,16 +309,16 @@ class ValuesMixin(object): def _data(self): ret = super()._data() - if len(self.values) > 1: - 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: + if len(self.values) == 1: v = self.values[0] if v: ret['value'] = getattr(v, 'data', v) + else: + values = [getattr(v, 'data', v) for v in self.values if v] + if len(values) == 1: + ret['value'] = values[0] + else: + ret['values'] = values return ret diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 0fe3d57..4d52b71 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -197,19 +197,19 @@ class TestRecord(TestCase): ) def test_values_mixin_data(self): - # no values, no value or values in data + # empty values -> empty values in data a = ARecord(self.zone, '', {'type': 'A', 'ttl': 600, 'values': []}) - self.assertNotIn('values', a.data) + self.assertEqual([], a.data['values']) # 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 + # empty/None values -> empty values in data c = ARecord( self.zone, '', {'type': 'A', 'ttl': 600, 'values': ['', None]} ) - self.assertNotIn('values', c.data) + self.assertEqual([], a.data['values']) # empty/None values and valid, value in data c = ARecord( From cb09b590e7dfab789d781a38207008d9a6efec3a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 29 Nov 2023 14:21:14 -0800 Subject: [PATCH 3/5] ValueMixin._data always includes 'value' --- octodns/record/base.py | 3 +-- tests/test_octodns_record.py | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/octodns/record/base.py b/octodns/record/base.py index 5d5d0af..443e7ba 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -374,8 +374,7 @@ class ValueMixin(object): def _data(self): ret = super()._data() - if self.value: - ret['value'] = getattr(self.value, 'data', self.value) + ret['value'] = getattr(self.value, 'data', self.value) return ret @property diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 4d52b71..86f92e5 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -225,13 +225,13 @@ class TestRecord(TestCase): a = AliasRecord( self.zone, '', {'type': 'ALIAS', 'ttl': 600, 'value': None} ) - self.assertNotIn('value', a.data) + self.assertIsNone(a.data['value']) # unspecified value, no value in data a = AliasRecord( self.zone, '', {'type': 'ALIAS', 'ttl': 600, 'value': ''} ) - self.assertNotIn('value', a.data) + self.assertIsNone(a.data['value']) def test_record_new(self): txt = Record.new( @@ -349,6 +349,16 @@ class TestRecord(TestCase): dup = txt.copy() self.assertEqual(txt.values, dup.values) + cname = Record.new( + self.zone, + 'cname', + {'ttl': 45, 'type': 'CNAME', 'value': ''}, + lenient=True, + ) + + dup = cname.copy() + self.assertEqual(cname.value, dup.value) + def test_change(self): existing = Record.new( self.zone, 'txt', {'ttl': 44, 'type': 'TXT', 'value': 'some text'} From 0d1fcb4a3973372d64fcedc092edbf40ef1f71bd Mon Sep 17 00:00:00 2001 From: "Janik H." Date: Thu, 30 Nov 2023 20:33:03 +0100 Subject: [PATCH 4/5] Update README.md --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4c5cac0..f69f17c 100644 --- a/README.md +++ b/README.md @@ -313,8 +313,8 @@ Similar to providers, but can only serve to populate records into a zone, cannot | Source | Record Support | Dynamic | Notes | |--|--|--|--| | [EnvVarSource](/octodns/source/envvar.py) | TXT | No | read-only environment variable injection | -| [AxfrSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, LOC, MX, NS, PTR, SPF, SRV, TXT | No | read-only | -| [ZoneFileSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, MX, NS, PTR, SPF, SRV, TXT | No | read-only | +| [AxfrSource](https://github.com/octodns/octodns-bind/) | A, AAAA, CAA, CNAME, LOC, MX, NS, PTR, SPF, SRV, TXT | No | read-only | +| [ZoneFileSource](https://github.com/octodns/octodns-bind/) | A, AAAA, CAA, CNAME, MX, NS, PTR, SPF, SRV, TXT | No | read-only | | [TinyDnsFileSource](/octodns/source/tinydns.py) | A, CNAME, MX, NS, PTR | No | read-only | ### Notes From 523a188e1decc9a60a114c6d85fad4f3d62e9dfd Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 1 Dec 2023 10:46:58 -0800 Subject: [PATCH 5/5] explicit test for values and value --- tests/test_octodns_record.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 86f92e5..c85341f 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -251,6 +251,20 @@ class TestRecord(TestCase): Record.new(self.zone, 'unknown', {'type': 'XXX'}) self.assertTrue('Unknown record type' in str(ctx.exception)) + def test_record_new_with_values_and_value(self): + a = Record.new( + self.zone, + 'a', + { + 'ttl': 44, + 'type': 'A', + 'value': '1.2.3.4', + 'values': ['2.3.4.5', '3.4.5.6'], + }, + ) + # values is preferred over value when both exist + self.assertEqual(['2.3.4.5', '3.4.5.6'], a.values) + def test_record_copy(self): a = Record.new( self.zone, 'a', {'ttl': 44, 'type': 'A', 'value': '1.2.3.4'}