From ef09282df8e01fc7f4fdbc56ffc1ee17b23cf337 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 2 Sep 2026 10:34:14 -0700 Subject: [PATCH 1/5] 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 56145929b1d3d460221b67845680e07a04049be9 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 2 Sep 2026 10:34:29 -0700 Subject: [PATCH 2/5] 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 b65d045cae9d8ea6a69189ed6b8cc9c5daf0444e Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 8 Sep 2026 08:57:36 -0700 Subject: [PATCH 3/5] 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 7535f2463b3adfecd48a2dcd8e6a24437e037e8a Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 8 Sep 2026 08:57:47 -0700 Subject: [PATCH 4/5] 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 6c69e227ea22b2ac46111121ec4ef72279b9cb43 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 8 Sep 2026 09:06:27 -0700 Subject: [PATCH 5/5] 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