BUG: Fix empty Group nodes lacking a "consolidated_metadata" field (zarr-developers#3967)

* BUG: Fix empty Group nodes lacking a "consolidated_metadata" field

All nodes opened via `open_consolidated` are expected to have a non-null `.metadata.consolidated_metadata`. zarr-developers#3954 showed an example, with a data file written by zarr-python 2.x, where that condition failed to hold.

This happened because of a slight difference between how zarr-python 3.x writes consolidated metadata and how zarr-python 2.x wrote it. For leaf `Group` nodes (a `Group` with no `Group` or `Array` children), zarr-python 3.x includes an "empty" `consolidated_metadata` field.  In zarr-python 3.x, we include a `consolidated_metadata: metadata: {}` field to indicate that it's empty. zarr-python 2.x *didn't* include that.

The fix is to update how we load this type of data. We effectively normalize a node missing consolidated metadata to one that is known not to have consolidated metadata (which is consistent with the idea behind consolidated metadata: just do one metadata read up front).

Author: TomAugspurger

changes/3954.bugfix.md

@@ -0,0 +1 @@
+Handle missing consolidated metadata in leaf Group nodes.

src/zarr/core/group.py

@@ -235,8 +235,6 @@ def _flat_to_nested(
         # array metadata of its immediate children.
         # In the example, the group at `/a/b` will have consolidated metadata
         # for its children `array-0` and `array-1`.
-        #
-        # metadata = dict(metadata)
 
         keys = sorted(metadata, key=lambda k: k.count("/"))
         grouped = {
@@ -269,13 +267,17 @@ def _flat_to_nested(
                 # These are already present, either thanks to being an array in the
                 # root, or by being collected as a child in the else clause
                 continue
-            children_keys = list(children_keys)
-            # We pop from metadata, since we're *moving* this under group
-            children = {
-                child_key.split("/")[-1]: metadata.pop(child_key)
-                for child_key in children_keys
-                if child_key != key
-            }
+            children: dict[str, ArrayV2Metadata | ArrayV3Metadata | GroupMetadata] = {}
+            # We pop from metadata, since we're *moving* this under group.
+            # While doing this, normalize leaf groups to carry empty consolidated metadata.
+            for child_key in children_keys:
+                if child_key == key:
+                    continue
+                child = metadata.pop(child_key)
+                if isinstance(child, GroupMetadata) and child.consolidated_metadata is None:
+                    child = replace(child, consolidated_metadata=ConsolidatedMetadata(metadata={}))
+                children[child_key.split("/")[-1]] = child
+
             parent[name] = replace(
                 node, consolidated_metadata=ConsolidatedMetadata(metadata=children)
             )

tests/test_metadata/test_consolidated.py

@@ -51,6 +51,74 @@ async def memory_store_with_hierarchy(memory_store: Store) -> Store:
 
 
 class TestConsolidated:
+    @pytest.mark.filterwarnings("ignore:Consolidated metadata")
+    async def test_getitem_consolidated_empty_leaf_group(
+        self, memory_store: zarr.storage.MemoryStore, zarr_format: ZarrFormat
+    ) -> None:
+        # This test writes the bytes directly, rather than using the zarr API, to mimic
+        # how older versions of zarr-python wrote the consolidated metadata.
+        # Notably, zarr-python 2.x does not include a
+        #
+        #     "consolidated_metadata": {"metadata": {}}
+        #
+        # field on the leaf group nodes.
+        if zarr_format == 2:
+            zmetadata: dict[str, JSON] = {
+                "metadata": {
+                    ".zattrs": {},
+                    ".zgroup": {"zarr_format": 2},
+                    "raw/.zattrs": {},
+                    "raw/.zgroup": {"zarr_format": 2},
+                    "raw/varm/.zattrs": {},
+                    "raw/varm/.zgroup": {"zarr_format": 2},
+                },
+                "zarr_consolidated_format": 1,
+            }
+            await memory_store.set(
+                ".zgroup", cpu.Buffer.from_bytes(json.dumps({"zarr_format": 2}).encode())
+            )
+            await memory_store.set(".zattrs", cpu.Buffer.from_bytes(json.dumps({}).encode()))
+            await memory_store.set(
+                ".zmetadata", cpu.Buffer.from_bytes(json.dumps(zmetadata).encode())
+            )
+
+        else:
+            zmetadata = {
+                "attributes": {},
+                "zarr_format": 3,
+                "consolidated_metadata": {
+                    "kind": "inline",
+                    "must_understand": False,
+                    "metadata": {
+                        "raw": {
+                            "attributes": {},
+                            "zarr_format": 3,
+                            "node_type": "group",
+                        },
+                        "raw/varm": {
+                            "attributes": {},
+                            "zarr_format": 3,
+                            "node_type": "group",
+                        },
+                    },
+                },
+                "node_type": "group",
+            }
+            await memory_store.set(
+                "zarr.json", cpu.Buffer.from_bytes(json.dumps(zmetadata).encode())
+            )
+
+        group = await zarr.api.asynchronous.open_consolidated(
+            store=memory_store, zarr_format=zarr_format
+        )
+        raw = await group.getitem("raw")
+        assert isinstance(raw, zarr.AsyncGroup)
+        assert raw.metadata.consolidated_metadata is not None
+
+        varm = await raw.getitem("varm")
+        assert isinstance(varm, zarr.AsyncGroup)
+        assert varm.metadata.consolidated_metadata == ConsolidatedMetadata(metadata={})
+
     async def test_open_consolidated_false_raises(self) -> None:
         store = zarr.storage.MemoryStore()
         with pytest.raises(TypeError, match="use_consolidated"):