Don't let unrecognized BISAC codes classify fiction as nonfiction (PP-4849)

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

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)
 

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

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",
         [

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
     ):

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().

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,