[FEAT] 학습 카드 추천#19
Conversation
|
Warning Rate limit exceeded@jud1thDev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 18 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)
📝 WalkthroughWalkthrough앱 버전을 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45분
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 |
![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
🧹 Nitpick comments (10)
tools/seed_learning_cards.py (1)
1-1: 이 파일의 의도와 구현 상태를 명확히 해주세요.새로운 파일이 추가되었지만 콘텐츠가 비어 있습니다. PR 목표에 따르면 학습 카드 추천 기능이 추가되고 데이터베이스 스키마 변경(report_learning_cards 테이블)이 포함되어 있는데, 이 파일의 목적과 현재 상태가 불명확합니다.
다음 중 하나를 확인해주세요:
- 이 파일이 학습 카드 초기 데이터 로드(seed)를 위한 구현이 필요한 경우, 구현을 완료해주세요.
- 이 파일이 의도하지 않은 추가인 경우, 제거해주세요.
- 이 파일이 향후 추가 예정인 경우, 적절한 설명과 함께 TODO 주석을 추가해주세요.
database/schema.sql (1)
81-89: 정렬 위치 중복 방지 제약 조건 누락현재 스키마는 동일한
trade_cycle_id에 대해 중복된position값을 허용합니다. 이는 정렬 순서가 모호해질 수 있습니다.position컬럼에 대한 UNIQUE 제약 조건을 추가하는 것을 권장합니다.다음 제약 조건을 추가하는 것을 고려하세요:
CREATE UNIQUE INDEX idx_report_learning_cards_position_unique ON report_learning_cards(trade_cycle_id, position);또는 테이블 정의에 다음을 추가:
UNIQUE (trade_cycle_id, position)app/models/response.py (1)
6-16:LearningCardResponse에ConfigDict설정 추가 권장
GenerateReportResponse와 달리LearningCardResponse에는ConfigDict가 없습니다.alias를 사용하는 필드들이 있으므로populate_by_name=True설정을 추가하여 일관성을 유지하고 유연한 데이터 바인딩을 지원하는 것이 좋습니다.다음과 같이 수정하세요:
class LearningCardResponse(BaseModel): + model_config = ConfigDict( + populate_by_name=True, + from_attributes=True + ) + id: int = Field(..., description="학습 카드 ID") title: str = Field(..., description="학습 카드 제목")app/routers/report.py (1)
312-319: 학습 카드 추천의 OpenAI 토큰 사용량 미추적
LearningCardRecommender.recommend_for_report는 내부적으로 OpenAI API를 호출하지만(app/services/report/learning_card_recommender.py line 143-146 참조), 이 과정에서 사용된 토큰은 추적되지 않습니다.리포트 생성 시 OpenAI 토큰 사용량이 중요한 메트릭이라면, 학습 카드 추천에서 사용된 토큰도 함께 추적하고 총 사용량에 합산하는 것을 고려하세요.
LearningCardRecommender가 토큰 사용량을 반환하도록 수정할까요? 또는 별도의 메트릭으로 로깅하는 방안을 제안해드릴 수 있습니다.app/services/report/learning_card_recommender.py (6)
31-33: 사용되지 않는preferred_levels파라미터
preferred_levels파라미터(line 24)가 정의되고 기본값이 설정되지만(lines 31-33), 실제로는 학습 카드 필터링이나 추천 로직에 사용되지 않습니다.이 파라미터를 실제로 활용하여 난이도별 필터링을 구현하거나, 사용하지 않는다면 제거하는 것이 좋습니다.
난이도 기반 필터링 로직을 추가하거나 파라미터를 제거하는 방안을 제안해드릴 수 있습니다.
36-49:is_report_recommendable필드를 활용한 필터링 누락쿼리에서
is_report_recommendable컬럼을 조회하지만(line 46), WHERE 절에서 이를 활용한 필터링이 없습니다. 리포트 추천에 적합하지 않은 카드들도 모두 조회되어 GPT에 전달됩니다.성능과 추천 품질 향상을 위해
WHERE is_report_recommendable = TRUE조건을 추가하는 것을 권장합니다.query = """ SELECT id, title, description, contents, category, level, keywords, image_urls, is_report_recommendable FROM learning_cards + WHERE is_report_recommendable = TRUE """
95-98: GPT가 반환한 카드 순서 보존 문제Line 96에서
set(gpt_ids)를 사용하여 순서가 손실되고, line 98의[:limit]는 리스트의 원래 순서를 기준으로 잘라냅니다. 이는 GPT가 의도한 추천 순서를 무시할 수 있습니다.GPT가 반환한 카드 ID의 순서를 유지하려면 다음과 같이 수정하세요.
if not gpt_ids: # GPT가 유효한 id를 반환하지 않으면 추천하지 않음 return [] id_set = set(gpt_ids) - recommended = [c for c in cards if c["id"] in id_set] + # GPT가 반환한 순서대로 카드를 정렬 + id_to_card = {c["id"]: c for c in cards} + recommended = [id_to_card[id] for id in gpt_ids if id in id_to_card] # GPT가 너무 많이 주면 상위 limit만 사용 recommended = recommended[:limit]
108-114: 예외 처리 개선 권장일반적인
Exception을 catch하고 있으며, 정적 분석 도구가 제안하는 것처럼logging.exception을 사용하는 것이 스택 트레이스 추적에 더 유용합니다.except Exception as e: - logger.error( + logger.exception( "learning_cards_recommend_failed", - error=str(e), - error_type=type(e).__name__, ) return []동일한 패턴이 lines 165-171에도 적용됩니다.
157-161: ID 변환 실패 시 로깅 누락Line 159에서
int(v)변환이 실패할 때 예외를 무시하고 계속 진행합니다. 이는 디버깅을 어렵게 만들 수 있습니다. 변환 실패 시 경고 로그를 남기는 것을 권장합니다.result: List[int] = [] for v in ids: try: result.append(int(v)) except Exception: + logger.warning("invalid_card_id_in_gpt_response", value=v) continue
204-204: GPT 프롬프트의 최소 개수 요구사항 불일치Line 204에서 "3개 이상, 최대 {limit}개"를 요청하지만,
limit의 기본값은 5이고(line 23)app/routers/report.py에서는 3으로 호출합니다(line 316).프롬프트에서 최소 개수를 강제하면 GPT가 적합하지 않은 카드를 억지로 추천할 수 있습니다. "최대 {limit}개"로 수정하여 GPT가 유연하게 선택하도록 하는 것을 권장합니다.
- "- recommendedCardIds에는 위에 제공된 카드 id 중에서 3개 이상, 최대 {limit}개까지 포함하세요." + "- recommendedCardIds에는 위에 제공된 카드 id 중에서 최대 {limit}개까지 포함하세요."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/config.py(1 hunks)app/models/response.py(2 hunks)app/routers/report.py(7 hunks)app/services/report/learning_card_recommender.py(1 hunks)database/schema.sql(1 hunks)tools/seed_learning_cards.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/routers/report.py (3)
app/models/response.py (2)
GenerateReportResponse(18-41)LearningCardResponse(6-15)app/services/report/learning_card_recommender.py (2)
LearningCardRecommender(14-255)recommend_for_report(20-114)app/services/database/db_service.py (3)
fetch(46-48)DatabaseService(8-58)execute(41-43)
app/services/report/learning_card_recommender.py (2)
app/services/database/db_service.py (2)
DatabaseService(8-58)fetch(46-48)app/services/external/openai_service.py (2)
OpenAIService(10-57)generate_completion(19-57)
🪛 Ruff (0.14.4)
app/services/report/learning_card_recommender.py
29-29: Local variable normalized_text is assigned to but never used
Remove assignment to unused variable normalized_text
(F841)
106-106: Consider moving this statement to an else block
(TRY300)
108-108: Do not catch blind exception: Exception
(BLE001)
109-113: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
160-161: try-except-continue detected, consider logging the exception
(S112)
160-160: Do not catch blind exception: Exception
(BLE001)
163-163: Consider moving this statement to an else block
(TRY300)
165-165: Do not catch blind exception: Exception
(BLE001)
166-170: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
app/config.py (1)
44-44: 버전 업데이트 확인 완료새로운 학습 카드 추천 기능 추가에 따른 버전 업데이트가 적절합니다.
| # 리포트별 추천 학습 카드 조회 | ||
| learning_cards_query = """ | ||
| SELECT | ||
| lc.id, | ||
| lc.title, | ||
| lc.description, | ||
| lc.contents, | ||
| lc.category, | ||
| lc.level, | ||
| lc.keywords, | ||
| lc.image_urls | ||
| FROM report_learning_cards rlc | ||
| JOIN learning_cards lc ON lc.id = rlc.learning_card_id | ||
| WHERE rlc.trade_cycle_id = $1 | ||
| ORDER BY rlc.position ASC | ||
| """ | ||
| learning_rows = await db_service.fetch(learning_cards_query, trade_cycle_id) | ||
| learning_cards = [ | ||
| LearningCardResponse( | ||
| id=r["id"], | ||
| title=r["title"], | ||
| description=r["description"], | ||
| contents=r["contents"], | ||
| category=r["category"], | ||
| level=r["level"], | ||
| keywords=r["keywords"], | ||
| imageUrls=r["image_urls"], | ||
| isReportRecommendable=True, | ||
| ) | ||
| for r in learning_rows | ||
| ] |
There was a problem hiding this comment.
is_report_recommendable 필드를 데이터베이스에서 가져와야 함
Line 159에서 isReportRecommendable=True를 하드코딩하고 있지만, 이 값은 learning_cards 테이블의 is_report_recommendable 컬럼에서 가져와야 합니다. 현재 쿼리(line 142)에서는 이 컬럼을 조회하지 않고 있습니다.
다음과 같이 수정하세요:
learning_cards_query = """
SELECT
lc.id,
lc.title,
lc.description,
lc.contents,
lc.category,
lc.level,
lc.keywords,
- lc.image_urls
+ lc.image_urls,
+ lc.is_report_recommendable
FROM report_learning_cards rlc
JOIN learning_cards lc ON lc.id = rlc.learning_card_id
WHERE rlc.trade_cycle_id = $1
ORDER BY rlc.position ASC
"""
learning_rows = await db_service.fetch(learning_cards_query, trade_cycle_id)
learning_cards = [
LearningCardResponse(
id=r["id"],
title=r["title"],
description=r["description"],
contents=r["contents"],
category=r["category"],
level=r["level"],
keywords=r["keywords"],
imageUrls=r["image_urls"],
- isReportRecommendable=True,
+ isReportRecommendable=r["is_report_recommendable"],
)
for r in learning_rows
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 리포트별 추천 학습 카드 조회 | |
| learning_cards_query = """ | |
| SELECT | |
| lc.id, | |
| lc.title, | |
| lc.description, | |
| lc.contents, | |
| lc.category, | |
| lc.level, | |
| lc.keywords, | |
| lc.image_urls | |
| FROM report_learning_cards rlc | |
| JOIN learning_cards lc ON lc.id = rlc.learning_card_id | |
| WHERE rlc.trade_cycle_id = $1 | |
| ORDER BY rlc.position ASC | |
| """ | |
| learning_rows = await db_service.fetch(learning_cards_query, trade_cycle_id) | |
| learning_cards = [ | |
| LearningCardResponse( | |
| id=r["id"], | |
| title=r["title"], | |
| description=r["description"], | |
| contents=r["contents"], | |
| category=r["category"], | |
| level=r["level"], | |
| keywords=r["keywords"], | |
| imageUrls=r["image_urls"], | |
| isReportRecommendable=True, | |
| ) | |
| for r in learning_rows | |
| ] | |
| # 리포트별 추천 학습 카드 조회 | |
| learning_cards_query = """ | |
| SELECT | |
| lc.id, | |
| lc.title, | |
| lc.description, | |
| lc.contents, | |
| lc.category, | |
| lc.level, | |
| lc.keywords, | |
| lc.image_urls, | |
| lc.is_report_recommendable | |
| FROM report_learning_cards rlc | |
| JOIN learning_cards lc ON lc.id = rlc.learning_card_id | |
| WHERE rlc.trade_cycle_id = $1 | |
| ORDER BY rlc.position ASC | |
| """ | |
| learning_rows = await db_service.fetch(learning_cards_query, trade_cycle_id) | |
| learning_cards = [ | |
| LearningCardResponse( | |
| id=r["id"], | |
| title=r["title"], | |
| description=r["description"], | |
| contents=r["contents"], | |
| category=r["category"], | |
| level=r["level"], | |
| keywords=r["keywords"], | |
| imageUrls=r["image_urls"], | |
| isReportRecommendable=r["is_report_recommendable"], | |
| ) | |
| for r in learning_rows | |
| ] |
🤖 Prompt for AI Agents
In app/routers/report.py around lines 132 to 162 the code hardcodes
isReportRecommendable=True for each LearningCardResponse while the value should
come from the learning_cards.is_report_recommendable column; update the SQL
SELECT to include that column (e.g. add "lc.is_report_recommendable" to the
select list) and then set isReportRecommendable=r["is_report_recommendable"]
when constructing LearningCardResponse (ensure the dict key matches the column
alias returned by the query).
| async def _save_report_learning_cards( | ||
| db_service: DatabaseService, | ||
| trade_cycle_id: int, | ||
| learning_cards: list[LearningCardResponse], | ||
| ) -> None: | ||
| delete_query = "DELETE FROM report_learning_cards WHERE trade_cycle_id = $1" | ||
| await db_service.execute(delete_query, trade_cycle_id) | ||
|
|
||
| insert_query = """ | ||
| INSERT INTO report_learning_cards ( | ||
| trade_cycle_id, | ||
| learning_card_id, | ||
| position | ||
| ) VALUES ($1, $2, $3) | ||
| """ | ||
|
|
||
| position = 1 | ||
| for card in learning_cards: | ||
| await db_service.execute( | ||
| insert_query, | ||
| trade_cycle_id, | ||
| card.id, | ||
| position, | ||
| ) | ||
| position += 1 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
트랜잭션 경계 없는 DELETE + INSERT 패턴의 데이터 일관성 문제
_save_report_learning_cards 함수는 기존 레코드를 삭제(line 270-271)한 후 새로운 레코드를 삽입(lines 273-289)합니다. 트랜잭션 경계가 없어서 다음과 같은 문제가 발생할 수 있습니다:
- 삭제와 삽입 사이에 프로세스가 중단되면 데이터가 손실됩니다
- 동시 요청 시 race condition이 발생할 수 있습니다
DatabaseService가 트랜잭션을 지원한다면 명시적인 트랜잭션 블록으로 감싸거나, 단일 쿼리로 UPSERT 패턴을 사용하는 것을 권장합니다.
다음 스크립트로 DatabaseService의 트랜잭션 지원 여부를 확인하세요:
🏁 Script executed:
#!/bin/bash
# DatabaseService에 트랜잭션 메서드가 있는지 확인
rg -n "transaction|begin|commit|rollback" app/services/database/Length of output: 0
추가 검증이 필요합니다. DatabaseService의 실제 위치와 메서드를 확인하겠습니다.
"""
🏁 Script executed:
#!/bin/bash
# DatabaseService 파일 위치 찾기
fd -e py "database" app/services/ 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# DatabaseService 클래스 정의 및 메서드 찾기
rg -n "class DatabaseService" --type py -A 30 app/Length of output: 2177
🏁 Script executed:
#!/bin/bash
# 트랜잭션 관련 패턴 더 넓게 검색 (async with, contextmanager 등)
rg -n "async def.*transaction|async def.*begin|async def.*commit|def.*transaction|def.*begin|def.*commit" --type py app/services/Length of output: 0
🏁 Script executed:
#!/bin/bash
# _save_report_learning_cards 함수가 호출되는 곳과 트랜잭션 사용 패턴 확인
rg -n "_save_report_learning_cards" --type py -B 5 -A 5 app/Length of output: 1272
DatabaseService의 전체 메서드와 구현을 확인하겠습니다.
"""
</parameter>
</invoke>
---
🏁 Script executed:
```shell
#!/bin/bash
# db_service.py 전체 파일 내용 확인
cat app/services/database/db_service.py
Length of output: 1866
🏁 Script executed:
#!/bin/bash
# 다른 곳에서 트랜잭션 사용 패턴 확인 (transaction, begin, async with 등)
rg -n "async with.*transaction|\.transaction|BEGIN|begin" --type py app/ -C 3Length of output: 1326
🏁 Script executed:
#!/bin/bash
# report.py 전체 _save_report_learning_cards 함수 및 호출 컨텍스트 확인
cat app/routers/report.py | sed -n '260,360p'Length of output: 3337
트랜잭션 경계 없는 DELETE + INSERT 패턴의 데이터 일관성 문제 확인됨
검증 결과, 원본 리뷰 코멘트의 지적이 타당합니다. _save_report_learning_cards 함수(app/routers/report.py, 라인 265-290)는 트랜잭션 없이 DELETE(라인 270-271)와 INSERT(라인 273-289)를 분리하여 실행하고 있습니다.
-
발견사항: asyncpg는
conn.transaction()컨텍스트 매니저를 지원하며, 코드베이스의 다른 서비스들(app/services/analysis/liquid_stocks_service.py, app/services/analysis/correlation_service.py)에서 이미async with conn.transaction():패턴으로 사용 중입니다. -
권장 해결책: 함수 내에서
async with self.db_service.pool.acquire() as conn:후async with conn.transaction():로 DELETE와 INSERT를 감싸거나, DatabaseService에 transaction 헬퍼 메서드를 추가하여 사용하세요.
🤖 Prompt for AI Agents
In app/routers/report.py around lines 265-290, the function performs a DELETE
followed by multiple INSERTs without a transaction boundary, risking
inconsistent state; wrap the DELETE and subsequent INSERT loop in a single
transaction by acquiring a connection from db_service.pool (async with
db_service.pool.acquire() as conn) and using async with conn.transaction(): to
execute the delete and inserts on that conn, or alternatively add/use a
DatabaseService.transaction helper to run the DELETE and INSERTs within one
transactional context so they commit or roll back together.
| await _save_report_learning_cards( | ||
| db_service=db_service, | ||
| trade_cycle_id=request.trade_cycle_id, | ||
| learning_cards=learning_cards, | ||
| ) | ||
| logger.info( | ||
| "report_saved_to_db", | ||
| trade_cycle_id=request.trade_cycle_id, | ||
| learning_cards_count=len(learning_cards), | ||
| ) |
There was a problem hiding this comment.
학습 카드 저장 실패 시 에러 처리 불충분
Line 346-350에서 _save_report_learning_cards 호출이 실패해도 예외가 로깅만 되고 전파되지 않습니다. 이는 리포트는 저장되었지만 학습 카드는 저장되지 않은 불일치 상태를 초래할 수 있습니다.
리포트와 학습 카드를 함께 저장하는 것이 핵심 기능이라면, 두 작업을 하나의 트랜잭션으로 묶거나 학습 카드 저장 실패 시 명시적으로 에러를 전파하여 일관성을 보장해야 합니다.
고려할 수 있는 옵션:
- 두 저장 작업을 하나의 트랜잭션으로 묶기
- 학습 카드 저장 실패 시 예외를 전파하여 리포트 저장도 롤백
- 학습 카드가 선택적 기능이라면, 현재대로 유지하되 response에 학습 카드 저장 실패 여부를 명시
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
database/schema.sql (1)
111-120:position컬럼에UNIQUE제약 조건 추가를 검토하세요.동일한
trade_cycle_id에 대해 여러 학습 카드가 같은position값을 가질 수 있습니다. 정렬 및 순서 지정에position을 사용하는 경우, 같은 리포트 내에서 각 카드가 고유한 위치를 가져야 한다면 복합UNIQUE제약을 추가하는 것이 좋습니다.다음과 같이 제약을 추가하세요:
CREATE TABLE IF NOT EXISTS report_learning_cards ( trade_cycle_id INTEGER NOT NULL REFERENCES reports(trade_cycle_id) ON DELETE CASCADE, learning_card_id INTEGER NOT NULL REFERENCES learning_cards(id) ON DELETE CASCADE, position INTEGER NOT NULL, -- 정렬 위치 created_at TIMESTAMP NOT NULL DEFAULT NOW(), - PRIMARY KEY (trade_cycle_id, learning_card_id) + PRIMARY KEY (trade_cycle_id, learning_card_id), + UNIQUE (trade_cycle_id, position) );단, 같은 리포트에서 여러 카드가 같은 위치를 가질 수 있도록 의도했다면 이 제안은 무시해도 됩니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
database/schema.sql(1 hunks)
⏰ 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: test-and-build
🔇 Additional comments (1)
database/schema.sql (1)
111-120: 스키마 설계 승인: 적절한 조인 테이블 구조리포트와 학습 카드 간의 다대다 관계를 효과적으로 구현했습니다:
- 복합 기본 키로 중복 추천 방지 ✓
ON DELETE CASCADE로 데이터 일관성 보장 ✓(trade_cycle_id, position)인덱스로 조회 성능 최적화 ✓created_at타임스탬프로 감사 추적 지원 ✓외래 키 참조가 정확하고 (
reports.trade_cycle_id,learning_cards.id), 정렬 위치 추적도 적절합니다.
1 participant
🎯 목적
📝 주요 변경사항
🔗 관련 이슈/PR
🔄 버전 검토
🧪 테스트
📋 체크리스트
📝 커밋 메시지 규칙
featfixrefactorstyledocstestchoreperfciconfig예시:
feat: 뉴스 칼럼 추천 API 추가Summary by CodeRabbit
새로운 기능
기타