Ensure DictStore contains only bytes (#350)

* Ensure `DictStore` gets `bytes` coercible data

As the spec requires that the data in a store be a sequence of `bytes`,
make sure that non-`DictStore` input meets this requirement when setting
values. This effectively ensures that other `DictStore` meet this
requirement as well. So we don't need to go through and check their
values too.

* Drop undefined buffer size case from `DictStore`

As everything in `DictStore` must either be another `DictStore` or
`bytes`, there shouldn't be any cases where the size is undefined nor
cases that this exception should need handling. Given this go ahead and
drop the special casing for unknown sizes in `DictStore`.

* Drop `test_getsize_ext`

While this test case does test a useful subset of the `getsize` API, the
contents being added to the store here are non-conforming to our
expectations of store contents. Namely the store should only contain
values that are an "arbitrary sequence of bytes", which this test case
is not.

* Test `getsize` with an unknown size case

This creates a non-conforming store to make sure that `getsize` handles
its contents in the expected way. Namely that it returns `-1`.

* Note `DictStore` only contains `bytes` now        [ci skip]

* Check for `TypeError` for non-buffer objects

Add a test to ensure that a non-buffer supporting object when stored
into a valid store, will raise a `TypeError` instead of storing it.
Disable this checking for generic `MappingStore`s (e.g. `dict`) as they
do not perform this sort of checking on the data they accept as values.

* Check that `DictStore` coerces all data to `bytes`

Provide a simple test for `DictStore` to ensure that non-`bytes` is
coerced to `bytes` before storing it and is retrieved as `bytes`.

* Disallow mutation of the internal `DictStore`

Make sure that users are only able to add data to the `DictStore`.
Disallow the storing of a nested `DictStore` though.

Author: jakirkham

docs/release.rst

@@ -35,6 +35,9 @@ Bug fixes
 * Failing tests related to pickling/unpickling have been fixed. By :user:`Ryan Williams <ryan-williams>`,
   :issue:`273`, :issue:`308`.
 
+* Ensure ``DictStore`` contains only ``bytes`` to facilitate comparisons and protect against writes.
+  By :user:`John Kirkham <jakirkham>`, :issue:`350`
+
 Maintenance
 ~~~~~~~~~~~
 

zarr/storage.py

@@ -544,6 +544,7 @@ def __getitem__(self, item):
     def __setitem__(self, item, value):
         with self.write_mutex:
             parent, key = self._require_parent(item)
+            value = ensure_bytes(value)
             parent[key] = value
 
     def __delitem__(self, item):
@@ -642,17 +643,11 @@ def getsize(self, path=None):
             size = 0
             for v in value.values():
                 if not isinstance(v, self.cls):
-                    try:
-                        size += buffer_size(v)
-                    except TypeError:
-                        return -1
+                    size += buffer_size(v)
             return size
 
         else:
-            try:
-                return buffer_size(value)
-            except TypeError:
-                return -1
+            return buffer_size(value)
 
     def clear(self):
         with self.write_mutex:

zarr/tests/test_storage.py

@@ -63,6 +63,12 @@ def test_get_set_del_contains(self):
                 # noinspection PyStatementEffect
                 del store['foo']
 
+    def test_set_invalid_content(self):
+        store = self.create_store()
+
+        with pytest.raises(TypeError):
+            store['baz'] = list(range(5))
+
     def test_clear(self):
         store = self.create_store()
         store['foo'] = b'bar'
@@ -586,6 +592,10 @@ class TestMappingStore(StoreTests, unittest.TestCase):
     def create_store(self):
         return dict()
 
+    def test_set_invalid_content(self):
+        # Generic mappings support non-buffer types
+        pass
+
 
 def setdel_hierarchy_checks(store):
     # these tests are for stores that are aware of hierarchy levels; this
@@ -629,17 +639,14 @@ class TestDictStore(StoreTests, unittest.TestCase):
     def create_store(self):
         return DictStore()
 
-    def test_setdel(self):
+    def test_store_contains_bytes(self):
         store = self.create_store()
-        setdel_hierarchy_checks(store)
+        store['foo'] = np.array([97, 98, 99, 100, 101], dtype=np.uint8)
+        assert store['foo'] == b'abcde'
 
-    def test_getsize_ext(self):
+    def test_setdel(self):
         store = self.create_store()
-        store['a'] = list(range(10))
-        store['b/c'] = list(range(10))
-        assert -1 == store.getsize()
-        assert -1 == store.getsize('a')
-        assert -1 == store.getsize('b')
+        setdel_hierarchy_checks(store)
 
 
 class TestDirectoryStore(StoreTests, unittest.TestCase):
@@ -1096,6 +1103,10 @@ def test_getsize():
     assert 7 == getsize(store)
     assert 5 == getsize(store, 'baz')
 
+    store = dict()
+    store['boo'] = None
+    assert -1 == getsize(store)
+
 
 def test_migrate_1to2():
     from zarr import meta_v1