diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 298ef95..4b7a3d6 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -4,6 +4,7 @@ fetching from various sources (PMID, DOI, file, URL) using a plugin architecture. """ +import json import logging import re from collections.abc import Iterator @@ -133,6 +134,28 @@ def _text_after_abstract(content: Optional[str]) -> str: return body if separator else text +#: Casefolded cache filename -> real names, per directory. Module-level on +#: purpose: see ``ReferenceFetcher._case_index`` for why per-instance state +#: leaves two fetchers on one directory blind to each other's writes. +_CASE_INDEXES: dict[Path, dict[str, set[str]]] = {} + +#: Characters ``json.dumps(ensure_ascii=False)`` leaves literal that the YAML +#: reader nonetheless rejects or the scanner treats as a line break: DEL, the +#: C1 block, and the Unicode line and paragraph separators. JSON escapes only +#: below 0x20, so these reach the file raw -- and a raw C1 byte makes the +#: reader refuse the whole entry, while a raw separator inside a quoted scalar +#: folds to a space on reload. The realistic source is mojibake: Windows-1252 +#: read as Latin-1 turns smart quotes and en-dashes into ``\x91``-``\x97``, a +#: common shape for bibliographic metadata. +_YAML_UNSAFE = re.compile("[\x7f-\x9f\u2028\u2029]") + + +def _json_scalar(value: str) -> str: + """Render ``value`` as a JSON string that survives a YAML round trip.""" + rendered = json.dumps(value, ensure_ascii=False) + return _YAML_UNSAFE.sub(lambda m: f"\\u{ord(m.group(0)):04x}", rendered) + + class _RefreshLoss: """Describe what a refused refresh returned, formatted only if logged. @@ -1057,7 +1080,129 @@ def _cache_path(self, reference_id: str, cache_dir: Path) -> Path: .replace("?", "_") .replace("=", "_") ) - return cache_dir / f"{safe_id}.md" + path = cache_dir / f"{safe_id}.md" + if prefix.upper() == "DOI": + return self._existing_case_variant(path) + return path + + @staticmethod + def _stored_reference_id(cache_path: Path, reference: ReferenceContent) -> str: + """The id already recorded in ``cache_path``, else the reference's own. + + Applied to DOI references only, since that is what the argument covers: + a DOI in two capitalizations is one reference, so the spelling first + written should stand. Sanitization also collapses ``:`` ``/`` ``?`` and + ``=`` to ``_``, so two *distinct* ``url:`` references can share a + filename; that collision predates this change and cementing the first + writer's id there would change how it is handled, for an identifier + type the argument does not cover. + + Read with ``errors="replace"``: an entry too mangled to decode is the one + that most needs overwriting, and a ``UnicodeDecodeError`` here would be + the one thing that stops it. Only the ``reference_id:`` line is read, so + replacement characters elsewhere cost nothing. + """ + if not reference.reference_id.upper().startswith("DOI:"): + return reference.reference_id + try: + with cache_path.open(encoding="utf-8", errors="replace") as handle: + for position, line in enumerate(handle): + if line.startswith("reference_id:"): + stored = line.split(":", 1)[1].strip() + return stored or reference.reference_id + # The opening delimiter is the first line; the closing one + # ends the frontmatter and with it anywhere the id can be. + if position and line.strip() == "---": + break + except OSError: # no existing entry, or unreadable + pass + return reference.reference_id + + def _existing_case_variant(self, path: Path) -> Path: + """Return an already-cached file differing from ``path`` only in case. + + DOI names are case-insensitive by specification, so + ``10.1016/S0002-9440(10)63332-9`` and its lowercase spelling are one + reference. The prefix is normalized but the suffix is not, so without + this each spelling gets its own file: a duplicate download, and two + committed entries for one paper in a project that versions its cache. + + Resolution looks for what is already on disk rather than imposing a + canonical spelling, so an existing cache is not renamed underneath + anyone -- a project holding thousands of DOI entries sees no diff, and + whichever spelling was written first keeps winning. + + Scoped to DOI on purpose. A PMID or an NCT id is not case-insensitive, + and folding those would merge genuinely distinct references. + + The index is built from real directory entries and ``exists()`` is never + consulted. On a case-insensitive filesystem -- macOS by default -- + ``exists()`` is true for a spelling that is *not* the one on disk, so + trusting it would return the caller's spelling and write a second entry + that silently collides with the first. + + Where a cache already holds *both* spellings -- what a case-sensitive + checkout of an already-duplicated repository looks like -- the caller's + exact spelling wins, and failing that the lexicographically first, so + the answer does not depend on directory order. + """ + index = self._case_index(path.parent) + names = index.get(path.name.casefold()) + if not names: + return path + if path.name in names: + return path + return path.parent / min(names) + + @staticmethod + def _case_index(cache_dir: Path) -> dict[str, set[str]]: + """Casefolded name -> real names, built once per directory per process. + + ``_cache_path`` is on the read path and the write path both, so scanning + the directory per resolution is quadratic over a cache: measured at + 6.7 ms a call against a 6,721-entry directory, or 45 seconds of pure + path resolution for a run that touches every reference. + + The index is shared across every fetcher in the process, not held per + instance. ``Repairer`` holds two fetchers over one directory -- its own + and the one inside ``SupportingTextValidator`` -- and interleaves them + across many references, so with per-instance state each one's writes + were invisible to the other and a cross-spelling DOI could still produce + two files. Every write funnels through :meth:`_remember_cache_file`, so + one shared map stays accurate for the whole process. + + What it cannot see is a writer *outside this process* -- another run, + or a subprocess this one shelled out to -- which is the window + :meth:`forget_cache_listing` exists for. + """ + cached = _CASE_INDEXES.get(cache_dir) + if cached is not None: + return cached + index: dict[str, set[str]] = {} + if cache_dir.is_dir(): + for entry in cache_dir.iterdir(): + index.setdefault(entry.name.casefold(), set()).add(entry.name) + _CASE_INDEXES[cache_dir] = index + return index + + @staticmethod + def forget_cache_listing() -> None: + """Drop the cached directory listings used to resolve DOI capitalization. + + Call this after something *outside this process* has written to a cache + directory -- another run, or a subprocess -- so the next DOI lookup sees + the new files. Writes from any fetcher in this process are already + tracked. Cheap: a listing is rebuilt on the next resolution that needs + it. + """ + _CASE_INDEXES.clear() + + @staticmethod + def _remember_cache_file(path: Path) -> None: + """Record a newly written file so a later lookup in another case finds it.""" + index = _CASE_INDEXES.get(path.parent) + if index is not None: + index.setdefault(path.name.casefold(), set()).add(path.name) def _quote_yaml_value(self, value: str) -> str: """Quote a YAML value if it contains special characters. @@ -1082,7 +1227,29 @@ def _quote_yaml_value(self, value: str) -> str: 'Normal title' >>> fetcher._quote_yaml_value("Title: with colon") '"Title: with colon"' + + A line break cannot be carried by a plain or a double-quoted YAML + scalar written on one line, so such a value is emitted JSON-style, + which is valid YAML and reloads with the break intact: + + >>> fetcher._quote_yaml_value("Two\\nlines") + '"Two\\\\nlines"' """ + # A literal line break ends the scalar, so interpolating one produces + # frontmatter that does not parse -- and re-fetching rewrites the same + # broken file, so nothing recovers it. Crossref titles do contain them. + # JSON escaping is valid YAML and preserves the break on reload, where + # folding it to a space would silently edit metadata that is compared + # against the fetched record elsewhere. + # ``splitlines`` splits on every break the YAML scanner recognises -- + # U+0085, U+2028 and U+2029 as well as \n and \r -- and on the vertical + # tab and form feed, which the reader rejects outright as non-printable. + # Each produces the same unrecoverable file, just with a rarer + # character. ``!= [value]`` rather than ``len(...) > 1`` because a + # trailing break splits to a single element. + if value.splitlines() != [value] or _YAML_UNSAFE.search(value): + return _json_scalar(value) + # Characters that require quoting in YAML values special_chars = '[]{}:,#&*?|<>=!%@`"\'\\' needs_quote = False @@ -1126,7 +1293,12 @@ def _save_to_disk( lines = [] lines.append("---") - lines.append(f"reference_id: {reference.reference_id}") + # An entry that already exists keeps the spelling it was written with. + # The path resolves to that file either way, so rewriting this line + # under the caller's capitalization would leave a one-line diff in a + # committed cache on every cross-spelling re-fetch -- the churn this + # resolution exists to remove, one layer in. + lines.append(f"reference_id: {self._stored_reference_id(cache_path, reference)}") lines.append(f"extractor_version: {EXTRACTOR_CACHE_VERSION}") html_version = (reference.metadata or {}).get("html_full_text_version") if reference.content_type == "full_text_html" and isinstance(html_version, int): @@ -1242,6 +1414,7 @@ def _save_to_disk( lines.append(reference.content) cache_path.write_text("\n".join(lines), encoding="utf-8") + self._remember_cache_file(cache_path) if private: cache_path.chmod(0o600) logger.info(f"Cached {reference.reference_id} to {cache_path}") diff --git a/tests/test_frontmatter_and_doi_case.py b/tests/test_frontmatter_and_doi_case.py new file mode 100644 index 0000000..1f686f8 --- /dev/null +++ b/tests/test_frontmatter_and_doi_case.py @@ -0,0 +1,334 @@ +"""Two ways a cache entry's identity or content is lost on the way to disk. + +Both were found downstream, where they had been worked around with runtime +monkeypatches over this module's internals. They are ordinary defects here. + +**A newline in a title produces invalid frontmatter.** ``_quote_yaml_value`` +quotes on a list of special characters that does not include ``\\n``, so a +Crossref title containing a literal line break is interpolated straight into +the frontmatter and the entry no longer parses. Nothing downstream can recover +it, because re-fetching writes the same broken file. + +**A DOI in two capitalizations names two files.** DOI names are +case-insensitive by specification, so ``DOI:10.1016/S0002-9440(10)63332-9`` and +``doi:10.1016/s0002-9440(10)63332-9`` are the same reference; the prefix is +normalized but the suffix is not, so each gets its own cache file. The second +one fetched is a duplicate download, and a project that commits its cache +carries both. +""" + + +from pathlib import Path +from unittest.mock import patch + +import pytest + +from linkml_reference_validator.etl.reference_fetcher import ReferenceFetcher +from linkml_reference_validator.models import ReferenceContent, ReferenceValidationConfig + + +@pytest.fixture +def fetcher(tmp_path): + return ReferenceFetcher(ReferenceValidationConfig(cache_dir=tmp_path)) + + +# -------------------------------------------------------------------------- +# Line breaks in metadata +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "title", + [ + "A Crossref title\nwith a literal newline", + "Carriage return\r\nand line feed", + "Trailing break\n", + # ruamel's scanner treats these as line breaks too, and its reader + # rejects the vertical-tab and form-feed outright as non-printable. + # Rarer characters, identical unrecoverable file. + "Next line\x85separator", + "Line separator\u2028here", + "Paragraph separator\u2029here", + "Vertical tab\x0bhere", + "Form feed\x0chere", + # ruamel's reader rejects DEL and the C1 block as non-printable, and + # json.dumps leaves them literal since it only escapes below 0x20. The + # realistic source is mojibake: Windows-1252 read as Latin-1 turns smart + # quotes and en-dashes into \x91-\x97, a common shape for metadata. + "Smart quote\x92s", + "Delete\x7fcharacter", + "C1 block\x80here", + ], +) +def test_a_title_containing_a_line_break_round_trips(fetcher, title): + """The entry must still parse, and the title must survive unchanged. + + Folding the break into a space would also "work" in the sense that the file + parses, but it silently edits the metadata -- and a title is compared + against the fetched record elsewhere, so an edited one reads as a mismatch. + """ + fetcher._save_to_disk( + ReferenceContent( + reference_id="DOI:10.1/x", + title=title, + content="Body text.", + content_type="abstract_only", + ) + ) + path = fetcher.get_cache_path("DOI:10.1/x") + + reloaded = fetcher._load_markdown_format(path.read_text(encoding="utf-8"), "DOI:10.1/x") + + assert reloaded is not None, "the entry must still parse" + assert reloaded.title == title + + +def test_quoting_is_unchanged_for_single_line_values(fetcher): + """The escape applies to the case that needs it and nothing else.""" + assert fetcher._quote_yaml_value("Normal title") == "Normal title" + assert fetcher._quote_yaml_value("Title: with colon") == '"Title: with colon"' + assert fetcher._quote_yaml_value("[Cholera].") == '"[Cholera]."' + + +# -------------------------------------------------------------------------- +# DOI capitalization +# -------------------------------------------------------------------------- + +UPPER = "DOI:10.1016/S0002-9440(10)63332-9" +LOWER = "doi:10.1016/s0002-9440(10)63332-9" + + +def test_a_doi_in_two_capitalizations_uses_one_cache_file(fetcher): + """DOI names are case-insensitive, so these are one reference.""" + fetcher._save_to_disk( + ReferenceContent( + reference_id=UPPER, title="A paper", content="Body.", + content_type="abstract_only", + ) + ) + + assert fetcher.get_cache_path(LOWER) == fetcher.get_cache_path(UPPER) + + +def test_the_second_capitalization_reads_the_first_one_s_entry(fetcher, tmp_path): + """The point of sharing the path: no duplicate download, no second file.""" + fetcher._save_to_disk( + ReferenceContent( + reference_id=UPPER, title="A paper", content="Body text here.", + content_type="abstract_only", + ) + ) + + path = fetcher.get_cache_path(LOWER) + reloaded = fetcher._load_markdown_format(path.read_text(encoding="utf-8"), LOWER) + + assert reloaded.title == "A paper" + assert len(list(tmp_path.glob("*.md"))) == 1, "one reference, one file" + + +def test_an_existing_entry_keeps_its_own_capitalization(fetcher, tmp_path): + """Backwards compatible: a committed cache is not renamed underneath anyone. + + Resolution finds whatever spelling is already on disk rather than imposing + one, so a project that has committed thousands of DOI entries sees no diff. + """ + written = tmp_path / "DOI_10.1016_S0002-9440(10)63332-9.md" + written.write_text( + "---\nreference_id: DOI:10.1016/S0002-9440(10)63332-9\n" + "title: A paper\ncontent_type: abstract_only\n---\n\n## Content\n\nBody.\n", + encoding="utf-8", + ) + + assert fetcher.get_cache_path(LOWER).name == written.name + + +def test_a_non_doi_reference_is_not_case_folded(fetcher): + """Only DOI names are case-insensitive; a PMID or an NCT id is not. + + Folding those would merge references that are genuinely distinct, so the + rule is scoped to the identifier type the specification covers. + """ + assert fetcher.get_cache_path("PMID:123") != fetcher.get_cache_path("PMID:123X") + assert "PMID_123" in fetcher.get_cache_path("PMID:123").name + + +def test_both_spellings_on_disk_resolve_deterministically(fetcher, tmp_path): + """The mixed state is not hypothetical -- it is what a case-sensitive + checkout of an already-duplicated cache looks like. + + ``iterdir()`` order is arbitrary, so returning the first match makes the + answer depend on inode order: two checkouts of the same repository can + disagree, and asking for one spelling can hand back the other. + """ + upper = tmp_path / "DOI_10.1016_S0002-9440(10)63332-9.md" + lower = tmp_path / "doi_10.1016_s0002-9440(10)63332-9.md" + for path, who in ((upper, "upper"), (lower, "lower")): + try: + path.write_text(f"---\nreference_id: x\n---\n\n## Content\n\n{who}\n") + except OSError: # pragma: no cover - case-insensitive filesystem + pytest.skip("filesystem cannot hold both spellings") + if not (upper.exists() and lower.exists()) or upper.read_text() == lower.read_text(): + pytest.skip("filesystem folded the two names together") + + assert fetcher.get_cache_path(UPPER).name == upper.name, "exact spelling wins" + assert fetcher.get_cache_path(LOWER).name == lower.name + # ...and repeated resolution is stable + assert fetcher.get_cache_path(UPPER) == fetcher.get_cache_path(UPPER) + + +def test_saving_under_the_second_spelling_does_not_create_a_second_file(fetcher, tmp_path): + """A single save was not enough to catch a second write.""" + for rid in (UPPER, LOWER): + fetcher._save_to_disk( + ReferenceContent( + reference_id=rid, title="A paper", content="Body.", + content_type="abstract_only", + ) + ) + + assert len(list(tmp_path.glob("*.md"))) == 1, "one reference, one file" + + +def test_a_cross_spelling_refetch_keeps_the_stored_reference_id(fetcher, tmp_path): + """The filename stops churning; the frontmatter must too. + + Rewriting ``reference_id`` under the caller's spelling would leave a + one-line diff in a committed cache on every re-fetch -- the same churn this + change exists to remove, one layer in. + """ + fetcher._save_to_disk( + ReferenceContent( + reference_id=UPPER, title="A paper", content="Body.", + content_type="abstract_only", + ) + ) + fetcher._save_to_disk( + ReferenceContent( + reference_id=LOWER, title="A paper", content="Body.", + content_type="abstract_only", + ) + ) + + stored = fetcher.get_cache_path(UPPER).read_text(encoding="utf-8") + assert f"reference_id: {UPPER}" in stored + + +def test_resolution_does_not_scan_the_directory_per_call(fetcher, tmp_path): + """A scan per resolution is quadratic over a cache. + + ``_cache_path`` is on the read *and* write paths, so a run touching every + reference scanned the whole directory once per reference: measured at 6.7 ms + a call against a 6,721-entry cache, or 45 seconds of pure path resolution. + """ + for i in range(50): + (tmp_path / f"DOI_10.1000_paper{i}.md").write_text("x") + + scans = 0 + real_iterdir = Path.iterdir + + def counting_iterdir(self): + nonlocal scans + scans += 1 + return real_iterdir(self) + + with patch.object(Path, "iterdir", counting_iterdir): + for i in range(20): + fetcher.get_cache_path(f"DOI:10.1000/paper{i}") + + assert scans <= 1, f"expected at most one directory scan, got {scans}" + + +def test_a_file_written_by_another_process_is_found_after_a_reset(fetcher, tmp_path): + """The index is per-process, so an outside writer leaves it stale. + + A downstream backfill script hit exactly this: it shells out to fetch a + missing reference, the subprocess writes the file into a directory the + parent has already indexed, and a later lookup of the same DOI in another + capitalization does not see it. The window is real but narrow, so the + remedy is an explicit reset rather than re-scanning on every miss, which + would undo the memoization. + """ + fetcher.get_cache_path(UPPER) # builds the index while the file is absent + + outsider = tmp_path / "DOI_10.1016_S0002-9440(10)63332-9.md" + outsider.write_text("---\nreference_id: x\n---\n\n## Content\n\nB.\n") + + # The index is stale here by design (per-process); not asserted, so that a + # future self-invalidating index is an improvement rather than a failure. + fetcher.forget_cache_listing() + + assert fetcher.get_cache_path(LOWER).name == outsider.name + + +def test_two_fetchers_on_one_directory_share_the_index(tmp_path): + """``Repairer`` holds two fetchers over one cache directory. + + ``SupportingTextValidator`` builds its own ``ReferenceFetcher`` and + ``Repairer`` builds another, and a repair run interleaves both across many + references. With a per-instance index, every file one writes is invisible + to the other, so a DOI arriving uppercase through one and lowercase through + the other misses and writes the second file -- the defect this change + removes, surviving in the code path most likely to hit it. + """ + config = ReferenceValidationConfig(cache_dir=tmp_path) + first, second = ReferenceFetcher(config), ReferenceFetcher(config) + + second.get_cache_path(LOWER) # indexes the directory while it is empty + first._save_to_disk( + ReferenceContent( + reference_id=UPPER, title="A paper", content="Body.", + content_type="abstract_only", + ) + ) + second._save_to_disk( + ReferenceContent( + reference_id=LOWER, title="A paper", content="Body.", + content_type="abstract_only", + ) + ) + + assert len(list(tmp_path.glob("*.md"))) == 1, "one reference, one file" + + +def test_an_undecodable_entry_can_still_be_overwritten(fetcher, tmp_path): + """The file that most needs rewriting must not be the one that cannot be. + + Before this change a mangled entry was simply overwritten and repaired. + Reading the stored ``reference_id`` must not turn that into a failed save. + """ + path = tmp_path / "DOI_10.1_x.md" + path.write_bytes(b"---\nreference_id: DOI:10.1/x\ntitle: \xff\xfe bad\n---\n") + + fetcher._save_to_disk( + ReferenceContent( + reference_id="DOI:10.1/x", title="Repaired", content="B.", + content_type="abstract_only", + ) + ) + + assert "title: Repaired" in path.read_text(encoding="utf-8") + + +def test_stored_reference_id_is_preserved_for_doi_only(fetcher, tmp_path): + """The justification is DOI case-insensitivity, so the behaviour is too. + + Sanitization collapses ``:`` ``/`` ``?`` ``=`` to ``_``, so two distinct + ``url:`` references can share a filename. That collision predates this + change; cementing the first writer's id on every later write would change + how it is handled, for an identifier type the argument does not cover. + """ + fetcher._save_to_disk( + ReferenceContent( + reference_id="url:https://ex.com/a?b", title="First", content="B.", + content_type="abstract_only", + ) + ) + fetcher._save_to_disk( + ReferenceContent( + reference_id="url:https://ex.com/a/b", title="Second", content="B.", + content_type="abstract_only", + ) + ) + + stored = fetcher.get_cache_path("url:https://ex.com/a/b").read_text(encoding="utf-8") + assert "reference_id: url:https://ex.com/a/b" in stored, "not cemented for url:"