From d5794449db86a88d75bf5aedb5b2e8a1a9a72003 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Sun, 20 Sep 2026 22:56:12 -0700 Subject: [PATCH 1/3] Read OtherAbstract, and re-test the entries that claim no content (#88) A refresh could delete the abstract a cache entry already held. `_parse_abstract` read only `Abstract`, so a record whose abstract PubMed keeps under `OtherAbstract` came back as having no content at all, and the refresh overwrote the cached text with an empty body. `PMID:5697815` went from 1787 to 850 bytes and `PMID:6680124` from 1695 to 650, losing 967 and 1175 characters of real abstract. `OtherAbstract` is where PubMed keeps abstracts contributed by other indexing programs -- PIP, KIE, NASA, AIDS -- mostly on pre-1990 records. They are genuine abstracts, not metadata. Three rules the tests pin: - `Abstract` still wins when a record has both; the fallback is a fallback. - English is preferred. `OtherAbstract` also carries translations, so taking the first element would return the French on a record that has both, and an English snippet would stop matching its own source. - A translation is still used when it is the only text available. Reporting nothing blanks the entry, and the text is at least the paper's own summary. Fixing the extractor does not by itself recover anything, which is the other half of this change. An entry damaged by the bug carries a current `extractor_version`, so `_is_stale_cache_entry` calls it fresh and it is never re-fetched -- the text stays deleted permanently. `ABSENT_CONTENT_CACHE_VERSION` versions the claim "this reference has no content at all", scoped to `content_type: unavailable` exactly as `html_full_text_version` and `xml_extraction_version` are scoped to theirs. Such an entry records what an extractor could *not* find, so an extractor fix can make it wrong; the stamp is what lets a fixed version re-test it. Scoped rather than bumping `EXTRACTOR_CACHE_VERSION` because only entries claiming no content can be wrong in this way. In the dismech cache that is 1,735 files against 57,551 -- a blanket bump would re-fetch every cached reference to find them, and churn on that scale is its own harm. Verified against the four PubMed records from the report: both PIP abstracts come back at full length, and `PMID:4869291` and `PMID:37769103` still correctly report nothing, having no abstract element of any kind. Those two are why the fallback is not simply "never demote to unavailable" -- sometimes the demotion is right, and their old cache entries were storing the MEDLINE citation header as though it were content. Full suite: 1149 passed, 1 skipped. Co-Authored-By: Claude Opus 5 (1M context) --- .../etl/reference_fetcher.py | 22 ++++ .../etl/sources/pmid.py | 82 ++++++++++--- tests/test_absent_abstract_recovery.py | 111 ++++++++++++++++++ tests/test_sources.py | 104 ++++++++++++++++ 4 files changed, 305 insertions(+), 14 deletions(-) create mode 100644 tests/test_absent_abstract_recovery.py diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 4b7a3d6..baa78b6 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -93,6 +93,18 @@ #: XML table extraction changes only XML caches, independent of HTML acceptance. XML_EXTRACTION_CACHE_VERSION = 1 +#: Version the claim "this reference has no content at all", scoped to +#: ``content_type: unavailable``. Such an entry records what an extractor could +#: not find, so an extractor fix can make it wrong -- and because the entry +#: carries a current ``extractor_version`` it would otherwise never be re-tested +#: and the text would stay lost. Version 1: reading ``OtherAbstract``, where +#: PubMed keeps PIP/KIE/NASA/AIDS abstracts; before it, such a record was stored +#: as having no content and a refresh deleted the abstract already cached for it +#: (issue #88). Scoped rather than bumping EXTRACTOR_CACHE_VERSION because only +#: entries claiming no content can be wrong in this way, and a blanket bump +#: would re-fetch every cached reference to find them. +ABSENT_CONTENT_CACHE_VERSION = 1 + #: A cache file's frontmatter delimiter: a line that is exactly ``---``. #: Splitting on the bare string instead lets any *value* containing ``---`` - a #: URL reference_id, a title - truncate the block, which loses every field after @@ -1307,6 +1319,8 @@ def _save_to_disk( xml_version = (reference.metadata or {}).get("xml_extraction_version") if reference.content_type == "full_text_xml" and type(xml_version) is int: lines.append(f"xml_extraction_version: {xml_version}") + if reference.content_type == "unavailable": + lines.append(f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}") if reference.title: lines.append(f"title: {self._quote_yaml_value(reference.title)}") if reference.authors: @@ -1639,6 +1653,14 @@ def _is_stale_cache_entry(cls, content_text: str) -> bool: if type(xml_version) is not int or xml_version < XML_EXTRACTION_CACHE_VERSION: return True + if isinstance(metadata, dict) and metadata.get("content_type") == "unavailable": + absent_version = metadata.get("absent_content_version") + if ( + type(absent_version) is not int + or absent_version < ABSENT_CONTENT_CACHE_VERSION + ): + return True + # A newer stamp is not stale: an older tool reading a cache written by a # newer one should leave it alone rather than re-fetch it on every run. return False diff --git a/src/linkml_reference_validator/etl/sources/pmid.py b/src/linkml_reference_validator/etl/sources/pmid.py index 99a2177..aedffc8 100644 --- a/src/linkml_reference_validator/etl/sources/pmid.py +++ b/src/linkml_reference_validator/etl/sources/pmid.py @@ -204,13 +204,54 @@ def _parse_authors(self, author_list: list) -> list[str]: """ return [str(author) for author in author_list if author] + @staticmethod + def _render_abstract_sections(element: Any) -> Optional[str]: + """Join an abstract element's ``AbstractText`` nodes into prose. + + Structured abstracts label each section (e.g. ``Label="METHODS"``); the + label is preserved as a ``"METHODS:"`` prefix and sections are joined by + blank lines, mirroring how PubMed renders them as text. + + Args: + element: An ``Abstract`` or ``OtherAbstract`` node + + Returns: + The joined prose, or None if the element carries no text + + Examples: + >>> from bs4 import BeautifulSoup + >>> xml = ''' + ... We ran a trial. + ... It worked. + ... ''' + >>> element = BeautifulSoup(xml, "xml").find("Abstract") + >>> PMIDSource._render_abstract_sections(element) + 'METHODS: We ran a trial.\\n\\nRESULTS: It worked.' + """ + sections = [] + for node in element.find_all("AbstractText"): + text = node.get_text().strip() + if not text: + continue + label = node.get("Label") + sections.append(f"{label}: {text}" if label else text) + + joined = "\n\n".join(sections) + return joined if joined else None + def _parse_abstract(self, soup: BeautifulSoup) -> Optional[str]: """Parse the abstract from a PubMed article XML document. - Reconstructs the abstract prose from ``Abstract/AbstractText`` nodes. - Structured abstracts label each section (e.g. ``Label="METHODS"``); - the label is preserved as a ``"METHODS:"`` prefix and sections are - joined by blank lines, mirroring how PubMed renders them as text. + Reads ``Abstract`` when the record has one. Otherwise falls back to + ``OtherAbstract``, which is where PubMed keeps abstracts contributed by + other indexing programs -- ``PIP``, ``KIE``, ``NASA``, ``AIDS`` -- mostly + on pre-1990 records. Those are genuine abstracts, and reading only + ``Abstract`` reports no content at all for such a record, which on a + refresh deletes the text a cache entry already held (issue #88). + + ``OtherAbstract`` also carries translations, so an English one is + preferred; a translated abstract is still used when it is the only text + available, because reporting nothing would blank the entry. Args: soup: Parsed PubMed article XML @@ -229,22 +270,35 @@ def _parse_abstract(self, soup: BeautifulSoup) -> Optional[str]: >>> xml = 'A summary.' >>> PMIDSource()._parse_abstract(BeautifulSoup(xml, "xml")) 'A summary.' + >>> xml = ''' + ... An indexer's summary. + ... ''' + >>> PMIDSource()._parse_abstract(BeautifulSoup(xml, "xml")) + "An indexer's summary." """ abstract = soup.find("Abstract") + if abstract: + rendered = self._render_abstract_sections(abstract) + if rendered: + return rendered - if not abstract: + others = soup.find_all("OtherAbstract") + if not others: return None - sections = [] - for node in abstract.find_all("AbstractText"): - text = node.get_text().strip() - if not text: - continue - label = node.get("Label") - sections.append(f"{label}: {text}" if label else text) + # Prefer English; OtherAbstract is also where translations live, and a + # French rendering would stop an English snippet matching its source. + english = [ + node + for node in others + if (node.get("Language") or "eng").strip().lower() in ("eng", "en") + ] + for node in english + others: + rendered = self._render_abstract_sections(node) + if rendered: + return rendered - joined = "\n\n".join(sections) - return joined if joined else None + return None def _read_entrez( self, diff --git a/tests/test_absent_abstract_recovery.py b/tests/test_absent_abstract_recovery.py new file mode 100644 index 0000000..17865f9 --- /dev/null +++ b/tests/test_absent_abstract_recovery.py @@ -0,0 +1,111 @@ +"""An entry that claims no content must be re-testable after the extractor improves. + +Issue #88: the extractor read only ``Abstract`` and ignored ``OtherAbstract``, +so a record whose abstract PubMed keeps under ``OtherAbstract`` was written as +``content_type: unavailable`` -- deleting, on refresh, the abstract the cache +already held. + +Fixing the extractor does not by itself recover those entries. They carry the +current ``extractor_version``, so ``_is_stale_cache_entry`` calls them fresh and +they are never re-fetched: the text stays deleted permanently. They need their +own staleness stamp, scoped the way ``html_full_text_version`` and +``xml_extraction_version`` already are, so the refresh cost falls on the +entries that could be wrong rather than on the whole cache. +""" + +from __future__ import annotations + +from linkml_reference_validator.etl.reference_fetcher import ( + ABSENT_CONTENT_CACHE_VERSION, + EXTRACTOR_CACHE_VERSION, + HTML_FULL_TEXT_CACHE_VERSION, + XML_EXTRACTION_CACHE_VERSION, + ReferenceFetcher, +) + + +def _entry(content_type: str, *, absent_version: int | None = None) -> str: + """A cache entry that is current in every respect except what a test varies. + + The per-content-type stamps are filled in so a test of the absent-content + rule is not accidentally measuring the pre-existing HTML or XML rules. + """ + lines = [ + "---", + "reference_id: PMID:5697815", + f"extractor_version: {EXTRACTOR_CACHE_VERSION}", + ] + if absent_version is not None: + lines.append(f"absent_content_version: {absent_version}") + if content_type == "full_text_html": + lines.append(f"html_full_text_version: {HTML_FULL_TEXT_CACHE_VERSION}") + if content_type == "full_text_xml": + lines.append(f"xml_extraction_version: {XML_EXTRACTION_CACHE_VERSION}") + lines += [f"content_type: {content_type}", "---", "", "## Content", ""] + return "\n".join(lines) + + +def test_unavailable_entry_without_the_stamp_is_stale(): + """This is the entry #88 damaged: current extractor stamp, but no content.""" + assert ReferenceFetcher._is_stale_cache_entry(_entry("unavailable")) + + +def test_unavailable_entry_with_the_current_stamp_is_fresh(): + """Once re-tested by a fixed extractor it must stop being re-fetched.""" + assert not ReferenceFetcher._is_stale_cache_entry( + _entry("unavailable", absent_version=ABSENT_CONTENT_CACHE_VERSION) + ) + + +def test_a_newer_stamp_is_not_stale(): + """An older tool reading a newer cache leaves it alone, as elsewhere.""" + assert not ReferenceFetcher._is_stale_cache_entry( + _entry("unavailable", absent_version=ABSENT_CONTENT_CACHE_VERSION + 1) + ) + + +def test_entries_that_do_have_content_are_untouched(): + """The stamp is scoped to ``unavailable`` so the blast radius stays small. + + A blanket ``EXTRACTOR_CACHE_VERSION`` bump would invalidate every cached + reference -- 57,551 files in the dismech cache against the 1,735 that claim + no content -- for a bug that can only have produced the latter. + """ + for content_type in ( + "abstract_only", + "full_text_xml", + "full_text_html", + "full_text_pdf", + "summary", + "structured_record", + ): + assert not ReferenceFetcher._is_stale_cache_entry(_entry(content_type)), ( + f"{content_type} should not be invalidated by the absent-content stamp" + ) + + +def test_a_freshly_written_absent_entry_is_not_immediately_stale(tmp_path): + """The emitter and the staleness rule must agree, or the fix does nothing. + + Without the emitted stamp every ``unavailable`` entry would be re-fetched on + every run forever; without the staleness rule the already-damaged ones would + never be re-fetched at all. This is the round trip. + """ + from linkml_reference_validator.models import ( + ReferenceContent, + ReferenceValidationConfig, + ) + + fetcher = ReferenceFetcher(ReferenceValidationConfig(cache_dir=tmp_path)) + fetcher._save_to_disk( + ReferenceContent( + reference_id="PMID:4869291", + content_type="unavailable", + title="A record with no abstract anywhere", + content="", + ) + ) + + written = next(tmp_path.glob("*.md")).read_text() + assert f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}" in written + assert not ReferenceFetcher._is_stale_cache_entry(written) diff --git a/tests/test_sources.py b/tests/test_sources.py index a4d886e..bd11f3e 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -420,6 +420,110 @@ def test_parse_abstract_absent(self, source): assert source._parse_abstract(soup) is None + def test_parse_abstract_falls_back_to_other_abstract(self, source): + """A record whose only abstract is in ``OtherAbstract`` must still be read. + + PubMed keeps abstracts contributed by other indexing programs (PIP, KIE, + NASA, AIDS) in ``OtherAbstract`` rather than ``Abstract``. Most are + pre-1990 records. Reading only ``Abstract`` reports no content for them, + which on a refresh *deletes* the abstract a cache entry already held + (issue #88). + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + "Glucose absorption raises sodium uptake." + "", + "xml", + ) + + assert source._parse_abstract(soup) == "Glucose absorption raises sodium uptake." + + def test_parse_abstract_prefers_abstract_over_other_abstract(self, source): + """``Abstract`` wins when both are present; the fallback is a fallback.""" + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + "The real abstract." + '' + "An indexer's summary." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "The real abstract." + + def test_parse_abstract_prefers_english_other_abstract(self, source): + """``OtherAbstract`` also holds translations; snippets are matched in English. + + Picking the first element would return the French here, so an English + snippet would stop matching its own source. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + '' + "Un resume en francais." + "" + '' + "An English summary." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "An English summary." + + def test_parse_abstract_takes_non_english_other_abstract_over_nothing(self, source): + """With no English option, a translated abstract still beats no content. + + Reporting nothing would blank the cache entry; the text is at least the + paper's own summary, and content_type still says abstract_only. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + "Un resume en francais." + "", + "xml", + ) + + assert source._parse_abstract(soup) == "Un resume en francais." + + def test_parse_abstract_keeps_labels_in_other_abstract(self, source): + """A structured ``OtherAbstract`` keeps its section labels like any other.""" + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + 'We ran a trial.' + 'It worked.' + "", + "xml", + ) + + assert source._parse_abstract(soup) == ( + "METHODS: We ran a trial.\n\nRESULTS: It worked." + ) + + def test_parse_abstract_skips_empty_other_abstract(self, source): + """An empty ``OtherAbstract`` yields None rather than an empty string.""" + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) is None + def test_parse_abstract_empty(self, source): """An Abstract with only empty AbstractText should yield None.""" from bs4 import BeautifulSoup From 0c73af520d033082299ea5603639f9fa9bc4e69a Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Sun, 20 Sep 2026 23:57:28 -0700 Subject: [PATCH 2/3] Address review on #90: mypy, docs, and the scope claim mypy was the CI failure, not the tests -- 1151 passed on all four Python versions. `node.get("Language")` can return a list for a multi-valued attribute, so `.strip()` failed type checking. Handled rather than silenced. I had run bare `pytest` instead of `just test`, which is pytest + mypy + format. The reviewer's point about blast radius was right, and the numbers are worse than stated. Of the 1,735 `unavailable` entries in the dismech cache, 1,499 are DOI and only 236 are PMID, so most of this refresh is cost rather than repair -- the OtherAbstract fix cannot help a DOI entry. The original claim that "only entries claiming no content can be wrong in this way" was true but invited the converse reading. The constant's comment now says so, and rests the design on a different argument: the stamp is deliberately general -- "this no-content claim was made by extractor version N" -- which a later fix to any other source will want, and it remains a 3% refresh against 100%. The `summary` escape hatch is real. With `source_extra_fields["PMID"]` set, a record with no abstract is stored as `summary` carrying the extra-fields blob, so an OtherAbstract-only record damaged by #88 falls outside the stamp and is never re-tested. dismech does not set that option, so its figures are unaffected, but the gap is documented in both the staleness rule and troubleshooting.md with the manual remedy. Not widened to `summary`: most such entries have nothing to do with a missing abstract, so it would cost a re-fetch of all of them to reach a config-dependent few. Also: - A comment at the emit site records why writing the stamp from the constant is correct here -- every path that saves an `unavailable` entry has just produced it with this extractor -- and names the invariant a future enrichment path would break. - `absent_content_version` is documented in troubleshooting.md beside the existing `xml_extraction_version` paragraph, including the one-time refresh. - The English-preference pass no longer rehearses English nodes in its fallback. - Two tests added for branches that were only implicitly covered: a present-but-empty `Abstract` falling through to `OtherAbstract`, and an `OtherAbstract` carrying no `Language` attribute at all, where the `or "eng"` default is load-bearing for real PIP records. just test: 1152 passed, 1 skipped; mypy clean; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- docs/troubleshooting.md | 20 ++++++++ .../etl/reference_fetcher.py | 32 +++++++++++-- .../etl/sources/pmid.py | 17 ++++--- tests/test_sources.py | 46 +++++++++++++++++++ 4 files changed, 106 insertions(+), 9 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 986416a..5488a65 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -882,6 +882,26 @@ XML remains available with the existing stale-cache warning and is not rewritten it may still lack table rows. A later process retries the refresh. Existing stale HTML rejection remains unchanged. +Entries that record *no* content carry their own stamp. A cache entry written +as `content_type: unavailable` is a claim about what an extractor could not +find, so an extractor fix can make it wrong -- and because such an entry carries +a current `extractor_version`, nothing would otherwise re-test it and the +missing text would stay missing permanently. Those entries now carry +`absent_content_version: 1`, and a missing or older stamp causes a one-time +refresh on the next validation fetch. Entries that do have content are +unaffected, so the refresh is confined to the entries that could be wrong +rather than falling on the whole cache. Version 1 is reading `OtherAbstract`, +where PubMed keeps abstracts contributed by other indexing programs (`PIP`, +`KIE`, `NASA`, `AIDS`, mostly on pre-1990 records); before it, such a record was +stored as having no content at all and a refresh deleted the abstract the cache +already held (issue #88). + +One gap is worth knowing if you set `source_extra_fields` for `PMID`: a record +with no abstract is then stored as `summary` carrying the extra-fields blob +rather than as `unavailable`, so it falls outside this stamp and is not +re-tested. Clear such entries by hand if you were running that setting before +this version. + **A cache-wide refresh adds one line to every enriched entry.** Both index providers now set `access_type` where they previously left it unset, so a refreshed public entry gains a `full_text_access_type: open` line. It means the diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index baa78b6..1d6762e 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -100,9 +100,17 @@ #: and the text would stay lost. Version 1: reading ``OtherAbstract``, where #: PubMed keeps PIP/KIE/NASA/AIDS abstracts; before it, such a record was stored #: as having no content and a refresh deleted the abstract already cached for it -#: (issue #88). Scoped rather than bumping EXTRACTOR_CACHE_VERSION because only -#: entries claiming no content can be wrong in this way, and a blanket bump -#: would re-fetch every cached reference to find them. +#: (issue #88). Scoped rather than bumping EXTRACTOR_CACHE_VERSION because an +#: entry that records content cannot be wrong in this way, and a blanket bump +#: would re-fetch every cached reference to find the ones that can be. +#: +#: The converse does not hold, and the stamp is deliberately general rather than +#: targeted: `unavailable` is emitted by doi, url, clinicaltrials, json_api, +#: entrez and ppr as well as pmid, and the OtherAbstract fix cannot help any of +#: those. In the dismech cache 1,499 of the 1,735 such entries are DOI, so most +#: of this refresh is cost rather than repair. That is accepted -- the stamp +#: means "this no-content claim was made by extractor version N", which a later +#: fix to any source will want, and it is still a 3% refresh against 100%. ABSENT_CONTENT_CACHE_VERSION = 1 #: A cache file's frontmatter delimiter: a line that is exactly ``---``. @@ -1319,6 +1327,16 @@ def _save_to_disk( xml_version = (reference.metadata or {}).get("xml_extraction_version") if reference.content_type == "full_text_xml" and type(xml_version) is int: lines.append(f"xml_extraction_version: {xml_version}") + # Written from the constant, unlike the HTML and XML stamps, which come + # from `reference.metadata` so a metadata-only rewrite preserves the + # original. That is safe here only because every path that saves an + # `unavailable` entry has just produced it with this extractor: + # `apply_full_text_location` saves only when it applied something, which + # moves content_type away from `unavailable`, and `_maybe_retry_full_text` + # runs on the fetch path where staleness already forced a re-fetch. An + # enrichment path that re-saved an untouched `unavailable` entry would + # certify it as re-tested when it was not -- recreating #88 one version + # up. Move this to metadata if such a path is ever added. if reference.content_type == "unavailable": lines.append(f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}") if reference.title: @@ -1653,6 +1671,14 @@ def _is_stale_cache_entry(cls, content_text: str) -> bool: if type(xml_version) is not int or xml_version < XML_EXTRACTION_CACHE_VERSION: return True + # Scoped to `unavailable`, which leaves one gap: with + # `source_extra_fields["PMID"]` configured, a record with no abstract is + # stored as `summary` carrying the extra-fields blob, so an + # OtherAbstract-only record damaged by #88 lands outside this rule and is + # never re-tested. Widening to `summary` is not worth it -- most summary + # entries have nothing to do with a missing abstract, so it would cost a + # re-fetch of all of them. A deployment using that setting should clear + # its affected entries by hand. if isinstance(metadata, dict) and metadata.get("content_type") == "unavailable": absent_version = metadata.get("absent_content_version") if ( diff --git a/src/linkml_reference_validator/etl/sources/pmid.py b/src/linkml_reference_validator/etl/sources/pmid.py index aedffc8..e69a7fc 100644 --- a/src/linkml_reference_validator/etl/sources/pmid.py +++ b/src/linkml_reference_validator/etl/sources/pmid.py @@ -288,12 +288,17 @@ def _parse_abstract(self, soup: BeautifulSoup) -> Optional[str]: # Prefer English; OtherAbstract is also where translations live, and a # French rendering would stop an English snippet matching its source. - english = [ - node - for node in others - if (node.get("Language") or "eng").strip().lower() in ("eng", "en") - ] - for node in english + others: + # `Language` is #IMPLIED with an `eng` default in the PubMed DTD, so an + # attribute-less node is English rather than an unknown translation. + def _is_english(node: Any) -> bool: + language = node.get("Language") + if isinstance(language, list): # bs4 may split a multi-valued attr + language = language[0] if language else None + return str(language or "eng").strip().lower() in ("eng", "en") + + english = [node for node in others if _is_english(node)] + rest = [node for node in others if not _is_english(node)] + for node in english + rest: rendered = self._render_abstract_sections(node) if rendered: return rendered diff --git a/tests/test_sources.py b/tests/test_sources.py index bd11f3e..012b70a 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -511,6 +511,52 @@ def test_parse_abstract_keeps_labels_in_other_abstract(self, source): "METHODS: We ran a trial.\n\nRESULTS: It worked." ) + def test_parse_abstract_falls_through_an_empty_abstract_element(self, source): + """A present-but-empty ``Abstract`` must not short-circuit the fallback. + + This is a distinct branch from "no ``Abstract`` at all", and the one that + regresses if the guard is ever rewritten back to ``if not abstract:`` + returning early on element presence alone. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + "" + '' + "The only real text here." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "The only real text here." + + def test_parse_abstract_treats_a_language_less_other_abstract_as_english( + self, source + ): + """``Language`` is ``#IMPLIED`` with an ``eng`` default in the PubMed DTD. + + Real PIP records often omit it, so the ``or "eng"`` default is + load-bearing: without it an attribute-less node would be treated as an + unknown translation and deprioritised. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + '' + "No language attribute at all." + "" + '' + "Un resume en francais." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "No language attribute at all." + def test_parse_abstract_skips_empty_other_abstract(self, source): """An empty ``OtherAbstract`` yields None rather than an empty string.""" from bs4 import BeautifulSoup From 0ac77b1c5f793f0592534ee435908650b9625bad Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Mon, 21 Sep 2026 15:53:34 -0700 Subject: [PATCH 3/3] Address re-review: correct the invariant comment, test the emitter's scoping The comment explaining why `absent_content_version` may be written from the constant named the wrong mechanism. It claimed `_maybe_retry_full_text` "runs on the fetch path where staleness already forced a re-fetch"; it is called inside the `if cached:` branch, which returns without fetching. The conclusion held but the reason did not, and the comment exists precisely to send a maintainer at version 2 to the right invariant. What actually guarantees it: `_load_from_disk` filters stale entries out, so an `unavailable` entry surviving a cache hit already carries the current stamp and re-writing it is a no-op -- and at version 2 a stamp-1 entry is stale, so that branch is never taken for it. The comment now also names `_stale_fallback` and `_preserve_cached_full_text`, the two paths that do hold an untouched cached entry and are safe only because both document that it is not re-saved. Those are the contracts a future change would have to violate. The emitter's scoping had no test. `test_entries_that_do_have_content_are_untouched` exercises the staleness rule, so deleting the `content_type == "unavailable"` guard in `_save_to_disk` left 81 tests green -- verified by removing it. What escapes is not a wrong answer but cache-wide churn: an unconditional stamp adds a line to every entry on refresh, the same problem troubleshooting.md already documents for `full_text_access_type`. `test_an_entry_with_content_is_not_stamped` closes it, and fails against the mutation. Also partitions the OtherAbstract nodes in one pass instead of two. just test: 1153 passed, 1 skipped; mypy clean; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../etl/reference_fetcher.py | 23 ++++++++++----- .../etl/sources/pmid.py | 7 +++-- tests/test_absent_abstract_recovery.py | 29 +++++++++++++++++++ 3 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 1d6762e..0e90f19 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -1329,14 +1329,21 @@ def _save_to_disk( lines.append(f"xml_extraction_version: {xml_version}") # Written from the constant, unlike the HTML and XML stamps, which come # from `reference.metadata` so a metadata-only rewrite preserves the - # original. That is safe here only because every path that saves an - # `unavailable` entry has just produced it with this extractor: - # `apply_full_text_location` saves only when it applied something, which - # moves content_type away from `unavailable`, and `_maybe_retry_full_text` - # runs on the fetch path where staleness already forced a re-fetch. An - # enrichment path that re-saved an untouched `unavailable` entry would - # certify it as re-tested when it was not -- recreating #88 one version - # up. Move this to metadata if such a path is ever added. + # original. The invariant that makes that safe: nothing reaches a save + # path holding an `unavailable` entry it did not just produce. + # + # `_load_from_disk` filters stale entries out, so an `unavailable` entry + # that survives a cache hit already carries the current stamp and + # re-writing it is a no-op -- and at version 2 a stamp-1 entry is stale, + # so the cache-hit branch is never taken for it at all. + # + # The two paths that *do* hold an untouched cached entry are + # `_stale_fallback` and `_preserve_cached_full_text`, and both are safe + # only because they document that the entry is deliberately not + # re-saved. Those are the contracts a future change would have to + # violate: re-saving an untouched `unavailable` entry would certify it as + # re-tested when it was not, recreating #88 one version up. Move this to + # metadata if such a path is ever added. if reference.content_type == "unavailable": lines.append(f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}") if reference.title: diff --git a/src/linkml_reference_validator/etl/sources/pmid.py b/src/linkml_reference_validator/etl/sources/pmid.py index e69a7fc..a38401c 100644 --- a/src/linkml_reference_validator/etl/sources/pmid.py +++ b/src/linkml_reference_validator/etl/sources/pmid.py @@ -296,8 +296,11 @@ def _is_english(node: Any) -> bool: language = language[0] if language else None return str(language or "eng").strip().lower() in ("eng", "en") - english = [node for node in others if _is_english(node)] - rest = [node for node in others if not _is_english(node)] + english: list[Any] = [] + rest: list[Any] = [] + for node in others: + (english if _is_english(node) else rest).append(node) + for node in english + rest: rendered = self._render_abstract_sections(node) if rendered: diff --git a/tests/test_absent_abstract_recovery.py b/tests/test_absent_abstract_recovery.py index 17865f9..91a4eb2 100644 --- a/tests/test_absent_abstract_recovery.py +++ b/tests/test_absent_abstract_recovery.py @@ -109,3 +109,32 @@ def test_a_freshly_written_absent_entry_is_not_immediately_stale(tmp_path): written = next(tmp_path.glob("*.md")).read_text() assert f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}" in written assert not ReferenceFetcher._is_stale_cache_entry(written) + + +def test_an_entry_with_content_is_not_stamped(tmp_path): + """The emitter's scoping needs its own test; the staleness rule's is separate. + + ``test_entries_that_do_have_content_are_untouched`` exercises + ``_is_stale_cache_entry``, so deleting the ``content_type == "unavailable"`` + guard in ``_save_to_disk`` leaves the whole suite green. What escapes is not + a wrong answer but cache-wide churn: an unconditional stamp adds a line to + every entry on refresh, the same diff-on-every-file problem + ``troubleshooting.md`` already calls out for ``full_text_access_type``. + """ + from linkml_reference_validator.models import ( + ReferenceContent, + ReferenceValidationConfig, + ) + + fetcher = ReferenceFetcher(ReferenceValidationConfig(cache_dir=tmp_path)) + fetcher._save_to_disk( + ReferenceContent( + reference_id="PMID:38463381", + content_type="abstract_only", + title="A record that does have an abstract", + content="Real abstract text.", + ) + ) + + written = next(tmp_path.glob("*.md")).read_text() + assert "absent_content_version" not in written