From 3fbd9e1440b4f9143b1f7c6bf7dad2451b774f19 Mon Sep 17 00:00:00 2001 From: Ayush8923 <80516839+Ayush8923@users.noreply.github.com> Date: Thu, 21 May 2026 22:12:58 +0530 Subject: [PATCH 1/5] fix(collection): added the field in collection endpoint --- backend/app/models/collection.py | 8 ++++++++ backend/app/services/collections/helpers.py | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/backend/app/models/collection.py b/backend/app/models/collection.py index ccd606deb..7c91793f4 100644 --- a/backend/app/models/collection.py +++ b/backend/app/models/collection.py @@ -201,6 +201,14 @@ class CollectionIDPublic(SQLModel): class CollectionPublic(SQLModel): id: UUID + name: str | None = Field( + default=None, + description="Name of the collection", + ) + description: str | None = Field( + default=None, + description="Description of the collection", + ) llm_service_id: str | None = Field( default=None, description="LLM service ID (e.g., Assistant ID) when model and instructions were provided", diff --git a/backend/app/services/collections/helpers.py b/backend/app/services/collections/helpers.py index 6985ac78e..440d6be94 100644 --- a/backend/app/services/collections/helpers.py +++ b/backend/app/services/collections/helpers.py @@ -171,6 +171,8 @@ def to_collection_public(collection: Collection) -> CollectionPublic: if is_vector_store: return CollectionPublic( id=collection.id, + name=collection.name, + description=collection.description, knowledge_base_id=collection.llm_service_id, knowledge_base_provider=collection.llm_service_name, project_id=collection.project_id, @@ -181,6 +183,8 @@ def to_collection_public(collection: Collection) -> CollectionPublic: else: return CollectionPublic( id=collection.id, + name=collection.name, + description=collection.description, llm_service_id=collection.llm_service_id, llm_service_name=collection.llm_service_name, project_id=collection.project_id, From f099b1501915829c4688a6ba21afb97719117ab5 Mon Sep 17 00:00:00 2001 From: Ayush8923 <80516839+Ayush8923@users.noreply.github.com> Date: Fri, 22 May 2026 09:42:02 +0530 Subject: [PATCH 2/5] fix(collection): added the edit collection api endpoint --- backend/app/api/docs/collections/update.md | 10 +++++++ backend/app/api/routes/collections.py | 32 ++++++++++++++++++++++ backend/app/crud/collection/collection.py | 21 +++++++++++++- backend/app/models/__init__.py | 1 + backend/app/models/collection.py | 11 ++++++++ 5 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 backend/app/api/docs/collections/update.md diff --git a/backend/app/api/docs/collections/update.md b/backend/app/api/docs/collections/update.md new file mode 100644 index 000000000..30e11c295 --- /dev/null +++ b/backend/app/api/docs/collections/update.md @@ -0,0 +1,10 @@ +Update the editable fields of an existing collection. + +You can update the collection's `name` and/or `description`. Both fields are optional in the request body — only the fields you include will be updated. Fields you omit (or send as `null`) are left unchanged. + +**Behavior:** + +- `name`: Must be unique within the project. If the new name is already in use by another active collection in the same project, the request fails with `409 Conflict`. Sending the same name the collection already has is a no-op (no conflict raised). +- `description`: Free-form text. Send an empty string `""` to clear it. + +Other collection fields (such as `llm_service_id`, `provider`, documents, etc.) cannot be modified through this endpoint. diff --git a/backend/app/api/routes/collections.py b/backend/app/api/routes/collections.py index 96272d38c..c612c5dc2 100644 --- a/backend/app/api/routes/collections.py +++ b/backend/app/api/routes/collections.py @@ -27,6 +27,7 @@ CallbackRequest, DeletionRequest, CollectionPublic, + CollectionUpdate, ) from app.utils import APIResponse, load_description, validate_callback_url from app.services.collections.helpers import ensure_unique_name, to_collection_public @@ -201,6 +202,37 @@ def delete_collection( ) +@router.patch( + "/{collection_id}", + description=load_description("collections/update.md"), + response_model=APIResponse[CollectionPublic], + dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], +) +def update_collection( + session: SessionDep, + current_user: AuthContextDep, + patch: CollectionUpdate, + collection_id: UUID = FastPath(description="Collection to update"), +): + with log_context( + tag="collection", + system="collection", + lifecycle="api.collection.update", + action="update", + collection_id=collection_id, + project_id=current_user.project_.id, + organization_id=current_user.organization_.id, + ): + collection_crud = CollectionCrud(session, current_user.project_.id) + collection = collection_crud.update(collection_id, patch) + + logger.info( + f"[update_collection] Collection updated | {{'collection_id': '{collection_id}'}}" + ) + + return APIResponse.success_response(to_collection_public(collection)) + + @router.get( "/{collection_id}", description=load_description("collections/info.md"), diff --git a/backend/app/crud/collection/collection.py b/backend/app/crud/collection/collection.py index cb8b6e27d..b8396d4ef 100644 --- a/backend/app/crud/collection/collection.py +++ b/backend/app/crud/collection/collection.py @@ -8,7 +8,7 @@ from sqlmodel import Session, select, and_ from sqlalchemy.exc import IntegrityError -from app.models import Document, Collection, DocumentCollection +from app.models import Document, Collection, CollectionUpdate, DocumentCollection from app.core.util import now from app.crud.document_collection import DocumentCollectionCrud @@ -93,6 +93,25 @@ def read_all(self): collections = self.session.exec(statement).all() return collections + def update(self, collection_id: UUID, patch: CollectionUpdate) -> Collection: + """Update editable fields of a collection (name, description).""" + collection = self.read_one(collection_id) + + changes = patch.model_dump(exclude_unset=True, exclude_none=True) + + if "name" in changes and changes["name"] != collection.name: + if self.exists_by_name(changes["name"]): + raise HTTPException( + status_code=409, + detail=f"Collection '{changes['name']}' already exists. Choose a different name.", + ) + + for field, value in changes.items(): + setattr(collection, field, value) + + collection.updated_at = now() + return self._update(collection) + def exists_by_name(self, collection_name: str) -> bool: statement = ( select(Collection.id) diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index b8eec6cde..3d9a2c4c6 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -31,6 +31,7 @@ Collection, CollectionIDPublic, CollectionPublic, + CollectionUpdate, CollectionWithDocsPublic, CreationRequest, DeletionRequest, diff --git a/backend/app/models/collection.py b/backend/app/models/collection.py index 7c91793f4..19b1f5ace 100644 --- a/backend/app/models/collection.py +++ b/backend/app/models/collection.py @@ -192,6 +192,17 @@ class DeletionRequest(CallbackRequest): collection_id: UUID = Field(description="Collection to delete") +class CollectionUpdate(SQLModel): + name: str | None = Field( + default=None, + description="New name for the collection", + ) + description: str | None = Field( + default=None, + description="New description for the collection", + ) + + # Response models From 72a9b3530dc08a9f82080f04eb6634968f15d355 Mon Sep 17 00:00:00 2001 From: Ayush8923 <80516839+Ayush8923@users.noreply.github.com> Date: Fri, 22 May 2026 09:48:55 +0530 Subject: [PATCH 3/5] fix(review): implement coderabbit suggestion --- backend/app/api/routes/collections.py | 2 +- backend/app/crud/collection/collection.py | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/backend/app/api/routes/collections.py b/backend/app/api/routes/collections.py index c612c5dc2..ae131dd7b 100644 --- a/backend/app/api/routes/collections.py +++ b/backend/app/api/routes/collections.py @@ -213,7 +213,7 @@ def update_collection( current_user: AuthContextDep, patch: CollectionUpdate, collection_id: UUID = FastPath(description="Collection to update"), -): +) -> APIResponse[CollectionPublic]: with log_context( tag="collection", system="collection", diff --git a/backend/app/crud/collection/collection.py b/backend/app/crud/collection/collection.py index b8396d4ef..0a5d80f36 100644 --- a/backend/app/crud/collection/collection.py +++ b/backend/app/crud/collection/collection.py @@ -110,7 +110,14 @@ def update(self, collection_id: UUID, patch: CollectionUpdate) -> Collection: setattr(collection, field, value) collection.updated_at = now() - return self._update(collection) + try: + return self._update(collection) + except IntegrityError: + self.session.rollback() + raise HTTPException( + status_code=409, + detail="Collection name already exists. Choose a different name.", + ) def exists_by_name(self, collection_name: str) -> bool: statement = ( From 3f84995d67a8c111bce70af1c78d4b4bd1a4c409 Mon Sep 17 00:00:00 2001 From: Ayush8923 <80516839+Ayush8923@users.noreply.github.com> Date: Fri, 22 May 2026 10:46:21 +0530 Subject: [PATCH 4/5] fix(collection): added the test cases for collection --- .../collections/test_collection_update.py | 91 ++++++++++++++++++ .../collection/test_crud_collection_update.py | 93 +++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 backend/app/tests/api/routes/collections/test_collection_update.py create mode 100644 backend/app/tests/crud/collections/collection/test_crud_collection_update.py diff --git a/backend/app/tests/api/routes/collections/test_collection_update.py b/backend/app/tests/api/routes/collections/test_collection_update.py new file mode 100644 index 000000000..7c2762570 --- /dev/null +++ b/backend/app/tests/api/routes/collections/test_collection_update.py @@ -0,0 +1,91 @@ +from uuid import uuid4 + +from fastapi.testclient import TestClient +from sqlmodel import Session + +from app.core.config import settings +from app.crud import CollectionCrud +from app.models import CollectionUpdate +from app.tests.utils.utils import get_project +from app.tests.utils.collection import get_assistant_collection + + +def test_update_collection_returns_updated_fields( + client: TestClient, + db: Session, + user_api_key_header: dict[str, str], +) -> None: + project = get_project(db, "Dalgo") + collection = get_assistant_collection(db, project) + + response = client.patch( + f"{settings.API_V1_STR}/collections/{collection.id}", + headers=user_api_key_header, + json={"name": "edited", "description": "edited desc"}, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["success"] is True + data = payload["data"] + assert data["id"] == str(collection.id) + assert data["name"] == "edited" + assert data["description"] == "edited desc" + + +def test_update_collection_partial_update_preserves_other_fields( + client: TestClient, + db: Session, + user_api_key_header: dict[str, str], +) -> None: + project = get_project(db, "Dalgo") + collection = get_assistant_collection(db, project) + CollectionCrud(db, project.id).update( + collection.id, CollectionUpdate(name="original", description="original-desc") + ) + + response = client.patch( + f"{settings.API_V1_STR}/collections/{collection.id}", + headers=user_api_key_header, + json={"description": "patched-desc"}, + ) + + assert response.status_code == 200 + data = response.json()["data"] + assert data["name"] == "original" + assert data["description"] == "patched-desc" + + +def test_update_collection_rename_to_existing_name_returns_409( + client: TestClient, + db: Session, + user_api_key_header: dict[str, str], +) -> None: + project = get_project(db, "Dalgo") + crud = CollectionCrud(db, project.id) + + first = get_assistant_collection(db, project) + crud.update(first.id, CollectionUpdate(name="duplicate")) + + second = get_assistant_collection(db, project) + + response = client.patch( + f"{settings.API_V1_STR}/collections/{second.id}", + headers=user_api_key_header, + json={"name": "duplicate"}, + ) + + assert response.status_code == 409 + + +def test_update_collection_not_found_returns_404( + client: TestClient, + user_api_key_header: dict[str, str], +) -> None: + response = client.patch( + f"{settings.API_V1_STR}/collections/{uuid4()}", + headers=user_api_key_header, + json={"name": "irrelevant"}, + ) + + assert response.status_code == 404 diff --git a/backend/app/tests/crud/collections/collection/test_crud_collection_update.py b/backend/app/tests/crud/collections/collection/test_crud_collection_update.py new file mode 100644 index 000000000..1ac0bf788 --- /dev/null +++ b/backend/app/tests/crud/collections/collection/test_crud_collection_update.py @@ -0,0 +1,93 @@ +from uuid import uuid4 +from unittest.mock import patch + +import pytest +from fastapi import HTTPException +from sqlalchemy.exc import IntegrityError +from sqlmodel import Session + +from app.crud import CollectionCrud +from app.models import CollectionUpdate +from app.tests.utils.utils import get_project +from app.tests.utils.collection import get_assistant_collection + + +class TestCollectionCrudUpdate: + def test_update_name_and_description(self, db: Session) -> None: + project = get_project(db, "Dalgo") + collection = get_assistant_collection(db, project) + + crud = CollectionCrud(db, project.id) + updated = crud.update( + collection.id, + CollectionUpdate(name="renamed", description="new desc"), + ) + + assert updated.name == "renamed" + assert updated.description == "new desc" + + def test_update_only_name_leaves_description_untouched(self, db: Session) -> None: + project = get_project(db, "Dalgo") + collection = get_assistant_collection(db, project) + crud = CollectionCrud(db, project.id) + + crud.update(collection.id, CollectionUpdate(description="initial")) + updated = crud.update(collection.id, CollectionUpdate(name="only-name")) + + assert updated.name == "only-name" + assert updated.description == "initial" + + def test_update_with_same_name_is_noop_no_conflict(self, db: Session) -> None: + project = get_project(db, "Dalgo") + collection = get_assistant_collection(db, project) + crud = CollectionCrud(db, project.id) + + crud.update(collection.id, CollectionUpdate(name="same")) + again = crud.update(collection.id, CollectionUpdate(name="same")) + + assert again.name == "same" + + def test_update_rename_to_existing_name_raises_409(self, db: Session) -> None: + project = get_project(db, "Dalgo") + crud = CollectionCrud(db, project.id) + + existing = get_assistant_collection(db, project) + crud.update(existing.id, CollectionUpdate(name="taken")) + + target = get_assistant_collection(db, project) + + with pytest.raises(HTTPException) as excinfo: + crud.update(target.id, CollectionUpdate(name="taken")) + + assert excinfo.value.status_code == 409 + assert "already exists" in excinfo.value.detail + + def test_update_nonexistent_collection_raises_404(self, db: Session) -> None: + project = get_project(db, "Dalgo") + crud = CollectionCrud(db, project.id) + + with pytest.raises(HTTPException) as excinfo: + crud.update(uuid4(), CollectionUpdate(name="anything")) + + assert excinfo.value.status_code == 404 + + def test_update_integrity_error_returns_409(self, db: Session) -> None: + """ + Simulate a concurrent insert: pre-check passes, but the DB commit + races and raises IntegrityError on the unique index. The CRUD should + catch it, roll back, and surface a clean 409. + """ + project = get_project(db, "Dalgo") + collection = get_assistant_collection(db, project) + crud = CollectionCrud(db, project.id) + + with patch.object( + crud, + "_update", + side_effect=IntegrityError("stmt", {}, Exception("duplicate")), + ): + with pytest.raises(HTTPException) as excinfo: + crud.update(collection.id, CollectionUpdate(name="race-condition")) + + assert excinfo.value.status_code == 409 + assert "already exists" in excinfo.value.detail From f427466d5117fc69192b05003f7eccb2fb76548a Mon Sep 17 00:00:00 2001 From: Ayush8923 <80516839+Ayush8923@users.noreply.github.com> Date: Fri, 22 May 2026 11:09:51 +0530 Subject: [PATCH 5/5] fix(collection): reviewer suggestion --- backend/app/api/routes/collections.py | 11 ++++++-- backend/app/crud/collection/collection.py | 26 ++++++++++++------- backend/app/models/collection.py | 17 ++++++++++-- .../collection/test_crud_collection_update.py | 18 ++++++------- 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/backend/app/api/routes/collections.py b/backend/app/api/routes/collections.py index ae131dd7b..6c9bee17a 100644 --- a/backend/app/api/routes/collections.py +++ b/backend/app/api/routes/collections.py @@ -2,7 +2,7 @@ from uuid import UUID from typing import List -from fastapi import APIRouter, Query, Body, Depends +from fastapi import APIRouter, HTTPException, Query, Body, Depends from fastapi import Path as FastPath from app.api.deps import SessionDep, AuthContextDep @@ -13,6 +13,7 @@ CollectionJobCrud, DocumentCollectionCrud, ) +from app.crud.collection.collection import CollectionNameConflictError from app.core.cloud import get_cloud_storage from app.models import ( CollectionJobStatus, @@ -224,7 +225,13 @@ def update_collection( organization_id=current_user.organization_.id, ): collection_crud = CollectionCrud(session, current_user.project_.id) - collection = collection_crud.update(collection_id, patch) + try: + collection = collection_crud.update(collection_id, patch) + except CollectionNameConflictError as err: + raise HTTPException( + status_code=409, + detail=f"Collection '{err.name}' already exists. Choose a different name.", + ) logger.info( f"[update_collection] Collection updated | {{'collection_id': '{collection_id}'}}" diff --git a/backend/app/crud/collection/collection.py b/backend/app/crud/collection/collection.py index 0a5d80f36..3d12e6f53 100644 --- a/backend/app/crud/collection/collection.py +++ b/backend/app/crud/collection/collection.py @@ -15,6 +15,14 @@ logger = logging.getLogger(__name__) +class CollectionNameConflictError(Exception): + """Raised when a collection name conflicts with an existing active collection.""" + + def __init__(self, name: str | None) -> None: + self.name = name + super().__init__(f"Collection name '{name}' already exists") + + class CollectionCrud: def __init__(self, session: Session, project_id: int): self.session = session @@ -94,17 +102,20 @@ def read_all(self): return collections def update(self, collection_id: UUID, patch: CollectionUpdate) -> Collection: - """Update editable fields of a collection (name, description).""" + """Update editable fields of a collection (name, description). + + Raises: + CollectionNameConflictError: when the requested name is already taken + by another active collection in the same project (caught either by + the pre-check or by a unique-index IntegrityError on commit). + """ collection = self.read_one(collection_id) changes = patch.model_dump(exclude_unset=True, exclude_none=True) if "name" in changes and changes["name"] != collection.name: if self.exists_by_name(changes["name"]): - raise HTTPException( - status_code=409, - detail=f"Collection '{changes['name']}' already exists. Choose a different name.", - ) + raise CollectionNameConflictError(changes["name"]) for field, value in changes.items(): setattr(collection, field, value) @@ -114,10 +125,7 @@ def update(self, collection_id: UUID, patch: CollectionUpdate) -> Collection: return self._update(collection) except IntegrityError: self.session.rollback() - raise HTTPException( - status_code=409, - detail="Collection name already exists. Choose a different name.", - ) + raise CollectionNameConflictError(changes.get("name")) def exists_by_name(self, collection_name: str) -> bool: statement = ( diff --git a/backend/app/models/collection.py b/backend/app/models/collection.py index 19b1f5ace..7767ff762 100644 --- a/backend/app/models/collection.py +++ b/backend/app/models/collection.py @@ -93,9 +93,16 @@ class Collection(SQLModel, table=True): # Request models class CollectionOptions(SQLModel): - name: str | None = Field(default=None, description="Name of the collection") + name: str | None = Field( + default=None, + min_length=1, + max_length=255, + description="Name of the collection", + ) description: str | None = Field( - default=None, description="Description of the collection" + default=None, + max_length=2000, + description="Description of the collection", ) documents: list[UUID] = Field( description="List of document IDs", @@ -195,10 +202,13 @@ class DeletionRequest(CallbackRequest): class CollectionUpdate(SQLModel): name: str | None = Field( default=None, + min_length=1, + max_length=255, description="New name for the collection", ) description: str | None = Field( default=None, + max_length=2000, description="New description for the collection", ) @@ -214,10 +224,13 @@ class CollectionPublic(SQLModel): id: UUID name: str | None = Field( default=None, + min_length=1, + max_length=255, description="Name of the collection", ) description: str | None = Field( default=None, + max_length=2000, description="Description of the collection", ) llm_service_id: str | None = Field( diff --git a/backend/app/tests/crud/collections/collection/test_crud_collection_update.py b/backend/app/tests/crud/collections/collection/test_crud_collection_update.py index 1ac0bf788..67480735a 100644 --- a/backend/app/tests/crud/collections/collection/test_crud_collection_update.py +++ b/backend/app/tests/crud/collections/collection/test_crud_collection_update.py @@ -7,6 +7,7 @@ from sqlmodel import Session from app.crud import CollectionCrud +from app.crud.collection.collection import CollectionNameConflictError from app.models import CollectionUpdate from app.tests.utils.utils import get_project from app.tests.utils.collection import get_assistant_collection @@ -47,7 +48,7 @@ def test_update_with_same_name_is_noop_no_conflict(self, db: Session) -> None: assert again.name == "same" - def test_update_rename_to_existing_name_raises_409(self, db: Session) -> None: + def test_update_rename_to_existing_name_raises_conflict(self, db: Session) -> None: project = get_project(db, "Dalgo") crud = CollectionCrud(db, project.id) @@ -56,11 +57,10 @@ def test_update_rename_to_existing_name_raises_409(self, db: Session) -> None: target = get_assistant_collection(db, project) - with pytest.raises(HTTPException) as excinfo: + with pytest.raises(CollectionNameConflictError) as excinfo: crud.update(target.id, CollectionUpdate(name="taken")) - assert excinfo.value.status_code == 409 - assert "already exists" in excinfo.value.detail + assert excinfo.value.name == "taken" def test_update_nonexistent_collection_raises_404(self, db: Session) -> None: project = get_project(db, "Dalgo") @@ -71,11 +71,12 @@ def test_update_nonexistent_collection_raises_404(self, db: Session) -> None: assert excinfo.value.status_code == 404 - def test_update_integrity_error_returns_409(self, db: Session) -> None: + def test_update_integrity_error_raises_conflict(self, db: Session) -> None: """ Simulate a concurrent insert: pre-check passes, but the DB commit races and raises IntegrityError on the unique index. The CRUD should - catch it, roll back, and surface a clean 409. + catch it, roll back, and raise the domain CollectionNameConflictError + (the route is responsible for translating it into HTTP 409). """ project = get_project(db, "Dalgo") collection = get_assistant_collection(db, project) @@ -86,8 +87,7 @@ def test_update_integrity_error_returns_409(self, db: Session) -> None: "_update", side_effect=IntegrityError("stmt", {}, Exception("duplicate")), ): - with pytest.raises(HTTPException) as excinfo: + with pytest.raises(CollectionNameConflictError) as excinfo: crud.update(collection.id, CollectionUpdate(name="race-condition")) - assert excinfo.value.status_code == 409 - assert "already exists" in excinfo.value.detail + assert excinfo.value.name == "race-condition"