fix(file): verify the upload before storing the file entity

- assert the upload API actually reported Success, mwclient only raises on an error key
- upload before store_entity so a failure cannot leave a metadata-only entity

Author: LukasGold

src/osw/controller/file/wiki.py

@@ -46,6 +46,24 @@ def reraise_upload_error(
     ) from error
 
 
+def assert_upload_success(result: Any, title: str, host: str) -> None:
+    """Raises unless the upload API reports that it stored the file
+
+    mwclient raises only when the response carries an 'error' key. A file that
+    MediaWiki declined for any other reason comes back as a normal return value
+    with a result other than 'Success', so without this check the upload would
+    fail silently.
+    """
+    status = result.get("result") if isinstance(result, dict) else None
+    if status == "Success":
+        return
+    warnings = result.get("warnings") if isinstance(result, dict) else None
+    detail = f" Warnings: {warnings}." if warnings else ""
+    raise ValueError(
+        f"Upload of '{title}' to {host} did not succeed (result: {status}).{detail}"
+    )
+
+
 class WikiFileController(model.WikiFile, RemoteFileController):
     """File controller for wiki files"""
 
@@ -168,15 +186,12 @@ def put(self, file: IO, **kwargs: Dict[str, Any]):
         }
         for key in ["entities", "namespace"]:
             se_params.pop(key, None)  # avoid duplicated kwargs
-        self.osw.store_entity(
-            OSW.StoreEntityParam(
-                entities=[self.cast(model.WikiFile, **wf_params)],
-                namespace=self.namespace,
-                **se_params,
-            )
-        )
+        # Upload before storing the entity: MediaWiki offers no transaction
+        # across the two writes, so one of them can be left standing. A file
+        # page without metadata is visibly incomplete, while metadata without a
+        # file looks like a valid entity until someone tries to download it.
         try:
-            self.osw.mw_site.upload(
+            result = self.osw.mw_site.upload(
                 file=file,
                 filename=self.title,
                 # comment="",
@@ -185,6 +200,14 @@ def put(self, file: IO, **kwargs: Dict[str, Any]):
             )
         except mwclient.errors.APIError as e:
             reraise_upload_error(e, self.osw.mw_site, self.title, self.suffix)
+        assert_upload_success(result, self.title, self.osw.mw_site.host)
+        self.osw.store_entity(
+            OSW.StoreEntityParam(
+                entities=[self.cast(model.WikiFile, **wf_params)],
+                namespace=self.namespace,
+                **se_params,
+            )
+        )
 
     def put_from(self, other: FileController, **kwargs: Dict[str, Any]):
         # if isinstance(file, LocalFileController) and self.suffix is None:

tests/test_wiki_file_upload_errors.py

@@ -2,12 +2,17 @@
 
 Regression guard for #51: a file rejected because of its extension must produce
 an error that names the extension, instead of a bare MediaWiki API code.
+
+Also guards the silent-failure path: mwclient raises only on an 'error' key, so
+an upload MediaWiki declined by other means must be caught by inspecting the
+returned result.
 """
 
 import mwclient.errors
 import pytest
 
 from osw.controller.file.wiki import (
+    assert_upload_success,
     get_allowed_file_extensions,
     reraise_upload_error,
 )
@@ -85,3 +90,29 @@ def test_get_allowed_file_extensions_reads_siteinfo():
     site = _FakeSite(extensions=["png", "jpg"])
 
     assert get_allowed_file_extensions(site) == ["png", "jpg"]
+
+
+def test_successful_upload_passes():
+    assert (
+        assert_upload_success({"result": "Success"}, "a.png", "wiki.example.org")
+        is None
+    )
+
+
+@pytest.mark.parametrize("result", [{}, None, {"result": "Poll"}])
+def test_upload_without_success_raises(result):
+    """Anything but a Success result means the file did not arrive."""
+    with pytest.raises(ValueError) as exc_info:
+        assert_upload_success(result, "a.png", "wiki.example.org")
+
+    assert "a.png" in str(exc_info.value)
+    assert "wiki.example.org" in str(exc_info.value)
+
+
+def test_warned_upload_reports_the_warnings():
+    result = {"result": "Warning", "warnings": {"badfilename": "a_png"}}
+
+    with pytest.raises(ValueError) as exc_info:
+        assert_upload_success(result, "a.png", "wiki.example.org")
+
+    assert "badfilename" in str(exc_info.value)