fix(core): verify that store_entity actually created the page

Merge pull request #182 from OpenSemanticLab/fix/175-store-entity-verify

Author: LukasGold

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
         )

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
 
 

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())
 
 

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]