Refactor metadata constructors and add factory

This commit better separates the Metadata class model from the
Metadata wireline format, by tailoring the constructors
towards class-based parameters and adding an additional
factory classmethod that creates Metadata objects based on the
wireline json/dictionary metadata representation. (pythonic
way of constructor overloading).

This 'from_dict' factory method recurses into the 'from_dict'
methods of each contained complex field/attribute that is also
represented by a class. Currently 'signed' is the only such
attribute.

This commit further:
- Changes optional constructor keyword arguments to mandatory
positional arguments: Reduces code and simplifies usage by
restricting it. For now, users are unlikely to call
constructor directly anyway, but the 'from_dict' factory (or
its 'from_json_file' wrapper) instead.

- Removes Signed.__expiration (datetime) vs. Signed.expires
(datestring) dichotomy: Keeping only one representation of the
same attribute in memory makes the interface simpler and less
ambiguous. We choose the datetime object, because it is more
convenient to modify. Transformation from and to the string
format required by the tuf wireline format is performed in the
corresponding metadata de/serialization methods, i.e.
('to_dict' and 'from_dict').

Signed-off-by: Lukas Puehringer <[email protected]>

tests/test_api.py

@@ -16,7 +16,7 @@
 import tempfile
 import unittest
 
-from datetime import timedelta
+from datetime import datetime, timedelta
 from dateutil.relativedelta import relativedelta
 
 IS_PY_VERSION_SUPPORTED = sys.version_info >= (3, 6)
@@ -186,11 +186,11 @@ def test_metadata_base(self):
         self.assertEqual(md.signed.version, 1)
         md.signed.bump_version()
         self.assertEqual(md.signed.version, 2)
-        self.assertEqual(md.signed.expires, '2030-01-01T00:00:00Z')
+        self.assertEqual(md.signed.expires, datetime(2030, 1, 1, 0, 0))
         md.signed.bump_expiration()
-        self.assertEqual(md.signed.expires, '2030-01-02T00:00:00Z')
+        self.assertEqual(md.signed.expires, datetime(2030, 1, 2, 0, 0))
         md.signed.bump_expiration(timedelta(days=365))
-        self.assertEqual(md.signed.expires, '2031-01-02T00:00:00Z')
+        self.assertEqual(md.signed.expires, datetime(2031, 1, 2, 0, 0))
 
 
     def test_metadata_snapshot(self):
@@ -218,20 +218,20 @@ def test_metadata_timestamp(self):
         timestamp.signed.bump_version()
         self.assertEqual(timestamp.signed.version, 2)
 
-        self.assertEqual(timestamp.signed.expires, '2030-01-01T00:00:00Z')
+        self.assertEqual(timestamp.signed.expires, datetime(2030, 1, 1, 0, 0))
         timestamp.signed.bump_expiration()
-        self.assertEqual(timestamp.signed.expires, '2030-01-02T00:00:00Z')
+        self.assertEqual(timestamp.signed.expires, datetime(2030, 1, 2, 0, 0))
         timestamp.signed.bump_expiration(timedelta(days=365))
-        self.assertEqual(timestamp.signed.expires, '2031-01-02T00:00:00Z')
+        self.assertEqual(timestamp.signed.expires, datetime(2031, 1, 2, 0, 0))
 
         # Test whether dateutil.relativedelta works, this provides a much
         # easier to use interface for callers
         delta = relativedelta(days=1)
         timestamp.signed.bump_expiration(delta)
-        self.assertEqual(timestamp.signed.expires, '2031-01-03T00:00:00Z')
+        self.assertEqual(timestamp.signed.expires, datetime(2031, 1, 3, 0, 0))
         delta = relativedelta(years=5)
         timestamp.signed.bump_expiration(delta)
-        self.assertEqual(timestamp.signed.expires, '2036-01-03T00:00:00Z')
+        self.assertEqual(timestamp.signed.expires, datetime(2036, 1, 3, 0, 0))
 
         hashes = {'sha256': '0ae9664468150a9aa1e7f11feecb32341658eb84292851367fea2da88e8a58dc'}
         fileinfo = timestamp.signed.meta['snapshot.json']

