From e91a825f6841cf6d7b826f699965b915af02a348 Mon Sep 17 00:00:00 2001 From: Luis Novo Date: Mon, 6 Apr 2026 07:33:25 -0300 Subject: [PATCH 1/3] fix: persist source asset, preserve custom titles, cascade-delete credential models - #627: Set source.asset (URL/file_path) before save() in async creation path so failed sources are identifiable and retry works - #670: Only overwrite source title if it's a placeholder ("Processing...") or empty, preserving user-set custom titles - #651: Cascade-delete linked models when credential is deleted instead of returning 409 Conflict; remove unused delete_models parameter - Add tests for all three fixes (12 new tests) - Add .harness and .mcp.json to .gitignore --- .gitignore | 5 +- api/routers/credentials.py | 20 +- api/routers/sources.py | 13 +- open_notebook/graphs/source.py | 4 +- tests/test_bug_fixes.py | 321 +++++++++++++++++++++++++++++++++ 5 files changed, 342 insertions(+), 21 deletions(-) create mode 100644 tests/test_bug_fixes.py diff --git a/.gitignore b/.gitignore index c280d0a..ac56086 100644 --- a/.gitignore +++ b/.gitignore @@ -138,4 +138,7 @@ specs/ *.local.yml -**/*.local.md \ No newline at end of file +**/*.local.md +.harness/ + +.mcp.json \ No newline at end of file diff --git a/api/routers/credentials.py b/api/routers/credentials.py index 59d0940..b3cf86f 100644 --- a/api/routers/credentials.py +++ b/api/routers/credentials.py @@ -248,7 +248,6 @@ async def update_credential(credential_id: str, request: UpdateCredentialRequest @router.delete("/{credential_id}", response_model=CredentialDeleteResponse) async def delete_credential( credential_id: str, - delete_models: bool = Query(False, description="Also delete linked models"), migrate_to: Optional[str] = Query( None, description="Migrate linked models to this credential ID" ), @@ -257,24 +256,13 @@ async def delete_credential( Delete a credential. If the credential has linked models: - - Pass delete_models=true to delete them - - Pass migrate_to= to reassign them - - Without either, returns 409 with linked model info + - Pass migrate_to= to reassign them to another credential + - Otherwise, linked models are cascade-deleted automatically """ try: cred = await Credential.get(credential_id) linked_models = await cred.get_linked_models() - if linked_models and not delete_models and not migrate_to: - raise HTTPException( - status_code=409, - detail={ - "message": f"Credential has {len(linked_models)} linked model(s)", - "model_ids": [m.id for m in linked_models], - "model_names": [f"{m.provider}/{m.name}" for m in linked_models], - }, - ) - deleted_models = 0 if linked_models and migrate_to: @@ -284,8 +272,8 @@ async def delete_credential( model.credential = target_cred.id await model.save() - elif linked_models and delete_models: - # Delete linked models + elif linked_models: + # Cascade-delete linked models (default behavior when no migrate_to) for model in linked_models: await model.delete() deleted_models += 1 diff --git a/api/routers/sources.py b/api/routers/sources.py index 70a1914..38e75a3 100644 --- a/api/routers/sources.py +++ b/api/routers/sources.py @@ -31,7 +31,7 @@ from api.models import ( from commands.source_commands import SourceProcessingInput from open_notebook.config import UPLOADS_FOLDER from open_notebook.database.repository import ensure_record_id, repo_query -from open_notebook.domain.notebook import Notebook, Source +from open_notebook.domain.notebook import Asset, Notebook, Source from open_notebook.domain.transformation import Transformation from open_notebook.exceptions import InvalidInputError @@ -353,10 +353,19 @@ async def create_source( # ASYNC PATH: Create source record first, then queue command logger.info("Using async processing path") - # Create minimal source record - let SurrealDB generate the ID + # Create source record with asset - let SurrealDB generate the ID + # Persist asset before save so it's available for retry if processing fails + if source_data.type == "link": + source_asset = Asset(url=source_data.url) + elif source_data.type == "upload": + source_asset = Asset(file_path=file_path or source_data.file_path) + else: + source_asset = None + source = Source( title=source_data.title or "Processing...", topics=[], + asset=source_asset, ) await source.save() diff --git a/open_notebook/graphs/source.py b/open_notebook/graphs/source.py index 145f747..617396b 100644 --- a/open_notebook/graphs/source.py +++ b/open_notebook/graphs/source.py @@ -106,8 +106,8 @@ async def save_source(state: SourceState) -> dict: source.asset = Asset(url=content_state.url, file_path=content_state.file_path) source.full_text = content_state.content - # Preserve existing title if none provided in processed content - if content_state.title: + # Preserve user-set title; only overwrite placeholder or empty titles + if content_state.title and (not source.title or source.title == "Processing..."): source.title = content_state.title await source.save() diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py new file mode 100644 index 0000000..1f7bfc6 --- /dev/null +++ b/tests/test_bug_fixes.py @@ -0,0 +1,321 @@ +""" +Tests for bug fixes #627 (asset persistence), #670 (title preservation), +and #651 (credential cascade delete). +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from fastapi.testclient import TestClient + +from open_notebook.domain.notebook import Asset, Source + + +@pytest.fixture +def client(): + """Create test client after environment variables have been cleared by conftest.""" + from api.main import app + + return TestClient(app) + + +# ============================================================================ +# TEST SUITE 1: #627 - Async source creation persists asset +# ============================================================================ + + +class TestAsyncSourceAssetPersistence: + """Tests for #627 - asset is persisted before async processing.""" + + def test_source_created_with_url_asset(self): + """Source created for link type has asset with url.""" + asset = Asset(url="https://example.com/article") + source = Source( + title="Processing...", + topics=[], + asset=asset, + ) + assert source.asset is not None + assert source.asset.url == "https://example.com/article" + assert source.asset.file_path is None + + def test_source_created_with_file_asset(self): + """Source created for upload type has asset with file_path.""" + asset = Asset(file_path="/tmp/uploads/video.mp4") + source = Source( + title="Processing...", + topics=[], + asset=asset, + ) + assert source.asset is not None + assert source.asset.file_path == "/tmp/uploads/video.mp4" + assert source.asset.url is None + + def test_source_created_without_asset_for_text(self): + """Source created for text type has no asset.""" + source = Source( + title="Processing...", + topics=[], + asset=None, + ) + assert source.asset is None + + def test_retry_with_url_asset(self): + """Retry endpoint can reconstruct content_state from url asset.""" + source = Source( + title="Processing...", + topics=[], + asset=Asset(url="https://example.com/video"), + ) + # Simulate what the retry endpoint does + content_state = {} + if source.asset: + if source.asset.file_path: + content_state = { + "file_path": source.asset.file_path, + "delete_source": False, + } + elif source.asset.url: + content_state = {"url": source.asset.url} + + assert content_state == {"url": "https://example.com/video"} + + def test_retry_with_file_asset(self): + """Retry endpoint can reconstruct content_state from file asset.""" + source = Source( + title="Processing...", + topics=[], + asset=Asset(file_path="/tmp/uploads/doc.pdf"), + ) + content_state = {} + if source.asset: + if source.asset.file_path: + content_state = { + "file_path": source.asset.file_path, + "delete_source": False, + } + elif source.asset.url: + content_state = {"url": source.asset.url} + + assert content_state == { + "file_path": "/tmp/uploads/doc.pdf", + "delete_source": False, + } + + +# ============================================================================ +# TEST SUITE 2: #670 - Custom title preservation +# ============================================================================ + + +class TestTitlePreservation: + """Tests for #670 - user-set titles are preserved after processing.""" + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) + async def test_custom_title_preserved(self, mock_save, mock_get): + """User-set title is NOT overwritten by content_state.title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = "My Custom Research Title" + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "video.mp4" + content_state.url = "https://example.com" + content_state.file_path = None + content_state.content = "Some content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + # Title should remain as the custom user title + assert mock_source.title == "My Custom Research Title" + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) + async def test_placeholder_title_replaced(self, mock_save, mock_get): + """Placeholder 'Processing...' title IS replaced by extracted title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = "Processing..." + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "Extracted Article Title" + content_state.url = "https://example.com" + content_state.file_path = None + content_state.content = "Some content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "Extracted Article Title" + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) + async def test_none_title_replaced(self, mock_save, mock_get): + """None title IS replaced by extracted title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = None + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "Extracted Title" + content_state.url = None + content_state.file_path = "/tmp/file.pdf" + content_state.content = "Content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "Extracted Title" + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) + async def test_empty_title_replaced(self, mock_save, mock_get): + """Empty string title IS replaced by extracted title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = "" + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "Extracted Title" + content_state.url = None + content_state.file_path = None + content_state.content = "Content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "Extracted Title" + + +# ============================================================================ +# TEST SUITE 3: #651 - Credential cascade delete +# ============================================================================ + + +class TestCredentialCascadeDelete: + """Tests for #651 - deleting credential cascade-deletes linked models.""" + + @pytest.mark.asyncio + @patch("api.routers.credentials.Credential.get") + async def test_cascade_delete_linked_models(self, mock_get, client): + """Deleting credential without options cascade-deletes linked models.""" + mock_model1 = AsyncMock() + mock_model1.id = "model:1" + mock_model1.provider = "openai" + mock_model1.name = "gpt-4" + + mock_model2 = AsyncMock() + mock_model2.id = "model:2" + mock_model2.provider = "openai" + mock_model2.name = "gpt-3.5-turbo" + + mock_cred = AsyncMock() + mock_cred.get_linked_models = AsyncMock( + return_value=[mock_model1, mock_model2] + ) + mock_cred.delete = AsyncMock() + mock_get.return_value = mock_cred + + response = client.delete("/api/credentials/cred:123") + + assert response.status_code == 200 + data = response.json() + assert data["deleted_models"] == 2 + assert data["message"] == "Credential deleted successfully" + + # Verify models were deleted + mock_model1.delete.assert_called_once() + mock_model2.delete.assert_called_once() + mock_cred.delete.assert_called_once() + + @pytest.mark.asyncio + @patch("api.routers.credentials.Credential.get") + async def test_delete_credential_no_linked_models(self, mock_get, client): + """Deleting credential with no linked models works cleanly.""" + mock_cred = AsyncMock() + mock_cred.get_linked_models = AsyncMock(return_value=[]) + mock_cred.delete = AsyncMock() + mock_get.return_value = mock_cred + + response = client.delete("/api/credentials/cred:123") + + assert response.status_code == 200 + data = response.json() + assert data["deleted_models"] == 0 + mock_cred.delete.assert_called_once() + + @pytest.mark.asyncio + @patch("api.routers.credentials.Credential.get") + async def test_migrate_models_instead_of_delete(self, mock_get, client): + """Passing migrate_to reassigns models instead of deleting them.""" + mock_model = AsyncMock() + mock_model.id = "model:1" + mock_model.credential = "cred:123" + mock_model.save = AsyncMock() + + mock_cred = AsyncMock() + mock_cred.get_linked_models = AsyncMock(return_value=[mock_model]) + mock_cred.delete = AsyncMock() + + mock_target_cred = AsyncMock() + mock_target_cred.id = "cred:456" + + # First call returns cred to delete, second returns target + mock_get.side_effect = [mock_cred, mock_target_cred] + + response = client.delete( + "/api/credentials/cred:123?migrate_to=cred:456" + ) + + assert response.status_code == 200 + data = response.json() + assert data["deleted_models"] == 0 # Models were migrated, not deleted + mock_model.save.assert_called_once() + assert mock_model.credential == "cred:456" + mock_cred.delete.assert_called_once() + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) From 18a7cab36f785f1f9ba5820be93f3e5b97f2acf2 Mon Sep 17 00:00:00 2001 From: Luis Novo Date: Mon, 6 Apr 2026 07:42:20 -0300 Subject: [PATCH 2/3] fix: improve test quality for #627 and #651 - #627: Replace model-construction tests with endpoint-level tests that exercise the real create_source async path via TestClient, capturing the Source instance passed to save() using patch.object(autospec=True) - #651: Use assert_awaited_once() instead of assert_called_once() on AsyncMock methods to catch missing await bugs - Remove redundant class-level @patch for Source.save in title tests --- tests/test_bug_fixes.py | 205 ++++++++++++++++++++++++---------------- 1 file changed, 123 insertions(+), 82 deletions(-) diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py index 1f7bfc6..3ba13d5 100644 --- a/tests/test_bug_fixes.py +++ b/tests/test_bug_fixes.py @@ -25,83 +25,126 @@ def client(): class TestAsyncSourceAssetPersistence: - """Tests for #627 - asset is persisted before async processing.""" + """Tests for #627 - asset is persisted before async processing. - def test_source_created_with_url_asset(self): - """Source created for link type has asset with url.""" - asset = Asset(url="https://example.com/article") - source = Source( - title="Processing...", - topics=[], - asset=asset, - ) + These tests hit the real create_source endpoint with mocked DB/command + calls, verifying that the Source saved to the database has the correct + asset set *before* async processing begins. + """ + + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + async def test_async_link_source_persists_url_asset( + self, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=link and async_processing=true persists Asset(url=...).""" + mock_nb_get.return_value = MagicMock() # notebook exists + mock_submit.return_value = "command:123" + + # Track the Source instance that save() is called on + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "link", + "url": "https://example.com/article", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + # The first save should have the asset with URL + source = saved_sources[0] assert source.asset is not None assert source.asset.url == "https://example.com/article" assert source.asset.file_path is None - def test_source_created_with_file_asset(self): - """Source created for upload type has asset with file_path.""" - asset = Asset(file_path="/tmp/uploads/video.mp4") - source = Source( - title="Processing...", - topics=[], - asset=asset, - ) + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + @patch("api.routers.sources.save_uploaded_file", new_callable=AsyncMock) + async def test_async_upload_source_persists_file_asset( + self, mock_upload, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=upload and async_processing=true persists Asset(file_path=...).""" + mock_nb_get.return_value = MagicMock() + mock_upload.return_value = "/tmp/uploads/video.mp4" + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "upload", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + files={"file": ("video.mp4", b"fake content", "video/mp4")}, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] assert source.asset is not None assert source.asset.file_path == "/tmp/uploads/video.mp4" assert source.asset.url is None - def test_source_created_without_asset_for_text(self): - """Source created for text type has no asset.""" - source = Source( - title="Processing...", - topics=[], - asset=None, - ) + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + async def test_async_text_source_has_no_asset( + self, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=text and async_processing=true has asset=None.""" + mock_nb_get.return_value = MagicMock() + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "text", + "content": "Some text content", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] assert source.asset is None - def test_retry_with_url_asset(self): - """Retry endpoint can reconstruct content_state from url asset.""" - source = Source( - title="Processing...", - topics=[], - asset=Asset(url="https://example.com/video"), - ) - # Simulate what the retry endpoint does - content_state = {} - if source.asset: - if source.asset.file_path: - content_state = { - "file_path": source.asset.file_path, - "delete_source": False, - } - elif source.asset.url: - content_state = {"url": source.asset.url} - - assert content_state == {"url": "https://example.com/video"} - - def test_retry_with_file_asset(self): - """Retry endpoint can reconstruct content_state from file asset.""" - source = Source( - title="Processing...", - topics=[], - asset=Asset(file_path="/tmp/uploads/doc.pdf"), - ) - content_state = {} - if source.asset: - if source.asset.file_path: - content_state = { - "file_path": source.asset.file_path, - "delete_source": False, - } - elif source.asset.url: - content_state = {"url": source.asset.url} - - assert content_state == { - "file_path": "/tmp/uploads/doc.pdf", - "delete_source": False, - } - # ============================================================================ # TEST SUITE 2: #670 - Custom title preservation @@ -113,8 +156,7 @@ class TestTitlePreservation: @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_custom_title_preserved(self, mock_save, mock_get): + async def test_custom_title_preserved(self, mock_get): """User-set title is NOT overwritten by content_state.title.""" from open_notebook.graphs.source import save_source @@ -138,13 +180,12 @@ class TestTitlePreservation: await save_source(state) - # Title should remain as the custom user title assert mock_source.title == "My Custom Research Title" + mock_source.save.assert_awaited_once() @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_placeholder_title_replaced(self, mock_save, mock_get): + async def test_placeholder_title_replaced(self, mock_get): """Placeholder 'Processing...' title IS replaced by extracted title.""" from open_notebook.graphs.source import save_source @@ -169,11 +210,11 @@ class TestTitlePreservation: await save_source(state) assert mock_source.title == "Extracted Article Title" + mock_source.save.assert_awaited_once() @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_none_title_replaced(self, mock_save, mock_get): + async def test_none_title_replaced(self, mock_get): """None title IS replaced by extracted title.""" from open_notebook.graphs.source import save_source @@ -198,11 +239,11 @@ class TestTitlePreservation: await save_source(state) assert mock_source.title == "Extracted Title" + mock_source.save.assert_awaited_once() @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_empty_title_replaced(self, mock_save, mock_get): + async def test_empty_title_replaced(self, mock_get): """Empty string title IS replaced by extracted title.""" from open_notebook.graphs.source import save_source @@ -227,6 +268,7 @@ class TestTitlePreservation: await save_source(state) assert mock_source.title == "Extracted Title" + mock_source.save.assert_awaited_once() # ============================================================================ @@ -265,10 +307,9 @@ class TestCredentialCascadeDelete: assert data["deleted_models"] == 2 assert data["message"] == "Credential deleted successfully" - # Verify models were deleted - mock_model1.delete.assert_called_once() - mock_model2.delete.assert_called_once() - mock_cred.delete.assert_called_once() + mock_model1.delete.assert_awaited_once() + mock_model2.delete.assert_awaited_once() + mock_cred.delete.assert_awaited_once() @pytest.mark.asyncio @patch("api.routers.credentials.Credential.get") @@ -284,7 +325,7 @@ class TestCredentialCascadeDelete: assert response.status_code == 200 data = response.json() assert data["deleted_models"] == 0 - mock_cred.delete.assert_called_once() + mock_cred.delete.assert_awaited_once() @pytest.mark.asyncio @patch("api.routers.credentials.Credential.get") @@ -312,9 +353,9 @@ class TestCredentialCascadeDelete: assert response.status_code == 200 data = response.json() assert data["deleted_models"] == 0 # Models were migrated, not deleted - mock_model.save.assert_called_once() + mock_model.save.assert_awaited_once() assert mock_model.credential == "cred:456" - mock_cred.delete.assert_called_once() + mock_cred.delete.assert_awaited_once() if __name__ == "__main__": From bcec7e89ef80dd94960b1678f3ea5c4210b8b2c1 Mon Sep 17 00:00:00 2001 From: Luis Novo Date: Mon, 6 Apr 2026 07:45:49 -0300 Subject: [PATCH 3/3] refactor: move tests from test_bug_fixes.py to proper test modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Title preservation tests → test_graphs.py (TestSaveSourceTitlePreservation) - Source asset persistence tests → test_sources_api.py (new file) - Credential cascade delete tests → test_credentials_api.py (new file) - Delete test_bug_fixes.py --- tests/test_bug_fixes.py | 362 ---------------------------------- tests/test_credentials_api.py | 100 ++++++++++ tests/test_graphs.py | 128 ++++++++++++ tests/test_sources_api.py | 140 +++++++++++++ 4 files changed, 368 insertions(+), 362 deletions(-) delete mode 100644 tests/test_bug_fixes.py create mode 100644 tests/test_credentials_api.py create mode 100644 tests/test_sources_api.py diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py deleted file mode 100644 index 3ba13d5..0000000 --- a/tests/test_bug_fixes.py +++ /dev/null @@ -1,362 +0,0 @@ -""" -Tests for bug fixes #627 (asset persistence), #670 (title preservation), -and #651 (credential cascade delete). -""" - -from unittest.mock import AsyncMock, MagicMock, patch - -import pytest -from fastapi.testclient import TestClient - -from open_notebook.domain.notebook import Asset, Source - - -@pytest.fixture -def client(): - """Create test client after environment variables have been cleared by conftest.""" - from api.main import app - - return TestClient(app) - - -# ============================================================================ -# TEST SUITE 1: #627 - Async source creation persists asset -# ============================================================================ - - -class TestAsyncSourceAssetPersistence: - """Tests for #627 - asset is persisted before async processing. - - These tests hit the real create_source endpoint with mocked DB/command - calls, verifying that the Source saved to the database has the correct - asset set *before* async processing begins. - """ - - @pytest.mark.asyncio - @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) - @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) - @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) - async def test_async_link_source_persists_url_asset( - self, mock_nb_get, mock_add_nb, mock_submit, client - ): - """POST /sources with type=link and async_processing=true persists Asset(url=...).""" - mock_nb_get.return_value = MagicMock() # notebook exists - mock_submit.return_value = "command:123" - - # Track the Source instance that save() is called on - saved_sources = [] - - async def capture_save(self_source): - saved_sources.append(self_source) - self_source.id = "source:fake" - self_source.command = None - - with patch.object(Source, "save", autospec=True, side_effect=capture_save): - response = client.post( - "/api/sources", - data={ - "type": "link", - "url": "https://example.com/article", - "notebooks": '["notebook:1"]', - "async_processing": "true", - }, - ) - - assert response.status_code == 200 - assert len(saved_sources) >= 1 - - # The first save should have the asset with URL - source = saved_sources[0] - assert source.asset is not None - assert source.asset.url == "https://example.com/article" - assert source.asset.file_path is None - - @pytest.mark.asyncio - @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) - @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) - @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) - @patch("api.routers.sources.save_uploaded_file", new_callable=AsyncMock) - async def test_async_upload_source_persists_file_asset( - self, mock_upload, mock_nb_get, mock_add_nb, mock_submit, client - ): - """POST /sources with type=upload and async_processing=true persists Asset(file_path=...).""" - mock_nb_get.return_value = MagicMock() - mock_upload.return_value = "/tmp/uploads/video.mp4" - mock_submit.return_value = "command:123" - - saved_sources = [] - - async def capture_save(self_source): - saved_sources.append(self_source) - self_source.id = "source:fake" - self_source.command = None - - with patch.object(Source, "save", autospec=True, side_effect=capture_save): - response = client.post( - "/api/sources", - data={ - "type": "upload", - "notebooks": '["notebook:1"]', - "async_processing": "true", - }, - files={"file": ("video.mp4", b"fake content", "video/mp4")}, - ) - - assert response.status_code == 200 - assert len(saved_sources) >= 1 - - source = saved_sources[0] - assert source.asset is not None - assert source.asset.file_path == "/tmp/uploads/video.mp4" - assert source.asset.url is None - - @pytest.mark.asyncio - @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) - @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) - @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) - async def test_async_text_source_has_no_asset( - self, mock_nb_get, mock_add_nb, mock_submit, client - ): - """POST /sources with type=text and async_processing=true has asset=None.""" - mock_nb_get.return_value = MagicMock() - mock_submit.return_value = "command:123" - - saved_sources = [] - - async def capture_save(self_source): - saved_sources.append(self_source) - self_source.id = "source:fake" - self_source.command = None - - with patch.object(Source, "save", autospec=True, side_effect=capture_save): - response = client.post( - "/api/sources", - data={ - "type": "text", - "content": "Some text content", - "notebooks": '["notebook:1"]', - "async_processing": "true", - }, - ) - - assert response.status_code == 200 - assert len(saved_sources) >= 1 - - source = saved_sources[0] - assert source.asset is None - - -# ============================================================================ -# TEST SUITE 2: #670 - Custom title preservation -# ============================================================================ - - -class TestTitlePreservation: - """Tests for #670 - user-set titles are preserved after processing.""" - - @pytest.mark.asyncio - @patch("open_notebook.graphs.source.Source.get") - async def test_custom_title_preserved(self, mock_get): - """User-set title is NOT overwritten by content_state.title.""" - from open_notebook.graphs.source import save_source - - mock_source = MagicMock(spec=Source) - mock_source.title = "My Custom Research Title" - mock_source.save = AsyncMock() - mock_get.return_value = mock_source - - content_state = MagicMock() - content_state.title = "video.mp4" - content_state.url = "https://example.com" - content_state.file_path = None - content_state.content = "Some content" - - state = { - "source_id": "source:123", - "content_state": content_state, - "embed": False, - "apply_transformations": [], - } - - await save_source(state) - - assert mock_source.title == "My Custom Research Title" - mock_source.save.assert_awaited_once() - - @pytest.mark.asyncio - @patch("open_notebook.graphs.source.Source.get") - async def test_placeholder_title_replaced(self, mock_get): - """Placeholder 'Processing...' title IS replaced by extracted title.""" - from open_notebook.graphs.source import save_source - - mock_source = MagicMock(spec=Source) - mock_source.title = "Processing..." - mock_source.save = AsyncMock() - mock_get.return_value = mock_source - - content_state = MagicMock() - content_state.title = "Extracted Article Title" - content_state.url = "https://example.com" - content_state.file_path = None - content_state.content = "Some content" - - state = { - "source_id": "source:123", - "content_state": content_state, - "embed": False, - "apply_transformations": [], - } - - await save_source(state) - - assert mock_source.title == "Extracted Article Title" - mock_source.save.assert_awaited_once() - - @pytest.mark.asyncio - @patch("open_notebook.graphs.source.Source.get") - async def test_none_title_replaced(self, mock_get): - """None title IS replaced by extracted title.""" - from open_notebook.graphs.source import save_source - - mock_source = MagicMock(spec=Source) - mock_source.title = None - mock_source.save = AsyncMock() - mock_get.return_value = mock_source - - content_state = MagicMock() - content_state.title = "Extracted Title" - content_state.url = None - content_state.file_path = "/tmp/file.pdf" - content_state.content = "Content" - - state = { - "source_id": "source:123", - "content_state": content_state, - "embed": False, - "apply_transformations": [], - } - - await save_source(state) - - assert mock_source.title == "Extracted Title" - mock_source.save.assert_awaited_once() - - @pytest.mark.asyncio - @patch("open_notebook.graphs.source.Source.get") - async def test_empty_title_replaced(self, mock_get): - """Empty string title IS replaced by extracted title.""" - from open_notebook.graphs.source import save_source - - mock_source = MagicMock(spec=Source) - mock_source.title = "" - mock_source.save = AsyncMock() - mock_get.return_value = mock_source - - content_state = MagicMock() - content_state.title = "Extracted Title" - content_state.url = None - content_state.file_path = None - content_state.content = "Content" - - state = { - "source_id": "source:123", - "content_state": content_state, - "embed": False, - "apply_transformations": [], - } - - await save_source(state) - - assert mock_source.title == "Extracted Title" - mock_source.save.assert_awaited_once() - - -# ============================================================================ -# TEST SUITE 3: #651 - Credential cascade delete -# ============================================================================ - - -class TestCredentialCascadeDelete: - """Tests for #651 - deleting credential cascade-deletes linked models.""" - - @pytest.mark.asyncio - @patch("api.routers.credentials.Credential.get") - async def test_cascade_delete_linked_models(self, mock_get, client): - """Deleting credential without options cascade-deletes linked models.""" - mock_model1 = AsyncMock() - mock_model1.id = "model:1" - mock_model1.provider = "openai" - mock_model1.name = "gpt-4" - - mock_model2 = AsyncMock() - mock_model2.id = "model:2" - mock_model2.provider = "openai" - mock_model2.name = "gpt-3.5-turbo" - - mock_cred = AsyncMock() - mock_cred.get_linked_models = AsyncMock( - return_value=[mock_model1, mock_model2] - ) - mock_cred.delete = AsyncMock() - mock_get.return_value = mock_cred - - response = client.delete("/api/credentials/cred:123") - - assert response.status_code == 200 - data = response.json() - assert data["deleted_models"] == 2 - assert data["message"] == "Credential deleted successfully" - - mock_model1.delete.assert_awaited_once() - mock_model2.delete.assert_awaited_once() - mock_cred.delete.assert_awaited_once() - - @pytest.mark.asyncio - @patch("api.routers.credentials.Credential.get") - async def test_delete_credential_no_linked_models(self, mock_get, client): - """Deleting credential with no linked models works cleanly.""" - mock_cred = AsyncMock() - mock_cred.get_linked_models = AsyncMock(return_value=[]) - mock_cred.delete = AsyncMock() - mock_get.return_value = mock_cred - - response = client.delete("/api/credentials/cred:123") - - assert response.status_code == 200 - data = response.json() - assert data["deleted_models"] == 0 - mock_cred.delete.assert_awaited_once() - - @pytest.mark.asyncio - @patch("api.routers.credentials.Credential.get") - async def test_migrate_models_instead_of_delete(self, mock_get, client): - """Passing migrate_to reassigns models instead of deleting them.""" - mock_model = AsyncMock() - mock_model.id = "model:1" - mock_model.credential = "cred:123" - mock_model.save = AsyncMock() - - mock_cred = AsyncMock() - mock_cred.get_linked_models = AsyncMock(return_value=[mock_model]) - mock_cred.delete = AsyncMock() - - mock_target_cred = AsyncMock() - mock_target_cred.id = "cred:456" - - # First call returns cred to delete, second returns target - mock_get.side_effect = [mock_cred, mock_target_cred] - - response = client.delete( - "/api/credentials/cred:123?migrate_to=cred:456" - ) - - assert response.status_code == 200 - data = response.json() - assert data["deleted_models"] == 0 # Models were migrated, not deleted - mock_model.save.assert_awaited_once() - assert mock_model.credential == "cred:456" - mock_cred.delete.assert_awaited_once() - - -if __name__ == "__main__": - pytest.main([__file__, "-v"]) diff --git a/tests/test_credentials_api.py b/tests/test_credentials_api.py new file mode 100644 index 0000000..c22f2cf --- /dev/null +++ b/tests/test_credentials_api.py @@ -0,0 +1,100 @@ +"""Tests for the credentials API endpoint.""" + +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi.testclient import TestClient + + +@pytest.fixture +def client(): + """Create test client after environment variables have been cleared by conftest.""" + from api.main import app + + return TestClient(app) + + +class TestCredentialCascadeDelete: + """Tests for #651 - deleting credential cascade-deletes linked models.""" + + @pytest.mark.asyncio + @patch("api.routers.credentials.Credential.get") + async def test_cascade_delete_linked_models(self, mock_get, client): + """Deleting credential without options cascade-deletes linked models.""" + mock_model1 = AsyncMock() + mock_model1.id = "model:1" + mock_model1.provider = "openai" + mock_model1.name = "gpt-4" + + mock_model2 = AsyncMock() + mock_model2.id = "model:2" + mock_model2.provider = "openai" + mock_model2.name = "gpt-3.5-turbo" + + mock_cred = AsyncMock() + mock_cred.get_linked_models = AsyncMock( + return_value=[mock_model1, mock_model2] + ) + mock_cred.delete = AsyncMock() + mock_get.return_value = mock_cred + + response = client.delete("/api/credentials/cred:123") + + assert response.status_code == 200 + data = response.json() + assert data["deleted_models"] == 2 + assert data["message"] == "Credential deleted successfully" + + mock_model1.delete.assert_awaited_once() + mock_model2.delete.assert_awaited_once() + mock_cred.delete.assert_awaited_once() + + @pytest.mark.asyncio + @patch("api.routers.credentials.Credential.get") + async def test_delete_credential_no_linked_models(self, mock_get, client): + """Deleting credential with no linked models works cleanly.""" + mock_cred = AsyncMock() + mock_cred.get_linked_models = AsyncMock(return_value=[]) + mock_cred.delete = AsyncMock() + mock_get.return_value = mock_cred + + response = client.delete("/api/credentials/cred:123") + + assert response.status_code == 200 + data = response.json() + assert data["deleted_models"] == 0 + mock_cred.delete.assert_awaited_once() + + @pytest.mark.asyncio + @patch("api.routers.credentials.Credential.get") + async def test_migrate_models_instead_of_delete(self, mock_get, client): + """Passing migrate_to reassigns models instead of deleting them.""" + mock_model = AsyncMock() + mock_model.id = "model:1" + mock_model.credential = "cred:123" + mock_model.save = AsyncMock() + + mock_cred = AsyncMock() + mock_cred.get_linked_models = AsyncMock(return_value=[mock_model]) + mock_cred.delete = AsyncMock() + + mock_target_cred = AsyncMock() + mock_target_cred.id = "cred:456" + + # First call returns cred to delete, second returns target + mock_get.side_effect = [mock_cred, mock_target_cred] + + response = client.delete( + "/api/credentials/cred:123?migrate_to=cred:456" + ) + + assert response.status_code == 200 + data = response.json() + assert data["deleted_models"] == 0 # Models were migrated, not deleted + mock_model.save.assert_awaited_once() + assert mock_model.credential == "cred:456" + mock_cred.delete.assert_awaited_once() + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) diff --git a/tests/test_graphs.py b/tests/test_graphs.py index 22661b4..259d095 100644 --- a/tests/test_graphs.py +++ b/tests/test_graphs.py @@ -6,9 +6,12 @@ without heavy mocking of the actual processing logic. """ from datetime import datetime +from unittest.mock import AsyncMock, MagicMock, patch import pytest +from open_notebook.domain.notebook import Source + from open_notebook.graphs.prompt import PatternChainState, graph from open_notebook.graphs.tools import get_current_timestamp from open_notebook.graphs.transformation import ( @@ -151,5 +154,130 @@ class TestTransformationGraph: assert hasattr(transformation_graph, "ainvoke") +# ============================================================================ +# TEST SUITE 4: Source Graph - Title Preservation +# ============================================================================ + + +class TestSaveSourceTitlePreservation: + """Test save_source node preserves user-set titles (#670).""" + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + async def test_custom_title_preserved(self, mock_get): + """User-set title is NOT overwritten by content_state.title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = "My Custom Research Title" + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "video.mp4" + content_state.url = "https://example.com" + content_state.file_path = None + content_state.content = "Some content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "My Custom Research Title" + mock_source.save.assert_awaited_once() + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + async def test_placeholder_title_replaced(self, mock_get): + """Placeholder 'Processing...' title IS replaced by extracted title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = "Processing..." + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "Extracted Article Title" + content_state.url = "https://example.com" + content_state.file_path = None + content_state.content = "Some content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "Extracted Article Title" + mock_source.save.assert_awaited_once() + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + async def test_none_title_replaced(self, mock_get): + """None title IS replaced by extracted title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = None + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "Extracted Title" + content_state.url = None + content_state.file_path = "/tmp/file.pdf" + content_state.content = "Content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "Extracted Title" + mock_source.save.assert_awaited_once() + + @pytest.mark.asyncio + @patch("open_notebook.graphs.source.Source.get") + async def test_empty_title_replaced(self, mock_get): + """Empty string title IS replaced by extracted title.""" + from open_notebook.graphs.source import save_source + + mock_source = MagicMock(spec=Source) + mock_source.title = "" + mock_source.save = AsyncMock() + mock_get.return_value = mock_source + + content_state = MagicMock() + content_state.title = "Extracted Title" + content_state.url = None + content_state.file_path = None + content_state.content = "Content" + + state = { + "source_id": "source:123", + "content_state": content_state, + "embed": False, + "apply_transformations": [], + } + + await save_source(state) + + assert mock_source.title == "Extracted Title" + mock_source.save.assert_awaited_once() + + if __name__ == "__main__": pytest.main([__file__, "-v"]) diff --git a/tests/test_sources_api.py b/tests/test_sources_api.py new file mode 100644 index 0000000..06db1c4 --- /dev/null +++ b/tests/test_sources_api.py @@ -0,0 +1,140 @@ +"""Tests for the sources API endpoint.""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from fastapi.testclient import TestClient + +from open_notebook.domain.notebook import Source + + +@pytest.fixture +def client(): + """Create test client after environment variables have been cleared by conftest.""" + from api.main import app + + return TestClient(app) + + +class TestAsyncSourceAssetPersistence: + """Tests for #627 - asset is persisted before async processing. + + These tests hit the real create_source endpoint with mocked DB/command + calls, verifying that the Source saved to the database has the correct + asset set *before* async processing begins. + """ + + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + async def test_async_link_source_persists_url_asset( + self, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=link and async_processing=true persists Asset(url=...).""" + mock_nb_get.return_value = MagicMock() + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "link", + "url": "https://example.com/article", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] + assert source.asset is not None + assert source.asset.url == "https://example.com/article" + assert source.asset.file_path is None + + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + @patch("api.routers.sources.save_uploaded_file", new_callable=AsyncMock) + async def test_async_upload_source_persists_file_asset( + self, mock_upload, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=upload and async_processing=true persists Asset(file_path=...).""" + mock_nb_get.return_value = MagicMock() + mock_upload.return_value = "/tmp/uploads/video.mp4" + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "upload", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + files={"file": ("video.mp4", b"fake content", "video/mp4")}, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] + assert source.asset is not None + assert source.asset.file_path == "/tmp/uploads/video.mp4" + assert source.asset.url is None + + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + async def test_async_text_source_has_no_asset( + self, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=text and async_processing=true has asset=None.""" + mock_nb_get.return_value = MagicMock() + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "text", + "content": "Some text content", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] + assert source.asset is None + + +if __name__ == "__main__": + pytest.main([__file__, "-v"])