Fetching audio recording using call_sid #428
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 WalkthroughA new API endpoint for fetching call recordings is introduced, retrieving recording URLs and provider information from the database, authenticating with provider-specific credentials (Twilio or Exotel), fetching audio via HTTP, and returning it with appropriate headers. The implementation spans the API router, database accessor, and query layers. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Endpoint
participant DB as Database
participant AuthN as Auth Layer
participant Provider as Recording Provider<br/>(Twilio/Exotel)
Client->>API: GET /recordings/{call_sid}
API->>AuthN: Validate JWT Token
AuthN-->>API: Token Valid
API->>DB: Query recording_url & provider<br/>by call_id
DB-->>API: (recording_url, provider_name)
alt Provider Credentials Configured
API->>API: Build Provider-Specific<br/>Basic Auth Header
API->>Provider: Fetch audio with Auth
Provider-->>API: Audio Content (200)
else Missing/Unsupported Provider
API-->>Client: Error Response
end
API->>Client: Return Audio Content<br/>+ Content-Type + Filename
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (3)
app/api/routers/breeze_buddy.py (3)
796-811: Consider streaming for large audio files.Using
await response.read()loads the entire audio file into memory before returning it. For large recordings, this could cause memory pressure on the server.Consider using
StreamingResponseto stream the audio content:from fastapi.responses import StreamingResponse async def stream_audio(): async with session.get(recording_url, headers=headers, timeout=aiohttp.ClientTimeout(total=60)) as response: if response.status != 200: raise HTTPException(...) async for chunk in response.content.iter_chunked(8192): yield chunk return StreamingResponse( stream_audio(), media_type=content_type, headers={"Content-Disposition": f'attachment; filename="recording_{call_sid}.mp3"'} )Note: This would require restructuring the session handling since the session must remain open during streaming.
808-810: Filename extension doesn't match actual content type.The filename is hardcoded as
.mp3but the actual content type could be different (e.g.,audio/wav,audio/ogg). Consider deriving the extension from the content type.import mimetypes # Derive extension from content type extension = mimetypes.guess_extension(content_type) or '.mp3' filename = f"recording_{call_sid}{extension}"
742-782: Auth header construction is duplicated - consider refactoring.The Basic Auth header construction logic is nearly identical for both Twilio and Exotel providers. This could be extracted to reduce duplication.
def _get_provider_credentials(provider: str) -> tuple[str, str]: """Get credentials for the given provider.""" if provider.upper() == "TWILIO": if not TWILIO_ACCOUNT_SID or not TWILIO_AUTH_TOKEN: raise HTTPException(status_code=500, detail="Twilio credentials not configured") return TWILIO_ACCOUNT_SID, TWILIO_AUTH_TOKEN elif provider.upper() == "EXOTEL": if not EXOTEL_API_KEY or not EXOTEL_API_TOKEN: raise HTTPException(status_code=500, detail="Exotel credentials not configured") return EXOTEL_API_KEY, EXOTEL_API_TOKEN else: raise HTTPException(status_code=400, detail=f"Unsupported provider: {provider}") def _build_basic_auth_header(username: str, password: str) -> dict[str, str]: """Build Basic Auth header.""" auth_string = f"{username}:{password}" base64_auth = b64encode(auth_string.encode('ascii')).decode('ascii') return {"Authorization": f"Basic {base64_auth}"}This simplifies the main endpoint logic and makes it easier to add new providers in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/api/routers/breeze_buddy.py(4 hunks)app/database/accessor/__init__.py(2 hunks)app/database/accessor/breeze_buddy/lead_call_tracker.py(2 hunks)app/database/queries/breeze_buddy/lead_call_tracker.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/database/accessor/__init__.py (1)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
get_recording_url_and_provider_by_call_id(398-431)
🪛 Ruff (0.14.8)
app/database/queries/breeze_buddy/lead_call_tracker.py
360-367: Possible SQL injection vector through string-based query construction
(S608)
app/database/accessor/breeze_buddy/lead_call_tracker.py
427-427: Consider moving this statement to an else block
(TRY300)
429-429: Do not catch blind exception: Exception
(BLE001)
app/api/routers/breeze_buddy.py
717-717: 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)
734-737: Abstract raise to an inner function
(TRY301)
748-751: Abstract raise to an inner function
(TRY301)
765-768: Abstract raise to an inner function
(TRY301)
779-782: Abstract raise to an inner function
(TRY301)
791-794: Abstract raise to an inner function
(TRY301)
819-819: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (3)
app/database/queries/breeze_buddy/lead_call_tracker.py (1)
353-369: LGTM - Query implementation is correct and safe.The parameterized query correctly uses
$1placeholder with values passed separately, which prevents SQL injection. The static analysis warning (S608) is a false positive since the f-string only interpolates constant table names defined at module level, not user input.app/database/accessor/__init__.py (1)
20-20: LGTM - Proper export of new accessor function.The new function is correctly imported and added to
__all__following the existing patterns in the module.Also applies to: 61-61
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
398-431: LGTM - Accessor function follows established patterns.The implementation correctly:
- Uses parameterized queries via the query function
- Handles missing data and exceptions gracefully
- Returns
Nonewhen recording_url or call_provider is missing- Follows the same error handling pattern as other functions in this file
The broad
Exceptioncatch is consistent with the existing codebase pattern where accessor functions returnNoneon failure, allowing the caller to handle the error appropriately.
app/api/routers/breeze_buddy.py
Outdated
| # Fetch the audio file | ||
| logger.info(f"Fetching recording from URL: {recording_url}") | ||
| async with session.get(recording_url, headers=headers) as response: | ||
| if response.status != 200: | ||
| logger.error( | ||
| f"Failed to fetch recording. Status: {response.status}, URL: {recording_url}" | ||
| ) | ||
| raise HTTPException( | ||
| status_code=response.status, | ||
| detail=f"Failed to fetch recording from provider. Status: {response.status}" | ||
| ) |
There was a problem hiding this comment.
Add timeout to prevent hanging requests.
The HTTP request to fetch the recording has no timeout configured. If the provider's server is slow or unresponsive, this request could hang indefinitely, consuming server resources.
# Fetch the audio file
logger.info(f"Fetching recording from URL: {recording_url}")
- async with session.get(recording_url, headers=headers) as response:
+ async with session.get(recording_url, headers=headers, timeout=aiohttp.ClientTimeout(total=60)) as response:
if response.status != 200:Alternatively, configure the timeout at the session level:
async with aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=60)) as session:🧰 Tools
🪛 Ruff (0.14.8)
791-794: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy.py around lines 784 to 794, the async HTTP GET
has no timeout and can hang; update the code to supply a timeout — either create
the ClientSession with a ClientTimeout (e.g., aiohttp.ClientTimeout(total=60))
or pass a timeout argument to session.get(...,
timeout=aiohttp.ClientTimeout(total=60)) — and ensure any timeout exceptions
(asyncio.TimeoutError or aiohttp.ClientError) are handled and converted to an
appropriate HTTPException so the request fails fast instead of hanging.
app/api/routers/breeze_buddy.py
Outdated
| except Exception as e: | ||
| logger.error(f"Error fetching call recording: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail=f"Internal server error while fetching recording: {str(e)}" | ||
| ) from e |
There was a problem hiding this comment.
Avoid leaking internal error details to clients.
Including str(e) in the error response could expose sensitive internal information (stack traces, file paths, or internal state) to API consumers.
except Exception as e:
logger.error(f"Error fetching call recording: {e}", exc_info=True)
raise HTTPException(
status_code=500,
- detail=f"Internal server error while fetching recording: {str(e)}"
+ detail="Internal server error while fetching recording"
) from e🧰 Tools
🪛 Ruff (0.14.8)
819-819: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy.py around lines 815 to 820, the HTTPException
detail currently includes str(e) which may leak internal information to clients;
change the response to return a generic error message (e.g., "Internal server
error while fetching recording") without including exception text, keep the full
error logged via logger.error(exc_info=True), and optionally generate and return
an opaque error ID in the response if you want clients to correlate with server
logs.
yugesh-ganipudi
force-pushed
the
BZ-47338-endpoint-for-fetching-call-recording-using-call-sid
branch
2 times, most recently
from
f6aa51b to
fc3743b
Compare
| async with aiohttp.ClientSession() as session: | ||
| if call_provider.upper() == "TWILIO": | ||
| # Twilio authentication using Basic Auth | ||
| from app.ai.voice.agents.breeze_buddy.services.telephony.twilio.recording import ( |
yugesh-ganipudi
force-pushed
the
BZ-47338-endpoint-for-fetching-call-recording-using-call-sid
branch
from
fc3743b to
81310ab
Compare
- We fetch recording url and call_provider with call_sid - We get credentials of provider from ENV and fetch audio using them
yugesh-ganipudi
force-pushed
the
BZ-47338-endpoint-for-fetching-call-recording-using-call-sid
branch
from
81310ab to
d15d4b4
Compare
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.