refactor: fix bugs

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)

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())

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

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()