From 9bbb64c7a163c5cb49577eb98043ba26d87cd460 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Fri, 25 Sep 2026 23:47:29 -0700 Subject: [PATCH] Let a consumer trust its cache instead of re-fetching it Staleness re-fetching suits a cache the tool owns and can rebuild. For a cache that is committed to a repository and reviewed, it inverts: the re-fetch is the risk, not the repair. Three failures downstream are the same re-fetch seen from different angles: dismech#12672 an unstamped full_text_html entry is withheld, and the repair route it points at cannot be reached dismech#12867 a fetch returns PMC's bot-check page on an HTTP 200 and its is compared against the curated title dismech#12879 a re-fetch shortens 64 bodies and breaks snippets that verified, in a pull request about something else Each of those can be attacked on its own -- serve the stale entry anyway, detect the interstitial, refuse a refresh that shrinks a body. I started doing exactly that, and the third of those needs a size heuristic that this project already tried and removed for good reasons. `trust_cached_entries` removes the cause instead. When set, a cache entry is served as it is on disk: no staleness re-fetch and no full-text retry, so there is no fetch during which any of the above can happen. Off by default, so caches still migrate for consumers that want that. An imperfect cached entry stays imperfect, which is the trade. That is a content problem to fix deliberately -- `cache reference --force` still rebuilds one on purpose -- rather than something to re-litigate on every validation run. This supersedes the CAPTCHA-detection guard proposed in #96, which is closed: it recognised the interstitial instead of not asking for it. just test: 1173 passed, 1 skipped; mypy clean; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --- .../etl/fulltext/pmc.py | 14 +++- .../etl/reference_fetcher.py | 29 +++++-- .../etl/sources/pmid.py | 8 +- src/linkml_reference_validator/models.py | 12 +++ tests/test_pmc_modern_container.py | 24 +++++- tests/test_serve_stale_html_optin.py | 59 ++++++++++++++ tests/test_trust_cached_entries.py | 81 +++++++++++++++++++ 7 files changed, 207 insertions(+), 20 deletions(-) create mode 100644 tests/test_trust_cached_entries.py diff --git a/src/linkml_reference_validator/etl/fulltext/pmc.py b/src/linkml_reference_validator/etl/fulltext/pmc.py index 360a49f..6d1c304 100644 --- a/src/linkml_reference_validator/etl/fulltext/pmc.py +++ b/src/linkml_reference_validator/etl/fulltext/pmc.py @@ -46,10 +46,16 @@ class TransientFullTextError(RuntimeError): #: same element. Matching it alone would match every page's ``<body>``, and the #: structural test is the only thing standing between this fetch and caching a #: bot-check interstitial served on an HTTP 200. +#: +#: Order is a priority order, not an alphabetical one: the first match wins, so +#: the current wrapper is tried before the legacy ones. It matters for a legacy +#: page carrying several ``tsec`` sections, where only the first is returned -- +#: the previous code behaved the same way, so this is a known limit rather than +#: a regression, but reordering the tuple would change which section that is. PMC_ARTICLE_BODY_CLASSES = ("main-article-body", "article-body", "tsec") -def _find_pmc_article_body(soup: BeautifulSoup) -> Optional[Any]: +def find_pmc_article_body(soup: BeautifulSoup) -> Optional[Any]: """Return PMC's article-body element, or None if the page has none. Matches on class rather than element name: the container moved from ``div`` @@ -65,9 +71,9 @@ def _find_pmc_article_body(soup: BeautifulSoup) -> Optional[Any]: Examples: >>> from bs4 import BeautifulSoup >>> html = '<section class="body main-article-body"><p>Text.</p></section>' - >>> _find_pmc_article_body(BeautifulSoup(html, "html.parser")) is not None + >>> find_pmc_article_body(BeautifulSoup(html, "html.parser")) is not None True - >>> _find_pmc_article_body(BeautifulSoup("<body><p>x</p></body>", "html.parser")) is None + >>> find_pmc_article_body(BeautifulSoup("<body><p>x</p></body>", "html.parser")) is None True """ for class_name in PMC_ARTICLE_BODY_CLASSES: @@ -192,7 +198,7 @@ def _fetch_pmc_html(self, pmcid: str, config: ReferenceValidationConfig) -> Opti ) soup = BeautifulSoup(response.content, "html.parser") - article_body = _find_pmc_article_body(soup) + article_body = find_pmc_article_body(soup) if article_body: # The region is selected here, but the text comes out of the shared # extractor rather than a private copy of the paragraph walk, so diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 5724d7d..af0a6d1 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -343,7 +343,18 @@ def fetch_with_provenance( # Check disk cache if not force_refresh: - cached = self._load_from_disk(normalized_reference_id) + cached = self._load_from_disk( + normalized_reference_id, + allow_stale=self.config.trust_cached_entries, + allow_stale_html=self.config.trust_cached_entries, + ) + if cached and self.config.trust_cached_entries: + # Trusted: serve what is on disk. No staleness re-fetch and no + # full-text retry, so nothing can shorten the body or replace it + # with a bot-check page. + return self._remember( + normalized_reference_id, FetchOutcome(content=cached) + ) if cached: # A record cached as abstract_only may predate full-text support, or # reflect a prior transient failure. Give the chain one more chance @@ -521,9 +532,12 @@ def _preserve_cached_full_text( cached.content_type, ) - # May its text be served? Asked without the bypass, so stale HTML is - # withheld here exactly as _stale_fallback withholds it, and validation - # falls back to the freshly fetched abstract. + # May its text be served? Asked with the consumer's serve policy, the + # same one _stale_fallback uses. By default stale HTML is withheld and + # validation falls back to the freshly fetched abstract; a consumer that + # has opted in gets the cached body instead. This is the path a refresh + # normally takes -- the source still answers, with an abstract -- so it + # is the one most consumers see, not _stale_fallback. servable = self._load_from_disk( normalized_reference_id, allow_stale=True, @@ -1536,8 +1550,11 @@ def _load_from_disk( ): logger.warning( "Refusing stale HTML full text for %s: it may be a repository " - "landing page. Retry when the source serves full text again to " - "repair it.", + "landing page. A re-fetch repairs it where the source still " + "serves full text -- but not for a PMC-only article, which " + "answers a Python HTTP client with a bot-check interstitial on " + "an HTTP 200. Set serve_stale_html_full_text if this cache is " + "one you review.", reference_id, ) return None diff --git a/src/linkml_reference_validator/etl/sources/pmid.py b/src/linkml_reference_validator/etl/sources/pmid.py index d2439a3..fff79e4 100644 --- a/src/linkml_reference_validator/etl/sources/pmid.py +++ b/src/linkml_reference_validator/etl/sources/pmid.py @@ -23,6 +23,7 @@ from bs4 import BeautifulSoup # type: ignore import requests # type: ignore +from linkml_reference_validator.etl.fulltext.pmc import find_pmc_article_body from linkml_reference_validator.models import ReferenceContent, ReferenceValidationConfig from linkml_reference_validator.etl.extract.html import HTMLExtractor from linkml_reference_validator.etl.extract import MIN_FULLTEXT_CHARS @@ -582,12 +583,7 @@ def _fetch_pmc_html( return None soup = BeautifulSoup(response.content, "html.parser") - # Shared with the PMC full-text provider: the container moved from a - # ``div`` to a ``section`` and changed class, so both call sites went - # blind at once (dismech#12672). - from linkml_reference_validator.etl.fulltext.pmc import _find_pmc_article_body - - article_body = _find_pmc_article_body(soup) + article_body = find_pmc_article_body(soup) if article_body: # Region selected here, text extracted by the shared extractor, for diff --git a/src/linkml_reference_validator/models.py b/src/linkml_reference_validator/models.py index 69fad13..ebc6106 100644 --- a/src/linkml_reference_validator/models.py +++ b/src/linkml_reference_validator/models.py @@ -524,6 +524,18 @@ class ReferenceValidationConfig(BaseModel): "the entry is left un-stamped so a later run still retries." ), ) + trust_cached_entries: bool = Field( + default=False, + description=( + "If True, use a cache entry as it is on disk and never re-fetch it " + "because an extractor stamp is missing or old. Off by default, so " + "caches still migrate. Set it for a committed, reviewed cache, where " + "a re-fetch is not an improvement but a risk: it can shorten a body, " + "cache a bot-check page, or replace text a curator quoted. Imperfect " + "cached text is a content problem to fix deliberately, not on every " + "validation run." + ), + ) full_text_providers: list[str] = Field( default_factory=lambda: ["pmc", "epmc_preprint", "unpaywall", "openalex"], description=( diff --git a/tests/test_pmc_modern_container.py b/tests/test_pmc_modern_container.py index 8944c5d..00ace8a 100644 --- a/tests/test_pmc_modern_container.py +++ b/tests/test_pmc_modern_container.py @@ -18,7 +18,7 @@ import pytest from bs4 import BeautifulSoup -from linkml_reference_validator.etl.fulltext.pmc import _find_pmc_article_body +from linkml_reference_validator.etl.fulltext.pmc import find_pmc_article_body BODY = "<p>" + ("Real article prose. " * 40) + "</p>" @@ -34,7 +34,7 @@ ) def test_article_containers_are_found(markup, label): soup = BeautifulSoup(markup, "html.parser") - assert _find_pmc_article_body(soup) is not None, f"{label} should be recognised" + assert find_pmc_article_body(soup) is not None, f"{label} should be recognised" @pytest.mark.parametrize( @@ -49,11 +49,27 @@ def test_article_containers_are_found(markup, label): ) def test_pages_without_an_article_container_are_declined(markup, label): soup = BeautifulSoup(markup, "html.parser") - assert _find_pmc_article_body(soup) is None, f"{label} must not be accepted" + assert find_pmc_article_body(soup) is None, f"{label} must not be accepted" def test_a_bare_body_element_is_not_an_article_container(): """``<body>`` is on every page, so matching class="body" alone would accept an interstitial. Only the article-body classes count.""" soup = BeautifulSoup(f"<html><body>{BODY}</body></html>", "html.parser") - assert _find_pmc_article_body(soup) is None + assert find_pmc_article_body(soup) is None + + +def test_a_challenge_page_carrying_a_classed_body_is_still_declined(): + """The interstitial arrives on an HTTP 200, so this selector is the only check. + + PMC's challenge page carries a classed ``<body>``. None of its classes may + overlap the article-body list, or a bot-check page gets cached as full text. + """ + markup = ( + '<html><body class="usa-page bot-check">' + "<h1>Checking your browser before accessing</h1>" + "<p>Enable JavaScript and cookies to continue.</p>" + "</body></html>" + ) + soup = BeautifulSoup(markup, "html.parser") + assert find_pmc_article_body(soup) is None diff --git a/tests/test_serve_stale_html_optin.py b/tests/test_serve_stale_html_optin.py index d0a587d..7c7dadc 100644 --- a/tests/test_serve_stale_html_optin.py +++ b/tests/test_serve_stale_html_optin.py @@ -22,6 +22,7 @@ from linkml_reference_validator.etl.reference_fetcher import ReferenceFetcher from linkml_reference_validator.models import ( + ReferenceContent, ReferenceValidationConfig, ) @@ -73,3 +74,61 @@ def test_opting_in_does_not_stamp_or_rewrite_the_entry(tmp_path): text = (tmp_path / "PMID_30929739.md").read_text(encoding="utf-8") assert "extractor_version" not in text assert BODY_SENTENCE in text + + +def _fetch_with_abstract_source(tmp_path, **cfg): + """The path a refresh normally takes: the source answers, with an abstract. + + ``_stale_fallback`` needs the source to yield *nothing*. For a PMC-only + article PubMed still returns the abstract, so the refresh reaches + ``_preserve_cached_full_text`` instead: the overwrite is refused, and then a + separate question decides what may be served. That second question is the + one the flag has to answer, and it is the one dismech actually hits. + """ + _cache(tmp_path) + fetcher = ReferenceFetcher( + ReferenceValidationConfig( + cache_dir=tmp_path, email="me@example.org", fetch_full_text=False, **cfg + ) + ) + fresh = ReferenceContent( + reference_id="PMID:30929739", + content="Just the abstract.", + content_type="abstract_only", + title="A paper", + ) + + class _Source: + def fetch(self, identifier, config): + return fresh + + with patch( + "linkml_reference_validator.etl.reference_fetcher" + ".ReferenceSourceRegistry.get_source", + return_value=_Source, + ): + return fetcher.fetch("PMID:30929739") + + +def test_refused_overwrite_serves_the_abstract_by_default(tmp_path): + content = _fetch_with_abstract_source(tmp_path) + assert content is not None + assert BODY_SENTENCE not in (content.content or ""), ( + "by default the cached body stays withheld even though it was preserved" + ) + assert "Just the abstract." in (content.content or "") + + +def test_refused_overwrite_serves_the_cached_body_when_opted_in(tmp_path): + content = _fetch_with_abstract_source(tmp_path, serve_stale_html_full_text=True) + assert content is not None + assert BODY_SENTENCE in (content.content or "") + + +def test_refused_overwrite_leaves_the_file_alone_either_way(tmp_path): + """Serving is not overwriting: the entry keeps its body and stays un-stamped.""" + for flag in (False, True): + _fetch_with_abstract_source(tmp_path, serve_stale_html_full_text=flag) + text = (tmp_path / "PMID_30929739.md").read_text(encoding="utf-8") + assert BODY_SENTENCE in text, f"body lost with flag={flag}" + assert "extractor_version" not in text, f"entry stamped with flag={flag}" diff --git a/tests/test_trust_cached_entries.py b/tests/test_trust_cached_entries.py new file mode 100644 index 0000000..a1a3692 --- /dev/null +++ b/tests/test_trust_cached_entries.py @@ -0,0 +1,81 @@ +"""A committed cache can be used as it is, without a re-fetch on every run. + +Staleness re-fetching is right for a cache the tool owns and can rebuild. For a +cache that is committed and reviewed it inverts: the re-fetch is the risk, not +the repair. It can shorten a body (dismech#12879), replace an entry with a +bot-check page served on an HTTP 200 (dismech#12867), or withhold text that +nothing can recover (dismech#12672). + +`trust_cached_entries` says use the file. Off by default, so caches still +migrate for consumers that want that. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from linkml_reference_validator.etl.reference_fetcher import ReferenceFetcher +from linkml_reference_validator.models import ReferenceValidationConfig + +BODY = "A sentence a curator quoted, present only in this cached body." + + +def _cache(tmp_path, content_type="full_text_html"): + (tmp_path / "PMID_20301575.md").write_text( + "---\nreference_id: PMID:20301575\ntitle: A paper\n" + f"content_type: {content_type}\n---\n\n## Content\n\n{BODY}\n", + encoding="utf-8", + ) + + +def _fetch(tmp_path, trust): + fetcher = ReferenceFetcher( + ReferenceValidationConfig( + cache_dir=tmp_path, email="me@example.org", trust_cached_entries=trust + ) + ) + calls = [] + + class _Source: + def fetch(self, identifier, config): + calls.append(identifier) + return None + + with patch( + "linkml_reference_validator.etl.reference_fetcher" + ".ReferenceSourceRegistry.get_source", + return_value=_Source, + ): + return fetcher.fetch("PMID:20301575"), calls + + +@pytest.mark.parametrize( + "content_type", ["full_text_html", "full_text_xml", "abstract_only"] +) +def test_a_trusted_entry_is_served_without_any_fetch(tmp_path, content_type): + """The point: no network call at all, so nothing can go wrong during one.""" + _cache(tmp_path, content_type) + content, calls = _fetch(tmp_path, trust=True) + + assert content is not None, f"{content_type} should be served from disk" + assert BODY in (content.content or "") + assert calls == [], f"a trusted entry must not be re-fetched, got {calls}" + + +def test_the_file_is_left_exactly_as_it_was(tmp_path): + """Nothing is rewritten, so a validation run leaves the cache untouched.""" + _cache(tmp_path) + before = (tmp_path / "PMID_20301575.md").read_text(encoding="utf-8") + _fetch(tmp_path, trust=True) + assert (tmp_path / "PMID_20301575.md").read_text(encoding="utf-8") == before + + +def test_the_default_still_re_fetches(tmp_path): + """Off by default: an unstamped entry is still treated as stale.""" + _cache(tmp_path) + _content, calls = _fetch(tmp_path, trust=False) + assert calls == ["20301575"], ( + "by default an unstamped entry is re-fetched; the flag must be opt-in" + )