From c61044d4e5d51c102de7ee09bbe288ba7cfaf801 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Sun, 20 Sep 2026 16:40:14 -0700 Subject: [PATCH 1/3] Keep a line break in metadata, and one cache file per DOI Two ways a cache entry's identity or content is lost on the way to disk. Both were found downstream, where each had been worked around with a runtime monkeypatch over this module's internals; they are ordinary defects here. A literal line break ends a YAML scalar, and `_quote_yaml_value` quotes on a character list that does not include one. A Crossref title containing a newline is therefore interpolated straight into the frontmatter and the entry stops parsing -- and re-fetching writes the same broken file, so nothing recovers it. Such a value is now emitted JSON-style, which is valid YAML and reloads with the break intact. The guard is `splitlines() != [value]`, which covers every break the scanner honours -- U+0085, U+2028 and U+2029 as well as \n and \r -- plus the vertical tab and form feed its reader rejects as non-printable; all five were verified to produce the same unparseable file. `len(...) > 1` would not do, since a trailing break splits to one element. json.dumps leaves NEL, LS and PS literal even with ensure_ascii=False, so those are escaped explicitly; emitted raw inside a quoted scalar they fold to a space on reload, which is the silent metadata edit the JSON form exists to avoid. Folding it to a space would also parse, but it silently edits metadata that is compared against the fetched record elsewhere. DOI names are case-insensitive by specification, so DOI:10.1016/S0002-9440(10) 63332-9 and its lowercase spelling are one reference. The prefix is normalized and the suffix is not, so each spelling got its own cache file: a duplicate download, and two committed entries for one paper in a project that versions its cache. A DOI path now resolves to whatever spelling is already on disk. Resolution is memoized per directory. Scanning on every call is quadratic over a cache, and `_cache_path` is on the read path and the write path both: measured against a 6,721-entry directory, 6.7 ms a call, or 45 seconds of pure path resolution for a run touching every reference. One scan brings that to 0.57 s. Where both spellings are already present -- what a case-sensitive checkout of an already-duplicated cache looks like -- the caller's exact spelling wins and the lexicographically first is the tiebreak, so the answer does not depend on directory order. An existing entry also keeps the reference_id recorded inside it. The path resolving to one file is not enough on its own: rewriting that line under the caller's capitalization leaves a one-line diff in a committed cache on every cross-spelling re-fetch, which is the churn this removes, one layer in. Resolution looks for the existing file rather than imposing a canonical spelling, so no cache is renamed underneath anyone -- one downstream project holds 6,721 DOI entries and sees no diff. The directory scan comes first and `exists()` is deliberately not consulted: on a case-insensitive filesystem it is true for a spelling that is not the one on disk, so trusting it would return the caller's spelling and write the colliding second file this is meant to prevent. Scoped to DOI. A PMID or an NCT id is not case-insensitive, and folding those would merge genuinely distinct references. Co-Authored-By: Claude Opus 5 (1M context) --- .../etl/reference_fetcher.py | 130 +++++++++- tests/test_frontmatter_and_doi_case.py | 231 ++++++++++++++++++ 2 files changed, 359 insertions(+), 2 deletions(-) create mode 100644 tests/test_frontmatter_and_doi_case.py diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 298ef95..aa93861 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,21 @@ def _text_after_abstract(content: Optional[str]) -> str: return body if separator else text +#: Line breaks the YAML scanner honours that ``json.dumps`` leaves literal even +#: with ``ensure_ascii=False``. Emitted inside a double-quoted scalar they are +#: folded to a space on reload, which is the silent metadata edit the JSON form +#: exists to avoid. The control characters below 0x20 are already escaped. +_UNESCAPED_YAML_BREAKS = {"\x85": "\\u0085", "\u2028": "\\u2028", "\u2029": "\\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) + for literal, escape in _UNESCAPED_YAML_BREAKS.items(): + rendered = rendered.replace(literal, escape) + return rendered + + class _RefreshLoss: """Describe what a refused refresh returned, formatted only if logged. @@ -227,6 +243,7 @@ def __init__(self, config: ReferenceValidationConfig): # obtained: a stale fallback stays flagged on every later read in this # process instead of being laundered into a fresh-looking hit. self._cache: dict[str, FetchOutcome] = {} + self._case_indexes: dict[Path, dict[str, set[str]]] = {} self._acquirer = ContentAcquirer() # Build the PDF extractor once: this validates config.pdf_backend up front # (an unknown backend raises here, at init, rather than mid-fetch) and avoids @@ -1057,7 +1074,87 @@ 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.""" + try: + with cache_path.open(encoding="utf-8") 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) + + def _case_index(self, cache_dir: Path) -> dict[str, set[str]]: + """Casefolded name -> real names, built once per directory. + + ``_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. + """ + cached = self._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) + self._case_indexes[cache_dir] = index + return index + + def _remember_cache_file(self, path: Path) -> None: + """Record a newly written file so a later lookup in another case finds it.""" + index = self._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 +1179,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]: + return _json_scalar(value) + # Characters that require quoting in YAML values special_chars = '[]{}:,#&*?|<>=!%@`"\'\\' needs_quote = False @@ -1126,7 +1245,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 +1366,8 @@ def _save_to_disk( lines.append(reference.content) cache_path.write_text("\n".join(lines), encoding="utf-8") + self._remember_cache_file(cache_path) + 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..4bed620 --- /dev/null +++ b/tests/test_frontmatter_and_doi_case.py @@ -0,0 +1,231 @@ +"""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", + ], +) +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}" From fdf0cee70cd5be07e2382e509e9a4d537b18a927 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Sun, 20 Sep 2026 17:45:17 -0700 Subject: [PATCH 2/3] Let a caller drop the cached directory listing The DOI case index is per-instance and per-process, so a file written by anything else -- another process, or a subprocess this one shelled out to -- is invisible to it until the process ends. That window is not hypothetical. A downstream backfill script shells out to fetch a missing reference; the subprocess writes into a directory the parent has already indexed, and a later lookup of the same DOI in another capitalization does not find the new file (monarch-initiative/dismech#12083). Re-scanning on every miss would close the window and undo the memoization the previous commit added, so the remedy is explicit: `forget_cache_listing()` drops the listings, and the next resolution that needs one rebuilds it. The docstring now states the window rather than leaving it to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) --- .../etl/reference_fetcher.py | 19 ++++++++++++++++ tests/test_frontmatter_and_doi_case.py | 22 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index aa93861..cc34afd 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -1139,6 +1139,15 @@ def _case_index(self, cache_dir: Path) -> dict[str, set[str]]: 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 per-instance and per-process. Files this fetcher writes are + added as they are written, but one written by anything else -- another + process, or a subprocess this one shelled out to -- is not seen until + :meth:`forget_cache_listing`. A downstream backfill script met exactly + that: it shells out to fetch a missing reference, and a later lookup of + the same DOI in another capitalization did not find the new file. + Re-scanning on every miss would close the window and undo the + memoization, so the remedy is explicit. """ cached = self._case_indexes.get(cache_dir) if cached is not None: @@ -1150,6 +1159,16 @@ def _case_index(self, cache_dir: Path) -> dict[str, set[str]]: self._case_indexes[cache_dir] = index return index + def forget_cache_listing(self) -> None: + """Drop the cached directory listings used to resolve DOI capitalization. + + Call this after something outside this fetcher has written to a cache + directory -- a subprocess, or a concurrent run -- so the next DOI lookup + sees the new files. Cheap: the listing is rebuilt on the next resolution + that needs it. + """ + self._case_indexes.clear() + def _remember_cache_file(self, path: Path) -> None: """Record a newly written file so a later lookup in another case finds it.""" index = self._case_indexes.get(path.parent) diff --git a/tests/test_frontmatter_and_doi_case.py b/tests/test_frontmatter_and_doi_case.py index 4bed620..b8409d6 100644 --- a/tests/test_frontmatter_and_doi_case.py +++ b/tests/test_frontmatter_and_doi_case.py @@ -229,3 +229,25 @@ def counting_iterdir(self): 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") + + assert fetcher.get_cache_path(LOWER).name != outsider.name, "stale, as documented" + + fetcher.forget_cache_listing() + + assert fetcher.get_cache_path(LOWER).name == outsider.name From 0311ee20b59488775182ec69a2dffad0601ce530 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Sun, 20 Sep 2026 18:25:21 -0700 Subject: [PATCH 3/3] Share the DOI case index across a process, and escape what the reader rejects Five fixes from review, kept as their own commit so the delta is readable. The case index was per-instance, and Repairer holds two fetchers over one cache directory -- its own and the one inside SupportingTextValidator -- interleaved across many references. Each one's writes were invisible to the other, so a DOI arriving uppercase through one and lowercase through the other still wrote two files: the defect this branch removes, surviving in the path most likely to hit it. The index is now module-level and keyed by directory, so every write already funnelling through _remember_cache_file keeps it accurate process-wide, and the per-directory scan is still paid once rather than once per instance. forget_cache_listing() is left covering exactly the window it is named for, a writer outside this process. json.dumps escapes only below 0x20, so DEL and the C1 block reached the file raw, and a raw C1 byte makes ruamel's reader refuse the entry outright -- verified for \x7f, \x80 and \x92. 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. The escape set is now a regex over the characters the reader rejects or the scanner folds, and it also decides the routing, so such a value reaches the JSON form in the first place. _stored_reference_id read the existing entry as strict UTF-8, so an entry too mangled to decode became one that could not be overwritten -- the file that most needs rewriting. It reads with errors="replace"; only the reference_id line is used. It is also now applied to DOI references alone, which is what the argument covers: sanitization collapses : / ? and = to _, so two distinct url: references can share a filename, and cementing the first writer's id there would change how a pre-existing collision is handled for an identifier type the DOI case-insensitivity argument says nothing about. Also drops a duplicated _remember_cache_file call, and stops a test asserting that a stale read stays stale -- pinning the absence of a fix makes a later self-invalidating index fail the suite as though it were a regression. Co-Authored-By: Claude Opus 5 (1M context) --- .../etl/reference_fetcher.py | 94 ++++++++++++------- tests/test_frontmatter_and_doi_case.py | 85 ++++++++++++++++- 2 files changed, 144 insertions(+), 35 deletions(-) diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index cc34afd..4b7a3d6 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -134,19 +134,26 @@ def _text_after_abstract(content: Optional[str]) -> str: return body if separator else text -#: Line breaks the YAML scanner honours that ``json.dumps`` leaves literal even -#: with ``ensure_ascii=False``. Emitted inside a double-quoted scalar they are -#: folded to a space on reload, which is the silent metadata edit the JSON form -#: exists to avoid. The control characters below 0x20 are already escaped. -_UNESCAPED_YAML_BREAKS = {"\x85": "\\u0085", "\u2028": "\\u2028", "\u2029": "\\u2029"} +#: 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) - for literal, escape in _UNESCAPED_YAML_BREAKS.items(): - rendered = rendered.replace(literal, escape) - return rendered + return _YAML_UNSAFE.sub(lambda m: f"\\u{ord(m.group(0)):04x}", rendered) class _RefreshLoss: @@ -243,7 +250,6 @@ def __init__(self, config: ReferenceValidationConfig): # obtained: a stale fallback stays flagged on every later read in this # process instead of being laundered into a fresh-looking hit. self._cache: dict[str, FetchOutcome] = {} - self._case_indexes: dict[Path, dict[str, set[str]]] = {} self._acquirer = ContentAcquirer() # Build the PDF extractor once: this validates config.pdf_backend up front # (an unknown backend raises here, at init, rather than mid-fetch) and avoids @@ -1081,9 +1087,25 @@ def _cache_path(self, reference_id: str, cache_dir: Path) -> Path: @staticmethod def _stored_reference_id(cache_path: Path, reference: ReferenceContent) -> str: - """The id already recorded in ``cache_path``, else the reference's own.""" + """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") as handle: + 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() @@ -1132,46 +1154,53 @@ def _existing_case_variant(self, path: Path) -> Path: return path return path.parent / min(names) - def _case_index(self, cache_dir: Path) -> dict[str, set[str]]: - """Casefolded name -> real names, built once per directory. + @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 per-instance and per-process. Files this fetcher writes are - added as they are written, but one written by anything else -- another - process, or a subprocess this one shelled out to -- is not seen until - :meth:`forget_cache_listing`. A downstream backfill script met exactly - that: it shells out to fetch a missing reference, and a later lookup of - the same DOI in another capitalization did not find the new file. - Re-scanning on every miss would close the window and undo the - memoization, so the remedy is explicit. + 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 = self._case_indexes.get(cache_dir) + 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) - self._case_indexes[cache_dir] = index + _CASE_INDEXES[cache_dir] = index return index - def forget_cache_listing(self) -> None: + @staticmethod + def forget_cache_listing() -> None: """Drop the cached directory listings used to resolve DOI capitalization. - Call this after something outside this fetcher has written to a cache - directory -- a subprocess, or a concurrent run -- so the next DOI lookup - sees the new files. Cheap: the listing is rebuilt on the next resolution - that needs it. + 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. """ - self._case_indexes.clear() + _CASE_INDEXES.clear() - def _remember_cache_file(self, path: Path) -> None: + @staticmethod + def _remember_cache_file(path: Path) -> None: """Record a newly written file so a later lookup in another case finds it.""" - index = self._case_indexes.get(path.parent) + index = _CASE_INDEXES.get(path.parent) if index is not None: index.setdefault(path.name.casefold(), set()).add(path.name) @@ -1218,7 +1247,7 @@ def _quote_yaml_value(self, value: str) -> str: # 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]: + if value.splitlines() != [value] or _YAML_UNSAFE.search(value): return _json_scalar(value) # Characters that require quoting in YAML values @@ -1386,7 +1415,6 @@ def _save_to_disk( cache_path.write_text("\n".join(lines), encoding="utf-8") self._remember_cache_file(cache_path) - 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 index b8409d6..1f686f8 100644 --- a/tests/test_frontmatter_and_doi_case.py +++ b/tests/test_frontmatter_and_doi_case.py @@ -51,6 +51,13 @@ def fetcher(tmp_path): "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): @@ -246,8 +253,82 @@ def test_a_file_written_by_another_process_is_found_after_a_reset(fetcher, tmp_p 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") - assert fetcher.get_cache_path(LOWER).name != outsider.name, "stale, as documented" - + # 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:"