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 <noreply@anthropic.com>
This commit is contained in:
Cal Corum 2026-01-28 14:32:08 -06:00
parent 3ec670753b
commit 7d397a2e22
9 changed files with 331 additions and 45 deletions

View File

@ -20,6 +20,7 @@ from uuid import UUID
from fastapi import APIRouter, HTTPException, status from fastapi import APIRouter, HTTPException, status
from app.api.deps import CardServiceDep, CollectionServiceDep, CurrentUser, DeckServiceDep from app.api.deps import CardServiceDep, CollectionServiceDep, CurrentUser, DeckServiceDep
from app.repositories.protocols import UNSET
from app.schemas.deck import ( from app.schemas.deck import (
DeckCreateRequest, DeckCreateRequest,
DeckListResponse, DeckListResponse,
@ -183,6 +184,9 @@ async def update_deck(
Only provided fields are updated. If cards or energy_cards change, Only provided fields are updated. If cards or energy_cards change,
the deck is re-validated with the provided deck_config. 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: Args:
deck_id: The deck's UUID. deck_id: The deck's UUID.
deck_in: Update request with optional fields. deck_in: Update request with optional fields.
@ -193,6 +197,10 @@ async def update_deck(
Raises: Raises:
404: If deck not found or not owned by user. 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: try:
deck = await deck_service.update_deck( deck = await deck_service.update_deck(
user_id=current_user.id, user_id=current_user.id,
@ -202,7 +210,7 @@ async def update_deck(
cards=deck_in.cards, cards=deck_in.cards,
energy_cards=deck_in.energy_cards, energy_cards=deck_in.energy_cards,
validate_ownership=deck_in.validate_ownership, validate_ownership=deck_in.validate_ownership,
description=deck_in.description, description=description,
) )
except DeckNotFoundError: except DeckNotFoundError:
raise HTTPException( raise HTTPException(

View File

@ -15,7 +15,7 @@ from sqlalchemy import func, select
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from app.db.models.deck import Deck 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: def _to_dto(model: Deck) -> DeckEntry:
@ -164,21 +164,22 @@ class PostgresDeckRepository:
cards: dict[str, int] | None = None, cards: dict[str, int] | None = None,
energy_cards: dict[str, int] | None = None, energy_cards: dict[str, int] | None = None,
is_valid: bool | None = None, is_valid: bool | None = None,
validation_errors: list[str] | None = None, validation_errors: list[str] | None = UNSET, # type: ignore[assignment]
description: str | None = None, description: str | None = UNSET, # type: ignore[assignment]
) -> DeckEntry | None: ) -> DeckEntry | None:
"""Update an existing deck. """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: Args:
deck_id: The deck's UUID. deck_id: The deck's UUID.
name: New name (optional). name: New name (optional, None keeps existing).
cards: New card composition (optional). cards: New card composition (optional, None keeps existing).
energy_cards: New energy composition (optional). energy_cards: New energy composition (optional, None keeps existing).
is_valid: New validation status (optional). is_valid: New validation status (optional, None keeps existing).
validation_errors: New validation errors (optional). validation_errors: New errors (UNSET=keep, None=clear, list=set).
description: New description (optional). description: New description (UNSET=keep, None=clear, str=set).
Returns: Returns:
Updated DeckEntry, or None if deck not found. Updated DeckEntry, or None if deck not found.
@ -197,9 +198,10 @@ class PostgresDeckRepository:
deck.energy_cards = energy_cards deck.energy_cards = energy_cards
if is_valid is not None: if is_valid is not None:
deck.is_valid = is_valid 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 deck.validation_errors = validation_errors
if description is not None: if description is not UNSET:
deck.description = description deck.description = description
await self._db.commit() await self._db.commit()

View File

@ -24,11 +24,38 @@ Example:
from dataclasses import dataclass from dataclasses import dataclass
from datetime import datetime from datetime import datetime
from typing import Protocol from typing import Any, Protocol
from uuid import UUID from uuid import UUID
from app.db.models.collection import CardSource 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) # Data Transfer Objects (DTOs)
# ============================================================================= # =============================================================================
@ -272,21 +299,22 @@ class DeckRepository(Protocol):
cards: dict[str, int] | None = None, cards: dict[str, int] | None = None,
energy_cards: dict[str, int] | None = None, energy_cards: dict[str, int] | None = None,
is_valid: bool | None = None, is_valid: bool | None = None,
validation_errors: list[str] | None = None, validation_errors: list[str] | None = UNSET, # type: ignore[assignment]
description: str | None = None, description: str | None = UNSET, # type: ignore[assignment]
) -> DeckEntry | None: ) -> DeckEntry | None:
"""Update an existing deck. """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: Args:
deck_id: The deck's UUID. deck_id: The deck's UUID.
name: New name (optional). name: New name (optional, None keeps existing).
cards: New card composition (optional). cards: New card composition (optional, None keeps existing).
energy_cards: New energy composition (optional). energy_cards: New energy composition (optional, None keeps existing).
is_valid: New validation status (optional). is_valid: New validation status (optional, None keeps existing).
validation_errors: New validation errors (optional). validation_errors: New errors (UNSET=keep, None=clear, list=set).
description: New description (optional). description: New description (UNSET=keep, None=clear, str=set).
Returns: Returns:
Updated DeckEntry, or None if deck not found. Updated DeckEntry, or None if deck not found.

View File

@ -28,10 +28,14 @@ from uuid import UUID
from pydantic import BaseModel, Field, field_validator from pydantic import BaseModel, Field, field_validator
from app.core.config import DeckConfig 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 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-]+$") 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]: def validate_card_quantities(cards: dict[str, int], field_name: str) -> dict[str, int]:
"""Validate card quantities are within valid range. """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 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]: def validate_card_id_format(cards: dict[str, int], field_name: str) -> dict[str, int]:
"""Validate card IDs match expected format. """Validate card IDs match expected format.
@ -116,7 +145,8 @@ class DeckCreateRequest(BaseModel):
@field_validator("energy_cards") @field_validator("energy_cards")
@classmethod @classmethod
def validate_energy_cards(cls, v: dict[str, int]) -> dict[str, int]: 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") validate_card_quantities(v, "energy_cards")
return v return v
@ -161,8 +191,9 @@ class DeckUpdateRequest(BaseModel):
@field_validator("energy_cards") @field_validator("energy_cards")
@classmethod @classmethod
def validate_energy_cards(cls, v: dict[str, int] | None) -> dict[str, int] | None: 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: if v is not None:
validate_energy_types(v, "energy_cards")
validate_card_quantities(v, "energy_cards") validate_card_quantities(v, "energy_cards")
return v return v
@ -249,7 +280,8 @@ class DeckValidateRequest(BaseModel):
@field_validator("energy_cards") @field_validator("energy_cards")
@classmethod @classmethod
def validate_energy_cards(cls, v: dict[str, int]) -> dict[str, int]: 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") validate_card_quantities(v, "energy_cards")
return v return v

