From f84a5297c605fc458783e5497edeb1f97113053b Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Fri, 18 Sep 2026 13:39:57 +0200 Subject: [PATCH 1/2] fix(core): verify that store_entity actually created the page - add StoreEntityParam.verify_write, default True - query the edited titles after the upload, one API request per 50 titles - an absent page goes to StoreEntityResult.failed as PageNotCreatedError instead of being reported in StoreEntityResult.pages - the query bypasses the page cache, which would answer pre-write - correct the comment claiming created_pages only records successes Closes #175 --- src/osw/core.py | 71 ++++++++++++- tests/test_overwrite_policy.py | 4 + tests/test_store_entity_failure.py | 4 + tests/test_store_entity_verify.py | 157 +++++++++++++++++++++++++++++ 4 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 tests/test_store_entity_verify.py diff --git a/src/osw/core.py b/src/osw/core.py index b85a4b7a..814f2a48 100644 --- a/src/osw/core.py +++ b/src/osw/core.py @@ -1639,6 +1639,12 @@ class StoreEntityParam(OswBaseModel): offline: Optional[bool] = False """If set to True, the processed entities are not upload but only returned as WtPages. Can be used to create WtPage objects from entities without uploading them.""" + verify_write: Optional[bool] = True + """If set to True, the existence of every edited page is queried after the + upload. A page that does not exist afterwards is reported in + StoreEntityResult.failed instead of StoreEntityResult.pages. This costs one + additional API request per 50 edited pages. Has no effect if 'offline' is + True.""" _overwrite_per_class: Dict[str, Dict[str, OSW.OverwriteClassParam]] = ( PrivateAttr() ) @@ -1717,6 +1723,50 @@ def __init__(self, result: OSW.StoreEntityResult): f"entities: {failed_titles}" ) + class PageNotCreatedError(Exception): + """Raised for a page that does not exist after store_entity() edited it. + + The edit was sent and no exception was raised, but the page is absent when + the wiki is asked afterwards. A form or template driven creation step on + the category can reject the content server-side without reporting an error + to the API client. + """ + + def __init__(self, title: str): + self.title = title + super().__init__( + f"Page '{title}' does not exist after the edit. The write was " + f"rejected by the wiki without an error response." + ) + + def _get_missing_page_titles(self, titles: List[str]) -> List[str]: + """Returns those of the given page titles that do not exist on the wiki. + + The query goes to the MediaWiki API directly and not through + WtSite.get_page(), because the page cache would answer with the state from + before the write. + + Parameters + ---------- + titles: + Full page titles to check. + """ + missing = [] + batch_size = 50 # maximum number of titles per API query + for start in range(0, len(titles), batch_size): + batch = titles[start : start + batch_size] + result = self.mw_site.api( + "query", titles="|".join(batch), prop="info", format="json" + ) + query = result.get("query", {}) + # the API normalizes titles, map them back to what was requested + normalized = {n["to"]: n["from"] for n in query.get("normalized", [])} + for page_info in query.get("pages", {}).values(): + if "missing" in page_info: + title = page_info.get("title") + missing.append(normalized.get(title, title)) + return missing + def store_entity( self, param: Union[StoreEntityParam, OswBaseModel, List[OswBaseModel]] ) -> StoreEntityResult: @@ -1739,6 +1789,8 @@ def store_entity( max_index = len(param.entities) created_pages = {} + edited_titles = set() + """Titles of the pages an edit was sent for, to be verified below.""" meta_category_templates = {} if param.namespace == "Category": @@ -1863,6 +1915,7 @@ def store_entity_( page.edit( param.edit_comment, bot_edit=param.bot_edit ) # will set page.changed if the content of the page has changed + edited_titles.add(page.title) if not param.offline and page.changed: if index is None: print(f"Entity stored at '{page.get_url()}'.") @@ -1922,7 +1975,10 @@ class UploadObject(BaseModel): def handle_upload_object_(upload_object: UploadObject) -> None: # Let exceptions propagate: the caller collects them per entity below, # so a single failure neither aborts the batch nor is silently - # swallowed (store_entity_ only records created_pages on success). + # swallowed. store_entity_ records a page in created_pages whenever it + # reaches its last statement, which only means that nothing raised. + # Whether the page exists afterwards is checked by the verification + # step below. store_entity_( upload_object.entity, upload_object.namespace, @@ -1969,6 +2025,19 @@ def failure_title_(upload_object: UploadObject) -> str: _logger.error(f"Error storing entity '{title}': {result}") failed[title] = result + if param.verify_write and not param.offline and edited_titles: + # An edit that raised no exception is not proof that the page exists: + # a form or template driven creation step can reject the content + # server-side. page.changed is no help either, it is True in that case. + titles_to_verify = [ + title for title in edited_titles if title in created_pages + ] + for title in self._get_missing_page_titles(titles_to_verify): + error = OSW.PageNotCreatedError(title) + _logger.error(f"Error storing entity '{title}': {error}") + failed[title] = error + del created_pages[title] + store_result = OSW.StoreEntityResult( change_id=param.change_id, pages=created_pages, failed=failed ) diff --git a/tests/test_overwrite_policy.py b/tests/test_overwrite_policy.py index d851dc5d..3508888e 100644 --- a/tests/test_overwrite_policy.py +++ b/tests/test_overwrite_policy.py @@ -355,6 +355,10 @@ def _stub_page_io(monkeypatch, exists: bool): ) edited = [] monkeypatch.setattr(WtPage, "edit", lambda self, *a, **k: edited.append(self.title)) + # the write verification of store_entity() would query the wiki. These tests + # cover the overwrite policy, so report every edited page as existing. The + # verification itself is covered by test_store_entity_verify.py + monkeypatch.setattr(OSW, "_get_missing_page_titles", lambda self, titles: []) return edited diff --git a/tests/test_store_entity_failure.py b/tests/test_store_entity_failure.py index 507d6396..6fb03775 100644 --- a/tests/test_store_entity_failure.py +++ b/tests/test_store_entity_failure.py @@ -26,6 +26,10 @@ def offline_osw(monkeypatch): monkeypatch.setattr( OSW, "_apply_overwrite_policy", staticmethod(lambda param: param.page) ) + # the write verification would query the wiki. These tests cover the reporting + # of a failed edit, so report every edited page as existing. The verification + # itself is covered by test_store_entity_verify.py + monkeypatch.setattr(OSW, "_get_missing_page_titles", lambda self, titles: []) return OSW.construct(site=object()) diff --git a/tests/test_store_entity_verify.py b/tests/test_store_entity_verify.py new file mode 100644 index 00000000..75ff5676 --- /dev/null +++ b/tests/test_store_entity_verify.py @@ -0,0 +1,157 @@ +"""Unit tests for the write verification of store_entity(). + +Regression guard for #175: store_entity() returned normally and listed a page in +created_pages when that page did not exist on the wiki afterwards. An edit that +raises nothing is not proof that the page was created, and page.changed is True +in the failing case as well. store_entity() must now query the edited pages and +report an absent one in result.failed. + +These run fully offline: WtPage.init, the overwrite policy and the existence +query are stubbed, so no network is required. +""" + +import pytest + +import osw.model.entity as model +from osw.core import OSW +from osw.utils.wiki import get_namespace, get_title +from osw.wtsite import WtPage + + +def _title(entity): + return f"{get_namespace(entity)}:{get_title(entity)}" + + +class _FakeMwSite: + """Records the API queries and answers them from a set of missing titles.""" + + def __init__(self, missing_titles=(), normalized=None): + self.missing_titles = set(missing_titles) + self.normalized = normalized or {} + self.queries = [] + + def api(self, action, **kwargs): + assert action == "query" + titles = kwargs["titles"].split("|") + self.queries.append(titles) + pages = {} + for i, title in enumerate(titles): + reported = self.normalized.get(title, title) + if title in self.missing_titles: + pages[str(-(i + 1))] = {"title": reported, "missing": ""} + else: + pages[str(i + 1)] = {"title": reported, "pageid": i + 1} + query = {"pages": pages} + if self.normalized: + query["normalized"] = [ + {"from": k, "to": v} for k, v in self.normalized.items() + ] + return {"query": query} + + +class _FakeSite: + def __init__(self, mw_site): + self.mw_site = mw_site + + +@pytest.fixture +def offline_osw(monkeypatch): + """An OSW that never touches the network, with a controllable existence query.""" + monkeypatch.setattr(WtPage, "init", lambda self: setattr(self, "exists", False)) + monkeypatch.setattr( + OSW, "_apply_overwrite_policy", staticmethod(lambda param: param.page) + ) + monkeypatch.setattr(WtPage, "edit", lambda self, *a, **kw: None) + + def _make(missing_titles=(), normalized=None): + mw_site = _FakeMwSite(missing_titles, normalized) + return OSW.construct(site=_FakeSite(mw_site)), mw_site + + return _make + + +def test_absent_page_is_reported_as_failed(offline_osw): + item = model.Item(label=[model.Label(text="Ghost")]) + title = _title(item) + osw_obj, _mw_site = offline_osw(missing_titles=[title]) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + osw_obj.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False)) + + err = exc_info.value + assert title in err.failed + assert isinstance(err.failed[title], OSW.PageNotCreatedError) + assert err.failed[title].title == title + assert title not in err.result.pages + assert err.stored == [] + + +def test_present_page_is_reported_as_stored(offline_osw): + item = model.Item(label=[model.Label(text="Real")]) + title = _title(item) + osw_obj, mw_site = offline_osw() + + result = osw_obj.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False)) + + assert set(result.pages.keys()) == {title} + assert result.failed == {} + assert mw_site.queries == [[title]] + + +def test_only_the_absent_page_of_a_batch_is_reported(offline_osw): + items = [model.Item(label=[model.Label(text=f"Batch{i}")]) for i in range(3)] + titles = [_title(it) for it in items] + osw_obj, _mw_site = offline_osw(missing_titles=[titles[1]]) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + osw_obj.store_entity(OSW.StoreEntityParam(entities=items, parallel=True)) + + err = exc_info.value + assert set(err.result.pages.keys()) == {titles[0], titles[2]} + assert set(err.failed.keys()) == {titles[1]} + + +def test_verification_is_skipped_when_disabled(offline_osw): + item = model.Item(label=[model.Label(text="Unchecked")]) + title = _title(item) + osw_obj, mw_site = offline_osw(missing_titles=[title]) + + result = osw_obj.store_entity( + OSW.StoreEntityParam(entities=[item], parallel=False, verify_write=False) + ) + + assert mw_site.queries == [] + assert set(result.pages.keys()) == {title} + + +def test_verification_is_skipped_offline(offline_osw): + item = model.Item(label=[model.Label(text="Offline")]) + title = _title(item) + osw_obj, mw_site = offline_osw(missing_titles=[title]) + + result = osw_obj.store_entity( + OSW.StoreEntityParam(entities=[item], parallel=False, offline=True) + ) + + assert mw_site.queries == [] + assert set(result.pages.keys()) == {title} + + +def test_titles_are_queried_in_batches_of_fifty(offline_osw): + osw_obj, mw_site = offline_osw() + titles = [f"Item:OSW{i:04d}" for i in range(120)] + + missing = osw_obj._get_missing_page_titles(titles) + + assert missing == [] + assert [len(batch) for batch in mw_site.queries] == [50, 50, 20] + + +def test_missing_titles_are_mapped_back_to_the_requested_form(offline_osw): + requested = "Item:OSW_with_underscores" + osw_obj, _mw_site = offline_osw( + missing_titles=[requested], + normalized={requested: "Item:OSW with underscores"}, + ) + + assert osw_obj._get_missing_page_titles([requested]) == [requested] From 5756436e08ba95be3e177c6a78c424bfdba756c8 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Fri, 18 Sep 2026 14:01:45 +0200 Subject: [PATCH 2/2] fix(core): confirm a missing page before reporting a failed store - query a title reported as missing a second time, 5 s later, so a read from a lagging database replica cannot fail a store that applied - a failing verification query no longer discards the results of the whole store_entity call: the pages stay reported and the error is logged - add tests for both paths --- src/osw/core.py | 45 +++++++++++++++++++++-- tests/test_store_entity_verify.py | 61 ++++++++++++++++++++++++++++--- 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/osw/core.py b/src/osw/core.py index 814f2a48..75c67e75 100644 --- a/src/osw/core.py +++ b/src/osw/core.py @@ -9,6 +9,7 @@ import warnings from copy import deepcopy from enum import Enum +from time import sleep from typing import Any, Dict, List, Optional, Type, Union, overload from uuid import UUID, uuid4 from warnings import warn @@ -1643,8 +1644,10 @@ class StoreEntityParam(OswBaseModel): """If set to True, the existence of every edited page is queried after the upload. A page that does not exist afterwards is reported in StoreEntityResult.failed instead of StoreEntityResult.pages. This costs one - additional API request per 50 edited pages. Has no effect if 'offline' is - True.""" + additional API request per 50 edited pages, and one further request some + seconds later if a page is reported as missing. If the query itself fails, + the pages are reported as stored and an error is logged. Has no effect if + 'offline' is True.""" _overwrite_per_class: Dict[str, Dict[str, OSW.OverwriteClassParam]] = ( PrivateAttr() ) @@ -1739,9 +1742,32 @@ def __init__(self, title: str): f"rejected by the wiki without an error response." ) - def _get_missing_page_titles(self, titles: List[str]) -> List[str]: + def _get_missing_page_titles( + self, titles: List[str], confirm_delay_s: int = 5 + ) -> List[str]: """Returns those of the given page titles that do not exist on the wiki. + A title the wiki reports as missing is queried a second time after + confirm_delay_s seconds. A read can be answered by a database replica that + does not have the write yet, and a title that is still absent seconds later + is not explained by that lag. + + Parameters + ---------- + titles: + Full page titles to check. + confirm_delay_s: + Seconds to wait before the second query. Set to 0 to query only once. + """ + missing = self._query_missing_page_titles(titles) + if missing and confirm_delay_s: + sleep(confirm_delay_s) + missing = self._query_missing_page_titles(missing) + return missing + + def _query_missing_page_titles(self, titles: List[str]) -> List[str]: + """Asks the wiki once which of the given page titles do not exist. + The query goes to the MediaWiki API directly and not through WtSite.get_page(), because the page cache would answer with the state from before the write. @@ -2032,7 +2058,18 @@ def failure_title_(upload_object: UploadObject) -> str: titles_to_verify = [ title for title in edited_titles if title in created_pages ] - for title in self._get_missing_page_titles(titles_to_verify): + try: + missing_titles = self._get_missing_page_titles(titles_to_verify) + except Exception as e: + # A failed query is no evidence that the writes failed. Report it + # and keep the pages, instead of discarding everything this call + # has collected so far. + missing_titles = [] + _logger.error( + f"Could not verify {len(titles_to_verify)} stored pages, they " + f"are reported as stored without being checked: {e}" + ) + for title in missing_titles: error = OSW.PageNotCreatedError(title) _logger.error(f"Error storing entity '{title}': {error}") failed[title] = error diff --git a/tests/test_store_entity_verify.py b/tests/test_store_entity_verify.py index 75ff5676..7ed5737c 100644 --- a/tests/test_store_entity_verify.py +++ b/tests/test_store_entity_verify.py @@ -12,6 +12,7 @@ import pytest +import osw.core as core_mod import osw.model.entity as model from osw.core import OSW from osw.utils.wiki import get_namespace, get_title @@ -23,21 +24,33 @@ def _title(entity): class _FakeMwSite: - """Records the API queries and answers them from a set of missing titles.""" + """Records the API queries and answers them from a set of missing titles. - def __init__(self, missing_titles=(), normalized=None): + missing_once holds titles that are reported as missing by the first query + only, which is what a read from a lagging database replica looks like. + """ + + def __init__( + self, missing_titles=(), normalized=None, missing_once=(), fails=False + ): self.missing_titles = set(missing_titles) + self.missing_once = set(missing_once) self.normalized = normalized or {} + self.fails = fails self.queries = [] def api(self, action, **kwargs): assert action == "query" + if self.fails: + raise RuntimeError("the API is not reachable") titles = kwargs["titles"].split("|") self.queries.append(titles) + missing_now = self.missing_titles | self.missing_once + self.missing_once = set() pages = {} for i, title in enumerate(titles): reported = self.normalized.get(title, title) - if title in self.missing_titles: + if title in missing_now: pages[str(-(i + 1))] = {"title": reported, "missing": ""} else: pages[str(i + 1)] = {"title": reported, "pageid": i + 1} @@ -62,9 +75,11 @@ def offline_osw(monkeypatch): OSW, "_apply_overwrite_policy", staticmethod(lambda param: param.page) ) monkeypatch.setattr(WtPage, "edit", lambda self, *a, **kw: None) + # the delay before the confirmation query, not worth waiting for in a test + monkeypatch.setattr(core_mod, "sleep", lambda *a, **kw: None) - def _make(missing_titles=(), normalized=None): - mw_site = _FakeMwSite(missing_titles, normalized) + def _make(missing_titles=(), normalized=None, missing_once=(), fails=False): + mw_site = _FakeMwSite(missing_titles, normalized, missing_once, fails) return OSW.construct(site=_FakeSite(mw_site)), mw_site return _make @@ -137,6 +152,42 @@ def test_verification_is_skipped_offline(offline_osw): assert set(result.pages.keys()) == {title} +def test_a_missing_page_is_confirmed_by_a_second_query(offline_osw): + item = model.Item(label=[model.Label(text="Ghost")]) + title = _title(item) + osw_obj, mw_site = offline_osw(missing_titles=[title]) + + with pytest.raises(OSW.StoreEntityPartialError): + osw_obj.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False)) + + assert mw_site.queries == [[title], [title]] + + +def test_a_page_that_appears_on_the_second_query_is_not_reported(offline_osw): + """A read answered by a lagging database replica must not fail the store.""" + items = [model.Item(label=[model.Label(text=f"Lag{i}")]) for i in range(2)] + titles = [_title(it) for it in items] + osw_obj, mw_site = offline_osw(missing_once=[titles[0]]) + + result = osw_obj.store_entity(OSW.StoreEntityParam(entities=items, parallel=False)) + + assert set(result.pages.keys()) == set(titles) + assert result.failed == {} + # the second query asks only for the title the first one reported as missing + assert mw_site.queries[1] == [titles[0]] + + +def test_a_failing_query_keeps_the_pages_and_does_not_raise(offline_osw): + items = [model.Item(label=[model.Label(text=f"Unverified{i}")]) for i in range(2)] + titles = [_title(it) for it in items] + osw_obj, _mw_site = offline_osw(fails=True) + + result = osw_obj.store_entity(OSW.StoreEntityParam(entities=items, parallel=False)) + + assert set(result.pages.keys()) == set(titles) + assert result.failed == {} + + def test_titles_are_queried_in_batches_of_fifty(offline_osw): osw_obj, mw_site = offline_osw() titles = [f"Item:OSW{i:04d}" for i in range(120)]