diff --git a/MPCAutofill/cardpicker/documents.py b/MPCAutofill/cardpicker/documents.py index dd540742d..946d21f73 100644 --- a/MPCAutofill/cardpicker/documents.py +++ b/MPCAutofill/cardpicker/documents.py @@ -1,3 +1,5 @@ +import logging + from django_elasticsearch_dsl import Document, fields from django_elasticsearch_dsl.registries import registry from elasticsearch_dsl import analyzer @@ -6,6 +8,8 @@ from cardpicker.models import Card +logger = logging.getLogger(__name__) + # custom elasticsearch analysers are configured here to add the `asciifolding` filter, which handles accents: # https://www.elastic.co/guide/en/elasticsearch/reference/7.17/analysis-asciifolding-tokenfilter.html # https://www.elastic.co/guide/en/elasticsearch/reference/current/analysis-standard-analyzer.html @@ -52,3 +56,19 @@ def get_queryset(self) -> QuerySet[Card]: "inferred_canonical_card__expansion", ) ) + + +def reindex_card_safely(card: Card) -> None: + """ + Pushes `card`'s current state into the Elasticsearch index, catching and logging any + failure rather than raising. Postgres is the source of truth for vote/consensus state - + by the time this runs, that write has already committed - so a search-index hiccup (ES + down, a transient connection error, etc.) must never break vote submission or roll back + a DB write that already succeeded. Shared by every vote-consensus module that needs to + push a single card's change into the index immediately, rather than waiting for the next + scheduled `update_database` re-scan or a manual `search_index --rebuild`. + """ + try: + CardSearch().update([card], action="index") + except Exception: + logger.exception("Failed to reindex card %s into Elasticsearch after a vote-consensus update", card.identifier) diff --git a/MPCAutofill/cardpicker/printing_consensus.py b/MPCAutofill/cardpicker/printing_consensus.py index 69a9e2f2d..71f3aca31 100644 --- a/MPCAutofill/cardpicker/printing_consensus.py +++ b/MPCAutofill/cardpicker/printing_consensus.py @@ -106,6 +106,19 @@ def resolve_printing(card: Card) -> CanonicalCard | Literal["NO_MATCH"] | None: return printings_by_id[winning_key] +def _effective_indexed_printing_id(status: str, printing_id: int | None) -> int | None: + """ + The printing id that actually reaches Elasticsearch for a card in `status`: `Card. + get_expansion_code`/`get_collector_number` (the fields `documents.py` indexes) only fall + back to `inferred_canonical_card` while `printing_tag_status == RESOLVED` - a card that's + UNRESOLVED or NO_MATCH is indexed as if `inferred_canonical_card` were `None`, regardless + of what's actually stored there. Comparing this derived value (not the raw fields) before + and after a consensus run is what lets a status flip with no printing change, or a + printing change with no status flip, both correctly count as "the index needs updating." + """ + return printing_id if status == PrintingTagStatus.RESOLVED else None + + def resolve_and_persist_printing(card: Card) -> CanonicalCard | Literal["NO_MATCH"] | None: """ Runs `resolve_printing(card)` and writes the outcome onto `card.inferred_canonical_card` @@ -116,7 +129,20 @@ def resolve_and_persist_printing(card: Card) -> CanonicalCard | Literal["NO_MATC vote is submitted for `card` - cheap, since it only touches this one card's own votes. Returns the same outcome `resolve_printing` returned, so callers don't need to recompute it again immediately afterwards. + + Also pushes `card` into Elasticsearch, but only when the outcome actually changes what's + indexed (see `_effective_indexed_printing_id`) - entering RESOLVED, leaving RESOLVED + (contested/unresolved again after new votes), or the resolved printing itself changing + while remaining RESOLVED. A re-resolve that lands on the same outcome as before (the + common case whenever this runs against a card that already has a settled consensus) does + not touch the index. The push itself is failure-isolated (`reindex_card_safely`) - an ES + hiccup is logged, never raised; this function's own DB write has already committed by + that point regardless. """ + prior_status = card.printing_tag_status + prior_printing_id = card.inferred_canonical_card_id + prior_effective = _effective_indexed_printing_id(prior_status, prior_printing_id) + result = resolve_printing(card) if result is None: card.inferred_canonical_card = None @@ -128,6 +154,15 @@ def resolve_and_persist_printing(card: Card) -> CanonicalCard | Literal["NO_MATC card.inferred_canonical_card = result card.printing_tag_status = PrintingTagStatus.RESOLVED card.save(update_fields=["inferred_canonical_card", "printing_tag_status"]) + + new_effective = _effective_indexed_printing_id(card.printing_tag_status, card.inferred_canonical_card_id) + if new_effective != prior_effective: + from cardpicker.documents import ( + reindex_card_safely, # local import - avoids a top-level ES dependency in this module + ) + + reindex_card_safely(card) + return result diff --git a/MPCAutofill/cardpicker/tag_consensus.py b/MPCAutofill/cardpicker/tag_consensus.py index 0f19e30fc..ccf5600a2 100644 --- a/MPCAutofill/cardpicker/tag_consensus.py +++ b/MPCAutofill/cardpicker/tag_consensus.py @@ -54,9 +54,12 @@ def resolve_and_persist_tag_votes(card: Card) -> None: rather than a single outcome), and merges the result directly into `card.tags`: a resolved APPLY adds the tag name if not already present; a resolved NOT_APPLICABLE removes it if present. Saves `card.tags` and pushes the change into Elasticsearch immediately - unlike - printing/artist consensus (whose denormalised fields aren't ES-indexed), `tags` *is* an - ES-indexed field (`documents.py`'s `KeywordField`), so a vote-triggered change has to reach - the search index directly rather than waiting for the next scheduled re-scan. + artist consensus (whose denormalised fields aren't ES-indexed), `tags` *is* an ES-indexed + field (`documents.py`'s `KeywordField`), so a vote-triggered change has to reach the + search index directly rather than waiting for the next scheduled re-scan. Only fires when + `tags_changed` is actually true, and the push itself is failure-isolated + (`reindex_card_safely`) - same rationale and mechanism as + `printing_consensus.resolve_and_persist_printing`'s equivalent hook. Also writes `card.tag_vote_statuses` (a JSONField, not ES-indexed, so no re-index needed for this part alone): for every voted tag, one of RESOLVED_APPLY/RESOLVED_REJECT/CONTESTED/ @@ -65,7 +68,7 @@ def resolve_and_persist_tag_votes(card: Card) -> None: unresolved means only one side has voted so far, or thresholds simply aren't cleared yet. """ from cardpicker.documents import ( - CardSearch, # local import - avoids a top-level ES dependency in this module + reindex_card_safely, # local import - avoids a top-level ES dependency in this module ) votes_by_tag_id: dict[int, set[int]] = defaultdict(set) @@ -107,7 +110,7 @@ def resolve_and_persist_tag_votes(card: Card) -> None: if update_fields: card.save(update_fields=update_fields) if tags_changed: - CardSearch().update([card], action="index") + reindex_card_safely(card) class TagVoteTallyEntry(TypedDict): diff --git a/MPCAutofill/cardpicker/tests/test_printing_consensus.py b/MPCAutofill/cardpicker/tests/test_printing_consensus.py index 82b2edb2f..77c3594ca 100644 --- a/MPCAutofill/cardpicker/tests/test_printing_consensus.py +++ b/MPCAutofill/cardpicker/tests/test_printing_consensus.py @@ -1,9 +1,12 @@ +from unittest.mock import patch + import pytest from cardpicker.models import PrintingTagStatus, VoteSource from cardpicker.printing_consensus import ( NO_MATCH, get_resolved_printings, + resolve_and_persist_printing, resolve_printing, ) from cardpicker.tests.factories import ( @@ -108,6 +111,88 @@ def test_ai_only_insufficient(self, db): assert resolve_printing(card) is None +class TestResolveAndPersistPrintingReindex: + """ + `resolve_and_persist_printing`'s ES side effect: reindex exactly when the outcome changes + what `documents.py` actually indexes (see `_effective_indexed_printing_id`), and never let + an ES failure take down the vote-submission DB write it rides in on. + """ + + def test_unresolved_to_resolved_fires_one_reindex_call(self, db): + card = CardFactory() + printing = CanonicalCardFactory() + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + + with patch("cardpicker.documents.reindex_card_safely") as mock_reindex: + result = resolve_and_persist_printing(card) + + assert result == printing + assert card.printing_tag_status == PrintingTagStatus.RESOLVED + mock_reindex.assert_called_once_with(card) + + def test_re_resolve_to_same_outcome_fires_zero_calls(self, db): + card = CardFactory() + printing = CanonicalCardFactory() + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + + with patch("cardpicker.documents.reindex_card_safely"): + resolve_and_persist_printing(card) # first call: UNRESOLVED -> RESOLVED, establishes the baseline + + with patch("cardpicker.documents.reindex_card_safely") as mock_reindex: + result = resolve_and_persist_printing(card) # same votes, same outcome, re-resolved + + assert result == printing + assert card.printing_tag_status == PrintingTagStatus.RESOLVED + mock_reindex.assert_not_called() + + def test_resolved_to_contested_fires_a_call_and_indexed_fields_clear(self, db): + card = CardFactory() + printing = CanonicalCardFactory() + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + + with patch("cardpicker.documents.reindex_card_safely"): + resolve_and_persist_printing(card) + assert card.printing_tag_status == PrintingTagStatus.RESOLVED + + # an equal-weight conflicting printing splits consensus (2 vs 2: share is exactly + # 0.5, below PRINTING_TAG_MIN_SHARE of 0.6 - same tie shape as test_tie_returns_none) + printing_b = CanonicalCardFactory() + CardPrintingTagFactory(card=card, printing=printing_b, source=VoteSource.USER) + CardPrintingTagFactory(card=card, printing=printing_b, source=VoteSource.USER) + + with patch("cardpicker.documents.reindex_card_safely") as mock_reindex: + result = resolve_and_persist_printing(card) + + assert result is None + assert card.printing_tag_status == PrintingTagStatus.UNRESOLVED + mock_reindex.assert_called_once_with(card) + # `card.canonical_card` is unset (no confirmed indexing match), so once RESOLVED is + # lost, the fields `documents.py` actually indexes fall all the way back to None - + # exactly what "the index needs updating" was gated on. + assert card.get_expansion_code() is None + assert card.get_collector_number() is None + + def test_es_failure_inside_reindex_does_not_block_the_db_write(self, db, caplog): + card = CardFactory() + printing = CanonicalCardFactory() + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + CardPrintingTagFactory(card=card, printing=printing, source=VoteSource.USER) + + with patch("cardpicker.documents.CardSearch") as mock_card_search: + mock_card_search.return_value.update.side_effect = Exception("ES is down") + result = resolve_and_persist_printing(card) # must not raise + + assert result == printing + assert "Failed to reindex card" in caplog.text + + card.refresh_from_db() + assert card.printing_tag_status == PrintingTagStatus.RESOLVED + assert card.inferred_canonical_card_id == printing.pk + + class TestGetResolvedPrintings: """ `get_resolved_printings` is the shared hard-gate helper consumed by both the search diff --git a/docs/features/printing-tags.md b/docs/features/printing-tags.md index 9218632a3..82c9d17c7 100644 --- a/docs/features/printing-tags.md +++ b/docs/features/printing-tags.md @@ -214,6 +214,40 @@ Orama-indexed) search gets no re-rank/filter/indicator parity — that path has no ES/DB access to consult `printing_tag_status`. Flagged explicitly, not silently built. +## Stage 3.5: immediate reindex on vote transition + +Stage 3's re-rank/filters read `printing_tag_status` and the indexed +`expansion_code`/`collector_number` (see the fallback widening above) — +but nothing pushed a changed card into ES until the next scheduled +`update_database` re-scan. A vote that just resolved (or un-resolved) a +printing was invisible to search until that next scan ran. + +`cardpicker/documents.py::reindex_card_safely(card)` — the shared, +failure-isolated push (`CardSearch().update([card], action="index")`, +exception caught and logged, never raised: Postgres is truth, ES is a +projection, so an ES hiccup must never break vote submission or roll back +a write that already committed). + +`printing_consensus.py::resolve_and_persist_printing` calls it, but only +when the _effective indexed_ printing id actually changes +(`_effective_indexed_printing_id` — the same RESOLVED-gated fallback +`get_expansion_code`/`get_collector_number` use). Covers both directions: +entering RESOLVED, leaving RESOLVED (contested/unresolved again), and the +resolved printing itself changing while staying RESOLVED. A re-resolve +landing on the same outcome (the common case for an already-settled card) +touches the DB but not the index. + +`tag_consensus.py::resolve_and_persist_tag_votes` already had an +ES push for `tags` (an ES-indexed field, unlike the printing/artist +denormalised columns) gated on its own `tags_changed` flag — switched to +`reindex_card_safely` for the same failure isolation, no change to when +it fires. + +Artist resolution (`artist_consensus.py`) never touches an ES-indexed +field (`inferred_canonical_artist`/`artist_vote_status` only, +serialise-time-only) — confirmed against `documents.py`'s field list, no +hook needed. + ## Key files - Backend: `cardpicker/printing_consensus.py`, @@ -221,7 +255,8 @@ not silently built. `cardpicker/integrations/game/mtg.py`, `cardpicker/models.py` (migration `0050_canonicalprintingmetadata_cardprintingtag_and_more.py`), `cardpicker/search/search_functions.py` (Stage 3 re-rank/filter), - `cardpicker/documents.py` (Stage 3 widened indexing) + `cardpicker/documents.py` (Stage 3 widened indexing; Stage 3.5 + `reindex_card_safely`), `cardpicker/tag_consensus.py` (Stage 3.5) - Frontend: `frontend/src/features/printingTags/` (`PrintingTagQueue.tsx`, `PrintingTagPicker.tsx`, `starburstShape.ts`, `useStickyTop`), `frontend/src/features/filters/ResolvedAttributeFilter.tsx` (Stage 3),