admin-valentin/open-notebook
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: persist source asset, preserve custom titles, cascade-delete credential models

Browse source 
- #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
This commit is contained in:
Luis Novo 2026-04-06 07:33:25 -03:00
parent c36782e5c5
commit e91a825f68
5 changed files with 342 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
.gitignore vendored
View file

@ -138,4 +138,7 @@ specs/
*.local.yml
**/*.local.md
**/*.local.md
.harness/
.mcp.json

20
api/routers/credentials.py
View file

@ -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=<credential_id> to reassign them
- Without either, returns 409 with linked model info
- Pass migrate_to=<credential_id> 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

13
api/routers/sources.py
View file

@ -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()

4
open_notebook/graphs/source.py
View file

@ -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()

321
tests/test_bug_fixes.py Normal file
View file

@ -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"])