Do not recurse when getting the metadata of an NWB file in a BIDS dataset

dandi/files/bids.py

@@ -248,6 +248,18 @@ def get_metadata(
         digest: Digest | None = None,
         ignore_errors: bool = True,
     ) -> BareAsset:
+        return self.get_bids_metadata(digest)
+
+    def get_bids_metadata(self, digest: Digest | None = None) -> BareAsset:
+        """
+        Return the metadata of the asset that comes from the BIDS dataset (its
+        path and files such as :file:`participants.tsv`) only.
+
+        Unlike `get_metadata()`, which subclasses override to also extract
+        metadata from the content of the asset (e.g., `NWBBIDSAsset` from the
+        NWB file), this never reads the content, so that the extraction of
+        metadata from the content can use it without recursing.
+        """
         metadata = self.bids_dataset_description.get_asset_metadata(self)
         start_time = end_time = datetime.now().astimezone()
         add_common_metadata(metadata, self.filepath, start_time, end_time, digest)
@@ -283,7 +295,7 @@ def get_metadata(
         digest: Digest | None = None,
         ignore_errors: bool = True,
     ) -> BareAsset:
-        bids_metadata = BIDSAsset.get_metadata(self, digest, ignore_errors)
+        bids_metadata = self.get_bids_metadata(digest)
         nwb_metadata = NWBAsset.get_metadata(self, digest, ignore_errors)
         return BareAsset(
             **{
@@ -318,10 +330,7 @@ def get_metadata(
         digest: Digest | None = None,
         ignore_errors: bool = True,
     ) -> BareAsset:
-        metadata = self.bids_dataset_description.get_asset_metadata(self)
-        start_time = end_time = datetime.now().astimezone()
-        add_common_metadata(metadata, self.filepath, start_time, end_time, digest)
-        metadata.path = self.path
+        metadata = self.get_bids_metadata(digest)
         metadata.encodingFormat = ZARR_MIME_TYPE
         return metadata
 

dandi/metadata/nwb.py

@@ -69,7 +69,9 @@ def get_metadata(
             assert isinstance(df, bids.BIDSAsset)
             if not digest:
                 digest = DUMMY_DANDI_ETAG
-            path_metadata = df.get_metadata(digest=digest)
+            # Not `df.get_metadata()`, which for an NWB file would extract
+            # its NWB metadata, i.e., call this function again
+            path_metadata = df.get_bids_metadata(digest=digest)
             meta["bids_version"] = df.get_validation_bids_version()
             # there might be a more elegant way to do this:
             if path_metadata.wasAttributedTo:

dandi/tests/test_metadata.py

@@ -36,14 +36,17 @@
 from pynwb import NWBHDF5IO, NWBFile, TimeSeries
 from pynwb.file import Subject
 import pytest
+from pytest_mock import MockerFixture
 import requests
 from semantic_version import Version
 
 from .fixtures import SampleDandiset
 from .skip import mark
 from .. import __version__
-from ..consts import metadata_nwb_subject_fields
+from ..consts import dandiset_metadata_file, metadata_nwb_subject_fields
 from ..dandiapi import RemoteBlobAsset
+from ..files import NWBBIDSAsset, dandi_file, find_bids_dataset_description
+from ..metadata import nwb as metadata_nwb
 from ..metadata.core import prepare_metadata
 from ..metadata.nwb import get_metadata, nwb2asset
 from ..metadata.util import (
@@ -124,6 +127,59 @@ def test_bids_nwb_metadata_integration(bids_examples: Path, tmp_path: Path) -> N
     assert metadata["sex"] == "U"
 
 
+@pytest.mark.ai_generated
+def test_bids_nwb_metadata_no_recursion(
+    bids_examples: Path, tmp_path: Path, mocker: MockerFixture
+) -> None:
+    # Getting the BIDS metadata of an NWB file in a BIDS dataset must not
+    # extract its NWB metadata again (which used to recurse until
+    # RecursionError, running the BIDS validator at every level)
+    dpath = tmp_path / "ieeg_epilepsyNWB"
+    shutil.copytree(bids_examples / "ieeg_epilepsyNWB", dpath)
+    file_path = (
+        dpath
+        / "sub-01"
+        / "ses-postimp"
+        / "ieeg"
+        / "sub-01_ses-postimp_task-seizure_run-01_ieeg.nwb"
+    )
+    spy = mocker.spy(metadata_nwb, "nwb2asset")
+    metadata = get_metadata.__wrapped__(file_path)
+    assert spy.call_count == 0
+    assert metadata["subject_id"] == "01"
+    assert metadata["sex"] == "U"
+
+
+@pytest.mark.ai_generated
+def test_nwb_bids_asset_metadata_no_recursion(
+    bids_examples: Path, tmp_path: Path, mocker: MockerFixture
+) -> None:
+    # The metadata of an NWBBIDSAsset merges its BIDS and NWB metadata,
+    # extracting the latter (which itself includes BIDS metadata) only once
+    dpath = tmp_path / "ieeg_epilepsyNWB"
+    shutil.copytree(bids_examples / "ieeg_epilepsyNWB", dpath)
+    (dpath / dandiset_metadata_file).write_text("identifier: '000027'\n")
+    file_path = (
+        dpath
+        / "sub-01"
+        / "ses-postimp"
+        / "ieeg"
+        / "sub-01_ses-postimp_task-seizure_run-01_ieeg.nwb"
+    )
+    # (The asset refers to it only weakly)
+    bids_dataset_description = find_bids_dataset_description(file_path)
+    df = dandi_file(file_path, dpath, bids_dataset_description=bids_dataset_description)
+    assert isinstance(df, NWBBIDSAsset)
+    spy = mocker.spy(metadata_nwb, "nwb2asset")
+    metadata = df.get_metadata(digest=DUMMY_DANDI_ETAG, ignore_errors=False)
+    assert spy.call_count == 1
+    assert metadata.wasAttributedTo is not None
+    participant = metadata.wasAttributedTo[0]
+    assert participant.identifier == "01"
+    assert participant.sex is not None
+    assert participant.sex.name == "Unknown"
+
+
 @pytest.mark.parametrize(
     "age,duration",
     [