Fix all model classes to meet invariants

Most model classes had some bugs causing them to fail invariant
checks. Fix them all. The most common issue was missing validation
against type.

Two larger changes include:

- MaintenanceReport: some fields were declared and documented
  as type=datetime, but all of the code was implemented using
  strings (see related test update). This has been fixed to use
  datetime throughout.

- Page previously was not frozen at all. Now it is, like the other
  model classes.

Author: rohanpm

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/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/maintenance.py

@@ -5,6 +5,8 @@
 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
@@ -17,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."""
 
 
@@ -58,14 +56,16 @@ 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(FrozenList), type=list, converter=FrozenList)
+    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.
@@ -101,10 +101,17 @@ 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=entries,
         )
@@ -114,7 +121,7 @@ def _from_data(cls, data):
     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": {},
         }
@@ -125,7 +132,7 @@ def _export_dict(self):
                     entry.repo_id: {
                         "message": entry.message,
                         "owner": entry.owner,
-                        "started": entry.started,
+                        "started": write_timestamp(entry.started),
                     }
                 }
             )
@@ -162,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)
@@ -181,7 +191,7 @@ def add(self, repo_ids, **kwargs):
             self,
             entries=filtered_entries,
             last_updated_by=owner,
-            last_updated=iso_time_now(),
+            last_updated=datetime.datetime.utcnow(),
         )
 
     def remove(self, repo_ids, **kwargs):

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

@@ -1,6 +1,7 @@
 import datetime
 import logging
 
+from attr import validators
 from more_executors.futures import f_map
 
 from ..common import PulpObject, DetachedException
@@ -32,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.
 
@@ -111,21 +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(
+    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.
 
@@ -147,12 +151,12 @@ class Repository(PulpObject):
     )
     """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.

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

@@ -1,6 +1,7 @@
 import os
 import logging
 
+from attr import validators
 from more_executors.futures import f_flat_map, f_map
 
 from .base import Repository, repo_type
@@ -27,6 +28,7 @@ class FileRepository(Repository):
             lambda self: self.id == "redhat-sigstore", takes_self=True
         ),
         type=bool,
+        validator=validators.instance_of(bool),
     )
 
     mutable_urls = attr.ib(

pubtools/pulplib/_impl/model/task.py

@@ -16,26 +16,26 @@ class Task(PulpObject):
     id = pulp_attrib(type=str, pulp_field="task_id")
     """ID of this task (str)."""
 
-    completed = attr.ib(default=None, type=bool)
+    completed = pulp_attrib(default=None, type=bool)
     """True if this task has completed, successfully or otherwise.
 
     May be `None` if the state of this task is unknown.
     """
 
-    succeeded = attr.ib(default=None, type=bool)
+    succeeded = pulp_attrib(default=None, type=bool)
     """True if this task has completed successfully.
 
     May be `None` if the state of this task is unknown.
     """
 
-    error_summary = attr.ib(default=None, type=str)
+    error_summary = pulp_attrib(default=None, type=str)
     """A summary of the reason for this task's failure (if any).
 
     This is a short string, generally a single line, suitable for display to users.
     The string includes the ID of the failed task.
     """
 
-    error_details = attr.ib(default=None, type=str)
+    error_details = pulp_attrib(default=None, type=str)
     """Detailed information for this task's failure (if any).
 
     This may be a multi-line string and may include technical information such as
@@ -62,7 +62,7 @@ class Task(PulpObject):
          "pulp:action:publish"]
     """
 
-    repo_id = attr.ib(type=str)
+    repo_id = pulp_attrib(type=str)
     """The ID of the repository associated with this task, otherwise None."""
 
     units = pulp_attrib(

pubtools/pulplib/_impl/page.py

@@ -1,12 +1,21 @@
 import logging
 import weakref
+from concurrent.futures import Future
 
 from . import compat_attr as attr
+from .model.frozenlist import FrozenList
+from .model.attr import pulp_attrib
 
 LOG = logging.getLogger("pubtools.pulplib")
 
 
-@attr.s(kw_only=True)
+# pylint shows these spurious errors on this file, not sure why:
+# page.py:86:23: E1101: Instance of '_CountingAttr' has no 'done' member (no-member)
+# Could be due to bug https://github.com/PyCQA/pylint/issues/1694 ?
+# pylint: disable=no-member
+
+
+@attr.s(kw_only=True, frozen=True)
 class Page(object):
     """A page of Pulp search results.
 
@@ -54,14 +63,16 @@ def handle_results(page):
 
     """
 
-    data = attr.ib(default=attr.Factory(list))
+    data = pulp_attrib(
+        default=attr.Factory(FrozenList), type=list, converter=FrozenList
+    )
     """List of Pulp objects in this page.
 
     This list will contain instances of the appropriate Pulp object type
     (e.g. :class:`~pubtools.pulplib.Repository` for a repository search).
     """
 
-    next = attr.ib(default=None)
+    next = pulp_attrib(default=None, type=Future)
     """None, if this is the last page of results.
 
     Otherwise, a Future[:class:`Page`] for the next page of results.

tests/client/test_client.py

@@ -1,4 +1,5 @@
 import logging
+import datetime
 import json
 import pytest
 import requests_mock
@@ -201,7 +202,7 @@ def test_can_get_maintenance_report(client, requests_mocker):
     assert requests_mocker.call_count == 1
     assert isinstance(report, MaintenanceReport)
     assert report.last_updated_by == "Content Delivery"
-    assert report.last_updated == "2019-08-15T14:21:12Z"
+    assert report.last_updated == datetime.datetime(2019, 8, 15, 14, 21, 12)
     assert report.entries[0].message == "Maintenance Mode Enabled"