Repository: zimeon/ocfl-py

Issue:
Detect E066 for fixture E066_E092_old_manifest_digest_incorrect
See https://github.com/OCFL/fixtures/pull/71

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/39152673/badge)](https://coveralls.io/builds/39152673)

Coverage increased (+0.03%) to 95.215% when pulling **32cab89ec8397c6dd2eb4779b57d3f34d9f138e6 on issue-49** into **008828b7a089aea1b43b87ffeb42d3254faee1ed on main**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/39152673/badge)](https://coveralls.io/builds/39152673)

Coverage increased (+0.03%) to 95.215% when pulling **32cab89ec8397c6dd2eb4779b57d3f34d9f138e6 on issue-49** into **008828b7a089aea1b43b87ffeb42d3254faee1ed on main**.


Patch:
diff --git a/ocfl/data/validation-errors.json b/ocfl/data/validation-errors.json
index 2c6f570..28f5fea 100644
--- a/ocfl/data/validation-errors.json
+++ b/ocfl/data/validation-errors.json
@@ -415,17 +415,29 @@
     }
   },
  "E066b": {
-    "params": ["version_dir" , "prior_head", "where"],
+    "params": ["version" , "prior_head", "where"],
     "description": {
        "en": "OCFL Object inventory manifest for %s in %s doesn't have a subset of manifest entries of inventory for %s"
     }
   },
   "E066c": {
-    "params": ["prior_head", "version_dir", "file", "prior_content", "where", "current_content"],
+    "params": ["prior_head", "version", "file", "prior_content", "where", "current_content"],
     "description": {
        "en": "OCFL Object %s inventory %s version state has file %s that maps to different content files (%s) than in the %s inventory (%s)"
     }
   },