View File

@ -209,6 +209,11 @@ class CollectionService:
) -> list[CollectionEntry]: ) -> list[CollectionEntry]:
"""Grant all cards from a starter deck to user's collection. """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. Uses CardSource.STARTER for all granted cards.
Args: Args:
@ -243,19 +248,6 @@ class CollectionService:
return entries 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]: async def get_collection_stats(self, user_id: UUID) -> dict[str, int]:
"""Get aggregate statistics for user's collection. """Get aggregate statistics for user's collection.

View File

@ -46,6 +46,7 @@ from sqlalchemy.exc import IntegrityError
from app.core.config import DeckConfig from app.core.config import DeckConfig
from app.core.models.card import CardDefinition from app.core.models.card import CardDefinition
from app.repositories.protocols import ( from app.repositories.protocols import (
UNSET,
CollectionRepository, CollectionRepository,
DeckEntry, DeckEntry,
DeckRepository, DeckRepository,
@ -186,7 +187,7 @@ class DeckService:
cards: dict[str, int] | None = None, cards: dict[str, int] | None = None,
energy_cards: dict[str, int] | None = None, energy_cards: dict[str, int] | None = None,
validate_ownership: bool = True, validate_ownership: bool = True,
description: str | None = None, description: str | None = UNSET, # type: ignore[assignment]
) -> DeckEntry: ) -> DeckEntry:
"""Update an existing deck. """Update an existing deck.
@ -196,11 +197,11 @@ class DeckService:
user_id: The user's UUID (for ownership verification). user_id: The user's UUID (for ownership verification).
deck_id: The deck's UUID. deck_id: The deck's UUID.
deck_config: Deck rules from the caller (frontend provides this). deck_config: Deck rules from the caller (frontend provides this).
name: New name (optional). name: New name (optional, None keeps existing).
cards: New card composition (optional). cards: New card composition (optional, None keeps existing).
energy_cards: New energy composition (optional). energy_cards: New energy composition (optional, None keeps existing).
validate_ownership: If True, checks card ownership (campaign mode). validate_ownership: If True, checks card ownership (campaign mode).
description: New description (optional). description: New description (UNSET=keep, None=clear, str=set).
Returns: Returns:
The updated DeckEntry. The updated DeckEntry.
@ -370,14 +371,31 @@ class DeckService:
Raises: Raises:
DeckNotFoundError: If deck not found or not owned by user. 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) deck = await self.get_deck(user_id, deck_id)
cards: list[CardDefinition] = [] cards: list[CardDefinition] = []
invalid_card_ids: list[str] = []
for card_id, quantity in deck.cards.items(): for card_id, quantity in deck.cards.items():
card_def = self._card_service.get_card(card_id) card_def = self._card_service.get_card(card_id)
if card_def is not None: if card_def is not None:
cards.extend([card_def] * quantity) 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 return cards
@ -401,8 +419,11 @@ class DeckService:
) -> DeckEntry: ) -> DeckEntry:
"""Create a starter deck for a user. """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. This creates the deck but does NOT grant the cards to collection.
Use CollectionService.grant_starter_deck() to grant the cards.
Args: Args:
user_id: The user's UUID. user_id: The user's UUID.

