From d24f3f989fece42d44a8fa90cac20f4a51515771 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Thu, 22 Oct 2026 17:33:46 -0700 Subject: [PATCH] fix(review): validate stored review evidence once per startup Startup used to run the full review validation twice (the early guard and the install) and the annotation group's row checks inside each, so every evidence document was parsed and re-encoded twice. The early guard is now catalog-only (a review group without annotation tables), and the install validates the group once, trusting the annotation group it has just validated. The evidence predicate no longer pattern-scans every document in SQL, because the streamed canonical pass proves it, and the codec is built once. Measured on a database with 40,000 items of 4 KiB evidence: startup open went from 7.7 s to 2.5 s. A test pins one evidence pass per reopen. Restore and verify_database_file keep the full validation. --- database/models.py | 11 +++-- database/review_schema.py | 83 ++++++++++++++++++++++++++----------- tests/test_review_schema.py | 37 +++++++++++++++++ 3 files changed, 104 insertions(+), 27 deletions(-) diff --git a/database/models.py b/database/models.py index ce439dd..3724465 100644 --- a/database/models.py +++ b/database/models.py @@ -18,7 +18,11 @@ from typing import Optional, Set, Tuple from .library_schema import library_schema_error, migrate_library_state from .image_favorites import image_favorites_schema_error, migrate_image_favorites from .annotation_schema import annotation_schema_error, migrate_annotation_state -from .review_schema import migrate_review_state, review_schema_error +from .review_schema import ( + migrate_review_state, + review_schema_error, + review_without_annotations_error, +) from .saved_views import migrate_saved_views, saved_views_schema_error from .execution_history_schema import ( history_schema_error, @@ -305,8 +309,9 @@ class PromptModel: migrate_image_favorites(conn) migrate_saved_views(conn) # A review group without its annotation group is refused before the - # annotation install could make it look valid. - review_error = review_schema_error(conn) + # annotation install could make it look valid (catalog only; the rows + # are validated once, in migrate_review_state). + review_error = review_without_annotations_error(conn) if review_error: raise sqlite3.DatabaseError(review_error) migrate_annotation_state(conn) diff --git a/database/review_schema.py b/database/review_schema.py index f610744..b7f5bdd 100644 --- a/database/review_schema.py +++ b/database/review_schema.py @@ -33,7 +33,10 @@ import re import sqlite3 from typing import Any, Optional -from .annotation_schema import BYTES_TABLE, annotation_schema_error +from .annotation_schema import BYTES_TABLE +from .annotation_schema import TABLES as ANNOTATION_TABLES +from .annotation_schema import _catalog as _annotation_catalog +from .annotation_schema import annotation_schema_error SCHEMA_VERSION = 1 METADATA_TABLE = "media_review_metadata" @@ -159,6 +162,13 @@ def _reject_constant(name: str) -> Any: raise _NonCanonical(f"JSON constant {name} is not allowed") +# Built once: validation re-encodes every stored document at startup. +_ENCODER = json.JSONEncoder( + sort_keys=True, separators=(",", ":"), ensure_ascii=True, allow_nan=False +) +_DECODER = json.JSONDecoder(parse_constant=_reject_constant) + + def canonical_evidence(value: Any) -> str: """Serialize one evidence object canonically; ``ValueError`` when invalid. @@ -168,13 +178,7 @@ def canonical_evidence(value: Any) -> str: if not isinstance(value, dict): raise ValueError("Review evidence must be a JSON object") try: - text = json.dumps( - value, - sort_keys=True, - separators=(",", ":"), - ensure_ascii=True, - allow_nan=False, - ) + text = _ENCODER.encode(value) except (TypeError, RecursionError) as error: raise ValueError(f"Review evidence is not serializable: {error}") from None if len(text) > MAX_EVIDENCE_BYTES: @@ -191,7 +195,7 @@ def _is_canonical(text: Any) -> bool: if not isinstance(text, str): return False try: - value = json.loads(text, parse_constant=_reject_constant) + value = _DECODER.decode(text) return canonical_evidence(value) == text except (ValueError, RecursionError): return False @@ -228,12 +232,12 @@ def _hex64(column: str) -> str: def _evidence(column: str) -> str: - # Printable ASCII only is a cheap necessary condition for canonical text; - # the streamed pass below proves the rest. + # Type and size only: the streamed pass below proves the text canonical + # (which implies printable ASCII), so a pattern scan here would read every + # document a second time. return ( f"typeof({column}) = 'text' " - f"AND length(CAST({column} AS BLOB)) BETWEEN 2 AND {MAX_EVIDENCE_BYTES} " - f"AND CAST({column} AS BLOB) NOT GLOB '*[^ -~]*'" + f"AND length(CAST({column} AS BLOB)) BETWEEN 2 AND {MAX_EVIDENCE_BYTES}" ) @@ -356,25 +360,55 @@ def _value_error(conn: sqlite3.Connection) -> Optional[str]: return _evidence_error(conn) +def review_without_annotations_error(conn: sqlite3.Connection) -> Optional[str]: + """Catalog only: a review group present without its annotation tables. + + Startup asks this before the annotation group installs, which would make + a review group copied without it look valid. It reads no rows; the full + validation runs once in :func:`migrate_review_state`. + """ + if not _catalog(conn): + return None + present = { + name for kind, name, _sql in _annotation_catalog(conn) if kind == "table" + } + if present != set(ANNOTATION_TABLES): + return ( + "Review queue schema requires a valid image annotation schema: " + "Missing image annotation schema" + ) + return None + + def review_schema_error( - conn: sqlite3.Connection, *, allow_missing: bool = True + conn: sqlite3.Connection, + *, + allow_missing: bool = True, + annotations_valid: bool = False, ) -> Optional[str]: """Reject partial or unsupported review schemas and invalid rows. Reads only: nothing is ever repaired. A database without any of the group's objects is acceptable for backups that predate it unless ``allow_missing`` is false. A present group requires a present, valid - image annotation group. + image annotation group; ``annotations_valid`` says the caller has just + validated it (startup's annotation install), so only its presence is + checked again. """ objects = _catalog(conn) if not objects: return None if allow_missing else "Missing review queue schema" - annotation_error = annotation_schema_error(conn, allow_missing=False) + if annotations_valid: + annotation_error = review_without_annotations_error(conn) + else: + annotation_error = annotation_schema_error(conn, allow_missing=False) + if annotation_error: + annotation_error = ( + "Review queue schema requires a valid image annotation schema: " + + annotation_error + ) if annotation_error: - return ( - "Review queue schema requires a valid image annotation schema: " - + annotation_error - ) + return annotation_error present = {name for kind, name, _sql in objects if kind == "table"} if present != set(TABLES): return "Incomplete review queue schema" @@ -387,13 +421,14 @@ def review_schema_error( def migrate_review_state(conn: sqlite3.Connection) -> None: """Install the group atomically; validate before and after creating it. - Runs after ``migrate_annotation_state``: the group is only valid beside - the annotation group it cites. + Runs after ``migrate_annotation_state`` (which has just validated the + annotation group): the group is only valid beside the annotation group it + cites. A present group is validated once, evidence included. """ conn.execute("SAVEPOINT media_review_schema") try: installed = bool(_catalog(conn)) - error = review_schema_error(conn) + error = review_schema_error(conn, annotations_valid=True) if error: raise sqlite3.DatabaseError(error) if installed: @@ -410,7 +445,7 @@ def migrate_review_state(conn: sqlite3.Connection) -> None: "(singleton_id, schema_version, review_index_revision) VALUES (1, ?, 0)", (SCHEMA_VERSION,), ) - error = review_schema_error(conn, allow_missing=False) + error = review_schema_error(conn, allow_missing=False, annotations_valid=True) if error: raise sqlite3.DatabaseError(error) conn.execute("RELEASE media_review_schema") diff --git a/tests/test_review_schema.py b/tests/test_review_schema.py index dfd6140..034e26d 100644 --- a/tests/test_review_schema.py +++ b/tests/test_review_schema.py @@ -330,6 +330,43 @@ class ReviewSchemaTests(unittest.TestCase): conn.execute(f"DELETE FROM {ITEMS}") self.assertEqual(table_rows(conn, TABLES), snapshot) + def test_startup_validates_stored_evidence_once(self): + """A reopen streams the evidence once and the annotation group twice + (its own install), never again for the review group (I3).""" + self.require_module() + import database.annotation_schema as annotation_module + import database.review_schema as review_module + + model = self.model() + with model.get_connection() as conn: + insert_group_rows(conn) + model.close_all() + for _ in range(2): + PromptModel.reset_schema_cache() + with ( + patch.object( + review_module, + "_evidence_error", + wraps=review_module._evidence_error, + ) as evidence, + patch.object( + review_module, + "annotation_schema_error", + wraps=review_module.annotation_schema_error, + ) as nested, + patch.object( + annotation_module, + "_value_error", + wraps=annotation_module._value_error, + ) as annotation_rows, + ): + model = self.model() + model.get_connection() + self.assertEqual(evidence.call_count, 1) + self.assertEqual(nested.call_count, 0) + self.assertEqual(annotation_rows.call_count, 2) + model.close_all() + def test_old_database_without_group_migrates_without_touching_legacy_rows(self): self.require_module() for label, dropped in (