fix: move inline SQLAlchemy queries out of ConversationController into service layer#667
fix: move inline SQLAlchemy queries out of ConversationController into service layer#667arnavp27 wants to merge 1 commit intopotpie-ai:mainfrom
Conversation
…o service layer Moves the remaining inline `select(CustomAgent)` query and response assembly from ConversationController.get_conversations_for_user() into a new service method ConversationService.list_conversations_for_user(), keeping all DB access behind the store/service layer and out of the controller.
WalkthroughThis PR refactors conversation listing by moving aggregation logic from the controller to a new service method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/modules/conversations/conversation/conversation_service.py (1)
1860-1865: Consider routing the CustomAgent lookup through its own store/service layer.The PR objective is to remove inline SQLAlchemy queries from the controller and route them through the store/service layer. However, this code directly accesses
self.conversation_store.async_dbto queryCustomAgentModel, which:
- Bypasses encapsulation of the store layer
- Creates a cross-module dependency (conversation service directly querying custom_agents table)
A cleaner approach would be to call
self.custom_agent_service(already injected via__init__) to resolve agent display names, or introduce a batch lookup method inCustomAgentService.♻️ Suggested approach using custom_agent_service
- if agent_ids: - stmt = select(CustomAgentModel).where( - CustomAgentModel.id.in_(agent_ids) - ) - result = await self.conversation_store.async_db.execute(stmt) - agents = result.scalars().all() - custom_agent_display_names = {agent.id: agent.role for agent in agents} + if agent_ids: + # Delegate to custom_agent_service for batch lookup + custom_agent_display_names = await self.custom_agent_service.get_agent_display_names_batch(agent_ids)This would require adding a
get_agent_display_names_batchmethod toCustomAgentService.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/modules/conversations/conversation/conversation_service.py` around lines 1860 - 1865, This code bypasses the store layer by directly using self.conversation_store.async_db to query CustomAgentModel; replace that inline SQLAlchemy lookup with a call to the injected self.custom_agent_service: add a batch lookup method (suggested name get_agent_display_names_batch) on CustomAgentService that accepts a list of agent_ids and returns a dict mapping id -> display_name/role, then in this function replace the select/execute/scalars logic with a single await self.custom_agent_service.get_agent_display_names_batch(agent_ids) to populate custom_agent_display_names and remove the direct reference to CustomAgentModel and async_db.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/modules/conversations/conversation/conversation_service.py`:
- Around line 1883-1884: When serializing timestamps in conversation_service
(the lines using conversation.created_at.isoformat() and
conversation.updated_at.isoformat()), guard against None to avoid AttributeError
by checking the attributes before calling isoformat; e.g., replace direct calls
with a conditional that yields None (or a safe default string) when
conversation.created_at or conversation.updated_at is falsy, or extract this
logic into a small helper (e.g., format_ts(ts)) and use it for both created_at
and updated_at to keep the code robust.
---
Nitpick comments:
In `@app/modules/conversations/conversation/conversation_service.py`:
- Around line 1860-1865: This code bypasses the store layer by directly using
self.conversation_store.async_db to query CustomAgentModel; replace that inline
SQLAlchemy lookup with a call to the injected self.custom_agent_service: add a
batch lookup method (suggested name get_agent_display_names_batch) on
CustomAgentService that accepts a list of agent_ids and returns a dict mapping
id -> display_name/role, then in this function replace the
select/execute/scalars logic with a single await
self.custom_agent_service.get_agent_display_names_batch(agent_ids) to populate
custom_agent_display_names and remove the direct reference to CustomAgentModel
and async_db.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/modules/conversations/conversation/conversation_controller.pyapp/modules/conversations/conversation/conversation_service.py
| created_at=conversation.created_at.isoformat(), | ||
| updated_at=conversation.updated_at.isoformat(), |
There was a problem hiding this comment.
Potential AttributeError if timestamps are None.
If conversation.created_at or conversation.updated_at is None, calling .isoformat() will raise an AttributeError. Based on the model definition, these fields have defaults and are marked nullable=False, so this is unlikely in practice—but consider adding a defensive check or using a helper function for robustness.
🛡️ Optional defensive approach
- created_at=conversation.created_at.isoformat(),
- updated_at=conversation.updated_at.isoformat(),
+ created_at=conversation.created_at.isoformat() if conversation.created_at else "",
+ updated_at=conversation.updated_at.isoformat() if conversation.updated_at else "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/modules/conversations/conversation/conversation_service.py` around lines
1883 - 1884, When serializing timestamps in conversation_service (the lines
using conversation.created_at.isoformat() and
conversation.updated_at.isoformat()), guard against None to avoid AttributeError
by checking the attributes before calling isoformat; e.g., replace direct calls
with a conditional that yields None (or a safe default string) when
conversation.created_at or conversation.updated_at is falsy, or extract this
logic into a small helper (e.g., format_ts(ts)) and use it for both created_at
and updated_at to keep the code robust.
1 participant
Problem
ConversationController.get_conversations_for_user()contained an inlineselect(CustomAgent)query executed directly viaself.async_db, bypassingthe store/service layer. Response assembly (
UserConversationListResponse)was also happening in the controller, violating the layered architecture.
Changes
conversation_controller.py: Removed inlineselect(CustomAgent)queryand unused imports.
get_conversations_for_user()now delegates entirely tothe service (3 lines).
conversation_service.py: Addedlist_conversations_for_user()thathandles the CustomAgent batch lookup via
conversation_store.async_dbandassembles
UserConversationListResponseobjects.Result
All DB reads/writes for the Conversation model now go exclusively through the
store/service layer. Zero inline SQLAlchemy queries remain in the controller.
Summary by CodeRabbit