+  "E066d": {
+      "params": ["where", "version", "digest", "logical_files", "prior_head"],
+      "description": {
+         "en": "OCFL Object %s inventory %s version state has digest %s (mapping to logical files %s) that does not appear in the %s inventory"
+      }
+  },
+  "E066e": {
+      "params": ["prior_head", "version", "digest", "logical_files", "where"],
+      "description": {
+         "en": "OCFL Object %s inventory %s version state has digest %s (mapping to logical files %s) that does not appear in the %s inventory"
+      }
+  },
   "E067": {
     "params": ["entry"],
     "description": {
@@ -614,7 +626,7 @@
     "spec": "In addition to the inventory in the OCFL Object Root, every version directory SHOULD include an inventory file that is an Inventory of all content for versions up to and including that particular version"
   },
   "W011": {
-    "params": ["key", "version_dir" , "prior_head", "where"],
+    "params": ["key", "version" , "prior_head", "where"],
     "description": {
        "en": "OCFL Object version metadata '%s' for %s in %s inventory does not match that in %s inventory"
     },
diff --git a/ocfl/inventory_validator.py b/ocfl/inventory_validator.py
index 0eb08ab..2dbe328 100644
--- a/ocfl/inventory_validator.py
+++ b/ocfl/inventory_validator.py
@@ -10,9 +10,15 @@ from .validation_logger import ValidationLogger
 from .w3c_datetime import str_to_datetime
 
 
-def get_file_map(inventory, version_dir):
-    """Get a map of files in state to files on disk for version_dir in inventory."""
-    state = inventory['versions'][version_dir]['state']
+def get_file_map(inventory, version):
+    """Get a map of files in state to files on disk for version in inventory.
+
+    Returns a dictionary: file_in_state -> set(content_files)
+
+    The set of content_files may includes references to duplicate files in
+    later versions than the version being described.
+    """
+    state = inventory['versions'][version]['state']
     manifest = inventory['manifest']
     file_map = {}
     for digest in state:
@@ -432,9 +438,11 @@ class InventoryValidator():
         return True
 
     def validate_as_prior_version(self, prior):
-        """Check that prior is a valid InventoryValidator for a prior version of the current inventory object.
+        """Check that prior is a valid prior version of the current inventory object.
 
-        Both inventories are assumed to have been checked for internal consistency.
+        The input prior is also expected to be an InventoryValidator object and
+        both self and prior inventories are assumed to have been checked for
+        internal consistency.
         """
         # Must have a subset of versions which also check zero padding format etc.
         if not set(prior.all_versions) < set(self.all_versions):
@@ -442,22 +450,53 @@ class InventoryValidator():
         else:
             # Check references to files but realize that there might be different
             # digest algorithms between versions
-            version_dir = 'no-version'
-            for version_dir in prior.all_versions:
-                prior_map = get_file_map(prior.inventory, version_dir)
-                self_map = get_file_map(self.inventory, version_dir)
+            version = 'no-version'
+            for version in prior.all_versions:
+                # If the digest algorithm is the same then we can make a
+                # direct check on whether the state blocks match
+                if prior.digest_algorithm == self.digest_algorithm:
+                    self.compare_states_for_version(prior, version)
+                # Now check the mappings from state to content files which must
+                # be consistent even if the digestAlgorithm is different between
+                # versions
+                prior_map = get_file_map(prior.inventory, version)
+                self_map = get_file_map(self.inventory, version)
                 if prior_map.keys() != self_map.keys():
-                    self.error('E066b', version_dir=version_dir, prior_head=prior.head)
+                    self.error('E066b', version=version, prior_head=prior.head)
                 else:
                     # Check them all...
                     for file in prior_map:
                         if not prior_map[file].issubset(self_map[file]):
-                            self.error('E066c', version_dir=version_dir, prior_head=prior.head,
+                            self.error('E066c', version=version, prior_head=prior.head,
                                        file=file, prior_content=','.join(prior_map[file]),
                                        current_content=','.join(self_map[file]))
-            # Check metadata
-            prior_version = prior.inventory['versions'][version_dir]
-            self_version = self.inventory['versions'][version_dir]
-            for key in ('created', 'message', 'user'):
-                if prior_version.get(key) != self_version.get(key):
-                    self.warning('W011', version_dir=version_dir, prior_head=prior.head, key=key)
+                # Check metadata
+                prior_version = prior.inventory['versions'][version]
+                self_version = self.inventory['versions'][version]
+                for key in ('created', 'message', 'user'):
+                    if prior_version.get(key) != self_version.get(key):
+                        self.warning('W011', version=version, prior_head=prior.head, key=key)
+
+    def compare_states_for_version(self, prior, version):
+        """Compare state blocks for version between self and prior.
+
+        The digest algorithm must be the same in both, do not call otherwise!
+        Looks only for digests that appear in one but not in the other, the code
+        in validate_as_prior_version(..) does a check for whether the same sets
+        of logical files appear and we don't want to duplicate an error message
+        about that.
+
+        While the mapping checks in validate_as_prior_version(..) do all that is
+        necessary to detect an error, the additional errors that may be generated
+        here provide more detailed diagnostics in the case that the digest
+        algorithm is the same across versions being compared.
+        """
+        self_state = self.inventory['versions'][version]['state']
+        prior_state = prior.inventory['versions'][version]['state']
+        for digest in set(self_state.keys()).union(prior_state.keys()):
+            if digest not in prior_state:
+                self.error('E066d', version=version, prior_head=prior.head,
+                           digest=digest, logical_files=', '.join(self_state[digest]))
+            elif digest not in self_state:
+                self.error('E066e', version=version, prior_head=prior.head,
+                           digest=digest, logical_files=', '.join(prior_state[digest]))


Test patch:
diff --git a/tests/test_inventory_validator.py b/tests/test_inventory_validator.py
index 1d2a387..6070546 100644
--- a/tests/test_inventory_validator.py
+++ b/tests/test_inventory_validator.py
@@ -300,11 +300,13 @@ class TestAll(unittest.TestCase):
         log.clear()
         # Good inventory in spite of diferent digests
         iv.all_versions = ['v1', 'v2']
+        iv.digest_algorithm = 'a1'
         iv.inventory = {"manifest": {"a1d1": ["v1/content/f1"],
                                      "a1d2": ["v1/content/f2"],
                                      "a1d3": ["v2/content/f3"]},
                         "versions": {"v1": {"state": {"a1d1": ["f1"], "a1d2": ["f2"]}},
                                      "v2": {"state": {"a1d1": ["f1"], "a1d3": ["f3"]}}}}
+        prior.digest_algorithm = 'a2'
         prior.inventory = {"manifest": {"a2d1": ["v1/content/f1"],
                                         "a2d2": ["v1/content/f2"]},
                            "versions": {"v1": {"state": {"a2d1": ["f1"], "a2d2": ["f2"]}}}}
@@ -322,6 +324,31 @@ class TestAll(unittest.TestCase):
         iv.validate_as_prior_version(prior)
         self.assertEqual(log.errors, ["E066c"])
 
+    def test_compare_states_for_version(self):
+        """Test compare_states_for_version method."""
+        log = TLogger()
+        iv = InventoryValidator(log=log)
+        prior = InventoryValidator(log=TLogger())
+        # Same digests
+        iv.inventory = {
+            "versions": {"v99": {"state": {"a1d1": ["f1"], "a1d2": ["f2", "f3"]}}}}
+        prior.inventory = {
+            "versions": {"v99": {"state": {"a1d1": ["f1"], "a1d2": ["f2", "f3"]}}}}
+        iv.compare_states_for_version(prior, 'v99')
+        self.assertEqual(log.errors, [])
+        log.clear()
+        # Extra in iv
+        iv.inventory = {
+            "versions": {"v99": {"state": {"a1d1": ["f1"], "a1d2": ["f2", "f3"], "a1d3": ["f4"]}}}}
+        iv.compare_states_for_version(prior, 'v99')
+        self.assertEqual(log.errors, ['E066d'])
+        log.clear()
+        # Extra in prior
+        iv.inventory = {
+            "versions": {"v99": {"state": {"a1d2": ["f2", "f3"]}}}}
+        iv.compare_states_for_version(prior, 'v99')
+        self.assertEqual(log.errors, ['E066e'])
+
     def test_check_content_path(self):
         """Test check_content_path method."""
         log = TLogger()
diff --git a/tests/test_validator.py b/tests/test_validator.py
index d77fde3..1d27e02 100644
--- a/tests/test_validator.py
+++ b/tests/test_validator.py
@@ -60,7 +60,7 @@ class TestAll(unittest.TestCase):
                            'E061_invalid_sidecar': ['E061'],
                            'E063_no_inv': ['E063'],
                            'E064_different_root_and_latest_inventories': ['E064'],
-                           'E066_E092_old_manifest_digest_incorrect': ['E092a'],
+                           'E066_E092_old_manifest_digest_incorrect': ['E066d', 'E066e', 'E092a'],
                            'E066_algorithm_change_state_mismatch': ['E066b'],
                            'E066_inconsistent_version_state': ['E066b'],
                            'E067_file_in_extensions_dir': ['E067'],