From a0bca417f243ad86ec47d029a7f6838fd90d0b4e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 13:36:41 +0000 Subject: [PATCH] Bind the files these fixtures declare DID-python#86 closed the fail-open in check_files step 3, so a document that declares a required file in its file_list and binds nothing in files.file_info is refused where it is created rather than accepted and failed on at read. #292 fixed the product half; these twenty-odd construction sites are the test half. Each site now attaches a file, as tests/matlab_tests/conftest.py's _add_doc_with_file already did, or uses a document class that declares none. No schema is relaxed: a generic_file genuinely requires generic_file.ext, and NDI-matlab#977 respected that same constraint when makeSourceFile moved to fileReference rather than emitting a fileless generic_file. The schemas describe reality; the fixtures were wrong. Two sites are not simply "attach a file": test_extract_doc_files_series.py's test_a_document_with_no_files_is_left_alone wants a document with NO files, so it builds a bare base document instead of a demoNDI. Attaching a file there would have inverted the test. test_did_integration.py's definitions gate validates a BLANK template, and a blank template legitimately binds nothing -- the document that binds the files is the one that gets stored. The files check now runs ahead of the field checks, so seven definitions reported DID:Database:ValidationFiles instead of whatever is actually wrong with them. The gate asks about a definition's structure, so it now binds the declared names first, exactly as _BLANK_TEMPLATE_IDENTIFIERS already discounts an unfilled dependency. The allowlist is unchanged and still earns its keep: the five listed definitions report the same ValidationFieldInteger/ValidationFieldDouble they were listed for, and data/geneList and sorting/SpikeInterfaceSortingOutputs -- which were never listed -- validate clean, confirming nothing was structurally wrong with them. Adding all seven to the allowlist under the new code would have erased that record. Measured on this container with did and vhlab-toolbox-python installed from main: 21 failed / 5155 passed / 552 skipped before, 5176 passed / 552 skipped after, with no test failing that was not failing already. Closes #293 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCfsstd8ma954CDDXg4Hm2 --- tests/matlab_tests/test_core.py | 8 ++++++ tests/matlab_tests/test_database.py | 7 +++++ tests/matlab_tests/test_fun.py | 36 ++++++++++++++++++------ tests/matlab_tests/test_session.py | 8 ++++++ tests/matlab_tests/test_validators.py | 19 +++++++++++-- tests/test_did_integration.py | 39 ++++++++++++++++++++++++++ tests/test_extract_doc_files_series.py | 7 +++-- tests/test_session_database_rm.py | 7 +++++ 8 files changed, 118 insertions(+), 13 deletions(-) diff --git a/tests/matlab_tests/test_core.py b/tests/matlab_tests/test_core.py index 33a1bf37..011eca1b 100644 --- a/tests/matlab_tests/test_core.py +++ b/tests/matlab_tests/test_core.py @@ -224,6 +224,14 @@ def test_query_integration(self, tmp_path): "demoNDI.value": 2, }, ) + # demoNDI declares filename1.ext in its file_list; a document that + # declares a required file and binds nothing is refused at add time + # (VH-Lab/DID-python#86). + for doc, name in ((doc1, "alpha"), (doc2, "beta")): + payload = session_dir / f"{name}.dat" + payload.write_text(name) + doc.add_file("filename1.ext", str(payload)) + session.database_add(doc1) session.database_add(doc2) diff --git a/tests/matlab_tests/test_database.py b/tests/matlab_tests/test_database.py index 21c6bb11..be8ecab0 100644 --- a/tests/matlab_tests/test_database.py +++ b/tests/matlab_tests/test_database.py @@ -204,6 +204,13 @@ def test_document_round_trip(self, tmp_path): ) original_id = doc.id + # demoNDI declares filename1.ext in its file_list; a document that + # declares a required file and binds nothing is refused at add time + # (VH-Lab/DID-python#86). + payload = session_dir / "persistent_doc.dat" + payload.write_text("persistent_doc") + doc = doc.add_file("filename1.ext", str(payload)) + # Save session.database_add(doc) diff --git a/tests/matlab_tests/test_fun.py b/tests/matlab_tests/test_fun.py index 922f8c60..78fdb536 100644 --- a/tests/matlab_tests/test_fun.py +++ b/tests/matlab_tests/test_fun.py @@ -47,6 +47,24 @@ def _make_demo_doc(name: str = "test", value: int = 42, session_id: str = "") -> return ndi_document(props) +def _make_demo_doc_with_file( + directory, name: str, value: int, session_id: str = "" +) -> ndi_document: + """A demoNDI document with its declared file bound. + + demoNDI declares ``filename1.ext`` in its ``file_list``, and a document + that declares a required file and binds nothing is refused at add time + (VH-Lab/DID-python#86). Mirrors ``conftest._add_doc_with_file``, except + that the source file is kept: several of these tests add a copy of the + same properties to a SECOND database, which needs the location to still + resolve after the first ingest. + """ + doc = _make_demo_doc(name, value, session_id) + payload = directory / f"{name}.dat" + payload.write_text(name) + return doc.add_file("filename1.ext", str(payload), delete_original=0) + + def _make_session_with_docs(tmp_path, ref, doc_specs): """Create a ndi_session_dir and add documents from a list of (name, value) tuples. @@ -163,7 +181,7 @@ def test_findFuid_not_found(self, tmp_path): session = ndi_session_dir("fuid_test2", session_dir) # Add a document (so the database is not empty) - doc = _make_demo_doc("some_doc", 99, session.id()) + doc = _make_demo_doc_with_file(session_dir, "some_doc", 99, session.id()) session.database_add(doc) found_doc, found_name = findFuid(session, "nonexistent_uid_12345") @@ -353,7 +371,7 @@ def test_identical_sessions(self, tmp_path): session = ndi_session_dir("identical", session_dir) for i in range(1, 4): - doc = _make_demo_doc(f"doc_{i}", i, session.id()) + doc = _make_demo_doc_with_file(session_dir, f"doc_{i}", i, session.id()) session.database_add(doc) # Reopen same session (same path -> same data) @@ -383,7 +401,7 @@ def test_docs_only_in_s1(self, tmp_path): # Add 3 docs to session1, 0 to session2 for i in range(1, 4): - doc = _make_demo_doc(f"doc_{i}", i, session1.id()) + doc = _make_demo_doc_with_file(s1_dir, f"doc_{i}", i, session1.id()) session1.database_add(doc) result = session_diff(session1, session2) @@ -410,7 +428,7 @@ def test_docs_only_in_s2(self, tmp_path): # Add 2 docs to session2, 0 to session1 for i in range(1, 3): - doc = _make_demo_doc(f"doc_{i}", i, session2.id()) + doc = _make_demo_doc_with_file(s2_dir, f"doc_{i}", i, session2.id()) session2.database_add(doc) result = session_diff(session1, session2) @@ -436,7 +454,7 @@ def test_mismatched_docs(self, tmp_path): session2 = ndi_session_dir("sess_b", s2_dir) # Create a document with a known ID - doc = _make_demo_doc("shared_doc", 10, session1.id()) + doc = _make_demo_doc_with_file(s1_dir, "shared_doc", 10, session1.id()) doc_id = doc.document_properties["base"]["id"] session1.database_add(doc) @@ -474,7 +492,7 @@ def test_identical_datasets(self, tmp_path): sess_dir.mkdir() session = ndi_session_dir("src", sess_dir) for i in range(1, 3): - doc = _make_demo_doc(f"doc_{i}", i, session.id()) + doc = _make_demo_doc_with_file(sess_dir, f"doc_{i}", i, session.id()) session.database_add(doc) # Create dataset and ingest @@ -502,7 +520,7 @@ def test_docs_only_in_dataset1(self, tmp_path): sess1_dir.mkdir() session1 = ndi_session_dir("sess1", sess1_dir) for i in range(1, 4): - doc = _make_demo_doc(f"doc_{i}", i, session1.id()) + doc = _make_demo_doc_with_file(sess1_dir, f"doc_{i}", i, session1.id()) session1.database_add(doc) ds1_dir = tmp_path / "ds1" @@ -536,7 +554,7 @@ def test_docs_only_in_dataset2(self, tmp_path): sess2_dir.mkdir() session2 = ndi_session_dir("sess2", sess2_dir) for i in range(1, 4): - doc = _make_demo_doc(f"doc_{i}", i, session2.id()) + doc = _make_demo_doc_with_file(sess2_dir, f"doc_{i}", i, session2.id()) session2.database_add(doc) ds2_dir = tmp_path / "ds2" @@ -555,7 +573,7 @@ def test_mismatched_datasets(self, tmp_path): MATLAB equivalent: diffTest.testMismatchedDocs """ # Create a document - doc = _make_demo_doc("shared", 10) + doc = _make_demo_doc_with_file(tmp_path, "shared", 10) doc.document_properties["base"]["id"] # ndi_dataset 1 diff --git a/tests/matlab_tests/test_session.py b/tests/matlab_tests/test_session.py index c4bde6c0..76a9b59b 100644 --- a/tests/matlab_tests/test_session.py +++ b/tests/matlab_tests/test_session.py @@ -124,6 +124,14 @@ def test_ingested_session_in_dataset(self, tmp_path): props["demoNDI"]["value"] = 42 props["base"]["session_id"] = session.id() doc = ndi_document(props) + + # demoNDI declares filename1.ext in its file_list; a document that + # declares a required file and binds nothing is refused at add time + # (VH-Lab/DID-python#86). Mirrors conftest._add_doc_with_file. + payload = session_dir / "test_doc.dat" + payload.write_text("test_doc") + doc = doc.add_file("filename1.ext", str(payload)) + session.database_add(doc) # Create dataset and ingest diff --git a/tests/matlab_tests/test_validators.py b/tests/matlab_tests/test_validators.py index cd858d24..8ad30110 100644 --- a/tests/matlab_tests/test_validators.py +++ b/tests/matlab_tests/test_validators.py @@ -23,6 +23,21 @@ from ndi.session.dir import ndi_session_dir from ndi.session.session_base import empty_id + +def _with_its_declared_file(doc, directory): + """Bind demoNDI's declared ``filename1.ext``. + + demoNDI declares that file in its ``file_list``, and a document that + declares a required file and binds nothing is refused at add time + (VH-Lab/DID-python#86). These tests are about session_id handling, so + the file's content is incidental -- it just has to be there for the add + to reach the behaviour under test. + """ + payload = directory / f"{doc.id}.ext" + payload.write_text(doc.id) + return doc.add_file("filename1.ext", str(payload)) + + # =========================================================================== # TestDocumentValidation # Port of: MATLAB validator tests - document-level validation @@ -87,7 +102,7 @@ def test_session_id_empty_accepted(self, tmp_path): props = doc.document_properties props["base"]["session_id"] = "" props["demoNDI"]["value"] = 99 - doc = ndi_document(props) + doc = _with_its_declared_file(ndi_document(props), session_dir) # Should not raise -- empty session_id is accepted and auto-assigned session.database_add(doc) @@ -105,7 +120,7 @@ def test_session_id_empty_id_accepted(self, tmp_path): props = doc.document_properties props["base"]["session_id"] = empty_id() props["demoNDI"]["value"] = 88 - doc = ndi_document(props) + doc = _with_its_declared_file(ndi_document(props), session_dir) # Should not raise -- empty_id is accepted and auto-assigned session.database_add(doc) diff --git a/tests/test_did_integration.py b/tests/test_did_integration.py index e4fc1100..e840d0dc 100644 --- a/tests/test_did_integration.py +++ b/tests/test_did_integration.py @@ -252,6 +252,44 @@ def _bundled_document_types(): yield rel_posix.removesuffix(".json"), path +def _bind_the_files_it_declares(props): + """Give a blank template a binding for every file name it declares. + + A definition is a TEMPLATE: it names the files a stored document must + carry and binds none of them, because the document that binds them is + the one that gets stored. Since VH-Lab/DID-python#86 the validator says + so -- ``DID:Database:ValidationFiles``, "declared in the file_list ... + but no file is bound to that name" -- and says it FIRST, ahead of the + field checks, so seven definitions started reporting that instead of + whatever is actually wrong with them. + + That is the right answer about a document and the wrong question here. + This gate asks about the DEFINITION's structure, and asks it of a blank + one; an unbound file is a property of the blank, exactly as an unfilled + dependency is (see ``_BLANK_TEMPLATE_IDENTIFIERS``). So bind the + declared names first -- the same fix the fixtures elsewhere in this + suite took -- and let validation reach the questions the lists below + record. Skipping the identifier instead would have hidden those: five + of the seven are listed for a field error that is still there. + + A name ending in ``#`` is a file SERIES, matched by ``is_filename_match`` + as the stem followed by digits, so it is bound under its first member's + name. Locations are ``https://`` URLs because ``can_find_one_file`` does + no network I/O and does not pre-check a non-local location: nothing here + touches the filesystem. + """ + file_list = [str(n) for n in (props.get("files") or {}).get("file_list") or []] + if not file_list: + return + props["files"]["file_info"] = [ + { + "name": f"{name[:-1]}1" if name.endswith("#") else name, + "locations": [{"location": "https://example.invalid/placeholder"}], + } + for name in file_list + ] + + def _structural_failure(doc_type): """Validate a blank document of ``doc_type``; return an error id or None.""" from did.validate import ValidationError, get_document_schema, validate_doc_vs_schema @@ -261,6 +299,7 @@ def _structural_failure(doc_type): doc_id = props["base"]["id"] session_id = ndi_ido().id props["base"]["session_id"] = session_id + _bind_the_files_it_declares(props) try: schema = get_document_schema(props["document_class"]["validation"]) with warnings.catch_warnings(): diff --git a/tests/test_extract_doc_files_series.py b/tests/test_extract_doc_files_series.py index fa4535e8..5d16310a 100644 --- a/tests/test_extract_doc_files_series.py +++ b/tests/test_extract_doc_files_series.py @@ -161,9 +161,12 @@ def test_the_file_list_is_read_before_file_info_is_cleared(self, tmp_path): def test_a_document_with_no_files_is_left_alone(self, tmp_path): session = _session(tmp_path, "src_session") - props = ndi_document("demoNDI").document_properties + # A bare base document, not a demoNDI: demoNDI declares filename1.ext + # in its file_list, so one with nothing bound is refused at add time + # (VH-Lab/DID-python#86) rather than being the fileless document this + # test is about. + props = ndi_document("base").document_properties props["base"]["session_id"] = session.id() - props["demoNDI"]["value"] = 1 session.database_add(ndi_document(props)) out = tmp_path / "out" diff --git a/tests/test_session_database_rm.py b/tests/test_session_database_rm.py index e219e272..5792c5aa 100644 --- a/tests/test_session_database_rm.py +++ b/tests/test_session_database_rm.py @@ -39,6 +39,13 @@ def _a_document(session): doc = session.newdocument("data/generic_file") doc.document_properties["generic_file"]["dateCreated"] = 0 doc.document_properties["generic_file"]["dateUpdated"] = 0 + # data/generic_file declares generic_file.ext in its file_list, and a + # document that declares a required file and binds nothing is refused at + # add time (VH-Lab/DID-python#86). The file is incidental here -- these + # tests are about database_rm -- but it has to exist. + payload = session.path / f"{doc.id}.ext" + payload.write_text(doc.id) + doc = doc.add_file("generic_file.ext", str(payload)) session.database_add(doc) return doc