Retire the CoverageProvider machinery (PP-4468)#3520
Conversation
|
Claude finished @dbernstein's task in 7m 6s —— View job Code ReviewI reviewed the diff against SummaryThe retirement is sound and the inline replacement is faithful to what it replaces. I verified: the availability endpoint's real behavior is unchanged ( DetailsMinor:
|
dbernstein
force-pushed
the
chore/retire-coverage-provider
branch
from
8f4574b to
27bf685
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| bibliographic.apply( | ||
| self._db, | ||
| edition, | ||
| self.collection, | ||
| replace=ReplacementPolicy.from_license_source(), | ||
| ) | ||
| except Exception as e: | ||
| # Mirror the resilience of the former | ||
| # ``BibliographicCoverageProvider.set_bibliographic``: a database | ||
| # error or unexpected data shape during apply should not crash | ||
| # ``update_licensepool``. Log and leave the pool without updated | ||
| # coverage, matching the prior "silently continue" behavior. | ||
| self.log.warning( | ||
| "Error applying Overdrive bibliographic data to edition %s: %s", | ||
| edition.id, | ||
| e, | ||
| exc_info=e, | ||
| ) | ||
| return |
There was a problem hiding this comment.
metadata_lookup exceptions propagate outside the guard
The except Exception block covers only bibliographic.apply(). A transient HTTP error or malformed JSON response from metadata_lookup() (lines 1812–1813) will propagate to update_licensepool and abort the entire availability update, preventing update_licensepool_with_book_info from running. This matches the old behaviour — the retired process_batch also did not wrap process_item in a try/except — so this is not a regression, but it is worth calling out explicitly because the guard communicates an intent ("bibliographic failures must not crash availability updates") that isn't fully satisfied if the fetch itself throws. Consider wrapping metadata_lookup in the same guard, or adding a dedicated try/except around just that call with a return on failure.
| if isinstance(work, ProblemDetail): | ||
| return work | ||
|
|
||
| if provider is None: | ||
| return METADATA_REFRESH_FAILURE | ||
|
|
||
| assert work.presentation_edition is not None | ||
| primary_identifier = work.presentation_edition.primary_identifier | ||
| try: | ||
| record = provider.ensure_coverage(primary_identifier, force=True) | ||
| except Exception: | ||
| # The coverage provider may raise an HTTPIntegrationException. | ||
| return REMOTE_INTEGRATION_FAILED | ||
|
|
||
| if record.exception: | ||
| # There was a coverage failure. | ||
| if str(record.exception).startswith("201") or str( | ||
| record.exception | ||
| ).startswith("202"): | ||
| # A 201/202 error means it's never looked up this work before | ||
| # so it's started the resolution process or looking for sources. | ||
| return METADATA_REFRESH_PENDING | ||
| # Otherwise, it just doesn't know anything. | ||
| return METADATA_REFRESH_FAILURE | ||
|
|
||
| return Response("", 200) | ||
| return METADATA_REFRESH_FAILURE | ||
|
|
||
| def classifications( | ||
| self, identifier_type: str, identifier: str |
There was a problem hiding this comment.
load_work is called even though the result is always discarded
The method now calls self.load_work(library, identifier_type, identifier) (and its auth check) before returning METADATA_REFRESH_FAILURE unconditionally. Since the endpoint never performs a refresh, the work lookup is dead work on every request. The auth check (require_librarian) is still valuable, but the load_work call could be removed to avoid the unnecessary database query.
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!
|
@dbernstein it looks like tests are failing on this one, and greptile has a valid review comment about unconditionally calling I'll let you resolve those issues before reviewing this one. When its ready you can request a review from @ThePalaceProject/backend again. |
jonathangreen
left a comment
jonathangreen
left a comment
There was a problem hiding this comment.
Some comments on this for consideration while you are resolving the test issues
| Metadata refresh used to run through the per-source CoverageProvider | ||
| machinery, which has been retired. No provider is wired up, so this | ||
| endpoint is retained for API compatibility but no longer performs a | ||
| refresh; it always reports failure. |
There was a problem hiding this comment.
Since this does nothing can we deprecate it, or just remove it entirely? What calls this endpoint? I think we need a plan to deal with this endpoint since its no longer functional.
| This replaces the former ``OverdriveBibliographicCoverageProvider`` | ||
| path that ran inside :meth:`update_licensepool`. A bad or unrecognized | ||
| Overdrive ID simply leaves the pool without a Work, matching the prior | ||
| behavior (``update_licensepool`` ignored the coverage result). |
There was a problem hiding this comment.
I find referring to old removed code in a new code like this does not add much value. Claude likes to do this a lot.
If anything it causes confusion as you have to go back in git history looking for OverdriveBibliographicCoverageProvider to try to understand the comment. This should just tell us what we are currently doing not give a history lesson.
| # Mirror the resilience of the former | ||
| # ``BibliographicCoverageProvider.set_bibliographic``: a database | ||
| # error or unexpected data shape during apply should not crash | ||
| # ``update_licensepool``. Log and leave the pool without updated | ||
| # coverage, matching the prior "silently continue" behavior. |
There was a problem hiding this comment.
Again we should explain why we are doing this, not talk about old removed code in the comment.
| self.collection, | ||
| replace=ReplacementPolicy.from_license_source(), | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
I really really don't like broad exceptions like this. We should avoid them as much as we possibly can. They catch and mask so much, like this would catch and hide a key name typo in the code.
| ): | ||
| # An exception raised while applying bibliographic data is caught and | ||
| # logged rather than propagating out of update_licensepool, mirroring | ||
| # the resilience of the retired BibliographicCoverageProvider. |
There was a problem hiding this comment.
Comment referring to removed code
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>
Restore the resilience of the retired BibliographicCoverageProvider: catch and log exceptions from bibliographic.apply in _ensure_bibliographic_coverage so a database error or unexpected data shape during apply does not propagate out of update_licensepool. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: the _ensure_bibliographic_coverage docstring, its exception-handling comment, and the matching test comment described the retired CoverageProvider classes instead of what the code does now. Rewrite them to explain the current behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: a bare `except Exception` around bibliographic.apply masks programming errors (a mistyped attribute raising AttributeError/KeyError would be silently swallowed). Catch BasePalaceException instead -- apply raises PalaceValueError for invalid data -- so genuine bugs propagate while a single bad title still does not abort the availability update. Update the guard test to raise PalaceValueError and add a test that an unexpected error propagates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Greptile feedback: refresh_metadata always returns METADATA_REFRESH_FAILURE, so loading the Work only to discard it was a needless database query on every request. Keep the librarian authorization check and return the failure directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dbernstein
force-pushed
the
chore/retire-coverage-provider
branch
from
27bf685 to
ac18fb5
Compare
test_ensure_bibliographic_coverage_apply_error asserted on caplog.text without setting a capture level, so it relied on the ambient root log level. Under pytest-randomly, a prior test that raises that level leaves the warning uncaptured, so caplog.text is empty and the assertion fails. Set the level explicitly with caplog.set_level(LogLevel.warning), matching the convention used by other caplog tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3520 +/- ##
==========================================
+ Coverage 93.45% 93.53% +0.07%
==========================================
Files 511 508 -3
Lines 46494 45847 -647
Branches 6346 6239 -107
==========================================
- Hits 43450 42881 -569
+ Misses 1968 1915 -53
+ Partials 1076 1051 -25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Description
Retires the legacy
CoverageProvidermachinery. Overdrive bibliographic coverage now runs as a direct metadata-apply step insideOverdriveAPI.update_licensepoolinstead of routing throughOverdriveBibliographicCoverageProvider.ensure_coverage. The baseCoverageProviderclasses, the Overdrive/Bibliotheca providers, the coverage-provider runner scripts, and the now-dead readers (Identifier.missing_coverage_from, theExplainscript's coverage block, the always-failing adminrefresh_metadataprovider path) are deleted.BibliographicData.applyno longer writesCoverageRecordrows (thecreate_coverage_recordparameter is removed; the Celery apply task and the Bibliotheca updater already passedcreate_coverage_record=False).The new
_ensure_bibliographic_coveragehelper wrapsbibliographic.apply(...)in a broadexcept Exceptionthat logs a warning and returns, preserving the resilience of the retiredBibliographicCoverageProvider.set_bibliographicso a database error or unexpected data shape during apply does not propagate out ofupdate_licensepool.The
CoverageRecordmodel andcoveragerecordstable are intentionally left in place (marked dormant) for one release, per the online-migration / N-1 rule. A stacked follow-up PR drops thecoveragerecordsand dormantequivalentscoveragerecordstables.Motivation and Context
JIRA (PP-4468)
The
CoverageProviderbatch-processing pattern is being retired in favor of Celery. Investigation showed nothing live reads coverage rows:ensure_coverage(Overdrive'supdate_licensepool) passesforce=Trueand ignores the result.refresh_metadataendpoint is wired with no provider, so it always returnsMETADATA_REFRESH_FAILUREwithout invoking a provider.Identifier.missing_coverage_from/should_update/ theExplaincoverage block are only reachable through the already-dead runner scripts.The Celery
bibliographic_applytask and the Bibliotheca updater already skip coverage-record writes, so Overdrive was the last writer.How Has This Been Tested?
mypyandruffpass on all changed files.tox -e py312-docker: Overdrive API including the rewrittenupdate_licensepoolbibliographic-coverage path and a new test covering thebibliographic.applyerror guard, the Bibliotheca circulation updater, data-layerapply, and the affected model tests.Checklist
🤖 Generated with Claude Code