fields: use polymorphism for effective substructure determination

This code is functionally equivalent to the previous version for
ConditionalField, but we now properly consider other fields that
could wrap SubstructureField to be SubstructureField to be such
(e.g. LengthField, TypeField).

This change also adds **kwargs to unpack() on all fields for consistency
and expansion as needed in the future.

Signed-off-by: Paul Osborne <Paul.Osborne@digi.com>

suitcase/fields.py

@@ -82,6 +82,10 @@ def _ph2f(self, placeholder):
     def __repr__(self):
         return repr(self.getval())
 
+    def is_substructure(self):
+        """Return True if this field is (effectively) a SubstructureField"""
+        return False
+
     def getval(self):
         return self._value
 
@@ -175,8 +179,8 @@ def pack(self, stream):
         # write placeholder during the first pass
         stream.write(b'\x00' * self.field.bytes_required)
 
-    def unpack(self, data):
-        self.field.unpack(data)
+    def unpack(self, data, **kwargs):
+        self.field.unpack(data, **kwargs)
 
 
 class Magic(BaseField):
@@ -196,7 +200,7 @@ def setval(self, *args):
     def pack(self, stream):
         stream.write(self.expected_sequence)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         if not data == self.expected_sequence:
             raise SuitcaseParseError(
                 "Expected sequence %r for magic field but got %r on "
@@ -270,7 +274,7 @@ def setval(self, value):
 
         self.field.setval(onset(value))
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         pass
 
     def pack(self, stream):
@@ -317,7 +321,7 @@ def __repr__(self):
     def pack(self, stream):
         return self.field.pack(stream)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         assert len(data) == self.bytes_required
         return self.field.unpack(data)
 
@@ -392,7 +396,7 @@ def setval(self, value):
     def pack(self, stream):
         return self._value._packer.write(stream)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         target_msg_type = self._lookup_msg_type()
         if target_msg_type is None:
             target_msg_type = self.dispatch_mapping.get(None)
@@ -446,6 +450,9 @@ def _default_get_length(self, field):
     def _default_set_length(self, field, length):
         field._value = length  # TODO: use setval() [problem with DependentField tests]?
 
+    def is_substructure(self):
+        return self.length_field.is_substructure()
+
     @property
     def bytes_required(self):
         return self.length_field.bytes_required
@@ -474,9 +481,9 @@ def pack(self, stream):
         self.set_length(self.length_field, self.length_value_provider())
         self.length_field.pack(stream)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         assert len(data) == self.bytes_required
-        return self.length_field.unpack(data)
+        return self.length_field.unpack(data, **kwargs)
 
     def get_adjusted_length(self):
         return self.get_length(self.length_field) * self.multiplier
@@ -532,6 +539,9 @@ def _lookup_msg_length(self):
     def __repr__(self):
         return repr(self.type_field)
 
+    def is_substructure(self):
+        return self.type_field.is_substructure()
+
     @property
     def bytes_required(self):
         return self.type_field.bytes_required
@@ -563,9 +573,9 @@ def pack(self, stream):
 
         self.type_field.pack(stream)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         assert len(data) == self.bytes_required
-        return self.type_field.unpack(data)
+        return self.type_field.unpack(data, **kwargs)
 
     def get_adjusted_length(self):
         return self._lookup_msg_length()
@@ -601,6 +611,9 @@ def __repr__(self):
         else:
             return "<ConditionalField: not included>"
 
+    def is_substructure(self):
+        return self.field.is_substructure()
+
     @property
     def bytes_required(self):
         if self.condition(self._parent):
@@ -666,7 +679,7 @@ def bytes_required(self):
     def pack(self, stream):
         stream.write(self._value)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         self._value = data
 
 
@@ -693,7 +706,7 @@ def pack(self, stream):
         except struct.error as e:
             raise SuitcasePackStructException(e)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         assert len(data) == self.bytes_required
 
         length = self.bytes_required
@@ -752,7 +765,7 @@ def __getattr__(self, attr):
     def pack(self, stream):
         pass
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         pass
 
     def getval(self):
@@ -803,6 +816,9 @@ def __init__(self, substructure, **kwargs):
         self.substructure = substructure
         self._value = substructure()
 
+    def is_substructure(self):
+        return True
+
     @property
     def bytes_required(self):
         # We return None but do not count as a greedy field to the packer
@@ -811,9 +827,9 @@ def bytes_required(self):
     def pack(self, stream):
         stream.write(self._value.pack())
 
-    def unpack(self, data, trailing):
+    def unpack(self, data, **kwargs):
         self._value = self.substructure()
-        return self._value.unpack(data, trailing)
+        return self._value.unpack(data, **kwargs)
 
 
 class FieldArray(BaseField):
@@ -868,10 +884,11 @@ def pack(self, stream):
         for structure in self._value:
             stream.write(structure.pack())
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
+        kwargs['trailing'] = True
         while True:
             structure = self.substructure()
-            data = structure.unpack(data, trailing=True).read()
+            data = structure.unpack(data, **kwargs).read()
             self._value.append(structure)
             if data == b"":
                 break
@@ -891,7 +908,7 @@ def pack(self, stream):
         except struct.error as e:
             raise SuitcasePackStructException(e)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         try:
             self._value = struct.unpack(self.format, data)
         except struct.error as e:
@@ -950,7 +967,7 @@ def pack(self, stream):
             raise SuitcasePackStructException(e)
         stream.write(to_write)
 
-    def unpack(self, data):
+    def unpack(self, data, **kwargs):
         value = 0
         if self.UNPACK_FORMAT[0] == b">"[0]:  # The element access makes this compatible with Python 2 and 3
             for i, byte in enumerate(reversed(struct.unpack(self.UNPACK_FORMAT, data))):
@@ -1366,8 +1383,8 @@ def pack(self, stream):
         out = sio.getvalue()[-self.number_bytes:]
         stream.write(out)
 
-    def unpack(self, data):
-        self._field.unpack(data)
+    def unpack(self, data, **kwargs):
+        self._field.unpack(data, **kwargs)
         value = self._field.getval()
         shift = self.number_bits
         for _key, field in self._ordered_bitfields:

suitcase/structure.py

@@ -86,11 +86,6 @@ def unpack_stream(self, stream):
         logic present for dealing with checksum fields.
 
         """
-        def is_substructure(field):
-            return isinstance(field, SubstructureField) or \
-                    isinstance(field, ConditionalField) and \
-                    isinstance(field.field, SubstructureField)
-
         crc_fields = []
         greedy_field = None
         # go through the fields from first to last.  If we hit a greedy
@@ -99,7 +94,7 @@ def is_substructure(field):
             if isinstance(field, CRCField):
                 crc_fields.append((field, stream.tell()))
             length = field.bytes_required
-            if is_substructure(field):
+            if field.is_substructure():
                 remaining_data = stream.getvalue()[stream.tell():]
                 returned_stream = field.unpack(remaining_data, trailing=True)
                 if returned_stream is not None: