ban slashes in keys where we can’t write

Author: flying-sheep

src/anndata/_io/h5ad.py

@@ -28,6 +28,7 @@
 from .specs import read_elem, write_elem
 from .specs.registry import IOSpec, write_spec
 from .utils import (
+    _check_has_no_slash_key,
     _read_legacy_raw,
     idx_chunks_along_axis,
     no_write_dataset_2d,
@@ -86,6 +87,8 @@ def write_h5ad(
         f.attrs.setdefault("encoding-type", "anndata")
         f.attrs.setdefault("encoding-version", "0.1.0")
         for k, elem in iter_outer(adata):
+            _check_has_no_slash_key(k, elem)
+
             if k == "raw":
                 _write_raw(
                     f, adata.raw, as_dense=as_dense, dataset_kwargs=dataset_kwargs

src/anndata/_io/specs/methods.py

@@ -20,7 +20,11 @@
 from anndata._core.index import _normalize_indices
 from anndata._core.merge import intersect_keys
 from anndata._core.sparse_dataset import _CSCDataset, _CSRDataset, sparse_dataset
-from anndata._io.utils import check_key, zero_dim_array_as_scalar
+from anndata._io.utils import (
+    _check_has_no_slash_key,
+    check_key,
+    zero_dim_array_as_scalar,
+)
 from anndata._warnings import OldFormatWarning
 from anndata.compat import (
     AwkArray,
@@ -287,19 +291,19 @@ def write_anndata(
 ):
     g = f.require_group(k)
     for sub_key, elem in iter_outer(adata):
-        if not (sub_key == "X" and elem is None):
-            if sub_key == "layers":
-                if None in elem:
-                    _writer.write_elem(
-                        g, "X", elem[None], dataset_kwargs=dataset_kwargs
-                    )
-                elem = {k: v for k, v in elem.items() if k is not None}
-            _writer.write_elem(
-                g,
-                sub_key,
-                dict(elem) if isinstance(elem, MutableMapping) else elem,
-                dataset_kwargs=dataset_kwargs,
-            )
+        if sub_key == "X" and elem is None:
+            continue
+        _check_has_no_slash_key(sub_key, elem)
+        if sub_key == "layers":
+            if None in elem:
+                _writer.write_elem(g, "X", elem[None], dataset_kwargs=dataset_kwargs)
+            elem = {k: v for k, v in elem.items() if k is not None}
+        _writer.write_elem(
+            g,
+            sub_key,
+            dict(elem) if isinstance(elem, MutableMapping) else elem,
+            dataset_kwargs=dataset_kwargs,
+        )
 
 
 @_REGISTRY.register_read(H5Group, IOSpec("anndata", "0.1.0"))

src/anndata/_io/specs/registry.py

@@ -368,8 +368,10 @@ def write_elem(
         if k.startswith(store.name) and k != "/":
             k = str(PurePosixPath(k).relative_to(store.name))
 
+        # Apart from this code, we also ban keys containing slashes in `write_adata`/`write_h5ad`
+        # for AnnData elements other than `obs`, `var`, and `uns`.
         if "/" in k and k != "/":
-            if settings.disallow_forward_slash_in_h5ad:
+            if isinstance(store, ZarrGroup) or settings.disallow_forward_slash_in_h5ad:
                 msg = f"Forward slashes are not allowed in keys in {type(store)}"
                 raise ValueError(msg)
             msg = "Forward slashes will be written differently in a future anndata version"

src/anndata/_io/utils.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from collections.abc import Callable
+from collections.abc import Callable, Mapping
 from functools import WRAPPER_ASSIGNMENTS, cache, wraps
 from itertools import pairwise
 from typing import TYPE_CHECKING, Literal, cast
@@ -12,7 +12,7 @@
 from ..utils import warn
 
 if TYPE_CHECKING:
-    from collections.abc import Callable, Mapping
+    from collections.abc import Callable
     from typing import Any, Literal
 
     from pandas.core.dtypes.dtypes import BaseMaskedDtype
@@ -279,6 +279,16 @@ def func_wrapper(*args, **kwargs):
     return func_wrapper
 
 
+def _check_has_no_slash_key(attr: str, elem: object) -> None:
+    """Only attempt to write slash keys where people rely on it for backwards compatibility."""
+    if attr in {"obs", "var", "uns", "raw"}:
+        return  # separate check for `settings.disallow_forward_slash_in_h5ad` is done in `write_elem`
+    assert isinstance(elem, Mapping)
+    if any("/" in k for k in elem if k not in {"/", None}):
+        msg = f"Forward slashes are not allowed in keys in {attr}"
+        raise ValueError(msg)
+
+
 # -------------------------------------------------------------------------------
 # Common h5ad/zarr stuff
 # -------------------------------------------------------------------------------

tests/test_readwrite.py

@@ -963,7 +963,9 @@ def test_h5py_attr_limit(tmp_path):
     "elem_key", ["obs", "var", "obsm", "varm", "layers", "obsp", "varp", "uns"]
 )
 @pytest.mark.parametrize("store_type", ["zarr", "h5ad"])
-@pytest.mark.parametrize("disallow_forward_slash_in_h5ad", [True, False])
+@pytest.mark.parametrize(
+    "disallow_forward_slash_in_h5ad", [True, False], ids=["ban_slash", "allow_slash"]
+)
 def test_forward_slash_key(
     elem_key: Literal["obs", "var", "obsm", "varm", "layers", "obsp", "varp", "uns"],
     tmp_path: Path,
@@ -976,25 +978,34 @@ def test_forward_slash_key(
         (10,) if elem_key in ["obs", "var"] else (10, 10)
     )
     path = tmp_path / f"test.{store_type}"
+    can_write_slash_key = (
+        elem_key in {"uns", "obs", "var"}
+        and store_type == "h5ad"
+        and not disallow_forward_slash_in_h5ad
+    )
 
+    # try to write bad key and make sure we warn or throw an error
     with (
         ad.settings.override(
             disallow_forward_slash_in_h5ad=disallow_forward_slash_in_h5ad
         ),
-        pytest.raises(ValueError, match=r"Forward slashes")
-        if disallow_forward_slash_in_h5ad
-        else pytest.warns(FutureWarning, match=r"Forward slashes"),
+        pytest.warns(FutureWarning, match=r"Forward slashes")
+        if can_write_slash_key
+        else pytest.raises(ValueError, match=r"Forward slashes"),
     ):
         getattr(a, f"write_{store_type}")(path)
 
-    if not disallow_forward_slash_in_h5ad:
-        adata = getattr(ad, f"read_{store_type}")(path)
+    # read and check that bad keys were only written if allowed
+    elem = getattr(getattr(ad, f"read_{store_type}")(path), elem_key)
+    if can_write_slash_key:
         if elem_key in {"obs", "var"}:
-            assert "bad/key" in getattr(adata, elem_key)
-        elif elem_key == "uns":
-            assert "bad" in getattr(adata, elem_key)
+            assert "bad/key" in elem
         else:
-            assert not getattr(adata, elem_key).keys()
+            assert "bad" in elem
+    elif elem_key in {"obs", "var"}:
+        assert set(elem.columns) == {"_index"}
+    else:
+        assert not elem.keys()
 
 
 @pytest.mark.skipif(