Conversation
|
Warning Rate limit exceeded@jpark0506 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough음성 파일 업로드·S3 저장, STT·감정 분석(융합), 결과를 외부 챗봇 API로 전송하는 AnalyzeChatService와 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as 클라이언트
participant API as /analyze/chat (FastAPI)
participant Service as AnalyzeChatService
participant S3 as S3
participant STT as STT 서비스
participant Emotion as Emotion 서비스
participant DB as DB
participant Chatbot as 외부 챗봇API
Client->>API: 파일 + session_id, user_id, question (multipart/form)
API->>Service: analyze_and_send(file, session_id, user_id, question)
par 병렬 I/O (asyncio.to_thread)
Service->>S3: _upload_to_s3(file) (thread)
Service->>STT: _transcribe_audio(file) (thread)
Service->>Emotion: _analyze_emotion(file) (thread)
end
Service->>Service: 텍스트·오디오 감정 융합 (fuse_VA)
Service->>DB: _get_user_name(user_id)
Service->>Chatbot: _send_to_chatbot(통합 페이로드) (httpx)
Chatbot-->>Service: 응답 / 오류
Service-->>API: 결과 Dict 반환 또는 예외 발생
API-->>Client: 성공/에러 응답
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements a new /analyze/chat endpoint for processing voice files independently from the emotion diary system. The endpoint handles voice file upload to S3, performs STT (Speech-to-Text) conversion, analyzes emotions from both audio and text, and forwards the results to an external chatbot API via SQS messaging.
Key Changes:
- New
AnalyzeChatServiceclass with dependency injection pattern for voice analysis and chatbot API integration - Async processing optimization using
asyncio.to_thread()for CPU-intensive tasks (S3 upload, STT, emotion analysis) - S3 file organization by session and user with UUID-based filename deduplication
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
app/services/analyze_chat_service.py |
New service class implementing voice analysis pipeline and chatbot API integration |
app/main.py |
Added /analyze/chat endpoint with dependency injection and error handling |
requirements.txt |
Added httpx>=0.25.0 for external API communication |
app/dto.py |
Made anxiety_bps field optional with default value in VoiceAnalyzePreviewResponse |
create_test_user.py |
Utility script for creating test users with configurable attributes |
update_emotion.py |
Script for updating emotion values in voice_analyze table |
list_care_users.py |
Utility script for listing CARE role users and their connections |
check_user.py |
Script for verifying user information and relationships |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/main.py
Outdated
| return get_analyze_chat_service(db) | ||
|
|
||
|
|
||
| @analyze_router.post("/analyze/chat") |
There was a problem hiding this comment.
The endpoint path is duplicated. The router already has the prefix "/analyze", so the endpoint decorator should be @analyze_router.post("/chat") instead of @analyze_router.post("/analyze/chat"). This will result in the actual path being /analyze/analyze/chat instead of the intended /analyze/chat.
| @analyze_router.post("/analyze/chat") | |
| @analyze_router.post("/chat") |
app/services/analyze_chat_service.py
Outdated
| import os as os_module | ||
| name, ext = os_module.path.splitext(file.filename) |
There was a problem hiding this comment.
The os module is already imported at line 1, so this redundant import as os_module is unnecessary. Use the already imported os module instead or remove this line and use os.path.splitext() directly.
| import os as os_module | |
| name, ext = os_module.path.splitext(file.filename) | |
| name, ext = os.path.splitext(file.filename) |
| class FileWrapper: | ||
| def __init__(self, content: bytes, filename: str, content_type: str): | ||
| self.file = BytesIO(content) | ||
| self.filename = filename | ||
| self.content_type = content_type |
There was a problem hiding this comment.
The FileWrapper class is duplicated in both _transcribe_audio and _analyze_emotion methods. This code duplication should be refactored by extracting FileWrapper as a module-level class or a separate helper method to improve maintainability.
app/services/analyze_chat_service.py
Outdated
| try: | ||
| error_response = e.response.json() | ||
| raise HTTPException(status_code=e.response.status_code, detail=error_response) | ||
| except: |
There was a problem hiding this comment.
Bare except clause without specifying exception type is a bad practice. This will catch all exceptions including KeyboardInterrupt and SystemExit. Specify the expected exception type (e.g., except ValueError: or except json.JSONDecodeError:) to handle only the intended error case.
| except: | |
| except ValueError: |
| raise HTTPException(status_code=500, detail="S3_BUCKET_NAME not configured") | ||
|
|
||
| # S3 키 생성: {session_id}/{user_id}/{filename} | ||
| s3_key = f"{session_id}/{user_id}/{filename}" if session_id and user_id else f"chat/{filename}" |
There was a problem hiding this comment.
The S3 key construction using user-provided session_id and user_id without sanitization could lead to path traversal vulnerabilities. If these values contain characters like ../, it could allow access to unintended S3 paths. Consider sanitizing these inputs by removing or replacing special characters before constructing the S3 key.
|
|
||
| if voice_analyze.top_emotion != old_emotion: | ||
| print(f"⚠️ 현재 감정이 '{voice_analyze.top_emotion}'입니다. (예상: '{old_emotion}')") | ||
| print(f" 그대로 '{new_emotion}'로 변경하시겠습니까?") |
There was a problem hiding this comment.
The script prints a confirmation message but doesn't wait for user input before proceeding with the update. When the current emotion doesn't match the expected old_emotion, the script should prompt for user confirmation (e.g., using input()) instead of just printing a message and proceeding automatically.
| print(f" 그대로 '{new_emotion}'로 변경하시겠습니까?") | |
| confirm = input(f" 그대로 '{new_emotion}'로 변경하시겠습니까? (y/N): ") | |
| if confirm.lower() != 'y': | |
| print("⏹️ 업데이트를 취소합니다.") | |
| return False |
| if file.filename: | ||
| # 원본 파일명이 있으면 확장자 추출 후 고유한 이름 생성 | ||
| import os as os_module | ||
| name, ext = os_module.path.splitext(file.filename) | ||
| filename = f"{name}_{current_time}_{unique_id}{ext}" |
There was a problem hiding this comment.
The filename constructed from file.filename could contain path separators or special characters that may cause issues with S3 keys or file system operations. The original filename should be sanitized (e.g., using os.path.basename() and removing special characters) before using it in the constructed filename to prevent potential security issues or path traversal attempts.
app/services/analyze_chat_service.py
Outdated
| filename = f"audio_{current_time}_{unique_id}.wav" | ||
|
|
||
| # 1. S3 파일 업로드 (별도 스레드에서 실행) | ||
| s3_key = await asyncio.to_thread( |
There was a problem hiding this comment.
Variable s3_key is not used.
| s3_key = await asyncio.to_thread( | |
| await asyncio.to_thread( |
check_user.py
Outdated
| import os | ||
| sys.path.insert(0, os.path.dirname(__file__)) | ||
|
|
||
| from app.database import SessionLocal, get_db |
There was a problem hiding this comment.
Import of 'SessionLocal' is not used.
| from app.database import SessionLocal, get_db | |
| from app.database import get_db |
create_test_user.py
Outdated
|
|
||
| from app.database import SessionLocal | ||
| from app.auth_service import get_auth_service | ||
| from datetime import datetime |
There was a problem hiding this comment.
Import of 'datetime' is not used.
| from datetime import datetime |
|
참고로 로컬에서는 10-15초 정도 걸리는데..클라우드 환경은 지켜봐야할 듯 합니다.. |
jpark0506
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/dto.py (1)
164-171:anxiety_bps를 Optional + 기본값 0으로 두는 설계는 fear→anxiety 매핑 의도와 잘 맞습니다.
per_emotion_bps["fear"]를 출력 시 anxiety로 노출하려는 주석(fear -> anxiety (출력용))과 구조가 일관되고,- 기본값 0을 줬기 때문에 클라이언트는 필드 존재 여부를 신경 쓰지 않고 그대로 수치로 사용할 수 있습니다.
한 가지 주의할 점은, 이 필드 이름 변경에 맞춰 사용 측(예:
VoiceAnalyzePreviewResponse를 생성하는 엔드포인트)도 모두anxiety_bps로 맞춰야 한다는 것입니다. 아래app/main.py의test_emotion_analyze는 아직fear_bps이름으로 생성하고 있어서 런타임 에러가 날 수 있으니 그 부분에서 별도로 코멘트 드리겠습니다.app/main.py (1)
851-910:VoiceAnalyzePreviewResponse생성 시fear_bps인자가 존재하지 않아 런타임 에러가 납니다.
app/dto.py에서VoiceAnalyzePreviewResponse가 다음과 같이 정의되어 있습니다.class VoiceAnalyzePreviewResponse(BaseModel): ... angry_bps: int anxiety_bps: Optional[int] = 0 # fear -> anxiety (출력용) surprise_bps: int그런데 여기에서는 여전히
fear_bps를 사용하고 있습니다.return VoiceAnalyzePreviewResponse( voice_id=None, happy_bps=happy, sad_bps=sad, neutral_bps=neutral, angry_bps=angry, fear_bps=fear, # ← 현재 DTO에 존재하지 않는 필드 surprise_bps=surprise, ... )이 상태로는
/test/voice/analyze호출 시TypeError: __init__() got an unexpected keyword argument 'fear_bps'가 발생합니다. DTO 변경에 맞춰 아래처럼 수정해야 합니다.- fear = to_bps(probs.get("fear", 0)) + fear = to_bps(probs.get("fear", 0)) ... - fear_bps=fear, + anxiety_bps=fear, # fear -> anxiety 매핑 surprise_bps=surprise,필드명만 맞추면 기존 계산 로직은 그대로 재사용할 수 있습니다.
🧹 Nitpick comments (9)
check_user.py (2)
10-45: CLI에서get_db()제너레이터를 직접next()로 쓰기보다는SessionLocal()을 직접 사용하는 편이 더 명확합니다.현재 구현도
finally블록에서db.close()를 호출하므로 리소스 누수는 없지만,get_db()는 FastAPI 의존성용 제너레이터라 CLI 스크립트에서는 살짝 어색합니다. 이미SessionLocal을 import 하고 있으니 아래처럼 단순화하면 가독성이 좋아집니다.-from app.database import SessionLocal, get_db +from app.database import SessionLocal def check_user(username: str): """사용자 정보 확인""" - db = next(get_db()) + db = SessionLocal()
list_care_users.py,update_emotion.py도 같은 패턴을 쓰고 있어서 함께 정리하면 관리가 더 쉬울 것 같습니다.
1-3: lint 수준의 자잘한 개선 포인트 (shebang + f-string).
- 파일 상단에 shebang (
#!/usr/bin/env python3)이 있지만 실행 비트가 없다면(예:chmod +x check_user.py안 된 경우) 린터가 경고를 낼 수 있습니다. 그냥 모듈로만 사용할 계획이면 shebang 을 지우는 것도 방법입니다.print(f"✅ 사용자 정보:")처럼 중괄호가 없는 f-string 들(예: Line 20, 32, 37)은 단순 문자열이므로fprefix를 제거하면 Ruff(F541) 경고를 없앨 수 있습니다.동작에는 영향이 없는 부분이라 나중에 린트 정리할 때 한 번에 손보셔도 됩니다.
Also applies to: 20-42
update_emotion.py (2)
10-45: 여기도 CLI에서는get_db()대신SessionLocal()직접 사용하는 편이 더 자연스럽습니다.
check_user.py와 동일하게db = next(get_db())를 쓰고 있지만, 이 제너레이터는 FastAPI 의존성용이라 스크립트에서는 약간 간접적입니다. 아래처럼SessionLocal()을 직접 쓰는 패턴으로 통일하는 것을 권장합니다.-from app.database import get_db +from app.database import SessionLocal def update_emotion(voice_analyze_id: int, old_emotion: str, new_emotion: str): """감정 업데이트""" - db = next(get_db()) + db = SessionLocal()또한
except Exception as e:로 모든 예외를 한 번에 잡고 문자열만 출력하고 끝내기보다는, 최소한 로그에 stack trace를 남기거나(예:traceback.print_exc()), 필요 시 특정 예외 타입만 별도로 처리하는 식으로 확장성을 생각해 두면 좋습니다.
47-49:__main__에서 하드코딩된 업데이트 호출은 실수로 데이터가 바뀔 위험이 있습니다.현재는 스크립트를 실행하면 무조건
voice_analyze_id=1,top_emotion: anxiety → fear로 변경을 시도합니다. 실제 운영/개발 DB를 다룰 때는 실수로 실행해도 안전하도록 CLI 인자로 받는 쪽이 더 좋습니다.-if __name__ == "__main__": - # voice_analyze_id=1, anxiety -> fear - update_emotion(voice_analyze_id=1, old_emotion="anxiety", new_emotion="fear") +if __name__ == "__main__": + import argparse + parser = argparse.ArgumentParser(description="voice_analyze.top_emotion 업데이트") + parser.add_argument("voice_analyze_id", type=int) + parser.add_argument("old_emotion", type=str) + parser.add_argument("new_emotion", type=str) + args = parser.parse_args() + update_emotion( + voice_analyze_id=args.voice_analyze_id, + old_emotion=args.old_emotion, + new_emotion=args.new_emotion, + )이렇게 하면 실수로 실행했을 때도 명시적으로 인자를 주지 않으면 아무 변화가 일어나지 않습니다.
list_care_users.py (1)
10-34: CARE 사용자 조회 로직은 단순하고 명확합니다.
User.role == 'CARE'필터,connecting_user_code로 연결된 사용자 조회 등 전반적인 흐름은 모델 정의와 잘 맞습니다.세부적으로는:
- DB 세션 생성 패턴(
db = next(get_db()))은check_user.py에서 언급한 것처럼SessionLocal()직사용으로 통일해도 좋고,print(f" → ⚠️ 연결된 피보호자를 찾을 수 없음")처럼 중괄호 없는 f-string은fprefix를 제거하면 Ruff 경고(F541)를 줄일 수 있습니다.동작 자체에는 문제가 없어 나중에 린트/리팩터링 타이밍에 같이 정리하면 될 것 같습니다.
app/main.py (1)
976-1003:analyze_chat의 의존성 및 예외 처리 구조는 적절합니다만, 예외 체이닝을 추가하면 디버깅에 도움이 됩니다.
- DI:
analyze_chat_service: "AnalyzeChatService" = Depends(get_analyze_chat_service_dep)구조는 FastAPI 관례에 부합합니다.- 예외 처리도
HTTPException은 그대로 통과시키고, 나머지는 500으로 래핑해서 반환하는 패턴이라 괜찮습니다.추가로 디버깅/로그 관점에서:
- except Exception as e: - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + except Exception as e: + # 필요하다면 여기서 logger로 예외를 기록한 뒤, + raise HTTPException( + status_code=500, + detail=f"Internal server error: {e!s}", + ) from e처럼
from e로 체이닝해 두면 추후 스택 추적 시 원인 파악이 더 쉬워집니다.app/services/analyze_chat_service.py (3)
56-65: S3 키(s3_key)를 받아두고 사용하지 않고 있습니다.s3_key = await asyncio.to_thread( self._upload_to_s3, ... )
- 현재
s3_key는 이후 어디에서도 사용되지 않습니다.- 만약 chatbot API 쪽에서 원본 음성 파일의 위치를 필요로 한다면,
_send_to_chatbot의request_payload에s3_key를 포함시키는 게 자연스럽고,- 그렇지 않다면 단순히 변수 할당을 없애 lint(F841) 경고를 제거할 수 있습니다.
예를 들어 S3 키를 payload에 포함시키려면:
- s3_key = await asyncio.to_thread( + s3_key = await asyncio.to_thread( self._upload_to_s3, ... ) ... - return await self._send_to_chatbot( + return await self._send_to_chatbot( content=content, emotion=emotion_data, question=question, user_id=user_id, - user_name=user_name + user_name=user_name, + s3_key=s3_key, )같은 식으로 확장하는 방향을 고려해 볼 수 있습니다.
211-221:user_id파라미터 이름과get_user_by_username호출이 의미상 어긋납니다.def _get_user_name(self, user_id: str) -> str: ... auth_service = get_auth_service(self.db) user = auth_service.get_user_by_username(user_id)
- 외부 API/엔드포인트 입장에서는
user_id라는 이름이면 보통 숫자 PK나 user_code를 떠올리기 쉬운데,- 실제로는 username을 기대하면서
get_user_by_username을 호출하고 있습니다.API 사용자가 혼동하기 쉬우니, 다음 중 하나로 정리하는 것을 권장합니다.
- 이 값이 username 이라면: 전체 체인을
username으로 rename (AnalyzeChatService._get_user_name,analyze_and_send인자, FastAPI 엔드포인트 폼 필드 이름 등).- 진짜 DB PK나 user_code를 받을 계획이라면:
_get_user_name안에서 해당 필드 기준으로 조회하도록 쿼리를 수정.지금 상태도 동작은 하지만, API 계약이 모호해질 수 있어서 초기에 명확히 해 두는 것이 좋습니다.
223-268: httpx 에러 처리에서 예외 타입 및 체이닝 개선 권장현재 구현에서 bare
except와 예외 체이닝이 부재합니다. 또한 웹 쿼리 결과에 따르면 response.json() 실패는 httpx.DecodingError로 표현되며, 기존에 ValueError를 직접 잡는 코드는 httpx.DecodingError를 함께 처리하는 편이 안전합니다.개선 제안:
except:대신 구체적인 예외(httpx.DecodingError또는ValueError)만 잡기raise HTTPException(...) from e형태로 체이닝하여 원인 추적 가능하도록 하기str(e)대신e!s포맷 사용 (Ruff RUF010 경고 회피)예시:
- except httpx.HTTPStatusError as e: - # HTTP 에러 응답을 그대로 반환 - try: - error_response = e.response.json() - raise HTTPException(status_code=e.response.status_code, detail=error_response) - except: - raise HTTPException(status_code=e.response.status_code, detail=f"External API error: {e.response.text}") - except httpx.RequestError as e: - raise HTTPException(status_code=500, detail=f"External API request failed: {str(e)}") + except httpx.HTTPStatusError as e: + # HTTP 에러 응답을 그대로 반환 시도 + try: + error_response = e.response.json() + except httpx.DecodingError: + raise HTTPException( + status_code=e.response.status_code, + detail=f"External API error: {e.response.text}", + ) from e + else: + raise HTTPException( + status_code=e.response.status_code, + detail=error_response, + ) from e + except httpx.RequestError as e: + raise HTTPException( + status_code=500, + detail=f"External API request failed: {e!s}", + ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/dto.py(1 hunks)app/main.py(3 hunks)app/services/analyze_chat_service.py(1 hunks)check_user.py(1 hunks)create_test_user.py(1 hunks)list_care_users.py(1 hunks)requirements.txt(1 hunks)update_emotion.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
check_user.py (2)
app/database.py (1)
get_db(38-44)app/models.py (1)
User(8-29)
update_emotion.py (2)
app/database.py (1)
get_db(38-44)app/models.py (1)
VoiceAnalyze(85-110)
app/main.py (2)
app/database.py (1)
get_db(38-44)app/services/analyze_chat_service.py (2)
get_analyze_chat_service(270-272)analyze_and_send(24-93)
list_care_users.py (2)
app/database.py (1)
get_db(38-44)app/models.py (1)
User(8-29)
create_test_user.py (1)
app/auth_service.py (2)
get_auth_service(297-299)signup(34-138)
app/services/analyze_chat_service.py (6)
app/services/va_fusion.py (1)
fuse_VA(202-291)app/nlp_service.py (1)
analyze_text_sentiment(202-204)app/emotion_service.py (1)
analyze_voice_emotion(225-227)app/stt_service.py (1)
transcribe_voice(145-147)app/s3_service.py (1)
upload_fileobj(26-32)app/auth_service.py (1)
get_auth_service(297-299)
🪛 Ruff (0.14.5)
check_user.py
1-1: Shebang is present but file is not executable
(EXE001)
20-20: f-string without any placeholders
Remove extraneous f prefix
(F541)
32-32: f-string without any placeholders
Remove extraneous f prefix
(F541)
37-37: f-string without any placeholders
Remove extraneous f prefix
(F541)
update_emotion.py
1-1: Shebang is present but file is not executable
(EXE001)
26-26: f-string without any placeholders
Remove extraneous f prefix
(F541)
35-35: f-string without any placeholders
Remove extraneous f prefix
(F541)
38-38: Consider moving this statement to an else block
(TRY300)
40-40: Do not catch blind exception: Exception
(BLE001)
42-42: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/main.py
976-976: 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)
987-987: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
988-988: Undefined name AnalyzeChatService
(F821)
988-988: 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)
1002-1002: Do not catch blind exception: Exception
(BLE001)
1003-1003: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1003-1003: Use explicit conversion flag
Replace with conversion flag
(RUF010)
list_care_users.py
1-1: Shebang is present but file is not executable
(EXE001)
31-31: f-string without any placeholders
Remove extraneous f prefix
(F541)
create_test_user.py
19-19: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
20-20: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
21-21: Possible hardcoded password assigned to function default: "password"
(S107)
23-23: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
39-39: f-string without any placeholders
Remove extraneous f prefix
(F541)
56-56: f-string without any placeholders
Remove extraneous f prefix
(F541)
61-61: f-string without any placeholders
Remove extraneous f prefix
(F541)
68-68: Do not catch blind exception: Exception
(BLE001)
69-69: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/services/analyze_chat_service.py
57-57: Local variable s3_key is assigned to but never used
Remove assignment to unused variable s3_key
(F841)
264-264: Do not use bare except
(E722)
265-265: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
267-267: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
267-267: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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: Agent
🔇 Additional comments (1)
requirements.txt (1)
20-20: httpx 의존성 추가는 구현과 일치합니다.
AnalyzeChatService에서 사용하는 기능(AsyncClient,post,timeout) 수준에서는httpx>=0.25.0이면 충분해 보입니다. 실제 배포/로컬 환경의httpx버전이 이 최소 요구 조건과 충돌하지 않는지만 한 번만 확인해 주세요.
|
솔직히 Fast api code를 봐서 알 수 있는게 많이 없어서 개요만 보고 질문 드리겠습니다 인수인계 : notion정리
|
ahtop00
left a comment
ahtop00
left a comment
There was a problem hiding this comment.
수고하셨습니다! 이미 AI가 많은 리뷰를 남겨놔서 제가 추가로 남길 부분이 몇 없네요...
HTTPException 관련 개선점이 있어서 해당 사항만 추가로 리뷰 남겼습니다!
app/services/analyze_chat_service.py
Outdated
| stt_result = transcribe_voice(wrapped_file) | ||
|
|
||
| if stt_result.get("error"): | ||
| raise HTTPException(status_code=400, detail=f"STT failed: {stt_result.get('error')}") |
There was a problem hiding this comment.
HTTPException를 직접 써도 괜찮지만, exception.py에서 사용자 정의 예외 클래스로 관리하고 있습니다. 후에 유지보수를 위해 exception에서 관련 에러 코드로 커스텀하고, message만 보내어 관리하는 식으로 가면 좋을 거 같습니다.
There was a problem hiding this comment.
오 그건 몰랐네요 말씀하신 방식이 유지보수에 좋을 것 같습니다. 수정해서 반영하겠습니다!
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
create_test_user.py (1)
21-22: 이 스크립트가 USER 전용이라면 CLI에서 CARE 선택은 막는 편이 더 명확합니다.현재
create_test_user()는 항상connecting_user_code=None으로signup을 호출하기 때문에,--role CARE를 주면 AuthService 로직상 항상 실패하게 됩니다. PR 코멘트에서 말씀하신 것처럼 이 스크립트를 USER 테스트용으로만 쓸 계획이라면, CLI choices에서"CARE"를 제거하거나, CARE가 들어오면 명시적으로 에러 메시지를 출력하게 하는 편이 혼란을 줄일 것 같습니다.- parser.add_argument("--role", type=str, choices=["USER", "CARE"], default="USER", help="역할 (기본값: USER)") + parser.add_argument("--role", type=str, choices=["USER"], default="USER", help="역할 (테스트용: USER만 지원)")Based on learnings, 이 스크립트는 USER 테스트 계정 생성용으로만 사용된다고 이해했습니다.
Also applies to: 81-83
app/services/analyze_chat_service.py (2)
115-140: STT/감정 분석용FileWrapper클래스가 중복되어 있어 헬퍼로 빼는 것을 고려할 수 있습니다.
_transcribe_audio와_analyze_emotion에 거의 동일한FileWrapper가 중복 정의되어 있어, 수정 시 두 군데를 같이 손봐야 합니다. 모듈 상단에 하나 정의하거나, private 헬퍼 메서드로 통합하면 유지보수성이 좋아집니다.+class _FileWrapper: + def __init__(self, content: bytes, filename: str, content_type: str): + self.file = BytesIO(content) + self.filename = filename + self.content_type = content_type ... - class FileWrapper: - def __init__(self, content: bytes, filename: str, content_type: str): - self.file = BytesIO(content) - self.filename = filename - self.content_type = content_type - - wrapped_file = FileWrapper(file_content, filename, content_type) + wrapped_file = _FileWrapper(file_content, filename, content_type) ... - class FileWrapper: - def __init__(self, content: bytes, filename: str, content_type: str): - self.file = BytesIO(content) - self.filename = filename - self.content_type = content_type - - wrapped_file = FileWrapper(file_content, filename, content_type) + wrapped_file = _FileWrapper(file_content, filename, content_type)Also applies to: 141-157
48-52:os_module미정의로 인해 업로드 시 바로 NameError가 발생합니다.
file.filename이 존재하는 정상 케이스에서os_module.path.splitext호출 시os_module이름이 정의되어 있지 않아 항상 예외가 납니다. 이미 상단에import os가 있으므로, 그대로os.path.splitext를 사용하면 됩니다.- if file.filename: - name, ext = os_module.path.splitext(file.filename) + if file.filename: + name, ext = os.path.splitext(file.filename) filename = f"{name}_{current_time}_{unique_id}{ext}"
🧹 Nitpick comments (4)
check_user.py (1)
10-45: 전반적으로 스크립트 로직은 문제없고, f-string 상수만 정리하면 좋겠습니다.
get_db()로 세션을 받아서finally에서db.close()하는 패턴은 CLI 용도로 충분해 보입니다.- 다만 Line 20, 32, 37처럼 포맷 변수 없는 f-string은 Ruff F541 경고만 유발하니, 단순 문자열로 바꾸면 정적 분석 노이즈를 줄일 수 있습니다.
- print(f"✅ 사용자 정보:") + print("✅ 사용자 정보:") ... - print(f"\n⚠️ 경고: 연결된 피보호자 코드가 설정되어 있지 않습니다.") + print("\n⚠️ 경고: 연결된 피보호자 코드가 설정되어 있지 않습니다.") ... - print(f"\n✅ 연결된 피보호자:") + print("\n✅ 연결된 피보호자:")create_test_user.py (1)
17-23: 테스트 스크립트이긴 하지만, 타입/로깅을 약간 다듬으면 정적 분석 경고를 줄일 수 있습니다.
- Optional 인자에 대해
str | None타입을 쓰면 Ruff의 RUF013 경고를 없앨 수 있습니다.- 상수 문자열만 출력하는 f-string(
print(f"사용자 생성 중...")등)은 단순 문자열로 바꾸면 됩니다.- CLI에서 예외를
except Exception으로 한 번 더 감싸는 것은 테스트용 스크립트라 허용되지만, 필요 없다면 제거하거나, 최소한Exception만 잡는 현재 형태를 유지하는 정도면 충분해 보입니다.-def create_test_user( - name: str = None, - username: str = None, - password: str = "test1234", - role: str = "USER", - birthdate: str = None -): +def create_test_user( + name: str | None = None, + username: str | None = None, + password: str = "test1234", + role: str = "USER", + birthdate: str | None = None, +): ... - print(f"사용자 생성 중...") + print("사용자 생성 중...")또한 이 스크립트가 “테스트용”임을 help 메시지에 명시해 두면, 기본 비밀번호 노출에 대한 오해도 줄일 수 있습니다.
Also applies to: 38-44, 67-73
app/services/analyze_chat_service.py (1)
93-113: S3 키에 session_id/user_id를 그대로 쓰는 부분은 최소한의 정규화가 있으면 더 안전합니다.보안 취약점까지는 아니지만,
session_id/user_id에/등의 문자가 들어오면 예기치 않은 키 구조가 만들어질 수 있습니다. 알파벳/숫자/-/_정도로만 normalize 하거나, 나머지를_로 치환하는 헬퍼를 한 번 거치면 S3 키가 더 예측 가능해집니다.- s3_key = f"{session_id}/{user_id}/{filename}" if session_id and user_id else f"chat/{filename}" + safe_session = (session_id or "unknown").replace("/", "_") + safe_user = (user_id or "unknown").replace("/", "_") + s3_key = f"{safe_session}/{safe_user}/{filename}" if session_id and user_id else f"chat/{filename}"app/main.py (1)
982-1003:analyze_chat내부의 광범위한 try/except는 전역 예외 핸들러와 중복될 여지가 있습니다.이미 파일 상단에
HTTPException및 일반Exception에 대한 전역 핸들러가 정의되어 있어서, 여기서 다시HTTPException을 re-raise 하고 일반Exception을 500으로 감싸는 로직은 거의 동일한 일을 두 번 하게 됩니다. 특별한 로깅/매핑이 없다면, 이 try/except 블록을 제거하고 예외를 그대로 전파시켜 전역 핸들러에 맡기는 편이 단순하고 Ruff BLE001 경고도 피할 수 있습니다.- try: - return await analyze_chat_service.analyze_and_send( - file=file, - session_id=session_id, - user_id=user_id, - question=question - ) - except HTTPException: - raise - except Exception as e: - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + return await analyze_chat_service.analyze_and_send( + file=file, + session_id=session_id, + user_id=user_id, + question=question, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/main.py(3 hunks)app/services/analyze_chat_service.py(1 hunks)check_user.py(1 hunks)create_test_user.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T10:18:23.542Z
Learnt from: jpark0506
Repo: safori-team/CARING-Back PR: 22
File: create_test_user.py:18-53
Timestamp: 2025-11-24T10:18:23.542Z
Learning: The create_test_user.py script in the CARING-Back repository is designed only for creating USER role test users, not CARE role users. CARE role creation is intentionally not supported in this script.
Applied to files:
create_test_user.py
🧬 Code graph analysis (4)
app/main.py (2)
app/database.py (1)
get_db(38-44)app/services/analyze_chat_service.py (2)
get_analyze_chat_service(268-270)analyze_and_send(24-91)
app/services/analyze_chat_service.py (6)
app/services/va_fusion.py (1)
fuse_VA(202-291)app/nlp_service.py (1)
analyze_text_sentiment(202-204)app/emotion_service.py (1)
analyze_voice_emotion(225-227)app/stt_service.py (1)
transcribe_voice(145-147)app/s3_service.py (1)
upload_fileobj(26-32)app/auth_service.py (1)
get_auth_service(297-299)
check_user.py (2)
app/database.py (1)
get_db(38-44)app/models.py (1)
User(8-29)
create_test_user.py (1)
app/auth_service.py (2)
get_auth_service(297-299)signup(34-138)
🪛 Ruff (0.14.5)
app/main.py
976-976: 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)
987-987: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
988-988: Undefined name AnalyzeChatService
(F821)
988-988: 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)
1002-1002: Do not catch blind exception: Exception
(BLE001)
1003-1003: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1003-1003: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/services/analyze_chat_service.py
49-49: Undefined name os_module
(F821)
262-262: Do not use bare except
(E722)
263-263: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
265-265: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
265-265: Use explicit conversion flag
Replace with conversion flag
(RUF010)
check_user.py
1-1: Shebang is present but file is not executable
(EXE001)
20-20: f-string without any placeholders
Remove extraneous f prefix
(F541)
32-32: f-string without any placeholders
Remove extraneous f prefix
(F541)
37-37: f-string without any placeholders
Remove extraneous f prefix
(F541)
create_test_user.py
18-18: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
20-20: Possible hardcoded password assigned to function default: "password"
(S107)
22-22: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
38-38: f-string without any placeholders
Remove extraneous f prefix
(F541)
55-55: f-string without any placeholders
Remove extraneous f prefix
(F541)
60-60: f-string without any placeholders
Remove extraneous f prefix
(F541)
67-67: Do not catch blind exception: Exception
(BLE001)
68-68: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (2)
app/main.py (1)
263-263: 라우터 구성과 의존성 주입이 기존 구조와 잘 맞습니다.
analyze_router = APIRouter(prefix="/analyze", ...)+@analyze_router.post("/chat")조합으로 최종 경로가/analyze/chat이 되어 PR 설명과 일치합니다.get_analyze_chat_service_dep를 통해 DB 세션 기반AnalyzeChatService를 주입하는 패턴도 기존auth_service,voice_service사용 방식과 일관적입니다.app.include_router(analyze_router)위치도 다른 라우터들과 동일 레벨이라 유지보수 측면에서 보기 좋습니다.Also applies to: 976-979, 1005-1013
app/services/analyze_chat_service.py (1)
248-265: Let me verify what exceptionsresponse.json()actually raises in httpx:Now let me verify the exact exception hierarchy one more time to be certain:
Based on my comprehensive verification, I've found an important issue with the review comment:
검토 의견에 오류가 있습니다. JSON 파싱 실패 시 올바른 예외를 명시하세요.
원본 코드의 bare
except:(라인 262)를 제거하는 것은 올바른 지적입니다. 그러나 제안된 예외 타입에 문제가 있습니다.httpx.DecodingError는 "malformed encoding"으로 인한 gzip, deflate, brotli, zstd 같은 압축 해제 오류를 나타내며, response.json()은 json.JSONDecodeError를 발생시킵니다. 따라서 정확한 수정은:
- try: - error_response = e.response.json() - raise HTTPException(status_code=e.response.status_code, detail=error_response) - except: - raise HTTPException(status_code=e.response.status_code, detail=f"External API error: {e.response.text}") + try: + error_response = e.response.json() + raise HTTPException( + status_code=e.response.status_code, + detail=error_response, + ) from e + except ValueError as parse_err: + raise HTTPException( + status_code=e.response.status_code, + detail=f"External API error: {e.response.text}", + ) from parse_errValueError를 catch하면 json.JSONDecodeError가 ValueError의 서브클래스이므로 모든 JSON 파싱 오류를 포함합니다. 예외 체인(
from e)을 사용하면 디버깅 시 원본 예외를 추적할 수 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/main.py (2)
982-989:AnalyzeChatService타입 힌트는 TYPE_CHECKING 블록으로 안전하게 정의하는 것을 권장합니다.문자열 타입 힌트
"AnalyzeChatService"때문에 Ruff F821(Undefined name)이 발생할 수 있어, 아래처럼TYPE_CHECKING을 활용해 정적 분석에서만 import 하도록 하면 깔끔하게 정리됩니다.-from typing import Optional +from typing import Optional, TYPE_CHECKING ... +if TYPE_CHECKING: + from .services.analyze_chat_service import AnalyzeChatService ... -async def analyze_chat( +async def analyze_chat( ... - analyze_chat_service: "AnalyzeChatService" = Depends(get_analyze_chat_service_dep) + analyze_chat_service: AnalyzeChatService = Depends(get_analyze_chat_service_dep) ):런타임 순환 참조 없이 정적 분석/IDE 지원을 개선하는 수준의 변경이라 부담 없이 적용 가능해 보입니다.
990-1003: 예외 처리:AppException재래이즈는 제거하거나raise만 쓰고, 일반 예외는from exc로 체이닝하는 것을 권장합니다.현재 블록은:
try: ... except AppException as exc: raise exc except Exception: raise InternalServerException("Internal server error while analyzing chat")
except AppException as exc: raise exc는 아무런 부가 작업 없이 동일 예외를 다시 던지는 것이어서, 그냥 생략해도 동일한 동작입니다.- 전체 예외를
InternalServerException으로 래핑하는 건 클라이언트에 내부 메시지를 숨기려는 의도로 보이는데, 디버깅을 위해서는 원래 예외를 체이닝하는 편이 좋습니다.제안:
- try: - return await analyze_chat_service.analyze_and_send(...) - except AppException as exc: - raise exc - except Exception: - raise InternalServerException("Internal server error while analyzing chat") + try: + return await analyze_chat_service.analyze_and_send(...) + except AppException: + # 서비스 레벨에서 이미 의미 있는 AppException으로 변환되었으므로 그대로 전파 + raise + except Exception as exc: + # 상세 원인은 로그/트레이스백 체이닝으로 확인 + raise InternalServerException( + "Internal server error while analyzing chat" + ) from exc이렇게 하면 Ruff TRY201/B904 경고도 자연스럽게 해소되고, 스택트레이스도 유지됩니다.
app/services/analyze_chat_service.py (5)
31-60: 업로드 파일명에 원본file.filename을 쓸 때는 최소한os.path.basename으로 sanitize 하는 것이 좋습니다.현재:
if file.filename: name, ext = os.path.splitext(file.filename) filename = f"{name}_{current_time}_{unique_id}{ext}"
- 클라이언트가
../../foo.wav같은 값을 보내면name에 슬래시 등이 그대로 남아 S3 key 가독성이 떨어지거나, 의도치 않은 pseudo-디렉터리 구조가 생길 수 있습니다.- 실제 파일 시스템은 아니더라도, prefix 기반 관리/S3 콘솔 탐색 시 혼란을 줄 수 있습니다.
예시 수정:
- if file.filename: - name, ext = os.path.splitext(file.filename) + if file.filename: + safe_name = os.path.basename(file.filename) + name, ext = os.path.splitext(safe_name) filename = f"{name}_{current_time}_{unique_id}{ext}"UUID + 타임스탬프 전략은 매우 좋으니, basename 한 번만 추가해 주면 더 안전해집니다.
100-120: S3 key에session_id/user_id를 그대로 사용하는 부분도 최소한의 정규화/필터링을 권장합니다.s3_key = f"{session_id}/{user_id}/{filename}" if session_id and user_id else f"chat/{filename}"
- S3는 파일 시스템이 아니어서 “path traversal” 취약점과 직접 연결되지는 않지만,
session_id,user_id에 공백, 슬래시, 제어문자 등을 그대로 허용하면 prefix 기반 정리·검색이 어려워질 수 있습니다.- 운영/모니터링 관점에서 S3 key 규칙이 깨지면 나중에 버킷 정리나 분석 작업이 까다로워집니다.
예를 들어, 알파벳/숫자/일부 구분자만 허용하도록 간단히 정규화하는 helper를 두고 쓰는 방식도 고려해 보세요.
def _normalize_key_component(value: str) -> str: # 예시: 공백 → `_`, 슬래시 제거 등 ... session = _normalize_key_component(session_id) user = _normalize_key_component(user_id) s3_key = f"{session}/{user}/{filename}" if session and user else f"chat/{filename}"보안 이슈라기보다는 운영/유지보수성 측면에서의 권장사항입니다.
122-147: STT용FileWrapper는 다른 메소드와 중복되므로 공통 helper로 빼면 좋습니다.
_transcribe_audio안에서 정의한FileWrapper는 아래_analyze_emotion의FileWrapper와 거의 동일한 구조라, 클래스/팩토리 함수를 모듈 상단으로 빼두면 중복 제거와 타입 추론에 모두 도움이 됩니다.예시:
class UploadFileLike: def __init__(self, content: bytes, filename: str, content_type: str): self.file = BytesIO(content) self.filename = filename self.content_type = content_type그리고:
- class FileWrapper: - ... - wrapped_file = FileWrapper(file_content, filename, content_type) + wrapped_file = UploadFileLike(file_content, filename, content_type)중복 정의를 줄이는 정도의 리팩터링이라 기능 영향 없이 적용 가능합니다.
216-227:user_id파라미터가 내부적으로 username으로 해석되므로 네이밍/검증을 명확히 해두면 좋습니다.def _get_user_name(self, user_id: str) -> str: ... user = auth_service.get_user_by_username(user_id)
- 외부 API/프론트 기준으로는 “user_id”가 실제 PK(정수)인지 username(문자열 코드)인지 헷갈릴 수 있습니다.
- 현재 구현은 username을 받는 형태이므로, 최소한 docstring이나 엔드포인트 설명에 “username string을 기대한다”는 점을 명시해 두는 것이 혼동을 줄여 줍니다.
- 이미
ValidationException/NotFoundException으로 에러가 잘 매핑되어 있는 점은 좋습니다.추가로, 나중에 실제 numeric user_id를 쓰는 API가 들어올 여지가 있다면, 이 메서드 시그니처를
username으로 바꾸는 것도 고려해 볼 수 있습니다.
228-267: 외부 chatbot API 예외는from exc체이닝으로 감싸면 디버깅과 Ruff B904 모두에 유리합니다.현재:
except httpx.HTTPStatusError as exc: error_message = self._extract_external_error(exc) raise ExternalAPIException(status_code=exc.response.status_code, message=error_message) except httpx.RequestError as exc: raise ExternalAPIException(status_code=500, message=f"External API request failed: {str(exc)}")
- 클라이언트에는
ExternalAPIException만 노출되지만, 내부적으로는 원래 httpx 예외를 체이닝하는 편이 트레이스백/로그 분석에 도움이 됩니다.- Ruff B904 경고도 자연스럽게 해결됩니다.
제안:
- except httpx.HTTPStatusError as exc: - error_message = self._extract_external_error(exc) - raise ExternalAPIException(status_code=exc.response.status_code, message=error_message) - except httpx.RequestError as exc: - raise ExternalAPIException(status_code=500, message=f"External API request failed: {str(exc)}") + except httpx.HTTPStatusError as exc: + error_message = self._extract_external_error(exc) + raise ExternalAPIException( + status_code=exc.response.status_code, + message=error_message, + ) from exc + except httpx.RequestError as exc: + raise ExternalAPIException( + status_code=500, + message=f"External API request failed: {exc}", + ) from exc30초 타임아웃 설정과
accept/Content-Type헤더 지정도 적절해 보이며, 에러 메시지 정책만 이렇게 조금 다듬으면 충분히 견고한 boundary 레이어가 됩니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/exceptions.py(1 hunks)app/main.py(3 hunks)app/services/analyze_chat_service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/main.py (3)
app/database.py (1)
get_db(38-44)app/services/analyze_chat_service.py (2)
get_analyze_chat_service(280-282)analyze_and_send(31-98)app/exceptions.py (2)
AppException(6-10)InternalServerException(37-40)
🪛 Ruff (0.14.5)
app/services/analyze_chat_service.py
111-111: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
176-176: Avoid specifying long messages outside the exception class
(TRY003)
219-219: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Avoid specifying long messages outside the exception class
(TRY003)
264-264: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
266-266: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
266-266: Use explicit conversion flag
Replace with conversion flag
(RUF010)
276-276: Do not catch blind exception: Exception
(BLE001)
app/main.py
976-976: 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)
987-987: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
988-988: Undefined name AnalyzeChatService
(F821)
988-988: 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)
1001-1001: Use raise without specifying exception name
Remove exception name
(TRY201)
1002-1002: Do not catch blind exception: Exception
(BLE001)
1003-1003: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1003-1003: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
app/exceptions.py (1)
43-52: 새 예외 타입 설계가 기존 패턴과 잘 맞습니다.
NotFoundException과ExternalAPIException이AppException패턴(고정 status_code + message)을 그대로 따르고 있고, 새/analyze/chat플로우에서 404와 외부 API 오류를 구분해서 처리하기에 적절해 보입니다. 추가 수정 없이 사용해도 될 것 같습니다.app/main.py (3)
263-263:analyze_routerprefix 설정이 의도한 경로(/analyze/chat)와 잘 맞습니다.
APIRouter(prefix="/analyze")+@analyze_router.post("/chat")조합으로 최종 경로가/analyze/chat이 되어, PR 설명과도 일치합니다. 기존 다른 router들과도 일관된 구조라 유지보수 측면에서도 괜찮아 보입니다.
976-979: DI 헬퍼는 패턴이 일관적이고, Ruff B008는 FastAPI에서는 예외적으로 무시해도 됩니다.
get_analyze_chat_service_dep가 다른 DI 함수들과 동일하게db: Session = Depends(get_db)를 사용하는 FastAPI 표준 패턴이라, Ruff의 B008 경고(“argument defaults에서 함수 호출 금지”)는 이 코드베이스 전체에서 일관되게 무시하는 편이 자연스러워 보입니다. 별도 수정 없이 유지하고, 필요하다면 Ruff 설정에서 FastAPI용으로 B008를 예외 처리하는 것도 고려해볼 만합니다.
1013-1013: 새analyze_router등록 위치가 자연스럽습니다.기존
users,care,admin,nlp,test,questions,composite_router뒤에analyze_router를 포함해 라우팅 구성이 명확하고 일관성 있습니다. 다른 라우터와의 prefix 충돌도 없어 보입니다.app/services/analyze_chat_service.py (3)
148-214: 감정 분석·VA fusion 로직은 설계 의도와 잘 맞고, fear→anxiety 매핑도 일관적입니다.
- 음성 감정(
analyze_voice_emotion) + 텍스트 감정(analyze_text_sentiment)을 받아fuse_VA로 arousal/valence를 [-1,1]→[0,1]로 정규화하는 흐름이 명확합니다.per_emotion_bps를 0–1로 스케일한 후 소수 둘째 자리까지 반올림해 내려주는 것도 외부 chatbot 쪽에서 쓰기 좋은 형태입니다.top_emotion == "fear"를anxiety로 매핑하고, details에서도fear를anxiety키로 노출하는 부분이 도메인 용어에 맞게 잘 정리되어 있습니다.현재 구현으로도 충분히 안정적으로 보이며, 추후 필요 시에만 세부 스케일/라벨링 정책을 조정하면 될 것 같습니다.
268-277: 외부 API 에러 메시지 파싱은 방어적으로 잘 짜여 있어, BLE001 경고는 실무적으로 무시 가능해 보입니다.
_extract_external_error에서:try: json_body = exc.response.json() ... except Exception: return f"External API error: {exc.response.text}"
- 여기서는 JSON 파싱/형식 관련 어떤 예외든 “적당한 문자열로 fallback” 하는 것이 목적이므로, 넓은
except Exception이 오히려 합리적입니다.- 이 레이어에서 예외를 다시 던지지 않고, 항상 문자열만 반환하기 때문에 상위 흐름에 악영향도 없습니다.
현재 구현을 유지하되, 필요하다면 Ruff에서 이 함수에 한해
# noqa: BLE001정도로 주석을 붙이는 정도만 고려하시면 될 것 같습니다.
280-282: 서비스 팩토리 함수(get_analyze_chat_service)가 DI 패턴과 잘 맞습니다.DB 세션을 받아
AnalyzeChatService를 생성하는 단일 책임 함수로,main.py의get_analyze_chat_service_dep와도 자연스럽게 연결됩니다. 나중에 서비스 생성 로직이 복잡해져도 이 함수 한 곳만 수정하면 되기 때문에 확장성 측면에서도 좋습니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/main.py (1)
976-980: DI 헬퍼 정의는 간결하고 재사용 가능하게 잘 구성되어 있습니다.요청 단위로
get_analyze_chat_service를 주입하는 패턴이 기존 서비스들과 일관되고, 순환 의존성 방지에도 도움이 됩니다.
성능/가독성 측면에서 큰 이슈는 없지만, 순환 의존성이 없다면.services.analyze_chat_service임포트를 파일 상단으로 올리는 것도 선택지입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/main.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
app/main.py
1000-1000: Expected a statement
(invalid-syntax)
1000-1001: Expected an expression
(invalid-syntax)
1001-1001: Unexpected indentation
(invalid-syntax)
1002-1002: Expected except or finally after try block
(invalid-syntax)
1002-1002: Expected a statement
(invalid-syntax)
1002-1003: Expected an expression
(invalid-syntax)
1003-1003: Unexpected indentation
(invalid-syntax)
🔇 Additional comments (2)
app/main.py (2)
263-263: 새analyze_router정의는 의도대로 보입니다.
prefix="/analyze"와tags=["analyze"]설정으로/analyze/...하위 엔드포인트를 명확히 분리하신 점 좋습니다. 아래의/chat라우트와도 잘 매칭됩니다.
1015-1015:analyze_router등록 위치와 방식이 일관적입니다.기존
users_router,care_router등과 동일한 방식으로app.include_router(analyze_router)를 추가하셔서 라우팅 구성이 한눈에 들어옵니다. 라우터 순서에 따른 충돌 가능성도 없어 보입니다.
🔎 Description
📋 개요
음성 파일을 받아 STT(음성-텍스트 변환), 감정 분석을 수행하고 외부 chatbot API로 결과를 전송하는 엔드포인트를 구현했습니다.
해당 API는 감정 일기와는 독립적으로 동작하고, 오직 S3 파일 업로드, 감정 분석 수행, SQS에 메세지 전송에만 집중합니다.
즉, 감정 일기 테이블에는 기록이 추가되는 로직이 아니라는 뜻입니다.
✨ 주요 기능
1.
/analyze/chat엔드포인트 구현POST /analyze/chatsession_id(필수): 세션 IDuser_id(필수): 사용자 ID (username)question(필수): 질문 내용file(필수): 음성 파일 (UploadFile)2. 처리 프로세스
{session_id}/{user_id}/{filename}구조로 저장🔧 기술적 개선사항
1. 비즈니스 로직 분리
AnalyzeChatService클래스로 비즈니스 로직 분리Depends사용)2. 비동기 처리 최적화
asyncio.to_thread()로 별도 스레드에서 실행3. S3 파일명 중복 방지
{원본이름}_{타임스탬프}_{UUID8자리}.{확장자}4. 환경 변수 설정
CHATBOT_API_URL: Chatbot API URL 설정 (기본값 제공)📁 변경된 파일
신규 파일
app/services/analyze_chat_service.py: 음성 분석 및 chatbot API 전송 서비스수정된 파일
app/main.py:/analyze/chat엔드포인트 추가requirements.txt:httpx>=0.25.0추가🔐 환경 변수 추가사항
> endpoint 공유하시면 수정 부탁드립니다.
📝 API 요청 예시
📤 응답 형식
외부 chatbot API의 응답을 그대로 반환합니다:
{ "success": true, "message": "Simulated Mind Diary event sent to SQS", "message_id": "c2e1d57c-517e-4981-993e-c4af366f5e02", "payload_preview": { "source": "mind-diary", "event": "analysis_completed", "user_id": "test_user", "user_name": "홍길동", "question": "오늘 가장 기억에 남는 순간은 언제였나요?", "content": "전사된 텍스트...", "recorded_at": "2025-11-24T18:30:00", "emotion": { "arousal": 0.6, "confidence": 0.65, "valence": 0.2, "top_emotion": "anxiety", "details": { "angry": 0, "anxiety": 0.65, "happy": 0.15, "neutral": 0.1, "sad": 0.05, "surprise": 0.05 } } } }실제 테스트 :
✅ 체크리스트
🧪 테스트
테스트용 사용자 생성 스크립트:
📌 참고사항
Summary by CodeRabbit
새 기능
개선사항
✏️ Tip: You can customize this high-level summary in your review settings.