From 06675d3bcc849ce6b1a3163605003dc2891a8168 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 2 Sep 2026 10:34:14 -0700 Subject: [PATCH 01/22] Don't let unrecognized BISAC codes vote nonfiction Everything on the Palace Marketplace / Feedbooks category scheme is stored with type='BISAC', including codes that are not BISAC at all -- language and territory categories such as INFEN000 ("English literature") and INFENUSA ("American and Canadian literature"). Such a code cannot be resolved to a canonical BISAC heading, so classification fell back to the name the distributor supplied and hit the catch-all rule closing BISACClassifier.FICTION, which reads "not filed under a Fiction heading, therefore nonfiction". That inference is sound for a real BISAC name and unsound for anything else, so an unresolvable code cast a nonfiction vote on every work it touched -- outvoting the genuine FBFIC* fiction codes on the same book. The AUDIENCE ruleset has the same catch-all, inferring Adult; that is the shape of the bug fixed for FBJUV* codes in PP-4128. is_fiction() and audience() now skip the BISAC rulesets when the identifier did not resolve, deferring instead to the KeywordBasedClassifier call already sitting at the end of both methods (unreachable until now, because the catch-alls always matched first). The keyword classifier recognizes "literature", so the INF* family goes from voting nonfiction to voting fiction; codes carrying no signal abstain. Subjects that supply a name but no identifier are unaffected. Work.assign_genres() gains the guard its audience handling already has, so a recalculation that reaches no fiction determination keeps the status the work already had rather than writing NULL over it. This matters more now that abstaining is common. Genre and target_age are left alone deliberately: the same reasoning applies, but changing genre assignment moves books between lanes and deserves its own change. Co-Authored-By: Claude Opus 5 --- src/palace/manager/core/classifier/bisac.py | 49 ++++++++++++---- src/palace/manager/sqlalchemy/model/work.py | 6 ++ tests/manager/core/classifiers/test_bisac.py | 57 +++++++++++++++++++ .../core/classifiers/test_classifier.py | 32 +++++++++++ tests/manager/sqlalchemy/model/test_work.py | 17 ++++++ 5 files changed, 149 insertions(+), 12 deletions(-) diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index 57c19aee63..72617f550c 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -654,25 +654,50 @@ class BISACClassifier(Classifier): m(classifier.Life_Strategies, nonfiction, social_topics), ] + @classmethod + def _unrecognized_code(cls, identifier: str | None) -> bool: + """Was a subject identifier supplied that is not an official BISAC code? + + By the time a classification method runs, `scrub_identifier` has already + stripped the "FB" prefix and "N" suffix used by some distributors and + applied `NON_STANDARD_CODE_ALIASES`, so an identifier that is still + absent from `NAMES` is not a BISAC code at all. That matters because it + also means `name` is whatever fragment the distributor supplied (e.g. + "Historical", "English") rather than a canonical BISAC heading. + + A subject that carries a name but no identifier is not affected: there + the name is expected to be a real BISAC heading. + """ + return bool(identifier) and identifier not in cls.NAMES + @classmethod def is_fiction(cls, identifier, name): - for ruleset in cls.FICTION: - fiction = ruleset.match(*name) - if fiction is cls.stop: - return None - if fiction is not None: - return fiction + # The rulesets below end in a catch-all that reads "not filed under a + # Fiction heading, therefore nonfiction". That inference is sound for a + # canonical BISAC name and unsound for anything else, so an unrecognized + # code skips them and goes straight to the keyword fallback, which + # abstains when the distributor's name carries no fiction signal. + if not cls._unrecognized_code(identifier): + for ruleset in cls.FICTION: + fiction = ruleset.match(*name) + if fiction is cls.stop: + return None + if fiction is not None: + return fiction keyword = "/".join(name) return KeywordBasedClassifier.is_fiction(identifier, keyword) @classmethod def audience(cls, identifier, name): - for ruleset in cls.AUDIENCE: - audience = ruleset.match(*name) - if audience is cls.stop: - return None - if audience is not None: - return audience + # As in is_fiction: the catch-all rule infers Adult from the absence of + # a juvenile heading, which only holds for a canonical BISAC name. + if not cls._unrecognized_code(identifier): + for ruleset in cls.AUDIENCE: + audience = ruleset.match(*name) + if audience is cls.stop: + return None + if audience is not None: + return audience keyword = "/".join(name) return KeywordBasedClassifier.audience(identifier, keyword) diff --git a/src/palace/manager/sqlalchemy/model/work.py b/src/palace/manager/sqlalchemy/model/work.py index 7bf6dae00f..9bb894a5b1 100644 --- a/src/palace/manager/sqlalchemy/model/work.py +++ b/src/palace/manager/sqlalchemy/model/work.py @@ -1400,6 +1400,12 @@ def assign_genres( if self.target_age != new_target_age: self.target_age = new_target_age + # Never let a recalculation erase a known fiction status. If the + # classifier came back with no determination (e.g. every classification + # it gathered abstained), keep whatever we already had rather than + # writing NULL over a previously-determined status. + if new_fiction is None and old_fiction is not None: + new_fiction = old_fiction if new_fiction != old_fiction: self.fiction = new_fiction # Never let a recalculation erase a known audience. If the classifier diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index c7b166ac1d..fc9a61741f 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -392,6 +392,63 @@ def test_palace_marketplace_n_suffix_codes( assert subject.audience == Classifier.AUDIENCE_CHILDREN assert subject.fiction is True + @pytest.mark.parametrize( + "identifier,stored_name", + [ + pytest.param("INFEN000", None, id="no_name"), + pytest.param("INFEN000", "English", id="language_name"), + pytest.param("INFENUSA", "USA", id="territory_name"), + pytest.param("FBINFEN000", "English", id="fb_prefixed"), + pytest.param("FBZZZ000000", "Historical", id="partial_bisac_heading"), + ], + ) + def test_unrecognized_code_abstains( + self, identifier: str, stored_name: str | None + ) -> None: + """A code that is not BISAC carries no fiction or audience signal. + + Everything on the Feedbooks/Palace Marketplace category scheme arrives + typed as BISAC, including codes that describe language or territory + rather than subject matter. When such a code cannot be resolved, the + name we fall back on is a distributor fragment, so the rulesets' "not + filed under Fiction, therefore nonfiction" and "no juvenile heading, + therefore Adult" inferences do not apply. Abstaining keeps an + unresolvable code from voting against the codes that did resolve. + """ + subject = self._subject(identifier, stored_name) + assert subject.fiction is None + assert subject.audience is None + + def test_unrecognized_code_still_uses_keyword_fallback(self) -> None: + """Abstaining is not the same as ignoring the name. + + An unrecognized code skips the BISAC rulesets but still goes through + the keyword classifier, so a distributor name that does carry a signal + is honored. + """ + subject = self._subject("INFEN000", "Science Fiction") + assert subject.fiction is True + + subject = self._subject("INFEN000", "Nonfiction") + assert subject.fiction is False + + def test_recognized_code_unaffected_by_abstention(self) -> None: + """Codes that do resolve are classified exactly as before. + + These are the Palace Marketplace codes whose partial names + ("Historical", "Literary") would each vote nonfiction if the canonical + lookup were ever to miss. + """ + for identifier, stored_name in [ + ("FBFIC000000", "Fiction"), + ("FBFIC014000", "Historical"), + ("FBFIC016000", "Humorous"), + ("FBFIC019000", "Literary"), + ]: + subject = self._subject(identifier, stored_name) + assert subject.fiction is True + assert subject.audience == Classifier.AUDIENCE_ADULT + @pytest.mark.parametrize( "identifier,expected", [ diff --git a/tests/manager/core/classifiers/test_classifier.py b/tests/manager/core/classifiers/test_classifier.py index 4960e09241..4fbf97f85e 100644 --- a/tests/manager/core/classifiers/test_classifier.py +++ b/tests/manager/core/classifiers/test_classifier.py @@ -827,6 +827,38 @@ def test_fiction_status_restricts_genre( genres = data.classifier.genres(False) assert [(nonfiction_genre.genredata, 100)] == list(genres.items()) + def test_unrecognized_bisac_codes_do_not_imply_nonfiction( + self, work_classifier_fixture: TestWorkClassifierFixture + ): + """An unresolvable BISAC code must not cast a nonfiction vote. + + Everything on the Palace Marketplace category scheme arrives typed as + BISAC, including codes that describe language or territory rather than + subject matter. Each of those used to vote nonfiction, so enough of + them could outvote the genuine fiction codes on the same book. + """ + data = work_classifier_fixture + session = data.transaction.session + source = DataSource.lookup(session, DataSource.GUTENBERG, autocreate=True) + + for code, name in [("INFEN000", "English"), ("INFENUSA", "USA")]: + data.classifier.add( + data.identifier.classify(source, Subject.BISAC, code, name, weight=1) + ) + + # No vote either way, so no determination. + assert 0 == data.classifier.fiction_weights[False] + assert 0 == data.classifier.fiction_weights[True] + assert None == data.classifier.fiction() + + # A single code that does resolve is now decisive. + data.classifier.add( + data.identifier.classify( + source, Subject.BISAC, "FBFIC014000", "Historical", weight=1 + ) + ) + assert True == data.classifier.fiction() + def test_genres_consolidated_before_classification( self, work_classifier_fixture: TestWorkClassifierFixture ): diff --git a/tests/manager/sqlalchemy/model/test_work.py b/tests/manager/sqlalchemy/model/test_work.py index 96d6209c98..7aa42e659a 100644 --- a/tests/manager/sqlalchemy/model/test_work.py +++ b/tests/manager/sqlalchemy/model/test_work.py @@ -389,6 +389,23 @@ def test_assign_genres_does_not_overwrite_audience_with_null( assert work.audience == Classifier.AUDIENCE_CHILDREN + def test_assign_genres_does_not_overwrite_fiction_with_null( + self, db: DatabaseTransactionFixture + ): + # Defensive invariant: when a recalculation pass reaches no fiction + # determination (e.g. every classification it gathered abstained), a + # previously-determined fiction status must be kept rather than + # overwritten with NULL. This matters more now that an unresolvable + # BISAC code abstains instead of voting nonfiction. + work = db.work(with_license_pool=True) + work.fiction = True + db.session.commit() + + # Force the no-evidence + null-default path directly. + work.assign_genres(work._direct_identifier_ids, default_fiction=None) + + assert work.fiction is True + def test__choose_summary(self, db: DatabaseTransactionFixture): # Test the _choose_summary helper method, called by # calculate_presentation(). From 78c45442c37f553eac23e57226950773f841fa75 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 2 Sep 2026 10:34:29 -0700 Subject: [PATCH 02/22] Reclassify subjects holding a fabricated nonfiction status Repairs the stored side of the preceding fix. Subjects are global and are only re-examined when checked=false, so a subject that was scored under the old rules keeps its value indefinitely; the FBJUV* resets in 45f74fdcec18 and 05a95c828149 did not cover these codes. Resets checked=false for BISAC subjects whose identifier is not a valid BISAC code shape and which currently hold fiction=false, so classify_unchecked_subjects re-scores them and recalculates the works they are attached to. Measured against production: 2,418 subject rows, 4,877 works recalculated -- roughly 2% of the nightly volume behind the reindex surge in PP-4472, so no scheduled window is needed. Scope is deliberately narrow. Canonical BISAC codes are untouched; the great majority of those are legitimately nonfiction, and there is no evidence the FBFIC* rows are stale. Non-canonical codes already holding fiction=true or NULL are untouched as well: they are not implicated, and re-scoring them through a different classifier risks regressions while roughly doubling the reindex. This migration must ship in the same release as the classifier fix. Run on its own, the nightly task would re-score these subjects with the old rules and re-stamp checked=true, paying for a full reindex that changes nothing. Adds subject() and fetch_subject() helpers to AlembicDatabaseFixture, following the existing identifier() and data_source() helpers. Co-Authored-By: Claude Opus 5 --- ...reset_checked_for_non_bisac_nonfiction_.py | 83 +++++++++++++++++++ tests/migration/conftest.py | 39 +++++++++ ...d1bbdd4671_reset_checked_for_non_bisac_.py | 71 ++++++++++++++++ 3 files changed, 193 insertions(+) create mode 100644 alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py create mode 100644 tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py diff --git a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py new file mode 100644 index 0000000000..a3ecaca432 --- /dev/null +++ b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py @@ -0,0 +1,83 @@ +"""reset_checked_for_non_bisac_nonfiction_subjects + +Everything on the Palace Marketplace / Feedbooks category scheme is stored with +type='BISAC', including codes that are not BISAC at all -- language and +territory categories such as INFEN000 ("English literature") and INFENUSA +("American and Canadian literature"), plus vendor codes like FBSACT000000. + +Those codes cannot be resolved to a canonical BISAC heading, so classification +fell back to the distributor's name and hit the catch-all rule at the end of +BISACClassifier.FICTION, which reads "not filed under a Fiction heading, +therefore nonfiction". Each such subject was therefore stored with +fiction=False and cast a nonfiction vote on every work it was attached to -- +outvoting the genuine FBFIC* fiction codes on the same book. + +The classifier no longer applies the BISAC rulesets to an unresolvable code; it +defers to the keyword classifier instead, which recognises "literature" and +scores the INF* family as fiction. This migration resets checked=False on the +affected subjects so classify_unchecked_subjects re-scores them and recalculates +the works they are attached to. + +Scope is deliberately narrow: only non-canonical identifiers that currently hold +fiction=False. Canonical BISAC codes are untouched (the great majority of those +are legitimately nonfiction), as are non-canonical codes already holding +fiction=True or NULL, which are not implicated and whose reset would enlarge the +reindex for no benefit. + +Revision ID: 52d1bbdd4671 +Revises: de6ae4bbf4a5 +Create Date: 2026-09-02 17:26:39.209822+00:00 + +""" + +import sqlalchemy as sa +from alembic import op + +from palace.manager.util.migration.helpers import migration_logger + +# revision identifiers, used by Alembic. +revision = "52d1bbdd4671" +down_revision = "de6ae4bbf4a5" +branch_labels = None +depends_on = None + +log = migration_logger(revision) + +# An official BISAC code is three letters followed by six digits. Some +# distributors add an "FB" prefix and/or an "N" suffix, both of which +# BISACClassifier.scrub_identifier strips before looking the code up. Anything +# that does not match this shape is not a BISAC code. +CANONICAL_BISAC_CODE = r"^(FB)?[A-Z]{3}[0-9]{6}N?$" + + +def upgrade() -> None: + conn = op.get_bind() + + result = conn.execute( + sa.text( + """ + UPDATE subjects + SET checked = false + WHERE type = 'BISAC' + AND checked + AND fiction IS FALSE + AND identifier !~ :canonical_bisac_code + RETURNING id, identifier, name + """ + ), + {"canonical_bisac_code": CANONICAL_BISAC_CODE}, + ) + rows = list(result) + for row in rows: + log.info( + f"Reset checked=False for subject id={row[0]} " + f"identifier={row[1]!r} name={row[2]!r}" + ) + log.info(f"Reset checked=False for {len(rows)} non-BISAC nonfiction subjects") + + +def downgrade() -> None: + # The previous checked values are not recorded, and re-marking these + # subjects checked would only re-suppress the reclassification this + # migration exists to trigger. Intentionally a no-op. + pass diff --git a/tests/migration/conftest.py b/tests/migration/conftest.py index 036005dfdf..bf6ad14883 100644 --- a/tests/migration/conftest.py +++ b/tests/migration/conftest.py @@ -326,6 +326,45 @@ def identifier( assert isinstance(identifier_record.id, int) return identifier_record.id + def subject( + self, + subject_type: str, + identifier: str, + name: str | None = None, + fiction: bool | None = None, + checked: bool = True, + ) -> int: + """Create a subject record.""" + with self._engine.begin() as connection: + subject_record = connection.execute( + text( + """ + INSERT INTO subjects (type, identifier, name, fiction, checked, locked) + VALUES (:type, :identifier, :name, :fiction, :checked, false) + RETURNING id + """ + ), + { + "type": subject_type, + "identifier": identifier, + "name": name, + "fiction": fiction, + "checked": checked, + }, + ).fetchone() + + assert subject_record is not None + assert isinstance(subject_record.id, int) + return subject_record.id + + def fetch_subject(self, subject_id: int) -> Row: + with self._engine.begin() as connection: + result = connection.execute( + text("SELECT * FROM subjects WHERE id = :id"), + {"id": subject_id}, + ) + return result.one() + def collection( self, integration_configuration_id: int, diff --git a/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py b/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py new file mode 100644 index 0000000000..2417d1e6d5 --- /dev/null +++ b/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py @@ -0,0 +1,71 @@ +import pytest +from pytest_alembic import MigrationContext + +from tests.migration.conftest import AlembicDatabaseFixture + +REVISION = "52d1bbdd4671" + + +@pytest.mark.parametrize( + "identifier,name", + [ + pytest.param("INFEN000", "English literature", id="language_category"), + pytest.param( + "INFENUSA", "American and Canadian literature", id="territory_category" + ), + pytest.param("FBSACT000000", "News and investigations", id="vendor_code"), + pytest.param("FSHUM000000N", "Human science", id="vendor_code_with_n_suffix"), + pytest.param("SOCO32000", None, id="malformed_code_without_name"), + ], +) +def test_resets_non_canonical_nonfiction_subjects( + alembic_runner: MigrationContext, + alembic_database: AlembicDatabaseFixture, + identifier: str, + name: str | None, +) -> None: + """Codes that are not BISAC at all, holding a fabricated fiction=False, are + marked unchecked so classify_unchecked_subjects re-scores them.""" + alembic_runner.migrate_down_to(REVISION) + alembic_runner.migrate_down_one() + + subject_id = alembic_database.subject( + "BISAC", identifier, name=name, fiction=False, checked=True + ) + + alembic_runner.migrate_up_one() + + assert alembic_database.fetch_subject(subject_id).checked is False + + +@pytest.mark.parametrize( + "subject_type,identifier,fiction", + [ + pytest.param("BISAC", "FIC014000", False, id="canonical_code"), + pytest.param("BISAC", "FBFIC014000", False, id="canonical_fb_prefixed"), + pytest.param("BISAC", "FBJUV000000N", False, id="canonical_fb_and_n"), + pytest.param("BISAC", "HIS027000", False, id="canonical_real_nonfiction"), + pytest.param("BISAC", "INFEN000", True, id="non_canonical_already_fiction"), + pytest.param("BISAC", "INFEN000", None, id="non_canonical_already_null"), + pytest.param("tag", "INFEN000", False, id="not_a_bisac_subject"), + ], +) +def test_leaves_everything_else_checked( + alembic_runner: MigrationContext, + alembic_database: AlembicDatabaseFixture, + subject_type: str, + identifier: str, + fiction: bool | None, +) -> None: + """The reset is narrow: canonical BISAC codes, non-canonical codes that are + not voting nonfiction, and non-BISAC subject types are all left alone.""" + alembic_runner.migrate_down_to(REVISION) + alembic_runner.migrate_down_one() + + subject_id = alembic_database.subject( + subject_type, identifier, fiction=fiction, checked=True + ) + + alembic_runner.migrate_up_one() + + assert alembic_database.fetch_subject(subject_id).checked is True From e35ca3af99e315745aaf2f5b24b26dd7860c1cf3 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 8 Sep 2026 08:57:36 -0700 Subject: [PATCH 03/22] Treat a BISAC heading in the identifier field as a name The abstention guard asked only whether the identifier was absent from NAMES, which made a heading indistinguishable from a code that failed to resolve. Some distributors put the heading in the identifier field -- Boundless sends "FICTION / Horror" rather than "FIC015000" -- so those subjects stopped voting, and a work relying on them for its Adult audience tipped to Young Adult (TestWorkController::test_edit_classifications). A BISAC code is a single unpunctuated token, so require that shape before concluding a code failed to resolve. A value containing spaces or slashes is a name and is matched as one, which is what the rulesets expect. The offenders this change exists for are unaffected: neither INFEN000 nor INFENUSA contains punctuation. Note that requiring a digit would not work -- INFENUSA has none. Pins the behaviour with a unit test over four heading shapes rather than leaving an admin controller test as the only guard, and adds the -> None annotations CLAUDE.md asks for on the two new tests that were written to match their unannotated neighbours. Co-Authored-By: Claude Opus 5 --- src/palace/manager/core/classifier/bisac.py | 29 +++++++++++++------ tests/manager/core/classifiers/test_bisac.py | 27 +++++++++++++++++ .../core/classifiers/test_classifier.py | 2 +- tests/manager/sqlalchemy/model/test_work.py | 2 +- 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index 72617f550c..be31bd8694 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -654,21 +654,32 @@ class BISACClassifier(Classifier): m(classifier.Life_Strategies, nonfiction, social_topics), ] + # A BISAC code is a single unpunctuated token (e.g. "FIC014000"). Some + # distributors instead put the BISAC *heading* in the identifier field + # (e.g. Boundless sends "FICTION / Horror"), which is not a failed code -- + # it is a name, and is matched as one. + _CODE_SHAPED = re.compile(r"^[A-Za-z0-9]+$") + @classmethod def _unrecognized_code(cls, identifier: str | None) -> bool: - """Was a subject identifier supplied that is not an official BISAC code? + """Was a BISAC code supplied that cannot be resolved to a real one? By the time a classification method runs, `scrub_identifier` has already stripped the "FB" prefix and "N" suffix used by some distributors and - applied `NON_STANDARD_CODE_ALIASES`, so an identifier that is still - absent from `NAMES` is not a BISAC code at all. That matters because it - also means `name` is whatever fragment the distributor supplied (e.g. - "Historical", "English") rather than a canonical BISAC heading. - - A subject that carries a name but no identifier is not affected: there - the name is expected to be a real BISAC heading. + applied `NON_STANDARD_CODE_ALIASES`, so a code-shaped identifier that is + still absent from `NAMES` is not a BISAC code at all. That matters + because it also means `name` is whatever fragment the distributor + supplied (e.g. "Historical", "English literature") rather than a + canonical BISAC heading. + + Two kinds of subject are deliberately excluded, because for both of them + `name` is expected to be a real BISAC heading and is matched as one: a + subject with no identifier, and a subject whose identifier is a heading + rather than a code. """ - return bool(identifier) and identifier not in cls.NAMES + if not identifier or not cls._CODE_SHAPED.match(identifier): + return False + return identifier not in cls.NAMES @classmethod def is_fiction(cls, identifier, name): diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index fc9a61741f..255525fc0b 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -432,6 +432,33 @@ def test_unrecognized_code_still_uses_keyword_fallback(self) -> None: subject = self._subject("INFEN000", "Nonfiction") assert subject.fiction is False + @pytest.mark.parametrize( + "identifier,expected_fiction", + [ + pytest.param("FICTION / Horror", True, id="fiction_heading"), + pytest.param( + "FICTION / Science Fiction / Time Travel", True, id="deep_heading" + ), + pytest.param("HISTORY / General", False, id="nonfiction_heading"), + pytest.param( + "Antiques & Collectibles / Kitchenware", False, id="mixed_case_heading" + ), + ], + ) + def test_heading_in_identifier_field_is_matched_as_a_name( + self, identifier: str, expected_fiction: bool + ) -> None: + """Some distributors put the BISAC heading in the identifier field. + + Boundless sends e.g. "FICTION / Horror" as the subject identifier rather + than "FIC015000". That is a name, not a code that failed to resolve, so + it must still be matched against the rulesets. Regression guard for the + abstention introduced alongside it. + """ + subject = self._subject(identifier, None) + assert subject.fiction is expected_fiction + assert subject.audience == Classifier.AUDIENCE_ADULT + def test_recognized_code_unaffected_by_abstention(self) -> None: """Codes that do resolve are classified exactly as before. diff --git a/tests/manager/core/classifiers/test_classifier.py b/tests/manager/core/classifiers/test_classifier.py index 4fbf97f85e..f357bbb872 100644 --- a/tests/manager/core/classifiers/test_classifier.py +++ b/tests/manager/core/classifiers/test_classifier.py @@ -829,7 +829,7 @@ def test_fiction_status_restricts_genre( def test_unrecognized_bisac_codes_do_not_imply_nonfiction( self, work_classifier_fixture: TestWorkClassifierFixture - ): + ) -> None: """An unresolvable BISAC code must not cast a nonfiction vote. Everything on the Palace Marketplace category scheme arrives typed as diff --git a/tests/manager/sqlalchemy/model/test_work.py b/tests/manager/sqlalchemy/model/test_work.py index 7aa42e659a..a78938de4b 100644 --- a/tests/manager/sqlalchemy/model/test_work.py +++ b/tests/manager/sqlalchemy/model/test_work.py @@ -391,7 +391,7 @@ def test_assign_genres_does_not_overwrite_audience_with_null( def test_assign_genres_does_not_overwrite_fiction_with_null( self, db: DatabaseTransactionFixture - ): + ) -> None: # Defensive invariant: when a recalculation pass reaches no fiction # determination (e.g. every classification it gathered abstained), a # previously-determined fiction status must be kept rather than From 99a4e92409b2267583ac8bdfc92a7469fb22c80e Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 8 Sep 2026 08:57:47 -0700 Subject: [PATCH 04/22] Ask the classifier which subjects to reset The migration approximated "not a real BISAC code" with a pattern on the identifier, which does not agree with what the classifier considers recognizable. A shape-valid but non-existent code such as FBZZZ000000 passed the pattern and was left checked, so its fabricated fiction=False would never have been re-scored. The pattern also missed a real FIC* code stored as nonfiction, which is stale for the same reason. Select via BISACClassifier instead: reset every subject stored as nonfiction that the classifier no longer scores that way. The predicate is then the definition of the problem rather than an approximation of it, and the two cannot drift apart. Migrations here already import application code (Identifier, Timestamp, BaseCoverageRecord); a pure-logic classifier over a static table is a safer import than an ORM model. Scope is unchanged: only subjects currently holding fiction=False are examined. The count reset can now exceed the 2,418 measured against production, by however many shape-valid-but-unknown codes are stored; the migration logs what it touched. Co-Authored-By: Claude Opus 5 --- ...reset_checked_for_non_bisac_nonfiction_.py | 68 ++++++++++++------- tests/migration/conftest.py | 1 + ...d1bbdd4671_reset_checked_for_non_bisac_.py | 37 ++++++---- 3 files changed, 67 insertions(+), 39 deletions(-) diff --git a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py index a3ecaca432..ff092c3427 100644 --- a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py +++ b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py @@ -18,11 +18,17 @@ affected subjects so classify_unchecked_subjects re-scores them and recalculates the works they are attached to. -Scope is deliberately narrow: only non-canonical identifiers that currently hold -fiction=False. Canonical BISAC codes are untouched (the great majority of those -are legitimately nonfiction), as are non-canonical codes already holding -fiction=True or NULL, which are not implicated and whose reset would enlarge the -reindex for no benefit. +Rather than approximating "not a real BISAC code" with a pattern, the selection +asks BISACClassifier itself and resets every subject stored as nonfiction that +the classifier no longer scores that way. That keeps the two definitions from +drifting apart: a pattern match on the identifier would, for instance, accept a +shape-valid but non-existent code like FBZZZ000000 that the classifier rejects, +leaving its fabricated nonfiction vote in place forever. + +Scope stays narrow: only subjects currently holding fiction=False are examined, +so codes already scored as fiction or as unknown are left alone. The great +majority of the rows examined are legitimate nonfiction BISAC codes and are +untouched. Revision ID: 52d1bbdd4671 Revises: de6ae4bbf4a5 @@ -33,6 +39,7 @@ import sqlalchemy as sa from alembic import op +from palace.manager.core.classifier.bisac import BISACClassifier from palace.manager.util.migration.helpers import migration_logger # revision identifiers, used by Alembic. @@ -43,37 +50,48 @@ log = migration_logger(revision) -# An official BISAC code is three letters followed by six digits. Some -# distributors add an "FB" prefix and/or an "N" suffix, both of which -# BISACClassifier.scrub_identifier strips before looking the code up. Anything -# that does not match this shape is not a BISAC code. -CANONICAL_BISAC_CODE = r"^(FB)?[A-Z]{3}[0-9]{6}N?$" - def upgrade() -> None: conn = op.get_bind() - result = conn.execute( + candidates = conn.execute( sa.text( """ - UPDATE subjects - SET checked = false + SELECT id, identifier, name + FROM subjects WHERE type = 'BISAC' AND checked AND fiction IS FALSE - AND identifier !~ :canonical_bisac_code - RETURNING id, identifier, name """ - ), - {"canonical_bisac_code": CANONICAL_BISAC_CODE}, - ) - rows = list(result) - for row in rows: - log.info( - f"Reset checked=False for subject id={row[0]} " - f"identifier={row[1]!r} name={row[2]!r}" ) - log.info(f"Reset checked=False for {len(rows)} non-BISAC nonfiction subjects") + ).all() + + stale_ids = [] + for row in candidates: + if not row.identifier and not row.name: + # Nothing to classify. Subject.lookup will not create such a row, + # but the columns are nullable, so don't assume. + continue + identifier, name = BISACClassifier.scrub_identifier_and_name( + row.identifier, row.name + ) + if BISACClassifier.is_fiction(identifier, name) is not False: + stale_ids.append(row.id) + log.info( + f"Reset checked=False for subject id={row.id} " + f"identifier={row.identifier!r} name={row.name!r}" + ) + + if stale_ids: + conn.execute( + sa.text("UPDATE subjects SET checked = false WHERE id = ANY(:ids)"), + {"ids": stale_ids}, + ) + + log.info( + f"Reset checked=False for {len(stale_ids)} of {len(candidates)} " + f"BISAC subjects stored as nonfiction" + ) def downgrade() -> None: diff --git a/tests/migration/conftest.py b/tests/migration/conftest.py index bf6ad14883..cbb00d7a1b 100644 --- a/tests/migration/conftest.py +++ b/tests/migration/conftest.py @@ -358,6 +358,7 @@ def subject( return subject_record.id def fetch_subject(self, subject_id: int) -> Row: + """Read back a subject record.""" with self._engine.begin() as connection: result = connection.execute( text("SELECT * FROM subjects WHERE id = :id"), diff --git a/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py b/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py index 2417d1e6d5..4852440b0f 100644 --- a/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py +++ b/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py @@ -16,16 +16,22 @@ pytest.param("FBSACT000000", "News and investigations", id="vendor_code"), pytest.param("FSHUM000000N", "Human science", id="vendor_code_with_n_suffix"), pytest.param("SOCO32000", None, id="malformed_code_without_name"), + pytest.param("FBZZZ000000", "Historical", id="shape_valid_but_nonexistent"), + pytest.param("FBFIC014000", "Historical", id="stale_canonical_fiction_code"), ], ) -def test_resets_non_canonical_nonfiction_subjects( +def test_resets_subjects_no_longer_scored_as_nonfiction( alembic_runner: MigrationContext, alembic_database: AlembicDatabaseFixture, identifier: str, name: str | None, ) -> None: - """Codes that are not BISAC at all, holding a fabricated fiction=False, are - marked unchecked so classify_unchecked_subjects re-scores them.""" + """Subjects stored as nonfiction that the classifier no longer scores that + way are marked unchecked, so classify_unchecked_subjects re-scores them. + + Covers both codes that are not BISAC at all and codes that merely look like + one (FBZZZ000000), which a pattern-based predicate would wrongly accept. + """ alembic_runner.migrate_down_to(REVISION) alembic_runner.migrate_down_one() @@ -39,15 +45,16 @@ def test_resets_non_canonical_nonfiction_subjects( @pytest.mark.parametrize( - "subject_type,identifier,fiction", + "subject_type,identifier,name,fiction", [ - pytest.param("BISAC", "FIC014000", False, id="canonical_code"), - pytest.param("BISAC", "FBFIC014000", False, id="canonical_fb_prefixed"), - pytest.param("BISAC", "FBJUV000000N", False, id="canonical_fb_and_n"), - pytest.param("BISAC", "HIS027000", False, id="canonical_real_nonfiction"), - pytest.param("BISAC", "INFEN000", True, id="non_canonical_already_fiction"), - pytest.param("BISAC", "INFEN000", None, id="non_canonical_already_null"), - pytest.param("tag", "INFEN000", False, id="not_a_bisac_subject"), + pytest.param("BISAC", "HIS027000", None, False, id="real_nonfiction_code"), + pytest.param("BISAC", "HIS000000", None, False, id="real_nonfiction_general"), + pytest.param( + "BISAC", "HISTORY / General", None, False, id="nonfiction_heading" + ), + pytest.param("BISAC", "INFEN000", None, True, id="already_scored_fiction"), + pytest.param("BISAC", "INFEN000", None, None, id="already_scored_unknown"), + pytest.param("tag", "INFEN000", None, False, id="not_a_bisac_subject"), ], ) def test_leaves_everything_else_checked( @@ -55,15 +62,17 @@ def test_leaves_everything_else_checked( alembic_database: AlembicDatabaseFixture, subject_type: str, identifier: str, + name: str | None, fiction: bool | None, ) -> None: - """The reset is narrow: canonical BISAC codes, non-canonical codes that are - not voting nonfiction, and non-BISAC subject types are all left alone.""" + """Legitimate nonfiction codes keep their value, and subjects outside the + examined set -- already scored as fiction or unknown, or not BISAC-typed -- + are not touched.""" alembic_runner.migrate_down_to(REVISION) alembic_runner.migrate_down_one() subject_id = alembic_database.subject( - subject_type, identifier, fiction=fiction, checked=True + subject_type, identifier, name=name, fiction=fiction, checked=True ) alembic_runner.migrate_up_one() From 2925881c4df60620d34bbd7510aecfbe4b49980a Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 8 Sep 2026 09:06:27 -0700 Subject: [PATCH 05/22] Rebase migration onto the current alembic head 912c566f3383 (#3694) merged while this PR was open, taking the same down_revision this migration had. Two heads meant every migration test failed with "Multiple heads are present; please specify a single target revision" on all three Python versions. Re-points down_revision at 912c566f3383, restoring a single head. Co-Authored-By: Claude Opus 5 --- ...02_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py index ff092c3427..a7ad804ea5 100644 --- a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py +++ b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py @@ -31,7 +31,7 @@ untouched. Revision ID: 52d1bbdd4671 -Revises: de6ae4bbf4a5 +Revises: 912c566f3383 Create Date: 2026-09-02 17:26:39.209822+00:00 """ @@ -44,7 +44,7 @@ # revision identifiers, used by Alembic. revision = "52d1bbdd4671" -down_revision = "de6ae4bbf4a5" +down_revision = "912c566f3383" branch_labels = None depends_on = None From a0e3467731c5e88cf5699c626bb79f712750c8d9 Mon Sep 17 00:00:00 2001 From: dbernstein Date: Mon, 14 Sep 2026 16:42:46 +0200 Subject: [PATCH 06/22] Update tests/manager/core/classifiers/test_classifier.py Co-authored-by: Tim DiLauro --- tests/manager/core/classifiers/test_classifier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/manager/core/classifiers/test_classifier.py b/tests/manager/core/classifiers/test_classifier.py index f357bbb872..6967451ca7 100644 --- a/tests/manager/core/classifiers/test_classifier.py +++ b/tests/manager/core/classifiers/test_classifier.py @@ -849,7 +849,7 @@ def test_unrecognized_bisac_codes_do_not_imply_nonfiction( # No vote either way, so no determination. assert 0 == data.classifier.fiction_weights[False] assert 0 == data.classifier.fiction_weights[True] - assert None == data.classifier.fiction() + assert data.classifier.fiction() is None # A single code that does resolve is now decisive. data.classifier.add( From 412dcc32c7f90c580765a1ae9fe0e21e22a45f79 Mon Sep 17 00:00:00 2001 From: dbernstein Date: Mon, 14 Sep 2026 16:43:27 +0200 Subject: [PATCH 07/22] Update tests/manager/core/classifiers/test_classifier.py Co-authored-by: Tim DiLauro --- tests/manager/core/classifiers/test_classifier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/manager/core/classifiers/test_classifier.py b/tests/manager/core/classifiers/test_classifier.py index 6967451ca7..3973a6b212 100644 --- a/tests/manager/core/classifiers/test_classifier.py +++ b/tests/manager/core/classifiers/test_classifier.py @@ -857,7 +857,7 @@ def test_unrecognized_bisac_codes_do_not_imply_nonfiction( source, Subject.BISAC, "FBFIC014000", "Historical", weight=1 ) ) - assert True == data.classifier.fiction() + assert data.classifier.fiction() is True def test_genres_consolidated_before_classification( self, work_classifier_fixture: TestWorkClassifierFixture From 0f14c0342fe55ed92318fceb8eca76c7cb7ed4a2 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 08:03:14 -0700 Subject: [PATCH 08/22] Describe the migration in its docstring, not the classifier Review feedback: most of the docstring explained how the classifier works and what used to be wrong with it, rather than what this migration does to the database. Trimmed to what a reader of this file needs -- what it changes, which rows it examines, and the constraint that it must ship in the same release as the classifier change. That last point was only in the commit message before, and it is the one thing that will bite someone running this out of order. The rationale for asking the classifier instead of pattern-matching the identifier moves to an inline comment at the selection loop, which is where a reader hits the question. The history it replaced is preserved in the commit and PR. Co-Authored-By: Claude Opus 5 --- ...reset_checked_for_non_bisac_nonfiction_.py | 53 +++++++++---------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py index a7ad804ea5..d5345047d5 100644 --- a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py +++ b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py @@ -1,34 +1,25 @@ """reset_checked_for_non_bisac_nonfiction_subjects -Everything on the Palace Marketplace / Feedbooks category scheme is stored with -type='BISAC', including codes that are not BISAC at all -- language and -territory categories such as INFEN000 ("English literature") and INFENUSA -("American and Canadian literature"), plus vendor codes like FBSACT000000. - -Those codes cannot be resolved to a canonical BISAC heading, so classification -fell back to the distributor's name and hit the catch-all rule at the end of -BISACClassifier.FICTION, which reads "not filed under a Fiction heading, -therefore nonfiction". Each such subject was therefore stored with -fiction=False and cast a nonfiction vote on every work it was attached to -- -outvoting the genuine FBFIC* fiction codes on the same book. - -The classifier no longer applies the BISAC rulesets to an unresolvable code; it -defers to the keyword classifier instead, which recognises "literature" and -scores the INF* family as fiction. This migration resets checked=False on the -affected subjects so classify_unchecked_subjects re-scores them and recalculates -the works they are attached to. - -Rather than approximating "not a real BISAC code" with a pattern, the selection -asks BISACClassifier itself and resets every subject stored as nonfiction that -the classifier no longer scores that way. That keeps the two definitions from -drifting apart: a pattern match on the identifier would, for instance, accept a -shape-valid but non-existent code like FBZZZ000000 that the classifier rejects, -leaving its fabricated nonfiction vote in place forever. - -Scope stays narrow: only subjects currently holding fiction=False are examined, -so codes already scored as fiction or as unknown are left alone. The great -majority of the rows examined are legitimate nonfiction BISAC codes and are -untouched. +Resets checked=False on BISAC subjects whose stored fiction=False no longer +matches what BISACClassifier scores them as, so classify_unchecked_subjects +re-scores them and recalculates the works they are attached to. + +These are subjects whose identifier is not a resolvable BISAC code. The Palace +Marketplace / Feedbooks scheme is stored as type='BISAC' but includes +non-subject codes such as INFEN000 ("English literature"), and classification +used to infer nonfiction from the distributor's name for those. It no longer +does, which leaves the stored values stale. + +Selection asks the classifier rather than pattern-matching the identifier, so +the migration and the runtime cannot disagree about which codes are real. + +Only subjects currently holding fiction=False are examined; most of those are +legitimate nonfiction BISAC codes and are left untouched. + +Must ship in the same release as the classifier change. Run against the old +code, classify_unchecked_subjects would re-score these subjects under the old +rules and re-stamp checked=True, paying for a full reindex that changes +nothing. Revision ID: 52d1bbdd4671 Revises: 912c566f3383 @@ -66,6 +57,10 @@ def upgrade() -> None: ) ).all() + # Ask the classifier rather than matching a pattern against the identifier. + # A pattern would accept a shape-valid but non-existent code such as + # FBZZZ000000, which the classifier rejects, and its fabricated nonfiction + # value would survive this repair. stale_ids = [] for row in candidates: if not row.identifier and not row.name: From ce7e23618961ab25647ff4e541d288cb74f09c9c Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 08:30:11 -0700 Subject: [PATCH 09/22] Share one ruleset walk between is_fiction and audience Review feedback: after the abstention guard landed, the two methods were identical apart from the ruleset and the keyword fallback. Extracts _apply_rulesets, parameterized on both, so the guard exists in one place and cannot drift between them. A TypeVar keeps the callers' return types intact -- bool | None for is_fiction, str | None for audience -- rather than widening to Any. genre and target_age keep their own loops. Not an oversight, and the docstring says so: GENRE has no catch-all but does have rules that match a bare fragment, so an unrecognized code named "Historical" is still a Historical Fiction signal; and TARGET_AGE keys off a juvenile first token, so "Juvenile Fiction / Early Readers" on an unrecognized code yields (5, 7) from the rulesets where the keyword classifier yields nothing. Applying the guard to either would lose information. Behaviour is unchanged: verified against resolvable and unresolvable codes, a juvenile heading on an unresolvable code, a heading in the identifier field, and name-only subjects hitting both a stop rule and the nonfiction catch-all. Co-Authored-By: Claude Opus 5 --- src/palace/manager/core/classifier/bisac.py | 70 ++++++++++++++------- 1 file changed, 46 insertions(+), 24 deletions(-) diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index be31bd8694..9d7753cc47 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -1,5 +1,7 @@ import csv import re +from collections.abc import Callable, Sequence +from typing import TypeVar from frozendict import frozendict @@ -239,6 +241,10 @@ def m(result, *ruleset): return MatchingRule(result, *ruleset) +# The value a ruleset yields: bool for fiction status, str for audience, etc. +RulesetResult = TypeVar("RulesetResult") + + class BISACClassifier(Classifier): """Handle real, genuine, according-to-Hoyle BISAC classifications. @@ -682,35 +688,51 @@ def _unrecognized_code(cls, identifier: str | None) -> bool: return identifier not in cls.NAMES @classmethod - def is_fiction(cls, identifier, name): - # The rulesets below end in a catch-all that reads "not filed under a - # Fiction heading, therefore nonfiction". That inference is sound for a - # canonical BISAC name and unsound for anything else, so an unrecognized - # code skips them and goes straight to the keyword fallback, which - # abstains when the distributor's name carries no fiction signal. + def _apply_rulesets( + cls, + identifier: str | None, + name: list[str], + rulesets: Sequence[MatchingRule], + keyword_fallback: Callable[[str | None, str], RulesetResult | None], + ) -> RulesetResult | None: + """Match `name` against `rulesets`, falling back to keyword matching. + + Both the FICTION and AUDIENCE rulesets end in a catch-all that reasons + from the top-level BISAC heading -- "not filed under Fiction, therefore + nonfiction", "no juvenile heading, therefore Adult". That inference + holds for a canonical BISAC name and for nothing else, so an + unrecognized code skips the rulesets entirely and is left to the keyword + classifier, which abstains when the distributor's name carries no + signal. + + Only those two callers share this. `genre` must not skip its rulesets -- + GENRE has no catch-all but does have rules that match a bare fragment, + so an unrecognized code named "Historical" is still a Historical Fiction + signal. Neither must `target_age`: its rules key off a juvenile first + token, and a name like "Juvenile Fiction / Early Readers" on an + unrecognized code yields (5, 7) from the rulesets where the keyword + classifier yields nothing. + """ if not cls._unrecognized_code(identifier): - for ruleset in cls.FICTION: - fiction = ruleset.match(*name) - if fiction is cls.stop: + for ruleset in rulesets: + result = ruleset.match(*name) + if result is cls.stop: return None - if fiction is not None: - return fiction - keyword = "/".join(name) - return KeywordBasedClassifier.is_fiction(identifier, keyword) + if result is not None: + return result + return keyword_fallback(identifier, "/".join(name)) + + @classmethod + def is_fiction(cls, identifier, name): + return cls._apply_rulesets( + identifier, name, cls.FICTION, KeywordBasedClassifier.is_fiction + ) @classmethod def audience(cls, identifier, name): - # As in is_fiction: the catch-all rule infers Adult from the absence of - # a juvenile heading, which only holds for a canonical BISAC name. - if not cls._unrecognized_code(identifier): - for ruleset in cls.AUDIENCE: - audience = ruleset.match(*name) - if audience is cls.stop: - return None - if audience is not None: - return audience - keyword = "/".join(name) - return KeywordBasedClassifier.audience(identifier, keyword) + return cls._apply_rulesets( + identifier, name, cls.AUDIENCE, KeywordBasedClassifier.audience + ) @classmethod def target_age(cls, identifier, name): From 9cef011205d136e062a5c69eb26ca7dc38b586cd Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 08:35:34 -0700 Subject: [PATCH 10/22] Name the keep-known-value rule instead of describing it twice Review feedback: the fiction guard added by this branch does the same thing as the audience guard below it, each explained by its own four-line comment saying the same thing. Extracts Work._keep_known_value(old, new). The name carries what the comments were carrying, so both delete: 18 lines become 6. The != guards stay rather than moving into the helper. Assigning an equal value still marks the instance dirty in SQLAlchemy and can emit a pointless UPDATE, and a helper that assigned would need setattr by attribute name, which is worse than what it replaces. target_age is left alone. It needs the tuple_to_numericrange conversion first, and it has no keep-known step at all -- a work losing its target age is legitimate in a way that losing its fiction status is not. Also switches the TypeVar added in the previous commit to PEP 695 syntax, which is what the rest of the codebase uses for generic functions (51 uses against 9 module-level TypeVars, and those are mostly class-level generics where PEP 695 does not apply). Co-Authored-By: Claude Opus 5 --- src/palace/manager/core/classifier/bisac.py | 7 +----- src/palace/manager/sqlalchemy/model/work.py | 24 ++++++++++----------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index 9d7753cc47..26bc362e72 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -1,7 +1,6 @@ import csv import re from collections.abc import Callable, Sequence -from typing import TypeVar from frozendict import frozendict @@ -241,10 +240,6 @@ def m(result, *ruleset): return MatchingRule(result, *ruleset) -# The value a ruleset yields: bool for fiction status, str for audience, etc. -RulesetResult = TypeVar("RulesetResult") - - class BISACClassifier(Classifier): """Handle real, genuine, according-to-Hoyle BISAC classifications. @@ -688,7 +683,7 @@ def _unrecognized_code(cls, identifier: str | None) -> bool: return identifier not in cls.NAMES @classmethod - def _apply_rulesets( + def _apply_rulesets[RulesetResult]( cls, identifier: str | None, name: list[str], diff --git a/src/palace/manager/sqlalchemy/model/work.py b/src/palace/manager/sqlalchemy/model/work.py index 9bb894a5b1..65a2351820 100644 --- a/src/palace/manager/sqlalchemy/model/work.py +++ b/src/palace/manager/sqlalchemy/model/work.py @@ -1368,6 +1368,15 @@ def calculate_quality(self, identifier_ids, default_quality=0): if new_quality != self.quality: self.quality = new_quality + @staticmethod + def _keep_known_value[T](old_value: T | None, new_value: T | None) -> T | None: + """Prefer `new_value`, but never replace a known value with None. + + A recalculation that gathered no usable classifications should leave an + existing determination alone rather than erasing it. + """ + return old_value if new_value is None else new_value + def assign_genres( self, identifier_ids, @@ -1400,20 +1409,11 @@ def assign_genres( if self.target_age != new_target_age: self.target_age = new_target_age - # Never let a recalculation erase a known fiction status. If the - # classifier came back with no determination (e.g. every classification - # it gathered abstained), keep whatever we already had rather than - # writing NULL over a previously-determined status. - if new_fiction is None and old_fiction is not None: - new_fiction = old_fiction + new_fiction = self._keep_known_value(old_fiction, new_fiction) if new_fiction != old_fiction: self.fiction = new_fiction - # Never let a recalculation erase a known audience. If the classifier - # came back with no audience (e.g. it gathered no usable - # classifications on this pass), keep whatever we already had rather - # than writing NULL over a previously-determined audience. - if new_audience is None and old_audience is not None: - new_audience = old_audience + + new_audience = self._keep_known_value(old_audience, new_audience) if new_audience != old_audience: self.audience = new_audience From 4146fdd6ceb687dea920516eb7d7fbbcd024cab4 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 08:41:07 -0700 Subject: [PATCH 11/22] Parameterize the keyword-fallback test Review feedback: the test stacked two independent cases in one body, so a failure in the first hid the second and the output did not say which signal broke. Each case is now collected separately, named for what it checks: [fiction_signal] and [nonfiction_signal]. Co-Authored-By: Claude Opus 5 --- tests/manager/core/classifiers/test_bisac.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index 255525fc0b..f007358ee0 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -419,18 +419,24 @@ def test_unrecognized_code_abstains( assert subject.fiction is None assert subject.audience is None - def test_unrecognized_code_still_uses_keyword_fallback(self) -> None: + @pytest.mark.parametrize( + "stored_name,expected_fiction", + [ + pytest.param("Science Fiction", True, id="fiction_signal"), + pytest.param("Nonfiction", False, id="nonfiction_signal"), + ], + ) + def test_unrecognized_code_still_uses_keyword_fallback( + self, stored_name: str, expected_fiction: bool + ) -> None: """Abstaining is not the same as ignoring the name. An unrecognized code skips the BISAC rulesets but still goes through the keyword classifier, so a distributor name that does carry a signal is honored. """ - subject = self._subject("INFEN000", "Science Fiction") - assert subject.fiction is True - - subject = self._subject("INFEN000", "Nonfiction") - assert subject.fiction is False + subject = self._subject("INFEN000", stored_name) + assert subject.fiction is expected_fiction @pytest.mark.parametrize( "identifier,expected_fiction", From 55bf453a16b1a379853647d2daf164352295506f Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 08:45:49 -0700 Subject: [PATCH 12/22] Parameterize the recognized-codes test Review feedback: the test looped over four codes, so a failure reported only the assert line with no indication of which code it was on, and one failing case skipped the rest. Each code is now collected separately, named for its partial name rather than its code: [fiction_general], [historical], [humorous], [literary]. Both asserts stay in the body. They are two facets of one case -- this code still classifies correctly -- rather than independent cases. Co-Authored-By: Claude Opus 5 --- tests/manager/core/classifiers/test_bisac.py | 25 ++++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index f007358ee0..f89b0a60ef 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -465,22 +465,27 @@ def test_heading_in_identifier_field_is_matched_as_a_name( assert subject.fiction is expected_fiction assert subject.audience == Classifier.AUDIENCE_ADULT - def test_recognized_code_unaffected_by_abstention(self) -> None: + @pytest.mark.parametrize( + "identifier,stored_name", + [ + pytest.param("FBFIC000000", "Fiction", id="fiction_general"), + pytest.param("FBFIC014000", "Historical", id="historical"), + pytest.param("FBFIC016000", "Humorous", id="humorous"), + pytest.param("FBFIC019000", "Literary", id="literary"), + ], + ) + def test_recognized_code_unaffected_by_abstention( + self, identifier: str, stored_name: str + ) -> None: """Codes that do resolve are classified exactly as before. These are the Palace Marketplace codes whose partial names ("Historical", "Literary") would each vote nonfiction if the canonical lookup were ever to miss. """ - for identifier, stored_name in [ - ("FBFIC000000", "Fiction"), - ("FBFIC014000", "Historical"), - ("FBFIC016000", "Humorous"), - ("FBFIC019000", "Literary"), - ]: - subject = self._subject(identifier, stored_name) - assert subject.fiction is True - assert subject.audience == Classifier.AUDIENCE_ADULT + subject = self._subject(identifier, stored_name) + assert subject.fiction is True + assert subject.audience == Classifier.AUDIENCE_ADULT @pytest.mark.parametrize( "identifier,expected", From 0be2ca9d6870eecaf591d8c6709df7f580fecfe1 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 08:49:42 -0700 Subject: [PATCH 13/22] Cover audience when classification falls back to keywords Review feedback: nothing pinned audience handling for an unrecognized code. The guard skips the AUDIENCE rulesets for those, so the keyword classifier is what recovers a juvenile or YA audience, and that was untested. Adds the two cases from review -- "Juvenile Fiction" -> Children and "Young Adult Fiction" -> Young Adult -- both of which already pass. No source change. The two existing cases gain an explicit audience of None. That is not padding: before this branch they returned Adult from the rulesets' catch-all, so asserting None pins the abstention. The four cases now document both halves of the contract -- honour an audience signal when the name carries one, abstain when it does not. Co-Authored-By: Claude Opus 5 --- tests/manager/core/classifiers/test_bisac.py | 22 +++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index f89b0a60ef..2c471a9cf7 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -420,23 +420,35 @@ def test_unrecognized_code_abstains( assert subject.audience is None @pytest.mark.parametrize( - "stored_name,expected_fiction", + "stored_name,expected_fiction,expected_audience", [ - pytest.param("Science Fiction", True, id="fiction_signal"), - pytest.param("Nonfiction", False, id="nonfiction_signal"), + pytest.param("Science Fiction", True, None, id="fiction_signal"), + pytest.param("Nonfiction", False, None, id="nonfiction_signal"), + pytest.param( + "Juvenile Fiction", True, Classifier.AUDIENCE_CHILDREN, id="juvenile" + ), + pytest.param( + "Young Adult Fiction", + True, + Classifier.AUDIENCE_YOUNG_ADULT, + id="young_adult", + ), ], ) def test_unrecognized_code_still_uses_keyword_fallback( - self, stored_name: str, expected_fiction: bool + self, stored_name: str, expected_fiction: bool, expected_audience: str | None ) -> None: """Abstaining is not the same as ignoring the name. An unrecognized code skips the BISAC rulesets but still goes through the keyword classifier, so a distributor name that does carry a signal - is honored. + is honored -- for audience as well as fiction status. A juvenile or YA + heading keeps its audience; a name with no audience signal abstains + rather than falling to the rulesets' Adult catch-all. """ subject = self._subject("INFEN000", stored_name) assert subject.fiction is expected_fiction + assert subject.audience == expected_audience @pytest.mark.parametrize( "identifier,expected_fiction", From e367b9a482650053e755e3d70a4e9b620f9d910e Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 09:30:55 -0700 Subject: [PATCH 14/22] Decide by the heading, not the shape of the identifier Review feedback, two related comments: the comment above _CODE_SHAPED claimed a BISAC code is a single unpunctuated token, which is not what a BISAC code is -- it is three letters and six digits, and the regex was far more permissive. And that permissiveness had a consequence: a single-word heading matched it, so a subject identified as "Juvenile" (JUV037020) was treated as a code that failed to resolve. The heuristic was asking the wrong question. The FICTION and AUDIENCE catch-alls reason from the top-level heading, so what has to be checked is whether the name actually starts with one -- not whether the identifier resembles a code. Replaces the regex and _unrecognized_code with TOP_LEVEL_HEADINGS and _has_canonical_heading. The rule is: when a subject carries an identifier, the rulesets run only if its name begins with a real BISAC top-level heading. Two groups of subject move: - Bare top-level headings in the identifier field. 29 of the 56 top-level headings are a single unpunctuated word -- Fiction, History, Humor, Poetry, Travel -- so any of them arriving there hit the old regex. This is the defect that broke test_edit_classifications, in its single-word form. - Unresolvable codes named "Juvenile Fiction" or "Young Adult Fiction", which now reach the rulesets instead of the keyword classifier and get the same answer from both. Nothing observable changes. Subjects with no identifier are explicitly exempt. They had no code that could fail to resolve, and some distributors classify entirely this way: Bibliotheca sends every genre as a bare name ("Action & Adventure", "Magic") with no code, so gating those would leave its titles with no fiction evidence at all and a NULL fiction status on first import. Whether a bare sub-heading should carry the rulesets' top-level inference is a real question, but a distributor-wide one that is not this change's to answer. scrub_identifier now strips the "N" suffix only when the stripped form resolves. It stripped unconditionally, which was harmless while the identifier field was assumed to hold a code, and wrong once a heading could arrive there: "FICTION" became "FICTIO" and stopped being recognized, as did RELIGION, EDUCATION, DESIGN and TRANSPORTATION. Uppercase is the casing Boundless sends. TOP_LEVEL_HEADINGS is derived from bisac.csv plus five former spellings of renamed categories the file no longer carries. That union is load-bearing, not defensive: without it "Psychology & Psychiatry / Foo" would start abstaining, because the Interchangeable tokens in the rulesets know that spelling and the CSV does not. Also corrects the _apply_rulesets docstring, which justified excluding target_age in terms of the old identifier-based guard. Under a name-based guard that reasoning no longer holds -- target_age is now out for scope rather than correctness. genre still must stay out. Co-Authored-By: Claude Opus 5 --- src/palace/manager/core/classifier/bisac.py | 87 ++++++++++++-------- tests/manager/core/classifiers/test_bisac.py | 87 +++++++++++++++++++- 2 files changed, 136 insertions(+), 38 deletions(-) diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index 26bc362e72..ee09d0f6e2 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -655,32 +655,40 @@ class BISACClassifier(Classifier): m(classifier.Life_Strategies, nonfiction, social_topics), ] - # A BISAC code is a single unpunctuated token (e.g. "FIC014000"). Some - # distributors instead put the BISAC *heading* in the identifier field - # (e.g. Boundless sends "FICTION / Horror"), which is not a failed code -- - # it is a name, and is matched as one. - _CODE_SHAPED = re.compile(r"^[A-Za-z0-9]+$") + # The top-level headings a canonical BISAC name can begin with ("Fiction", + # "Juvenile Nonfiction", "Antiques & Collectibles", ...). + TOP_LEVEL_HEADINGS: frozenset[str] = frozenset( + Lowercased(name.split("/")[0].strip()) for name in NAMES.values() + ) | frozenset( + # Renamed categories whose former top-level spelling is no longer in + # bisac.csv but which distributors still send. The Interchangeable + # tokens above let the rulesets match these, so this set has to agree. + Lowercased(name) + for name in ( + "Mind & Spirit", + "Psychology & Psychiatry", + "Technology", + "Foreign Language Study", + "Literary Criticism & Collections", + ) + ) @classmethod - def _unrecognized_code(cls, identifier: str | None) -> bool: - """Was a BISAC code supplied that cannot be resolved to a real one? - - By the time a classification method runs, `scrub_identifier` has already - stripped the "FB" prefix and "N" suffix used by some distributors and - applied `NON_STANDARD_CODE_ALIASES`, so a code-shaped identifier that is - still absent from `NAMES` is not a BISAC code at all. That matters - because it also means `name` is whatever fragment the distributor - supplied (e.g. "Historical", "English literature") rather than a - canonical BISAC heading. - - Two kinds of subject are deliberately excluded, because for both of them - `name` is expected to be a real BISAC heading and is matched as one: a - subject with no identifier, and a subject whose identifier is a heading - rather than a code. + def _has_canonical_heading(cls, name: list[str]) -> bool: + """Does `name` begin with a real BISAC top-level heading? + + This is the premise the FICTION and AUDIENCE catch-all rules rest on, + so it is what has to be checked before running them. It holds for a + name that came from `NAMES`, and for one a distributor supplied in + either the name or the identifier field -- Boundless sends headings + like "FICTION / Horror" as the identifier, and a bare top-level + heading such as "Juvenile" is equally a heading. + + It does not hold for the fragment left over when a code cannot be + resolved ("Historical", "English literature"), which is the case these + rulesets must not be applied to. """ - if not identifier or not cls._CODE_SHAPED.match(identifier): - return False - return identifier not in cls.NAMES + return bool(name) and name[0] in cls.TOP_LEVEL_HEADINGS @classmethod def _apply_rulesets[RulesetResult]( @@ -700,15 +708,22 @@ def _apply_rulesets[RulesetResult]( classifier, which abstains when the distributor's name carries no signal. - Only those two callers share this. `genre` must not skip its rulesets -- + Only those two callers share this. `genre` must not skip its rulesets: GENRE has no catch-all but does have rules that match a bare fragment, - so an unrecognized code named "Historical" is still a Historical Fiction - signal. Neither must `target_age`: its rules key off a juvenile first - token, and a name like "Juvenile Fiction / Early Readers" on an - unrecognized code yields (5, 7) from the rulesets where the keyword - classifier yields nothing. + so a subject named "Historical" is still a Historical Fiction signal + even though "Historical" is not a top-level heading. `target_age` is + left out for scope rather than correctness -- its rules key off a + juvenile first token, which only a real heading produces, so routing it + through here would be safe. """ - if not cls._unrecognized_code(identifier): + # A subject with no identifier had no code that could fail to resolve, + # so there is nothing here to protect it from -- and some distributors + # classify entirely this way. Bibliotheca sends every genre as a bare + # name ("Action & Adventure", "Magic") with no code at all; gating + # those on the heading would leave its titles with no fiction evidence + # whatsoever. Whether a bare sub-heading should carry the rulesets' + # top-level inference is a real question, but a separate one. + if not identifier or cls._has_canonical_heading(name): for ruleset in rulesets: result = ruleset.match(*name) if result is cls.stop: @@ -778,10 +793,14 @@ def scrub_identifier(cls, identifier): identifier = identifier.removeprefix("FB") # Some distributors (e.g. Palace Marketplace) append an "N" suffix to # standard BISAC codes (e.g. "FBJUV000000N" becomes "JUV000000N" after - # FB-stripping). Official BISAC codes always end with digits, so a - # trailing "N" is always a non-standard extension; strip it so the code - # resolves to its canonical entry. - identifier = identifier.removesuffix("N") + # FB-stripping). Strip it only when doing so produces a code we know, + # because the identifier field does not always hold a code: a heading + # can arrive there too, and stripping unconditionally would turn + # "FICTION" into "FICTIO" (likewise RELIGION, EDUCATION, DESIGN, + # TRANSPORTATION) and stop it being recognized as a heading. + stripped = identifier.removesuffix("N") + if stripped in cls.NAMES or stripped in cls.NON_STANDARD_CODE_ALIASES: + identifier = stripped # Remap any remaining non-standard codes to their canonical equivalents. identifier = cls.NON_STANDARD_CODE_ALIASES.get(identifier, identifier) if identifier in cls.NAMES: diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index 2c471a9cf7..bfccc9fe23 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -1,6 +1,6 @@ import pytest -from palace.manager.core.classifier import Classifier +from palace.manager.core.classifier import Classifier, Lowercased from palace.manager.core.classifier.bisac import ( RE, BISACClassifier, @@ -461,22 +461,101 @@ def test_unrecognized_code_still_uses_keyword_fallback( pytest.param( "Antiques & Collectibles / Kitchenware", False, id="mixed_case_heading" ), + # 29 of the 56 top-level headings are a single unpunctuated word, + # so a heading in the identifier field does not always look like a + # heading. "Juvenile" is the one whose full canonical name is a + # single word (JUV037020). + pytest.param("Juvenile", False, id="single_word_juvenile"), + pytest.param("Fiction", True, id="single_word_fiction"), + # Uppercase is what Boundless sends, and these end in "N" -- the + # suffix scrub_identifier strips from codes like FBJUV000000N. + pytest.param("FICTION", True, id="single_word_uppercase"), + pytest.param("RELIGION", False, id="single_word_uppercase_religion"), + pytest.param("History", False, id="single_word_history"), + pytest.param("Humor", None, id="single_word_stop_rule"), ], ) def test_heading_in_identifier_field_is_matched_as_a_name( - self, identifier: str, expected_fiction: bool + self, identifier: str, expected_fiction: bool | None ) -> None: """Some distributors put the BISAC heading in the identifier field. Boundless sends e.g. "FICTION / Horror" as the subject identifier rather than "FIC015000". That is a name, not a code that failed to resolve, so - it must still be matched against the rulesets. Regression guard for the - abstention introduced alongside it. + it must still be matched against the rulesets -- including when the + heading is a single word and so is indistinguishable from a code by + shape alone. """ subject = self._subject(identifier, None) assert subject.fiction is expected_fiction assert subject.audience == Classifier.AUDIENCE_ADULT + @pytest.mark.parametrize( + "name", + [ + pytest.param("Action & Adventure", id="bibliotheca_fiction_genre"), + pytest.param("Magic", id="bibliotheca_magic"), + pytest.param("Health", id="bibliotheca_nonfiction_genre"), + pytest.param("Horror", id="bare_subheading"), + ], + ) + def test_name_only_subject_still_reaches_the_rulesets(self, name: str) -> None: + """A subject with no identifier keeps the behaviour it always had. + + The heading gate exists to stop the rulesets being applied to the + fragment left behind when a code fails to resolve. A subject that never + carried a code has no such fragment, and some distributors classify + entirely this way -- Bibliotheca sends every genre as a bare name with + no code, so gating these would leave its titles with no fiction + evidence at all and a NULL fiction status on first import. + + Whether a bare sub-heading should carry the rulesets' top-level + inference is a real question, but a distributor-wide one that is not + this change's to answer. + """ + subject = self._subject("", name) + assert subject.fiction is False + assert subject.audience == Classifier.AUDIENCE_ADULT + + @pytest.mark.parametrize( + "heading", + [ + pytest.param("Fiction", id="fiction"), + pytest.param("Juvenile Nonfiction", id="two_words"), + pytest.param("Antiques & Collectibles", id="punctuated"), + pytest.param("Psychology & Psychiatry", id="deprecated_spelling"), + pytest.param( + "Literary Criticism & Collections", id="deprecated_spelling_lit_crit" + ), + ], + ) + def test_top_level_headings_recognized(self, heading: str) -> None: + """TOP_LEVEL_HEADINGS is what decides whether the rulesets apply. + + It is derived from bisac.csv, plus the former spellings of renamed + categories that are no longer in the file but that distributors still + send -- those have Interchangeable tokens in the rulesets, so the set + has to agree with them. + """ + assert Lowercased(heading) in BISACClassifier.TOP_LEVEL_HEADINGS + + @pytest.mark.parametrize( + "fragment", + [ + pytest.param("Historical", id="subheading_only"), + pytest.param("English literature", id="vendor_language_category"), + pytest.param("infen000", id="raw_code"), + ], + ) + def test_fragments_are_not_top_level_headings(self, fragment: str) -> None: + """The leftovers from an unresolvable code are not headings. + + These are exactly the values the rulesets must not be applied to: the + catch-alls would read them as "not filed under Fiction" and vote + nonfiction. + """ + assert Lowercased(fragment) not in BISACClassifier.TOP_LEVEL_HEADINGS + @pytest.mark.parametrize( "identifier,stored_name", [ From 5395f9bb42c126231217aa1029fcc3854af3cea1 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 17:09:39 -0700 Subject: [PATCH 15/22] Drop the migration; the repair moves to a startup task Review follow-up. The migration and the Celery task in the follow-up PR were two implementations of one algorithm -- select BISAC subjects stored as nonfiction, filter through contradicts_stored_fiction, reset checked, log the counts -- written once in raw SQL and once through the ORM. Only the per-row predicate was shared. Carrying both for the life of the repair is not worth it. The startup task in the follow-up PR runs in the same init container moments after migrations, so the reset lands at the same point in the deploy either way, and the task is where the re-run and the next-release re-apply already live. This leaves the PR as what it actually is: the classifier fix. The data repair belongs with the mechanism that can re-run it. Removes the migration, its 13-case test, and the AlembicDatabaseFixture subject helpers that existed only for that test -- tests/migration/conftest.py is now identical to main again. Alembic head returns to 912c566f3383. The trade, for the record: a migration runs regardless of Celery's health, where a dispatched task is lost if the broker is down at deploy. The next-release re-apply covers that, which is what makes dropping this safe. Co-Authored-By: Claude Opus 5 --- ...reset_checked_for_non_bisac_nonfiction_.py | 96 ------------------- tests/migration/conftest.py | 40 -------- ...d1bbdd4671_reset_checked_for_non_bisac_.py | 80 ---------------- 3 files changed, 216 deletions(-) delete mode 100644 alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py delete mode 100644 tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py diff --git a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py deleted file mode 100644 index d5345047d5..0000000000 --- a/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py +++ /dev/null @@ -1,96 +0,0 @@ -"""reset_checked_for_non_bisac_nonfiction_subjects - -Resets checked=False on BISAC subjects whose stored fiction=False no longer -matches what BISACClassifier scores them as, so classify_unchecked_subjects -re-scores them and recalculates the works they are attached to. - -These are subjects whose identifier is not a resolvable BISAC code. The Palace -Marketplace / Feedbooks scheme is stored as type='BISAC' but includes -non-subject codes such as INFEN000 ("English literature"), and classification -used to infer nonfiction from the distributor's name for those. It no longer -does, which leaves the stored values stale. - -Selection asks the classifier rather than pattern-matching the identifier, so -the migration and the runtime cannot disagree about which codes are real. - -Only subjects currently holding fiction=False are examined; most of those are -legitimate nonfiction BISAC codes and are left untouched. - -Must ship in the same release as the classifier change. Run against the old -code, classify_unchecked_subjects would re-score these subjects under the old -rules and re-stamp checked=True, paying for a full reindex that changes -nothing. - -Revision ID: 52d1bbdd4671 -Revises: 912c566f3383 -Create Date: 2026-09-02 17:26:39.209822+00:00 - -""" - -import sqlalchemy as sa -from alembic import op - -from palace.manager.core.classifier.bisac import BISACClassifier -from palace.manager.util.migration.helpers import migration_logger - -# revision identifiers, used by Alembic. -revision = "52d1bbdd4671" -down_revision = "912c566f3383" -branch_labels = None -depends_on = None - -log = migration_logger(revision) - - -def upgrade() -> None: - conn = op.get_bind() - - candidates = conn.execute( - sa.text( - """ - SELECT id, identifier, name - FROM subjects - WHERE type = 'BISAC' - AND checked - AND fiction IS FALSE - """ - ) - ).all() - - # Ask the classifier rather than matching a pattern against the identifier. - # A pattern would accept a shape-valid but non-existent code such as - # FBZZZ000000, which the classifier rejects, and its fabricated nonfiction - # value would survive this repair. - stale_ids = [] - for row in candidates: - if not row.identifier and not row.name: - # Nothing to classify. Subject.lookup will not create such a row, - # but the columns are nullable, so don't assume. - continue - identifier, name = BISACClassifier.scrub_identifier_and_name( - row.identifier, row.name - ) - if BISACClassifier.is_fiction(identifier, name) is not False: - stale_ids.append(row.id) - log.info( - f"Reset checked=False for subject id={row.id} " - f"identifier={row.identifier!r} name={row.name!r}" - ) - - if stale_ids: - conn.execute( - sa.text("UPDATE subjects SET checked = false WHERE id = ANY(:ids)"), - {"ids": stale_ids}, - ) - - log.info( - f"Reset checked=False for {len(stale_ids)} of {len(candidates)} " - f"BISAC subjects stored as nonfiction" - ) - - -def downgrade() -> None: - # The previous checked values are not recorded, and re-marking these - # subjects checked would only re-suppress the reclassification this - # migration exists to trigger. Intentionally a no-op. - pass diff --git a/tests/migration/conftest.py b/tests/migration/conftest.py index cbb00d7a1b..036005dfdf 100644 --- a/tests/migration/conftest.py +++ b/tests/migration/conftest.py @@ -326,46 +326,6 @@ def identifier( assert isinstance(identifier_record.id, int) return identifier_record.id - def subject( - self, - subject_type: str, - identifier: str, - name: str | None = None, - fiction: bool | None = None, - checked: bool = True, - ) -> int: - """Create a subject record.""" - with self._engine.begin() as connection: - subject_record = connection.execute( - text( - """ - INSERT INTO subjects (type, identifier, name, fiction, checked, locked) - VALUES (:type, :identifier, :name, :fiction, :checked, false) - RETURNING id - """ - ), - { - "type": subject_type, - "identifier": identifier, - "name": name, - "fiction": fiction, - "checked": checked, - }, - ).fetchone() - - assert subject_record is not None - assert isinstance(subject_record.id, int) - return subject_record.id - - def fetch_subject(self, subject_id: int) -> Row: - """Read back a subject record.""" - with self._engine.begin() as connection: - result = connection.execute( - text("SELECT * FROM subjects WHERE id = :id"), - {"id": subject_id}, - ) - return result.one() - def collection( self, integration_configuration_id: int, diff --git a/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py b/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py deleted file mode 100644 index 4852440b0f..0000000000 --- a/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py +++ /dev/null @@ -1,80 +0,0 @@ -import pytest -from pytest_alembic import MigrationContext - -from tests.migration.conftest import AlembicDatabaseFixture - -REVISION = "52d1bbdd4671" - - -@pytest.mark.parametrize( - "identifier,name", - [ - pytest.param("INFEN000", "English literature", id="language_category"), - pytest.param( - "INFENUSA", "American and Canadian literature", id="territory_category" - ), - pytest.param("FBSACT000000", "News and investigations", id="vendor_code"), - pytest.param("FSHUM000000N", "Human science", id="vendor_code_with_n_suffix"), - pytest.param("SOCO32000", None, id="malformed_code_without_name"), - pytest.param("FBZZZ000000", "Historical", id="shape_valid_but_nonexistent"), - pytest.param("FBFIC014000", "Historical", id="stale_canonical_fiction_code"), - ], -) -def test_resets_subjects_no_longer_scored_as_nonfiction( - alembic_runner: MigrationContext, - alembic_database: AlembicDatabaseFixture, - identifier: str, - name: str | None, -) -> None: - """Subjects stored as nonfiction that the classifier no longer scores that - way are marked unchecked, so classify_unchecked_subjects re-scores them. - - Covers both codes that are not BISAC at all and codes that merely look like - one (FBZZZ000000), which a pattern-based predicate would wrongly accept. - """ - alembic_runner.migrate_down_to(REVISION) - alembic_runner.migrate_down_one() - - subject_id = alembic_database.subject( - "BISAC", identifier, name=name, fiction=False, checked=True - ) - - alembic_runner.migrate_up_one() - - assert alembic_database.fetch_subject(subject_id).checked is False - - -@pytest.mark.parametrize( - "subject_type,identifier,name,fiction", - [ - pytest.param("BISAC", "HIS027000", None, False, id="real_nonfiction_code"), - pytest.param("BISAC", "HIS000000", None, False, id="real_nonfiction_general"), - pytest.param( - "BISAC", "HISTORY / General", None, False, id="nonfiction_heading" - ), - pytest.param("BISAC", "INFEN000", None, True, id="already_scored_fiction"), - pytest.param("BISAC", "INFEN000", None, None, id="already_scored_unknown"), - pytest.param("tag", "INFEN000", None, False, id="not_a_bisac_subject"), - ], -) -def test_leaves_everything_else_checked( - alembic_runner: MigrationContext, - alembic_database: AlembicDatabaseFixture, - subject_type: str, - identifier: str, - name: str | None, - fiction: bool | None, -) -> None: - """Legitimate nonfiction codes keep their value, and subjects outside the - examined set -- already scored as fiction or unknown, or not BISAC-typed -- - are not touched.""" - alembic_runner.migrate_down_to(REVISION) - alembic_runner.migrate_down_one() - - subject_id = alembic_database.subject( - subject_type, identifier, name=name, fiction=fiction, checked=True - ) - - alembic_runner.migrate_up_one() - - assert alembic_database.fetch_subject(subject_id).checked is True From e9ec925002d1a0e1516c6aadcae545d9aefe020d Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 16 Sep 2026 13:58:07 -0700 Subject: [PATCH 16/22] Correct two inaccurate rationales in the new comments Both were wrong about mechanism rather than behaviour; no code changes. _has_canonical_heading credited Boundless with sending headings in the identifier field. It does not: BoundlessParser._extract_subjects sends identifier=None with the heading in name, so its subjects take the "no identifier" branch and never reach the gate. The real path is an OPDS with no label, since the OPDS1 extractor maps term to the identifier and label to the name. The misattribution came from the test_edit_classifications fixture, which passes a heading as subject_identifier under the Boundless data source but does not match what the parser produces. The matching test docstring said the same thing and is corrected too. The deprecated-spelling list claimed the Interchangeable tokens in the rulesets were why the entries are needed. FICTION has no Interchangeable tokens at all and AUDIENCE's four are canonical spellings; all five deprecated spellings appear only in GENRE, which has its own loop and never consults TOP_LEVEL_HEADINGS. What actually makes the entries load-bearing is the catch-all: without them a deprecated spelling carried by an unresolvable code abstains instead of being read as nonfiction/Adult. Verified both ways -- "Psychology & Psychiatry / Foo" on INFEN000 reaches the rulesets with the entry and does not without it. The comment now states that test, so a maintainer can tell whether a new entry belongs. Co-Authored-By: Claude Opus 5 --- src/palace/manager/core/classifier/bisac.py | 20 +++++++++++++------- tests/manager/core/classifiers/test_bisac.py | 9 +++++---- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index ee09d0f6e2..f81254109e 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -660,9 +660,14 @@ class BISACClassifier(Classifier): TOP_LEVEL_HEADINGS: frozenset[str] = frozenset( Lowercased(name.split("/")[0].strip()) for name in NAMES.values() ) | frozenset( - # Renamed categories whose former top-level spelling is no longer in - # bisac.csv but which distributors still send. The Interchangeable - # tokens above let the rulesets match these, so this set has to agree. + # Former top-level spellings of renamed categories, which distributors + # still send and bisac.csv no longer lists. They belong here so that a + # name beginning with one still reaches the catch-all rules: without + # the entry, a deprecated spelling carried by a code that does not + # resolve abstains instead of being read as nonfiction/Adult. That is + # the test for whether a new entry belongs -- not whether the rulesets + # have an Interchangeable for it, which is a separate mechanism in + # GENRE, and GENRE does not consult this set. Lowercased(name) for name in ( "Mind & Spirit", @@ -679,10 +684,11 @@ def _has_canonical_heading(cls, name: list[str]) -> bool: This is the premise the FICTION and AUDIENCE catch-all rules rest on, so it is what has to be checked before running them. It holds for a - name that came from `NAMES`, and for one a distributor supplied in - either the name or the identifier field -- Boundless sends headings - like "FICTION / Horror" as the identifier, and a bare top-level - heading such as "Juvenile" is equally a heading. + name that came from `NAMES`, and for a heading a distributor supplied + in the identifier field -- an OPDS `` + with no `label` arrives that way, since the OPDS1 extractor maps `term` + to the identifier and `label` to the name. A bare top-level heading + such as "Juvenile" is equally a heading. It does not hold for the fragment left over when a code cannot be resolved ("Historical", "English literature"), which is the case these diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index bfccc9fe23..6ddae2c067 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -478,11 +478,12 @@ def test_unrecognized_code_still_uses_keyword_fallback( def test_heading_in_identifier_field_is_matched_as_a_name( self, identifier: str, expected_fiction: bool | None ) -> None: - """Some distributors put the BISAC heading in the identifier field. + """A BISAC heading can arrive in the identifier field. - Boundless sends e.g. "FICTION / Horror" as the subject identifier rather - than "FIC015000". That is a name, not a code that failed to resolve, so - it must still be matched against the rulesets -- including when the + An OPDS `` with no `label` lands that + way: the OPDS1 extractor maps `term` to the identifier and `label` to + the name. That is a heading, not a code that failed to resolve, so it + must still be matched against the rulesets -- including when the heading is a single word and so is indistinguishable from a code by shape alone. """ From 4d1593e7dafa38c41da8bd70c349e25bb3b67fc8 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 17 Sep 2026 10:08:06 -0700 Subject: [PATCH 17/22] Filter genres against the fiction status we are about to keep `_keep_known_value` restored the work's stored fiction status *after* `classifier.classify()` had already run. But `classify()` derives the genres from the fiction status, and `WorkClassifier.genres()` skips its consistency filter entirely when that status is `None` -- so the filter saw `None`, kept genres of either status, and we then stamped the old status back onto a work whose own genres contradicted it. Both halves of this PR are needed to reach that state: abstention leaves fiction undetermined, and the restore turns "undetermined plus a fiction genre" into "nonfiction plus a fiction genre". It is reachable with real data. Tags are the vector -- `Horror`, `Romance` and `Historical` each contribute a genre while casting no fiction vote at all -- so any work whose BISAC codes now abstain will hit it if it also carries a descriptive tag. The mirror direction has the volume: `FSHUM000000N` (569 titles) abstains and yields the nonfiction-only genre `Social Sciences`, which a work stored as fiction would keep. Hand the retained values to `classify()` as its defaults instead, so the genre filter runs against the status the work will actually end up with. The post-call restores become unreachable once the defaults can no longer be `None`, so they go. The audience half is symmetric but inert in practice: `_get_default_audience()` never returns `None`, so only a caller that passes `default_audience=None` explicitly can reach it. It is worth having anyway -- `target_age` is derived from the audience the same way genres are derived from fiction, and is not protected by any restore. Co-Authored-By: Claude Opus 5 --- src/palace/manager/sqlalchemy/model/work.py | 17 +++++++---- tests/manager/sqlalchemy/model/test_work.py | 31 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/palace/manager/sqlalchemy/model/work.py b/src/palace/manager/sqlalchemy/model/work.py index 65a2351820..d65ec259d3 100644 --- a/src/palace/manager/sqlalchemy/model/work.py +++ b/src/palace/manager/sqlalchemy/model/work.py @@ -1372,8 +1372,9 @@ def calculate_quality(self, identifier_ids, default_quality=0): def _keep_known_value[T](old_value: T | None, new_value: T | None) -> T | None: """Prefer `new_value`, but never replace a known value with None. - A recalculation that gathered no usable classifications should leave an - existing determination alone rather than erasing it. + Handed to the classifier as its default, this leaves an existing + determination alone when a recalculation gathers no usable + classifications, rather than erasing it. """ return old_value if new_value is None else new_value @@ -1401,19 +1402,25 @@ def assign_genres( for classification in classifications: classifier.add(classification) + # Hand the retained values to the classifier as its defaults rather + # than restoring them after it has run. classify() derives the genres + # from the fiction status and the target age from the audience, so a + # value restored afterwards arrives too late to be consulted: the genre + # filter would already have run with None -- which disables it, keeping + # genres of either fiction status -- and we would then stamp the + # retained status back onto a work whose own genres now contradict it. (genre_weights, new_fiction, new_audience, target_age) = classifier.classify( - default_fiction=default_fiction, default_audience=default_audience + default_fiction=self._keep_known_value(old_fiction, default_fiction), + default_audience=self._keep_known_value(old_audience, default_audience), ) new_target_age = tuple_to_numericrange(target_age) if self.target_age != new_target_age: self.target_age = new_target_age - new_fiction = self._keep_known_value(old_fiction, new_fiction) if new_fiction != old_fiction: self.fiction = new_fiction - new_audience = self._keep_known_value(old_audience, new_audience) if new_audience != old_audience: self.audience = new_audience diff --git a/tests/manager/sqlalchemy/model/test_work.py b/tests/manager/sqlalchemy/model/test_work.py index a78938de4b..122029402e 100644 --- a/tests/manager/sqlalchemy/model/test_work.py +++ b/tests/manager/sqlalchemy/model/test_work.py @@ -406,6 +406,37 @@ def test_assign_genres_does_not_overwrite_fiction_with_null( assert work.fiction is True + @pytest.mark.parametrize( + "stored_fiction,expected_genres", + [ + pytest.param(False, [], id="nonfiction-work-drops-the-fiction-genre"), + pytest.param(True, ["Horror"], id="fiction-work-keeps-the-fiction-genre"), + ], + ) + def test_assign_genres_filters_genres_against_the_retained_fiction_status( + self, + db: DatabaseTransactionFixture, + stored_fiction: bool, + expected_genres: list[str], + ) -> None: + # A tag contributes a genre without voting on fiction status, so a work + # whose BISAC codes all abstain reaches the genre filter with no fiction + # determination at all -- and the filter is a no-op when fiction is + # None, keeping genres of either status. The retained status therefore + # has to be in hand *before* the filter runs; restoring it afterwards + # would leave the work stamped with a status its own genres contradict. + work = db.work(with_license_pool=True) + work.fiction = stored_fiction + work.license_pools[0].identifier.classify( + DataSource.lookup(db.session, DataSource.OCLC), Subject.TAG, "Horror" + ) + db.session.commit() + + work.assign_genres(work._direct_identifier_ids, default_fiction=None) + + assert work.fiction is stored_fiction + assert sorted(genre.name for genre in work.genres) == expected_genres + def test__choose_summary(self, db: DatabaseTransactionFixture): # Test the _choose_summary helper method, called by # calculate_presentation(). From a8391b04f515f01071723e1671a1251d2fede4dc Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 11:17:56 -0700 Subject: [PATCH 18/22] Make the non-BISAC nonfiction reset re-runnable (PP-5129) Migration 52d1bbdd4671 repairs subjects that were stored as nonfiction because an unresolvable BISAC code fell through the ruleset catch-all. It only sets checked=false; the re-scoring happens later, in classify_unchecked_subjects. That leaves a window. If old code reaches those subjects first -- a still-running scripts server, or a host that redeploys itself via watchtower or ECS/Fargate auto_restart -- it re-scores them under the superseded rules and re-stamps checked=true. The reset is consumed rather than lost, so nothing errors, nothing retries, and no later run revisits them. The repair quietly did nothing, having paid for a full reindex. Raised in review on #3726; it is not preventable from inside the migration, so make it recoverable instead. Adds reset_non_bisac_nonfiction_subjects, a Celery task that re-applies the reset, with ResetNonBisacNonfictionSubjectsScript and bin/work_reset_non_bisac_nonfiction_subjects to queue it -- the same three layers as reclassify_null_audience_works, which repairs the sibling audience defect. The task resets only. Re-scoring stays with classify_unchecked_subjects, which picks these subjects up on its next nightly run and can be triggered immediately through bin/work_classify_unchecked_subjects. One task, one job, and no large reindex fired the moment someone runs the repair. The selection lives on BISACClassifier as contradicts_stored_fiction, so the migration and the task cannot disagree about which rows are affected. Both already import the classifier, so this adds no coupling that was not there, and it puts the definition with the code that owns the answer. The migration is updated to use it. Co-Authored-By: Claude Opus 5 --- bin/work_reset_non_bisac_nonfiction_subjects | 11 ++++ src/palace/manager/celery/tasks/work.py | 50 ++++++++++++++++ src/palace/manager/core/classifier/bisac.py | 30 ++++++++++ src/palace/manager/scripts/work.py | 20 +++++++ tests/manager/celery/tasks/test_work.py | 61 ++++++++++++++++++++ tests/manager/core/classifiers/test_bisac.py | 42 ++++++++++++++ tests/manager/scripts/test_work.py | 11 ++++ 7 files changed, 225 insertions(+) create mode 100755 bin/work_reset_non_bisac_nonfiction_subjects diff --git a/bin/work_reset_non_bisac_nonfiction_subjects b/bin/work_reset_non_bisac_nonfiction_subjects new file mode 100755 index 0000000000..00e84b51f1 --- /dev/null +++ b/bin/work_reset_non_bisac_nonfiction_subjects @@ -0,0 +1,11 @@ +#!/usr/bin/env python +"""Queue the reset_non_bisac_nonfiction_subjects Celery task. + +Convenience wrapper that manually dispatches the repair for BISAC subjects +stored as nonfiction in error (the `reset_non_bisac_nonfiction_subjects` Celery +task) for a worker to process. +""" + +from palace.manager.scripts.work import ResetNonBisacNonfictionSubjectsScript + +ResetNonBisacNonfictionSubjectsScript().run() diff --git a/src/palace/manager/celery/tasks/work.py b/src/palace/manager/celery/tasks/work.py index 32c10c52bf..1241419290 100644 --- a/src/palace/manager/celery/tasks/work.py +++ b/src/palace/manager/celery/tasks/work.py @@ -4,6 +4,7 @@ from sqlalchemy.orm import Session from palace.manager.celery.task import Task +from palace.manager.core.classifier.bisac import BISACClassifier from palace.manager.data_layer.policy.presentation import PresentationCalculationPolicy from palace.manager.service.celery.celery import QueueNames from palace.manager.sqlalchemy.model.classification import Classification, Subject @@ -39,6 +40,55 @@ def reclassify_null_audience_works(task: Task) -> None: session.commit() +@shared_task(queue=QueueNames.default, bind=True) +def reset_non_bisac_nonfiction_subjects(task: Task) -> None: + """Re-apply the reset that repairs subjects stored as nonfiction in error. + + Migration 52d1bbdd4671 marks these subjects unchecked so that + classify_unchecked_subjects re-scores them. That reset can be consumed + before it takes effect: if old code reaches the subjects first -- a + still-running scripts server, or a host that redeploys itself -- it + re-scores them under the superseded rules and re-stamps checked=True. + Nothing errors, and nothing revisits them afterwards, so the repair + quietly did nothing. This task exists to run the reset again. + + It resets only. The re-scoring stays with classify_unchecked_subjects, + which picks these subjects up on its next nightly run; trigger + bin/work_classify_unchecked_subjects to have it happen sooner. + + Idempotent: a second run finds nothing to do. + """ + with task.session() as session: + candidates = ( + session.query(Subject.id, Subject.identifier, Subject.name) + .filter( + Subject.type == Subject.BISAC, + Subject.checked == True, # noqa: E712 + Subject.fiction == False, # noqa: E712 + ) + .all() + ) + + stale_ids = [ + row.id + for row in candidates + if BISACClassifier.contradicts_stored_fiction( + row.identifier, row.name, False + ) + ] + + if stale_ids: + session.query(Subject).filter(Subject.id.in_(stale_ids)).update( + {Subject.checked: False}, synchronize_session=False + ) + session.commit() + + task.log.info( + f"Reset checked=False for {len(stale_ids)} of {len(candidates)} " + f"BISAC subjects stored as nonfiction." + ) + + @shared_task(queue=QueueNames.default, bind=True) def classify_unchecked_subjects(task: Task) -> None: """Reclassify all Works whose current classifications appear to diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index f81254109e..34d6f04739 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -696,6 +696,36 @@ def _has_canonical_heading(cls, name: list[str]) -> bool: """ return bool(name) and name[0] in cls.TOP_LEVEL_HEADINGS + @classmethod + def contradicts_stored_fiction( + cls, + identifier: str | None, + name: str | None, + stored_fiction: bool | None, + ) -> bool: + """Does this classifier disagree with a subject's stored fiction status? + + Subjects are only re-examined when `checked` is false, so a value + scored under superseded rules persists indefinitely. Repairs that + reset `checked` need to identify those rows, and they need to agree + with each other about which rows they are. Expressing the question + here keeps that definition in one place: a subject is stale when the + classifier, run now, does not return what is stored. + + :param identifier: The subject's identifier, as stored. + :param name: The subject's name, as stored. + :param stored_fiction: The subject's current `fiction` value. + :return: True when the classifier no longer agrees with `stored_fiction`. + """ + if not identifier and not name: + # Nothing to classify. Subject.lookup will not create such a row, + # but both columns are nullable, so do not assume. + return False + scrubbed_identifier, scrubbed_name = cls.scrub_identifier_and_name( + identifier, name + ) + return cls.is_fiction(scrubbed_identifier, scrubbed_name) is not stored_fiction + @classmethod def _apply_rulesets[RulesetResult]( cls, diff --git a/src/palace/manager/scripts/work.py b/src/palace/manager/scripts/work.py index 0c848b3d2b..a87412ff40 100644 --- a/src/palace/manager/scripts/work.py +++ b/src/palace/manager/scripts/work.py @@ -11,6 +11,7 @@ from palace.manager.celery.tasks.work import ( classify_unchecked_subjects, reclassify_null_audience_works, + reset_non_bisac_nonfiction_subjects, ) from palace.manager.data_layer.policy.presentation import ( PresentationCalculationPolicy, @@ -252,6 +253,25 @@ class WorkOPDSScript(WorkPresentationScript): ) +class ResetNonBisacNonfictionSubjectsScript(Script): + """Manually dispatch the ``reset_non_bisac_nonfiction_subjects`` Celery task. + + The work itself happens in the Celery task; this script just queues it. It + exists so the repair in migration 52d1bbdd4671 can be applied again, in + case its reset was consumed by old code before the new classifier was live. + + TODO: Remove this script when the ``reset_non_bisac_nonfiction_subjects`` + Celery task is removed. + """ + + def do_run(self, *args: Any, **kwargs: Any) -> None: + reset_non_bisac_nonfiction_subjects.delay() + self.log.info( + 'The "reset_non_bisac_nonfiction_subjects" task has been queued for ' + "execution. See the celery logs for details about task execution." + ) + + class ReclassifyNullAudienceWorksScript(Script): """Manually dispatch the ``reclassify_null_audience_works`` Celery task. diff --git a/tests/manager/celery/tasks/test_work.py b/tests/manager/celery/tasks/test_work.py index d7923c5fc5..ce8b680773 100644 --- a/tests/manager/celery/tasks/test_work.py +++ b/tests/manager/celery/tasks/test_work.py @@ -119,3 +119,64 @@ def test_reclassify_null_audience_works( policy = call_obj[1]["policy"] assert policy.classify is True assert policy.choose_edition is False + + +def test_reset_non_bisac_nonfiction_subjects( + db: DatabaseTransactionFixture, + celery_fixture: CeleryFixture, +): + """The task re-applies the reset for subjects stored as nonfiction in error. + + Re-runnable stand-in for migration 52d1bbdd4671, for when that migration's + reset was consumed by old code before the new classifier was live. + """ + stale = db.subject(Subject.BISAC, "INFEN000") + stale.name = "English literature" + stale.fiction = False + stale.checked = True + + # A real nonfiction code the classifier still agrees with. + agrees = db.subject(Subject.BISAC, "HIS027000") + agrees.fiction = False + agrees.checked = True + + # Already scored as fiction, so outside the set the task examines. + scored_fiction = db.subject(Subject.BISAC, "INFENUSA") + scored_fiction.name = "American and Canadian literature" + scored_fiction.fiction = True + scored_fiction.checked = True + + # Same identifier, but not a BISAC subject. + other_type = db.subject(Subject.TAG, "INFEN000") + other_type.fiction = False + other_type.checked = True + + db.session.commit() + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + + assert stale.checked is False + assert agrees.checked is True + assert scored_fiction.checked is True + assert other_type.checked is True + + +def test_reset_non_bisac_nonfiction_subjects_is_idempotent( + db: DatabaseTransactionFixture, + celery_fixture: CeleryFixture, +): + """A second run finds nothing left to do and leaves the reset in place.""" + subject = db.subject(Subject.BISAC, "INFEN000") + subject.name = "English literature" + subject.fiction = False + subject.checked = True + db.session.commit() + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + assert subject.checked is False + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + assert subject.checked is False diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index 6ddae2c067..252ac57a63 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -557,6 +557,48 @@ def test_fragments_are_not_top_level_headings(self, fragment: str) -> None: """ assert Lowercased(fragment) not in BISACClassifier.TOP_LEVEL_HEADINGS + @pytest.mark.parametrize( + "identifier,name,stored_fiction,expected", + [ + pytest.param( + "INFEN000", "English literature", False, True, id="vendor_code_is_stale" + ), + pytest.param( + "FBZZZ000000", "Historical", False, True, id="unreal_code_is_stale" + ), + pytest.param( + "FBFIC014000", + "Historical", + False, + True, + id="real_fiction_code_is_stale", + ), + pytest.param("HIS027000", None, False, False, id="real_nonfiction_agrees"), + pytest.param("FBFIC014000", "Historical", True, False, id="fiction_agrees"), + pytest.param( + "INFEN000", "English literature", True, False, id="keyword_agrees" + ), + pytest.param(None, None, False, False, id="nothing_to_classify"), + ], + ) + def test_contradicts_stored_fiction( + self, + identifier: str | None, + name: str | None, + stored_fiction: bool | None, + expected: bool, + ) -> None: + """The shared definition of a subject whose stored value went stale. + + Subjects are only re-examined when `checked` is false, so the repairs + that reset it need one definition of which rows are affected. Both the + migration and the re-run task ask this. + """ + assert ( + BISACClassifier.contradicts_stored_fiction(identifier, name, stored_fiction) + is expected + ) + @pytest.mark.parametrize( "identifier,stored_name", [ diff --git a/tests/manager/scripts/test_work.py b/tests/manager/scripts/test_work.py index 14dda5de47..58fb519207 100644 --- a/tests/manager/scripts/test_work.py +++ b/tests/manager/scripts/test_work.py @@ -10,6 +10,7 @@ from palace.manager.scripts.work import ( ReclassifyNullAudienceWorksScript, ReclassifyWorksForUncheckedSubjectsScript, + ResetNonBisacNonfictionSubjectsScript, WorkProcessingScript, ) from palace.manager.sqlalchemy.model.datasource import DataSource @@ -198,3 +199,13 @@ def test_run(self, db: DatabaseTransactionFixture): ) as task: ReclassifyNullAudienceWorksScript(db.session).run() assert task.delay.call_count == 1 + + +class TestResetNonBisacNonfictionSubjectsScript: + def test_run(self, db: DatabaseTransactionFixture): + """The script queues the reset_non_bisac_nonfiction_subjects Celery task.""" + with patch( + "palace.manager.scripts.work.reset_non_bisac_nonfiction_subjects" + ) as task: + ResetNonBisacNonfictionSubjectsScript(db.session).run() + assert task.delay.call_count == 1 From e3a6fcb89844db02b28cb5a9c317ef4faa61ee62 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 16:34:29 -0700 Subject: [PATCH 19/22] Re-score the reset subjects at deploy instead of overnight (PP-5129) Migration 52d1bbdd4671 only resets checked=False. classify_unchecked_subjects is what re-scores those subjects and recalculates their works, and left to itself that does not happen until the nightly run. The gap between the two is where the repair is exposed: anything reaching Subject.assign_to_genre in the meantime consumes the reset, and if it is running the superseded rules it re-stamps checked=True with the same wrong value. Nothing errors and nothing revisits the subject afterwards. Adds a startup task that dispatches the re-score immediately after the migration, shrinking that gap from about a day to seconds. This is the pattern the null-audience repair already used -- migration d856ff4dbefb makes the data change, startup task 2026_05_12 dispatches the follow-up. The timing works out: helpers/migrate.yml stops the scripts container -- which is where every Celery worker and beat run -- before running the migration, and starts it again afterwards from the new image. So no worker is alive when this dispatches, and the one that picks the task up is necessarily new code. Web containers are the remaining exposure. The deploy recycles them after the migration, and they can reach assign_to_genre through a presentation recalculation. A second startup task in the next release re-applies the reset once no old code is running anywhere. This moves roughly 4,877 works' worth of recalculation and reindexing from overnight to deploy time. That is about 2% of the nightly volume behind PP-4472, so it should be unremarkable, but it is a deliberate choice. Co-Authored-By: Claude Opus 5 --- ...eclassify_non_bisac_nonfiction_subjects.py | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py diff --git a/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py new file mode 100644 index 0000000000..91e8578a63 --- /dev/null +++ b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py @@ -0,0 +1,38 @@ +"""Re-score the subjects that migration 52d1bbdd4671 marked unchecked. + +That migration resets ``checked=False`` on BISAC subjects stored as nonfiction +because an unresolvable code fell through the ruleset catch-all. It only resets; +``classify_unchecked_subjects`` is what re-scores them and recalculates their +works, and left to itself that does not happen until the nightly run. + +The gap between the two is where the repair is exposed. Anything that reaches +``Subject.assign_to_genre`` in the meantime consumes the reset -- and if it is +running the superseded rules, it re-stamps ``checked=True`` with the same wrong +value. Nothing errors and nothing revisits the subject afterwards, so the repair +silently did nothing, having paid for a reindex to do it. + +Dispatching here closes that gap to seconds. This runs from the migrate +container immediately after the migration, at a point in the deploy where the +Celery workers have been stopped and will come back on the new image, so the +task is picked up by new code. + +The remaining exposure is the web containers, which the deploy recycles after +the migration and which can reach ``assign_to_genre`` through a presentation +recalculation. A later startup task re-applies the reset once no old code is +running anywhere. + +TODO: Remove this task once it has run on all deployments (PP-5129).""" + +from __future__ import annotations + +import logging + +from celery.canvas import Signature +from sqlalchemy.orm import Session + +from palace.manager.celery.tasks.work import classify_unchecked_subjects +from palace.manager.service.container import Services + + +def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: + return classify_unchecked_subjects.s() From 8fdbb7590027ed717575670339b2acfeb6769c06 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 14 Sep 2026 17:14:34 -0700 Subject: [PATCH 20/22] Do the reset from the startup task, not a migration Follow-on from dropping the migration. The reset and the re-score are now chained from one place: reset_non_bisac_nonfiction_subjects marks the stale subjects unchecked, classify_unchecked_subjects re-scores them. The second signature is immutable so the chain does not pass the first task's return value into a task that takes no arguments. There is now one implementation of the selection instead of two. The task is the only thing that knows how to find these subjects, and all three callers -- this startup task, the next-release re-apply, and the bin wrapper -- go through it. Chaining rather than dispatching the reset alone is what keeps the repair from being exposed. The reset on its own can be consumed by anything reaching Subject.assign_to_genre first, and code on the superseded rules re-stamps checked=True with the same wrong value, silently. Running the re-score straight after closes that to seconds instead of waiting for the nightly. Docstrings that referred to migration 52d1bbdd4671 now describe the condition they repair rather than pointing at a file that no longer exists. Co-Authored-By: Claude Opus 5 --- src/palace/manager/celery/tasks/work.py | 23 ++++--- src/palace/manager/scripts/work.py | 4 +- ...eclassify_non_bisac_nonfiction_subjects.py | 64 ++++++++++++------- 3 files changed, 53 insertions(+), 38 deletions(-) diff --git a/src/palace/manager/celery/tasks/work.py b/src/palace/manager/celery/tasks/work.py index 1241419290..de76d8cfc2 100644 --- a/src/palace/manager/celery/tasks/work.py +++ b/src/palace/manager/celery/tasks/work.py @@ -42,21 +42,20 @@ def reclassify_null_audience_works(task: Task) -> None: @shared_task(queue=QueueNames.default, bind=True) def reset_non_bisac_nonfiction_subjects(task: Task) -> None: - """Re-apply the reset that repairs subjects stored as nonfiction in error. + """Mark BISAC subjects unchecked when their stored fiction status went stale. - Migration 52d1bbdd4671 marks these subjects unchecked so that - classify_unchecked_subjects re-scores them. That reset can be consumed - before it takes effect: if old code reaches the subjects first -- a - still-running scripts server, or a host that redeploys itself -- it - re-scores them under the superseded rules and re-stamps checked=True. - Nothing errors, and nothing revisits them afterwards, so the repair - quietly did nothing. This task exists to run the reset again. + A code that cannot be resolved to a canonical BISAC heading used to be + read as nonfiction by the ruleset catch-all, so those subjects carry a + fabricated fiction=False. Subjects are only re-examined when checked is + false, so repairing them means resetting that flag. - It resets only. The re-scoring stays with classify_unchecked_subjects, - which picks these subjects up on its next nightly run; trigger - bin/work_classify_unchecked_subjects to have it happen sooner. + This resets only. Re-scoring is classify_unchecked_subjects' job: the + startup task that runs this at deploy chains the two together, and the + nightly run picks up anything left over. - Idempotent: a second run finds nothing to do. + Idempotent and self-selecting -- it recomputes which subjects the + classifier no longer agrees with, so a second run finds nothing to do. + That is what lets a later release re-apply it safely. """ with task.session() as session: candidates = ( diff --git a/src/palace/manager/scripts/work.py b/src/palace/manager/scripts/work.py index a87412ff40..2d33297957 100644 --- a/src/palace/manager/scripts/work.py +++ b/src/palace/manager/scripts/work.py @@ -257,8 +257,8 @@ class ResetNonBisacNonfictionSubjectsScript(Script): """Manually dispatch the ``reset_non_bisac_nonfiction_subjects`` Celery task. The work itself happens in the Celery task; this script just queues it. It - exists so the repair in migration 52d1bbdd4671 can be applied again, in - case its reset was consumed by old code before the new classifier was live. + exists so the repair can be applied again on demand, in case its reset was + consumed by old code before the new classifier was live everywhere. TODO: Remove this script when the ``reset_non_bisac_nonfiction_subjects`` Celery task is removed. diff --git a/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py index 91e8578a63..d8b29c1e21 100644 --- a/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py +++ b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py @@ -1,24 +1,34 @@ -"""Re-score the subjects that migration 52d1bbdd4671 marked unchecked. - -That migration resets ``checked=False`` on BISAC subjects stored as nonfiction -because an unresolvable code fell through the ruleset catch-all. It only resets; -``classify_unchecked_subjects`` is what re-scores them and recalculates their -works, and left to itself that does not happen until the nightly run. - -The gap between the two is where the repair is exposed. Anything that reaches -``Subject.assign_to_genre`` in the meantime consumes the reset -- and if it is -running the superseded rules, it re-stamps ``checked=True`` with the same wrong -value. Nothing errors and nothing revisits the subject afterwards, so the repair -silently did nothing, having paid for a reindex to do it. - -Dispatching here closes that gap to seconds. This runs from the migrate -container immediately after the migration, at a point in the deploy where the -Celery workers have been stopped and will come back on the new image, so the -task is picked up by new code. - -The remaining exposure is the web containers, which the deploy recycles after -the migration and which can reach ``assign_to_genre`` through a presentation -recalculation. A later startup task re-applies the reset once no old code is +"""Repair BISAC subjects stored as nonfiction because their code did not resolve. + +Everything on the Palace Marketplace / Feedbooks category scheme is stored with +``type='BISAC'``, including codes that are not BISAC at all -- language and +territory categories such as ``INFEN000`` ("English literature"). Those cannot +be resolved to a canonical heading, so classification used to infer nonfiction +from the distributor's name and store ``fiction=False``. The classifier no +longer does that, which leaves the stored values stale. + +Subjects are only re-examined when ``checked`` is false, so this dispatches two +steps: ``reset_non_bisac_nonfiction_subjects`` marks the stale ones unchecked, +then ``classify_unchecked_subjects`` re-scores them and recalculates their +works. The second signature is immutable so the chain does not pass the first +task's return value into it. + +Doing both here matters. The reset on its own is exposed: anything reaching +``Subject.assign_to_genre`` before the re-score consumes it, and code running +the superseded rules re-stamps ``checked=True`` with the same wrong value. +Nothing errors and nothing revisits the subject afterwards, so the repair +silently did nothing, having paid for a reindex to do it. Chaining the re-score +closes that gap to seconds rather than waiting for the nightly run. + +The timing works out. ``helpers/migrate.yml`` stops the scripts container -- +where every Celery worker and beat run -- before migrating, and starts it again +from the new image afterwards, so the worker that picks this up is necessarily +new code. + +Web containers are the remaining exposure: the deploy recycles them after the +migration step, and they can reach ``assign_to_genre`` through a presentation +recalculation. Fargate deployments are not governed by that playbook at all. A +second startup task re-applies the reset a release later, once no old code is running anywhere. TODO: Remove this task once it has run on all deployments (PP-5129).""" @@ -27,12 +37,18 @@ import logging -from celery.canvas import Signature +from celery.canvas import Signature, chain from sqlalchemy.orm import Session -from palace.manager.celery.tasks.work import classify_unchecked_subjects +from palace.manager.celery.tasks.work import ( + classify_unchecked_subjects, + reset_non_bisac_nonfiction_subjects, +) from palace.manager.service.container import Services def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: - return classify_unchecked_subjects.s() + return chain( + reset_non_bisac_nonfiction_subjects.s(), + classify_unchecked_subjects.si(), + ) From dc8c7b9abaf1725d64b9e60c6d3a33e898297532 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 15 Sep 2026 07:39:51 -0700 Subject: [PATCH 21/22] Drop the stale migration reference from the task test The migration this named was removed when the repair moved to a startup task. Co-Authored-By: Claude Opus 5 --- tests/manager/celery/tasks/test_work.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/manager/celery/tasks/test_work.py b/tests/manager/celery/tasks/test_work.py index ce8b680773..5a3236e27b 100644 --- a/tests/manager/celery/tasks/test_work.py +++ b/tests/manager/celery/tasks/test_work.py @@ -125,10 +125,10 @@ def test_reset_non_bisac_nonfiction_subjects( db: DatabaseTransactionFixture, celery_fixture: CeleryFixture, ): - """The task re-applies the reset for subjects stored as nonfiction in error. + """The task resets subjects whose stored nonfiction status went stale. - Re-runnable stand-in for migration 52d1bbdd4671, for when that migration's - reset was consumed by old code before the new classifier was live. + Re-runnable, so the repair can be applied again when its reset was consumed + by old code before the new classifier was live everywhere. """ stale = db.subject(Subject.BISAC, "INFEN000") stale.name = "English literature" From 2d9063235faedf55d6b2a3f45aaafcc030522a1e Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Fri, 18 Sep 2026 13:30:28 -0700 Subject: [PATCH 22/22] Restore the reset predicate's case coverage in the task test Deleting the migration took its test with it, and the Celery task test that replaced it carried four subjects where the migration test had thirteen. The selection logic is the same either way -- both ask `BISACClassifier.contradicts_stored_fiction` -- so the cases port straight across. Back are the offender shapes that were only covered there: the `N`-suffix vendor code, a malformed code with no name at all, a shape-valid but non-existent code (which a pattern-based predicate would wrongly accept), and a stale canonical fiction code. On the other side: a second real nonfiction code, a nonfiction heading in the identifier field, and a row already scored as unknown rather than fiction. Parametrized rather than asserted in a batch, so a failing case does not hide the ones after it. Co-Authored-By: Claude Opus 5 --- tests/manager/celery/tasks/test_work.py | 91 +++++++++++++++++-------- 1 file changed, 62 insertions(+), 29 deletions(-) diff --git a/tests/manager/celery/tasks/test_work.py b/tests/manager/celery/tasks/test_work.py index 5a3236e27b..3a662a7f5a 100644 --- a/tests/manager/celery/tasks/test_work.py +++ b/tests/manager/celery/tasks/test_work.py @@ -1,5 +1,7 @@ from unittest.mock import patch +import pytest + from palace.manager.celery.tasks import work as work_tasks from palace.manager.sqlalchemy.model.classification import Subject from palace.manager.sqlalchemy.model.work import Work @@ -121,45 +123,76 @@ def test_reclassify_null_audience_works( assert policy.choose_edition is False +@pytest.mark.parametrize( + "identifier,name", + [ + pytest.param("INFEN000", "English literature", id="language_category"), + pytest.param( + "INFENUSA", "American and Canadian literature", id="territory_category" + ), + pytest.param("FBSACT000000", "News and investigations", id="vendor_code"), + pytest.param("FSHUM000000N", "Human science", id="vendor_code_with_n_suffix"), + pytest.param("SOCO32000", None, id="malformed_code_without_name"), + pytest.param("FBZZZ000000", "Historical", id="shape_valid_but_nonexistent"), + pytest.param("FBFIC014000", "Historical", id="stale_canonical_fiction_code"), + ], +) def test_reset_non_bisac_nonfiction_subjects( db: DatabaseTransactionFixture, celery_fixture: CeleryFixture, -): - """The task resets subjects whose stored nonfiction status went stale. - - Re-runnable, so the repair can be applied again when its reset was consumed - by old code before the new classifier was live everywhere. + identifier: str, + name: str | None, +) -> None: + """Subjects stored as nonfiction that the classifier no longer scores that + way are marked unchecked, so classify_unchecked_subjects re-scores them. + + Covers both codes that are not BISAC at all and codes that merely look like + one (FBZZZ000000), which a pattern-based predicate would wrongly accept. """ - stale = db.subject(Subject.BISAC, "INFEN000") - stale.name = "English literature" - stale.fiction = False - stale.checked = True - - # A real nonfiction code the classifier still agrees with. - agrees = db.subject(Subject.BISAC, "HIS027000") - agrees.fiction = False - agrees.checked = True - - # Already scored as fiction, so outside the set the task examines. - scored_fiction = db.subject(Subject.BISAC, "INFENUSA") - scored_fiction.name = "American and Canadian literature" - scored_fiction.fiction = True - scored_fiction.checked = True - - # Same identifier, but not a BISAC subject. - other_type = db.subject(Subject.TAG, "INFEN000") - other_type.fiction = False - other_type.checked = True + subject = db.subject(Subject.BISAC, identifier) + subject.name = name + subject.fiction = False + subject.checked = True + db.session.commit() + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + + assert subject.checked is False + + +@pytest.mark.parametrize( + "subject_type,identifier,fiction", + [ + pytest.param(Subject.BISAC, "HIS027000", False, id="real_nonfiction_code"), + pytest.param(Subject.BISAC, "HIS000000", False, id="real_nonfiction_general"), + pytest.param( + Subject.BISAC, "HISTORY / General", False, id="nonfiction_heading" + ), + pytest.param(Subject.BISAC, "INFEN000", True, id="already_scored_fiction"), + pytest.param(Subject.BISAC, "INFEN000", None, id="already_scored_unknown"), + pytest.param(Subject.TAG, "INFEN000", False, id="not_a_bisac_subject"), + ], +) +def test_reset_non_bisac_nonfiction_subjects_leaves_everything_else_checked( + db: DatabaseTransactionFixture, + celery_fixture: CeleryFixture, + subject_type: str, + identifier: str, + fiction: bool | None, +) -> None: + """Legitimate nonfiction codes keep their value, and subjects outside the + examined set -- already scored as fiction or unknown, or not BISAC-typed -- + are not touched.""" + subject = db.subject(subject_type, identifier) + subject.fiction = fiction + subject.checked = True db.session.commit() work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() db.session.expire_all() - assert stale.checked is False - assert agrees.checked is True - assert scored_fiction.checked is True - assert other_type.checked is True + assert subject.checked is True def test_reset_non_bisac_nonfiction_subjects_is_idempotent(