From 088d30b96b297e2359cf0068965bc49d373f4808 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 00:00:12 -0500 Subject: [PATCH 1/6] docs: fix dev PostgreSQL container/db/user in CLAUDE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dev environment uses sba_postgres container, paperdynasty_dev database, sba_admin user — not pd_postgres/pd_master as previously documented. Co-Authored-By: Claude Opus 4.6 (1M context) --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index e57b6ed..f3c3426 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,7 +31,7 @@ docker build -t paper-dynasty-db . # Build image | **URL** | pddev.manticorum.com | pd.manticorum.com | | **Host** | `ssh pd-database` | `ssh akamai` → `/root/container-data/paper-dynasty` | | **API container** | `dev_pd_database` | `pd_api` | -| **PostgreSQL** | `pd_postgres` (port 5432) | `pd_postgres` | +| **PostgreSQL** | `sba_postgres` / `paperdynasty_dev` / `sba_admin` | `pd_postgres` / `pd_master` | | **Adminer** | port 8081 | — | | **API port** | 816 | 815 | | **Image** | `manticorum67/paper-dynasty-database` | `manticorum67/paper-dynasty-database` | From 534d50f1a88e3ead708b162b47a2c0e968b43c36 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 17:05:53 -0500 Subject: [PATCH 2/6] feat: add card_storage S3 upload utility for variant cards New service with S3 upload functions for the refractor card art pipeline. backfill_variant_image_url reads rendered PNGs from disk, uploads to S3, and sets image_url on BattingCard/PitchingCard rows. 18 tests covering key construction, URL formatting, upload params, and error swallowing. Co-Authored-By: Claude Opus 4.6 (1M context) --- app/services/card_storage.py | 201 ++++++++++++++++++++ requirements.txt | 1 + tests/test_card_storage.py | 350 +++++++++++++++++++++++++++++++++++ 3 files changed, 552 insertions(+) create mode 100644 app/services/card_storage.py create mode 100644 tests/test_card_storage.py diff --git a/app/services/card_storage.py b/app/services/card_storage.py new file mode 100644 index 0000000..38def67 --- /dev/null +++ b/app/services/card_storage.py @@ -0,0 +1,201 @@ +""" +card_storage.py — S3 upload utility for variant card images. + +Public API +---------- +get_s3_client() + Create and return a boto3 S3 client using ambient AWS credentials + (environment variables or instance profile). + +build_s3_key(cardset_id, player_id, variant, card_type) + Construct the S3 object key for a variant card image. + +build_s3_url(s3_key, render_date) + Return the full HTTPS S3 URL with a cache-busting date query param. + +upload_card_to_s3(s3_client, png_bytes, s3_key) + Upload raw PNG bytes to S3 with correct ContentType and CacheControl headers. + +backfill_variant_image_url(player_id, variant, card_type, cardset_id, png_path) + End-to-end: read PNG from disk, upload to S3, update BattingCard or + PitchingCard.image_url in the database. All exceptions are caught and + logged; this function never raises (safe to call as a background task). + +Design notes +------------ +- backfill_variant_image_url uses a lazy import for BattingCard/PitchingCard + to avoid circular imports at module load time (db_engine imports routers + indirectly via the app module in some paths). +- S3 credentials are resolved from the environment by boto3 at call time; + no credentials are hard-coded here. +- The cache-bust ?d= param matches the card-creation pipeline convention so + that clients can compare URLs across pipelines. +""" + +import logging +from datetime import date + +import boto3 + +logger = logging.getLogger(__name__) + +S3_BUCKET = "paper-dynasty" +S3_REGION = "us-east-1" + + +def get_s3_client(): + """Create and return a boto3 S3 client for the configured region. + + Credentials are resolved by boto3 from the standard chain: + environment variables → ~/.aws/credentials → instance profile. + + Returns: + A boto3 S3 client instance. + """ + return boto3.client("s3", region_name=S3_REGION) + + +def build_s3_key(cardset_id: int, player_id: int, variant: int, card_type: str) -> str: + """Construct the S3 object key for a variant card image. + + Key format: + cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.png + + Args: + cardset_id: Numeric cardset ID (zero-padded to 3 digits). + player_id: Player ID. + variant: Variant number (0 = base, 1-4 = refractor tiers). + card_type: Either "batting" or "pitching". + + Returns: + The S3 object key string. + """ + return ( + f"cards/cardset-{cardset_id:03d}/player-{player_id}" + f"/v{variant}/{card_type}card.png" + ) + + +def build_s3_url(s3_key: str, render_date: date) -> str: + """Return the full HTTPS S3 URL for a card image with a cache-bust param. + + URL format: + https://{bucket}.s3.{region}.amazonaws.com/{key}?d={date} + + The ?d= query param matches the card-creation pipeline convention so that + clients invalidate their cache after each re-render. + + Args: + s3_key: S3 object key (from build_s3_key). + render_date: The date the card was rendered, used for cache-busting. + + Returns: + Full HTTPS URL string. + """ + base_url = f"https://{S3_BUCKET}.s3.{S3_REGION}.amazonaws.com" + date_str = render_date.strftime("%Y-%m-%d") + return f"{base_url}/{s3_key}?d={date_str}" + + +def upload_card_to_s3(s3_client, png_bytes: bytes, s3_key: str) -> None: + """Upload raw PNG bytes to S3 with the standard card image headers. + + Sets ContentType=image/png and CacheControl=public, max-age=300 (5 min) + so that CDN and browser caches are refreshed within a short window after + a re-render. + + Args: + s3_client: A boto3 S3 client (from get_s3_client). + png_bytes: Raw PNG image bytes. + s3_key: S3 object key (from build_s3_key). + + Returns: + None + """ + s3_client.put_object( + Bucket=S3_BUCKET, + Key=s3_key, + Body=png_bytes, + ContentType="image/png", + CacheControl="public, max-age=300", + ) + + +def backfill_variant_image_url( + player_id: int, + variant: int, + card_type: str, + cardset_id: int, + png_path: str, +) -> None: + """Read a rendered PNG from disk, upload it to S3, and update the DB row. + + Determines the correct card model (BattingCard or PitchingCard) from + card_type, then: + 1. Reads PNG bytes from png_path. + 2. Uploads to S3 via upload_card_to_s3. + 3. Fetches the card row by (player_id, variant). + 4. Sets image_url to the new S3 URL and calls save(). + + All exceptions are caught and logged — this function is intended to be + called as a background task and must never propagate exceptions. + + Args: + player_id: Player ID used to locate the card row. + variant: Variant number (matches the card row's variant field). + card_type: "batting" or "pitching" — selects the model. + cardset_id: Cardset ID used for the S3 key. + png_path: Absolute path to the rendered PNG file on disk. + + Returns: + None + """ + # Lazy import to avoid circular imports at module load time + from app.db_engine import BattingCard, PitchingCard + + try: + # 1. Read PNG from disk + with open(png_path, "rb") as f: + png_bytes = f.read() + + # 2. Build key and upload + s3_key = build_s3_key( + cardset_id=cardset_id, + player_id=player_id, + variant=variant, + card_type=card_type, + ) + s3_client = get_s3_client() + upload_card_to_s3(s3_client, png_bytes, s3_key) + + # 3. Build URL with today's date for cache-busting + image_url = build_s3_url(s3_key, render_date=date.today()) + + # 4. Locate the card row and update image_url + if card_type == "batting": + card = BattingCard.get( + BattingCard.player == player_id, BattingCard.variant == variant + ) + else: + card = PitchingCard.get( + PitchingCard.player == player_id, PitchingCard.variant == variant + ) + + card.image_url = image_url + card.save() + + logger.info( + "backfill_variant_image_url: updated %s card player=%s variant=%s url=%s", + card_type, + player_id, + variant, + image_url, + ) + + except Exception: + logger.exception( + "backfill_variant_image_url: failed for player=%s variant=%s card_type=%s", + player_id, + variant, + card_type, + ) diff --git a/requirements.txt b/requirements.txt index f3dc46d..7d2cd9d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,3 +12,4 @@ requests==2.32.3 html2image==2.0.6 jinja2==3.1.4 playwright==1.45.1 +boto3>=1.34.0 diff --git a/tests/test_card_storage.py b/tests/test_card_storage.py new file mode 100644 index 0000000..6fe528e --- /dev/null +++ b/tests/test_card_storage.py @@ -0,0 +1,350 @@ +""" +Unit tests for app/services/card_storage.py — S3 upload utility. + +This module covers: + - S3 key construction for variant cards (batting, pitching, zero-padded cardset) + - Full S3 URL construction with cache-busting date param + - put_object call validation (correct params, return value) + - End-to-end backfill: read PNG from disk, upload to S3, update DB row + +Why we test S3 key construction separately: + The key format is a contract used by both the renderer and the URL builder. + Validating it in isolation catches regressions before they corrupt stored URLs. + +Why we test URL construction separately: + The cache-bust param (?d=...) must be appended consistently so that clients + invalidate cached images after a re-render. Testing it independently prevents + the formatter from silently changing. + +Why we test upload params: + ContentType and CacheControl must be set exactly so that S3 serves images + with the correct headers. A missing header is a silent misconfiguration. + +Why we test backfill error swallowing: + The backfill function is called as a background task — it must never raise + exceptions that would abort a card render response. We verify that S3 failures + and missing files are both silently logged, not propagated. + +Test isolation: + All tests use unittest.mock; no real S3 calls or DB connections are made. + The `backfill_variant_image_url` tests patch `get_s3_client` and the DB + model classes at the card_storage module level so lazy imports work correctly. +""" + +import os +from datetime import date +from unittest.mock import MagicMock, patch + +# Set env before importing module so db_engine doesn't try to connect +os.environ.setdefault("DATABASE_TYPE", "postgresql") +os.environ.setdefault("POSTGRES_PASSWORD", "test-dummy") + +from app.services.card_storage import ( + build_s3_key, + build_s3_url, + upload_card_to_s3, + backfill_variant_image_url, + S3_BUCKET, + S3_REGION, +) + + +# --------------------------------------------------------------------------- +# TestBuildS3Key +# --------------------------------------------------------------------------- + + +class TestBuildS3Key: + """Tests for build_s3_key — S3 object key construction. + + The key format must match the existing card-creation pipeline so that + the database API and card-creation tool write to the same S3 paths. + """ + + def test_batting_card_key(self): + """batting card type produces 'battingcard.png' in the key.""" + key = build_s3_key(cardset_id=27, player_id=42, variant=1, card_type="batting") + assert key == "cards/cardset-027/player-42/v1/battingcard.png" + + def test_pitching_card_key(self): + """pitching card type produces 'pitchingcard.png' in the key.""" + key = build_s3_key(cardset_id=27, player_id=99, variant=2, card_type="pitching") + assert key == "cards/cardset-027/player-99/v2/pitchingcard.png" + + def test_cardset_zero_padded_to_three_digits(self): + """Single-digit cardset IDs are zero-padded to three characters.""" + key = build_s3_key(cardset_id=5, player_id=1, variant=0, card_type="batting") + assert "cardset-005" in key + + def test_cardset_two_digit_zero_padded(self): + """Two-digit cardset IDs are zero-padded correctly.""" + key = build_s3_key(cardset_id=27, player_id=1, variant=0, card_type="batting") + assert "cardset-027" in key + + def test_cardset_three_digit_no_padding(self): + """Three-digit cardset IDs are not altered.""" + key = build_s3_key(cardset_id=100, player_id=1, variant=0, card_type="batting") + assert "cardset-100" in key + + def test_variant_included_in_key(self): + """Variant number is included in the path so variants have distinct keys.""" + key_v0 = build_s3_key( + cardset_id=27, player_id=1, variant=0, card_type="batting" + ) + key_v3 = build_s3_key( + cardset_id=27, player_id=1, variant=3, card_type="batting" + ) + assert "/v0/" in key_v0 + assert "/v3/" in key_v3 + assert key_v0 != key_v3 + + +# --------------------------------------------------------------------------- +# TestBuildS3Url +# --------------------------------------------------------------------------- + + +class TestBuildS3Url: + """Tests for build_s3_url — full URL construction with cache-bust param. + + The URL format must be predictable so clients can construct and verify + image URLs without querying the database. + """ + + def test_url_contains_bucket_and_region(self): + """URL includes bucket name and region in the S3 hostname.""" + key = "cards/cardset-027/player-42/v1/battingcard.png" + render_date = date(2026, 4, 6) + url = build_s3_url(key, render_date) + assert S3_BUCKET in url + assert S3_REGION in url + + def test_url_contains_s3_key(self): + """URL path includes the full S3 key.""" + key = "cards/cardset-027/player-42/v1/battingcard.png" + render_date = date(2026, 4, 6) + url = build_s3_url(key, render_date) + assert key in url + + def test_url_has_cache_bust_param(self): + """URL ends with ?d= for cache invalidation.""" + key = "cards/cardset-027/player-42/v1/battingcard.png" + render_date = date(2026, 4, 6) + url = build_s3_url(key, render_date) + assert "?d=2026-04-06" in url + + def test_url_format_full(self): + """Full URL matches expected S3 pattern exactly.""" + key = "cards/cardset-027/player-1/v0/battingcard.png" + render_date = date(2025, 11, 8) + url = build_s3_url(key, render_date) + expected = ( + f"https://{S3_BUCKET}.s3.{S3_REGION}.amazonaws.com/{key}?d=2025-11-08" + ) + assert url == expected + + +# --------------------------------------------------------------------------- +# TestUploadCardToS3 +# --------------------------------------------------------------------------- + + +class TestUploadCardToS3: + """Tests for upload_card_to_s3 — S3 put_object call validation. + + We verify the exact parameters passed to put_object so that S3 serves + images with the correct Content-Type and Cache-Control headers. + """ + + def test_put_object_called_with_correct_params(self): + """put_object is called once with bucket, key, body, ContentType, CacheControl.""" + mock_client = MagicMock() + png_bytes = b"\x89PNG\r\n\x1a\n" + s3_key = "cards/cardset-027/player-42/v1/battingcard.png" + + upload_card_to_s3(mock_client, png_bytes, s3_key) + + mock_client.put_object.assert_called_once_with( + Bucket=S3_BUCKET, + Key=s3_key, + Body=png_bytes, + ContentType="image/png", + CacheControl="public, max-age=300", + ) + + def test_upload_returns_none(self): + """upload_card_to_s3 returns None (callers should not rely on a return value).""" + mock_client = MagicMock() + result = upload_card_to_s3(mock_client, b"PNG", "some/key.png") + assert result is None + + +# --------------------------------------------------------------------------- +# TestBackfillVariantImageUrl +# --------------------------------------------------------------------------- + + +class TestBackfillVariantImageUrl: + """Tests for backfill_variant_image_url — end-to-end disk→S3→DB path. + + The function is fire-and-forget: it reads a PNG from disk, uploads to S3, + then updates the appropriate card model's image_url. All errors are caught + and logged; the function must never raise. + + Test strategy: + - Use tmp_path for temporary PNG files so no filesystem state leaks. + - Patch get_s3_client at the module level to intercept the S3 call. + - Patch BattingCard/PitchingCard at the module level (lazy import target). + """ + + def test_batting_card_image_url_updated(self, tmp_path): + """BattingCard.image_url is updated after a successful upload.""" + png_path = tmp_path / "card.png" + png_path.write_bytes(b"\x89PNG\r\n\x1a\n fake png data") + + mock_s3 = MagicMock() + mock_card = MagicMock() + + with ( + patch("app.services.card_storage.get_s3_client", return_value=mock_s3), + patch("app.db_engine.BattingCard") as MockBatting, + ): + MockBatting.get.return_value = mock_card + + backfill_variant_image_url( + player_id=42, + variant=1, + card_type="batting", + cardset_id=27, + png_path=str(png_path), + ) + + MockBatting.get.assert_called_once_with( + MockBatting.player == 42, MockBatting.variant == 1 + ) + assert mock_card.image_url is not None + mock_card.save.assert_called_once() + + def test_pitching_card_image_url_updated(self, tmp_path): + """PitchingCard.image_url is updated after a successful upload.""" + png_path = tmp_path / "card.png" + png_path.write_bytes(b"\x89PNG\r\n\x1a\n fake png data") + + mock_s3 = MagicMock() + mock_card = MagicMock() + + with ( + patch("app.services.card_storage.get_s3_client", return_value=mock_s3), + patch("app.db_engine.PitchingCard") as MockPitching, + ): + MockPitching.get.return_value = mock_card + + backfill_variant_image_url( + player_id=99, + variant=2, + card_type="pitching", + cardset_id=27, + png_path=str(png_path), + ) + + MockPitching.get.assert_called_once_with( + MockPitching.player == 99, MockPitching.variant == 2 + ) + assert mock_card.image_url is not None + mock_card.save.assert_called_once() + + def test_s3_upload_called_with_png_bytes(self, tmp_path): + """The PNG bytes read from disk are passed to put_object.""" + png_bytes = b"\x89PNG\r\n\x1a\n real png content" + png_path = tmp_path / "card.png" + png_path.write_bytes(png_bytes) + + mock_s3 = MagicMock() + + with ( + patch("app.services.card_storage.get_s3_client", return_value=mock_s3), + patch("app.db_engine.BattingCard") as MockBatting, + ): + MockBatting.get.return_value = MagicMock() + + backfill_variant_image_url( + player_id=1, + variant=0, + card_type="batting", + cardset_id=5, + png_path=str(png_path), + ) + + mock_s3.put_object.assert_called_once() + call_kwargs = mock_s3.put_object.call_args.kwargs + assert call_kwargs["Body"] == png_bytes + + def test_s3_error_is_swallowed(self, tmp_path): + """If S3 raises an exception, backfill swallows it and returns normally. + + The function is called as a background task — it must never propagate + exceptions that would abort the calling request handler. + """ + png_path = tmp_path / "card.png" + png_path.write_bytes(b"PNG data") + + mock_s3 = MagicMock() + mock_s3.put_object.side_effect = Exception("S3 connection refused") + + with ( + patch("app.services.card_storage.get_s3_client", return_value=mock_s3), + patch("app.db_engine.BattingCard"), + ): + # Must not raise + backfill_variant_image_url( + player_id=1, + variant=0, + card_type="batting", + cardset_id=27, + png_path=str(png_path), + ) + + def test_missing_file_is_swallowed(self, tmp_path): + """If the PNG file does not exist, backfill swallows the error and returns. + + Render failures may leave no file on disk; the background task must + handle this gracefully rather than crashing the request. + """ + missing_path = str(tmp_path / "nonexistent.png") + + with ( + patch("app.services.card_storage.get_s3_client"), + patch("app.db_engine.BattingCard"), + ): + # Must not raise + backfill_variant_image_url( + player_id=1, + variant=0, + card_type="batting", + cardset_id=27, + png_path=missing_path, + ) + + def test_db_error_is_swallowed(self, tmp_path): + """If the DB save raises, backfill swallows it and returns normally.""" + png_path = tmp_path / "card.png" + png_path.write_bytes(b"PNG data") + + mock_s3 = MagicMock() + mock_card = MagicMock() + mock_card.save.side_effect = Exception("DB connection lost") + + with ( + patch("app.services.card_storage.get_s3_client", return_value=mock_s3), + patch("app.db_engine.BattingCard") as MockBatting, + ): + MockBatting.get.return_value = mock_card + + # Must not raise + backfill_variant_image_url( + player_id=1, + variant=0, + card_type="batting", + cardset_id=27, + png_path=str(png_path), + ) From 4ccc0841a8bbc41a177531e0fe9973903281afdd Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 17:12:36 -0500 Subject: [PATCH 3/6] feat: schedule S3 upload for variant cards after Playwright render Adds BackgroundTasks to the card render endpoint. After rendering a variant card (variant > 0) where image_url is None, schedules backfill_variant_image_url to upload the PNG to S3 and populate image_url on the card row. Co-Authored-By: Claude Opus 4.6 (1M context) --- app/routers_v2/players.py | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/app/routers_v2/players.py b/app/routers_v2/players.py index dc3b07b..dbb2ea4 100644 --- a/app/routers_v2/players.py +++ b/app/routers_v2/players.py @@ -2,7 +2,15 @@ import datetime import os.path import pandas as pd -from fastapi import APIRouter, Depends, HTTPException, Request, Response, Query +from fastapi import ( + APIRouter, + BackgroundTasks, + Depends, + HTTPException, + Request, + Response, + Query, +) from fastapi.responses import FileResponse from fastapi.templating import Jinja2Templates from typing import Optional, List, Literal @@ -732,6 +740,7 @@ async def get_one_player(player_id: int, csv: Optional[bool] = False): @router.get("/{player_id}/{card_type}card/{d}/{variant}") async def get_batter_card( request: Request, + background_tasks: BackgroundTasks, player_id: int, card_type: Literal["batting", "pitching"], variant: int = 0, @@ -906,6 +915,27 @@ async def get_batter_card( # save_as=f'{player_id}-{d}-v{variant}.png' # ) + # Schedule S3 upload for variant cards that don't have an image_url yet + if variant > 0 and tier is None: + CardModel = BattingCard if card_type == "batting" else PitchingCard + try: + card_row = CardModel.get( + (CardModel.player_id == player_id) & (CardModel.variant == variant) + ) + if card_row.image_url is None: + from app.services.card_storage import backfill_variant_image_url + + background_tasks.add_task( + backfill_variant_image_url, + player_id=player_id, + variant=variant, + card_type=card_type, + cardset_id=this_player.cardset.id, + png_path=file_path, + ) + except CardModel.DoesNotExist: + pass + return FileResponse(path=file_path, media_type="image/png", headers=headers) From be8bebe663195422653dc0ec9f7653618355fabf Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 17:12:41 -0500 Subject: [PATCH 4/6] feat: include image_url in refractor cards API response Adds image_url field to each card state entry in the GET /api/v2/refractor/cards response. Resolved by looking up the variant BattingCard/PitchingCard row. Returns null when no image has been rendered yet. Co-Authored-By: Claude Opus 4.6 (1M context) --- app/routers_v2/refractor.py | 20 +- tests/test_refractor_image_url.py | 311 ++++++++++++++++++++++++++++++ 2 files changed, 330 insertions(+), 1 deletion(-) create mode 100644 tests/test_refractor_image_url.py diff --git a/app/routers_v2/refractor.py b/app/routers_v2/refractor.py index ba9c4c5..d680a32 100644 --- a/app/routers_v2/refractor.py +++ b/app/routers_v2/refractor.py @@ -4,7 +4,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query import logging from typing import Optional -from ..db_engine import model_to_dict +from ..db_engine import model_to_dict, BattingCard, PitchingCard from ..dependencies import oauth2_scheme, valid_token from ..services.refractor_init import initialize_card_refractor, _determine_card_type @@ -67,6 +67,24 @@ def _build_card_state_response(state, player_name=None) -> dict: if player_name is not None: result["player_name"] = player_name + # Resolve image_url from the variant card row + image_url = None + if state.variant and state.variant > 0: + card_type = ( + state.track.card_type if hasattr(state, "track") and state.track else None + ) + if card_type: + CardModel = BattingCard if card_type == "batter" else PitchingCard + try: + variant_card = CardModel.get( + (CardModel.player_id == state.player_id) + & (CardModel.variant == state.variant) + ) + image_url = variant_card.image_url + except CardModel.DoesNotExist: + pass + result["image_url"] = image_url + return result diff --git a/tests/test_refractor_image_url.py b/tests/test_refractor_image_url.py new file mode 100644 index 0000000..ccdae09 --- /dev/null +++ b/tests/test_refractor_image_url.py @@ -0,0 +1,311 @@ +"""Tests for image_url field in refractor cards API response. + +What: Verifies that GET /api/v2/refractor/cards includes image_url in each card state +item, pulling the URL from the variant BattingCard or PitchingCard row. + +Why: The refractor card art pipeline stores rendered card image URLs in the +BattingCard/PitchingCard rows. The Discord bot and website need image_url in +the /refractor/cards response so they can display variant art without a separate +lookup. These tests guard against regressions where image_url is accidentally +dropped from the response serialization. + +Test cases: + test_cards_response_includes_image_url -- BattingCard with image_url set; verify + the value appears in the /cards response. + test_cards_response_image_url_null_when_not_set -- BattingCard with image_url=None; + verify null is returned (not omitted). + +Uses the shared-memory SQLite TestClient pattern from test_refractor_state_api.py +so no PostgreSQL connection is required. +""" + +import os + +os.environ.setdefault("API_TOKEN", "test") + +import pytest +from fastapi import FastAPI, Request +from fastapi.testclient import TestClient +from peewee import SqliteDatabase + +from app.db_engine import ( + BattingCard, + BattingSeasonStats, + Card, + Cardset, + Decision, + Event, + MlbPlayer, + Pack, + PackType, + PitchingCard, + PitchingSeasonStats, + Player, + ProcessedGame, + Rarity, + RefractorCardState, + RefractorCosmetic, + RefractorTierBoost, + RefractorTrack, + Roster, + RosterSlot, + ScoutClaim, + ScoutOpportunity, + StratGame, + StratPlay, + Team, +) + +AUTH_HEADER = {"Authorization": "Bearer test"} + +# --------------------------------------------------------------------------- +# SQLite database + model list +# --------------------------------------------------------------------------- + +_img_url_db = SqliteDatabase( + "file:imgurlapitest?mode=memory&cache=shared", + uri=True, + pragmas={"foreign_keys": 1}, +) + +# Full model list matching the existing state API tests — needed so all FK +# constraints resolve in SQLite. +_IMG_URL_MODELS = [ + Rarity, + Event, + Cardset, + MlbPlayer, + Player, + BattingCard, + PitchingCard, + Team, + PackType, + Pack, + Card, + Roster, + RosterSlot, + StratGame, + StratPlay, + Decision, + ScoutOpportunity, + ScoutClaim, + BattingSeasonStats, + PitchingSeasonStats, + ProcessedGame, + RefractorTrack, + RefractorCardState, + RefractorTierBoost, + RefractorCosmetic, +] + + +@pytest.fixture(autouse=False) +def setup_img_url_db(): + """Bind image-url test models to shared-memory SQLite and create tables. + + What: Initialises the in-process SQLite database before each test and drops + all tables afterwards to ensure test isolation. + + Why: SQLite shared-memory databases persist between tests in the same + process unless tables are dropped. Creating and dropping around each test + guarantees a clean state without requiring a real PostgreSQL instance. + """ + _img_url_db.bind(_IMG_URL_MODELS) + _img_url_db.connect(reuse_if_open=True) + _img_url_db.create_tables(_IMG_URL_MODELS) + yield _img_url_db + _img_url_db.drop_tables(list(reversed(_IMG_URL_MODELS)), safe=True) + + +def _build_image_url_app() -> FastAPI: + """Minimal FastAPI app with refractor router for image_url tests.""" + from app.routers_v2.refractor import router as refractor_router + + app = FastAPI() + + @app.middleware("http") + async def db_middleware(request: Request, call_next): + _img_url_db.connect(reuse_if_open=True) + return await call_next(request) + + app.include_router(refractor_router) + return app + + +@pytest.fixture +def img_url_client(setup_img_url_db): + """FastAPI TestClient backed by shared-memory SQLite for image_url tests.""" + with TestClient(_build_image_url_app()) as c: + yield c + + +# --------------------------------------------------------------------------- +# Seed helpers +# --------------------------------------------------------------------------- + + +def _make_rarity(): + r, _ = Rarity.get_or_create( + value=10, name="IU_Common", defaults={"color": "#ffffff"} + ) + return r + + +def _make_cardset(): + cs, _ = Cardset.get_or_create( + name="IU Test Set", + defaults={"description": "image url test cardset", "total_cards": 1}, + ) + return cs + + +def _make_player(name: str = "Test Player") -> Player: + return Player.create( + p_name=name, + rarity=_make_rarity(), + cardset=_make_cardset(), + set_num=1, + pos_1="CF", + image="https://example.com/img.png", + mlbclub="TST", + franchise="TST", + description="image url test", + ) + + +def _make_team(suffix: str = "IU") -> Team: + return Team.create( + abbrev=suffix, + sname=suffix, + lname=f"Team {suffix}", + gmid=99900 + len(suffix), + gmname=f"gm_{suffix.lower()}", + gsheet="https://docs.google.com/iu_test", + wallet=500, + team_value=1000, + collection_value=1000, + season=11, + is_ai=False, + ) + + +def _make_track(card_type: str = "batter") -> RefractorTrack: + track, _ = RefractorTrack.get_or_create( + name=f"IU {card_type} Track", + defaults=dict( + card_type=card_type, + formula="pa", + t1_threshold=100, + t2_threshold=300, + t3_threshold=700, + t4_threshold=1200, + ), + ) + return track + + +def _make_batting_card(player: Player, variant: int, image_url=None) -> BattingCard: + return BattingCard.create( + player=player, + variant=variant, + steal_low=1, + steal_high=3, + steal_auto=False, + steal_jump=1.0, + bunting="N", + hit_and_run="N", + running=5, + offense_col=1, + hand="R", + image_url=image_url, + ) + + +def _make_card_state( + player: Player, + team: Team, + track: RefractorTrack, + variant: int, + current_tier: int = 1, + current_value: float = 150.0, +) -> RefractorCardState: + import datetime + + return RefractorCardState.create( + player=player, + team=team, + track=track, + current_tier=current_tier, + current_value=current_value, + fully_evolved=False, + last_evaluated_at=datetime.datetime(2026, 4, 1, 12, 0, 0), + variant=variant, + ) + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +def test_cards_response_includes_image_url(setup_img_url_db, img_url_client): + """GET /api/v2/refractor/cards includes image_url when the variant BattingCard has one. + + What: Seeds a RefractorCardState at variant=1 and a matching BattingCard with + image_url set. Calls the /cards endpoint and asserts that image_url in the + response matches the seeded URL. + + Why: This is the primary happy-path test for the image_url feature. If the + DB lookup in _build_card_state_response fails or the field is accidentally + omitted from the response dict, this test will catch it. + """ + player = _make_player("Homer Simpson") + team = _make_team("IU1") + track = _make_track("batter") + + expected_url = ( + "https://s3.example.com/cards/cardset-001/player-1/v1/battingcard.png" + ) + _make_batting_card(player, variant=1, image_url=expected_url) + _make_card_state(player, team, track, variant=1) + + resp = img_url_client.get( + f"/api/v2/refractor/cards?team_id={team.id}&evaluated_only=false", + headers=AUTH_HEADER, + ) + assert resp.status_code == 200, resp.text + data = resp.json() + assert data["count"] == 1 + item = data["items"][0] + assert "image_url" in item, "image_url key missing from response" + assert item["image_url"] == expected_url + + +def test_cards_response_image_url_null_when_not_set(setup_img_url_db, img_url_client): + """GET /api/v2/refractor/cards returns image_url: null when BattingCard.image_url is None. + + What: Seeds a BattingCard with image_url=None and a RefractorCardState at + variant=1. Verifies the response contains image_url with a null value. + + Why: The image_url field must always be present in the response (even when + null) so API consumers can rely on its presence. Returning null rather than + omitting the key is the correct contract — omitting it would break consumers + that check for the key's presence to determine upload status. + """ + player = _make_player("Bart Simpson") + team = _make_team("IU2") + track = _make_track("batter") + + _make_batting_card(player, variant=1, image_url=None) + _make_card_state(player, team, track, variant=1) + + resp = img_url_client.get( + f"/api/v2/refractor/cards?team_id={team.id}&evaluated_only=false", + headers=AUTH_HEADER, + ) + assert resp.status_code == 200, resp.text + data = resp.json() + assert data["count"] == 1 + item = data["items"][0] + assert "image_url" in item, "image_url key missing from response" + assert item["image_url"] is None From f4a90da6294b73d82fdb4f7dc900e9c5fc5c0ca6 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 17:31:17 -0500 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20review=20feedback=20=E2=80=94=20pin?= =?UTF-8?q?=20boto3,=20use=20player=5Fid=20consistently,=20add=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Pin boto3==1.42.65 to match project convention of exact version pins - Use player_id (not player) for FK column access in card_storage.py to match the pattern used throughout the codebase - Add comment explaining the tier is None guard in S3 upload scheduling Co-Authored-By: Claude Opus 4.6 (1M context) --- app/routers_v2/players.py | 4 +++- app/services/card_storage.py | 4 ++-- requirements.txt | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/routers_v2/players.py b/app/routers_v2/players.py index dbb2ea4..744bad6 100644 --- a/app/routers_v2/players.py +++ b/app/routers_v2/players.py @@ -915,7 +915,9 @@ async def get_batter_card( # save_as=f'{player_id}-{d}-v{variant}.png' # ) - # Schedule S3 upload for variant cards that don't have an image_url yet + # Schedule S3 upload for variant cards that don't have an image_url yet. + # Skip when tier is overridden (?tier= dev preview) — those renders don't + # correspond to real variant card rows. if variant > 0 and tier is None: CardModel = BattingCard if card_type == "batting" else PitchingCard try: diff --git a/app/services/card_storage.py b/app/services/card_storage.py index 38def67..7ad4513 100644 --- a/app/services/card_storage.py +++ b/app/services/card_storage.py @@ -174,11 +174,11 @@ def backfill_variant_image_url( # 4. Locate the card row and update image_url if card_type == "batting": card = BattingCard.get( - BattingCard.player == player_id, BattingCard.variant == variant + BattingCard.player_id == player_id, BattingCard.variant == variant ) else: card = PitchingCard.get( - PitchingCard.player == player_id, PitchingCard.variant == variant + PitchingCard.player_id == player_id, PitchingCard.variant == variant ) card.image_url = image_url diff --git a/requirements.txt b/requirements.txt index 7d2cd9d..c27075d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,4 +12,4 @@ requests==2.32.3 html2image==2.0.6 jinja2==3.1.4 playwright==1.45.1 -boto3>=1.34.0 +boto3==1.42.65 From c75e2781be3d0d3725e5f904d6462108e0b70d84 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 20:02:58 -0500 Subject: [PATCH 6/6] fix: address review feedback (#187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move lazy imports to top level in card_storage.py and players.py (CLAUDE.md violation) - Use os.environ.get() for S3_BUCKET/S3_REGION to allow dev/prod bucket separation - Fix test patch targets from app.db_engine to app.services.card_storage (required after top-level import move) - Fix assert_called_once_with field name: MockBatting.player → MockBatting.player_id Co-Authored-By: Claude Sonnet 4.6 --- app/routers_v2/players.py | 3 +-- app/services/card_storage.py | 13 +++++-------- tests/test_card_storage.py | 16 ++++++++-------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/app/routers_v2/players.py b/app/routers_v2/players.py index 744bad6..6579aae 100644 --- a/app/routers_v2/players.py +++ b/app/routers_v2/players.py @@ -40,6 +40,7 @@ from ..db_engine import ( ) from ..db_helpers import upsert_players from ..dependencies import oauth2_scheme, valid_token +from ..services.card_storage import backfill_variant_image_url from ..services.refractor_boost import compute_variant_hash # --------------------------------------------------------------------------- @@ -925,8 +926,6 @@ async def get_batter_card( (CardModel.player_id == player_id) & (CardModel.variant == variant) ) if card_row.image_url is None: - from app.services.card_storage import backfill_variant_image_url - background_tasks.add_task( backfill_variant_image_url, player_id=player_id, diff --git a/app/services/card_storage.py b/app/services/card_storage.py index 7ad4513..1d68962 100644 --- a/app/services/card_storage.py +++ b/app/services/card_storage.py @@ -23,9 +23,6 @@ backfill_variant_image_url(player_id, variant, card_type, cardset_id, png_path) Design notes ------------ -- backfill_variant_image_url uses a lazy import for BattingCard/PitchingCard - to avoid circular imports at module load time (db_engine imports routers - indirectly via the app module in some paths). - S3 credentials are resolved from the environment by boto3 at call time; no credentials are hard-coded here. - The cache-bust ?d= param matches the card-creation pipeline convention so @@ -33,14 +30,17 @@ Design notes """ import logging +import os from datetime import date import boto3 +from app.db_engine import BattingCard, PitchingCard + logger = logging.getLogger(__name__) -S3_BUCKET = "paper-dynasty" -S3_REGION = "us-east-1" +S3_BUCKET = os.environ.get("S3_BUCKET", "paper-dynasty") +S3_REGION = os.environ.get("S3_REGION", "us-east-1") def get_s3_client(): @@ -150,9 +150,6 @@ def backfill_variant_image_url( Returns: None """ - # Lazy import to avoid circular imports at module load time - from app.db_engine import BattingCard, PitchingCard - try: # 1. Read PNG from disk with open(png_path, "rb") as f: diff --git a/tests/test_card_storage.py b/tests/test_card_storage.py index 6fe528e..4f26d63 100644 --- a/tests/test_card_storage.py +++ b/tests/test_card_storage.py @@ -207,7 +207,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard") as MockBatting, + patch("app.services.card_storage.BattingCard") as MockBatting, ): MockBatting.get.return_value = mock_card @@ -220,7 +220,7 @@ class TestBackfillVariantImageUrl: ) MockBatting.get.assert_called_once_with( - MockBatting.player == 42, MockBatting.variant == 1 + MockBatting.player_id == 42, MockBatting.variant == 1 ) assert mock_card.image_url is not None mock_card.save.assert_called_once() @@ -235,7 +235,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.PitchingCard") as MockPitching, + patch("app.services.card_storage.PitchingCard") as MockPitching, ): MockPitching.get.return_value = mock_card @@ -248,7 +248,7 @@ class TestBackfillVariantImageUrl: ) MockPitching.get.assert_called_once_with( - MockPitching.player == 99, MockPitching.variant == 2 + MockPitching.player_id == 99, MockPitching.variant == 2 ) assert mock_card.image_url is not None mock_card.save.assert_called_once() @@ -263,7 +263,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard") as MockBatting, + patch("app.services.card_storage.BattingCard") as MockBatting, ): MockBatting.get.return_value = MagicMock() @@ -293,7 +293,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard"), + patch("app.services.card_storage.BattingCard"), ): # Must not raise backfill_variant_image_url( @@ -314,7 +314,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client"), - patch("app.db_engine.BattingCard"), + patch("app.services.card_storage.BattingCard"), ): # Must not raise backfill_variant_image_url( @@ -336,7 +336,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard") as MockBatting, + patch("app.services.card_storage.BattingCard") as MockBatting, ): MockBatting.get.return_value = mock_card