Read OtherAbstract, and re-test the entries that claim no content

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

src/linkml_reference_validator/etl/reference_fetcher.py

@@ -93,6 +93,26 @@
 #: 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 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 ``---``.
 #: 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 +1327,25 @@ 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. 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:
             lines.append(f"title: {self._quote_yaml_value(reference.title)}")
         if reference.authors:
@@ -1639,6 +1678,22 @@ 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 (
+                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

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 = '''<Abstract>
+            ...   <AbstractText Label="METHODS">We ran a trial.</AbstractText>
+            ...   <AbstractText Label="RESULTS">It worked.</AbstractText>
+            ... </Abstract>'''
+            >>> 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,43 @@ def _parse_abstract(self, soup: BeautifulSoup) -> Optional[str]:
             >>> xml = '<Abstract><AbstractText>A summary.</AbstractText></Abstract>'
             >>> PMIDSource()._parse_abstract(BeautifulSoup(xml, "xml"))
             'A summary.'
+            >>> xml = '''<OtherAbstract Type="PIP" Language="eng">
+            ...   <AbstractText>An indexer's summary.</AbstractText>
+            ... </OtherAbstract>'''
+            >>> 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.
+        # `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: 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:
+                return rendered
 
-        joined = "\n\n".join(sections)
-        return joined if joined else None
+        return None
 
     def _read_entrez(
         self,

tests/test_absent_abstract_recovery.py

@@ -0,0 +1,140 @@
+"""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)
+
+
+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