Skip to content

Per-face illustration ids: the column already works — document the data model, the refresh semantics, and the expected coverage - #643

Merged
WilfordGrimley merged 1 commit into
masterfrom
docs/per-face-illustration-data-model
Jul 30, 2026
Merged

Per-face illustration ids: the column already works — document the data model, the refresh semantics, and the expected coverage#643
WilfordGrimley merged 1 commit into
masterfrom
docs/per-face-illustration-data-model

Conversation

@WilfordGrimley

Copy link
Copy Markdown

What this is

An investigation into why CanonicalPrintingMetadata.face_illustrations
reads 0 populated rows in production, and the doc change that follows
from the answer.

The answer is that nothing is broken. The feature is complete and
merged. What was missing was any wiki-facing description of it, plus a
stated expectation for coverage so a mostly-empty column is not later
misread as a bug. No code change was needed and none is made here.

What already existed (all on master, PR #565, merged 2026-07-29)

Piece Where State
Column + partial index migration 0095 applied in prod
Per-face parse PrintingMetadataRow.face_illustrations complete, DOUBLE_FACED_LAYOUTS-gated
Write path import_scryfall_printing_metadata passes face_illustrations=
Diff participation _METADATA_SYNC_FIELDS present
Consumer local_illustration.py reads printing_metadata__face_illustrations
Tests TestFaceIllustrations (17 tests) pass, and genuinely bite (below)

Production reads 0 for one reason only: import_scryfall_printing_metadata
has not run since #565 merged.

Is the importer a full refresh or a diff/upsert? (issue #638)

A full-set, value-diffing upsert — and it does populate existing rows.

_sync_printing_metadata re-derives the desired row for every printing in
the bulk file, streams the stored table, and joins on canonical_card_id:
no match → CREATE; any _METADATA_SYNC_FIELDS member differs → UPDATE;
all equal → SKIP; stored key not in the desired set → DELETE. The diff is
on values, not on a timestamp or a version marker, so a row whose only
difference is face_illustrations: [] → [...] is an UPDATE.

face_illustrations is in _METADATA_SYNC_FIELDS, therefore a plain
re-run backfills all affected pre-existing rows.
No backfill command, no
flag, no migration — consistent with "default the default things".

That membership is the entire load-bearing detail. bulk_create writes
every column regardless of the list, so had face_illustrations been
omitted from _METADATA_SYNC_FIELDS, newly-seen printings would have
populated normally while all 113,224 already-stored rows stayed []
forever — a bug that would look exactly like the symptom reported here.

Expected coverage on the first run — 1.4%, and that is correct

Counted from the on-disk default_cards.json the importer already
downloads (zero new Scryfall requests — the file was read in place in
the prod container) and intersected against production, 2026-07-30:

Measure Count
Bulk-data rows 116,254
Genuine double-faced rows (DOUBLE_FACED_LAYOUTS, ≥2 faces) 1,594
…that join to a CanonicalCard with a metadata row 1,594 (all of them)
…with a real illustration_id on every face 1,534
…with name-only entries (illustration_id: None) 60
Back-face illustrations newly addressable 1,534
CanonicalPrintingMetadata rows total 113,224

A first import populates 1,594 of 113,224 rows (1.4%). The other
~111,630 are single-faced and correctly stay [] — the partial index
cpm_face_illustrations_present is built for exactly that shape. The 60
name-only rows are double-faced layouts for which Scryfall publishes no
per-face artwork id; they record None rather than dropping the face, so
list indices keep meaning "face position".

Red-then-green

The brief asked for a test shown red before the change and green after.
There is no change, so instead the existing tests were shown to be
non-vacuous by mutating the two load-bearing lines on master and
confirming they fail — the check that would have caught a column silently
staying empty:

Mutation Result
baseline (unmodified master) 17 passed
drop "face_illustrations" from _METADATA_SYNC_FIELDS 1 failedtest_a_face_illustration_change_alone_is_a_detected_diff
face_illustrations property returns [] (front-face-only) 9 failed, incl. test_the_import_persists_face_illustrations_to_the_database

Note the first row: only one test catches the sync-fields omission,
because bulk_create masks it for new rows. That single test is what
stands between the feature and a permanently-empty column in production.
Working tree restored; both mutations were local and are not in this PR.

Verification

  • pytest cardpicker/tests/3,416 passed, 8 skipped (4m06s)
  • docs_lint.py --strictclean
  • No migration added; 0095 already exists and is applied, so the
    contended 01000103 range is untouched and PR Fail CI on a migration graph that forks WHEN MERGED, not just on the branch #611's leaf guard is
    unaffected.
  • Read-only throughout: no management command run, no writes to
    production, no Scryfall requests.

The one operational follow-up

Populating the column requires an ordinary import_scryfall_printing_metadata
run. That is a write and was deliberately not performed here.

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

`CanonicalPrintingMetadata.face_illustrations` (migration 0095) has been
built, merged (#565) and applied in production since 2026-07-29, and the
importer already populates it — but nothing said so in a wiki-facing doc,
and production still reads 0 populated rows purely because
`import_scryfall_printing_metadata` has not run since #565 merged.

Records in `docs/features/printing-tags.md` (wiki: `Printing-Tags`):

- the two illustration grains on the model — scalar `illustration_id` is
  front-face-only, `face_illustrations` retains every face — and the two
  load-bearing properties of the list (the `DOUBLE_FACED_LAYOUTS` gate,
  and `None` rather than dropping an art-less face so indices hold).
- the refresh answer issue #638 left open: the importer is a full-set,
  value-diffing upsert, and `face_illustrations` is in
  `_METADATA_SYNC_FIELDS`, so an ordinary run populates already-stored
  rows. No backfill command and no flag — running the importer is the
  backfill.
- expected coverage, so a mostly-empty column is not later read as a bug:
  1,594 of 113,224 rows (1.4%) populate on a first run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
@WilfordGrimley
WilfordGrimley merged commit 491f4ab into master Jul 30, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant