Add Crud Operations for Assistant Table #286
Conversation
WalkthroughThis update introduces full CRUD (Create, Read, Update, Delete) REST API endpoints for managing assistants, adds soft-deletion support, and enforces project-level scoping and validation for vector store IDs. The changes include new and updated models, database migrations, expanded CRUD logic, and comprehensive test coverage for the new assistant management features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant OpenAI
Client->>API: POST /assistant/ (create assistant)
API->>OpenAI: Validate vector_store_ids
OpenAI-->>API: Validation result
API->>DB: Create assistant record
DB-->>API: Assistant created
API-->>Client: APIResponse[Assistant]
Client->>API: PATCH /assistant/{id} (update)
API->>OpenAI: Validate added vector_store_ids
OpenAI-->>API: Validation result
API->>DB: Update assistant record
DB-->>API: Assistant updated
API-->>Client: APIResponse[Assistant]
Client->>API: GET /assistant/{id}
API->>DB: Retrieve assistant by id and project
DB-->>API: Assistant or 404
API-->>Client: APIResponse[Assistant] or 404
Client->>API: GET /assistant/
API->>DB: List assistants (paginated, project-scoped)
DB-->>API: List of assistants
API-->>Client: APIResponse[list[Assistant]]
Client->>API: DELETE /assistant/{id}
API->>DB: Soft delete assistant (set is_deleted)
DB-->>API: Assistant marked deleted
API-->>Client: APIResponse
Estimated code review effort4 (60–120 minutes) Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🪛 Ruff (0.12.2)backend/app/crud/assistants.py21-21: Use Convert to (UP045) 27-27: Avoid equality comparisons to Replace with (E712) 46-46: Avoid equality comparisons to Replace with (E712) backend/app/models/assistants.py2-2: (UP035) 36-36: Use Convert to (UP045) 39-39: Undefined name (F821) 40-40: Undefined name (F821) 🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Ruff (0.12.2)backend/app/crud/assistants.py21-21: Use Convert to (UP045) 27-27: Avoid equality comparisons to Replace with (E712) 46-46: Avoid equality comparisons to Replace with (E712) backend/app/models/assistants.py2-2: (UP035) 36-36: Use Convert to (UP045) 39-39: Undefined name (F821) 40-40: Undefined name (F821) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (11)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
backend/app/models/assistants.py (1)
70-93: Refactor duplicate validator and modernize type annotations.The model structure is good, but the validator duplicates code from AssistantCreate.
First, update type annotations:
- name: Optional[str] = None - instructions: Optional[str] = None - model: Optional[str] = None - vector_store_ids_add: Optional[List[str]] = None - vector_store_ids_remove: Optional[List[str]] = None - temperature: Optional[float] = PydanticField( + name: str | None = None + instructions: str | None = None + model: str | None = None + vector_store_ids_add: list[str] | None = None + vector_store_ids_remove: list[str] | None = None + temperature: float | None = PydanticField( default=None, ge=0, le=2, description="Sampling temperature between 0 and 2" ) - max_num_results: Optional[int] = PydanticField( + max_num_results: int | None = PydanticField( default=None, ge=1, le=100, description="Maximum number of results (must be between 1 and 100)", )Then, extract the validator to a shared function to follow DRY principle:
def validate_openai_model_name(v: str) -> str: """Validate that the model name is in the allowed list.""" if v not in ALLOWED_OPENAI_MODELS: raise ValueError( f"Model '{v}' is not a supported OpenAI model. Choose from: {', '.join(ALLOWED_OPENAI_MODELS)}" ) return v # In both classes: @field_validator("model") def validate_openai_model(cls, v): return validate_openai_model_name(v)backend/app/tests/crud/test_assistants.py (1)
390-398: Remove unused variable assignment.The
projectvariable is assigned but never used.def test_get_assistants_by_project_empty(self, db: Session): """Returns empty list when project has no assistants""" - project = get_project(db) non_existent_project_id = get_non_existent_id(db, Project) result = get_assistants_by_project(db, non_existent_project_id) assert result == []
🧹 Nitpick comments (5)
backend/app/tests/utils/utils.py (1)
88-109: Improve boolean comparison style.The
get_assistantutility function is well-structured and correctly implements soft deletion filtering. However, improve the boolean comparisons to follow Python best practices.Apply this diff to fix the boolean comparisons:
if name: statement = ( select(Assistant) - .where(Assistant.name == name, Assistant.is_deleted == False) + .where(Assistant.name == name, not Assistant.is_deleted) .limit(1) ) else: - statement = select(Assistant).where(Assistant.is_deleted == False).limit(1) + statement = select(Assistant).where(not Assistant.is_deleted).limit(1)backend/app/alembic/versions/d4af4420f652_add_is_deleted_column_in_assistant_table.py (1)
10-10: Remove unused import.The
sqlmodel.sql.sqltypesimport is not used in this migration file.-import sqlmodel.sql.sqltypesbackend/app/crud/assistants.py (2)
3-3: Modernize type annotation for Set.-from typing import Optional, Set +from typing import OptionalUpdate line 206 to use
set[str]instead ofSet[str].
76-97: Robust vector store ID validation with proper error handling.The implementation correctly validates each vector store ID with appropriate error handling.
Consider batching the validation requests in the future if performance becomes an issue with large numbers of vector store IDs.
backend/app/tests/api/routes/test_assistants.py (1)
121-144: Remove print statement from test.Debug print statement should be removed.
- print(api_key.key) response = client.patch(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/app/alembic/versions/d4af4420f652_add_is_deleted_column_in_assistant_table.py(1 hunks)backend/app/api/routes/assistants.py(2 hunks)backend/app/api/routes/responses.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/assistants.py(4 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/assistants.py(2 hunks)backend/app/tests/api/routes/test_assistants.py(2 hunks)backend/app/tests/crud/test_assistants.py(2 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/app/models/__init__.py (1)
backend/app/models/assistants.py (4)
Assistant(27-38)AssistantBase(13-24)AssistantCreate(41-67)AssistantUpdate(70-92)
backend/app/models/assistants.py (1)
backend/app/core/util.py (1)
now(13-14)
backend/app/api/routes/assistants.py (5)
backend/app/api/deps.py (1)
get_current_user_org_project(110-131)backend/app/crud/assistants.py (5)
create_assistant(164-182)update_assistant(185-238)get_assistant_by_id(19-30)get_assistants_by_project(33-52)delete_assistant(241-265)backend/app/models/user.py (1)
UserProjectOrg(68-69)backend/app/models/assistants.py (3)
AssistantCreate(41-67)AssistantUpdate(70-92)Assistant(27-38)backend/app/utils.py (3)
APIResponse(27-46)get_openai_client(168-191)success_response(34-37)
🪛 Ruff (0.12.2)
backend/app/models/__init__.py
57-57: .assistants.Assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
57-57: .assistants.AssistantBase imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
57-57: .assistants.AssistantCreate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
57-57: .assistants.AssistantUpdate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/crud/__init__.py
51-51: .assistants.create_assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
52-52: .assistants.update_assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
53-53: .assistants.get_assistants_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
54-54: .assistants.delete_assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/alembic/versions/d4af4420f652_add_is_deleted_column_in_assistant_table.py
10-10: sqlmodel.sql.sqltypes imported but unused
Remove unused import: sqlmodel.sql.sqltypes
(F401)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
34-34: Use X | None for type annotations
Convert to X | None
(UP045)
37-37: Undefined name Project
(F821)
38-38: Undefined name Organization
(F821)
47-47: Use list instead of List for type annotation
Replace with list
(UP006)
51-51: Use X | None for type annotations
Convert to X | None
(UP045)
54-54: Use X | None for type annotations
Convert to X | None
(UP045)
71-71: Use X | None for type annotations
Convert to X | None
(UP045)
72-72: Use X | None for type annotations
Convert to X | None
(UP045)
73-73: Use X | None for type annotations
Convert to X | None
(UP045)
74-74: Use X | None for type annotations
Convert to X | None
(UP045)
74-74: Use list instead of List for type annotation
Replace with list
(UP006)
75-75: Use X | None for type annotations
Convert to X | None
(UP045)
75-75: Use list instead of List for type annotation
Replace with list
(UP006)
76-76: Use X | None for type annotations
Convert to X | None
(UP045)
79-79: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/tests/utils/utils.py
98-98: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
102-102: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
backend/app/crud/assistants.py
3-3: typing.Set is deprecated, use set instead
(UP035)
21-21: Use X | None for type annotations
Convert to X | None
(UP045)
27-27: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
46-46: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
206-206: Use set instead of Set for type annotation
Replace with set
(UP006)
backend/app/tests/api/routes/test_assistants.py
125-125: Unused function argument: normal_user_api_key_header
(ARG001)
161-161: Unused function argument: normal_user_api_key_header
(ARG001)
backend/app/tests/crud/test_assistants.py
392-392: Local variable project is assigned to but never used
Remove assignment to unused variable project
(F841)
🔇 Additional comments (20)
backend/app/models/__init__.py (1)
57-57: LGTM! Imports are correctly added for CRUD operations.The new
AssistantCreateandAssistantUpdateimports are necessary for the assistant CRUD functionality, despite being flagged by static analysis as unused. These models will be used in API routes and CRUD operations.backend/app/api/routes/responses.py (1)
223-223: Confirmed project_id parameter in get_assistant_by_id across codebaseThe
get_assistant_by_idfunction signature inbackend/app/crud/assistants.pynow takesproject_id: intas its third parameter, and all callers—including inbackend/app/api/routes/responses.py,backend/app/api/routes/assistants.py, and the CRUD tests inbackend/app/tests/crud/test_assistants.py—have been updated accordingly.Approving this change.
backend/app/crud/__init__.py (1)
51-54: LGTM! New CRUD function imports are properly added.The new assistant CRUD function imports (
create_assistant,update_assistant,get_assistants_by_project,delete_assistant) are correctly added to support the expanded assistant management functionality, despite static analysis warnings.backend/app/alembic/versions/d4af4420f652_add_is_deleted_column_in_assistant_table.py (2)
20-33: Excellent migration structure for soft deletion.The migration correctly implements soft deletion by:
- Adding
is_deletedwith a server default to handle existing rows- Adding
deleted_atas nullable for tracking deletion timestamps- Removing the server default after initial setup to prevent auto-defaults
This approach ensures backward compatibility and proper handling of existing data.
36-40: Proper downgrade implementation.The downgrade function correctly reverses the migration by dropping both columns, ensuring clean rollback capability.
backend/app/models/assistants.py (1)
33-34: LGTM! Soft deletion fields are properly implemented.The soft deletion implementation with
is_deletedflag anddeleted_attimestamp follows best practices.backend/app/crud/assistants.py (6)
19-31: LGTM! Project-scoped filtering with soft deletion check.The function correctly filters by project and excludes soft-deleted assistants.
33-53: Well-implemented pagination for project-scoped assistant listing.The function correctly implements pagination with sensible defaults and proper filtering.
110-111: Correctly updated to use project-scoped assistant lookup.The function now properly uses the updated
get_assistant_by_idwith project filtering.
164-183: Well-structured assistant creation with validation.The function correctly validates vector store IDs before creation and properly handles the database transaction.
185-239: Excellent implementation of partial updates with conflict detection.The function handles all edge cases well:
- Partial field updates
- Vector store ID add/remove with conflict detection
- Validation of new vector store IDs
- Proper timestamp updates
Great defensive programming with the conflicting IDs check.
241-266: Proper soft deletion implementation.The function correctly implements soft deletion by setting the appropriate flags and timestamp.
backend/app/api/routes/assistants.py (6)
22-49: Updated response model for consistency.Good change to use
APIResponse[Assistant]for consistency across all endpoints.
51-71: Well-implemented assistant creation endpoint.The endpoint correctly:
- Uses appropriate HTTP method and status code
- Validates vector store IDs before creation
- Properly handles authentication and authorization
73-94: Proper implementation of partial updates using PATCH.The endpoint correctly implements partial updates and delegates complex logic to the CRUD layer.
96-115: Standard REST implementation for resource retrieval.Good error handling with appropriate 404 response.
117-136: Well-designed list endpoint with pagination.Good use of Query parameters with validation constraints to prevent abuse.
138-155: Proper soft deletion endpoint.The endpoint correctly implements soft deletion and returns an appropriate success message.
backend/app/tests/api/routes/test_assistants.py (1)
1-328: Excellent comprehensive test coverage!The test suite thoroughly covers:
- All CRUD operations
- Success and failure scenarios
- Edge cases like pagination limits
- Proper mocking of external dependencies
- Soft deletion behavior
Great job on the test implementation!
backend/app/tests/crud/test_assistants.py (1)
1-398: Excellent CRUD-level test coverage with clear organization!The test suite is well-structured:
- Clear separation between sync operations and CRUD operations
- Comprehensive edge case testing (conflicts, validation failures, soft deletion)
- Good use of test utilities and fixtures
- Thorough testing of pagination and filtering
Great implementation!
backend/app/models/assistants.py
Outdated
| ALLOWED_OPENAI_MODELS = {"gpt-3.5-turbo", "gpt-4", "gpt-4o"} | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify OpenAI model names are current and consider a more maintainable approach.
The hardcoded list of allowed models may become outdated quickly. Please verify these are the current models you want to support.
Consider implementing a configuration-based approach or periodic validation against the OpenAI API to keep the allowed models list current.
🌐 Web query:
What are the latest available OpenAI chat models including GPT-4 Turbo and other variants?
💡 Result:
As of July 2025, the latest available OpenAI chat models include several advanced variants, featuring notable updates to the GPT-4 family and newer experimental "o-series" models. Here’s a summary of the most current and important models:
1. GPT-4 Turbo
- Designed for enhanced speed and affordability compared to the original GPT-4.
- Context window: 128,000 tokens—much larger than standard GPT-4, enabling extended conversations and context handling[2][5][8].
- Modalities: Supports text and images as input; available via ChatGPT and API[3][6][8].
- Knowledge cutoff: December 2023[8].
- Pricing: More cost-effective than previous GPT-4 models[8].
- Best for: Real-time interactive applications, dynamic content, and wherever fast, nuanced responses are essential[3][6][8].
2. GPT-4o (omni) and Variants
- Flagship model excelling in multimodal capabilities: accepts and generates text, images, and audio[3][5][6].
- Context window: 128,000 tokens[5][6].
- Response latency: Extremely fast (average ~320 milliseconds for audio)[3][6].
- Improvements: Enhanced vision and audio understanding, creative reasoning, improved non-English and coding skills, more intuitive responses[3][5][6][7].
- Pricing: Cheaper and faster than GPT-4 Turbo[6].
- Variants:
- GPT-4o mini: Lower-cost version with slightly reduced speed and capabilities[6].
- Best for: Advanced chatbots, high-stakes content, complex reasoning, and seamless human-computer interaction[3][6].
3. The "o-series" Reasoning Models
- o3 and o4-mini mark a step forward towards “agentic” AI—capable of independently choosing and using tools (e.g., web search, code execution, image generation) to solve sophisticated and multi-step tasks[4].
- o3-pro: Available to ChatGPT Pro users and API, designed for detailed, reliable, long-form reasoning[4].
- These models set a new standard for academic/research benchmarks and real-world utility by deeply integrating reasoning and tool use[4].
4. Standard GPT-4
- Remains widely used, but is outpaced by Turbo and o-series models in speed, cost efficiency, context capacity, and modalities[5][6][8][9].
- Context window: Up to 8,192 or 32,000 tokens, depending on variant[5].
5. GPT-3.5 Variants
- Still used for cost-sensitive or legacy applications but significantly less advanced in contextual understanding, safety, and multimodal capability compared to the GPT-4 family and o-series[9].
6. Upcoming Models
- GPT-4.1: Quietly released in mid-April 2025, but reportedly shown mixed performance in independent tests. (Not clearly positioned as flagship over 4o or Turbo)[1].
- GPT-5: Announced for release later in 2025 as a "unified next-gen" model, integrating prior advancements (e.g., from the canceled "o3") but not yet widely available as of July[1].
- Open "first" model: OpenAI is preparing its first open language model since GPT-2, scheduled for public prototype testing in the coming months[1].
| Model | Release Date | Context Window | Modalities | Speed/Cost | Notes |
|---|---|---|---|---|---|
| GPT-4 | Mar 2023 | 8k–32k tokens | txt, (limited img) | Moderate | Outpaced by newer variants |
| GPT-4 Turbo | Nov 2023 | 128k tokens | txt, img | Fast, cheap | More up-to-date, improved, larger window |
| GPT-4o | May 2024 | 128k tokens | txt, img, audio | Fastest, cheapest | Best for multimodal interaction |
| GPT-4o mini | 2024 | 128k tokens | txt, img, audio | Cheapest | Slightly lower speed than 4o, more affordable |
| o3, o3-pro | Apr/Jun 2025 | -- | txt, tools, images | Premium | Advanced autonomous reasoning, tool use |
| o4-mini | Apr 2025 | -- | txt, tools, images | -- | Smaller, experimental model |
| GPT-3.5 Turbo | 2022 | 4k tokens | txt | Fastest, cheap | Legacy, simple tasks |
In summary: OpenAI’s latest chat model lineup is led by GPT-4o and o3-series reasoning models, followed by highly capable Turbo variants. All offer improved context, multimodal input/output, and enhanced reasoning, with further advances continually in development[1][3][4][5][6].
Citations:
- 1: https://techcrunch.com/2025/07/18/chatgpt-everything-to-know-about-the-ai-chatbot/
- 2: https://openai.com/index/new-models-and-developer-products-announced-at-devday/
- 3: https://www.creolestudios.com/openai-models-comparison/
- 4: https://openai.com/index/introducing-o3-and-o4-mini/
- 5: https://www.techtarget.com/whatis/feature/GPT-4o-explained-Everything-you-need-to-know
- 6: https://teamai.com/blog/large-language-models-llms/understanding-different-chatgpt-models/
- 7: https://community.openai.com/t/chatgpt-release-notes-2025-march-27-gpt-4o-a-new-update/1153887
- 8: https://www.grammarly.com/blog/ai/what-is-gpt-4-turbo/
- 9: https://gettalkative.com/info/gpt-models-compared
- 10: https://help.openai.com/en/articles/9624314-model-release-notes
Reevaluate ALLOWED_OPENAI_MODELS for completeness and migrate to dynamic or config-based listing
The hardcoded set ({"gpt-3.5-turbo", "gpt-4", "gpt-4o"}) omits several widely used July 2025 chat models—e.g. gpt-4-turbo, gpt-4o-mini, the o-series (o3, o3-pro, o4-mini), and upcoming releases. Relying on a static list will quickly block users from accessing newer variants.
• File: backend/app/models/assistants.py (lines 10–11)
• Replace the inline ALLOWED_OPENAI_MODELS set with either:
– A runtime fetch of available models via the OpenAI /models API (filtering for chat-capable engines), or
– An external configuration (JSON/YAML) that can be updated independently of code changes.
• Ensure fallback or validation logic for unsupported models at request time.
🤖 Prompt for AI Agents
In backend/app/models/assistants.py around lines 10 to 11, replace the hardcoded
ALLOWED_OPENAI_MODELS set with a dynamic approach by either implementing a
runtime fetch from the OpenAI /models API to retrieve and filter chat-capable
models or loading the allowed models from an external configuration file like
JSON or YAML. Additionally, add fallback or validation logic to handle
unsupported models gracefully during request processing to ensure flexibility
and future-proofing against new model releases.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/app/models/assistants.py (2)
47-98: LGTM! Well-structured Pydantic models with type annotation improvements needed.The
AssistantCreateandAssistantUpdatemodels are well-designed with appropriate validation constraints. The update model's approach of using separate add/remove lists for vector store IDs is particularly elegant.Please modernize the type annotations as suggested in previous reviews:
-from typing import List, Optional +from typing import Optional - vector_store_ids: List[str] = PydanticField( + vector_store_ids: list[str] = PydanticField( - temperature: Optional[float] = PydanticField( + temperature: float | None = PydanticField( - max_num_results: Optional[int] = PydanticField( + max_num_results: int | None = PydanticField(Apply similar changes throughout the
AssistantUpdatemodel.
12-12: Consider dynamic model validation as suggested in previous reviews.The hardcoded
ALLOWED_OPENAI_MODELSset may become outdated quickly. As mentioned in previous reviews, consider implementing a configuration-based approach or periodic validation against the OpenAI API to keep the model list current.
🧹 Nitpick comments (5)
backend/app/tests/api/routes/test_responses.py (3)
41-43: Update comment to match the actual query.The comment mentions "Glific project" but the code queries for "Dalgo" project.
- # Get the Glific project ID (the assistant is created for this project) + # Get the Dalgo project ID (the assistant is created for this project) glific_project = db.exec(select(Project).where(Project.name == "Dalgo")).first() if not glific_project: - pytest.skip("Glific project not found in the database") + pytest.skip("Dalgo project not found in the database")
56-56: Remove debugging print statement.This appears to be leftover debugging code that should be removed from the test.
- print(response.json()) # Debugging output
51-52: Remove commented-out debugging code.The commented sleep statement suggests timing issues during development but should be cleaned up.
- # import time - # time.sleep(2000)backend/app/tests/utils/utils.py (1)
96-117: LGTM! Helpful utility function with style improvements needed.The
get_assistantfunction is well-implemented and follows the same pattern as the existingget_projectutility. It correctly handles the soft deletion logic using theis_deletedflag.Apply this style improvement for better Python idiom:
- .where(Assistant.name == name, Assistant.is_deleted == False) + .where(Assistant.name == name, not Assistant.is_deleted)- statement = select(Assistant).where(Assistant.is_deleted == False).limit(1) + statement = select(Assistant).where(not Assistant.is_deleted).limit(1)backend/app/tests/api/routes/test_assistants.py (1)
117-148: LGTM! Good update test with minor cleanup needed.The update test properly validates the functionality, but there's a debugging print statement that should be removed.
- print(api_key.key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/crud/__init__.py(1 hunks)backend/app/models/assistants.py(2 hunks)backend/app/tests/api/routes/test_assistants.py(2 hunks)backend/app/tests/api/routes/test_responses.py(1 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/tests/api/routes/test_responses.py (2)
backend/app/tests/conftest.py (3)
db(20-37)client(48-51)normal_user_api_key_headers(73-75)backend/app/models/project.py (1)
Project(28-51)
backend/app/crud/__init__.py (2)
backend/app/seed_data/seed_data.py (1)
create_assistant(236-276)backend/app/crud/assistants.py (4)
create_assistant(164-182)update_assistant(185-238)get_assistants_by_project(33-52)delete_assistant(241-265)
🪛 Ruff (0.12.2)
backend/app/crud/__init__.py
52-52: .assistants.create_assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
53-53: .assistants.update_assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
54-54: .assistants.get_assistants_by_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
55-55: .assistants.delete_assistant imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
40-40: Use X | None for type annotations
Convert to X | None
(UP045)
43-43: Undefined name Project
(F821)
44-44: Undefined name Organization
(F821)
53-53: Use list instead of List for type annotation
Replace with list
(UP006)
57-57: Use X | None for type annotations
Convert to X | None
(UP045)
60-60: Use X | None for type annotations
Convert to X | None
(UP045)
77-77: Use X | None for type annotations
Convert to X | None
(UP045)
78-78: Use X | None for type annotations
Convert to X | None
(UP045)
79-79: Use X | None for type annotations
Convert to X | None
(UP045)
80-80: Use X | None for type annotations
Convert to X | None
(UP045)
80-80: Use list instead of List for type annotation
Replace with list
(UP006)
81-81: Use X | None for type annotations
Convert to X | None
(UP045)
81-81: Use list instead of List for type annotation
Replace with list
(UP006)
82-82: Use X | None for type annotations
Convert to X | None
(UP045)
85-85: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/tests/utils/utils.py
106-106: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
110-110: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
🔇 Additional comments (6)
backend/app/crud/__init__.py (1)
52-55: LGTM! Assistant CRUD imports are correctly exposed.The newly added assistant CRUD function imports are essential for the API routes and align with the PR's objective to implement full CRUD operations for assistants. The static analysis warnings about unused imports are false positives - these functions are being exposed through this module for consumption by other parts of the application, specifically the assistant API routes.
backend/app/models/assistants.py (1)
39-40: LGTM! Soft deletion fields properly implemented.The soft deletion implementation with
is_deletedanddeleted_atfields is correctly designed and aligns with the PR objectives.backend/app/tests/api/routes/test_assistants.py (4)
12-26: LGTM! Well-structured test fixtures.The fixtures provide clean, reusable test data that makes the tests more maintainable and readable.
51-88: LGTM! Comprehensive create assistant test coverage.Excellent test coverage for both successful creation and validation failure scenarios. The mocking of
verify_vector_store_ids_existproperly isolates the API logic from OpenAI dependencies.
236-285: LGTM! Thorough pagination validation testing.Excellent coverage of edge cases for pagination parameters, including negative skip, excessive limits, and boundary conditions.
287-320: LGTM! Complete CRUD test coverage.The delete tests properly validate both successful soft deletion and error handling for non-existent resources, completing the comprehensive test suite for assistant operations.
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/models/assistants.py (2)
12-13: Static model list quickly goes stale
Hard-codingALLOWED_OPENAI_MODELSwas flagged previously; consider loading from config or the OpenAI/modelsendpoint to avoid frequent code edits.Also applies to: 67-73
2-3: Modernise type hints & drop unused import
typing.Listis deprecated; changevector_store_ids: List[str]tolist[str]and remove the now-unusedListimport.-from typing import List, Optional +from typing import Optional ... - vector_store_ids: List[str] = Field( + vector_store_ids: list[str] = Field(Also applies to: 20-22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/models/assistants.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/models/assistants.py (1)
backend/app/core/util.py (1)
now(13-14)
🪛 Ruff (0.12.2)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
40-40: Use X | None for type annotations
Convert to X | None
(UP045)
43-43: Undefined name Project
(F821)
44-44: Undefined name Organization
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/app/tests/api/routes/test_assistants.py (5)
24-27: Remove unused fixtureassistant_id.The fixture isn’t referenced by any test in this module; drop it to keep the suite lean.
51-67: Verify that vector-store validation is executed.Add an assertion to ensure the handler actually calls
verify_vector_store_ids_existwith the expected IDs:@@ response = client.post( "/api/v1/assistant", json=assistant_create_payload, headers=normal_user_api_key_headers, ) assert response.status_code == 201 + mock_verify_vector_ids.assert_called_once_with( + assistant_create_payload["vector_store_ids"] + )
99-111: Also assert validation call in the negative path.After the 400 response, add:
mock_verify_vector_ids.assert_called_once_with(["vs_test_999"])to confirm the route attempted validation before failing.
150-169: Strengthen the invalid-update test with a call assertion.Append:
mock_verify_vector_ids.assert_called_once_with(["vs_invalid"])to ensure the update logic performs the expected check.
29-49: Assert that the OpenAI fetch helper is invoked once.Right after the 201 assertion, add:
mock_fetch_assistant.assert_called_once_with(mock_assistant.id)This guarantees the ingest route delegates to the helper as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/tests/api/routes/test_assistants.py(2 hunks)backend/app/tests/api/routes/test_responses.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/api/routes/test_responses.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/app/models/assistants.py (3)
2-2: Modernize type annotations as previously suggested.This issue was already identified in previous reviews. Please implement the suggested changes to replace deprecated typing imports with built-in types.
12-13: Address the hardcoded model list as previously discussed.The limitations of the hardcoded
ALLOWED_OPENAI_MODELSset have been thoroughly analyzed in previous reviews, with comprehensive suggestions for a dynamic approach provided.
100-106: Address the validator issue for partial updates as previously identified.The model validator breaking partial updates has been thoroughly analyzed in previous reviews with a clear solution provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/models/assistants.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/models/assistants.py (1)
backend/app/core/util.py (1)
now(13-14)
🪛 Ruff (0.12.2)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
40-40: Use X | None for type annotations
Convert to X | None
(UP045)
43-43: Undefined name Project
(F821)
44-44: Undefined name Organization
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (4)
backend/app/models/assistants.py (4)
15-31: Base model structure is well-designed.The foreign key relationships and field definitions are appropriate, with proper CASCADE delete behavior and database column specifications.
39-40: Soft delete implementation is correctly designed.The soft delete fields follow best practices:
is_deletedwith appropriate default and nullable settingsdeleted_atfor audit trail purposes- Proper typing for the optional datetime field
47-78: Validation constraints are well-designed and comprehensive.The field validation logic is excellent:
- Appropriate string length constraints for user-facing fields
- Proper numeric bounds for OpenAI API parameters
- Sensible default values and helpful descriptions
- Comprehensive coverage of all required fields
The implementation follows Pydantic best practices for input validation.
88-89: Vector store ID management design is well-thought-out.The separate
vector_store_ids_addandvector_store_ids_removefields provide a clean API for managing vector store associations in partial updates, avoiding the complexity of full replacement semantics.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/assistants.py(4 hunks)backend/app/utils.py(2 hunks)
🧬 Code Graph Analysis (1)
backend/app/crud/assistants.py (5)
backend/app/models/assistants.py (3)
Assistant(30-41)AssistantCreate(44-69)AssistantUpdate(72-94)backend/app/utils.py (1)
mask_string(156-165)backend/app/core/util.py (1)
now(13-14)backend/app/tests/api/routes/test_assistants.py (1)
assistant_id(25-26)backend/app/seed_data/seed_data.py (1)
create_assistant(236-276)
🪛 Ruff (0.12.2)
backend/app/crud/assistants.py
3-3: typing.Set is deprecated, use set instead
(UP035)
21-21: Use X | None for type annotations
Convert to X | None
(UP045)
27-27: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
46-46: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
212-212: Use set instead of Set for type annotation
Replace with set
(UP006)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/crud/assistants.py (5)
backend/app/models/assistants.py (3)
Assistant(30-41)AssistantCreate(44-69)AssistantUpdate(72-94)backend/app/utils.py (1)
mask_string(156-165)backend/app/core/util.py (1)
now(13-14)backend/app/tests/api/routes/test_assistants.py (1)
assistant_id(25-26)backend/app/seed_data/seed_data.py (1)
create_assistant(236-276)
🪛 Ruff (0.12.2)
backend/app/crud/assistants.py
3-3: typing.Set is deprecated, use set instead
(UP035)
21-21: Use X | None for type annotations
Convert to X | None
(UP045)
27-27: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
46-46: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
212-212: Use set instead of Set for type annotation
Replace with set
(UP006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (7)
backend/app/utils.py (2)
180-182: Enhanced logging improves diagnostics.The addition of project_id context in the warning log will help with debugging credential configuration issues across different projects.
191-194: Comprehensive error logging with full traceback.The enhanced error logging with
exc_info=Trueand project_id context provides valuable diagnostic information for troubleshooting OpenAI client initialization failures. This will be particularly useful for the new assistant CRUD operations.backend/app/crud/assistants.py (5)
12-14: Import organization looks good.The imports are well-organized and include all necessary models and utilities for the CRUD operations.
76-97: Robust vector store validation implementation.The function provides comprehensive validation with proper error handling and logging. The approach of checking each vector store ID individually ensures specific error reporting.
110-114: Improved project-scoped conflict detection.The updated function call correctly uses project_id for scoping and provides clear logging with masked assistant IDs for security.
191-245: Comprehensive update logic with proper validation.The update function implements sophisticated logic for managing vector store IDs with add/remove semantics. The conflict detection and validation are well-implemented. The use of set operations for managing vector store IDs is efficient and clear.
248-272: Clean soft deletion implementation.The soft deletion implementation correctly marks the assistant as deleted and sets the deletion timestamp. The error handling and logging are consistent with the other functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/crud/test_assistants.py (1)
397-404: Remove unused variable assignment.The
projectvariable is assigned but never used in this test method.- project = get_project(db) - non_existent_project_id = get_non_existent_id(db, Project) + non_existent_project_id = get_non_existent_id(db, Project)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/crud/test_assistants.py(2 hunks)
🪛 Ruff (0.12.2)
backend/app/tests/crud/test_assistants.py
399-399: Local variable project is assigned to but never used
Remove assignment to unused variable project
(F841)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/crud/test_assistants.py
399-399: Local variable project is assigned to but never used
Remove assignment to unused variable project
(F841)
🔇 Additional comments (14)
backend/app/tests/crud/test_assistants.py (14)
26-26: LGTM: Clear separation of test concerns.Good refactoring to rename the existing test class to
TestSyncAssistantto distinguish it from the new CRUD operations tests.
135-159: LGTM: Well-structured test with proper mocking.The test correctly validates successful assistant creation with proper mocking of the vector store verification function. The assertions comprehensively verify all fields of the created assistant.
160-187: LGTM: Proper error handling validation.The test correctly validates that invalid vector store IDs trigger appropriate HTTP exceptions. The mocking strategy effectively simulates the validation failure scenario.
188-213: LGTM: Comprehensive update operation testing.The test validates successful assistant updates with vector store additions. The assertions verify both the updated fields and the vector store modification behavior.
214-226: LGTM: Proper 404 error handling for non-existent assistants.The test correctly validates that updating a non-existent assistant raises an HTTP 404 exception with appropriate error messaging.
227-251: LGTM: Validation of vector store error propagation.The test ensures that vector store validation errors are properly propagated during update operations, maintaining data integrity.
252-277: LGTM: Important business logic validation.Excellent test that validates the conflicting vector store IDs scenario (same ID in both add and remove lists). This prevents logical inconsistencies in the update operation.
278-300: LGTM: Partial update functionality validation.The test correctly validates that partial updates work as expected, preserving unchanged fields while updating only the specified ones.
301-312: LGTM: Soft deletion behavior validation.The test properly validates soft deletion by checking both the
is_deletedflag anddeleted_attimestamp, and confirms that soft-deleted assistants cannot be retrieved through normal queries.
313-323: LGTM: Delete operation error handling.The test correctly validates that attempting to delete a non-existent assistant raises an HTTP 404 exception.
324-334: LGTM: Basic retrieval functionality.The test validates successful assistant retrieval by ID with proper project scoping and soft deletion filtering.
335-343: LGTM: Proper handling of missing assistants.The test correctly validates that retrieving a non-existent assistant returns None rather than raising an exception.
344-353: LGTM: Soft deletion filtering validation.The test ensures that soft-deleted assistants are properly filtered out from retrieval operations, maintaining data consistency.
354-396: LGTM: Comprehensive project-scoped listing.The test validates the project-scoped assistant listing functionality with multiple assistants, ensuring proper filtering and association.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
backend/app/models/assistants.py (1)
1-94: Fix Black formatting issues to pass CI.The pipeline is failing due to Black formatting violations. Run the following command to fix the formatting:
black backend/app/models/assistants.py
♻️ Duplicate comments (3)
backend/app/models/assistants.py (3)
2-2: Modernize type annotations to use built-in types.
36-36: Update type annotation to use modern union syntax.- deleted_at: Optional[datetime] = Field(default=None, nullable=True) + deleted_at: datetime | None = Field(default=None, nullable=True)
39-40: Add TYPE_CHECKING imports for forward-reference models.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/models/assistants.py(2 hunks)
🪛 Ruff (0.12.2)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
36-36: Use X | None for type annotations
Convert to X | None
(UP045)
39-39: Undefined name Project
(F821)
40-40: Undefined name Organization
(F821)
🪛 GitHub Actions: AI Platform CI
backend/app/models/assistants.py
[error] 1-1: Black formatting check failed. The file was reformatted by Black. Run 'black backend/app/models/assistants.py' to apply formatting.
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
36-36: Use X | None for type annotations
Convert to X | None
(UP045)
39-39: Undefined name Project
(F821)
40-40: Undefined name Organization
(F821)
🪛 GitHub Actions: AI Platform CI
backend/app/models/assistants.py
[error] 1-1: Black formatting check failed. The file was reformatted by Black. Run 'black backend/app/models/assistants.py' to apply formatting.
🔇 Additional comments (1)
backend/app/models/assistants.py (1)
43-69: Well-designed input validation model with comprehensive constraints.The
AssistantCreatemodel has appropriate field validation constraints and uses modern type annotations. The temperature range (0-2), max results limit (1-100), and string length requirements provide good data integrity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/models/assistants.py (2)
2-3: Modernize type annotations to use built-in types.Replace deprecated typing module imports with built-in types for better readability and consistency with modern Python.
39-40: Add TYPE_CHECKING imports for forward-reference models.The forward references "Project" and "Organization" need proper imports to resolve undefined name errors.
🧹 Nitpick comments (1)
backend/app/models/assistants.py (1)
35-36: Soft deletion fields implemented correctly.The
is_deletedanddeleted_atfields are properly configured for soft deletion functionality. Consider modernizing the type annotation:- deleted_at: Optional[datetime] = Field(default=None, nullable=True) + deleted_at: datetime | None = Field(default=None, nullable=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/models/assistants.py(2 hunks)
🪛 Ruff (0.12.2)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
36-36: Use X | None for type annotations
Convert to X | None
(UP045)
39-39: Undefined name Project
(F821)
40-40: Undefined name Organization
(F821)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/models/assistants.py
2-2: typing.List is deprecated, use list instead
(UP035)
36-36: Use X | None for type annotations
Convert to X | None
(UP045)
39-39: Undefined name Project
(F821)
40-40: Undefined name Organization
(F821)
🔇 Additional comments (3)
backend/app/models/assistants.py (3)
6-6: Import update looks good.The SQLModel import has been appropriately updated to support the new model definitions.
43-69: Well-structured AssistantCreate model with appropriate validation.The model has good field definitions with proper constraints:
- String length limits are reasonable
- Numeric ranges are well-defined (temperature 0-2, max_results 1-100)
- Default values are sensible
- Modern type annotations are used correctly
71-93: AssistantUpdate model supports partial updates effectively.The update model correctly uses optional fields for partial updates and maintains consistent validation constraints with the create model. The separate
vector_store_ids_addandvector_store_ids_removefields provide flexible vector store management.
backend/app/alembic/versions/e8ee93526b37_add_is_deleted_column_in_assistant_table.py
Outdated
Show resolved
Hide resolved
3 participants
Summary
Target issue is #271
This PR introduces full CRUD support for the Assistant table, including the following:
Assistant CRUD Implementation
Database Migration
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Database
Documentation