From 449889bd87d8289a1f0222ef96bba2b3688dbef3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 13:41:04 +0000 Subject: [PATCH 1/2] Fetch a remote-only series manifest through the customFileHandler (#87) A series document that arrived from a store keeping bytes remote -- a SyncFiles=false round trip through NDI Cloud, say -- lands with its manifest's file_info naming an ndic:// address and no manifest bytes on this machine. _open_series_member's local-only manifest lookup then raised DID:SQLITEDB:FileSeries:ManifestNotLocal without ever offering the manifest's location to the handler, even when the caller had passed one. Port DID-matlab's fix (VH-Lab/DID-matlab#201, PR #202): add SQLiteDB._fetch_series_manifest_bytes -- one level up from the existing _fetch_remote_to_cache with the same call shape. It reads the manifest's file_info straight from the document, offers every non-'file' location to the custom_file_handler with the manifest's uid in the context (seriesName='', mode='open'), and lands the bytes at filecachepath/ under the same single-flight lock the member fetch already holds. Success means the next cached_path_for_uid is a hit, whether the next call is this member, another member of the same series, or a later session. _open_series_member's fast local path is unchanged; the fetch only fires when the manifest is not on disk AND a custom_file_handler was supplied, so exist_doc and the no-handler cases keep their old shape. When the handler cannot service the manifest's location, the existing DID:SQLITEDB:FileSeries:ManifestNotLocal identifier and message are what gets raised, so callers that pattern-match on it keep working. _series_manifest_path stays no-network -- the promise that lets cached_path_for_file resolve files from any thread and any process without holding the database session. The lazy-fetch path lives on the sqlite implementation, where the handler already does. Companion port of VH-Lab/DID-matlab#202. Together they unblock the NDI-matlab workaround in ndi.cloud.sync.internal.updateFileInfoForLocalFiles (which installs a second location on the manifest so the handler has a non-'file' path to resolve) and will let it retire in a follow-up. --- src/did/database.py | 11 ++ src/did/implementations/sqlitedb.py | 87 ++++++++++++++ tests/test_open_doc_file_series.py | 169 ++++++++++++++++++++++++++++ 3 files changed, 267 insertions(+) diff --git a/src/did/database.py b/src/did/database.py index d5389e1..22abcb4 100644 --- a/src/did/database.py +++ b/src/did/database.py @@ -612,6 +612,17 @@ def _series_manifest_path(document_obj, name, additional_roots): file_info and finding it needs nothing but the document and the filesystem. Shared by the member lookup and the series accessors so they all agree on which copy is authoritative. + + REMOTE-ONLY MANIFESTS. This helper is deliberately no-network: a + manifest whose bytes live only in the cloud resolves to ``None`` + here, and it is up to the retrieval-authorized caller to fetch. That + path is + :meth:`did.implementations.sqlitedb.SQLiteDB._fetch_series_manifest_bytes`, + which offers the manifest's ``file_info`` location to the + ``custom_file_handler`` and lands the bytes at + ``filecachepath/`` -- so the next call to this function + is a :func:`did.file.cached_path_for_uid` hit, with no network again. + See VH-Lab/DID-matlab#201 and VH-Lab/DID-python#87. """ from .file import cached_path_for_uid diff --git a/src/did/implementations/sqlitedb.py b/src/did/implementations/sqlitedb.py index a4a8da0..336f1b9 100644 --- a/src/did/implementations/sqlitedb.py +++ b/src/did/implementations/sqlitedb.py @@ -1695,6 +1695,20 @@ def _open_series_member(self, series_locations, custom_file_handler): additional_roots = self._do_cached_path_roots() manifest_path = _series_manifest_path(doc, stem, additional_roots) + if manifest_path is None and custom_file_handler is not None: + # The manifest bytes are not on this machine, but a handler was + # supplied. Offer the manifest's own file_info location(s) to + # it (mode='open', ctx.uid=, ctx.seriesName='') -- + # the same call shape do_open_doc uses for a non-'file' file + # -- so a document that arrived from a store which keeps bytes + # remote (SyncFiles=false through NDI Cloud, say) can still be + # resolved. Success caches at filecachepath/, so + # the next cached_path_for_uid is a hit and a second member + # open on the same series pays no network for the manifest + # again. See VH-Lab/DID-matlab#201 and VH-Lab/DID-python#87. + manifest_path = self._fetch_series_manifest_bytes( + doc, stem, custom_file_handler + ) if manifest_path is None: # We cannot open the manifest, so we cannot know what member the # series records. This is the same "not here" cached_path_for_file @@ -1863,6 +1877,79 @@ def _discard_fetched(self, fetched_path, uid): with contextlib.suppress(OSError): os.remove(fetched_path) + def _fetch_series_manifest_bytes(self, document_obj, series_name, custom_file_handler): + """Fetch a series' manifest via ``custom_file_handler``. + + ONE LEVEL UP from :meth:`_fetch_remote_to_cache` for a member: the + manifest is an ordinary file of the document, so its ``file_info`` + entry names its location(s) directly -- no SQL, no recursion into + ``open_doc``. Every non-``'file'`` location is offered to the + handler with the manifest's own uid in the context and + ``mode='open'``, in the same call shape ``open_doc`` uses for a + non-``'file'`` file_info location on the read path. Success puts + the bytes at ``filecachepath/`` so the next + :func:`cached_path_for_uid` is a hit, whether the next call is + this member, another member, or a later session. + + The no-network primitive :func:`did.database._series_manifest_path` + must stay callable from any thread and any process, so teaching it + to fetch would break that. The lazy-fetch path lives here on the + sqlite implementation, where the handler already lives, and the + local-first fast path in :meth:`_open_series_member` keeps the + second open of a series one :func:`cached_path_for_uid` hit. + + Returns the local path on success, or ``None`` on any failure -- + the caller reports the miss in its own words. See + VH-Lab/DID-matlab#201 and VH-Lab/DID-python#87. + """ + from ..common import PathConstants + + if custom_file_handler is None or document_obj is None: + return None + + is_in, info, _ = document_obj.is_in_file_list(series_name) + if not is_in or not isinstance(info, dict): + return None + locations = info.get("locations") + if isinstance(locations, dict): + locations = [locations] + if not isinstance(locations, list): + return None + + temp_dir = PathConstants().temppath + for entry in locations: + if not isinstance(entry, dict): + continue + location = entry.get("location") + uid = entry.get("uid") + location_type = entry.get("location_type") + if not location or not uid: + continue + # Only non-'file' locations are offered. cached_path_for_uid has + # already checked filecachepath/ and FileDir/, and a + # manifest orig_location that points elsewhere is not a resolution + # rule DID promises. + if not self._is_remote_location(location, location_type): + continue + + fetched_path, _error = self._fetch_remote_to_cache( + uid, + location, + temp_dir, + custom_file_handler, + context={ + "documentId": document_obj.id(), + "filename": str(series_name), + "seriesName": "", + "uid": uid, + "mode": "open", + }, + ) + if fetched_path is not None and os.path.isfile(fetched_path): + return fetched_path + + return None + def _manifest_locations_for_handler(self, document_obj, stem): """Locations the handler should be offered to fetch a series member. diff --git a/tests/test_open_doc_file_series.py b/tests/test_open_doc_file_series.py index 6c9ebef..cad16d6 100644 --- a/tests/test_open_doc_file_series.py +++ b/tests/test_open_doc_file_series.py @@ -717,5 +717,174 @@ def test_a_name_the_document_has_no_manifest_for_offers_nothing(self): self.assertEqual(self.db._manifest_locations_for_handler(doc, "nosuch.bin"), []) +class TestFetchingAnAbsentManifest(SeriesReadTestCase): + """VH-Lab/DID-matlab#201 / VH-Lab/DID-python#87. A series document + that arrived from a store keeping bytes remote (SyncFiles=false through + NDI Cloud, say) lands with its manifest's ``file_info`` naming an + ``ndic://`` address and no manifest bytes on this machine. Opening a + member offers the manifest's location to the ``custom_file_handler`` + with the manifest's own uid in the context, caches the result at + ``filecachepath/``, and proceeds as normal -- so a second + member open pays no network for the manifest again. + """ + + CLOUD_MANIFEST_URI = "ndic://demo-dataset/manifest-uid" + + def stored_series_cloud_only(self, count=2): + """SyncFiles=false shape: the manifest's only recorded location is + ``ndic://`` with ``ingest=0``, so add_docs never installs the + manifest bytes locally. Members are ingested normally, so the + interesting miss is the manifest itself. + + Records ``self.uid_bytes``: a uid-keyed map the handler serves + from, so a single fixture-provided handler answers for the + manifest (by its uid) without ever needing to know the scheme. + """ + locations = self._write_members(count) + doc = Document("demoSeries", **{"demoSeries.value": 1}) + doc.add_file_series(SERIES, locations, delete_original=0) + + _is_in, info, _ = doc.is_in_file_list(SERIES) + with open(info["locations"][0]["location"], "rb") as handle: + manifest_bytes = handle.read() + manifest_uid = info["locations"][0]["uid"] + + info["locations"] = [ + { + "location": self.CLOUD_MANIFEST_URI, + "location_type": "ndicloud", + "uid": manifest_uid, + "ingest": 0, + "delete_original": 0, + "parameters": "", + } + ] + + self.member_uids = [e["uid"] for e in doc.series_ingest_locations(SERIES)] + self.manifest_uid = manifest_uid + self.manifest_bytes = manifest_bytes + + self.uid_bytes = {manifest_uid: manifest_bytes} + for path, uid in zip(locations, self.member_uids): + with open(path, "rb") as handle: + self.uid_bytes[uid] = handle.read() + + # No handler is needed at add-docs time because ingest=0 on the + # only location: add_docs writes the files-table row and installs + # no bytes. + self.db.add_docs([doc], validate=False) + return self.db.get_docs(doc.id()) + + def _uid_keyed_handler(self): + """A handler that serves whatever bytes ``self.uid_bytes`` records + for the uid in the context, and records each call for assertion. + """ + calls = [] + + def handler(dest_path, source_path, context): + calls.append((dest_path, source_path, dict(context))) + uid = context.get("uid") + if uid in self.uid_bytes: + with open(dest_path, "wb") as handle: + handle.write(self.uid_bytes[uid]) + + return handler, calls + + def test_a_cloud_only_manifest_is_retrieved_through_the_handler(self): + """The acceptance case for #201: opening a member calls the handler + for the manifest first (uid = manifest's own, seriesName = ''), + then reads the member. The member bytes come back byte-for-byte.""" + doc = self.stored_series_cloud_only(count=3) + handler, calls = self._uid_keyed_handler() + + file_obj = self.db.open_doc( + doc.id(), f"{SERIES}_2", custom_file_handler=handler + ) + self.assertEqual(self.read(file_obj), b"member-1-bytes") + + self.assertGreaterEqual(len(calls), 1, "the handler was never asked") + _dest, source_path, context = calls[0] + self.assertEqual( + context["uid"], self.manifest_uid, "ctx.uid names the manifest" + ) + self.assertEqual( + context["seriesName"], "", "the manifest is a file, not a member" + ) + self.assertEqual(context["mode"], "open") + self.assertEqual( + source_path, + self.CLOUD_MANIFEST_URI, + "sourcePath is the manifest's ndic:// location, verbatim", + ) + + def test_the_manifest_lands_at_filecachepath_after_the_fetch(self): + """The next cached_path_for_uid must be a hit: that is what keeps a + second series read from re-fetching the manifest.""" + doc = self.stored_series_cloud_only(count=2) + handler, _calls = self._uid_keyed_handler() + + self.db.open_doc(doc.id(), f"{SERIES}_1", custom_file_handler=handler) + + cached = cached_path_for_uid(self.manifest_uid) + self.assertIsNotNone( + cached, "the manifest was not cached at filecachepath/" + ) + self.assertTrue(os.path.isfile(cached)) + + def test_different_members_share_one_manifest_fetch(self): + """The rule that keeps a 28,000-member level from turning into + 28,000 manifest downloads: the manifest is fetched once per + session, not once per member open.""" + doc = self.stored_series_cloud_only(count=3) + handler, calls = self._uid_keyed_handler() + + for index in (1, 2, 3): + self.db.open_doc( + doc.id(), f"{SERIES}_{index}", custom_file_handler=handler + ) + + manifest_calls = [ + c for c in calls if c[2].get("uid") == self.manifest_uid + ] + self.assertEqual( + len(manifest_calls), + 1, + "the manifest must be fetched exactly once across three member opens", + ) + + def test_a_handler_that_serves_nothing_raises_manifest_not_local(self): + """Handler-refused case for the manifest. Without an installed + manifest and with a handler that produces nothing, the error is + the same DID:SQLITEDB:FileSeries:ManifestNotLocal callers already + match on -- so an out-of-band failure stays visible.""" + doc = self.stored_series_cloud_only(count=2) + + def handler(dest_path, source_path, context): + return None + + with self.assertRaises(FileAccessError) as caught: + self.db.open_doc( + doc.id(), f"{SERIES}_1", custom_file_handler=handler + ) + self.assertEqual( + caught.exception.identifier, + "DID:SQLITEDB:FileSeries:ManifestNotLocal", + ) + self.assertIn(SERIES, str(caught.exception)) + + def test_no_handler_still_raises_manifest_not_local(self): + """The no-handler case is unchanged: DID retrieves nothing itself, + so a manifest that is not on this machine cannot be resolved and + the caller is told which series is missing.""" + doc = self.stored_series_cloud_only(count=2) + + with self.assertRaises(FileAccessError) as caught: + self.db.open_doc(doc.id(), f"{SERIES}_1") + self.assertEqual( + caught.exception.identifier, + "DID:SQLITEDB:FileSeries:ManifestNotLocal", + ) + + if __name__ == "__main__": unittest.main() From 0068412053e30a070474b27447c2c190b5102699 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 13:45:42 +0000 Subject: [PATCH 2/2] Reformat with black and bump matlab_last_sync_hash through PR #202 CI's lint job (black --check) reformatted src/did/implementations/sqlitedb.py and tests/test_open_doc_file_series.py; no code change, only line breaks. CI's Bridge coverage vs DID-matlab reported drift on database and sqlitedb because DID-matlab#202 landed the port this PR mirrors. Bump both entries' matlab_last_sync_hash to 8bea92c (the merge) and record in sync_notes what was ported. This is the "READ THE DIFF AND PORT THE CHANGE, then set matlab_last_sync_hash to the commit you examined" step -- the diff is the one this PR itself carries, so the notes name it directly. No behavior change from this commit; the port itself is in 449889b. --- src/did/did_matlab_python_bridge.yaml | 13 ++++++++- ..._matlab_python_bridge_implementations.yaml | 27 ++++++++++++++++++- src/did/implementations/sqlitedb.py | 4 ++- tests/test_open_doc_file_series.py | 12 +++------ 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/src/did/did_matlab_python_bridge.yaml b/src/did/did_matlab_python_bridge.yaml index dce371b..5e2e58a 100644 --- a/src/did/did_matlab_python_bridge.yaml +++ b/src/did/did_matlab_python_bridge.yaml @@ -40,7 +40,7 @@ classes: - name: database type: class matlab_path: "+did/database.m" - matlab_last_sync_hash: "9b36124" + matlab_last_sync_hash: "8bea92c" status: ported python_path: "did/database.py" python_class: "Database" @@ -95,6 +95,17 @@ classes: declared default; the old space-separated, timezone-less form was read as LOCAL time by a JS consumer's new Date(). + 2026-09-14: bumped through DID-matlab 8bea92c (PR #202 / DID-matlab#201). + localSeriesManifestPath is the no-SQL/no-network primitive + cached_path_for_file rests on, which is why it stays local-only on + both sides: the lazy-fetch path for a remote manifest lives on the + sqlite implementation. MATLAB's docstring gained a paragraph naming + that path (fetchSeriesManifestBytes) and the identifier callers + match on (DID:SQLITEDB:FileSeries:ManifestNotLocal); the Python + analogue _series_manifest_path was updated in the same shape. No + behavior change here -- see the sqlitedb entry in + did_matlab_python_bridge_implementations.yaml for the fetch itself. + properties: - name: connection python_name: connection diff --git a/src/did/did_matlab_python_bridge_implementations.yaml b/src/did/did_matlab_python_bridge_implementations.yaml index 1bb02d8..282a03c 100644 --- a/src/did/did_matlab_python_bridge_implementations.yaml +++ b/src/did/did_matlab_python_bridge_implementations.yaml @@ -29,7 +29,7 @@ classes: - name: sqlitedb type: class matlab_path: "+did/+implementations/sqlitedb.m" - matlab_last_sync_hash: "717fbad" + matlab_last_sync_hash: "8bea92c" status: ported python_path: "did/implementations/sqlitedb.py" python_class: "SQLiteDB" @@ -108,6 +108,31 @@ classes: reclaim, so this is a third divergence, recorded on a new do_delete_branch entry below. + 2026-09-14: bumped through DID-matlab 8bea92c (PR #202 / DID-matlab#201). + seriesMemberPath used to fall through to do_open_doc(document_id, stem) + when the manifest was not on disk, which cost an SQL round trip and + recursed the whole read path just to invoke the same handler. MATLAB + replaced that with a dedicated fetchSeriesManifestBytes helper: it + reads the manifest's file_info straight from the document (no SQL), + offers every non-'file' location to the customFileHandler with the + manifest's uid in the context (seriesName='', mode='open'), and lands + the bytes at filecachepath/. The Python side ports the + same shape: SQLiteDB._fetch_series_manifest_bytes reuses the existing + _fetch_remote_to_cache primitive (same lock, same cache path), and + _open_series_member calls it only on a miss and only when a handler + was supplied -- so the fast local path and exist_doc's no-network + answer are unchanged, and the DID:SQLITEDB:FileSeries:ManifestNotLocal + identifier callers pattern-match on is still what gets raised when + the handler cannot service the location. MATLAB's second commit + (b4491ba) additionally set memberOf=stem when the manifest could not + be resolved, so do_open_doc raises "member of file series ... not on + this machine" rather than "does not include a file named"; Python's + _open_series_member already raises ManifestNotLocal by identifier, + so there is no message change to port. Coverage: + tests/test_open_doc_file_series.py:TestFetchingAnAbsentManifest (5 + new tests, including the acceptance case, the "one fetch shared + across members" cache guarantee, and the handler-refused failure). + properties: - name: FileDir python_name: ~ # not ported diff --git a/src/did/implementations/sqlitedb.py b/src/did/implementations/sqlitedb.py index 336f1b9..ce4545f 100644 --- a/src/did/implementations/sqlitedb.py +++ b/src/did/implementations/sqlitedb.py @@ -1877,7 +1877,9 @@ def _discard_fetched(self, fetched_path, uid): with contextlib.suppress(OSError): os.remove(fetched_path) - def _fetch_series_manifest_bytes(self, document_obj, series_name, custom_file_handler): + def _fetch_series_manifest_bytes( + self, document_obj, series_name, custom_file_handler + ): """Fetch a series' manifest via ``custom_file_handler``. ONE LEVEL UP from :meth:`_fetch_remote_to_cache` for a member: the diff --git a/tests/test_open_doc_file_series.py b/tests/test_open_doc_file_series.py index cad16d6..e5c1b18 100644 --- a/tests/test_open_doc_file_series.py +++ b/tests/test_open_doc_file_series.py @@ -839,13 +839,9 @@ def test_different_members_share_one_manifest_fetch(self): handler, calls = self._uid_keyed_handler() for index in (1, 2, 3): - self.db.open_doc( - doc.id(), f"{SERIES}_{index}", custom_file_handler=handler - ) + self.db.open_doc(doc.id(), f"{SERIES}_{index}", custom_file_handler=handler) - manifest_calls = [ - c for c in calls if c[2].get("uid") == self.manifest_uid - ] + manifest_calls = [c for c in calls if c[2].get("uid") == self.manifest_uid] self.assertEqual( len(manifest_calls), 1, @@ -863,9 +859,7 @@ def handler(dest_path, source_path, context): return None with self.assertRaises(FileAccessError) as caught: - self.db.open_doc( - doc.id(), f"{SERIES}_1", custom_file_handler=handler - ) + self.db.open_doc(doc.id(), f"{SERIES}_1", custom_file_handler=handler) self.assertEqual( caught.exception.identifier, "DID:SQLITEDB:FileSeries:ManifestNotLocal",