From 9c940cedfd3fa8a6a5304012616cfc1ac650d08a Mon Sep 17 00:00:00 2001 From: itqop Date: Sun, 9 Nov 2025 02:05:40 +0300 Subject: [PATCH] refactor: fix bugs --- src/hubgw/api/v1/whitelist.py | 9 ++-- src/hubgw/repositories/whitelist_repo.py | 14 +----- src/hubgw/schemas/whitelist.py | 64 +++++++++++------------- src/hubgw/services/whitelist_service.py | 23 ++++++++- 4 files changed, 58 insertions(+), 52 deletions(-) diff --git a/src/hubgw/api/v1/whitelist.py b/src/hubgw/api/v1/whitelist.py index 39f3c15..95df0f1 100644 --- a/src/hubgw/api/v1/whitelist.py +++ b/src/hubgw/api/v1/whitelist.py @@ -9,7 +9,8 @@ from hubgw.api.deps import (get_luckperms_service, get_user_service, from hubgw.core.errors import AppError, create_http_exception from hubgw.schemas.whitelist import (WhitelistAddRequest, WhitelistCheckRequest, - WhitelistCheckResponse, WhitelistEntry, + WhitelistCheckResponse, + WhitelistCountResponse, WhitelistEntry, WhitelistListResponse, WhitelistRemoveRequest) from hubgw.services.luckperms_service import LuckPermsService from hubgw.services.users_service import UserService @@ -71,14 +72,14 @@ async def list_players( raise create_http_exception(e) -@router.get("/count") +@router.get("/count", response_model=WhitelistCountResponse) async def get_count( service: Annotated[WhitelistService, Depends(get_whitelist_service)], _: Annotated[str, Depends(verify_api_key)], ): """Get total count of whitelisted players.""" try: - count = await service.repo.count() - return {"total": count} + count = await service.get_count() + return WhitelistCountResponse(total=count) except AppError as e: raise create_http_exception(e) diff --git a/src/hubgw/repositories/whitelist_repo.py b/src/hubgw/repositories/whitelist_repo.py index 87c95ac..d164d1e 100644 --- a/src/hubgw/repositories/whitelist_repo.py +++ b/src/hubgw/repositories/whitelist_repo.py @@ -7,8 +7,7 @@ from sqlalchemy import and_, delete, func, select from sqlalchemy.ext.asyncio import AsyncSession from hubgw.models.whitelist import WhitelistEntry -from hubgw.schemas.whitelist import (WhitelistAddRequest, - WhitelistCheckRequest, WhitelistRemoveRequest) +from hubgw.schemas.whitelist import WhitelistAddRequest, WhitelistCheckRequest class WhitelistRepository: @@ -62,21 +61,12 @@ class WhitelistRepository: stmt = select(WhitelistEntry).where( and_( WhitelistEntry.player_name == request.player_name, - WhitelistEntry.is_active == True, + WhitelistEntry.is_active.is_(True), ) ) result = await self.session.execute(stmt) return result.scalar_one_or_none() - async def remove(self, request: WhitelistRemoveRequest) -> bool: - """Remove player from whitelist.""" - stmt = delete(WhitelistEntry).where( - WhitelistEntry.player_name == request.player_name - ) - result = await self.session.execute(stmt) - await self.session.commit() - return result.rowcount > 0 - async def list_all(self) -> List[WhitelistEntry]: """List all whitelisted players.""" stmt = select(WhitelistEntry).order_by(WhitelistEntry.added_at.desc()) diff --git a/src/hubgw/schemas/whitelist.py b/src/hubgw/schemas/whitelist.py index a1be54f..6caea95 100644 --- a/src/hubgw/schemas/whitelist.py +++ b/src/hubgw/schemas/whitelist.py @@ -5,7 +5,17 @@ from typing import Optional from uuid import UUID import re -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator + + +def validate_minecraft_username(v: str) -> str: + """Validate Minecraft username format (alphanumeric and underscore only).""" + if not re.match(r'^[a-zA-Z0-9_]{3,16}$', v): + raise ValueError( + 'Invalid Minecraft username: must be 3-16 characters, ' + 'only letters, numbers, and underscores allowed' + ) + return v class WhitelistAddRequest(BaseModel): @@ -18,16 +28,7 @@ class WhitelistAddRequest(BaseModel): is_active: bool = True reason: Optional[str] = Field(None, max_length=500) - @field_validator('player_name') - @classmethod - def validate_minecraft_username(cls, v: str) -> str: - """Validate Minecraft username format (alphanumeric and underscore only).""" - if not re.match(r'^[a-zA-Z0-9_]{3,16}$', v): - raise ValueError( - 'Invalid Minecraft username: must be 3-16 characters, ' - 'only letters, numbers, and underscores allowed' - ) - return v + _validate_username = field_validator('player_name')(validate_minecraft_username) @field_validator('added_by') @classmethod @@ -74,30 +75,28 @@ class WhitelistAddRequest(BaseModel): """Validate and sanitize reason field.""" if v is None: return v - + v = v.strip() - + if not v: return None - + return v + @model_validator(mode='after') + def validate_expires_after_added(self) -> 'WhitelistAddRequest': + """Validate expires_at is after added_at.""" + if self.expires_at and self.expires_at <= self.added_at: + raise ValueError('expires_at must be after added_at') + return self + class WhitelistRemoveRequest(BaseModel): """Whitelist remove request schema.""" player_name: str = Field(min_length=3, max_length=16) - @field_validator('player_name') - @classmethod - def validate_minecraft_username(cls, v: str) -> str: - """Validate Minecraft username format.""" - if not re.match(r'^[a-zA-Z0-9_]{3,16}$', v): - raise ValueError( - 'Invalid Minecraft username: must be 3-16 characters, ' - 'only letters, numbers, and underscores allowed' - ) - return v + _validate_username = field_validator('player_name')(validate_minecraft_username) class WhitelistCheckRequest(BaseModel): @@ -105,16 +104,7 @@ class WhitelistCheckRequest(BaseModel): player_name: str = Field(min_length=3, max_length=16) - @field_validator('player_name') - @classmethod - def validate_minecraft_username(cls, v: str) -> str: - """Validate Minecraft username format.""" - if not re.match(r'^[a-zA-Z0-9_]{3,16}$', v): - raise ValueError( - 'Invalid Minecraft username: must be 3-16 characters, ' - 'only letters, numbers, and underscores allowed' - ) - return v + _validate_username = field_validator('player_name')(validate_minecraft_username) class WhitelistCheckResponse(BaseModel): @@ -142,3 +132,9 @@ class WhitelistListResponse(BaseModel): entries: list[WhitelistEntry] total: int + + +class WhitelistCountResponse(BaseModel): + """Whitelist count response schema.""" + + total: int diff --git a/src/hubgw/services/whitelist_service.py b/src/hubgw/services/whitelist_service.py index 1f1990e..b0754e6 100644 --- a/src/hubgw/services/whitelist_service.py +++ b/src/hubgw/services/whitelist_service.py @@ -1,6 +1,8 @@ """Whitelist service.""" +from loguru import logger +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from hubgw.core.errors import AlreadyExistsError, NotFoundError @@ -36,6 +38,10 @@ class WhitelistService: ) except AlreadyExistsError: pass + except Exception as e: + logger.error( + f"Failed to create LuckPerms player '{request.player_name}': {type(e).__name__}: {e}" + ) existing = await self.repo.get_by_player_name(request.player_name) if existing: @@ -52,8 +58,17 @@ class WhitelistService: updated_entry = await self.repo.update(existing) return SchemaWhitelistEntry.model_validate(updated_entry) - created_entry = await self.repo.create(request) - return SchemaWhitelistEntry.model_validate(created_entry) + try: + created_entry = await self.repo.create(request) + return SchemaWhitelistEntry.model_validate(created_entry) + except IntegrityError: + await self.repo.session.rollback() + existing = await self.repo.get_by_player_name(request.player_name) + if existing and existing.is_active: + raise AlreadyExistsError( + f"Player '{request.player_name}' is already whitelisted" + ) + raise async def remove_player(self, request: WhitelistRemoveRequest) -> None: success = await self.repo.delete_by_player_name(request.player_name) @@ -78,3 +93,7 @@ class WhitelistService: entry_list = [SchemaWhitelistEntry.model_validate(entry) for entry in entries] return WhitelistListResponse(entries=entry_list, total=total) + + async def get_count(self) -> int: + """Get total count of whitelisted players.""" + return await self.repo.count()