diff --git a/src/did/did_matlab_python_bridge_implementations.yaml b/src/did/did_matlab_python_bridge_implementations.yaml index 11ab65c..b8f8a59 100644 --- a/src/did/did_matlab_python_bridge_implementations.yaml +++ b/src/did/did_matlab_python_bridge_implementations.yaml @@ -59,6 +59,17 @@ classes: index before building it (with the approximate doc_data row count), reports the time it took, and reports a build that failed; ported to _ensure_indexes, which prints the same lines to stderr. + Two Python-only differences, 2026-10-24 (issue #101): (1) a resolved + depends_on (hasanysubfield_exact_string on depends_on) is matched in SQL + against meta.depends_on with an anchored, case-sensitive instr() + (';name,value;', or ',value;' for '*'), where it used to brute-force + every document of the branch. MATLAB's own depends_on SQL is LIKE + '%name,value;%', which is case-insensitive and unanchored ('id,X;' also + matches 'subject_id,X;'); Python follows brute force's exact semantics + instead, and keeps brute force for an empty name/value or one containing + ',' or ';'. (2) _ensure_indexes builds the indexes without announcing + them when doc_data is empty -- every new Python database, since + _create_db_tables does not create them; MATLAB's create_db_tables does. sync_notes: > MATLAB 14439fb (2026-06-12, audit 6.1-3) added a private escapeSqlLiteral() helper and applied it at the 14 sites where diff --git a/src/did/implementations/sqlitedb.py b/src/did/implementations/sqlitedb.py index d2f727d..1031ac5 100644 --- a/src/did/implementations/sqlitedb.py +++ b/src/did/implementations/sqlitedb.py @@ -239,6 +239,68 @@ def _base_id_lookup_values(search_struct): return True, found +def _depends_on_sql_clause(name, value): + """The SQL for "a depends_on entry has this name (or any, '*') and value". + + doc2sql stores a document's dependencies in ``meta.depends_on`` as + ``name1,value1;name2,value2;`` -- one ``name,value;`` per entry whose name + and value are both non-empty. So "an entry has name N and value V" is "the + string, with a ';' put in front, contains ';N,V;'", and "an entry has + value V" is "it contains ',V;'". ``instr`` is a plain, case-sensitive + substring test, as brute force's ``==`` is; MATLAB's ``LIKE + '%name,value;%'`` is neither anchored (``id,X;`` would also match + ``subject_id,X;``) nor case-sensitive. + + That reading is exact only when the query's name and value are non-empty + and contain no ',' or ';' -- the separators. Anything else returns None, + and the caller brute-forces it. (It also assumes the stored names and + values are strings without separators, which document ids and schema + field names are.) + """ + if not isinstance(value, str) or not value or any(c in value for c in ",;"): + return None + if name == "*": + pattern = f",{value};" + haystack = "doc_data.value" + else: + if not isinstance(name, str) or not name or any(c in name for c in ",;"): + return None + pattern = f";{name},{value};" + haystack = "';' || doc_data.value" + return ( + "fields.field_name = 'meta.depends_on' AND " + f"instr({haystack}, '{_sql_escape(pattern)}') > 0" + ) + + +def _depends_on_lookup(search_struct): + """(name, value) if a search structure is a resolved depends_on, else None. + + Query resolves ``depends_on(name, value)`` into ``hasanysubfield_exact_string`` + on the ``depends_on`` field with param1 ``["name", "value"]`` and param2 + ``[name, value]`` -- or param1 ``["value"]`` for the ``'*'`` name. + """ + if not isinstance(search_struct, dict): + return None + op = str(search_struct.get("operation") or "").lstrip("~").strip().lower() + if ( + op != "hasanysubfield_exact_string" + or search_struct.get("field") != "depends_on" + ): + return None + keys, values = search_struct.get("param1"), search_struct.get("param2") + if not isinstance(keys, list) or not isinstance(values, list): + return None + if len(keys) != len(values) or len(set(keys)) != len(keys): + return None + wanted = dict(zip(keys, values)) + if set(wanted) == {"name", "value"}: + return wanted["name"], wanted["value"] + if set(wanted) == {"value"}: + return "*", wanted["value"] + return None + + def _search_rank(item): """Where a term of an AND is searched -- see SQLiteDB._search_doc_ids.""" if _base_id_lookup_values(item)[0]: @@ -246,6 +308,10 @@ def _search_rank(item): if not isinstance(item, dict): return 2 op = str(item.get("operation") or "").lstrip("~").strip().lower() + lookup = _depends_on_lookup(item) + if lookup is not None and _depends_on_sql_clause(*lookup) is not None: + # One row per document, tested in SQL: cheap, and usually selective + return 2 if op in _SCAN_OPERATIONS: return 4 if item.get("field") == "base.session_id": @@ -420,12 +486,18 @@ def _ensure_indexes(self): ).fetchone()[0] except sqlite3.Error: num_rows = None - print( - f"DID: adding search index {name} to {self.connection} " - f"(about {num_rows or 0:,} rows). This happens once per database " - "and can take several minutes for a large one...", - file=sys.stderr, - ) + # An empty table builds instantly and is not worth a word. That is + # every new database: _create_db_tables makes the tables and this + # then adds the indexes -- where DID-matlab's create_db_tables + # makes both, so it never gets here. + announce = bool(num_rows) + if announce: + print( + f"DID: adding search index {name} to {self.connection} " + f"(about {num_rows:,} rows). This happens once per database " + "and can take several minutes for a large one...", + file=sys.stderr, + ) started = time.perf_counter() try: self.dbid.execute( @@ -441,11 +513,12 @@ def _ensure_indexes(self): file=sys.stderr, ) return - print( - f"DID: added search index {name} in " - f"{time.perf_counter() - started:.1f} s.", - file=sys.stderr, - ) + if announce: + print( + f"DID: added search index {name} in " + f"{time.perf_counter() - started:.1f} s.", + file=sys.stderr, + ) self._search_indexes_ready = True def _close_db(self): @@ -1862,31 +1935,19 @@ def _query_struct_to_sql_str(self, search_struct, restricted=False): ) elif op_lower == "depends_on": - # depends_on: search meta.depends_on using LIKE '%name,value;%'. - # NOTE: this branch is currently unreachable — Query._resolve_single - # rewrites every 'depends_on' into 'hasanysubfield_exact_string' - # before it reaches here, so depends_on always brute-forces. The - # LIKE-wildcard escaping below is applied defensively so that if the - # resolution is ever changed to let this branch run, '_'/'%' in the - # dependency name/value can't silently act as wildcards. The ','/';' - # delimiters and the bracketing '%' are the real pattern structure - # and stay unescaped. - name = _sql_escape(_sql_like_escape(param1)) - value = _sql_escape(_sql_like_escape(param2)) - if param1 == "*": - return ( - "fields.field_name = 'meta.depends_on' AND doc_data.value " - f"LIKE '%,{value};%' ESCAPE '{_LIKE_ESCAPE_CHAR}'" - ) - return ( - "fields.field_name = 'meta.depends_on' AND doc_data.value " - f"LIKE '%{name},{value};%' ESCAPE '{_LIKE_ESCAPE_CHAR}'" - ) + # Unreachable in practice: Query._resolve_single rewrites every + # 'depends_on' into 'hasanysubfield_exact_string' (below) before it + # gets here. Kept on the same exact matcher so the two cannot + # disagree if that ever changes. + return _depends_on_sql_clause(param1, param2) elif op_lower == "hasanysubfield_exact_string": - # unreachable — see Query._resolve_single (resolved depends_on); - # falls back to brute force - return None + # A resolved depends_on is matched in SQL against meta.depends_on; + # any other use brute-forces + lookup = _depends_on_lookup(search_struct) + if lookup is None: + return None + return _depends_on_sql_clause(*lookup) elif op_lower == "hasanysubfield_contains_string": # Used by resolved isa - fall back to brute force diff --git a/tests/test_depends_on_search.py b/tests/test_depends_on_search.py new file mode 100644 index 0000000..059a50e --- /dev/null +++ b/tests/test_depends_on_search.py @@ -0,0 +1,166 @@ +"""depends_on searches run in SQL, with the same results as brute force. + +``Query("", "depends_on", name, value)`` resolves to +``hasanysubfield_exact_string`` on ``depends_on``, which used to have no SQL +form: every such search loaded and tested every document of the branch +(issue #101). It is now matched against ``meta.depends_on`` -- doc2sql's +``name1,value1;name2,value2;`` -- with an anchored, case-sensitive ``instr``. +These tests check that the SQL path returns exactly what brute force does on +documents chosen to trip it up, and that it no longer loads documents. +""" + +import os +import tempfile +import unittest +import uuid + +from did.document import Document +from did.implementations.sqlitedb import SQLiteDB +from did.query import Query + + +class _LoadCountingDB(SQLiteDB): + """A SQLiteDB that counts the documents get_docs is asked for.""" + + def __init__(self, filename): + self.loaded = 0 + super().__init__(filename) + + def get_docs(self, document_ids, *args, **kwargs): + ids = document_ids if isinstance(document_ids, list) else [document_ids] + self.loaded += len(ids) + return super().get_docs(document_ids, *args, **kwargs) + + +def _doc(depends_on, session="s1"): + props = { + "base": { + "id": uuid.uuid4().hex, + "session_id": session, + "name": "", + "datestamp": "2026-10-24T00:00:00.000Z", + }, + "document_class": {"class_name": "thing", "superclasses": []}, + } + if depends_on is not None: + props["depends_on"] = depends_on + return Document(props) + + +def _dep(name, value): + return {"name": name, "value": value} + + +class TestDependsOnSearch(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.tmpdir = tempfile.TemporaryDirectory() + cls.db = _LoadCountingDB(os.path.join(cls.tmpdir.name, "depends_on.sqlite")) + cls.db.add_branch("a") + docs = [ + _doc([_dep("element_id", "E1")]), + _doc([_dep("element_id", "E1"), _dep("epoch_id", "P1")]), + _doc([_dep("epoch_id", "P1"), _dep("element_id", "E2")]), + # 'id' must not match inside 'subject_id', nor 'E1' inside 'xE1' + _doc([_dep("subject_id", "E1")]), + _doc([_dep("element_id", "xE1")]), + _doc([_dep("element_id", "E10")]), + # case matters, as it does for brute force's == + _doc([_dep("Element_id", "E1")]), + _doc([_dep("element_id", "e1")]), + # enumerated dependency names + _doc([_dep("input_1", "E1"), _dep("input_2", "E2")]), + # an empty value is not stored in meta.depends_on + _doc([_dep("element_id", "")]), + # no dependencies at all + _doc([]), + _doc(None), + ] + cls.db.add_docs(docs, branch_id="a", validate=False) + cls.all_ids = [d.id() for d in docs] + + @classmethod + def tearDownClass(cls): + cls.db._close_db() + cls.tmpdir.cleanup() + + def brute_force(self, query): + """What brute force returns for a query (one leaf).""" + (struct,) = query.to_search_structure() + return sorted(self.db._brute_force_search(struct, "a")) + + def assert_same_as_brute_force(self, name, value): + for op in ("depends_on", "~depends_on"): + query = Query("", op, name, value) + with self.subTest(op=op, name=name, value=value): + self.assertEqual( + sorted(self.db.search(query, branch_id="a")), + self.brute_force(query), + ) + + def test_sql_matches_brute_force(self): + cases = [ + ("element_id", "E1"), + ("element_id", "E2"), + ("epoch_id", "P1"), + ("id", "E1"), # a suffix of 'element_id' / 'subject_id' + ("element_id", "1"), # a suffix of 'E1' + ("Element_id", "E1"), + ("element_id", "e1"), + ("input_2", "E2"), + ("element_id", "nosuch"), + ("*", "E1"), + ("*", "P1"), + ("*", "1"), + ("*", "nosuch"), + ] + for name, value in cases: + self.assert_same_as_brute_force(name, value) + + def test_known_matches(self): + def search(name, value): + return len( + self.db.search(Query("", "depends_on", name, value), branch_id="a") + ) + + self.assertEqual(search("element_id", "E1"), 2) + self.assertEqual(search("id", "E1"), 0) + self.assertEqual( + search("*", "E1"), 5 + ) # element_id x2, subject_id, Element_id, input_1 + + def test_runs_in_sql_without_loading_documents(self): + for name, value in (("element_id", "E1"), ("*", "P1")): + self.db.loaded = 0 + self.db.search(Query("", "depends_on", name, value), branch_id="a") + self.assertEqual(self.db.loaded, 0, f"depends_on {name} {value}") + + def test_what_sql_cannot_represent_still_brute_forces(self): + # an empty value is not stored in meta.depends_on, and ',' / ';' are + # its separators: these keep the brute-force path, and its results + for name, value in (("element_id", ""), ("element_id", "a,b"), ("a;b", "E1")): + query = Query("", "depends_on", name, value) + self.db.loaded = 0 + found = sorted(self.db.search(query, branch_id="a")) + self.assertGreater( + self.db.loaded, 0, f"{name!r} {value!r} should brute-force" + ) + self.assertEqual(found, self.brute_force(query)) + self.assertEqual( + len( + self.db.search(Query("", "depends_on", "element_id", ""), branch_id="a") + ), + 1, + ) + + def test_depends_on_with_session(self): + q = Query("", "depends_on", "element_id", "E1") & Query( + "base.session_id", "exact_string", "s1" + ) + self.db.loaded = 0 + self.assertEqual(len(self.db.search(q, branch_id="a")), 2) + self.assertEqual(self.db.loaded, 0) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_query_correctness.py b/tests/test_query_correctness.py index d533c6e..f3a9b65 100644 --- a/tests/test_query_correctness.py +++ b/tests/test_query_correctness.py @@ -136,9 +136,10 @@ def test_contains_string_percent_is_not_a_wildcard(self): self.assertEqual(ids, [match_id]) - def test_depends_on_like_pattern_escapes_wildcards(self): - # The (currently unreachable) SQL depends_on branch must LIKE-escape the - # dependency name so a '_' in it cannot act as a single-char wildcard. + def test_depends_on_sql_has_no_wildcards(self): + # The (currently unreachable) SQL depends_on branch must not let a '_' + # or '%' in the dependency name act as a wildcard. It matches with + # instr(), a plain substring test, rather than LIKE. struct = { "field": "", "operation": "depends_on", @@ -147,9 +148,9 @@ def test_depends_on_like_pattern_escapes_wildcards(self): } sql = self.db._query_struct_to_sql_str(struct) - # The underscore is escaped and an ESCAPE clause is emitted. - self.assertIn("a\\_b", sql) - self.assertIn("ESCAPE", sql) + self.assertNotIn("LIKE", sql) + self.assertIn("instr(", sql) + self.assertIn("';a_b,val;'", sql) # --- numeric op with a non-numeric param1: graceful fallback -------------- diff --git a/tests/test_search_indexes.py b/tests/test_search_indexes.py index eab04cd..409b4b9 100644 --- a/tests/test_search_indexes.py +++ b/tests/test_search_indexes.py @@ -152,6 +152,19 @@ def test_new_database_has_search_indexes(self): self.assertIn(name, names) self.assertTrue(self.db._search_indexes_ready) + def test_new_database_is_created_quietly(self): + # A new database's tables are empty, so its indexes build instantly; + # announcing them would print three lines for every database created. + with tempfile.TemporaryDirectory() as tmp: + stderr = io.StringIO() + with contextlib.redirect_stderr(stderr): + db = SQLiteDB(os.path.join(tmp, "new.sqlite")) + try: + self.assertTrue(db._search_indexes_ready) + self.assertNotIn("search index", stderr.getvalue()) + finally: + db._close_db() + def test_indexes_added_when_older_database_opened(self): with tempfile.TemporaryDirectory() as tmp: path = os.path.join(tmp, "older.sqlite")