Repository: zimeon/ocfl-py

Issue:
Storage Root `extensions/` directory flagged as validation error
When using a custom storage layout extension, the configuration of the extension is placed in the storage root as:
`[storage-root]/extensions/NNNN-flat-omit-prefix-storage-layout/config.json`

The ocfl-validate utility flags this as an error:
```
[E072] OCFL storage root hierarchy include directory /extensions/NNNN-flat-omit-prefix-storage-layout with at least one file but no object declaration. Such additional files are not allowed (see https://ocfl.io/1.0/spec/#E072)
```

Per the spec, I believe this should be valid.
See: https://ocfl.io/1.0/spec/#storage-root-extensions

Discussion:

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index b6f1827..d7167bf 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -5,7 +5,8 @@
   * Additional validation improvements:
     * Checks between version state in different version inventories
     * Check to see is extra directories look like version directories
-    * Fix URI scheme syntax check
+    * Fix URI scheme syntax check (https://github.com/zimeon/ocfl-py/issues/75)
+    * Check extensions directory in storage root (https://github.com/zimeon/ocfl-py/issues/76)
   * Use additional fixtures in https://github.com/OCFL/fixtures for tests
 
 ## 2021-04-26 v1.2.2
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/0=ocfl_1.0 b/extra_fixtures/good-storage-roots/reg-extension-dir-root/0=ocfl_1.0
new file mode 100644
index 0000000..03c9c3d
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/0=ocfl_1.0
@@ -0,0 +1,1 @@
+ocfl_1.0
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/0=ocfl_object_1.0 b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/0=ocfl_object_1.0
new file mode 100644
index 0000000..4d1d62c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/0=ocfl_object_1.0
@@ -0,0 +1,1 @@
+ocfl_object_1.0
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/inventory.json b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/inventory.json
new file mode 100644
index 0000000..a0f568c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/inventory.json
@@ -0,0 +1,26 @@
+{
+  "digestAlgorithm": "sha512",
+  "head": "v1",
+  "id": "ark:123/abc",
+  "manifest": {
+    "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+      "v1/content/a_file.txt"
+    ]
+  },
+  "type": "https://ocfl.io/1.0/spec/#inventory",
+  "versions": {
+    "v1": {
+      "created": "2019-01-01T02:03:04Z",
+      "message": "An version with one file",
+      "state": {
+        "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+          "a_file.txt"
+        ]
+      },
+      "user": {
+        "address": "mailto:a_person@example.org",
+        "name": "A Person"
+      }
+    }
+  }
+}
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/inventory.json.sha512 b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/inventory.json.sha512
new file mode 100644
index 0000000..35c7a7c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/inventory.json.sha512
@@ -0,0 +1,1 @@
+f889cd4ba8cfd5b52c5f8c9ca99cb404586e60ee5d5b9b5508338f296776bf613719175253d9027c7e166ede7182785889b5ca59e19e441e47cde56b5bc20949 inventory.json
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/content/a_file.txt b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/content/a_file.txt
new file mode 100644
index 0000000..2baefe4
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/content/a_file.txt
@@ -0,0 +1,1 @@
+Hello! I am a file.
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/inventory.json b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/inventory.json
new file mode 100644
index 0000000..a0f568c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/inventory.json
@@ -0,0 +1,26 @@
+{
+  "digestAlgorithm": "sha512",
+  "head": "v1",
+  "id": "ark:123/abc",
+  "manifest": {
+    "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+      "v1/content/a_file.txt"
+    ]
+  },
+  "type": "https://ocfl.io/1.0/spec/#inventory",
+  "versions": {
+    "v1": {
+      "created": "2019-01-01T02:03:04Z",
+      "message": "An version with one file",
+      "state": {
+        "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+          "a_file.txt"
+        ]
+      },
+      "user": {
+        "address": "mailto:a_person@example.org",
+        "name": "A Person"
+      }
+    }
+  }
+}
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/inventory.json.sha512 b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/inventory.json.sha512
new file mode 100644
index 0000000..35c7a7c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/b02/c71/b67/afe/ark%3A123%2Fabc/v1/inventory.json.sha512
@@ -0,0 +1,1 @@
+f889cd4ba8cfd5b52c5f8c9ca99cb404586e60ee5d5b9b5508338f296776bf613719175253d9027c7e166ede7182785889b5ca59e19e441e47cde56b5bc20949 inventory.json
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/extensions/0003-hash-and-id-n-tuple-storage-layout/config.json b/extra_fixtures/good-storage-roots/reg-extension-dir-root/extensions/0003-hash-and-id-n-tuple-storage-layout/config.json
new file mode 100644
index 0000000..687bc8f
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/extensions/0003-hash-and-id-n-tuple-storage-layout/config.json
@@ -0,0 +1,7 @@
+{
+    "extensionName": "0003-hash-and-id-n-tuple-storage-layout",
+    "digestAlgorithm": "sha256",
+    "tupleSize": 3,
+    "numberOfTuples": 4
+}
+
diff --git a/extra_fixtures/good-storage-roots/reg-extension-dir-root/ocfl_layout.json b/extra_fixtures/good-storage-roots/reg-extension-dir-root/ocfl_layout.json
new file mode 100644
index 0000000..c77faff
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/reg-extension-dir-root/ocfl_layout.json
@@ -0,0 +1,4 @@
+{
+    "extension": "0003-hash-and-id-n-tuple-storage-layout",
+    "description": "OCFL Community Extension 0003: Hashed Truncated N-tuple Trees with Object ID Encapsulating Directory for OCFL Storage Hierarchies"
+}
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/0=ocfl_1.0 b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/0=ocfl_1.0
new file mode 100644
index 0000000..03c9c3d
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/0=ocfl_1.0
@@ -0,0 +1,1 @@
+ocfl_1.0
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/0=ocfl_object_1.0 b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/0=ocfl_object_1.0
new file mode 100644
index 0000000..4d1d62c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/0=ocfl_object_1.0
@@ -0,0 +1,1 @@
+ocfl_object_1.0
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/inventory.json b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/inventory.json
new file mode 100644
index 0000000..a0f568c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/inventory.json
@@ -0,0 +1,26 @@
+{
+  "digestAlgorithm": "sha512",
+  "head": "v1",
+  "id": "ark:123/abc",
+  "manifest": {
+    "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+      "v1/content/a_file.txt"
+    ]
+  },
+  "type": "https://ocfl.io/1.0/spec/#inventory",
+  "versions": {
+    "v1": {
+      "created": "2019-01-01T02:03:04Z",
+      "message": "An version with one file",
+      "state": {
+        "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+          "a_file.txt"
+        ]
+      },
+      "user": {
+        "address": "mailto:a_person@example.org",
+        "name": "A Person"
+      }
+    }
+  }
+}
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/inventory.json.sha512 b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/inventory.json.sha512
new file mode 100644
index 0000000..35c7a7c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/inventory.json.sha512
@@ -0,0 +1,1 @@
+f889cd4ba8cfd5b52c5f8c9ca99cb404586e60ee5d5b9b5508338f296776bf613719175253d9027c7e166ede7182785889b5ca59e19e441e47cde56b5bc20949 inventory.json
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/content/a_file.txt b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/content/a_file.txt
new file mode 100644
index 0000000..2baefe4
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/content/a_file.txt
@@ -0,0 +1,1 @@
+Hello! I am a file.
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/inventory.json b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/inventory.json
new file mode 100644
index 0000000..a0f568c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/inventory.json
@@ -0,0 +1,26 @@
+{
+  "digestAlgorithm": "sha512",
+  "head": "v1",
+  "id": "ark:123/abc",
+  "manifest": {
+    "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+      "v1/content/a_file.txt"
+    ]
+  },
+  "type": "https://ocfl.io/1.0/spec/#inventory",
+  "versions": {
+    "v1": {
+      "created": "2019-01-01T02:03:04Z",
+      "message": "An version with one file",
+      "state": {
+        "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f": [
+          "a_file.txt"
+        ]
+      },
+      "user": {
+        "address": "mailto:a_person@example.org",
+        "name": "A Person"
+      }
+    }
+  }
+}
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/inventory.json.sha512 b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/inventory.json.sha512
new file mode 100644
index 0000000..35c7a7c
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/ark%3A123%2Fabc/v1/inventory.json.sha512
@@ -0,0 +1,1 @@
+f889cd4ba8cfd5b52c5f8c9ca99cb404586e60ee5d5b9b5508338f296776bf613719175253d9027c7e166ede7182785889b5ca59e19e441e47cde56b5bc20949 inventory.json
diff --git a/extra_fixtures/good-storage-roots/unreg-extension-dir-root/extensions/unregistered-extension-name/config.json b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/extensions/unregistered-extension-name/config.json
new file mode 100644
index 0000000..2c63c08
--- /dev/null
+++ b/extra_fixtures/good-storage-roots/unreg-extension-dir-root/extensions/unregistered-extension-name/config.json
@@ -0,0 +1,2 @@
+{
+}
diff --git a/ocfl/data/validation-errors.json b/ocfl/data/validation-errors.json
index b60e07a..cc5a170 100644
--- a/ocfl/data/validation-errors.json
+++ b/ocfl/data/validation-errors.json
@@ -462,6 +462,12 @@
        "en": "OCFL storage root hierarchy contains an empty directory: %s"
     }
   },
