Skip to content

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

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

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
@dbernstein

Copy link
Copy Markdown
Contributor Author

Superseded by #3521, which moves this branch to an upstream branch stacked on #3520 (and additionally removes the now-orphaned BaseCoverageRecord mixin).

@dbernstein dbernstein closed this Jun 30, 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