tuf/api/metadata.py

@@ -69,9 +69,7 @@ class Metadata():
             ]
 
     """
-    def __init__(
-            self, signed: 'Signed' = None, signatures: list = None) -> None:
-        # TODO: How much init magic do we want?
+    def __init__(self, signed: 'Signed', signatures: list) -> None:
         self.signed = signed
         self.signatures = signatures
 
@@ -167,18 +165,37 @@ def from_json_file(
 
         Raises:
             securesystemslib.exceptions.StorageError: The file cannot be read.
-            securesystemslib.exceptions.Error, ValueError: The metadata cannot
-                be parsed.
+            securesystemslib.exceptions.Error, ValueError, KeyError: The
+                metadata cannot be parsed.
 
         Returns:
             A TUF Metadata object.
 
         """
-        signable = load_json_file(filename, storage_backend)
+        return cls.from_dict(load_json_file(filename, storage_backend))
+
+    @classmethod
+    def from_dict(cls, metadata: JsonDict) -> 'Metadata':
+        """Creates Metadata object from its JSON/dict representation.
+
+        Calls 'from_dict' for any complex metadata attribute represented by a
+        class also that has a 'from_dict' factory method. (Currently this is
+        only the signed attribute.)
+
+        Arguments:
+            metadata: TUF metadata in JSON/dict representation, as e.g.
+            returned by 'json.loads'.
+
+        Raises:
+            KeyError: The metadata dict format is invalid.
+            ValueError: The metadata has an unrecognized signed._type field.
 
-        # TODO: Should we use constants?
-        # And/or maybe a dispatch table? (<-- maybe too much magic)
-        _type = signable['signed']['_type']
+        Returns:
+            A TUF Metadata object.
+
+        """
+        # Dispatch to contained metadata class on metadata _type field.
+        _type = metadata['signed']['_type']
 
         if _type == 'targets':
             inner_cls = Targets
