Refactor models to satisfy desired invariants

CHANGELOG.md

@@ -7,7 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-- n/a
+### Changed
+- **API break**: types of fields on model objects are now strictly validated
+  during construction.
+- **API break**: objects documented as immutable are now more deeply immutable;
+  it is no longer possible to mutate list fields on these objects.
+- **API break**: fixed inconsistencies on collection model fields. All fields
+  previously declared as tuples have been updated to use (immutable) lists.
+
+
+### Fixed
+- **API break**: `MaintenanceReport.last_updated`, `MaintenanceEntry.started`
+  are now `datetime` objects as documented. In previous versions, these were
+  documented as datetimes but implemented as `str`.
 
 ## [1.5.0] - 2019-09-03
 

pubtools/pulplib/_impl/compat_attr.py

@@ -47,3 +47,4 @@ def fields_dict(cls):
 fields = attr.fields
 evolve = attr.evolve
 has = attr.has
+validators = attr.validators

pubtools/pulplib/_impl/fake/client.py

@@ -189,7 +189,7 @@ def _do_unassociate(self, repo_id, type_ids):
             repo_id=repo_id,
             completed=True,
             succeeded=True,
-            units=tuple(removed),
+            units=removed,
         )
 
         return f_return([task])

pubtools/pulplib/_impl/model/attr.py

@@ -1,3 +1,5 @@
+import six
+
 from pubtools.pulplib._impl import compat_attr as attr
 
 
@@ -17,10 +19,14 @@
 PY_PULP2_CONVERTER = "_pubtools.pulplib.py_to_pulp2_converter"
 
 
-# make usage of the above less ugly
 def pulp_attrib(
     pulp_field=None, pulp_py_converter=None, py_pulp_converter=None, **kwargs
 ):
+    """Drop-in replacement for attr.ib with added features:
+
+    - applies a validator based on type automatically
+    - supports pulplib-specific metadata via extra keyword arguments
+    """
     metadata = kwargs.get("metadata") or {}
 
     if pulp_field:
