From 5d87b22a1c90505b2a4d82d462439548bd213b0b Mon Sep 17 00:00:00 2001 From: Derek Gulbranson Date: Sat, 17 Oct 2026 20:51:43 -0700 Subject: [PATCH] Fix the name_usage index defects found reviewing #712's fixes - A batch that overran under a new listing and was cached as halves left the old whole-batch file beside them, and that file was read first: a complete index loaded as missing offline and re-sent the overrunning query on every run. The halved marker is read first now, and the old file is removed when a batch is halved. - Batches were keyed by their first and last item, so a re-listing that changed an item in between raised "unusable" or silently missed the new item. Each batch file now carries a digest of its whole membership and is fetched again when that changes. - --refresh-index with only Latin words deleted the index and, if the rebuild failed, exited 0 with nothing said. The failure is printed and the exit status is 2. - HTTPErrors are closed wherever the report lets go of one (the Wikidata row, the ends-with row, a pairing, a source loader), not only in the retry and overrun paths; 3.14 warns on one left open. - _CJK_CHARS reaches Extension H; the separate compatibility-supplement range sat inside it. INDEX_VERSION 3, so the cache is rebuilt. Tests: a pre-versioning cache (no version file), each end and the middle of a batch changing, the stale whole batch beside its halves, corrupt batch shapes ([] and short or non-string rows), a listing with no item column, the pairing union with the larger chunk first, closed HTTPErrors on each path, and the refresh failure. Co-Authored-By: Claude Opus 5.5 --- tests/v2/test_name_usage_tool.py | 161 ++++++++++++++++++++++++++++++- tools/name_usage.py | 90 +++++++++++------ 2 files changed, 216 insertions(+), 35 deletions(-) diff --git a/tests/v2/test_name_usage_tool.py b/tests/v2/test_name_usage_tool.py index a539e0d1..e79ec4f4 100644 --- a/tests/v2/test_name_usage_tool.py +++ b/tests/v2/test_name_usage_tool.py @@ -753,14 +753,20 @@ def test_the_listing_count_allows_a_thousandth_of_slack( ("Q101352-ids.json", '["Q1", "L5"]'), ("Q101352-ids.json", "[]"), ("Q101352-0-1.json", '{"rows": []}'), - ("Q101352-0-1.json", '{"fetched": "x", "span": ["Q1", "Q1"], "rows": null}'), - ("Q101352-0-1.json", '{"fetched": "x", "span": ["Q1", "Q1"], "rows": [["Q9", "殿"]]}'), + ("Q101352-0-1.json", '[]'), + ("Q101352-0-1.json", '{"fetched": "x", "digest": "DIGEST", "rows": null}'), + ("Q101352-0-1.json", '{"fetched": "x", "digest": "DIGEST", "rows": [["Q9", "殿"]]}'), + ("Q101352-0-1.json", '{"fetched": "x", "digest": "DIGEST", "rows": [["Q1"]]}'), + ("Q101352-0-1.json", '{"fetched": "x", "digest": "DIGEST", "rows": [["Q1", 5]]}'), + ("version.json", "{"), ]) def test_an_unusable_cache_file_is_named_with_its_remedy( monkeypatch: pytest.MonkeyPatch, name: str, content: str) -> None: _stub_sparql(monkeypatch, [*_listing("Q1"), _labels(("Q1", "向殿"))]) tool.load_index(download=True) - (tool.cache_dir() / tool.INDEX_DIR / name).write_text(content) + # DIGEST stands for the batch's own, so the row checks are reached. + (tool.cache_dir() / tool.INDEX_DIR / name).write_text( + content.replace("DIGEST", tool._batch_digest(["Q1"]))) _stub_sparql(monkeypatch, []) with pytest.raises(ValueError, match=f"{name} is unusable.*--refresh-index"): tool.load_index(download=True) @@ -871,7 +877,9 @@ def test_a_boundary_count_is_chunked_and_counts_a_person_once( monkeypatch.setattr(tool, "ITEM_CHUNK", 1) index = _index(("Q1", "family", "阿部"), ("Q2", "family", "岡部"), ("Q3", "given", "長")) - queries = _stub_sparql(monkeypatch, [_people("Q9"), _people("Q9", "Q8")]) + # The larger answer first: a count that kept only the last chunk's + # people would read 1. + queries = _stub_sparql(monkeypatch, [_people("Q9", "Q8"), _people("Q9")]) usage = tool.ends_with_usage("部長", index, top=5) assert len(queries) == 2 assert usage.splits[0].people == 2 @@ -1001,3 +1009,148 @@ def test_main_reports_an_unusable_cache_without_losing_the_other_sources( assert tool.main(["--json", "殿"]) == 2 rows = json.loads(capsys.readouterr().out)["殿"] assert rows[-1]["status"] == "failed" and "unusable" in rows[-1]["note"] + + +# -- second review of #712 --------------------------------------------------- + +@pytest.mark.usefixtures("one_class") +@pytest.mark.parametrize("relisted", [ + ("Q1", "Q3"), # the first item kept + ("Q0", "Q2"), # the last item kept +]) +def test_a_batch_whose_items_changed_at_either_end_is_fetched_again( + monkeypatch: pytest.MonkeyPatch, relisted: tuple[str, str]) -> None: + _stub_sparql(monkeypatch, [*_listing("Q1", "Q2"), _labels(("Q1", "向殿"))]) + tool.load_index(download=True) + (tool.cache_dir() / tool.INDEX_DIR / "Q101352-ids.json").unlink() + queries = _stub_sparql(monkeypatch, [*_listing(*relisted), _labels()]) + tool.load_index(download=True) + assert _values(queries[2]) == [f"wd:{q}" for q in relisted] + + +@pytest.mark.usefixtures("one_class") +def test_a_batch_whose_middle_item_changed_is_fetched_again( + monkeypatch: pytest.MonkeyPatch) -> None: + # Same first and last items, another in between: the batch's whole + # membership is what its file is checked against. + monkeypatch.setattr(tool, "INDEX_BATCH", 3) + _stub_sparql(monkeypatch, [*_listing("Q1", "Q2", "Q4"), + _labels(("Q2", "向殿"))]) + tool.load_index(download=True) + (tool.cache_dir() / tool.INDEX_DIR / "Q101352-ids.json").unlink() + queries = _stub_sparql(monkeypatch, [*_listing("Q1", "Q3", "Q4"), + _labels(("Q3", "井殿"))]) + index = tool.load_index(download=True) + assert index is not None + assert [e.label for e in index.entries] == ["井殿"] + assert _values(queries[2]) == ["wd:Q1", "wd:Q3", "wd:Q4"] + + +@pytest.mark.usefixtures("one_class") +def test_a_batch_halved_under_a_new_listing_leaves_no_stale_whole( + monkeypatch: pytest.MonkeyPatch) -> None: + # Listing A fetched the batch whole; listing B overran it and cached + # its halves. The whole batch's old file must not shadow the halves, + # or the complete index reads as missing offline. + _stub_sparql(monkeypatch, [*_listing("Q1", "Q2"), _labels(("Q1", "向殿"))]) + tool.load_index(download=True) + (tool.cache_dir() / tool.INDEX_DIR / "Q101352-ids.json").unlink() + _stub_sparql(monkeypatch, [*_listing("Q1", "Q3"), _overrun(), + _labels(("Q1", "向殿")), _labels(("Q3", "井殿"))]) + tool.load_index(download=True) + _stub_sparql(monkeypatch, []) # any query fails the test + offline = tool.load_index(download=False) + assert offline is not None + assert sorted(e.label for e in offline.entries) == ["井殿", "向殿"] + online = tool.load_index(download=True) + assert online is not None and len(online.entries) == 2 + + +@pytest.mark.usefixtures("one_class") +def test_an_index_built_before_versioning_is_rebuilt( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str]) -> None: + # The first index (2026-10-17) had no version file at all, and its + # character class missed supplementary-plane Han. + _stub_sparql(monkeypatch, [*_listing("Q1"), _labels(("Q1", "向殿"))]) + tool.load_index(download=True) + (tool.cache_dir() / tool.INDEX_DIR / "version.json").unlink() + _stub_sparql(monkeypatch, []) + assert tool.load_index(download=False) is None + capsys.readouterr() + _stub_sparql(monkeypatch, [*_listing("Q1"), _labels(("Q1", "𠮷田"))]) + index = tool.load_index(download=True) + assert index is not None and index.entries[0].label == "𠮷田" + assert "older version" in capsys.readouterr().err + + +@pytest.mark.usefixtures("one_class") +def test_a_listing_without_an_item_column_is_an_error( + monkeypatch: pytest.MonkeyPatch) -> None: + _stub_sparql(monkeypatch, [[{"n": "1"}], [{"x": "y"}]]) + with pytest.raises(ValueError, match="not item"): + tool.load_index(download=True) + + +@pytest.mark.parametrize(("word", "cjk"), [ + ("\U00031350", True), # Extension H + ("\U0002f800", True), # the supplementary compatibility block +]) +def test_is_cjk_covers_extension_h(word: str, cjk: bool) -> None: + assert tool.is_cjk(word) is cjk + + +def _closable(code: int, body: bytes = b"") -> tuple[urllib.error.HTTPError, io.BytesIO]: + fp = io.BytesIO(body) + return urllib.error.HTTPError("u", code, "x", {}, fp), fp # type: ignore[arg-type] + + +@pytest.mark.usefixtures("one_class") +@pytest.mark.parametrize(("code", "body"), [ + (504, b""), (500, b"java.util.concurrent.TimeoutException"), (400, b"syntax")]) +def test_the_index_closes_each_http_error_it_reads( + monkeypatch: pytest.MonkeyPatch, code: int, body: bytes) -> None: + # Since 3.14 an HTTPError left open warns when collected. + error, fp = _closable(code, body) + _stub_sparql(monkeypatch, [*_listing("Q1", "Q2"), error, _labels(), _labels()]) + try: + tool.load_index(download=True) + except urllib.error.HTTPError: + pass + assert fp.closed + + +def test_a_failed_wikidata_row_closes_its_http_error( + monkeypatch: pytest.MonkeyPatch) -> None: + error, fp = _closable(400) + _stub_sparql(monkeypatch, [error]) + assert tool.wikidata_usage("shehu", top=5).status == "failed" + assert fp.closed + + +def test_a_failed_ends_with_row_and_pairing_close_their_http_errors( + monkeypatch: pytest.MonkeyPatch) -> None: + first, first_fp = _closable(400) + _stub_sparql(monkeypatch, [first]) + tool.ends_with_usage("殿", _index(("Q1", "family", "向殿")), top=5) + second, second_fp = _closable(400) + _stub_sparql(monkeypatch, [second]) + tool.ends_with_usage( + "部長", _index(("Q1", "family", "阿部"), ("Q3", "given", "長")), top=5) + assert first_fp.closed and second_fp.closed + + +def test_main_reports_a_refresh_that_failed_for_a_latin_word( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, + capsys: pytest.CaptureFixture[str]) -> None: + # A Latin word has no ends-with row to carry the failure, and the + # refresh has already deleted the old index. + monkeypatch.setenv("NAMEPARSER_TOOLS_CACHE", str(tmp_path)) + monkeypatch.setattr(tool, "load_census", lambda download: None) + + def broken(download: bool, refresh: bool = False) -> None: + raise OSError("down") + monkeypatch.setattr(tool, "load_index", broken) + _stub_sparql(monkeypatch, [[], []]) + assert tool.main(["--refresh-index", "knight"]) == 2 + assert "CJK name index: OSError: down" in capsys.readouterr().err diff --git a/tools/name_usage.py b/tools/name_usage.py index 87be7b17..aaca9ddd 100644 --- a/tools/name_usage.py +++ b/tools/name_usage.py @@ -66,6 +66,7 @@ import argparse import csv +import hashlib import http.client import io import json @@ -454,7 +455,7 @@ def wikidata_usage(word: str, top: int) -> Usage: _bindings(_sparql(wikidata_countries_query(forms))), top) except (OSError, ValueError, KeyError, http.client.HTTPException) as exc: return Usage("Wikidata people", status="failed", lower_bound=True, - note=f"query failed: {type(exc).__name__}: {exc}") + note=_failure(exc)) zero = "a zero proves nothing" if not (usage.given or usage.family) else "" usage.note = _join("matched as " + ", ".join(forms), zero) return usage @@ -576,10 +577,11 @@ def _download(url: str, path: Path, required_member: str) -> None: assert set(NAME_CLASSES.values()) == {"given", "family"}, NAME_CLASSES INDEX_DIR = "wikidata_cjk_names" -#: Bumped whenever what the index holds changes (version 2 widened the -#: character class to supplementary-plane Han); a cache built by another -#: version is rebuilt rather than trusted. -INDEX_VERSION = 2 +#: Bumped whenever what the index holds changes (2 widened the character +#: class to supplementary-plane Han, 3 to Extension H and keyed each batch +#: by its whole membership); a cache built by another version is rebuilt +#: rather than trusted. +INDEX_VERSION = 3 #: Items per label query. Measured 2026-10-17 on family-name items not #: queried before: 1,000 took 2.2-2.4s and 2,000 took 24s. INDEX_BATCH = 1000 @@ -591,14 +593,15 @@ def _download(url: str, path: Path, required_member: str) -> None: ENDS_WITH_SOURCE = "Wikidata people, name ends with" #: The iteration mark 々 and 〇, kana, the CJK ideograph blocks (Extension -#: A, the unified block, compatibility, and Extensions B onward in the -#: supplementary planes, where rare surname characters live), hangul -#: syllables, and halfwidth katakana. The Yijing hexagrams between -#: Extension A and the unified block are left out. One such letter makes -#: a label or a word CJK; the query and is_cjk share the class. -_CJK_CHARS = ("々〇぀-ヿ㐀-䶿一-鿿" - "가-힯豈-﫿･-ﾟ" - "\U00020000-\U0003134f\U0002f800-\U0002fa1f") +#: A, the unified block, compatibility, and the supplementary planes from +#: Extension B through Extension H as of Unicode 15, where rare surname +#: characters live), hangul syllables, and halfwidth katakana. The Yijing +#: hexagrams between Extension A and the unified block are left out. One +#: such letter makes a label or a word CJK; the query and is_cjk share +#: the class, so widening it is an INDEX_VERSION bump. +_CJK_CHARS = ("\u3005\u3007\u3040-\u30ff\u3400-\u4dbf\u4e00-\u9fff" + "\uac00-\ud7af\uf900-\ufaff\uff65-\uff9f" + "\U00020000-\U000323af") _CJK = re.compile(f"[{_CJK_CHARS}]") @@ -777,16 +780,22 @@ def _class_ids(cls: str, fetch: bool) -> list[str]: return ids -def _cached_batch(path: Path, span: list[str], +def _batch_digest(qids: list[str]) -> str: + """What a batch file is keyed by: its whole membership, since batches + are named by position and, after a class is listed again, the same + position can hold other items -- at either end or in between.""" + return hashlib.sha256(" ".join(qids).encode()).hexdigest() + + +def _cached_batch(path: Path, digest: str, ids: list[str]) -> tuple[str, list[list[str]]] | None: - """A batch file's date and rows, or None when it was built against - another listing: batches are named by position, so after a class is - listed again the same position can hold other items.""" + """A batch file's date and rows, or None when it was built for other + items.""" data = _read_json(path) if not (isinstance(data, dict) and isinstance(data.get("fetched"), str) and isinstance(data.get("rows"), list)): - raise _unusable(path, "not a batch: needs fetched, span and rows") - if data.get("span") != span: + raise _unusable(path, "not a batch: needs fetched, digest and rows") + if data.get("digest") != digest: return None rows = data["rows"] batch = set(ids) @@ -807,20 +816,23 @@ def _fetch_batch(cls: str, ids: list[str], start: int, end: int, fetch: bool, base = cache_dir() / INDEX_DIR / f"{cls}-{start}-{end}" done, halved = base.with_suffix(".json"), base.with_suffix(".halved") batch = ids[start:end] - span = [batch[0], batch[-1]] + middle = (start + end) // 2 + # The marker is read first: a whole batch's file from an earlier + # listing must not shadow the halves this one was cached as. A marker + # left by an earlier listing only splits the batch, which costs a + # query and loses nothing, since each half is checked by itself. + if halved.exists(): + first = _fetch_batch(cls, ids, start, middle, fetch, out, dates) + second = _fetch_batch(cls, ids, middle, end, fetch, out, dates) + return first or second + digest = _batch_digest(batch) if done.exists(): - cached = _cached_batch(done, span, batch) + cached = _cached_batch(done, digest, batch) if cached is not None: dates.add(cached[0]) out.extend(IndexEntry(q, NAME_CLASSES[cls], label) for q, label in cached[1]) return False - halved.unlink(missing_ok=True) # it was halved for other items - elif halved.exists(): - middle = (start + end) // 2 - first = _fetch_batch(cls, ids, start, middle, fetch, out, dates) - second = _fetch_batch(cls, ids, middle, end, fetch, out, dates) - return first or second if not fetch: raise _Missing(done.name) try: @@ -830,14 +842,14 @@ def _fetch_batch(cls: str, ids: list[str], start: int, end: int, fetch: bool, if end - start <= INDEX_MIN_BATCH: raise ValueError(f"wd:{cls} items {start}-{end} still " f"overran ({exc})") from exc + done.unlink(missing_ok=True) halved.parent.mkdir(parents=True, exist_ok=True) halved.touch() - middle = (start + end) // 2 _fetch_batch(cls, ids, start, middle, fetch, out, dates) _fetch_batch(cls, ids, middle, end, fetch, out, dates) return True today = date.today().isoformat() - _write(done, {"fetched": today, "span": span, "rows": rows}) + _write(done, {"fetched": today, "digest": digest, "rows": rows}) dates.add(today) out.extend(IndexEntry(q, NAME_CLASSES[cls], label) for q, label in rows) return True @@ -920,6 +932,15 @@ def _count_split(split: Split) -> None: _QUERY_ERRORS = (OSError, ValueError, KeyError, http.client.HTTPException) +def _failure(exc: BaseException) -> str: + """The note for a query that failed. An HTTPError holds its response + open, and since 3.14 one collected unclosed warns, so it is closed + here, where the report lets go of it.""" + if isinstance(exc, urllib.error.HTTPError): + exc.close() + return f"query failed: {type(exc).__name__}: {exc}" + + def ends_with_usage(word: str, index: NameIndex, top: int) -> Usage: """Bearers of the indexed name items whose label ends in the word, and of the family/given pairs that end in it together. A failed @@ -949,14 +970,14 @@ def ends_with_usage(word: str, index: NameIndex, top: int) -> Usage: about)) except _QUERY_ERRORS as exc: return Usage(ENDS_WITH_SOURCE, status="failed", lower_bound=True, - note=f"query failed: {type(exc).__name__}: {exc}") + note=_failure(exc)) usage.source = ENDS_WITH_SOURCE usage.splits = index.boundaries(word) for split in usage.splits: try: _count_split(split) except _QUERY_ERRORS as exc: - split.error = f"query failed: {type(exc).__name__}: {exc}" + split.error = _failure(exc) return usage @@ -970,6 +991,8 @@ def _attempt(load: Callable[[], T | None]) -> tuple[T | None, str]: try: return load(), "" except (OSError, ValueError, http.client.HTTPException) as exc: + if isinstance(exc, urllib.error.HTTPError): + exc.close() return None, f"{type(exc).__name__}: {exc}" @@ -1064,6 +1087,11 @@ def main(argv: list[str] | None = None) -> int: print("\n" + ssa_missing_message(), file=sys.stderr) failed = any(u.status == "failed" or any(s.error for s in u.splits) for us in report.values() for u in us) + if args.refresh_index and index_error: + # A Latin word has no ends-with row to carry this, and the refresh + # has already deleted the old index. + print(f"CJK name index: {index_error}", file=sys.stderr) + failed = True return 2 if failed else 0