From 825522c06b065b4ad992f0792bafabf5ce6e7c19 Mon Sep 17 00:00:00 2001 From: CREDO23 Date: Wed, 5 Aug 2026 23:50:23 +0200 Subject: [PATCH] feat(knowledge-store): route folder CRUD/move through the facade Folder create, rename, move and delete now record to git after the row op, through thin module verbs (record_created_folder, record_moved_folder, record_removed_folder, folder_virtual_path). Routes never spell a path or bind a workspace; every verb self-guards, so an unflipped workspace is untouched. Rename and move capture the old path before mutating the row, then record the move: the row is already at its new name, so the in-place reparent no-ops and git still follows, id kept. Delete drops only the folder's .keep markers (remove_folder_markers), never its files. The incremental indexer prunes a row the moment its file leaves the tree, so removing documents here would race the purge task that owns their chunks and blobs. The markers are the resurrection gap; the purge owns the documents. --- .../08-store-facade-and-paths.md | 15 ++- .../app/knowledge_store/service.py | 93 ++++++++++++++++++- .../app/routes/folders_routes.py | 52 ++++++++++- .../knowledge_store/test_folders.py | 93 +++++++++++++++++++ 4 files changed, 248 insertions(+), 5 deletions(-) diff --git a/plans/git-native-kb/08-store-facade-and-paths.md b/plans/git-native-kb/08-store-facade-and-paths.md index 7209874e6..1f0d519ec 100644 --- a/plans/git-native-kb/08-store-facade-and-paths.md +++ b/plans/git-native-kb/08-store-facade-and-paths.md @@ -219,9 +219,18 @@ Ordered so the tree is safe before the fleet touches it. folder id and its children ride along on `parent_id`; and the seed materializes each empty leaf folder as a `.keep` (`migrate.py:_empty_folder_keeps`), so a flipped workspace's whole-workspace reconcile no longer prunes pre-existing empty folders. - **Still deferred:** the route rewire itself (point `folders_routes` handlers at the - facade verbs). Making the projection the *sole* row writer (stripping creation from - upload/notes/connectors) remains the Phase-5 cut — unflipped prod still writes rows on +7d. ✅ **Routed folder CRUD/move through the facade.** `folders_routes` create, rename, + move and delete now record to git after the row op, through thin module verbs + (`record_created_folder`, `record_moved_folder`, `record_removed_folder`, + `folder_virtual_path`) — routes never spell a path or bind a workspace. Rename/move + capture the old path *before* mutating, then record the move; the in-place reparent + no-ops (the row is already at its new name) and git still follows, id kept. Delete + drops only the folder's `.keep` markers (`remove_folder_markers`), never its files: + the incremental indexer prunes a row the moment its file leaves the tree, so removing + documents here would race the purge task that owns their chunks and blobs. All verbs + self-guard, so an unflipped workspace is untouched. + **Still deferred:** making the projection the *sole* row writer (stripping creation + from upload/notes/connectors) — the Phase-5 cut; unflipped prod still writes rows on those paths. 8. ✅ **Folder projection + prune** — `index/folders.py` derives `folders` rows from document paths ∪ keep-files and prunes rows with neither. Runs on every folder diff --git a/surfsense_backend/app/knowledge_store/service.py b/surfsense_backend/app/knowledge_store/service.py index 7a80b334b..60ad6909a 100644 --- a/surfsense_backend/app/knowledge_store/service.py +++ b/surfsense_backend/app/knowledge_store/service.py @@ -55,7 +55,7 @@ from app.knowledge_store.transaction import Transaction if TYPE_CHECKING: from sqlalchemy.ext.asyncio import AsyncSession - from app.db import Document + from app.db import Document, Folder logger = logging.getLogger(__name__) @@ -556,6 +556,31 @@ class KnowledgeStore: await self._reconcile_folders(revision) return await self._outcome(revision) + async def remove_folder_markers(self, path: str) -> Outcome: + """Drop a folder's ``.keep`` markers, leaving its documents in place. + + The delete route hands document rows to the purge task, which clears + their chunks and blobs before dropping the rows. Removing their files + here would race that task: the indexer prunes a row the moment its file + leaves the tree, and the purge would then find nothing left to clean. So + this touches only the empty-folder markers, which no row hangs off, and + lets the purge own the documents. + """ + if not await knowledge_store_enabled_for(self._workspace_id): + return Outcome(revision=None) + from app.knowledge_store.paths import KEEP_FILE + + keeps = [ + p + for p in await self._subtree_paths(path) + if p.rsplit("/", 1)[-1] == KEEP_FILE + ] + revision = await self._commit_files( + files={}, removes=keeps, message=f"docs: delete folder {_leaf(path)}" + ) + await self._reconcile_folders(revision) + return await self._outcome(revision) + async def move_folder(self, source: str, destination: str) -> Outcome: """Move a folder and every descendant in one revision, ids preserved.""" if not await knowledge_store_enabled_for(self._workspace_id): @@ -952,5 +977,71 @@ async def record_moved_documents( return (await store.move_documents(documents)).revision +async def folder_virtual_path(session: AsyncSession, folder: Folder) -> str | None: + """The ``/documents`` path a folder row occupies, or ``None`` if unplaced. + + The one resolver a route reaches for, so a caller never spells a folder path + itself. Capture it before a rename mutates the row: git still holds the old + path, and the move needs both ends. + """ + from app.knowledge_store.paths import build_path_index + + index = await build_path_index( + session, folder.workspace_id, populate_occupants=False + ) + return index.folder_paths.get(folder.id) + + +async def record_created_folder( + session: AsyncSession, + folder: Folder, + *, + author_user_id: str | None = None, +) -> str | None: + """Materialize a new empty folder in git so a rebuild keeps it.""" + path = await folder_virtual_path(session, folder) + if path is None: + return None + store = ( + KnowledgeStore.for_workspace(folder.workspace_id) + .with_session(session) + .as_user(author_user_id) + ) + return (await store.create_folder(path)).revision + + +async def record_moved_folder( + session: AsyncSession, + workspace_id: int, + *, + source: str, + destination: str, + author_user_id: str | None = None, +) -> str | None: + """Move a folder's git subtree from ``source`` to ``destination``, id kept.""" + store = ( + KnowledgeStore.for_workspace(workspace_id) + .with_session(session) + .as_user(author_user_id) + ) + return (await store.move_folder(source, destination)).revision + + +async def record_removed_folder( + session: AsyncSession, + workspace_id: int, + *, + path: str, + author_user_id: str | None = None, +) -> str | None: + """Drop a deleted folder's ``.keep`` markers so an empty folder stays gone.""" + store = ( + KnowledgeStore.for_workspace(workspace_id) + .with_session(session) + .as_user(author_user_id) + ) + return (await store.remove_folder_markers(path)).revision + + async def drop_workspace_store(workspace_id: int | str) -> None: await KnowledgeStore.for_workspace(workspace_id).drop_workspace() diff --git a/surfsense_backend/app/routes/folders_routes.py b/surfsense_backend/app/routes/folders_routes.py index 68093b7e5..33594245d 100644 --- a/surfsense_backend/app/routes/folders_routes.py +++ b/surfsense_backend/app/routes/folders_routes.py @@ -9,7 +9,13 @@ from sqlalchemy.future import select from app.auth.context import AuthContext from app.db import Document, Folder, Permission, get_async_session -from app.knowledge_store.service import record_moved_documents +from app.knowledge_store.service import ( + folder_virtual_path, + record_created_folder, + record_moved_documents, + record_moved_folder, + record_removed_folder, +) from app.schemas import ( BulkDocumentMove, DocumentMove, @@ -33,6 +39,32 @@ from app.utils.rbac import check_permission router = APIRouter() +async def _record_folder_move( + session: AsyncSession, + folder: Folder, + source: str | None, + auth: AuthContext, +) -> None: + """Follow a committed folder rename or reparent in git, ids preserved. + + ``source`` is the path read before the row moved; ``folder`` is refreshed to + its new home. A no-op on an unflipped workspace, or when either end is + unplaced (the row carried no git presence yet). + """ + if source is None: + return + destination = await folder_virtual_path(session, folder) + if destination is None or destination == source: + return + await record_moved_folder( + session, + folder.workspace_id, + source=source, + destination=destination, + author_user_id=str(auth.user.id), + ) + + @router.post("/folders", response_model=FolderRead) async def create_folder( request: FolderCreate, @@ -76,6 +108,9 @@ async def create_folder( session.add(folder) await session.commit() await session.refresh(folder) + # Give the empty folder a git presence so a rebuild keeps it. A no-op on + # an unflipped workspace (the verb self-guards). + await record_created_folder(session, folder, author_user_id=str(user.id)) return folder except HTTPException: @@ -245,9 +280,11 @@ async def update_folder( "You don't have permission to update folders in this workspace", ) + source = await folder_virtual_path(session, folder) folder.name = request.name await session.commit() await session.refresh(folder) + await _record_folder_move(session, folder, source, auth) return folder except HTTPException: @@ -304,10 +341,12 @@ async def move_folder( position = await generate_folder_position( session, folder.workspace_id, request.new_parent_id ) + source = await folder_virtual_path(session, folder) folder.parent_id = request.new_parent_id folder.position = position await session.commit() await session.refresh(folder) + await _record_folder_move(session, folder, source, auth) return folder except HTTPException: @@ -386,6 +425,7 @@ async def delete_folder( "You don't have permission to delete folders in this workspace", ) + folder_path = await folder_virtual_path(session, folder) subtree_ids = await get_folder_subtree_ids(session, folder_id) doc_result = await session.execute( @@ -404,6 +444,16 @@ async def delete_folder( ) await session.commit() + # Drop the folder's empty markers now; the purge task owns its documents + # (and their chunks and blobs). A no-op on an unflipped workspace. + if folder_path is not None: + await record_removed_folder( + session, + folder.workspace_id, + path=folder_path, + author_user_id=str(auth.user.id), + ) + try: from app.tasks.celery_tasks.document_tasks import ( delete_folder_documents_task, diff --git a/surfsense_backend/tests/integration/knowledge_store/test_folders.py b/surfsense_backend/tests/integration/knowledge_store/test_folders.py index 98685967f..f7c36d703 100644 --- a/surfsense_backend/tests/integration/knowledge_store/test_folders.py +++ b/surfsense_backend/tests/integration/knowledge_store/test_folders.py @@ -221,6 +221,99 @@ async def test_move_folder_keeps_the_folder_id( assert renamed.id == child_before.parent_id +async def test_remove_folder_markers_keeps_documents( + knowledge_root, db_session, db_workspace, db_user +): + """Delete drops a folder's empty markers but leaves its files to the purge.""" + store = _store(db_workspace, db_session, db_user) + await store.create_folder("/documents/Docs/Empty") + body = "# Note\n\na body long enough for the indexer to embed and chunk\n" + await store.write("documents/Docs/note.md", body) + + outcome = await store.remove_folder_markers("/documents/Docs") + + paths = await _paths(store, outcome.revision) + assert "documents/Docs/note.md" in paths + assert not any(p.rsplit("/", 1)[-1] == KEEP_FILE for p in paths) + names = await _folder_names(db_session, db_workspace.id) + assert "Empty" not in names and "Docs" in names + + +async def test_record_created_folder_gives_a_row_git_presence( + knowledge_root, db_session, db_workspace, db_user +): + """The create route's contract: a committed row is materialized in git.""" + from app.knowledge_store.service import record_created_folder + from app.services.folder_service import ensure_folder_hierarchy + + store = _store(db_workspace, db_session, db_user) + await ensure_folder_hierarchy( + db_session, + workspace_id=db_workspace.id, + created_by_id=str(db_user.id), + folder_parts=["Fresh"], + ) + folder = ( + await db_session.execute( + select(Folder).where( + Folder.workspace_id == db_workspace.id, Folder.name == "Fresh" + ) + ) + ).scalar_one() + + await record_created_folder(db_session, folder, author_user_id=str(db_user.id)) + + assert f"documents/Fresh/{KEEP_FILE}" in await _paths(store, await store.head()) + + +async def test_route_rename_flow_keeps_the_folder_id( + knowledge_root, db_session, db_workspace, db_user +): + """The route renames the row, then records the move; git follows, id kept. + + Order matters: the row is already at its new name when the move records, so + the in-place reparent is a no-op and git still moves the subtree. + """ + from app.knowledge_store.service import folder_virtual_path, record_moved_folder + + store = _store(db_workspace, db_session, db_user) + await store.create_folder("/documents/Old") + folder = ( + await db_session.execute( + select(Folder).where( + Folder.workspace_id == db_workspace.id, Folder.name == "Old" + ) + ) + ).scalar_one() + + source = await folder_virtual_path(db_session, folder) + folder.name = "New" + await db_session.commit() + await db_session.refresh(folder) + destination = await folder_virtual_path(db_session, folder) + await record_moved_folder( + db_session, + db_workspace.id, + source=source, + destination=destination, + author_user_id=str(db_user.id), + ) + + assert source == "/documents/Old" + assert destination == "/documents/New" + paths = await _paths(store, await store.head()) + assert f"documents/New/{KEEP_FILE}" in paths + assert not any(p.startswith("documents/Old/") for p in paths) + renamed = ( + await db_session.execute( + select(Folder).where( + Folder.workspace_id == db_workspace.id, Folder.name == "New" + ) + ) + ).scalar_one() + assert renamed.id == folder.id + + def test_keep_is_rejected_as_a_document_path(): with pytest.raises(StorePathError): StorePath.from_virtual(f"/documents/x/{KEEP_FILE}")