@@ -32,5 +38,19 @@ def pulp_attrib(
     if py_pulp_converter:
         metadata[PY_PULP2_CONVERTER] = py_pulp_converter
 
+    if "type" in kwargs:
+        # As a convenience, you may define string types as type=str
+        # on any python version, but what you'll actually get is
+        # whatever's the primary string type (e.g. basestr on py2)
+        if kwargs["type"] is str:
+            kwargs["type"] = six.string_types[0]
+
+        # If you haven't defined a validator, you get one automatically
+        # for your requested type
+        if "validator" not in kwargs:
+            kwargs["validator"] = attr.validators.optional(
+                attr.validators.instance_of(kwargs["type"])
+            )
+
     kwargs["metadata"] = metadata
     return attr.ib(**kwargs)

pubtools/pulplib/_impl/model/convert.py

@@ -29,3 +29,7 @@ def null_convert(value):
 
 def read_timestamp(value):
     return datetime.datetime.strptime(value, "%Y-%m-%dT%H:%M:%SZ")
+
+
+def write_timestamp(value):
+    return value.strftime("%Y-%m-%dT%H:%M:%SZ")

pubtools/pulplib/_impl/model/frozenlist.py

@@ -0,0 +1,35 @@
+class FrozenList(list):
+    # An immutable list subclass, intended for use on model fields.
+
+    # Set of overridden methods is taken from collections.abc.MutableSequence.
+
+    def __raise_immutable(self):
+        raise NotImplementedError("cannot modify immutable list")
+
+    def __setitem__(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def __delitem__(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def __iadd__(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def insert(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def append(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def extend(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def pop(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    def remove(self, *_args, **_kwargs):
+        self.__raise_immutable()
+
+    # FrozenList is hashable if everything within it is hashable
+    def __hash__(self):
+        return hash(tuple(self))

pubtools/pulplib/_impl/model/maintenance.py

@@ -5,7 +5,10 @@
 import jsonschema
 
 from pubtools.pulplib._impl import compat_attr as attr
+from .attr import pulp_attrib
+from .convert import read_timestamp, write_timestamp
 from ..schema import load_schema
+from .frozenlist import FrozenList
 from .common import InvalidDataException
 
 
@@ -16,27 +19,23 @@
 HOSTNAME = os.environ.get("HOSTNAME")
 
 
-def iso_time_now():
-    return datetime.datetime.utcnow().strftime("%Y-%m-%dT%H:%M:%SZ")
-
-
 @attr.s(kw_only=True, frozen=True)
 class MaintenanceEntry(object):
     """Details about the maintenance status of a specific repository.
 
     .. versionadded:: 1.4.0
     """
 
-    repo_id = attr.ib(type=str)
+    repo_id = pulp_attrib(type=str)
     """ID of repository in maintenance.
 
     Note: there is no guarantee that a repository of this ID currently exists
     in the Pulp server."""
-    message = attr.ib(default=None, type=str)
+    message = pulp_attrib(default=None, type=str)
     """Why this repository is in maintenance."""
-    owner = attr.ib(default=None, type=str)
+    owner = pulp_attrib(default=None, type=str)
     """Who set this repository in maintenance mode."""
-    started = attr.ib(default=None, type=datetime.datetime)
+    started = pulp_attrib(default=None, type=datetime.datetime)
     """:class:`~datetime.datetime` in UTC at when the maintenance started."""
 
 
@@ -57,15 +56,17 @@ class MaintenanceReport(object):
 
     _SCHEMA = load_schema("maintenance")
 
-    last_updated = attr.ib(default=None, type=datetime.datetime)
+    last_updated = pulp_attrib(default=None, type=datetime.datetime)
     """:class:`~datetime.datetime` in UTC when this report was last updated,
     if it's the first time the report is created, current time is used."""
 
-    last_updated_by = attr.ib(default=None, type=str)
+    last_updated_by = pulp_attrib(default=None, type=str)
     """Person/party who updated the report last time."""
 
-    entries = attr.ib(default=attr.Factory(tuple), type=tuple)
-    """A tuple of :class:`MaintenanceEntry` objects, indicating
+    entries = pulp_attrib(
+        default=attr.Factory(FrozenList), type=list, converter=FrozenList
+    )
+    """A list of :class:`MaintenanceEntry` objects, indicating
     which repositories are in maintenance mode and details.
     If empty, then it means no repositories are in maintenance mode.
     """
@@ -100,20 +101,27 @@ def _from_data(cls, data):
 
         entries = []
         for repo_id, details in data["repos"].items():
-            entries.append(MaintenanceEntry(repo_id=repo_id, **details))
+            entries.append(
+                MaintenanceEntry(
+                    repo_id=repo_id,
+                    message=details["message"],
+                    owner=details["owner"],
+                    started=read_timestamp(details["started"]),
+                )
+            )
 
         maintenance = cls(
-            last_updated=data["last_updated"],
+            last_updated=read_timestamp(data["last_updated"]),
             last_updated_by=data["last_updated_by"],
-            entries=tuple(entries),
+            entries=entries,
         )
 
         return maintenance
 
     def _export_dict(self):
         """export a raw dictionary of maintenance report"""
         report = {
-            "last_updated": self.last_updated,
+            "last_updated": write_timestamp(self.last_updated),
             "last_updated_by": self.last_updated_by,
             "repos": {},
         }
@@ -124,7 +132,7 @@ def _export_dict(self):
                     entry.repo_id: {
                         "message": entry.message,
                         "owner": entry.owner,
-                        "started": entry.started,
+                        "started": write_timestamp(entry.started),
                     }
                 }
             )
@@ -161,7 +169,10 @@ def add(self, repo_ids, **kwargs):
         for repo in repo_ids:
             to_add.append(
                 MaintenanceEntry(
-                    repo_id=repo, owner=owner, message=message, started=iso_time_now()
+                    repo_id=repo,
+                    owner=owner,
+                    message=message,
+                    started=datetime.datetime.utcnow(),
                 )
             )
         entries = list(self.entries)
@@ -178,9 +189,9 @@ def add(self, repo_ids, **kwargs):
 
         return attr.evolve(
             self,
-            entries=tuple(filtered_entries),
+            entries=filtered_entries,
             last_updated_by=owner,
-            last_updated=iso_time_now(),
+            last_updated=datetime.datetime.utcnow(),
         )
 
     def remove(self, repo_ids, **kwargs):
@@ -208,4 +219,4 @@ def remove(self, repo_ids, **kwargs):
             if entry.repo_id not in repo_ids:
                 new_entries.append(entry)
 
-        return attr.evolve(self, last_updated_by=owner, entries=tuple(new_entries))
+        return attr.evolve(self, last_updated_by=owner, entries=new_entries)

pubtools/pulplib/_impl/model/repository/base.py

@@ -1,11 +1,13 @@
 import datetime
 import logging
 
+from attr import validators
 from more_executors.futures import f_map
 
 from ..common import PulpObject, DetachedException
 from ..attr import pulp_attrib
 from ..distributor import Distributor
+from ..frozenlist import FrozenList
 from ...schema import load_schema
 from ... import compat_attr as attr
 
@@ -31,22 +33,22 @@ class PublishOptions(object):
     :meth:`~pubtools.pulplib.Repository.publish`.
     """
 
-    force = attr.ib(default=None, type=bool)
+    force = pulp_attrib(default=None, type=bool)
     """If True, Pulp should publish all data within a repository, rather than attempting
     to publish only changed data (or even skipping the publish).
 
     Setting ``force=True`` may have a major performance impact when publishing large repos.
     """
 
-    clean = attr.ib(default=None, type=bool)
+    clean = pulp_attrib(default=None, type=bool)
     """If True, certain publish tasks will not only publish new/changed content, but
     will also attempt to erase formerly published content which is no longer present
     in the repo.
 
     Setting ``clean=True`` generally implies ``force=True``.
     """
 
-    origin_only = attr.ib(default=None, type=bool)
+    origin_only = pulp_attrib(default=None, type=bool)
     """If ``True``, Pulp should only update the content units / origin path on
     remote hosts.
 
@@ -89,14 +91,15 @@ class Repository(PulpObject):
     """
 
     distributors = pulp_attrib(
-        default=attr.Factory(tuple),
-        type=tuple,
+        default=attr.Factory(FrozenList),
+        type=list,
         pulp_field="distributors",
-        pulp_py_converter=lambda ds: tuple([Distributor.from_data(d) for d in ds]),
+        converter=FrozenList,
+        pulp_py_converter=lambda ds: FrozenList([Distributor.from_data(d) for d in ds]),
         # It's too noisy to let repr descend into sub-objects
         repr=False,
     )
-    """tuple of :class:`~pubtools.pulplib.Distributor` objects belonging to this
+    """list of :class:`~pubtools.pulplib.Distributor` objects belonging to this
     repository.
     """
 
@@ -109,19 +112,24 @@ class Repository(PulpObject):
     )
     """ID of the product to which this repository belongs (if any)."""
 
-    relative_url = attr.ib(default=None, type=str)
+    relative_url = pulp_attrib(default=None, type=str)
     """Default publish URL for this repository, relative to the Pulp content root."""
 
-    mutable_urls = attr.ib(default=attr.Factory(list), type=list, hash=False)
+    mutable_urls = pulp_attrib(
+        default=attr.Factory(FrozenList), type=list, converter=FrozenList
+    )
     """A list of URLs relative to repository publish root which are expected
     to change at every publish (if any content of repo changed)."""
 
-    is_sigstore = attr.ib(default=False, type=bool)
+    is_sigstore = pulp_attrib(default=False, type=bool)
     """True if this is a sigstore repository, used for container image manifest
     signatures."""
 
     is_temporary = pulp_attrib(
-        default=False, type=bool, pulp_field="notes.pub_temp_repo"
+        default=False,
+        type=bool,
+        validator=validators.instance_of(bool),
+        pulp_field="notes.pub_temp_repo",
     )
     """True if this is a temporary repository.
 
@@ -134,22 +142,21 @@ class Repository(PulpObject):
     """
 
     signing_keys = pulp_attrib(
-        default=attr.Factory(list),
+        default=attr.Factory(FrozenList),
         type=list,
         pulp_field="notes.signatures",
         pulp_py_converter=lambda sigs: sigs.split(","),
         py_pulp_converter=",".join,
-        converter=lambda keys: [k.strip() for k in keys],
-        hash=False,
+        converter=lambda keys: FrozenList([k.strip() for k in keys]),
     )
     """A list of GPG signing key IDs used to sign content in this repository."""
 
-    skip_rsync_repodata = attr.ib(default=False, type=bool)
+    skip_rsync_repodata = pulp_attrib(default=False, type=bool)
     """True if this repository is explicitly configured such that a publish of
     this repository will not publish repository metadata to remote hosts.
     """
 
-    _client = attr.ib(default=None, init=False, repr=False, cmp=False)
+    _client = attr.ib(default=None, init=False, repr=False, cmp=False, hash=False)
     # hidden attribute for client attached to this object
 
     @property

pubtools/pulplib/_impl/model/repository/container.py

@@ -10,7 +10,7 @@ class ContainerImageRepository(Repository):
 
     type = pulp_attrib(default="docker-repo", type=str, pulp_field="notes._repo-type")
 
-    registry_id = attr.ib(
+    registry_id = pulp_attrib(
         default=attr.Factory(lambda self: self.id, takes_self=True), type=str
     )
     """The ID of this repository in a container image registry.