Support to update template using PATCH endpoint #474
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR adds a partial update feature for templates using a new PATCH endpoint. It introduces an Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router as API Router
participant Handler as Handler
participant Accessor as DB Accessor
participant Query as Query Builder
participant DB as Database
Client->>Router: PATCH /templates/{id}
Router->>Handler: update_template_handler(id, update_data, user)
rect rgba(100, 150, 200, 0.2)
Note over Handler: RBAC & Validation
Handler->>Handler: Verify RBAC permissions
Handler->>Handler: Validate fields
Handler->>Handler: Check outbound number if updated
Handler->>Handler: Validate flow structure if updated
end
Handler->>Accessor: update_template(id, fields, timestamp)
rect rgba(150, 200, 150, 0.2)
Note over Accessor,Query: Query Construction
Accessor->>Query: update_template_query(id, fields, timestamp)
Query->>Query: Map fields to DB columns
Query->>Query: Cast JSONB fields
Query->>Query: Build dynamic SET clauses
Query-->>Accessor: Return SQL + values
end
Accessor->>DB: Execute UPDATE query
DB-->>Accessor: Return updated TemplateModel
rect rgba(200, 150, 150, 0.2)
Note over Accessor: Handling
Accessor->>Accessor: Decode to TemplateModel
Accessor-->>Handler: Return updated template or None
end
Handler-->>Client: 200 OK {updated_fields, success: true}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In @app/api/routers/breeze_buddy/templates/handlers.py:
- Around line 306-431: The handler ignores the description field on
UpdateTemplateRequest causing a silent no-op; either remove the field from the
request models or persist it—if you persist it, add a description column to the
templates table (create_template_query), update the create_template and
update_template DB queries/functions to include description, and modify
update_template_handler to accept update_data.description (add
update_fields["description"] when provided, validate/serialize as needed) so
PATCH actually updates description; if removing, delete description from
UpdateTemplateRequest and CreateTemplateRequest types to keep API surface
consistent.
🧹 Nitpick comments (1)
app/api/routers/breeze_buddy/templates/handlers.py (1)
370-376: Consider validating for empty flow dictionary.The flow validation checks for the presence of
initial_nodeandnodes, but it doesn't validate against an empty dictionary. Ifupdate_data.flowis{}, the checks will raise errors as expected. However, explicitly checking for empty dict first would provide a clearer error message.Optional improvement for clearer validation
if update_data.flow is not None: + # Validate flow is not empty + if not update_data.flow: + raise ValueError("Flow structure cannot be empty") # Validate flow structure if "initial_node" not in update_data.flow: raise ValueError("initial_node must be specified in flow structure") if "nodes" not in update_data.flow or not update_data.flow["nodes"]: raise ValueError("nodes must be specified in flow structure") update_fields["flow"] = update_data.flow
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/ai/voice/agents/breeze_buddy/template/types.pyapp/api/routers/breeze_buddy/templates/__init__.pyapp/api/routers/breeze_buddy/templates/handlers.pyapp/database/accessor/breeze_buddy/template.pyapp/database/queries/breeze_buddy/template.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T12:11:37.686Z
Learnt from: swaroopvarma1
Repo: juspay/clairvoyance PR: 447
File: app/database/queries/breeze_buddy/call_execution_config.py:268-285
Timestamp: 2025-12-23T12:11:37.686Z
Learning: Guideline: In Breeze Buddy, treat shop_identifier as identifying individual merchant locations, while merchant_id acts as a parent/group identifier. Therefore, functions that retrieve shop_identifiers should be named as 'merchants' functions (e.g., get_merchants_, list_merchants_). Apply this naming consistently across all Breeze Buddy query modules and related tests. This pattern should apply to all Python files under app/database/queries/breeze_buddy/ (and similar modules following the same domain naming) to maintain consistency across the codebase.
Applied to files:
app/database/queries/breeze_buddy/template.py
🧬 Code graph analysis (4)
app/api/routers/breeze_buddy/templates/__init__.py (5)
app/ai/voice/agents/breeze_buddy/template/types.py (1)
UpdateTemplateRequest(138-152)app/api/routers/breeze_buddy/templates/handlers.py (1)
update_template_handler(306-431)app/database/accessor/breeze_buddy/template.py (1)
update_template(280-329)app/schemas/breeze_buddy/auth.py (1)
UserInfo(103-112)app/api/security/breeze_buddy/rbac_token.py (1)
get_current_user_with_rbac(194-216)
app/database/accessor/breeze_buddy/template.py (7)
app/database/queries/breeze_buddy/template.py (1)
update_template_query(148-212)app/ai/voice/agents/breeze_buddy/template/types.py (1)
TemplateModel(90-103)app/database/queries/__init__.py (1)
run_parameterized_query(14-32)app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
get_row_count(38-42)app/database/accessor/breeze_buddy/outbound_number.py (1)
get_row_count(29-33)app/database/accessor/breeze_buddy/call_execution_config.py (1)
get_row_count(29-33)app/database/decoder/breeze_buddy/template.py (1)
decode_template(16-79)
app/ai/voice/agents/breeze_buddy/template/types.py (1)
app/ai/voice/agents/breeze_buddy/template/context.py (1)
expected_callback_response_schema(127-129)
app/api/routers/breeze_buddy/templates/handlers.py (5)
app/ai/voice/agents/breeze_buddy/template/types.py (1)
UpdateTemplateRequest(138-152)app/database/accessor/breeze_buddy/outbound_number.py (1)
get_outbound_number_by_id(77-99)app/database/accessor/breeze_buddy/template.py (1)
update_template(280-329)app/api/routers/breeze_buddy/templates/__init__.py (1)
update_template(168-226)app/api/routers/breeze_buddy/templates/rbac.py (1)
validate_template_access(14-63)
🪛 Ruff (0.14.10)
app/database/queries/breeze_buddy/template.py
163-163: Avoid specifying long messages outside the exception class
(TRY003)
205-210: Possible SQL injection vector through string-based query construction
(S608)
app/api/routers/breeze_buddy/templates/__init__.py
171-171: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/database/accessor/breeze_buddy/template.py
325-325: Consider moving this statement to an else block
(TRY300)
327-327: Do not catch blind exception: Exception
(BLE001)
app/api/routers/breeze_buddy/templates/handlers.py
333-336: Abstract raise to an inner function
(TRY301)
361-364: Abstract raise to an inner function
(TRY301)
373-373: Abstract raise to an inner function
(TRY301)
373-373: Avoid specifying long messages outside the exception class
(TRY003)
375-375: Abstract raise to an inner function
(TRY301)
375-375: Avoid specifying long messages outside the exception class
(TRY003)
395-398: Abstract raise to an inner function
(TRY301)
405-408: Abstract raise to an inner function
(TRY301)
425-425: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
426-426: Do not catch blind exception: Exception
(BLE001)
428-431: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
430-430: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (8)
app/api/routers/breeze_buddy/templates/handlers.py (1)
12-15: LGTM! Import additions are correct.The UpdateTemplateRequest import is properly added to support the new PATCH endpoint functionality.
app/database/queries/breeze_buddy/template.py (1)
148-212: LGTM! Query construction is secure and correct.The function properly:
- Validates non-empty update_fields
- Maps API field names to database columns
- Applies
::jsonbcasting for JSON fields- Uses parameterized queries (the S608 static analysis warning about SQL injection is a false positive)
- Always updates
updated_attimestamp- Returns all necessary fields via RETURNING clause
The missing
descriptionfield handling is intentional since description is not a database column, which aligns with the issue flagged in the handlers file.app/database/accessor/breeze_buddy/template.py (2)
24-24: LGTM! Import addition is correct.The update_template_query import is properly added to support the new update functionality.
280-329: LGTM! Update function implementation is correct and consistent.The function properly:
- Serializes JSONB fields (flow, expected_payload_schema, expected_callback_response_schema, configurations) to JSON strings before database storage
- Matches the serialization pattern used in
create_template(lines 98-110)- Calls the query function with processed fields
- Decodes the result using the existing decoder
- Handles errors gracefully by returning None (which the handler converts to appropriate HTTP errors)
The broad exception handling and logging approach is consistent with other accessor functions in this file.
app/api/routers/breeze_buddy/templates/__init__.py (3)
21-21: LGTM! Import addition is correct.The UpdateTemplateRequest import properly supports the new PATCH endpoint.
31-31: LGTM! Import addition is correct.The update_template_handler import is properly added to wire the PATCH endpoint.
167-226: Remove unsupported field from documentation.The endpoint implementation is correct, but the docstring should not mention the
descriptionfield since it's not actually supported in the implementation (as noted in the handlers.py review).Fix the docstring to remove unsupported description field
Request Body (all fields optional): { "template_name": "new-name", # Optional: Update template name "identifier": "new-shop-id", # Optional: Update shop identifier "outbound_number_id": "uuid", # Optional: Update outbound number "is_active": true, # Optional: Update active status - "flow": {...}, # Optional: Update flow structure + "flow": {...}, # Optional: Update flow structure "expected_payload_schema": {...}, # Optional: Update payload schema "expected_callback_response_schema": {...}, # Optional: Update callback schema "configurations": { # Optional: Update configurations "tts_voice_name": "rhea", "stt_language": "en-US", "payload_based_language_selection": false } }Note: If the description field is intentionally not documented here because it's in the schema but not implemented, that's fine - just be aware it creates an inconsistency between the UpdateTemplateRequest schema and the actual API behavior.
Likely an incorrect or invalid review comment.
app/ai/voice/agents/breeze_buddy/template/types.py (1)
138-152: Model structure is correct, but note the unsupporteddescriptionfield.The UpdateTemplateRequest model is properly structured with all optional fields. However, as flagged in the handlers.py review, the
descriptionfield (line 148) is not processed by the update handler and is not a database column. This creates a misleading API surface.Consider removing this field or implementing full support for it across all layers (database schema, query, accessor, and handler).
Likely an incorrect or invalid review comment.
| async def update_template_handler( | ||
| template_id: str, update_data: UpdateTemplateRequest, current_user: UserInfo | ||
| ): | ||
| """ | ||
| Update a template with partial updates. | ||
|
|
||
| Args: | ||
| template_id: Template UUID | ||
| update_data: Fields to update | ||
| current_user: Current authenticated user | ||
|
|
||
| Returns: | ||
| Updated TemplateModel | ||
|
|
||
| Raises: | ||
| HTTPException: 404 if template not found, 403 if access denied, 400/500 on error | ||
| """ | ||
| logger.info( | ||
| f"User {current_user.username} (role: {current_user.role}) updating template {template_id}" | ||
| ) | ||
|
|
||
| try: | ||
| # First, get the existing template to validate access | ||
| existing_template = await get_template_by_id(template_id) | ||
|
|
||
| if not existing_template: | ||
| logger.warning(f"Template not found: {template_id}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=f"Template not found: {template_id}", | ||
| ) | ||
|
|
||
| # Validate RBAC access | ||
| validate_template_access( | ||
| current_user, | ||
| existing_template.merchant_id, | ||
| existing_template.shop_identifier, | ||
| operation="update template", | ||
| ) | ||
|
|
||
| # Build update fields dictionary (only include fields that are set) | ||
| update_fields = {} | ||
|
|
||
| if update_data.template_name is not None: | ||
| update_fields["template_name"] = update_data.template_name | ||
|
|
||
| if update_data.identifier is not None: | ||
| update_fields["identifier"] = update_data.identifier | ||
|
|
||
| if update_data.outbound_number_id is not None: | ||
| # Validate outbound_number_id if provided | ||
| outbound_number = await get_outbound_number_by_id( | ||
| update_data.outbound_number_id | ||
| ) | ||
| if not outbound_number: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=f"Outbound number with ID {update_data.outbound_number_id} does not exist", | ||
| ) | ||
| update_fields["outbound_number_id"] = update_data.outbound_number_id | ||
|
|
||
| if update_data.is_active is not None: | ||
| update_fields["is_active"] = update_data.is_active | ||
|
|
||
| if update_data.flow is not None: | ||
| # Validate flow structure | ||
| if "initial_node" not in update_data.flow: | ||
| raise ValueError("initial_node must be specified in flow structure") | ||
| if "nodes" not in update_data.flow or not update_data.flow["nodes"]: | ||
| raise ValueError("nodes must be specified in flow structure") | ||
| update_fields["flow"] = update_data.flow | ||
|
|
||
| if update_data.expected_payload_schema is not None: | ||
| update_fields["expected_payload_schema"] = ( | ||
| update_data.expected_payload_schema | ||
| ) | ||
|
|
||
| if update_data.expected_callback_response_schema is not None: | ||
| update_fields["expected_callback_response_schema"] = ( | ||
| update_data.expected_callback_response_schema | ||
| ) | ||
|
|
||
| if update_data.configurations is not None: | ||
| update_fields["configurations"] = update_data.configurations.model_dump( | ||
| exclude_none=True | ||
| ) | ||
|
|
||
| # Check if any fields to update | ||
| if not update_fields: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="No fields to update", | ||
| ) | ||
|
|
||
| # Update the template | ||
| now = datetime.now(timezone.utc) | ||
| updated_template = await update_template(template_id, update_fields, now) | ||
|
|
||
| if not updated_template: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to update template", | ||
| ) | ||
|
|
||
| logger.info( | ||
| f"Successfully updated template {template_id} with fields: {list(update_fields.keys())}" | ||
| ) | ||
|
|
||
| return { | ||
| "status": "success", | ||
| "template_id": updated_template.id, | ||
| "message": f"Template updated successfully with {len(update_fields)} field(s)", | ||
| "updated_fields": list(update_fields.keys()), | ||
| } | ||
|
|
||
| except HTTPException: | ||
| raise | ||
| except ValueError as e: | ||
| logger.error(f"Validation error updating template: {e}") | ||
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | ||
| except Exception as e: | ||
| logger.error(f"Error updating template: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Error updating template: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
Critical: The description field in UpdateTemplateRequest is not being processed.
The UpdateTemplateRequest model includes a description field (line 148 in types.py), but this handler doesn't process it. This means users can send a description in the PATCH request, but it will be silently ignored. Looking at the database schema (create_template_query), the description field doesn't exist in the database table.
This creates a misleading API surface where users might expect to update the description, but the operation has no effect.
Recommendation: Either:
- Remove the
descriptionfield from UpdateTemplateRequest (and CreateTemplateRequest) if it's not needed, OR - Add description to the database schema and implement full support for storing/updating it
Option 1: Remove description field from UpdateTemplateRequest
In app/ai/voice/agents/breeze_buddy/template/types.py:
class UpdateTemplateRequest(BaseModel):
"""
Request model for updating a template.
All fields are optional - only provided fields will be updated.
"""
template_name: Optional[str] = None
identifier: Optional[str] = None
outbound_number_id: Optional[str] = None
is_active: Optional[bool] = None
- description: Optional[str] = None
flow: Optional[Dict[str, Any]] = None
expected_payload_schema: Optional[Dict[str, Any]] = None
expected_callback_response_schema: Optional[Dict[str, Any]] = None
configurations: Optional[ConfigurationModel] = NoneCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
333-336: Abstract raise to an inner function
(TRY301)
361-364: Abstract raise to an inner function
(TRY301)
373-373: Abstract raise to an inner function
(TRY301)
373-373: Avoid specifying long messages outside the exception class
(TRY003)
375-375: Abstract raise to an inner function
(TRY301)
375-375: Avoid specifying long messages outside the exception class
(TRY003)
395-398: Abstract raise to an inner function
(TRY301)
405-408: Abstract raise to an inner function
(TRY301)
425-425: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
426-426: Do not catch blind exception: Exception
(BLE001)
428-431: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
430-430: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In @app/api/routers/breeze_buddy/templates/handlers.py around lines 306 - 431,
The handler ignores the description field on UpdateTemplateRequest causing a
silent no-op; either remove the field from the request models or persist it—if
you persist it, add a description column to the templates table
(create_template_query), update the create_template and update_template DB
queries/functions to include description, and modify update_template_handler to
accept update_data.description (add update_fields["description"] when provided,
validate/serialize as needed) so PATCH actually updates description; if
removing, delete description from UpdateTemplateRequest and
CreateTemplateRequest types to keep API surface consistent.
There was a problem hiding this comment.
Need for future development
There was a problem hiding this comment.
@yugesh-ganipudi, understood! Since this will be addressed in future development, would you like me to create a GitHub issue to track this work? I can document the requirement to either add full description field support (including database schema changes) or remove it from the request models to maintain API consistency.
- Now one can update templates using this endpoint
yugesh-ganipudi
force-pushed
the
BZ-47763-patch-endpoint-for-updating-templates
branch
from
3c75337 to
28abbc9
Compare
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.