From 083f0aa9cf310e79bba65e8a0817138fa54cc7ee Mon Sep 17 00:00:00 2001 From: Mohammad Hijjawi Date: Wed, 30 Sep 2026 23:52:25 +0100 Subject: [PATCH] fix(export): give each exported document its own path When two documents in one folder share a title, the second gets a _2 suffix. The suffix was not checked, so if another document was already titled "Notes_2", both were written to Notes_2.md: the ZIP held two entries with that name and the manifest listed the path twice. Import reads the last entry for both, so one document never arrived. Keep counting until the path is free. Also apply ruff format to the file, which the backend-lint hook checks for every changed file. --- .../app/services/export_service.py | 35 +++++----- .../services/test_export_document_paths.py | 69 +++++++++++++++++++ 2 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 surfsense_backend/tests/unit/services/test_export_document_paths.py diff --git a/surfsense_backend/app/services/export_service.py b/surfsense_backend/app/services/export_service.py index b79d82af44..defaba7148 100644 --- a/surfsense_backend/app/services/export_service.py +++ b/surfsense_backend/app/services/export_service.py @@ -326,13 +326,18 @@ async def _export_workspace_markdown( rel_path = f"{dir_path}/{base_name}.md" if dir_path else f"{base_name}.md" if rel_path in used_paths: - used_paths[rel_path] += 1 - suffix = used_paths[rel_path] - base_name = f"{base_name}_{suffix}" - rel_path = ( - f"{dir_path}/{base_name}.md" if dir_path else f"{base_name}.md" - ) - used_paths[rel_path] = used_paths.get(rel_path, 0) + 1 + # A suffixed name can already belong to another title + # ("Notes_2"), so keep counting until the path is free. + stem, taken_path = base_name, rel_path + suffix = used_paths[taken_path] + while rel_path in used_paths: + suffix += 1 + base_name = f"{stem}_{suffix}" + rel_path = ( + f"{dir_path}/{base_name}.md" if dir_path else f"{base_name}.md" + ) + used_paths[taken_path] = suffix + used_paths[rel_path] = 1 zip_rel = _join_zip_path(path_prefix, rel_path) entries.append((zip_rel, document_to_concept(doc, body=markdown))) @@ -347,9 +352,7 @@ async def _export_workspace_markdown( ) metadata = ( - doc.document_metadata - if isinstance(doc.document_metadata, dict) - else {} + doc.document_metadata if isinstance(doc.document_metadata, dict) else {} ) description = metadata.get("description") dir_concepts.setdefault(dir_path, []).append( @@ -426,7 +429,9 @@ async def build_export_zip( folder_result = await session.execute( select(Folder).where(Folder.workspace_id == workspace_id) ) - folder_path_map = _build_folder_path_map(list(folder_result.scalars().all())) + folder_path_map = _build_folder_path_map( + list(folder_result.scalars().all()) + ) export_name = "knowledge-base" if folder_id is not None and folder_id in folder_path_map: @@ -510,9 +515,7 @@ async def flatten_workspace_chats( exported: list[dict[str, Any]] = [] for thread in threads: messages_out: list[dict[str, Any]] = [] - ordered = sorted( - thread.messages, key=lambda item: (item.created_at, item.id) - ) + ordered = sorted(thread.messages, key=lambda item: (item.created_at, item.id)) for message in ordered: role = _role_value(message.role) if role not in _CHAT_ROLES: @@ -566,9 +569,7 @@ def _manifest_first_zip(staging_path: str, manifest: dict[str, Any]) -> str: return final_path -async def build_account_export_zip( - session: AsyncSession, user_id: Any -) -> ExportResult: +async def build_account_export_zip(session: AsyncSession, user_id: Any) -> ExportResult: """Build a contract-3 ZIP of every workspace the user is a member of.""" workspaces = await _member_workspaces(session, user_id) fd, staging_path = tempfile.mkstemp(suffix=".zip") diff --git a/surfsense_backend/tests/unit/services/test_export_document_paths.py b/surfsense_backend/tests/unit/services/test_export_document_paths.py new file mode 100644 index 0000000000..5b29d9a123 --- /dev/null +++ b/surfsense_backend/tests/unit/services/test_export_document_paths.py @@ -0,0 +1,69 @@ +"""Contract-3 producer: every exported document gets a path of its own.""" + +from __future__ import annotations + +import json +import os +import zipfile +from types import SimpleNamespace + +import pytest + +from app.db import Document, DocumentType +from app.services.export_service import build_account_export_zip + +pytestmark = pytest.mark.unit + + +class _Rows: + def __init__(self, rows: list): + self._rows = rows + + def scalars(self): + return self + + def unique(self): + return self + + def all(self): + return self._rows + + +class _Session: + """Answers the exporter's queries in order: workspaces, folders, document + batches, then chat threads.""" + + def __init__(self, workspace, documents: list[Document]): + self._responses = [[workspace], [], documents, [], []] + + async def execute(self, _stmt): + return _Rows(self._responses.pop(0)) + + +def _note(doc_id: int, title: str) -> Document: + return Document( + id=doc_id, + title=title, + document_type=DocumentType.NOTE, + source_markdown=f"Body of document {doc_id}", + ) + + +async def test_suffixed_name_does_not_overwrite_a_title_that_already_has_it(): + workspace = SimpleNamespace(id=7, name="Research", created_at=None) + documents = [_note(1, "Notes"), _note(2, "Notes_2"), _note(3, "Notes")] + + result = await build_account_export_zip(_Session(workspace, documents), "user") + try: + with zipfile.ZipFile(result.zip_path) as archive: + manifest = json.loads(archive.read("manifest.json")) + listed = manifest["workspaces"][0]["documents"] + paths = [doc["path"] for doc in listed] + assert len(set(paths)) == len(paths) + for doc in listed: + assert ( + f"Body of document {doc['id']}" + in archive.read(doc["path"]).decode() + ) + finally: + os.unlink(result.zip_path)