From 7d397a2e22a082f40e13c876776d0bf519791ce2 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Wed, 28 Jan 2026 14:32:08 -0600 Subject: [PATCH] Fix medium priority issues from code review UNSET sentinel pattern: - Add UNSET sentinel in protocols.py for nullable field updates - Fix inability to clear deck description (UNSET=keep, None=clear) - Fix repository inability to clear validation_errors Starter deck improvements: - Remove unused has_starter_deck from CollectionService - Add deprecation notes to old starter deck methods Validation improvements: - Add energy type validation in deck_validator.py - Add energy type validation in deck schemas - Add VALID_ENERGY_TYPES constant Game loading fix: - Fix get_deck_for_game silently skipping invalid cards - Now raises ValueError with clear error message Tests: - Add TestEnergyTypeValidation test class - Add TestGetDeckForGame test class - Add tests for validate_energy_types utility function Co-Authored-By: Claude Opus 4.5 --- backend/app/api/decks.py | 10 +- backend/app/repositories/postgres/deck.py | 26 +++--- backend/app/repositories/protocols.py | 48 ++++++++-- backend/app/schemas/deck.py | 38 +++++++- backend/app/services/collection_service.py | 18 +--- backend/app/services/deck_service.py | 33 +++++-- backend/app/services/deck_validator.py | 30 ++++++ backend/tests/services/test_deck_service.py | 80 ++++++++++++++++ .../unit/services/test_deck_validator.py | 93 +++++++++++++++++++ 9 files changed, 331 insertions(+), 45 deletions(-) diff --git a/backend/app/api/decks.py b/backend/app/api/decks.py index 57d7e2e..e398745 100644 --- a/backend/app/api/decks.py +++ b/backend/app/api/decks.py @@ -20,6 +20,7 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, status from app.api.deps import CardServiceDep, CollectionServiceDep, CurrentUser, DeckServiceDep +from app.repositories.protocols import UNSET from app.schemas.deck import ( DeckCreateRequest, DeckListResponse, @@ -183,6 +184,9 @@ async def update_deck( Only provided fields are updated. If cards or energy_cards change, the deck is re-validated with the provided deck_config. + To clear the description, explicitly send `"description": null` in the request. + Omitting the field keeps the existing description. + Args: deck_id: The deck's UUID. deck_in: Update request with optional fields. @@ -193,6 +197,10 @@ async def update_deck( Raises: 404: If deck not found or not owned by user. """ + # Check if description was explicitly provided (even if null) + # UNSET means "keep existing", whereas None means "clear" + description = deck_in.description if "description" in deck_in.model_fields_set else UNSET + try: deck = await deck_service.update_deck( user_id=current_user.id, @@ -202,7 +210,7 @@ async def update_deck( cards=deck_in.cards, energy_cards=deck_in.energy_cards, validate_ownership=deck_in.validate_ownership, - description=deck_in.description, + description=description, ) except DeckNotFoundError: raise HTTPException( diff --git a/backend/app/repositories/postgres/deck.py b/backend/app/repositories/postgres/deck.py index 9865101..9bd9fe4 100644 --- a/backend/app/repositories/postgres/deck.py +++ b/backend/app/repositories/postgres/deck.py @@ -15,7 +15,7 @@ from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession from app.db.models.deck import Deck -from app.repositories.protocols import DeckEntry +from app.repositories.protocols import UNSET, DeckEntry def _to_dto(model: Deck) -> DeckEntry: @@ -164,21 +164,22 @@ class PostgresDeckRepository: cards: dict[str, int] | None = None, energy_cards: dict[str, int] | None = None, is_valid: bool | None = None, - validation_errors: list[str] | None = None, - description: str | None = None, + validation_errors: list[str] | None = UNSET, # type: ignore[assignment] + description: str | None = UNSET, # type: ignore[assignment] ) -> DeckEntry | None: """Update an existing deck. - Only provided (non-None) fields are updated. + Only provided (non-UNSET) fields are updated. + Use UNSET (default) to keep existing value, or None to clear. Args: deck_id: The deck's UUID. - name: New name (optional). - cards: New card composition (optional). - energy_cards: New energy composition (optional). - is_valid: New validation status (optional). - validation_errors: New validation errors (optional). - description: New description (optional). + name: New name (optional, None keeps existing). + cards: New card composition (optional, None keeps existing). + energy_cards: New energy composition (optional, None keeps existing). + is_valid: New validation status (optional, None keeps existing). + validation_errors: New errors (UNSET=keep, None=clear, list=set). + description: New description (UNSET=keep, None=clear, str=set). Returns: Updated DeckEntry, or None if deck not found. @@ -197,9 +198,10 @@ class PostgresDeckRepository: deck.energy_cards = energy_cards if is_valid is not None: deck.is_valid = is_valid - if validation_errors is not None: + # Use UNSET pattern for nullable fields that can be cleared + if validation_errors is not UNSET: deck.validation_errors = validation_errors - if description is not None: + if description is not UNSET: deck.description = description await self._db.commit() diff --git a/backend/app/repositories/protocols.py b/backend/app/repositories/protocols.py index e06f682..9dbdbad 100644 --- a/backend/app/repositories/protocols.py +++ b/backend/app/repositories/protocols.py @@ -24,11 +24,38 @@ Example: from dataclasses import dataclass from datetime import datetime -from typing import Protocol +from typing import Any, Protocol from uuid import UUID from app.db.models.collection import CardSource +# ============================================================================= +# Sentinel Value for Optional Updates +# ============================================================================= +# UNSET is used to distinguish between "not provided" (keep existing) +# and "explicitly set to None" (clear the field). +# +# Example usage: +# async def update(description: str | None = UNSET): +# if description is not UNSET: +# # User explicitly provided a value (could be None to clear) +# record.description = description + + +class _UnsetType: + """Sentinel class for unset optional parameters.""" + + __slots__ = () + + def __repr__(self) -> str: + return "UNSET" + + def __bool__(self) -> bool: + return False + + +UNSET: Any = _UnsetType() + # ============================================================================= # Data Transfer Objects (DTOs) # ============================================================================= @@ -272,21 +299,22 @@ class DeckRepository(Protocol): cards: dict[str, int] | None = None, energy_cards: dict[str, int] | None = None, is_valid: bool | None = None, - validation_errors: list[str] | None = None, - description: str | None = None, + validation_errors: list[str] | None = UNSET, # type: ignore[assignment] + description: str | None = UNSET, # type: ignore[assignment] ) -> DeckEntry | None: """Update an existing deck. - Only provided (non-None) fields are updated. + Only provided (non-UNSET) fields are updated. + Use UNSET (default) to keep existing value, or None to clear. Args: deck_id: The deck's UUID. - name: New name (optional). - cards: New card composition (optional). - energy_cards: New energy composition (optional). - is_valid: New validation status (optional). - validation_errors: New validation errors (optional). - description: New description (optional). + name: New name (optional, None keeps existing). + cards: New card composition (optional, None keeps existing). + energy_cards: New energy composition (optional, None keeps existing). + is_valid: New validation status (optional, None keeps existing). + validation_errors: New errors (UNSET=keep, None=clear, list=set). + description: New description (UNSET=keep, None=clear, str=set). Returns: Updated DeckEntry, or None if deck not found. diff --git a/backend/app/schemas/deck.py b/backend/app/schemas/deck.py index 6e42381..a012ff7 100644 --- a/backend/app/schemas/deck.py +++ b/backend/app/schemas/deck.py @@ -28,10 +28,14 @@ from uuid import UUID from pydantic import BaseModel, Field, field_validator from app.core.config import DeckConfig +from app.core.enums import EnergyType # Card ID format: set-number-name (e.g., a1-001-bulbasaur, a1-094-pikachu-ex) CARD_ID_PATTERN = re.compile(r"^[a-z0-9]+-\d{3}-[a-z0-9-]+$") +# Set of valid energy type names (lowercase values from EnergyType enum) +VALID_ENERGY_TYPES: frozenset[str] = frozenset(e.value for e in EnergyType) + def validate_card_quantities(cards: dict[str, int], field_name: str) -> dict[str, int]: """Validate card quantities are within valid range. @@ -58,6 +62,31 @@ def validate_card_quantities(cards: dict[str, int], field_name: str) -> dict[str return cards +def validate_energy_types(energy_cards: dict[str, int], field_name: str) -> dict[str, int]: + """Validate energy type names are valid. + + Args: + energy_cards: Mapping of energy type names to quantities. + field_name: Name of the field for error messages. + + Returns: + The validated energy card dictionary. + + Raises: + ValueError: If any energy type is invalid. + """ + invalid_types = [et for et in energy_cards if et not in VALID_ENERGY_TYPES] + if invalid_types: + display_types = invalid_types[:5] + more = len(invalid_types) - 5 + error_msg = f"{field_name}: invalid energy types: {', '.join(display_types)}" + if more > 0: + error_msg += f" (and {more} more)" + error_msg += f". Valid types: {', '.join(sorted(VALID_ENERGY_TYPES))}" + raise ValueError(error_msg) + return energy_cards + + def validate_card_id_format(cards: dict[str, int], field_name: str) -> dict[str, int]: """Validate card IDs match expected format. @@ -116,7 +145,8 @@ class DeckCreateRequest(BaseModel): @field_validator("energy_cards") @classmethod def validate_energy_cards(cls, v: dict[str, int]) -> dict[str, int]: - """Validate energy card quantities.""" + """Validate energy card types and quantities.""" + validate_energy_types(v, "energy_cards") validate_card_quantities(v, "energy_cards") return v @@ -161,8 +191,9 @@ class DeckUpdateRequest(BaseModel): @field_validator("energy_cards") @classmethod def validate_energy_cards(cls, v: dict[str, int] | None) -> dict[str, int] | None: - """Validate energy card quantities if provided.""" + """Validate energy card types and quantities if provided.""" if v is not None: + validate_energy_types(v, "energy_cards") validate_card_quantities(v, "energy_cards") return v @@ -249,7 +280,8 @@ class DeckValidateRequest(BaseModel): @field_validator("energy_cards") @classmethod def validate_energy_cards(cls, v: dict[str, int]) -> dict[str, int]: - """Validate energy card quantities.""" + """Validate energy card types and quantities.""" + validate_energy_types(v, "energy_cards") validate_card_quantities(v, "energy_cards") return v diff --git a/backend/app/services/collection_service.py b/backend/app/services/collection_service.py index 50fbb7d..81b62e6 100644 --- a/backend/app/services/collection_service.py +++ b/backend/app/services/collection_service.py @@ -209,6 +209,11 @@ class CollectionService: ) -> list[CollectionEntry]: """Grant all cards from a starter deck to user's collection. + .. deprecated:: + Use DeckService.select_and_grant_starter_deck() instead, which + atomically creates the starter deck AND grants the cards with + race condition protection. + Uses CardSource.STARTER for all granted cards. Args: @@ -243,19 +248,6 @@ class CollectionService: return entries - async def has_starter_deck(self, user_id: UUID) -> bool: - """Check if user has already received a starter deck. - - Checks for any cards with STARTER source in collection. - - Args: - user_id: The user's UUID. - - Returns: - True if user has starter cards, False otherwise. - """ - return await self._repo.exists_with_source(user_id, CardSource.STARTER) - async def get_collection_stats(self, user_id: UUID) -> dict[str, int]: """Get aggregate statistics for user's collection. diff --git a/backend/app/services/deck_service.py b/backend/app/services/deck_service.py index 08790b8..9a2dcd9 100644 --- a/backend/app/services/deck_service.py +++ b/backend/app/services/deck_service.py @@ -46,6 +46,7 @@ from sqlalchemy.exc import IntegrityError from app.core.config import DeckConfig from app.core.models.card import CardDefinition from app.repositories.protocols import ( + UNSET, CollectionRepository, DeckEntry, DeckRepository, @@ -186,7 +187,7 @@ class DeckService: cards: dict[str, int] | None = None, energy_cards: dict[str, int] | None = None, validate_ownership: bool = True, - description: str | None = None, + description: str | None = UNSET, # type: ignore[assignment] ) -> DeckEntry: """Update an existing deck. @@ -196,11 +197,11 @@ class DeckService: user_id: The user's UUID (for ownership verification). deck_id: The deck's UUID. deck_config: Deck rules from the caller (frontend provides this). - name: New name (optional). - cards: New card composition (optional). - energy_cards: New energy composition (optional). + name: New name (optional, None keeps existing). + cards: New card composition (optional, None keeps existing). + energy_cards: New energy composition (optional, None keeps existing). validate_ownership: If True, checks card ownership (campaign mode). - description: New description (optional). + description: New description (UNSET=keep, None=clear, str=set). Returns: The updated DeckEntry. @@ -370,14 +371,31 @@ class DeckService: Raises: DeckNotFoundError: If deck not found or not owned by user. + ValueError: If deck contains invalid card IDs that cannot be resolved. """ deck = await self.get_deck(user_id, deck_id) cards: list[CardDefinition] = [] + invalid_card_ids: list[str] = [] + for card_id, quantity in deck.cards.items(): card_def = self._card_service.get_card(card_id) if card_def is not None: cards.extend([card_def] * quantity) + else: + invalid_card_ids.append(card_id) + + if invalid_card_ids: + logger = logging.getLogger(__name__) + logger.warning( + f"Deck {deck_id} contains invalid card IDs: {invalid_card_ids[:5]}" + + (f" (and {len(invalid_card_ids) - 5} more)" if len(invalid_card_ids) > 5 else "") + ) + raise ValueError( + f"Deck contains {len(invalid_card_ids)} invalid card(s) that cannot be found: " + f"{', '.join(invalid_card_ids[:5])}" + + (f" (and {len(invalid_card_ids) - 5} more)" if len(invalid_card_ids) > 5 else "") + ) return cards @@ -401,8 +419,11 @@ class DeckService: ) -> DeckEntry: """Create a starter deck for a user. + .. deprecated:: + Use select_and_grant_starter_deck() instead, which atomically + creates the deck AND grants cards with race condition protection. + This creates the deck but does NOT grant the cards to collection. - Use CollectionService.grant_starter_deck() to grant the cards. Args: user_id: The user's UUID. diff --git a/backend/app/services/deck_validator.py b/backend/app/services/deck_validator.py index 4e2ac11..9bdc7fd 100644 --- a/backend/app/services/deck_validator.py +++ b/backend/app/services/deck_validator.py @@ -25,8 +25,12 @@ from collections.abc import Callable from dataclasses import dataclass, field from app.core.config import DeckConfig +from app.core.enums import EnergyType from app.core.models.card import CardDefinition +# Set of valid energy type names (lowercase values from EnergyType enum) +VALID_ENERGY_TYPES: frozenset[str] = frozenset(e.value for e in EnergyType) + @dataclass class ValidationResult: @@ -93,6 +97,16 @@ def validate_deck( f"got {total_energy}" ) + # 2b. Validate energy types are valid + invalid_energy_types = [et for et in energy_cards if et not in VALID_ENERGY_TYPES] + if invalid_energy_types: + display_types = invalid_energy_types[:5] + more = len(invalid_energy_types) - 5 + error_msg = f"Invalid energy types: {', '.join(display_types)}" + if more > 0: + error_msg += f" (and {more} more)" + result.add_error(error_msg) + # 3. Validate max copies per card for card_id, quantity in cards.items(): if quantity > deck_config.max_copies_per_card: @@ -182,3 +196,19 @@ def count_basic_pokemon( if card_def and card_def.is_basic_pokemon(): count += quantity return count + + +def validate_energy_types(energy_types: list[str]) -> list[str]: + """Check which energy type names are invalid. + + Args: + energy_types: List of energy type names to check. + + Returns: + List of invalid energy type names (empty if all valid). + + Example: + invalid = validate_energy_types(["grass", "fire", "invalid"]) + # Returns: ["invalid"] + """ + return [et for et in energy_types if et not in VALID_ENERGY_TYPES] diff --git a/backend/tests/services/test_deck_service.py b/backend/tests/services/test_deck_service.py index e83628c..35e6fa9 100644 --- a/backend/tests/services/test_deck_service.py +++ b/backend/tests/services/test_deck_service.py @@ -801,3 +801,83 @@ class TestOwnershipValidation: # Should NOT have ownership errors ownership_errors = [e for e in (deck.validation_errors or []) if "Insufficient" in e] assert len(ownership_errors) == 0 + + +# ============================================================================= +# Get Deck For Game Tests +# ============================================================================= + + +class TestGetDeckForGame: + """Tests for expanding decks to CardDefinition lists for gameplay.""" + + @pytest.mark.asyncio + async def test_get_deck_for_game_returns_card_definitions( + self, + db_session: AsyncSession, + deck_service: DeckService, + ): + """ + Test that get_deck_for_game returns CardDefinition objects. + + Expands the deck dict into a list of CardDefinition with duplicates + based on quantity. + """ + user = await UserFactory.create(db_session) + # Create a deck with valid cards (a1-001-bulbasaur exists in test data) + deck = await DeckFactory.create_for_user( + db_session, + user, + cards={"a1-001-bulbasaur": 4}, + energy_cards={"grass": 20}, + ) + + cards = await deck_service.get_deck_for_game(user.id, deck.id) + + assert len(cards) == 4 + assert all(card.id == "a1-001-bulbasaur" for card in cards) + + @pytest.mark.asyncio + async def test_get_deck_for_game_deck_not_found( + self, + db_session: AsyncSession, + deck_service: DeckService, + ): + """ + Test that get_deck_for_game raises DeckNotFoundError. + + Should fail for non-existent deck. + """ + user = await UserFactory.create(db_session) + from uuid import uuid4 + + with pytest.raises(DeckNotFoundError): + await deck_service.get_deck_for_game(user.id, uuid4()) + + @pytest.mark.asyncio + async def test_get_deck_for_game_invalid_card_raises( + self, + db_session: AsyncSession, + deck_service: DeckService, + ): + """ + Test that get_deck_for_game raises ValueError for invalid cards. + + If the deck contains card IDs that don't exist in the card database, + the method should raise ValueError rather than silently skipping. + This prevents games from starting with incomplete decks. + """ + user = await UserFactory.create(db_session) + # Create a deck with invalid card ID + deck = await DeckFactory.create_for_user( + db_session, + user, + cards={"nonexistent-card-id": 4, "also-invalid": 2}, + energy_cards={"grass": 20}, + ) + + with pytest.raises(ValueError) as exc_info: + await deck_service.get_deck_for_game(user.id, deck.id) + + assert "invalid card" in str(exc_info.value).lower() + assert "nonexistent-card-id" in str(exc_info.value) diff --git a/backend/tests/unit/services/test_deck_validator.py b/backend/tests/unit/services/test_deck_validator.py index 4e6c888..e8e6a76 100644 --- a/backend/tests/unit/services/test_deck_validator.py +++ b/backend/tests/unit/services/test_deck_validator.py @@ -18,10 +18,12 @@ from app.core.config import DeckConfig from app.core.enums import CardType, EnergyType, PokemonStage from app.core.models.card import CardDefinition from app.services.deck_validator import ( + VALID_ENERGY_TYPES, ValidationResult, count_basic_pokemon, validate_cards_exist, validate_deck, + validate_energy_types, ) # ============================================================================= @@ -209,6 +211,64 @@ class TestEnergyCountValidation: assert any("got 21" in e for e in result.errors) +class TestEnergyTypeValidation: + """Tests for energy type name validation.""" + + def test_valid_energy_types_pass(self, default_config): + """Test that valid energy type names pass validation. + + Uses the standard EnergyType enum values (lowercase). + """ + cards = {f"card-{i:03d}": 4 for i in range(10)} + energy = {"fire": 10, "water": 5, "grass": 5} # All valid types + + result = validate_deck(cards, energy, default_config, basic_pokemon_lookup) + + assert "Invalid energy types" not in str(result.errors) + + def test_invalid_energy_type_fails(self, default_config): + """Test that invalid energy type names fail validation. + + Energy types must match EnergyType enum values (e.g., 'fire', not 'FIRE'). + """ + cards = {f"card-{i:03d}": 4 for i in range(10)} + energy = {"invalid_type": 20} + + result = validate_deck(cards, energy, default_config, basic_pokemon_lookup) + + assert result.is_valid is False + assert any("Invalid energy types" in e for e in result.errors) + assert any("invalid_type" in e for e in result.errors) + + def test_multiple_invalid_energy_types_reported(self, default_config): + """Test that multiple invalid energy types are all reported. + + The error message should list up to 5 invalid types. + """ + cards = {f"card-{i:03d}": 4 for i in range(10)} + energy = {"bad_type1": 10, "bad_type2": 5, "grass": 5} # 2 invalid + + result = validate_deck(cards, energy, default_config, basic_pokemon_lookup) + + assert result.is_valid is False + error_str = str(result.errors) + assert "bad_type1" in error_str + assert "bad_type2" in error_str + + def test_case_sensitive_energy_types(self, default_config): + """Test that energy type validation is case-sensitive. + + EnergyType uses lowercase values, so 'FIRE' should fail. + """ + cards = {f"card-{i:03d}": 4 for i in range(10)} + energy = {"FIRE": 10, "Fire": 10} # Both wrong case + + result = validate_deck(cards, energy, default_config, basic_pokemon_lookup) + + assert result.is_valid is False + assert any("Invalid energy types" in e for e in result.errors) + + # ============================================================================= # Max Copies Per Card Tests # ============================================================================= @@ -513,3 +573,36 @@ class TestUtilityFunctions: count = count_basic_pokemon(cards, mixed_lookup) assert count == 7 # basic-1: 4 + basic-2: 3 + + def test_validate_energy_types_all_valid(self): + """Test validate_energy_types returns empty list when all valid.""" + energy_types = ["fire", "water", "grass", "colorless"] + + invalid = validate_energy_types(energy_types) + + assert invalid == [] + + def test_validate_energy_types_some_invalid(self): + """Test validate_energy_types returns invalid types.""" + energy_types = ["fire", "bad_type", "water", "invalid"] + + invalid = validate_energy_types(energy_types) + + assert set(invalid) == {"bad_type", "invalid"} + + def test_valid_energy_types_constant(self): + """Test VALID_ENERGY_TYPES matches EnergyType enum.""" + expected = { + "colorless", + "darkness", + "dragon", + "fighting", + "fire", + "grass", + "lightning", + "metal", + "psychic", + "water", + } + + assert expected == VALID_ENERGY_TYPES