Fix: history response #118
Conversation
mjy926
commented
Nov 28, 2025
mjy926
commented
- history에 unit을 추가했습니다.
- key가 중복될 경우 기존의 내용을 수정합니다.
Walkthrough히스토리 엔티티에 선택적 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)wacruit/src/admin/views.py (1)
⏰ 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)
🔇 Additional comments (3)
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: 3
🧹 Nitpick comments (3)
wacruit/src/apps/history/repositories.py (3)
5-5: 사용되지 않는 import를 제거하세요.
sqlalchemy.delete가 import되었지만 코드에서 사용되지 않습니다.delete_history메서드는session.delete()인스턴스 메서드를 사용하며, SQLAlchemy의delete()구문을 사용하지 않습니다.-from sqlalchemy import delete from sqlalchemy import select
27-44: 쿼리 패턴을 일관되게 유지하세요.
update_history메서드(Line 30)는 레거시query()API를 사용하지만,get_history메서드(Line 24)는 SQLAlchemy 2.0 스타일의select()를 사용합니다. 일관성을 위해 동일한 패턴을 사용하는 것을 권장합니다.Line 30을 SQLAlchemy 2.0 스타일로 변경:
- existing_key = ( - self.session.query(History).filter(History.history_key.in_(keys)).all() - ) + query = select(History).where(History.history_key.in_(keys)) + existing_key = list(self.session.execute(query).scalars().all())
54-59: 쿼리 패턴을 일관되게 유지하세요.
delete_history메서드도 레거시query()API를 사용합니다. 코드베이스 전체에서 일관성을 위해 SQLAlchemy 2.0 스타일의select()를 사용하는 것을 권장합니다.- def delete_history(self, history_key: str) -> bool: - history = self.session.query(History).filter_by(history_key=history_key).first() - if not history: - return False - self.session.delete(history) - return True + def delete_history(self, history_key: str) -> bool: + query = select(History).where(History.history_key == history_key) + history = self.session.execute(query).scalar_one_or_none() + if not history: + return False + self.session.delete(history) + return True
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wacruit/src/apps/history/exceptions.py(1 hunks)wacruit/src/apps/history/models.py(1 hunks)wacruit/src/apps/history/repositories.py(3 hunks)wacruit/src/apps/history/schemas.py(1 hunks)wacruit/src/apps/history/services.py(2 hunks)wacruit/src/apps/history/views.py(2 hunks)wacruit/src/database/migrations/versions/2025_11_27_2152-60e3aaf301ce_add_history_unit.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
wacruit/src/apps/history/exceptions.py (1)
wacruit/src/apps/common/exceptions.py (1)
WacruitException(6-24)
wacruit/src/apps/history/repositories.py (3)
wacruit/src/apps/history/services.py (2)
update_history(21-34)delete_history(36-39)wacruit/src/apps/history/views.py (2)
update_history(17-27)delete_history(38-43)wacruit/src/apps/history/models.py (1)
History(8-14)
wacruit/src/apps/history/schemas.py (1)
wacruit/src/apps/common/schemas.py (1)
OrmModel(19-25)
wacruit/src/apps/history/views.py (4)
wacruit/src/apps/history/schemas.py (1)
DeleteHistoryRequest(18-19)wacruit/src/apps/history/repositories.py (1)
delete_history(54-59)wacruit/src/apps/history/services.py (2)
delete_history(36-39)HistoryService(12-39)wacruit/src/tests/announcement/conftest.py (1)
admin_user(34-47)
wacruit/src/apps/history/services.py (5)
wacruit/src/apps/history/exceptions.py (1)
HistoryNotFoundException(4-6)wacruit/src/apps/history/models.py (1)
History(8-14)wacruit/src/apps/history/repositories.py (3)
update_history(27-44)get_history(23-25)delete_history(54-59)wacruit/src/apps/history/schemas.py (1)
DeleteHistoryRequest(18-19)wacruit/src/apps/history/views.py (3)
update_history(17-27)get_history(31-34)delete_history(38-43)
🪛 Ruff (0.14.6)
wacruit/src/apps/history/views.py
39-39: Unused function argument: admin_user
(ARG001)
🔇 Additional comments (7)
wacruit/src/apps/history/models.py (1)
14-14: LGTM!history_unit 필드 추가가 적절하며 마이그레이션 스키마와 일치합니다.
wacruit/src/apps/history/exceptions.py (1)
4-6: LGTM!
HistoryNotFoundException예외 구현이 적절하며 기존 패턴을 따릅니다.wacruit/src/apps/history/views.py (1)
37-43: LGTM!DELETE 엔드포인트 구현이 적절합니다.
admin_user매개변수는 의도적으로 사용되지 않으며, FastAPI의 의존성 주입을 통한 권한 검증에 사용됩니다.참고: Ruff의 ARG001 경고는 false positive입니다.
AdminUser의존성은 엔드포인트에 대한 관리자 권한을 보장하기 위해 필요합니다.wacruit/src/apps/history/schemas.py (2)
8-11: LGTM!
HistoryItemRequest스키마가 적절하게 정의되었습니다.history_unit을 선택 사항으로 설정한 것은 유연성을 제공합니다.
18-19: LGTM!
DeleteHistoryRequest스키마가 간결하고 적절합니다.wacruit/src/apps/history/services.py (1)
36-39: LGTM!
delete_history메서드가 적절하게 구현되었으며, repository의 boolean 반환 값을 기반으로HistoryNotFoundException을 발생시킵니다.wacruit/src/apps/history/repositories.py (1)
44-44: Based on my verification, I can now provide the rewritten review comment.
Return value is not used by callers; no practical issue exists.
The repository's
update_history()method technically returns objects that may lack auto-generated IDs for new records. However, the service layer (line 32 of services.py) discards this return value entirely and instead callsget_history()to fetch fresh data from the database (line 34). The API layer receives fresh, correct data with all IDs populated. The return statement in the repository is redundant but causes no actual problem in practice.Likely an incorrect or invalid review comment.
wacruit/src/apps/history/schemas.py
Outdated
| id: int | ||
| history_key: str | ||
| history_value: str | ||
| history_unit: str |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Find and examine the schemas.py file
find . -name "schemas.py" -path "*/history/*" -type fRepository: wafflestudio/wacruit-server
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Read the schemas.py file to see the current schema definitions
cat -n ./wacruit/src/apps/history/schemas.pyRepository: wafflestudio/wacruit-server
Length of output: 733
🏁 Script executed:
#!/bin/bash
# Find the History ORM model definition
find . -name "models.py" -path "*/history/*" -type fRepository: wafflestudio/wacruit-server
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Read the History ORM model
cat -n ./wacruit/src/apps/history/models.pyRepository: wafflestudio/wacruit-server
Length of output: 541
🏁 Script executed:
#!/bin/bash
# Find database migrations related to history
find . -name "*migration*" -o -name "*alembic*" | head -20Repository: wafflestudio/wacruit-server
Length of output: 118
🏁 Script executed:
#!/bin/bash
# List migration files
ls -la ./wacruit/src/database/migrations/versions/ | head -30Repository: wafflestudio/wacruit-server
Length of output: 2944
🏁 Script executed:
#!/bin/bash
# Search for history table in migrations
rg -n "history" ./wacruit/src/database/migrations/versions/ -iRepository: wafflestudio/wacruit-server
Length of output: 2203
🏁 Script executed:
#!/bin/bash
# Read the add_history_unit migration
cat -n ./wacruit/src/database/migrations/versions/2025_11_27_2152-60e3aaf301ce_add_history_unit.pyRepository: wafflestudio/wacruit-server
Length of output: 1025
🏁 Script executed:
#!/bin/bash
# Search for HistoryResponse.from_orm usage
rg -n "HistoryResponse|from_orm" . -A3 -B3 --type=py | grep -A5 -B5 "from_orm"Repository: wafflestudio/wacruit-server
Length of output: 33306
🏁 Script executed:
#!/bin/bash
# Search for HistoryItemRequest and UpdateHistoryRequest usage
rg -n "HistoryItemRequest|UpdateHistoryRequest" ./wacruit/src/apps/history -A5 -B5Repository: wafflestudio/wacruit-server
Length of output: 4500
🏁 Script executed:
#!/bin/bash
# Read the full history services.py
cat -n ./wacruit/src/apps/history/services.pyRepository: wafflestudio/wacruit-server
Length of output: 1711
🏁 Script executed:
#!/bin/bash
# Search for history tests
find . -name "*test*history*" -o -path "*/tests/*" -name "*history*" | head -20Repository: wafflestudio/wacruit-server
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check if there are any existing history records to understand migration impact
# Also verify the migration one more time for any server_default we might have missed
cat ./wacruit/src/database/migrations/versions/2025_11_27_2152-60e3aaf301ce_add_history_unit.pyRepository: wafflestudio/wacruit-server
Length of output: 815
Based on my thorough verification of the codebase, I can now provide the rewritten review comment.
Schema mismatch: history_unit field allows None in input but requires non-null value in database and response schema.
The issue is confirmed:
- Input schema (
HistoryItemRequest, line 11):history_unit: str | None = None— acceptsNone - Service layer (
services.py, line 28): Passesitem.history_unitdirectly toHistoryconstructor, which can beNone - ORM model (
models.py, line 14):history_unit: Mapped[str50]— NOT NULL in database - Response schema (
HistoryResponse, line 26):history_unit: str— requires non-null - Migration (2025_11_27_2152): Adds column with
nullable=Falseand no server_default — will fail on existing data or fail when insertingNone - Serialization (
views.py, lines 25, 34): CallsHistoryResponse.from_orm()which will fail validation ifhistory_unitisNone
Options to fix:
Option 1: Make history_unit optional in response schema
class HistoryResponse(OrmModel):
id: int
history_key: str
history_value: str
- history_unit: str
+ history_unit: str | None = NoneOption 2: Make history_unit required in input schema and add migration default
Modify HistoryItemRequest to require history_unit, and update the migration to include server_default="" or similar.
📝 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.
| history_unit: str | |
| history_unit: str | None = None |
| op.add_column( | ||
| "history", sa.Column("history_unit", sa.String(length=50), nullable=False) | ||
| ) |
There was a problem hiding this comment.
NOT NULL 제약 조건이 있는 컬럼을 기본값 없이 추가하면 마이그레이션이 실패합니다.
history_unit 컬럼이 nullable=False로 추가되지만 기본값이 제공되지 않습니다. history 테이블에 기존 데이터가 있는 경우 마이그레이션이 실패합니다.
다음 중 하나의 방법으로 수정하세요:
옵션 1: 기본값 제공 (권장)
op.add_column(
- "history", sa.Column("history_unit", sa.String(length=50), nullable=False)
+ "history", sa.Column("history_unit", sa.String(length=50), nullable=False, server_default="")
)옵션 2: 2단계 마이그레이션
- 먼저 nullable=True로 컬럼 추가
- 기존 데이터 업데이트
- nullable=False로 변경
op.add_column(
- "history", sa.Column("history_unit", sa.String(length=50), nullable=False)
+ "history", sa.Column("history_unit", sa.String(length=50), nullable=True)
)
+ # Update existing rows
+ op.execute("UPDATE history SET history_unit = '' WHERE history_unit IS NULL")
+ # Make it non-nullable
+ op.alter_column("history", "history_unit", nullable=False)📝 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.
| op.add_column( | |
| "history", sa.Column("history_unit", sa.String(length=50), nullable=False) | |
| ) | |
| op.add_column( | |
| "history", sa.Column("history_unit", sa.String(length=50), nullable=False, server_default="") | |
| ) |
| op.add_column( | |
| "history", sa.Column("history_unit", sa.String(length=50), nullable=False) | |
| ) | |
| op.add_column( | |
| "history", sa.Column("history_unit", sa.String(length=50), nullable=True) | |
| ) | |
| # Update existing rows | |
| op.execute("UPDATE history SET history_unit = '' WHERE history_unit IS NULL") | |
| # Make it non-nullable | |
| op.alter_column("history", "history_unit", nullable=False) |
🤖 Prompt for AI Agents
In
wacruit/src/database/migrations/versions/2025_11_27_2152-60e3aaf301ce_add_history_unit.py
around lines 21-23, the migration adds history_unit with nullable=False but no
default which will fail if history already has rows; fix by either (A) adding a
server_default (e.g., sa.text("'your_default'") or sa.literal_column) when
creating the column and then optionally removing the server_default in a
subsequent alter, or (B) perform a two-step migration: first add the column as
nullable=True, run an update to populate existing rows with the desired value,
then alter the column to nullable=False; implement one of these approaches in
this migration or split into two migrations accordingly.
