Rebuild series ingest_locations on SyncFiles=false download (#302)

src/ndi/cloud/filehandler.py

@@ -54,7 +54,13 @@ def parse_ndic_uri(uri: str) -> tuple[str, str]:
     return parts[0], parts[1]
 
 
-def updateFileInfoForRemoteFiles(doc_props: dict, cloud_dataset_id: str) -> None:
+def updateFileInfoForRemoteFiles(
+    doc_props: dict,
+    cloud_dataset_id: str,
+    *,
+    custom_file_handler=None,
+    client: CloudClient | None = None,
+) -> None:
     """Rewrite a document's file_info locations to use ``ndic://`` URIs.
 
     MATLAB equivalent: ``ndi.cloud.sync.internal.updateFileInfoForRemoteFiles``
@@ -66,9 +72,39 @@ def updateFileInfoForRemoteFiles(doc_props: dict, cloud_dataset_id: str) -> None
     Handles both list-style and dict-style (MATLAB struct) ``file_info``
     and ``locations`` fields.
 
+    SERIES. When the document declares any file series with ``n_present > 0``
+    and empty ``ingest_locations``, the series' ``ingest_locations`` are
+    reconstructed from the downloaded manifest bytes -- fetched here on the
+    fly, either through *custom_file_handler* (the DID contract DID uses on
+    the read side, VH-Lab/DID-matlab#201 / VH-Lab/DID-python#88) or, when
+    no handler is given, through :func:`fetch_cloud_file`. Without this,
+    DID's ``MembersNotLocatable`` guard (DID-matlab#185) refuses the
+    document on the following ``add_docs`` -- the whole SyncFiles=false
+    path was blocked on that guard for any document carrying a populated
+    series. See VH-Lab/NDI-matlab#988.
+
+    The manifest bytes are dropped when the function returns, so no local
+    files persist -- the manifest is expected to land in the DID file cache
+    on the next member open, through DID#201's handler-fetch path. If the
+    cache is ever evicted, the same path re-fetches.
+
+    A per-manifest fetch failure is caught and logged; the entry is left
+    un-reconstructed so DID's guard fires on the following ``add_docs`` --
+    the right signal a partial download deserves.
+
     Args:
         doc_props: ndi_document properties dict (as from JSON).
         cloud_dataset_id: The cloud dataset ID to embed in URIs.
+        custom_file_handler: Optional callable following DID's
+            ``custom_file_handler`` contract (``(dest_path, source_path[,
+            context])``) used to fetch each qualifying series' manifest by
+            uid. When omitted, the manifest is fetched by minting a signed
+            URL against ``ndi.cloud.api.files``, matching what
+            ``download_file_from_cloud`` does when a manifest is asked for
+            by uid.
+        client: Authenticated cloud client for the direct-fetch path; falls
+            back to the ambient one. Ignored when *custom_file_handler* is
+            given.
     """
     files = doc_props.get("files")
     if not files or not isinstance(files, dict):
@@ -123,6 +159,198 @@ def updateFileInfoForRemoteFiles(doc_props: dict, cloud_dataset_id: str) -> None
     if was_dict:
         files["file_info"] = fi_list[0]
 
+    if _needs_series_reconstruction(files):
+        _reconstruct_series_from_cloud(
+            doc_props,
+            cloud_dataset_id,
+            custom_file_handler=custom_file_handler,
+            client=client,
+        )
+
+
+def _needs_series_reconstruction(files: dict) -> bool:
+    """True if any series in *files* has ``n_present > 0`` and no
+    ``ingest_locations``.
+
+    Same predicate :func:`reconstructSeriesIngestLocations` uses per entry,
+    hoisted here so an all-good document skips the whole scratch-dir dance.
+    """
+    series_info = _as_list(files.get("series_info"))
+    for entry in series_info:
+        if not isinstance(entry, dict):
+            continue
+        try:
+            n_present = int(entry.get("n_present", 0) or 0)
+        except (TypeError, ValueError):
+            continue
+        if n_present <= 0:
+            continue
+        if entry.get("ingest_locations"):
+            continue
+        return True
+    return False
+
+
+def _reconstruct_series_from_cloud(
+    doc_props: dict,
+    cloud_dataset_id: str,
+    *,
+    custom_file_handler=None,
+    client: CloudClient | None = None,
+) -> None:
+    """Fetch each qualifying series' manifest into a scratch dir and rebuild
+    its ``ingest_locations``.
+
+    Manifests do NOT land in the DID file cache here -- a subsequent member
+    open trips DID#201's lazy fetch, which is what populates the cache.
+    That is the design: the manifest is never a persistent local file the
+    caller depends on, and cache eviction on a later session is answered by
+    the same re-fetch path.
+
+    Deletes the scratch dir on exit. A per-manifest fetch failure is
+    logged; the entry is left un-reconstructed so DID's guard fires on
+    ``add_docs``.
+    """
+    import os
+    import tempfile
+
+    files = doc_props.get("files")
+    if not isinstance(files, dict):
+        return
+    file_info = _as_list(files.get("file_info"))
+    series_info = _as_list(files.get("series_info"))
+    document_id = ""
+    try:
+        base = doc_props.get("base")
+        if isinstance(base, dict):
+            document_id = str(base.get("id", "") or "")
+    except Exception:  # noqa: BLE001 - a malformed doc is not ours to fix
+        document_id = ""
+
+    with tempfile.TemporaryDirectory(prefix="ndi-manifest-fetch-") as tmp_dir:
+        for entry in series_info:
+            if not isinstance(entry, dict):
+                continue
+            try:
+                n_present = int(entry.get("n_present", 0) or 0)
+            except (TypeError, ValueError):
+                continue
+            if n_present <= 0:
+                continue
+            if entry.get("ingest_locations"):
+                continue
+
+            series_name = str(entry.get("name", "") or "")
+            manifest_uid = _manifest_uid(file_info, series_name)
+            if not manifest_uid:
+                continue
+
+            dest_path = os.path.join(tmp_dir, manifest_uid)
+            try:
+                _fetch_manifest(
+                    dest_path,
+                    cloud_dataset_id,
+                    manifest_uid,
+                    document_id,
+                    series_name,
+                    custom_file_handler=custom_file_handler,
+                    client=client,
+                )
+            except Exception as error:  # noqa: BLE001
+                # A manifest that cannot be fetched leaves the entry
+                # un-reconstructed; DID's #185 guard will then fire on
+                # add_docs with the document's own identity, which is the
+                # right signal a partial download deserves.
+                logger.warning(
+                    'Cannot fetch manifest for series "%s" (uid %s): %s',
+                    series_name,
+                    manifest_uid,
+                    error,
+                )
+
+        reconstructSeriesIngestLocations(doc_props, tmp_dir, cloud_dataset_id)
+
+
+def _fetch_manifest(
+    dest_path: str,
+    cloud_dataset_id: str,
+    manifest_uid: str,
+    document_id: str,
+    series_name: str,
+    *,
+    custom_file_handler=None,
+    client: CloudClient | None = None,
+) -> None:
+    """Fetch one manifest by uid to *dest_path*.
+
+    When *custom_file_handler* is given, dispatch through the DID contract
+    with ``seriesName=""`` in the context (a non-empty seriesName marks a
+    member fetch, where the handler treats ``ctx.uid`` as the file to
+    retrieve and ``source_path`` as the manifest -- the exact opposite of
+    what we want here). Otherwise, call :func:`fetch_cloud_file` directly,
+    matching what the read-side handler does when a manifest is asked for
+    by uid.
+    """
+    source_path = f"{NDIC_SCHEME}{cloud_dataset_id}/{manifest_uid}"
+    if custom_file_handler is not None:
+        context = {
+            "documentId": document_id,
+            "filename": series_name,
+            "seriesName": "",
+            "uid": manifest_uid,
+            "mode": "open",
+        }
+        _dispatch_custom_file_handler(custom_file_handler, dest_path, source_path, context)
+        return
+
+    fetch_cloud_file(
+        source_path,
+        dest_path,
+        client=client,
+        ndi_document_id=document_id,
+        series_name="",
+    )
+
+
+def _dispatch_custom_file_handler(handler, dest_path: str, source_path: str, context: dict) -> None:
+    """Arity-aware call of a ``custom_file_handler``.
+
+    A handler declared with three or more positional inputs (or with
+    ``*args``) is called with ``(dest_path, source_path, context)``. A
+    handler declared with two inputs is called without context, so an
+    older two-argument signature still works. Mirrors
+    ``did.implementations.sqlitedb.SqliteDb._dispatch_custom_file_handler``
+    and MATLAB's ``did.implementations.sqlitedb.dispatchCustomFileHandler``.
+    """
+    import inspect
+
+    try:
+        signature = inspect.signature(handler)
+    except (TypeError, ValueError):
+        # Builtin or C-implemented callable: fall back to positional try.
+        try:
+            handler(dest_path, source_path, context)
+            return
+        except TypeError:
+            handler(dest_path, source_path)
+            return
+
+    positional = 0
+    has_var_positional = False
+    for p in signature.parameters.values():
+        if p.kind in (
+            inspect.Parameter.POSITIONAL_ONLY,
+            inspect.Parameter.POSITIONAL_OR_KEYWORD,
+        ):
+            positional += 1
+        elif p.kind == inspect.Parameter.VAR_POSITIONAL:
+            has_var_positional = True
+
+    if has_var_positional or positional >= 3:
+        handler(dest_path, source_path, context)
+    else:
+        handler(dest_path, source_path)
+
 
 #: Installed by :func:`watchFetches`. Module-level rather than a parameter
 #: because DID fixes the file handler's signature -- it counts positional

src/ndi/cloud/ndi_matlab_python_bridge.yaml

@@ -999,7 +999,7 @@ functions:
   # --- filehandler.py functions ---
   - name: updateFileInfoForRemoteFiles
     matlab_path: "+ndi/+cloud/+sync/+internal/updateFileInfoForRemoteFiles.m"
-    matlab_last_sync_hash: "fe64a9f5"
+    matlab_last_sync_hash: "aea0c72c"
     python_path: "ndi/cloud/filehandler.py"
     input_arguments:
       - name: doc_props
@@ -1008,6 +1008,16 @@ functions:
       - name: cloud_dataset_id
         type_matlab: "char"
         type_python: "str"
+      - name: custom_file_handler
+        type_matlab: "function_handle (name-value)"
+        type_python: "callable | None"
+        default: "None"
+        keyword_only: true
+      - name: client
+        type_matlab: "n/a"
+        type_python: "CloudClient | None"
+        default: "None"
+        keyword_only: true
     output_arguments: []
     decision_log: >
       MATLAB places this in +sync/+internal. Python places it in
@@ -1019,22 +1029,29 @@ functions:
       and the per-series record survives on its own.
       SERIES ARE PORTED NOW (NDI-python#215; this entry used to say they
       were not). cloudDatasetId is the third argument on both sides, and
-      with it a downloaded series gets the two things it needs to be
-      readable: the manifest keeps the ndic:// reference it came from as a
-      second location beside the local copy (504c5bcf4, NDI-matlab#966),
-      so DID can hand that location to the file handler with a member's uid
-      in the context; and files.series_info(:).ingest_locations are rebuilt
-      from the downloaded manifest by reconstructSeriesIngestLocations
-      (cd3c11673, NDI-matlab#958), without which DID refuses the document
-      outright. The representation itself is DID's -- files.series_info,
-      file_series and n_present are read through did.document, not
-      reimplemented here, per AGENTS.md section 4.
-      Python builds the manifest's second location record directly rather
-      than through ndi.document.add_file, because this works on a properties
-      DICT: the whole download pipeline mutates dicts in place, as
-      updateFileInfoForRemoteFiles does, and converting to a document would
-      deep-copy and lose the mutation. The record shape is pinned to
-      add_file's by test_cloud_series_reconstruction.py.
+      with it files.series_info(:).ingest_locations are rebuilt from the
+      downloaded manifest by reconstructSeriesIngestLocations (cd3c11673,
+      NDI-matlab#958) -- without which DID refuses the document outright.
+      The representation itself is DID's -- files.series_info, file_series
+      and n_present are read through did.document, not reimplemented here,
+      per AGENTS.md section 4.
+      NDI-matlab#988 (1a6ec2fc, 414e8572, a4dfb076) then extended this to
+      the SyncFiles=false path: when a document returns from a cloud round
+      trip with n_present populated and ingest_locations stripped (which
+      DID does on store), the manifest bytes are NOT on disk to read.
+      Reconstruction here fetches each qualifying series' manifest through
+      the DID custom_file_handler contract (VH-Lab/DID-matlab#201,
+      VH-Lab/DID-python#88) -- the same contract DID uses on the read
+      side, so a caller with a handler already can pass it in and both
+      sides use the same one. With no handler, fetch_cloud_file is called
+      directly, matching what the read-side handler does when a manifest
+      is asked for by uid. Manifests fetched here do NOT land in the DID
+      file cache; a subsequent member open trips DID#201's lazy fetch,
+      which is what populates the cache. Cache eviction on a later
+      session is answered by the same re-fetch path. A per-manifest
+      fetch failure leaves the entry un-reconstructed so DID's #185
+      guard fires on add_docs with the document's own identity -- the
+      right signal a partial download deserves.
   - name: updateFileInfoForLocalFiles
     matlab_path: "+ndi/+cloud/+sync/+internal/updateFileInfoForLocalFiles.m"
     matlab_last_sync_hash: "504c5bcf4"

src/ndi/gui/ndi_matlab_python_bridge.yaml

@@ -838,7 +838,7 @@ ported_late:
 
   - name: GEFManager
     matlab_path: "+ndi/+gui/+app/GEFManager.m"
-    matlab_last_sync_hash: "06f31a4e5"
+    matlab_last_sync_hash: "59c96e57"
     status: porting_deferred
     decision_log: >
       MATLAB App Designer window that lists, views, adds and removes
@@ -875,3 +875,9 @@ ported_late:
       dialog with it. Nothing to port: the Python side draws no dialog at
       all. View builds a command line and runs napariViewGEF, which is
       already its own process and has never blocked anything.
+
+      NDI-matlab 59c96e57 (VH-Lab/NDI-matlab#984) raises the open GEF
+      Manager for a session instead of building a second window when the
+      navigator's Apps menu opens it again. MATLAB App Designer window
+      uniqueness only; nothing to port on this side, which draws no such
+      window in the first place. Hash bumped to acknowledge the drift.