From ea73b1475c202caa19b109aa835487b46d7774b2 Mon Sep 17 00:00:00 2001 From: Tarik Moussa Date: Wed, 17 Jun 2026 00:31:01 +0200 Subject: [PATCH] fix(openalex): normalize DOI to bare canonical id so re-ingest is idempotent (DQ-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openalex._map_paper set Paper.id from the raw `doi` field, which OpenAlex returns as a full URL (https://doi.org/10.x). The canonical papers.id (M-1 migration) is the bare, lower-cased DOI, so re-ingesting a DOI paper missed ON CONFLICT (id) and tripped the separate papers_openalex_id_key constraint (UniqueViolation). Fix: _normalize_doi() strips https://doi.org//http://doi.org//doi: and lower-cases; applied in the DOI branch of _map_paper only (W-id fallback untouched). Also un-breaks _s2_id_for/Crossref recovery, which the URL form silently defeated. Corrected the _SAMPLE_WORK fixture to the URL form (the bare fixture hid the bug) + added _normalize_doi and re-ingest idempotency regression tests. Live verification: re-ingest of 10.1007/s00454-019-00132-8 (previously crashed) now UPDATEs in place — single row, added_at unchanged, no stray row, citations 45->45. Unblocks roadmap R-A/R-C. Full suite 348 green. Co-Authored-By: Claude Opus 4.8 --- codex/sources/openalex.py | 23 +++++++++- docs/audit/DATA-QUALITY-2026-06-15.md | 61 +++++++++++++++++++++------ tests/ingest/test_ingest.py | 58 +++++++++++++++++++++++++ tests/sources/test_openalex.py | 54 +++++++++++++++++++++++- 4 files changed, 181 insertions(+), 15 deletions(-) diff --git a/codex/sources/openalex.py b/codex/sources/openalex.py index 80b5307..f9be139 100644 --- a/codex/sources/openalex.py +++ b/codex/sources/openalex.py @@ -85,13 +85,34 @@ def _reconstruct_abstract(inverted_index: dict[str, list[int]] | None) -> str | return " ".join(w for _, w in word_positions) +def _normalize_doi(doi: str) -> str: + """Normalize an OpenAlex DOI to the canonical bare, lower-cased form. + + OpenAlex returns the ``doi`` field as a full URL + (``https://doi.org/10.1007/s00454-019-00132-8``), but the canonical + ``papers.id`` produced by the M-1 migration is the BARE, lower-cased DOI + (``10.1007/s00454-019-00132-8``). Stripping the ``https://doi.org/`` / + ``doi:`` prefix and lower-casing (DOIs are case-insensitive by spec) makes a + re-ingested DOI paper match its stored row, so the ingest + ``INSERT … ON CONFLICT (id)`` updates in place instead of attempting a fresh + INSERT that trips the separate ``papers_openalex_id_key`` unique constraint + (DQ-5). Mirrors the cited-id convention in ``ingest._norm_cited_id``. + """ + s = doi.strip().lower() + for prefix in ("https://doi.org/", "http://doi.org/", "doi:"): + if s.startswith(prefix): + return s[len(prefix) :] + return s + + def _map_paper(data: dict[str, Any]) -> Paper: authors: list[str] = [ a.get("author", {}).get("display_name") or "" for a in data.get("authorships", []) ] abstract = _reconstruct_abstract(data.get("abstract_inverted_index")) + doi = data.get("doi") return Paper( - id=data.get("doi") or data.get("id") or "", + id=_normalize_doi(doi) if doi else (data.get("id") or ""), title=data.get("title") or "", openalex_id=data.get("id"), authors=authors, diff --git a/docs/audit/DATA-QUALITY-2026-06-15.md b/docs/audit/DATA-QUALITY-2026-06-15.md index 1d46ea4..88d62ee 100644 --- a/docs/audit/DATA-QUALITY-2026-06-15.md +++ b/docs/audit/DATA-QUALITY-2026-06-15.md @@ -6,8 +6,9 @@ strong after fixing a P0 `codex search paper` crash. **DQ-2 RESOLVED 2026-06-16* (1911.00966 fully recovered from arXiv; s00454 abstract from S2; book chapter is a documented limit; recovery wired into ingest). **DQ-3 RESOLVED 2026-06-16** (content fidelity clean — full coverage, only OCR junk dropped). **All four -assessment items (DQ-1..DQ-4) DONE. New: DQ-5** (`ingest_paper` not idempotent -for DOI papers — blocks R-A/R-C, tracked separately). +assessment items (DQ-1..DQ-4) DONE. DQ-5 RESOLVED 2026-06-17** (DOI id normalized +to bare canonical form in `openalex._map_paper`; live re-ingest of +`10.1007/s00454-019-00132-8` confirmed idempotent → **unblocks R-A/R-C**). **Roadmap R-A..R-E** added 2026-06-16 (free-source acquisition levers — see Roadmap section). **Author:** Audit-Loop (Opus). **Intended reader:** a *cold* session with no memory of the audit conversation — everything needed to continue is in this file. @@ -239,7 +240,43 @@ is fine (monotonic); only the absolute score label is misleading. - **Acceptance:** a short relevance table (query → top-5 → on/off-topic) good enough to trust the KB for lookups, or a list of failure modes to fix. -### DQ-5 — `ingest_paper` is NOT idempotent for DOI papers · **HIGH (blocks R-A/R-C)** · found 2026-06-16 +### DQ-5 — `ingest_paper` is NOT idempotent for DOI papers · **RESOLVED 2026-06-17** (found 2026-06-16) + +**Resolution (what was done):** +- **Root-cause fix, minimal + contained:** added `openalex._normalize_doi()` and + applied it in `_map_paper` so the DOI branch yields the canonical **bare, + lower-cased** form (strips `https://doi.org/` / `http://doi.org/` / `doi:`; + lower-cases — DOIs are case-insensitive, matching `ingest._norm_cited_id`). The + no-DOI fallback (OpenAlex W-id URL) is left untouched. `_map_paper`→`fetch_paper` + →`ingest_paper` is the only consumer chain, so blast radius is one function. +- **Checked nothing depended on the URL form.** It was the opposite: the full-URL + id silently *defeated* `_s2_id_for(paper.id)` and `_recover_abstract`'s + `paper.id.startswith("10.")` (a `https://…` string is neither `10.`- nor + `doi:`-prefixed), so S2/Crossref recovery never fired for DOI papers. The fix + repairs those paths too. +- **Tests (offline, mocked DB):** closed audit **T-3** — the OpenAlex fixture now + emits the real URL-form DOI and asserts `paper.id` is the bare form; added + `_normalize_doi` unit tests (https/http/`doi:`/upper-case/already-bare) and a + W-id-fallback test; added ingest regression `test_ingest_doi_paper_upserts_bare + _canonical_id` (runs the real `_map_paper` through ingest, asserts the papers + upsert carries the **bare** id into `%(id)s` and uses `ON CONFLICT (id)`). + Verified it has teeth (fails on the pre-fix code). Full suite **348 passed**; + ruff + mypy --strict clean. +- **Live idempotency confirmed (tunnel up):** re-ingested + `10.1007/s00454-019-00132-8` against the Jetson DB → no `UniqueViolation`; + `IngestResult(paper_id='10.1007/s00454-019-00132-8', citations_upserted=45)`. + After-state: **UPDATE in place** — `id` and `openalex_id` unchanged, **`added_at` + unchanged** (proves no fresh INSERT), exactly **1** row for the DOI (no URL-form + duplicate), citations 45→45, total papers 29→29. +- **Unblocks R-A (GROBID re-ingest) and R-C (.tex re-ingest)** — both re-ingest + existing papers. **Files touched:** `codex/sources/openalex.py`, + `tests/sources/test_openalex.py`, `tests/ingest/test_ingest.py`. +- **Note (out of scope, pre-existing):** arXiv papers stored as bare ids + (`2305.10988`) are still not round-trip idempotent if OpenAlex returns an + arXiv-DOI/W-id instead of the bare arXiv id — a separate canonicalization + concern, not this UniqueViolation. The DOI path (this finding) is fixed. + +**Original finding (for context):** - **Symptom:** re-ingesting an existing DOI paper crashes with `UniqueViolation: papers_openalex_id_key`. Surfaced trying to re-ingest `10.1007/s00454-019-00132-8` during the DQ-2 backfill. @@ -276,8 +313,8 @@ Paywall/uni-login was explicitly considered and **deferred** (see R-E). Each ite is self-contained so a cold session can pick it up. ### R-A — Run GROBID reference extraction on arXiv PDFs · **HIGH lever, LOW-MED effort** -- **⚠ Blocked by DQ-5:** re-ingesting existing DOI papers currently crashes - (`papers_openalex_id_key`). Fix DQ-5 first, or the GROBID re-ingest aborts. +- **✓ Unblocked (DQ-5 RESOLVED 2026-06-17):** DOI re-ingest is now idempotent, so + the GROBID re-ingest no longer aborts on `papers_openalex_id_key`. - **What:** the whole corpus was ingested from `.txt` (`PAPERS_DIR=…/papers/txt`), so the GROBID PDF path never ran. Re-ingest with a PDF `source_path` per paper so `codex.parsing.grobid.extract_references` parses each bibliography. @@ -350,13 +387,13 @@ is self-contained so a cold session can pick it up. from S2; book chapter documented; recovery wired into ingest). 4. ~~**DQ-3**~~ — **DONE 2026-06-16** (content fidelity clean; full coverage, only 18 OCR-junk chunks dropped, no math lost). **Assessment axis DQ-1..DQ-4 complete.** -5. **DQ-5** (NEXT — `ingest_paper` not idempotent for DOI papers; **blocks R-A/R-C**; - careful — M-1 canonical-id territory). -6. **Roadmap levers** (see section above): **R-A** (GROBID on arXiv PDFs — closes - the last 2 zero-citation theses; needs DQ-5 first), **R-B** (Crossref refs — - abstract half already shipped in DQ-2), **R-C** (`.tex` ingest — serves DQ-3; - needs DQ-5 first), **R-D** (F-15 coverage warning, quick). **R-E** (paywall) - deferred. +5. ~~**DQ-5**~~ — **DONE 2026-06-17** (DOI id normalized to bare form in + `openalex._map_paper`; live re-ingest of `10.1007/s00454-019-00132-8` idempotent + — UPDATE in place, `added_at` unchanged; suite 348 green). **Unblocks R-A/R-C.** +6. **Roadmap levers** (see section above — now all unblocked): **R-A** (GROBID on + arXiv PDFs — closes the last 2 zero-citation theses), **R-B** (Crossref refs — + abstract half already shipped in DQ-2), **R-C** (`.tex` ingest — serves DQ-3), + **R-D** (F-15 coverage warning, quick). **R-E** (paywall) deferred. DQ-1..DQ-4 are *data*/assessment work; DQ-5 + roadmap R-A..R-D add **new code** (canonical-id fix, re-ingest scripts, Crossref references, a `.tex` path, a graph diff --git a/tests/ingest/test_ingest.py b/tests/ingest/test_ingest.py index 5729466..5011fbf 100644 --- a/tests/ingest/test_ingest.py +++ b/tests/ingest/test_ingest.py @@ -10,6 +10,7 @@ from contextlib import contextmanager from typing import Any from unittest.mock import MagicMock, patch +import httpx import numpy as np import pytest @@ -592,6 +593,63 @@ def test_ingest_sets_bibkey_when_none(tmp_path: Any) -> None: assert upsert_params["bibkey"] == "AliceBob2023" +def test_ingest_doi_paper_upserts_bare_canonical_id() -> None: + """DQ-5 regression: re-ingesting an existing DOI paper must be idempotent. + + OpenAlex returns the ``doi`` as a full URL (and possibly mixed-case), but the + stored canonical ``papers.id`` is the BARE, lower-cased DOI (M-1 migration). + The papers upsert must therefore carry the BARE id into ``%(id)s`` so + ``ON CONFLICT (id)`` matches the existing row and UPDATEs it — rather than + attempting a fresh INSERT that trips the separate ``papers_openalex_id_key`` + unique constraint (the UniqueViolation crash). + + The real ``_map_paper``/``fetch_paper`` run here (only the HTTP ``_get``, the + embedder and the DB are mocked), so this exercises the actual normalization + through the ingest path. The DB is a MagicMock, so it cannot raise the real + constraint error; the live re-ingest of 10.1007/s00454-019-00132-8 is the + end-to-end UniqueViolation proof (see DATA-QUALITY-2026-06-15.md, DQ-5). + """ + bare = "10.1007/s00454-019-00132-8" + work = { + "id": "https://openalex.org/W2899262408", + # URL form + an upper-case suffix char — the worst case for ON CONFLICT. + "doi": "https://doi.org/10.1007/S00454-019-00132-8", + "title": "A Test Paper", + "publication_year": 2019, + "authorships": [{"author": {"display_name": "Alice Smith"}}], + "abstract_inverted_index": {"Hello": [0], "world": [1]}, + # Non-empty so fetch_citations yields edges and the S2 fallback is skipped. + "referenced_works": ["https://openalex.org/W1"], + } + + mock_conn = MagicMock() + mock_conn.execute = MagicMock() + mock_conn.executemany = MagicMock() + mock_conn.commit = MagicMock() + + with ( + patch( + "codex.ingest.openalex._get", + return_value=httpx.Response(200, json=work), + ), + patch("codex.ingest.get_embedder", return_value=_fake_embedder()), + patch("codex.ingest.get_conn", side_effect=_make_conn_cm(mock_conn)), + ): + result = ingest_paper(bare) + + # The IngestResult and the upsert must use the bare canonical id, not the URL. + assert result.paper_id == bare + + upsert_sql: str = mock_conn.execute.call_args_list[0][0][0] + upsert_params: dict[str, Any] = mock_conn.execute.call_args_list[0][0][1] + assert "INSERT INTO papers" in upsert_sql + assert "ON CONFLICT (id)" in upsert_sql + assert upsert_params["id"] == bare, ( + f"upsert id must be the bare canonical DOI '{bare}', " + f"not the OpenAlex URL form '{upsert_params['id']}'" + ) + + # --------------------------------------------------------------------------- diff --git a/tests/sources/test_openalex.py b/tests/sources/test_openalex.py index d481aa2..6b1e9a2 100644 --- a/tests/sources/test_openalex.py +++ b/tests/sources/test_openalex.py @@ -7,7 +7,7 @@ import pytest from codex.models import Citation, Paper from codex.sources import openalex -from codex.sources.openalex import _resolve_id +from codex.sources.openalex import _normalize_doi, _resolve_id # --------------------------------------------------------------------------- # Helpers @@ -15,7 +15,9 @@ from codex.sources.openalex import _resolve_id _SAMPLE_WORK = { "id": "https://openalex.org/W2741809807", - "doi": "10.1145/3592430", + # Real OpenAlex returns the DOI as a full URL (T-3: the old bare fixture hid + # the URL-form behaviour that DQ-5 is about). + "doi": "https://doi.org/10.1145/3592430", "title": "Conformal Prediction: A Review", "publication_year": 2023, "authorships": [ @@ -67,6 +69,51 @@ def test_resolve_prefixed_unchanged() -> None: assert _resolve_id("doi:10.1145/x") == "doi:10.1145/x" +# --------------------------------------------------------------------------- +# _normalize_doi — canonical bare, lower-cased form (DQ-5) +# --------------------------------------------------------------------------- + + +def test_normalize_doi_strips_https_url() -> None: + # The exact form OpenAlex returns in the `doi` field. + assert _normalize_doi("https://doi.org/10.1007/s00454-019-00132-8") == ( + "10.1007/s00454-019-00132-8" + ) + + +def test_normalize_doi_strips_http_and_doi_prefix() -> None: + assert _normalize_doi("http://doi.org/10.1145/3592430") == "10.1145/3592430" + assert _normalize_doi("doi:10.1145/3592430") == "10.1145/3592430" + + +def test_normalize_doi_lowercases() -> None: + # DOIs are case-insensitive; the canonical id (and _norm_cited_id) is lower-case. + assert _normalize_doi("https://doi.org/10.1007/S00454-019-00132-8") == ( + "10.1007/s00454-019-00132-8" + ) + + +def test_normalize_doi_already_bare_is_idempotent() -> None: + assert _normalize_doi("10.1007/s00454-019-00132-8") == "10.1007/s00454-019-00132-8" + + +def test_map_paper_falls_back_to_openalex_id_without_doi( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A work with no DOI keeps the OpenAlex W-id (URL form) as id, unchanged — + normalization only rewrites the DOI branch.""" + work = {k: v for k, v in _SAMPLE_WORK.items() if k != "doi"} + + def mock_get(url: str, params: dict[str, str] | None = None) -> httpx.Response: + return httpx.Response(200, json=work) + + monkeypatch.setattr(openalex, "_get", mock_get) + paper = openalex.fetch_paper("W2741809807") + + assert paper is not None + assert paper.id == "https://openalex.org/W2741809807" + + # --------------------------------------------------------------------------- # fetch_paper — success # --------------------------------------------------------------------------- @@ -85,6 +132,9 @@ def test_fetch_paper_success(monkeypatch: pytest.MonkeyPatch) -> None: assert paper is not None assert isinstance(paper, Paper) + # DQ-5 / C-7: paper.id is the canonical BARE DOI, not the full URL OpenAlex + # returns. This is what makes the ingest ON CONFLICT (id) upsert idempotent. + assert paper.id == "10.1145/3592430" assert paper.title == "Conformal Prediction: A Review" assert paper.year == 2023 assert paper.authors == ["Alice Smith", "Bob Jones"]