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..a7ad804ea5 --- /dev/null +++ b/alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py @@ -0,0 +1,101 @@ +"""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. + +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() + + 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/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index 57c19aee63..be31bd8694 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -654,25 +654,61 @@ 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 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. + """ + 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): - 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..255525fc0b 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -392,6 +392,90 @@ 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 + + @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. + + 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..f357bbb872 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 + ) -> None: + """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..a78938de4b 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 + ) -> 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 + # 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(). diff --git a/tests/migration/conftest.py b/tests/migration/conftest.py index 036005dfdf..cbb00d7a1b 100644 --- a/tests/migration/conftest.py +++ b/tests/migration/conftest.py @@ -326,6 +326,46 @@ 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 new file mode 100644 index 0000000000..4852440b0f --- /dev/null +++ b/tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py @@ -0,0 +1,80 @@ +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