diff --git a/code_review_graph/graph.py b/code_review_graph/graph.py index 2dfa97f..2fa5c29 100644 --- a/code_review_graph/graph.py +++ b/code_review_graph/graph.py @@ -236,6 +236,13 @@ def store_file_nodes_edges( self, file_path: str, nodes: list[NodeInfo], edges: list[EdgeInfo], fhash: str = "" ) -> None: """Atomically replace all data for a file.""" + # Flush any implicit transaction started by prior DML (e.g. + # remove_file_data DELETEs during incremental_update/full_build). + # Python sqlite3 auto-begins a DEFERRED transaction on the first + # DML; BEGIN IMMEDIATE inside an active transaction raises + # OperationalError. See GitHub issue #135. + if self._conn.in_transaction: + self._conn.commit() self._conn.execute("BEGIN IMMEDIATE") try: self.remove_file_data(file_path) diff --git a/tests/test_graph.py b/tests/test_graph.py index 5923f57..8a2641f 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -242,3 +242,29 @@ def test_empty_changed_files(self): assert result["changed_nodes"] == [] assert result["impacted_nodes"] == [] assert result["total_impacted"] == 0 + + def test_store_after_remove_no_transaction_error(self): + """Regression test for #135: remove_file_data then store_file_nodes_edges + must not raise 'cannot start a transaction within a transaction'.""" + file_a = "/proj/a.py" + file_b = "/proj/b.py" + node_a = NodeInfo( + kind="File", name=file_a, file_path=file_a, + line_start=1, line_end=10, language="python", + ) + node_b = NodeInfo( + kind="File", name=file_b, file_path=file_b, + line_start=1, line_end=10, language="python", + ) + # Populate both files + self.store.store_file_nodes_edges(file_a, [node_a], [], "aaa") + self.store.store_file_nodes_edges(file_b, [node_b], [], "bbb") + + # Simulate incremental_update: delete file_a, then re-store file_b + self.store.remove_file_data(file_a) + # This must not raise sqlite3.OperationalError + self.store.store_file_nodes_edges(file_b, [node_b], [], "ccc") + + # Verify: file_a gone, file_b present + assert self.store.get_nodes_by_file(file_a) == [] + assert len(self.store.get_nodes_by_file(file_b)) == 1 diff --git a/tests/test_incremental.py b/tests/test_incremental.py index d7b3992..eeb9b51 100644 --- a/tests/test_incremental.py +++ b/tests/test_incremental.py @@ -241,6 +241,64 @@ def test_incremental_deleted_file(self, tmp_path): store.close() + def test_incremental_deleted_and_modified_files(self, tmp_path): + """Regression test for #135: incremental update with a mix of deleted + and modified files must not raise 'cannot start a transaction within + a transaction'.""" + db_path = tmp_path / "test.db" + store = GraphStore(db_path) + try: + # Create two files and build them into the graph + a = tmp_path / "a.py" + b = tmp_path / "b.py" + a.write_text("def fa():\n pass\n") + b.write_text("def fb():\n pass\n") + incremental_update(tmp_path, store, changed_files=["a.py", "b.py"]) + assert len(store.get_nodes_by_file(str(a))) > 0 + assert len(store.get_nodes_by_file(str(b))) > 0 + + # Delete a.py, modify b.py, then do incremental update + a.unlink() + b.write_text("def fb_v2():\n return 42\n") + # This must not raise sqlite3.OperationalError + incremental_update( + tmp_path, store, changed_files=["a.py", "b.py"] + ) + # a.py removed, b.py updated + assert store.get_nodes_by_file(str(a)) == [] + assert len(store.get_nodes_by_file(str(b))) > 0 + finally: + store.close() + + def test_full_build_stale_files_then_parse(self, tmp_path): + """Regression test for #135: full_build removing stale files then + parsing remaining files must not raise transaction errors.""" + (tmp_path / ".git").mkdir() + db_path = tmp_path / "test.db" + store = GraphStore(db_path) + try: + # Pre-populate with a file that will become stale + stale = tmp_path / "stale.py" + stale.write_text("x = 1\n") + mock_target = "code_review_graph.incremental.get_all_tracked_files" + with patch(mock_target, return_value=["stale.py"]): + full_build(tmp_path, store) + assert len(store.get_nodes_by_file(str(stale))) > 0 + + # Remove stale.py, add fresh.py + stale.unlink() + fresh = tmp_path / "fresh.py" + fresh.write_text("def new_func():\n pass\n") + with patch(mock_target, return_value=["fresh.py"]): + # This must not raise sqlite3.OperationalError + result = full_build(tmp_path, store) + assert result["files_parsed"] == 1 + assert store.get_nodes_by_file(str(stale)) == [] + assert len(store.get_nodes_by_file(str(fresh))) > 0 + finally: + store.close() + + class TestParallelParsing: def test_parse_single_file(self, tmp_path): py_file = tmp_path / "single.py"