-
Notifications
You must be signed in to change notification settings - Fork 10
Collections: Customizable collection names #878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3fbd9e1
f099b15
72a9b35
3f84995
f427466
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
|
|
@@ -192,6 +199,20 @@ class DeletionRequest(CallbackRequest): | |
| collection_id: UUID = Field(description="Collection to delete") | ||
|
|
||
|
|
||
| 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", | ||
| ) | ||
|
|
||
|
|
||
| # Response models | ||
|
|
||
|
|
||
|
|
@@ -201,6 +222,17 @@ class CollectionIDPublic(SQLModel): | |
|
|
||
| class CollectionPublic(SQLModel): | ||
| id: UUID | ||
| name: str | None = Field( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while this is fine can we add some restriction like name should be there instead of name and minimum and maximum length
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the code accordingly. |
||
| 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", | ||
| ) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add testcases for this as well
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, added the testcases. |
||
| llm_service_id: str | None = Field( | ||
| default=None, | ||
| description="LLM service ID (e.g., Assistant ID) when model and instructions were provided", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nitpick) If there is an error at the crud layer we can raise the httpexception at crud file only .. no need of raising httpexception here..
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this comment #878 (comment). Earlier, I added the httpexception at crub file. But then I change the code according to comment.