diff --git a/backend/app/services/card_service.py b/backend/app/services/card_service.py index 3e7b19d..6e953f9 100644 --- a/backend/app/services/card_service.py +++ b/backend/app/services/card_service.py @@ -58,6 +58,36 @@ class SetInfo(BaseModel): card_count: int +class CardLoadError(Exception): + """Raised when a card definition file fails to load. + + Attributes: + file_path: Path to the file that failed to load. + reason: Description of why loading failed. + """ + + def __init__(self, file_path: str, reason: str) -> None: + self.file_path = file_path + self.reason = reason + super().__init__(f"Failed to load card from {file_path}: {reason}") + + +class CardServiceLoadError(Exception): + """Raised when CardService fails to load one or more card files. + + Collects all individual load failures for reporting. + + Attributes: + failures: List of (file_path, error_message) tuples. + """ + + def __init__(self, failures: list[tuple[str, str]]) -> None: + self.failures = failures + file_list = "\n ".join(f"{path}: {err}" for path, err in failures[:5]) + more = f"\n ... and {len(failures) - 5} more" if len(failures) > 5 else "" + super().__init__(f"Failed to load {len(failures)} card definition(s):\n {file_list}{more}") + + class CardService: """Load and serve card definitions from JSON files. @@ -109,7 +139,7 @@ class CardService: Raises: FileNotFoundError: If the definitions directory doesn't exist. - ValueError: If any card definition is invalid. + CardServiceLoadError: If any card definition files fail to load. """ if self._loaded: logger.warning("CardService.load_all() called but cards already loaded") @@ -132,14 +162,21 @@ class CardService: ) ) + # Collect all failures across card types + all_failures: list[tuple[str, str]] = [] + # Load Pokemon cards - await self._load_card_type("pokemon", CardType.POKEMON) + all_failures.extend(await self._load_card_type("pokemon", CardType.POKEMON)) # Load Trainer cards - await self._load_card_type("trainer", CardType.TRAINER) + all_failures.extend(await self._load_card_type("trainer", CardType.TRAINER)) # Load Energy cards - await self._load_energy_cards() + all_failures.extend(await self._load_energy_cards()) + + # Fail fast if any card files couldn't be loaded + if all_failures: + raise CardServiceLoadError(all_failures) self._loaded = True logger.info( @@ -149,17 +186,21 @@ class CardService: f"{len(self._by_type[CardType.ENERGY])} Energy)" ) - async def _load_card_type(self, subdir: str, card_type: CardType) -> None: + async def _load_card_type(self, subdir: str, card_type: CardType) -> list[tuple[str, str]]: """Load all cards of a specific type from a subdirectory. Args: subdir: Subdirectory name (e.g., "pokemon", "trainer"). card_type: The CardType these cards should be. + + Returns: + List of (file_path, error_message) tuples for any files that failed. """ + failures: list[tuple[str, str]] = [] type_dir = self._definitions_dir / subdir if not type_dir.exists(): logger.debug(f"Directory {type_dir} does not exist, skipping") - return + return failures for set_dir in type_dir.iterdir(): if not set_dir.is_dir(): @@ -170,45 +211,62 @@ class CardService: self._by_set[set_code] = [] for card_file in set_dir.glob("*.json"): - card = await self._load_card_file(card_file) - if card: + try: + card = await self._load_card_file(card_file) self._index_card(card) + except CardLoadError as e: + failures.append((e.file_path, e.reason)) - async def _load_energy_cards(self) -> None: - """Load energy cards from the energy subdirectory.""" + return failures + + async def _load_energy_cards(self) -> list[tuple[str, str]]: + """Load energy cards from the energy subdirectory. + + Returns: + List of (file_path, error_message) tuples for any files that failed. + """ + failures: list[tuple[str, str]] = [] energy_dir = self._definitions_dir / "energy" if not energy_dir.exists(): logger.debug(f"Directory {energy_dir} does not exist, skipping") - return + return failures # Energy can be in subdirectories (e.g., energy/basic/) or directly in energy/ for item in energy_dir.iterdir(): if item.is_file() and item.suffix == ".json": - card = await self._load_card_file(item) - if card: + try: + card = await self._load_card_file(item) self._index_card(card) + except CardLoadError as e: + failures.append((e.file_path, e.reason)) elif item.is_dir(): for card_file in item.glob("*.json"): - card = await self._load_card_file(card_file) - if card: + try: + card = await self._load_card_file(card_file) self._index_card(card) + except CardLoadError as e: + failures.append((e.file_path, e.reason)) - async def _load_card_file(self, file_path: Path) -> CardDefinition | None: + return failures + + async def _load_card_file(self, file_path: Path) -> CardDefinition: """Load and validate a single card definition file. Args: file_path: Path to the JSON file. Returns: - CardDefinition if valid, None if loading failed. + CardDefinition if valid. + + Raises: + CardLoadError: If loading or validation fails. """ try: with open(file_path) as f: card_data = json.load(f) return CardDefinition.model_validate(card_data) except Exception as e: - logger.error(f"Failed to load card from {file_path}: {e}") - return None + raise CardLoadError(str(file_path), str(e)) from e def _index_card(self, card: CardDefinition) -> None: """Add a card to all indexes. diff --git a/backend/app/services/game_service.py b/backend/app/services/game_service.py index 77e8b5e..67d71b9 100644 --- a/backend/app/services/game_service.py +++ b/backend/app/services/game_service.py @@ -771,19 +771,23 @@ class GameService: # Energy card IDs follow pattern: energy-basic-{type} energy_id = f"energy-basic-{energy_type}" energy_def = self._card_service.get_card(energy_id) - if energy_def: - energy1_cards.extend([energy_def] * qty) - else: - logger.warning(f"Energy card not found: {energy_id}") + if energy_def is None: + raise GameCreationError( + f"Energy card definition not found: {energy_id}. " + "Check that card data is loaded correctly." + ) + energy1_cards.extend([energy_def] * qty) energy2_cards = [] for energy_type, qty in deck2_entry.energy_cards.items(): energy_id = f"energy-basic-{energy_type}" energy_def = self._card_service.get_card(energy_id) - if energy_def: - energy2_cards.extend([energy_def] * qty) - else: - logger.warning(f"Energy card not found: {energy_id}") + if energy_def is None: + raise GameCreationError( + f"Energy card definition not found: {energy_id}. " + "Check that card data is loaded correctly." + ) + energy2_cards.extend([energy_def] * qty) # Convert CardDefinitions to CardInstances with unique IDs deck1_instances = self._cards_to_instances(deck1_cards, p1_str, "main") diff --git a/backend/app/services/game_state_manager.py b/backend/app/services/game_state_manager.py index f588aab..20bb77d 100644 --- a/backend/app/services/game_state_manager.py +++ b/backend/app/services/game_state_manager.py @@ -43,6 +43,7 @@ Example: """ import logging +from dataclasses import dataclass, field from datetime import UTC, datetime from uuid import UUID @@ -57,6 +58,22 @@ from app.db.session import get_session logger = logging.getLogger(__name__) + +@dataclass(frozen=True) +class RecoveryResult: + """Result of game recovery operation. + + Attributes: + recovered: Number of games successfully recovered. + failed: List of (game_id, error_message) tuples for failed recoveries. + total: Total number of games attempted. + """ + + recovered: int + failed: tuple[tuple[str, str], ...] = field(default_factory=tuple) + total: int = 0 + + # Redis key patterns GAME_KEY_PREFIX = "game:" @@ -376,7 +393,7 @@ class GameStateManager: async def recover_active_games( self, session: AsyncSession | None = None, - ) -> int: + ) -> RecoveryResult: """Recover all active games from Postgres to Redis. Called on server startup to restore game state. Loads all games @@ -386,31 +403,49 @@ class GameStateManager: session: Optional existing session. Returns: - Number of games recovered. + RecoveryResult with counts and any failures. Example: @app.on_event("startup") async def startup(): - count = await game_state_manager.recover_active_games() - logger.info(f"Recovered {count} active games") + result = await game_state_manager.recover_active_games() + logger.info(f"Recovered {result.recovered}/{result.total} games") + if result.failed: + logger.error(f"Failed to recover {len(result.failed)} games") """ - async def _recover(db: AsyncSession) -> int: - count = 0 + async def _recover(db: AsyncSession) -> RecoveryResult: + recovered = 0 + failures: list[tuple[str, str]] = [] result = await db.execute(select(ActiveGame)) active_games = result.scalars().all() + total = len(active_games) for active_game in active_games: + game_id = str(active_game.id) try: state = GameState.model_validate(active_game.game_state) await self.save_to_cache(state) - count += 1 - logger.debug(f"Recovered game: {active_game.id}") + recovered += 1 + logger.debug(f"Recovered game: {game_id}") except Exception as e: - logger.error(f"Failed to recover game {active_game.id}: {e}") + error_msg = str(e) + failures.append((game_id, error_msg)) + logger.error(f"Failed to recover game {game_id}: {error_msg}") - logger.info(f"Recovered {count} active games from database") - return count + logger.info(f"Recovered {recovered}/{total} active games from database") + if failures: + logger.warning( + f"Failed to recover {len(failures)} games: " + f"{[gid for gid, _ in failures[:5]]}" + + (f" and {len(failures) - 5} more" if len(failures) > 5 else "") + ) + + return RecoveryResult( + recovered=recovered, + failed=tuple(failures), + total=total, + ) if session: return await _recover(session) diff --git a/backend/tests/services/test_game_state_manager.py b/backend/tests/services/test_game_state_manager.py index c42cb41..19ac6c9 100644 --- a/backend/tests/services/test_game_state_manager.py +++ b/backend/tests/services/test_game_state_manager.py @@ -455,8 +455,10 @@ class TestHighLevelOperations: helper.store.clear() # Recover (pass session) - count = await manager.recover_active_games(session=db_session) - assert count == 2 + result = await manager.recover_active_games(session=db_session) + assert result.recovered == 2 + assert result.total == 2 + assert len(result.failed) == 0 # Both should be in cache now assert await manager.cache_exists(game1.game_id) @@ -672,10 +674,15 @@ class TestAdditionalCoverage: helper.store.clear() # Recovery should handle the error and still recover the valid game - count = await manager.recover_active_games(session=db_session) + result = await manager.recover_active_games(session=db_session) # Only the valid game should be recovered (invalid one fails validation) - assert count == 1 + assert result.recovered == 1 + assert result.total == 2 + assert len(result.failed) == 1 + # Verify the failed game ID is reported + failed_ids = [gid for gid, _ in result.failed] + assert invalid_game_id in failed_ids assert await manager.cache_exists(valid_game.game_id) assert not await manager.cache_exists(invalid_game_id) diff --git a/backend/tests/unit/services/test_game_service.py b/backend/tests/unit/services/test_game_service.py index e68ee6e..85196c9 100644 --- a/backend/tests/unit/services/test_game_service.py +++ b/backend/tests/unit/services/test_game_service.py @@ -1200,6 +1200,43 @@ class TestCreateGame: assert "No basic Pokemon" in str(exc_info.value) + @pytest.mark.asyncio + async def test_create_game_missing_energy_card_raises_error( + self, + mock_state_manager: AsyncMock, + mock_deck_service: AsyncMock, + ) -> None: + """Test that missing energy card definition raises GameCreationError. + + If an energy card definition cannot be found (e.g., 'energy-basic-fire' + is not in the card registry), game creation should fail immediately + with a clear error rather than silently creating a broken game. + """ + from app.core.config import RulesConfig + from app.services.game_service import GameService + + # CardService that returns None for energy cards (simulating missing data) + mock_card_service = MagicMock() + mock_card_service.get_card = MagicMock(return_value=None) + + service = GameService( + state_manager=mock_state_manager, + card_service=mock_card_service, + ) + + with pytest.raises(GameCreationError) as exc_info: + await service.create_game( + player1_id=uuid4(), + player2_id=uuid4(), + deck1_id=uuid4(), + deck2_id=uuid4(), + rules_config=RulesConfig(), + deck_service=mock_deck_service, + ) + + assert "Energy card definition not found" in str(exc_info.value) + assert "energy-basic-" in str(exc_info.value) + class TestDefaultEngineFactory: """Tests for the _default_engine_factory method.