diff --git a/.beads/.gitignore b/.beads/.gitignore index 374adb8..ce9166d 100644 --- a/.beads/.gitignore +++ b/.beads/.gitignore @@ -10,6 +10,8 @@ daemon.lock daemon.log daemon.pid bd.sock +.jsonl.lock +.migration-hint-ts # Local version tracking (prevents upgrade notification spam after git ops) .local_version diff --git a/openforge/db/fixtures/incremental.py b/openforge/db/fixtures/incremental.py index f2bd4a4..7e4c25b 100644 --- a/openforge/db/fixtures/incremental.py +++ b/openforge/db/fixtures/incremental.py @@ -107,6 +107,13 @@ def __init__(self, conn: connection, verbose: bool = False): self.current_fixture_files = ( set() ) # Will be populated when processing fixture data + # Blueprint IDs that were renamed in this load — used to skip the + # subsequent deprecation step so a renamed row isn't tombstoned and + # stripped of tags/images. Per-apply state: populated in + # `_handle_addition`'s rename branch and reset at the top of + # `_apply_changes_with_cursor` so it doesn't leak between fixtures + # when one loader instance processes a directory. + self._renamed_blueprint_ids = set() def _load_existing_blueprints(self, curs: cursor = None) -> Dict[str, Dict]: """Load existing blueprints from database for comparison. @@ -752,6 +759,9 @@ def _apply_changes_with_cursor(self, curs: cursor, changes: ComparisonResult): # Load existing blueprints for modification handling existing_blueprints = self._load_existing_blueprints(curs) + # Reset rename tracking for this load. + self._renamed_blueprint_ids = set() + # Track new blueprint IDs for version change linking new_blueprint_ids = {} new_blueprint_ids_by_md5 = {} @@ -798,6 +808,17 @@ def _handle_deprecation( deprecated_bp: Blueprint to deprecate successor_id: Optional ID of the successor blueprint (for version changes) """ + # If this blueprint was renamed in this same load, the row was + # already updated in place. Deprecating it now would tombstone the + # live row and strip its freshly-inserted tags/images. + if deprecated_bp.get("id") in self._renamed_blueprint_ids: + if self.verbose: + write_output( + f"Skipping deprecation of blueprint {deprecated_bp['id']} " + f"(renamed in this load)\n" + ) + return + # Check if a deprecated blueprint with this MD5 already exists if deprecated_bp.get("file_md5"): existing_deprecated = self._find_deprecated_blueprint_by_md5( @@ -897,12 +918,26 @@ def _handle_addition(self, curs: cursor, new_item: Dict): "adding to consolidated_paths\n" ) - # Update the blueprint's full_name to the new path + # Update the blueprint's full_name to the new path. + # Also resync blueprint_name + search_text and clear + # deprecated, since the underlying file is the same + # bytes under a new name. + new_file_name = os.path.basename(new_full_name) + words = self._get_words(new_item) + search_text = blueprint_sql.blueprint_search_text( + {"blueprint_name": new_file_name}, words + ) update_data = { "full_name": new_full_name, - "file_name": os.path.basename(new_full_name), + "file_name": new_file_name, + "blueprint_name": new_file_name, + "search_text": search_text, + "deprecated": False, } blueprint_sql.update_blueprint(curs, bp["id"], update_data) + # Mark this id as renamed so a later deprecation step + # in the same load won't tombstone it. + self._renamed_blueprint_ids.add(bp["id"]) # Also update tags and images from the new fixture data # First remove old tags and images @@ -1014,8 +1049,14 @@ def _handle_modification( blueprint_id = existing_bp["id"] - # Update blueprint data + # Update blueprint data. Recompute search_text alongside the column + # update because tag changes are a common modify trigger and tag + # words feed into search_text — without this, search_text drifts + # whenever tags are edited. bp_data = self._munge_blueprint(modified_item) + bp_data["search_text"] = blueprint_sql.blueprint_search_text( + bp_data, self._get_words(modified_item) + ) blueprint_sql.update_blueprint(curs, blueprint_id, bp_data) # Update tags (delete old, insert new) diff --git a/openforge/db/sql/blueprints.py b/openforge/db/sql/blueprints.py index 0984d82..c03761e 100644 --- a/openforge/db/sql/blueprints.py +++ b/openforge/db/sql/blueprints.py @@ -94,7 +94,7 @@ def get_deprecated_blueprints(curs: cursor) -> list[dict]: return results -def _blueprint_search_text(data: dict, words: list[str]) -> str: +def blueprint_search_text(data: dict, words: list[str]) -> str: retwords = set() retwords.update(words) retwords.update(re.split(r"[^a-zA-Z0-9]", data["blueprint_name"])) @@ -104,7 +104,7 @@ def _blueprint_search_text(data: dict, words: list[str]) -> str: def insert_blueprint( curs: cursor, data: dict, rescue_md5_conflict: bool = False, words: list[str] = [] ) -> dict: - data["search_text"] = _blueprint_search_text(data, words) + data["search_text"] = blueprint_search_text(data, words) query_list = [ sql.SQL( """ @@ -191,6 +191,7 @@ def update_blueprint(curs: cursor, blueprint_id: uuid.UUID, data: dict) -> dict: "full_name", "file_modified_at", "storage_address", + "search_text", # Phase 1 fields "consolidated_paths", "deprecated", diff --git a/tests/test_incremental_fixtures.py b/tests/test_incremental_fixtures.py index a7bcd0c..fce61fd 100644 --- a/tests/test_incremental_fixtures.py +++ b/tests/test_incremental_fixtures.py @@ -2,7 +2,7 @@ Tests for incremental fixtures loading functionality. """ -from unittest.mock import Mock +from unittest.mock import Mock, patch import pytest @@ -499,3 +499,137 @@ def test_comparison_result_summary(self): assert "1 modified" in summary assert "1 deprecated" in summary assert result.has_changes() + + # ------------------------------------------------------------------ + # Rename-within-same-fixture regression tests + # ------------------------------------------------------------------ + # These tests pin down the bug where renaming a file in Dropbox + # (same MD5, new full_name) corrupted the DB row: blueprint_name and + # search_text went stale, the row got tombstoned in the same load, + # and tags/images were deleted. + + def _rename_loader(self): + """Build a loader primed for the same-fixture rename branch.""" + mock_conn = create_mock_connection() + loader = IncrementalFixturesLoader(mock_conn, verbose=False) + # Trigger the is_rename=True branch in _handle_addition: both + # paths must share the subset path, and the existing path must + # not be in the current fixture files set. + loader.fixture_subset_path = "tiles/aztlan" + loader.current_fixture_files = { + "tiles/aztlan/floors/floor/openforge/aztlan#floor.1x1.openforge.stl" + } + return loader + + def test_rename_resyncs_blueprint_name_and_clears_deprecated(self): + """Rename branch must resync blueprint_name + search_text and + clear deprecated, not just full_name/file_name.""" + loader = self._rename_loader() + + # Existing DB row uses the old (typo'd) name; insert returns it + # as the rescued md5 conflict. + existing_bp = { + "id": "bp-1", + "full_name": ( + "tiles/aztlan/floors/floor/openforge/atzlan#floor.1x1.openforge.stl" + ), + } + + new_item = create_mock_fixture_item( + "tiles/aztlan/floors/floor/openforge/aztlan#floor.1x1.openforge.stl", + "md5-shared", + tags=[["texture", "aztlan"], ["shape", "floor"]], + ) + # _munge_blueprint reads file_metadata["file"]; create_mock_fixture_item + # omits it, so add the basename here. + new_item["file_metadata"]["file"] = "aztlan#floor.1x1.openforge.stl" + + captured = {} + + def fake_update(curs, blueprint_id, data): + captured["id"] = blueprint_id + captured["data"] = data + + with ( + patch( + "openforge.db.fixtures.incremental.blueprint_sql.insert_blueprint", + return_value=existing_bp, + ), + patch( + "openforge.db.fixtures.incremental.blueprint_sql.update_blueprint", + side_effect=fake_update, + ), + patch( + "openforge.db.fixtures.incremental.tag_sql.delete_all_blueprint_tags" + ), + patch("openforge.db.fixtures.incremental.tag_sql.insert_tag"), + patch( + "openforge.db.fixtures.incremental.image_sql" + ".delete_images_for_blueprint" + ), + patch( + "openforge.db.fixtures.incremental.image_sql.insert_image_for_blueprint" + ), + ): + loader._handle_addition(Mock(), new_item) + + # The rename id is tracked so the deprecation step skips it. + assert "bp-1" in loader._renamed_blueprint_ids + # All four corruption-prone fields must be in the update payload. + assert captured["data"]["full_name"] == ( + "tiles/aztlan/floors/floor/openforge/aztlan#floor.1x1.openforge.stl" + ) + assert captured["data"]["file_name"] == "aztlan#floor.1x1.openforge.stl" + assert captured["data"]["blueprint_name"] == ("aztlan#floor.1x1.openforge.stl") + assert captured["data"]["deprecated"] is False + # search_text reflects the new name and tag words, not the old. + search_text = captured["data"]["search_text"] + assert "aztlan" in search_text.split() + assert "atzlan" not in search_text.split() + assert "floor" in search_text.split() + + def test_renamed_id_skips_deprecation(self): + """A bp id added to _renamed_blueprint_ids during a load must + not be re-deprecated by _handle_deprecation in the same load.""" + loader = self._rename_loader() + loader._renamed_blueprint_ids.add("bp-1") + + deprecated_bp = {"id": "bp-1", "file_md5": "md5-shared"} + + with ( + patch( + "openforge.db.fixtures.incremental.blueprint_sql" + ".mark_blueprint_deprecated" + ) as mark_dep, + patch( + "openforge.db.fixtures.incremental.tag_sql.delete_all_blueprint_tags" + ) as del_tags, + patch( + "openforge.db.fixtures.incremental.image_sql" + ".delete_images_for_blueprint" + ) as del_imgs, + ): + loader._handle_deprecation(Mock(), deprecated_bp) + + # The skip path means none of the deprecation side-effects fire. + mark_dep.assert_not_called() + del_tags.assert_not_called() + del_imgs.assert_not_called() + + def test_apply_changes_resets_renamed_ids_per_load(self): + """_renamed_blueprint_ids must reset at the top of each apply so + state from one fixture doesn't leak into the next.""" + loader = self._rename_loader() + loader._renamed_blueprint_ids.add("stale-id-from-prior-load") + + empty_changes = ComparisonResult() + + # _link_deprecated_to_successors runs at the end and calls + # cursor.fetchall(); make it return an empty list. + cursor = Mock() + cursor.fetchall = Mock(return_value=[]) + + with patch.object(loader, "_load_existing_blueprints", return_value={}): + loader._apply_changes_with_cursor(cursor, empty_changes) + + assert loader._renamed_blueprint_ids == set()