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..de76d8cfc2 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,54 @@ 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: + """Mark BISAC subjects unchecked when their stored fiction status went stale. + + 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. + + 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 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 = ( + 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..2d33297957 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 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. + """ + + 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/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..d8b29c1e21 --- /dev/null +++ b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py @@ -0,0 +1,54 @@ +"""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).""" + +from __future__ import annotations + +import logging + +from celery.canvas import Signature, chain +from sqlalchemy.orm import Session + +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 chain( + reset_non_bisac_nonfiction_subjects.s(), + classify_unchecked_subjects.si(), + ) diff --git a/tests/manager/celery/tasks/test_work.py b/tests/manager/celery/tasks/test_work.py index d7923c5fc5..5a3236e27b 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 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. + """ + 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