View File

@ -25,8 +25,12 @@ from collections.abc import Callable
from dataclasses import dataclass, field from dataclasses import dataclass, field
from app.core.config import DeckConfig from app.core.config import DeckConfig
from app.core.enums import EnergyType
from app.core.models.card import CardDefinition 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 @dataclass
class ValidationResult: class ValidationResult:
@ -93,6 +97,16 @@ def validate_deck(
f"got {total_energy}" 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 # 3. Validate max copies per card
for card_id, quantity in cards.items(): for card_id, quantity in cards.items():
if quantity > deck_config.max_copies_per_card: 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(): if card_def and card_def.is_basic_pokemon():
count += quantity count += quantity
return count 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]

View File

@ -801,3 +801,83 @@ class TestOwnershipValidation:
# Should NOT have ownership errors # Should NOT have ownership errors
ownership_errors = [e for e in (deck.validation_errors or []) if "Insufficient" in e] ownership_errors = [e for e in (deck.validation_errors or []) if "Insufficient" in e]
assert len(ownership_errors) == 0 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)

View File

@ -18,10 +18,12 @@ from app.core.config import DeckConfig
from app.core.enums import CardType, EnergyType, PokemonStage from app.core.enums import CardType, EnergyType, PokemonStage
from app.core.models.card import CardDefinition from app.core.models.card import CardDefinition
from app.services.deck_validator import ( from app.services.deck_validator import (
VALID_ENERGY_TYPES,
ValidationResult, ValidationResult,
count_basic_pokemon, count_basic_pokemon,
validate_cards_exist, validate_cards_exist,
validate_deck, validate_deck,
validate_energy_types,
) )
# ============================================================================= # =============================================================================
@ -209,6 +211,64 @@ class TestEnergyCountValidation:
assert any("got 21" in e for e in result.errors) 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 # Max Copies Per Card Tests
# ============================================================================= # =============================================================================
@ -513,3 +573,36 @@ class TestUtilityFunctions:
count = count_basic_pokemon(cards, mixed_lookup) count = count_basic_pokemon(cards, mixed_lookup)
assert count == 7 # basic-1: 4 + basic-2: 3 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