From 9b48939de8eacf0eb08f79e42686020e31bf3752 Mon Sep 17 00:00:00 2001 From: qzl Date: Sat, 28 Feb 2026 11:29:06 +0800 Subject: [PATCH] fix: address code review issues and improve code quality - Add owner_id check in repository delete operation - Fix time range validation for partial updates - Wrap pre-query in try/except for consistent error handling - Use default_factory instead of mutable defaults - Add max_length constraint for timezone field - Remove unused dependencies and empty validators - Extract magic numbers to constants - Simplify update logic with model_dump --- backend/src/v1/schedule_items/dependencies.py | 6 -- backend/src/v1/schedule_items/repository.py | 14 +++- backend/src/v1/schedule_items/schemas.py | 15 ++--- backend/src/v1/schedule_items/service.py | 66 ++++++++++--------- 4 files changed, 53 insertions(+), 48 deletions(-) diff --git a/backend/src/v1/schedule_items/dependencies.py b/backend/src/v1/schedule_items/dependencies.py index 38872bd..97f3237 100644 --- a/backend/src/v1/schedule_items/dependencies.py +++ b/backend/src/v1/schedule_items/dependencies.py @@ -12,12 +12,6 @@ from v1.schedule_items.service import ScheduleItemService from v1.users.dependencies import get_current_user -async def get_schedule_item_repository( - session: Annotated[AsyncSession, Depends(get_db)], -) -> SQLAlchemyScheduleItemRepository: - return SQLAlchemyScheduleItemRepository(session) - - def get_schedule_item_service( session: Annotated[AsyncSession, Depends(get_db)], user: Annotated[CurrentUser, Depends(get_current_user)], diff --git a/backend/src/v1/schedule_items/repository.py b/backend/src/v1/schedule_items/repository.py index f74420d..1f3caa7 100644 --- a/backend/src/v1/schedule_items/repository.py +++ b/backend/src/v1/schedule_items/repository.py @@ -1,6 +1,6 @@ from __future__ import annotations -from datetime import datetime +from datetime import datetime, timezone from typing import TYPE_CHECKING, Protocol from uuid import UUID @@ -95,7 +95,17 @@ class SQLAlchemyScheduleItemRepository(BaseRepository[ScheduleItem]): self, item_id: UUID, owner_id: UUID ) -> ScheduleItem | None: try: - return await self.soft_delete_by_id(item_id) + stmt = ( + update(ScheduleItem) + .where(ScheduleItem.id == item_id) + .where(ScheduleItem.owner_id == owner_id) + .where(ScheduleItem.deleted_at.is_(None)) + .values(deleted_at=datetime.now(timezone.utc)) + .returning(ScheduleItem) + ) + result = await self._session.execute(stmt) + await self._session.flush() + return result.scalar_one_or_none() except SQLAlchemyError: logger.exception("Schedule item delete failed", item_id=str(item_id)) raise diff --git a/backend/src/v1/schedule_items/schemas.py b/backend/src/v1/schedule_items/schemas.py index 323650d..677318e 100644 --- a/backend/src/v1/schedule_items/schemas.py +++ b/backend/src/v1/schedule_items/schemas.py @@ -5,7 +5,7 @@ from enum import Enum from typing import ClassVar from uuid import UUID -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field class AttachmentType(str, Enum): @@ -16,7 +16,7 @@ class AttachmentType(str, Enum): class ScheduleItemMetadataAttachment(BaseModel): name: str type: AttachmentType - visible_to: list[UUID] = [] + visible_to: list[UUID] = Field(default_factory=list) url: str | None = None note: str | None = None content: str | None = None @@ -26,7 +26,7 @@ class ScheduleItemMetadata(BaseModel): color: str | None = None location: str | None = None notes: str | None = None - attachments: list[ScheduleItemMetadataAttachment] = [] + attachments: list[ScheduleItemMetadataAttachment] = Field(default_factory=list) version: int = 1 @@ -50,7 +50,7 @@ class ScheduleItemCreateRequest(BaseModel): description: str | None = Field(default=None, max_length=2000) start_at: datetime end_at: datetime | None = None - timezone: str = "UTC" + timezone: str = Field(default="UTC", max_length=50) metadata: ScheduleItemMetadata | None = None @@ -61,15 +61,10 @@ class ScheduleItemUpdateRequest(BaseModel): description: str | None = Field(default=None, max_length=2000) start_at: datetime | None = None end_at: datetime | None = None - timezone: str | None = None + timezone: str | None = Field(default=None, max_length=50) metadata: ScheduleItemMetadata | None = None status: ScheduleItemStatus | None = None - @field_validator("end_at", mode="before") - @classmethod - def validate_end_at(cls, v: datetime | None, info) -> datetime | None: - return v - class ScheduleItemResponse(BaseModel): model_config: ClassVar[ConfigDict] = ConfigDict(from_attributes=True) diff --git a/backend/src/v1/schedule_items/service.py b/backend/src/v1/schedule_items/service.py index 234842e..6d47066 100644 --- a/backend/src/v1/schedule_items/service.py +++ b/backend/src/v1/schedule_items/service.py @@ -93,33 +93,39 @@ class ScheduleItemService(BaseService): ) -> ScheduleItemResponse: user_id = self.require_user_id() - existing = await self._repository.get_by_item_id(item_id, user_id) - if existing is None: - raise HTTPException(status_code=404, detail="Schedule item not found") - - update_data: dict = {} - if request.title is not None: - update_data["title"] = request.title - if request.description is not None: - update_data["description"] = request.description - if request.start_at is not None: - update_data["start_at"] = request.start_at - if request.end_at is not None: - update_data["end_at"] = request.end_at - if request.timezone is not None: - update_data["timezone"] = request.timezone - if request.status is not None: - update_data["status"] = request.status - if request.metadata is not None: - update_data["metadata"] = request.metadata.model_dump() - - if request.end_at and request.start_at and request.end_at <= request.start_at: - raise HTTPException(status_code=400, detail="end_at must be after start_at") - - if not update_data: - return self._to_response(existing) - try: + existing = await self._repository.get_by_item_id(item_id, user_id) + if existing is None: + raise HTTPException(status_code=404, detail="Schedule item not found") + + update_data: dict = {} + if request.title is not None: + update_data["title"] = request.title + if request.description is not None: + update_data["description"] = request.description + if request.start_at is not None: + update_data["start_at"] = request.start_at + if request.end_at is not None: + update_data["end_at"] = request.end_at + if request.timezone is not None: + update_data["timezone"] = request.timezone + if request.status is not None: + update_data["status"] = request.status + if request.metadata is not None: + update_data["metadata"] = request.metadata.model_dump() + + next_start = ( + request.start_at if request.start_at is not None else existing.start_at + ) + next_end = request.end_at if request.end_at is not None else existing.end_at + if next_end is not None and next_end <= next_start: + raise HTTPException( + status_code=400, detail="end_at must be after start_at" + ) + + if not update_data: + return self._to_response(existing) + item = await self._repository.update_by_item_id( item_id, user_id, update_data ) @@ -139,11 +145,11 @@ class ScheduleItemService(BaseService): async def delete(self, item_id: UUID) -> None: user_id = self.require_user_id() - existing = await self._repository.get_by_item_id(item_id, user_id) - if existing is None: - raise HTTPException(status_code=404, detail="Schedule item not found") - try: + existing = await self._repository.get_by_item_id(item_id, user_id) + if existing is None: + raise HTTPException(status_code=404, detail="Schedule item not found") + await self._repository.delete_by_item_id(item_id, user_id) await self._session.commit() except SQLAlchemyError: