mirror of
https://github.com/MODSetter/SurfSense.git
synced 2026-08-21 06:43:38 +00:00
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.
This commit is contained in:
parent
7ccc789b5e
commit
825522c06b
4 changed files with 248 additions and 5 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue