Conversation
9 tasks
📝 WalkthroughWalkthroughNew member-only training plan endpoints were added: create/save, list by month, toggle a day's completion, and delete. A new Changes
Sequence DiagramsequenceDiagram
actor Client
participant API as FastAPI Endpoints
participant Auth as Auth Service
participant DB as Database
Note over Client,DB: Save Plan Flow
Client->>API: POST /plans (SavePlanRequest)
API->>Auth: get_current_user
Auth-->>API: user
API->>DB: INSERT SavedPlan (completed_days = [])
DB-->>API: saved record
API-->>Client: SuccessResponse[SavedPlanResponse]
Note over Client,DB: Complete Day Flow
Client->>API: PATCH /plans/{id}/complete (day_number, completed)
API->>Auth: get_current_user
Auth-->>API: user
API->>DB: SELECT SavedPlan WHERE id & user_id
DB-->>API: plan
API->>API: add/remove day_number in completed_days
API->>DB: UPDATE SavedPlan
DB-->>API: updated record
API-->>Client: SuccessResponse[SavedPlanResponse]
Note over Client,DB: List Plans by Month Flow
Client->>API: GET /plans?year=&month=
API->>Auth: get_current_user
Auth-->>API: user
API->>API: compute month start/end
API->>DB: SELECT plans WHERE user_id AND start_date <= month_end AND (start_date + total_days -1) >= month_start
DB-->>API: matching plans
API-->>Client: SuccessResponse[list[SavedPlanResponse]]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/db/models.py (1)
25-37: Enforce core plan invariants at the DB layer.
total_daysandplan_typeare currently only constrained by request schema logic. Invalid rows can still be inserted via non-API paths and break overlap/completion logic.Suggested hardening
-from sqlalchemy import Date, DateTime, ForeignKey, Integer, JSON, String +from sqlalchemy import CheckConstraint, Date, DateTime, ForeignKey, Integer, JSON, String class SavedPlan(Base): __tablename__ = "saved_plans" + __table_args__ = ( + CheckConstraint("total_days > 0", name="ck_saved_plans_total_days_positive"), + CheckConstraint("plan_type IN ('weekly', 'skill')", name="ck_saved_plans_plan_type"), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/models.py` around lines 25 - 37, SavedPlan currently relies on app-layer validation; add DB-level constraints on the SavedPlan model to prevent invalid rows by: add CheckConstraint(s) to the SavedPlan class enforcing plan_type IN ('weekly','skill') and total_days >= 1, and if using Postgres JSONB add a CheckConstraint ensuring json_array_length(completed_days) <= total_days (or equivalent DB-specific JSON length check) so completed_days cannot exceed totalDays; update the SavedPlan class (referencing SavedPlan, plan_type, total_days, completed_days) to include these CheckConstraint entries in its table args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/v1/endpoints/plans.py`:
- Around line 85-90: Prevent storing out-of-range day numbers by validating
req.day_number against the plan bounds before mutating completed; specifically,
check that req.day_number is an integer >= 1 and <= plan.total_days (reference
plan.total_days and req.day_number) and only then append to completed (the list
built from plan.completed_days) or remove it; return or raise a validation error
if the day is out of range so completed is never modified with invalid values.
- Around line 38-47: The endpoint currently accepts raw integers for year and
month and can raise ValueError when constructing date(); update the Query
parameter declarations for year and month to enforce bounds (e.g., year ge=1
le=9999 and month ge=1 le=12) so FastAPI returns validation errors instead of
500s; modify the parameters (year: int = Query(...), month: int = Query(...)) in
this function signature to include the ge/le constraints while keeping
current_user: User = Depends(get_current_user) and db: Session = Depends(get_db)
unchanged.
- Around line 78-95: The query that reads and updates SavedPlan (the plan
variable from db.query(SavedPlan).filter(...).first()) must acquire a row-level
lock to avoid lost updates; change the read to use with_for_update() on the
query (e.g., db.query(SavedPlan).filter(...).with_for_update().first()) inside
the same transaction so the read-modify-write on plan.completed_days is
serialized, then proceed to modify plan.completed_days, db.commit(), and
db.refresh(plan) as before.
In `@src/models/plan_schema.py`:
- Around line 11-14: The Plan model's fields title and total_days need
validation: update the Pydantic model in plan_schema.py to use Field constraints
on title and total_days (e.g., replace the plain type annotations for title and
total_days with pydantic.Field(..., min_length=1, max_length=200) for title and
Field(..., ge=1) for total_days) so empty titles and non-positive durations are
rejected; ensure you import Field from pydantic and keep the existing names
(title, total_days) so other code referencing the Plan schema continues to work.
- Around line 30-32: CompleteDayRequest currently allows zero and negative
day_number values; update the Pydantic model (CompleteDayRequest) to enforce
day_number >= 1 by changing its annotation to a constrained int (e.g., use
Field(..., ge=1) or conint(ge=1)) or add a `@validator`("day_number") that raises
a ValueError for values < 1 so inputs are rejected before being persisted to
completed_days.
---
Nitpick comments:
In `@src/db/models.py`:
- Around line 25-37: SavedPlan currently relies on app-layer validation; add
DB-level constraints on the SavedPlan model to prevent invalid rows by: add
CheckConstraint(s) to the SavedPlan class enforcing plan_type IN
('weekly','skill') and total_days >= 1, and if using Postgres JSONB add a
CheckConstraint ensuring json_array_length(completed_days) <= total_days (or
equivalent DB-specific JSON length check) so completed_days cannot exceed
totalDays; update the SavedPlan class (referencing SavedPlan, plan_type,
total_days, completed_days) to include these CheckConstraint entries in its
table args.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3376096b-2e8a-42ca-8344-40d732ef4ab2
📒 Files selected for processing (4)
src/api/v1/endpoints/plans.pysrc/api/v1/router.pysrc/db/models.pysrc/models/plan_schema.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/api/v1/endpoints/plans.py (1)
78-97:⚠️ Potential issue | 🟠 MajorSerialize
completed_daysupdates to prevent lost updates.
Line 78 to Line 97 still has a read-modify-write race; simultaneous PATCH requests can drop one update. This was flagged earlier and remains unresolved.Suggested fix
- plan = db.query(SavedPlan).filter( - SavedPlan.id == plan_id, - SavedPlan.user_id == current_user.id, - ).first() + plan = ( + db.query(SavedPlan) + .filter( + SavedPlan.id == plan_id, + SavedPlan.user_id == current_user.id, + ) + .with_for_update() + .first() + )#!/bin/bash # Verify DB backend and locking support context (read-only). rg -n "DATABASE_URL|create_engine|sqlite|postgres|mysql" src -C2 rg -n "complete_plan_day|with_for_update|completed_days" src/api/v1/endpoints/plans.py -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/plans.py` around lines 78 - 97, The read-modify-write on SavedPlan.completed_days can race; serialize updates by acquiring a row lock before modifying: change the lookup to use a SELECT ... FOR UPDATE (e.g., call .with_for_update() on the db.query(SavedPlan).filter(...)) inside a transactional context so the retrieved plan row is locked, then modify plan.completed_days, commit and refresh; alternatively implement optimistic locking by adding a version/timestamp column on SavedPlan and performing a conditional update (check-and-set) to detect concurrent modifications—choose the FOR UPDATE approach if your DB backend supports row-level locking, and ensure the code paths using plan = db.query(...).filter(...).first() are replaced with the locked/transactional variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/api/v1/endpoints/plans.py`:
- Around line 78-97: The read-modify-write on SavedPlan.completed_days can race;
serialize updates by acquiring a row lock before modifying: change the lookup to
use a SELECT ... FOR UPDATE (e.g., call .with_for_update() on the
db.query(SavedPlan).filter(...)) inside a transactional context so the retrieved
plan row is locked, then modify plan.completed_days, commit and refresh;
alternatively implement optimistic locking by adding a version/timestamp column
on SavedPlan and performing a conditional update (check-and-set) to detect
concurrent modifications—choose the FOR UPDATE approach if your DB backend
supports row-level locking, and ensure the code paths using plan =
db.query(...).filter(...).first() are replaced with the locked/transactional
variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c7e5a19-34e6-4ee3-a5f0-b6282f3d9b28
📒 Files selected for processing (2)
src/api/v1/endpoints/plans.pysrc/models/plan_schema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/models/plan_schema.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
어떤 변경사항인가요?
작업 상세 내용
src/db/models.py—SavedPlan모델 추가 (user_id FK, plan_type, title, data JSON, start_date, total_days, completed_days JSON)src/models/plan_schema.py신규 생성 —SavePlanRequest,SavedPlanResponse,CompleteDayRequest스키마src/api/v1/endpoints/plans.py신규 생성 — CRUD 엔드포인트 (모두get_current_user보호)POST /api/v1/plans/— 플랜 저장GET /api/v1/plans/?year=&month=— 월별 플랜 목록PATCH /api/v1/plans/{id}/complete— day 완료/취소 토글DELETE /api/v1/plans/{id}— 플랜 삭제src/api/v1/router.py— plans 라우터 등록체크리스트
관련 이슈
리뷰 포인트
completed_days를 별도 테이블 없이 JSON 배열로 저장 — 단순성을 위해 선택start_date <= last_day로 조회 후 Python에서 end_date 범위 검사 (다월에 걸친 플랜 처리)user_id일치 여부 확인 (타 유저 플랜 접근 차단)Summary by CodeRabbit