diff --git a/src/osw/core.py b/src/osw/core.py index b85a4b7..75c67e7 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 @@ -1639,6 +1640,14 @@ 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, 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() ) @@ -1717,6 +1726,73 @@ 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], 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. + + 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 +1815,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 +1941,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 +2001,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 +2051,30 @@ 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 + ] + 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 + 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 d851dc5..3508888 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 507d639..6fb0377 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 0000000..7ed5737 --- /dev/null +++ b/tests/test_store_entity_verify.py @@ -0,0 +1,208 @@ +"""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.core as core_mod +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. + + 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 missing_now: + 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) + # 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, missing_once=(), fails=False): + mw_site = _FakeMwSite(missing_titles, normalized, missing_once, fails) + 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_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)] + + 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]