+  "E086": {
+    "params": ["entry"],
+    "description": {
+       "en": "OCFL Storage Root extensions direct contains an unexpected non-directory entry: %s"
+    }
+  },
   "E087": {
     "description": {
        "en": "NOTE - E087 is a processing instruction and can't be tested for."
@@ -643,5 +649,11 @@
     "description": {
        "en": "OCFL Object includes unregistered extension directory '%s'"
     }
+  },
+  "W901": {
+    "params": ["entry"],
+    "description": {
+       "en": "OCFL Storage Root includes unregistered extension directory '%s'"
+    }
   }
 }
diff --git a/ocfl/pyfs.py b/ocfl/pyfs.py
index d897698..f225955 100644
--- a/ocfl/pyfs.py
+++ b/ocfl/pyfs.py
@@ -74,9 +74,12 @@ def ocfl_walk(f, dir='/', is_storage_root=False):
 
     For walking storage roots (is_storage_root=True) then the condition to
     descend is:
-        1) this is the root (dirpaht == '/'), or
+        1) this is the root (dirpath == '/'), or
         2) there are no files in this directory (see https://ocfl.io/1.0/spec/#root-structure)
 
+    For each directory will yield (dirpath, dirs, files) as does os.walk. The
+    value of dirs my be pruned to avoid descending into particular directories.
+
     FIXME - QUICK AND DIRTY HACK, CAN DO BETTER!
     """
     if not dir.startswith('/'):
@@ -88,7 +91,6 @@ def ocfl_walk(f, dir='/', is_storage_root=False):
         entries = f.listdir(dirpath)
         files = []
         dirs = []
-        dirpaths = []
         for entry in entries:
             entry_path = fs.path.join(dirpath, entry)
             is_dir = True
@@ -100,14 +102,15 @@ def ocfl_walk(f, dir='/', is_storage_root=False):
                 pass  # Assume to be a directory
             if is_dir:
                 dirs.append(entry)
-                dirpaths.append(entry_path)
             else:
                 files.append(entry)
+        yield(dirpath, dirs, files)
+        # dirs may have been modified to prune
         if not is_storage_root or dirpath == '/' or len(files) == 0:
             # If this is not the storage root itself and there are files
             # present then we should not descend further
-            stack.extend(dirpaths)
-        yield(dirpath, dirs, files)
+            for entry in dirs:
+                stack.append(fs.path.join(dirpath, entry))
 
 
 def ocfl_opendir(pyfs, dir, **kwargs):
diff --git a/ocfl/store.py b/ocfl/store.py
index 71ec5ec..d66dc31 100644
--- a/ocfl/store.py
+++ b/ocfl/store.py
@@ -34,12 +34,19 @@ class Store():
         self.declaration_tvalue = 'ocfl_1.0'
         self.spec_file = 'ocfl_1.0.txt'
         self.layout_file = 'ocfl_layout.json'
+        self.registered_extensions = [
+            # '0002-flat-direct-storage-layout',  # not included because doesn't have config
+            '0003-hash-and-id-n-tuple-storage-layout',
+            '0004-hashed-n-tuple-storage-layout'
+        ]
         #
         self.root_fs = None
         self.num_traversal_errors = 0
         self.extension = None
         self.description = None
         self.log = None
+        self.num_objects = 0
+        self.good_objects = 0
 
     def open_root_fs(self, create=False):
         """Open pyfs filesystem for this OCFL storage root."""
@@ -146,7 +153,10 @@ class Store():
         """
         for (dirpath, dirs, files) in ocfl_walk(self.root_fs, is_storage_root=True):
             if dirpath == '/':
-                pass  # Ignore files in storage root
+                if 'extensions' in dirs:
+                    self.validate_extensions_dir()
+                    dirs.remove('extensions')
+                # Ignore any other files in storage root
             elif (len(dirs) + len(files)) == 0:
                 self.traversal_error("E073", path=dirpath)
             elif len(files) == 0:
@@ -168,19 +178,35 @@ class Store():
                 else:
                     self.traversal_error("E072", path=dirpath)
 
+    def validate_extensions_dir(self):
+        """Validate content of extensions directory inside storage root.
+
+        Validate the extensions directory by checking that there aren't any
+        entries in the extensions directory that aren't directories themselves.
+        Where there are extension directories they SHOULD be registered and
+        this code relies up the registered_extensions property to list known
+        storage root extensions.
+        """
+        for entry in self.root_fs.scandir('extensions'):
+            if entry.is_dir:
+                if entry.name not in self.registered_extensions:
+                    self.log.warning('W901', entry=entry.name)  # FIXME - No good warning code in spec
+            else:
+                self.traversal_error('E086', entry=entry.name)
+
     def list(self):
         """List contents of this OCFL storage root."""
         self.open_root_fs()
         self.check_root_structure()
-        num_objects = 0
+        self.num_objects = 0
         for dirpath in self.object_paths():
             with ocfl_opendir(self.root_fs, dirpath) as obj_fs:
                 # Parse inventory to extract id
                 id = Object(obj_fs=obj_fs).id_from_inventory()
                 print("%s -- id=%s" % (dirpath, id))
-                num_objects += 1
+                self.num_objects += 1
                 # FIXME - maybe do some more stuff in here
-        logging.info("Found %d OCFL Objects under root %s", num_objects, self.root)
+        logging.info("Found %d OCFL Objects under root %s", self.num_objects, self.root)
 
     def validate_hierarchy(self, validate_objects=True, check_digests=True, show_warnings=False):
         """Validate storage root hierarchy.
@@ -217,18 +243,18 @@ class Store():
         except StoreException as e:
             valid = False
             logging.info("Storage root structure is INVALID (%s)", str(e))
-        num_objects, good_objects = self.validate_hierarchy(validate_objects=validate_objects, check_digests=check_digests, show_warnings=show_warnings)
+        self.num_objects, self.good_objects = self.validate_hierarchy(validate_objects=validate_objects, check_digests=check_digests, show_warnings=show_warnings)
         if validate_objects:
-            if good_objects == num_objects:
-                logging.info("Objects checked: %d / %d are VALID", good_objects, num_objects)
+            if self.good_objects == self.num_objects:
+                logging.info("Objects checked: %d / %d are VALID", self.good_objects, self.num_objects)
             else:
                 valid = False
-                logging.info("Objects checked: %d / %d are INVALID", num_objects - good_objects, num_objects)
+                logging.info("Objects checked: %d / %d are INVALID", self.num_objects - self.good_objects, self.num_objects)
         else:
             logging.info("Not checking OCFL objects")
+        print(str(self.log))
         if self.num_traversal_errors > 0:
             valid = False
-            print(str(self.log))
             logging.info("Encountered %d errors traversing storage root", self.num_traversal_errors)
         # FIXME - do some stuff in here
         if valid:


Test patch:
diff --git a/tests/test_store.py b/tests/test_store.py
index ea65fbb..328ec20 100644
--- a/tests/test_store.py
+++ b/tests/test_store.py
@@ -155,7 +155,24 @@ class TestAll(unittest.TestCase):
 
     def test_validate(self):
         """Test validate method."""
-        for root in ['extra_fixtures/good-storage-roots/fedora-root',
-                     'extra_fixtures/good-storage-roots/simple-root']:
-            s = Store(root=root)
-            self.assertTrue(s.validate())
+        s = Store(root='extra_fixtures/good-storage-roots/fedora-root')
+        self.assertTrue(s.validate())
+        self.assertEqual(s.num_objects, 176)
+        self.assertEqual(s.good_objects, 176)
+        # Simple case of three objects
+        s = Store(root='extra_fixtures/good-storage-roots/simple-root')
+        self.assertTrue(s.validate())
+        self.assertEqual(s.num_objects, 3)
+        self.assertEqual(s.good_objects, 3)
+        # Reg extension will not give warning
+        s = Store(root='extra_fixtures/good-storage-roots/reg-extension-dir-root')
+        self.assertTrue(s.validate())
+        self.assertEqual(s.num_objects, 1)
+        self.assertEqual(s.good_objects, 1)
+        self.assertNotIn('W901', s.log.codes)
+        # Unreg extension will give warning
+        s = Store(root='extra_fixtures/good-storage-roots/unreg-extension-dir-root')
+        self.assertTrue(s.validate())
+        self.assertEqual(s.num_objects, 1)
+        self.assertEqual(s.good_objects, 1)
+        self.assertIn('W901', s.log.codes)