@@ -192,9 +209,13 @@ def from_json_file(
         else:
             raise ValueError(f'unrecognized metadata type "{_type}"')
 
-        return Metadata(
-                signed=inner_cls(**signable['signed']),
-                signatures=signable['signatures'])
+        # NOTE: If Signature becomes a class, we should iterate over
+        # metadata['signatures'], call Signature.from_dict for each item, and
+        # pass a list of Signature objects to the Metadata constructor intead.
+        return cls(
+                signed=inner_cls.from_dict(metadata['signed']),
+                signatures=metadata['signatures'])
+
 
     def to_json_file(
             self, filename: str, compact: bool = False,
@@ -236,41 +257,48 @@ class Signed:
     # we keep it to match spec terminology (I often refer to this as "payload",
     # or "inner metadata")
 
-    # TODO: Re-think default values. It might be better to pass some things
-    # as args and not es kwargs. Then we'd need to pop those from
-    # signable["signed"] in read_from_json and pass them explicitly, which
-    # some say is better than implicit. :)
     def __init__(
-            self, _type: str = None, version: int = 0,
-            spec_version: str = None, expires: datetime = None
-        ) -> None:
-        # TODO: How much init magic do we want?
+            self, _type: str, version: int, spec_version: str,
+            expires: datetime) -> None:
 
         self._type = _type
+        self.version = version
         self.spec_version = spec_version
+        self.expires = expires
 
-        # We always intend times to be UTC
-        # NOTE: we could do this with datetime.fromisoformat() but that is not
-        # available in Python 2.7's datetime
-        # NOTE: Store as datetime object for convenient handling, use 'expires'
-        # property to get the TUF metadata format representation
-        self.__expiration = iso8601.parse_date(expires).replace(tzinfo=None)
-
+        # TODO: Should we separate data validation from constructor?
         if version < 0:
             raise ValueError(f'version must be < 0, got {version}')
         self.version = version
 
+    @classmethod
+    def from_dict(cls, signed_dict) -> 'Signed':
+        """Creates Signed object from its JSON/dict representation. """
+
+        # Convert 'expires' TUF metadata string to a datetime object, which is
+        # what the constructor expects and what we store. The inverse operation
+        # is implemented in 'to_dict'.
+        signed_dict['expires'] = iso8601.parse_date(
+                signed_dict['expires']).replace(tzinfo=None)
+        # NOTE: We write the converted 'expires' back into 'signed_dict' above
+        # so that we can pass it to the constructor as  '**signed_dict' below,
+        # along with other fields that belong to Signed subclasses.
+        # Any 'from_dict'(-like) conversions of fields that correspond to a
+        # subclass should be performed in the 'from_dict' method of that
+        # subclass and also be written back into 'signed_dict' before calling
+        # super().from_dict.
+
+        # NOTE: cls might be a subclass of Signed, if 'from_dict' was called on
+        # that subclass (see e.g. Metadata.from_dict).
+        return cls(**signed_dict)
 
-    @property
-    def expires(self) -> str:
-        return self.__expiration.isoformat() + 'Z'
     def to_canonical_bytes(self) -> bytes:
         """Returns the UTF-8 encoded canonical JSON representation of self. """
         return encode_canonical(self.to_dict()).encode('UTF-8')
 
     def bump_expiration(self, delta: timedelta = timedelta(days=1)) -> None:
         """Increments the expires attribute by the passed timedelta. """
-        self.__expiration = self.__expiration + delta
+        self.expires += delta
 
     def bump_version(self) -> None:
         """Increments the metadata version number by 1."""
@@ -282,7 +310,7 @@ def to_dict(self) -> JsonDict:
             '_type': self._type,
             'version': self.version,
             'spec_version': self.spec_version,
-            'expires': self.expires
+            'expires': self.expires.isoformat() + 'Z'
         }
 
 class Timestamp(Signed):
@@ -304,10 +332,11 @@ class Timestamp(Signed):
             }
 
     """
-    def __init__(self, meta: JsonDict = None, **kwargs) -> None:
-        super().__init__(**kwargs)
-        # TODO: How much init magic do we want?
-        # TODO: Is there merit in creating classes for dict fields?
+    def __init__(
+            self, _type: str, version: int, spec_version: str,
+            expires: datetime, meta: JsonDict) -> None:
+        super().__init__(_type, version, spec_version, expires)
+        # TODO: Add class for meta
         self.meta = meta
 
     def to_dict(self) -> JsonDict:
@@ -353,10 +382,11 @@ class Snapshot(Signed):
             }
 
     """
-    def __init__(self, meta: JsonDict = None, **kwargs) -> None:
-        # TODO: How much init magic do we want?
-        # TODO: Is there merit in creating classes for dict fields?
-        super().__init__(**kwargs)
+    def __init__(
+            self, _type: str, version: int, spec_version: str,
+            expires: datetime, meta: JsonDict) -> None:
+        super().__init__(_type, version, spec_version, expires)
+        # TODO: Add class for meta
         self.meta = meta
 
     def to_dict(self) -> JsonDict:
@@ -436,14 +466,15 @@ class Targets(Signed):
 
     """
     def __init__(
-            self, targets: JsonDict = None, delegations: JsonDict = None,
-            **kwargs) -> None:
-        # TODO: How much init magic do we want?
-        # TODO: Is there merit in creating classes for dict fields?
-        super().__init__(**kwargs)
+            self, _type: str, version: int, spec_version: str,
+            expires: datetime, targets: JsonDict, delegations: JsonDict
+            ) -> None:
+        super().__init__(_type, version, spec_version, expires)
+        # TODO: Add class for meta
         self.targets = targets
         self.delegations = delegations
 
+
     def to_dict(self) -> JsonDict:
         """Returns the JSON-serializable dictionary representation of self. """
         json_dict = super().to_dict()