diff --git a/src/did/did_matlab_python_bridge.yaml b/src/did/did_matlab_python_bridge.yaml index 38d225b..dce371b 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: "ec1e733" + matlab_last_sync_hash: "9b36124" status: ported python_path: "did/database.py" python_class: "Database" @@ -59,6 +59,14 @@ classes: MissingOptionalDependencyWarning gated on the DID_FORCE_VALIDATION_WARNINGS environment variable. + 2026-09-09: bumped through DID-matlab 9b36124 (PR #200). Only + check_files needed a change -- the fail-open on a required file + declared in file_list with nothing bound to it in file_info, same + shape as PR #182's step-1 fail-open, one step down. See the checkfiles + method entry for the port. The rest of PR #200 was MATLAB-only: a + try/catch that read file_info before file_list under one arm, which + Python does not share. + Reviewed 2026-08-29 through MATLAB c0a9507, nothing to port. Two MATLAB fixes landed in this window and Python has neither defect: @@ -768,6 +776,20 @@ classes: meant a coverage check could not tell that isfilenamematch and canfindonefile were bridged at all. Split 2026-08-28. + 2026-09-09 (DID-matlab#200 / #199): step 3 used to loop over the + actual file_info entries and fall through to is_valid = True when a + required name had none -- the same fail-open PR #182 closed for + step 1, one step further down. Ported here: check_files now + separates the two absences, keeping the file_list message for a + name missing from the list and adding a new one for a name that IS + in the file_list with nothing bound for it (naming the file and + file_info). The other half of DID-matlab#200, an independent read + of file_list before file_info, is not a bug on the Python side -- + _validate_files already reads the two fields independently and did + not share MATLAB's try/catch shape -- so no code change beyond + check_files was needed. Coverage: + tests/test_validation.py:TestFileValidationDiagnosis. + - name: isfilenamematch kind: static python_name: is_filename_match diff --git a/src/did/validate.py b/src/did/validate.py index 08c26c3..12b032b 100644 --- a/src/did/validate.py +++ b/src/did/validate.py @@ -640,6 +640,15 @@ def check_files( Returns ``(is_valid, error_message)``. Mirrors MATLAB ``database.checkfiles``. + + Steps 1 and 3 report DIFFERENT ABSENCES. A schema-required name can be + missing from the document's file_list (step 1), or it can be there in the + file_list -- declared, correctly -- with nothing bound to it in file_info + (step 3). The second is the common one, and step 3 used to fall out of its + empty match loop without raising: a required file was absent and the + document was accepted anyway. That is the same fail-open MATLAB PR #182 + closed for step 1, one step further down, and DID-matlab#199 / #200 closed + for step 3. """ expected_unique = sorted(set(expected_names)) actual_unique = sorted(set(actual_file_names)) @@ -665,9 +674,23 @@ def check_files( if not _is_truthy(expected_value): continue item_name = expected_names[index] - for file_index, actual in enumerate(actual_file_names): - if not is_filename_match(item_name, actual): - continue + matches = [ + file_index + for file_index, actual in enumerate(actual_file_names) + if is_filename_match(item_name, actual) + ] + if not matches: + # The name IS in the document's file_list -- step 1 above + # established that -- but nothing is bound to it in file_info, so + # there is no file to look for. Say that, and say it about + # file_info: the file_list is correct here, and blaming it is what + # cost the debugging time in DID-matlab#199. + return False, ( + f'Required file "{item_name}" is declared in the file_list of ' + f"{doc_name} but no file is bound to that name " + f"(files.file_info has no entry for it)" + ) + for file_index in matches: entry = files[file_index] if file_index < len(files) else {} locations = entry.get("locations") if isinstance(entry, dict) else None if not can_find_one_file(locations): diff --git a/tests/test_validation.py b/tests/test_validation.py index da904db..ba0a0bf 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -404,3 +404,140 @@ def test_the_url_survives_the_round_trip(self): entries = entries if isinstance(entries, list) else [entries] locations.extend(item.get("location") for item in entries) self.assertIn(self.URL, locations) + + +class TestFileValidationDiagnosis(_DatabaseTestCase): + """WHICH FIELD a file-validation failure blames. + + check_files' step 3 used to loop over the actual file_info entries: a + required name declared in the file_list with nothing bound to it in + file_info matched nothing and fell through to is_valid = True. That is + the same fail-open MATLAB PR #182 closed for step 1, one step further + down; DID-matlab#199 closed it for step 3. Python's _validate_files + already read file_info and file_list independently -- MATLAB's own bug + from that PR was a try/catch shape Python does not share -- but the + fail-open in check_files itself was shared, so the port is that fix. + """ + + def _doc_with_file_info(self, file_info, file_list=None): + """A demoFile document whose files section is set by the caller. + + demoFile declares filename1.ext and filename2.ext, both + mustbenotempty. + """ + doc = Document("demoFile", **{"demoFile.value": 1}) + doc.document_properties["files"]["file_info"] = file_info + if file_list is not None: + doc.document_properties["files"]["file_list"] = file_list + return doc + + def _assert_refused(self, doc): + with self.assertRaises(ValidationError) as caught: + self.db.add_docs([doc]) + self.assertEqual( + caught.exception.identifier, + "DID:Database:ValidationFiles", + f"refused for the wrong reason: {caught.exception}", + ) + return str(caught.exception) + + def test_empty_file_info_blames_the_unbound_file_not_the_file_list(self): + # The reported case. file_list declares both names and is correct; + # nothing is bound to them. file_info = [] used to fall out of the + # empty match loop and be accepted (Python) or throw and get the + # file_list blamed (MATLAB). + doc = self._doc_with_file_info([]) + + msg = self._assert_refused(doc) + + self.assertIn( + "filename1.ext", + msg, + "the message must name the file that is not bound", + ) + self.assertIn( + "file_info", + msg, + "the message must name the field that is actually empty", + ) + self.assertNotIn( + "from the file_list", + msg, + ( + "the file_list declares the file and is correct, so the " + f"message must not send the reader there: {msg}" + ), + ) + + def test_name_missing_from_file_list_names_the_absent_name(self): + # A document whose file_list is genuinely incomplete: it holds + # filename1.ext and not filename2.ext. The absent name is + # filename2.ext, and that is what the message must say. + doc = self._doc_with_file_info([], file_list=["filename1.ext"]) + + msg = self._assert_refused(doc) + + self.assertIn( + "file_list", + msg, + "a name absent from the file_list IS a file_list problem", + ) + self.assertIn( + "filename2.ext", + msg, + "the message must name the file that is absent from the list", + ) + self.assertNotIn( + "filename1.ext", + msg, + ( + "filename1.ext is in the file_list; only filename2.ext is " + f"absent from it: {msg}" + ), + ) + + def test_document_with_no_files_field_still_validates(self): + # Most classes declare no files at all, and their documents have no + # files section. That must keep working. + doc = Document("demoA", **{"demoA.value": 1}) + self.assertNotIn( + "files", + doc.document_properties, + "precondition: a demoA document has no files section", + ) + self.db.add_docs([doc]) + self.assertIsNotNone(self.db.get_docs(doc.id())) + + def test_declared_optional_files_need_not_be_bound(self): + # demoSeries declares both of its files mustbenotempty = 0, so a + # document that binds neither is valid: the file_list is correct + # and nothing it declares is required. + doc = Document("demoSeries", **{"demoSeries.value": 1}) + doc.document_properties["files"]["file_info"] = [] + self.db.add_docs([doc]) + self.assertIsNotNone(self.db.get_docs(doc.id())) + + def test_bound_required_files_validate(self): + # The other side of the tightening: a document that binds what its + # schema requires still validates. Validation does no file I/O, so + # the locations need not exist yet. + doc = Document("demoFile", **{"demoFile.value": 1}) + doc.add_file("filename1.ext", "placeholder", ingest=0, delete_original=0) + doc.add_file("filename2.ext", "placeholder", ingest=0, delete_original=0) + self.db.add_docs([doc]) + self.assertIsNotNone(self.db.get_docs(doc.id())) + + def test_check_files_directly_reports_unbound_required_file(self): + # The unit-level counterpart, calling check_files without a database. + is_valid, message = check_files( + expected_names=["filename1.ext", "filename2.ext"], + must_have_value=[1, 1], + actual_file_names=[], + doc_name="demoFile doc X", + files=[], + actual_file_list=["filename1.ext", "filename2.ext"], + ) + self.assertFalse(is_valid) + self.assertIn("filename1.ext", message) + self.assertIn("file_info", message) + self.assertNotIn("from the file_list", message)