Merge pull request #1113 from octodns/record-data-copy

ValuesMixin._data improvements & related fixes of Record.copy

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
 

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):
@@ -306,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
 
@@ -371,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

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(
@@ -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(
@@ -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'}
@@ -338,6 +352,27 @@ 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)
+
+        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'}
@@ -631,10 +666,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(