Skip to content

Drop coveragerecords and equivalentscoveragerecords tables (PP-4653)#3504

Draft
dbernstein wants to merge 2 commits into
ThePalaceProject:mainfrom
dbernstein:chore/drop-coverage-tables
Draft

Drop coveragerecords and equivalentscoveragerecords tables (PP-4653)#3504
dbernstein wants to merge 2 commits into
ThePalaceProject:mainfrom
dbernstein:chore/drop-coverage-tables

Conversation

@dbernstein

@dbernstein dbernstein commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Description

Stacked follow-up to #3503 ("Retire the CoverageProvider machinery"). Now that no running code reads or writes the coverage tables, this PR removes the CoverageRecord and (already-dormant) EquivalencyCoverageRecord models, the coverage_records relationships on Identifier/DataSource/Collection, and drops the coveragerecords and equivalentscoveragerecords tables along with their shared coverage_status enum.

Timestamp, its separate service_type enum, and the BaseCoverageRecord mixin (still imported by the workcoveragerecords-removal migration for status_enum) are retained.

Motivation and Context

JIRA (PP-4653)
This is the "release 2" half of the CoverageProvider retirement. Per the online-migration / N-1 rule, the tables could not be dropped in the same release that stopped using them (#3503) — N-1 app servers still wrote to them during a rolling deploy.

⚠️ Draft until the release containing #3503 has shipped. Merging earlier would break N-1 webservers that still write to these tables.

How Has This Been Tested?

  • mypy and ruff pass on all changed files.
  • The migration round-trips on a real Postgres: upgrade drops both tables and the coverage_status enum (leaving timestamps / service_type intact); downgrade recreates both tables with identical indexes, constraints, and foreign keys; a second upgrade drops them again.
  • The full tests/migration/ suite passes under tox -e py312-docker (incl. pytest-alembic single-head / up-down-consistency / model-matches-DDL checks and a new drop test), along with the Timestamp, collection-deletion, identifier, and data-layer apply tests.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

🤖 Generated with Claude Code

dbernstein and others added 2 commits June 24, 2026 15:37
Apply Overdrive bibliographic metadata directly instead of routing it through
the CoverageProvider machinery, then delete the now-dead machinery.

Nothing read the coverage rows: the only live caller of ensure_coverage was
OverdriveAPI.update_licensepool (force=True, return value ignored), and the
admin refresh_metadata endpoint was wired with no provider. The Celery apply
task and the Bibliotheca updater already skipped coverage-record writes, so
Overdrive was the last writer.

- Rewrite OverdriveAPI.update_licensepool to fetch + apply metadata and make
  the work presentation-ready via a new _ensure_bibliographic_coverage helper,
  replacing OverdriveBibliographicCoverageProvider.ensure_coverage.
- Stop writing coverage records: drop create_coverage_record from
  BibliographicData.apply and its call sites.
- Delete core/coverage.py, the Overdrive and Bibliotheca coverage providers,
  the coverage-provider runner scripts, Identifier.missing_coverage_from, and
  the Explain script's coverage block.
- Neutralize the now-dead admin refresh_metadata endpoint.
- Leave the CoverageRecord model and coveragerecords table in place (dormant)
  for one release; the drop migration follows in a stacked PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to "Retire the CoverageProvider machinery": now that no running code
reads or writes the coverage tables, remove the CoverageRecord and
EquivalencyCoverageRecord models, their relationships, and the tables themselves
along with the shared coverage_status enum.

- Remove CoverageRecord and EquivalencyCoverageRecord from
  sqlalchemy/model/coverage.py and the Identifier/DataSource/Collection
  coverage_records relationships.
- Add a migration dropping coveragerecords and equivalentscoveragerecords and
  the now-orphaned coverage_status enum; the downgrade recreates them.
- Keep the Timestamp model, its service_type enum, and the BaseCoverageRecord
  mixin (still referenced by the workcoveragerecords-removal migration).
- Remove the obsolete CoverageRecord tests and the db.coverage_record fixture.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dbernstein dbernstein added the DB migration This PR contains a DB migration label Jun 24, 2026
@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This is the "Release 2" cleanup of the CoverageProvider retirement: it drops the coveragerecords and equivalentscoveragerecords tables and their shared coverage_status Postgres enum, removes the CoverageRecord/EquivalencyCoverageRecord SQLAlchemy models, deletes core/coverage.py and the coverage provider scripts, and strips all coverage_records relationships from Identifier/DataSource/Collection. BaseCoverageRecord and Timestamp are intentionally kept because the existing workcoveragerecords-removal migration still imports status_enum from BaseCoverageRecord.

  • Migration (20260624_5ca948100902) correctly drops indexes → tables → enum in upgrade, and reverses in downgrade with manual create_type=False lifecycle management; the checkfirst=False flag is intentional.
  • Overdrive API replaces the retired OverdriveBibliographicCoverageProvider with an inline _ensure_bibliographic_coverage helper that fetches metadata, applies it to the edition, and sets the work presentation-ready — preserving the old behavior without the coverage-record side effect.
  • refresh_metadata admin endpoint now unconditionally returns METADATA_REFRESH_FAILURE (no provider is wired up); the docstring and updated test explicitly document this intentional behavioral stub.

Confidence Score: 4/5

Safe to merge once the release containing #3503 has shipped; the draft guard in the PR description is the only merge blocker.

The change is a well-scoped cleanup: dead tables are dropped in the correct release slot per the N-1 convention, all code references to the removed models have been cleaned up, and the migration round-trips correctly. The only finding is a stale one-liner docstring on BaseCoverageRecord that still mentions the now-deleted CoverageRecord class.

src/palace/manager/sqlalchemy/model/coverage.py — stale docstring on BaseCoverageRecord; all other files look clean.

Important Files Changed

Filename Overview
alembic/versions/20260624_5ca948100902_drop_coveragerecords_and_.py New migration that drops coveragerecords, equivalentscoveragerecords, and the coverage_status enum; downgrade recreates all three. create_type=False and checkfirst=False are correctly paired; enum lifecycle is manually managed.
src/palace/manager/sqlalchemy/model/coverage.py Removed CoverageRecord and EquivalencyCoverageRecord models; retained BaseCoverageRecord (needed by the workcoveragerecords migration) and Timestamp. Minor stale docstring on BaseCoverageRecord.
src/palace/manager/integration/license/overdrive/api.py Replaced OverdriveBibliographicCoverageProvider.ensure_coverage call with a new inline _ensure_bibliographic_coverage method; correctly fetches metadata, applies to edition, and sets work presentation-ready.
src/palace/manager/api/admin/controller/work_editor.py refresh_metadata now always returns METADATA_REFRESH_FAILURE; provider parameter removed; behavior is intentional and documented in the updated docstring.
src/palace/manager/data_layer/bibliographic.py Removed create_coverage_record parameter from BibliographicData.apply and the CoverageRecord.add_for call inside it.
src/palace/manager/core/coverage.py Entire file deleted; all CoverageProvider classes (BaseCoverageProvider, IdentifierCoverageProvider, CollectionCoverageProvider, BibliographicCoverageProvider, CoverageFailure) removed along with the module.
tests/migration/test_20260624_5ca948100902_drop_coverage_tables.py New migration test that steps down one revision, verifies both tables exist, then steps back up and verifies they are dropped while timestamps remains.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant OD as OverdriveAPI
    participant ECB as _ensure_bibliographic_coverage (new)
    participant ME as metadata_lookup
    participant ORE as OverdriveRepresentationExtractor
    participant BD as BibliographicData.apply
    participant LP as LicensePool

    OD->>OD: update_licensepool()
    OD->>ECB: _ensure_bibliographic_coverage(license_pool)
    ECB->>ME: metadata_lookup(identifier)
    ME-->>ECB: info dict
    alt errorCode in (NotFound, InvalidGuid)
        ECB-->>OD: return (log warning)
    else
        ECB->>ORE: book_info_to_bibliographic(info)
        ORE-->>ECB: BibliographicData
        ECB->>BD: apply(db, edition, collection, replace)
        BD-->>ECB: (edition, changed)
        alt work missing or not presentation_ready
            ECB->>LP: calculate_work()
            LP-->>ECB: work
            ECB->>LP: work.set_presentation_ready()
        end
    end
    ECB-->>OD: return
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant OD as OverdriveAPI
    participant ECB as _ensure_bibliographic_coverage (new)
    participant ME as metadata_lookup
    participant ORE as OverdriveRepresentationExtractor
    participant BD as BibliographicData.apply
    participant LP as LicensePool

    OD->>OD: update_licensepool()
    OD->>ECB: _ensure_bibliographic_coverage(license_pool)
    ECB->>ME: metadata_lookup(identifier)
    ME-->>ECB: info dict
    alt errorCode in (NotFound, InvalidGuid)
        ECB-->>OD: return (log warning)
    else
        ECB->>ORE: book_info_to_bibliographic(info)
        ORE-->>ECB: BibliographicData
        ECB->>BD: apply(db, edition, collection, replace)
        BD-->>ECB: (edition, changed)
        alt work missing or not presentation_ready
            ECB->>LP: calculate_work()
            LP-->>ECB: work
            ECB->>LP: work.set_presentation_ready()
        end
    end
    ECB-->>OD: return
Loading

Comments Outside Diff (1)

  1. src/palace/manager/sqlalchemy/model/coverage.py, line 33-34 (link)

    P2 The class docstring still references CoverageRecord, which no longer exists after this PR removes the model.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Drop coveragerecords and equivalentscove..." | Re-trigger Greptile

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.56%. Comparing base (79681e2) to head (95ebc82).

Files with missing lines Patch % Lines
...alace/manager/integration/license/overdrive/api.py 58.82% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3504      +/-   ##
==========================================
+ Coverage   93.48%   93.56%   +0.07%     
==========================================
  Files         511      508       -3     
  Lines       46570    45808     -762     
  Branches     6339     6221     -118     
==========================================
- Hits        43538    42860     -678     
+ Misses       1968     1913      -55     
+ Partials     1064     1035      -29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbernstein dbernstein changed the title Drop coveragerecords and equivalentscoveragerecords tables Drop coveragerecords and equivalentscoveragerecords tables (PP-4653) Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DB migration This PR contains a DB migration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant