From bb49392a4fc81801d3662613faae96944dd8ed91 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Mon, 31 Aug 2026 17:31:04 +0200 Subject: [PATCH 1/6] feat(file): report a rejected file extension clearly on upload - translate MediaWiki filetype-banned errors into a readable ValueError - name the offending extension and list the extensions the wiki accepts - re-raise every other APIError unchanged - closes #51 --- src/osw/controller/file/wiki.py | 54 ++++++++++++++--- tests/test_wiki_file_upload_errors.py | 87 +++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 tests/test_wiki_file_upload_errors.py diff --git a/src/osw/controller/file/wiki.py b/src/osw/controller/file/wiki.py index 8a032291..414a58bd 100644 --- a/src/osw/controller/file/wiki.py +++ b/src/osw/controller/file/wiki.py @@ -2,6 +2,8 @@ import os from typing import IO, Any, Dict, List, Optional +import mwclient.errors + from osw.controller.file.base import FileController from osw.controller.file.remote import RemoteFileController from osw.core import OSW, model @@ -9,6 +11,41 @@ from osw.wtsite import WtSite +def get_allowed_file_extensions(mw_site) -> Optional[List[str]]: + """Queries the file extensions the wiki accepts, None if unavailable""" + try: + result = mw_site.api( + "query", meta="siteinfo", siprop="fileextensions", formatversion=2 + ) + except Exception: + # only used to enrich an error message, never worth failing over + return None + extensions = result.get("query", {}).get("fileextensions", []) + return [entry["ext"] for entry in extensions if "ext" in entry] + + +def reraise_upload_error( + error: mwclient.errors.APIError, mw_site, title: str, suffix: Optional[str] +) -> None: + """Turns a rejected file extension into a readable error, re-raises the rest + + MediaWiki reports a rejected extension as filetype-banned, + filetype-banned-type or filetype-badtype, depending on the version. + """ + if "filetype" not in str(getattr(error, "code", "")): + raise error + allowed = get_allowed_file_extensions(mw_site) + hint = ( + f" Extensions allowed on this wiki: {', '.join(sorted(allowed))}." + if allowed + else "" + ) + raise ValueError( + f"Upload of '{title}' was rejected because the file extension " + f"'{suffix}' is not allowed on {mw_site.host}.{hint}" + ) from error + + class WikiFileController(model.WikiFile, RemoteFileController): """File controller for wiki files""" @@ -138,13 +175,16 @@ def put(self, file: IO, **kwargs: Dict[str, Any]): **se_params, ) ) - self.osw.mw_site.upload( - file=file, - filename=self.title, - # comment="", - # description="", - ignore=True, - ) + try: + self.osw.mw_site.upload( + file=file, + filename=self.title, + # comment="", + # description="", + ignore=True, + ) + except mwclient.errors.APIError as e: + reraise_upload_error(e, self.osw.mw_site, self.title, self.suffix) def put_from(self, other: FileController, **kwargs: Dict[str, Any]): # if isinstance(file, LocalFileController) and self.suffix is None: diff --git a/tests/test_wiki_file_upload_errors.py b/tests/test_wiki_file_upload_errors.py new file mode 100644 index 00000000..b55ea617 --- /dev/null +++ b/tests/test_wiki_file_upload_errors.py @@ -0,0 +1,87 @@ +"""Unit tests for upload error reporting in osw.controller.file.wiki. + +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. +""" + +import mwclient.errors +import pytest + +from osw.controller.file.wiki import ( + get_allowed_file_extensions, + reraise_upload_error, +) + + +class _FakeSite: + host = "wiki.example.org" + + def __init__(self, extensions=None, fail=False): + self._extensions = extensions + self._fail = fail + + def api(self, *args, **kwargs): + if self._fail: + raise RuntimeError("siteinfo unavailable") + return {"query": {"fileextensions": [{"ext": e} for e in self._extensions]}} + + +def _api_error(code): + return mwclient.errors.APIError(code, "info", {}) + + +@pytest.mark.parametrize( + "code", ["filetype-banned", "filetype-banned-type", "filetype-badtype"] +) +def test_rejected_extension_names_the_extension(code): + site = _FakeSite(extensions=["png", "pdf"]) + + with pytest.raises(ValueError) as exc_info: + reraise_upload_error(_api_error(code), site, "OSW123.exe", ".exe") + + message = str(exc_info.value) + assert ".exe" in message + assert "OSW123.exe" in message + assert "wiki.example.org" in message + assert "pdf, png" in message # allowed extensions, sorted + + +def test_original_error_is_chained(): + site = _FakeSite(extensions=["png"]) + original = _api_error("filetype-banned") + + with pytest.raises(ValueError) as exc_info: + reraise_upload_error(original, site, "OSW123.exe", ".exe") + + assert exc_info.value.__cause__ is original + + +def test_unrelated_api_error_is_reraised_unchanged(): + site = _FakeSite(extensions=["png"]) + original = _api_error("readapidenied") + + with pytest.raises(mwclient.errors.APIError) as exc_info: + reraise_upload_error(original, site, "OSW123.png", ".png") + + assert exc_info.value is original + + +def test_message_omits_the_hint_when_siteinfo_fails(): + """A failing siteinfo lookup must not mask the upload error.""" + site = _FakeSite(fail=True) + + with pytest.raises(ValueError) as exc_info: + reraise_upload_error(_api_error("filetype-banned"), site, "a.exe", ".exe") + + assert "allowed on this wiki" not in str(exc_info.value) + assert ".exe" in str(exc_info.value) + + +def test_get_allowed_file_extensions_returns_none_on_failure(): + assert get_allowed_file_extensions(_FakeSite(fail=True)) is None + + +def test_get_allowed_file_extensions_reads_siteinfo(): + site = _FakeSite(extensions=["png", "jpg"]) + + assert get_allowed_file_extensions(site) == ["png", "jpg"] From 4a09bf541a3ea404dcd2e1cd41ea36b8c53ce141 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Tue, 1 Sep 2026 14:11:47 +0200 Subject: [PATCH 2/6] 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 --- src/osw/controller/file/wiki.py | 39 +++++++++++++++++++++------ tests/test_wiki_file_upload_errors.py | 31 +++++++++++++++++++++ 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/osw/controller/file/wiki.py b/src/osw/controller/file/wiki.py index 414a58bd..7cfb990b 100644 --- a/src/osw/controller/file/wiki.py +++ b/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: diff --git a/tests/test_wiki_file_upload_errors.py b/tests/test_wiki_file_upload_errors.py index b55ea617..aad1c5ef 100644 --- a/tests/test_wiki_file_upload_errors.py +++ b/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) From eb9e0e112e8b75128c8a3fe5c8d5943531c16d6a Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Tue, 1 Sep 2026 14:21:19 +0200 Subject: [PATCH 3/6] fix(file): write the metadata onto the page the upload created - store_entity saw the page the upload had just made and kept its empty content - pass overwrite='replace remote' when the file page did not exist beforehand - an already existing page keeps whatever policy the caller asked for --- src/osw/controller/file/wiki.py | 19 ++++++++++++++++++- tests/test_wiki_file_upload_errors.py | 22 ++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/osw/controller/file/wiki.py b/src/osw/controller/file/wiki.py index 7cfb990b..de6b2a79 100644 --- a/src/osw/controller/file/wiki.py +++ b/src/osw/controller/file/wiki.py @@ -64,6 +64,22 @@ def assert_upload_success(result: Any, title: str, host: str) -> None: ) +def store_params_for_upload( + se_params: Dict[str, Any], page_existed: bool +) -> Dict[str, Any]: + """Picks the overwrite policy for the entity that accompanies an upload + + An upload creates the file page when it was not there yet. store_entity + would then see an existing page and, under the default 'keep existing', + leave the metadata unwritten. So the entity has to replace what the upload + put there. A page that was already there keeps whatever the caller asked + for. + """ + if page_existed: + return se_params + return {**se_params, "overwrite": "replace remote"} + + class WikiFileController(model.WikiFile, RemoteFileController): """File controller for wiki files""" @@ -190,6 +206,7 @@ def put(self, file: IO, **kwargs: Dict[str, Any]): # 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. + page_existed = self.osw.mw_site.pages[f"{self.namespace}:{self.title}"].exists try: result = self.osw.mw_site.upload( file=file, @@ -205,7 +222,7 @@ def put(self, file: IO, **kwargs: Dict[str, Any]): OSW.StoreEntityParam( entities=[self.cast(model.WikiFile, **wf_params)], namespace=self.namespace, - **se_params, + **store_params_for_upload(se_params, page_existed), ) ) diff --git a/tests/test_wiki_file_upload_errors.py b/tests/test_wiki_file_upload_errors.py index aad1c5ef..211cf1b8 100644 --- a/tests/test_wiki_file_upload_errors.py +++ b/tests/test_wiki_file_upload_errors.py @@ -15,6 +15,7 @@ assert_upload_success, get_allowed_file_extensions, reraise_upload_error, + store_params_for_upload, ) @@ -109,6 +110,27 @@ def test_upload_without_success_raises(result): assert "wiki.example.org" in str(exc_info.value) +def test_a_page_the_upload_created_gets_its_metadata_written(): + """Otherwise store_entity keeps the empty page the upload just left behind.""" + assert store_params_for_upload({}, page_existed=False) == { + "overwrite": "replace remote" + } + + +def test_an_existing_page_keeps_the_callers_overwrite_policy(): + se_params = {"overwrite": "keep existing", "edit_comment": "hi"} + + assert store_params_for_upload(se_params, page_existed=True) == se_params + + +def test_store_params_are_not_mutated(): + se_params = {"edit_comment": "hi"} + + store_params_for_upload(se_params, page_existed=False) + + assert se_params == {"edit_comment": "hi"} + + def test_warned_upload_reports_the_warnings(): result = {"result": "Warning", "warnings": {"badfilename": "a_png"}} From 79a645b3cb5a67e9b769c76270be7297220cde98 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Tue, 1 Sep 2026 17:05:07 +0200 Subject: [PATCH 4/6] fix: read the uuid from a file page title with suffixes - merge the two get_uuid copies into osw.utils.wiki.get_uuid - OSW.get_uuid delegates to it instead of holding a second copy - ignore the OSW prefix and any number of file suffixes via regex - reject a string that is not an OSW-ID instead of parsing it partly --- src/osw/core.py | 15 ++++++++----- src/osw/utils/wiki.py | 31 ++++++++++++++++++++++---- tests/utils/utils_test.py | 46 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 9 deletions(-) diff --git a/src/osw/core.py b/src/osw/core.py index fd37d314..6f485f24 100644 --- a/src/osw/core.py +++ b/src/osw/core.py @@ -59,12 +59,14 @@ get_full_title, get_namespace, get_title, - get_uuid, is_empty, namespace_from_full_title, remove_empty, title_from_full_title, ) +from osw.utils.wiki import ( + get_uuid as get_uuid_from_osw_id, +) from osw.wiki_tools import SearchParam from osw.wtsite import WtPage, WtSite @@ -197,18 +199,21 @@ def get_osw_id(uuid: Union[str, UUID]) -> str: @staticmethod def get_uuid(osw_id: str) -> UUID: - """Returns the uuid for a given OSW-ID + """Returns the uuid for a given OSW-ID. Kept for backwards compatibility, + the implementation lives in osw.utils.wiki.get_uuid() Parameters ---------- osw_id - OSW-ID string, e.g. OSW2ea5b605c91f4e5a95593dff79fdd4a5 + OSW-ID string, e.g. OSW2ea5b605c91f4e5a95593dff79fdd4a5, with or + without file suffixes, e.g. + OSW2ea5b605c91f4e5a95593dff79fdd4a5.drawio.png Returns ------- uuid object, e.g. UUID("2ea5b605-c91f-4e5a-9559-3dff79fdd4a5") """ - return UUID(osw_id.replace("OSW", "")) + return get_uuid_from_osw_id(osw_id) class SortEntitiesResult(OswBaseModel): by_name: Dict[str, List[OswBaseModel]] @@ -1348,7 +1353,7 @@ def validate_entity(cls, entity, values): if jsondata is None: # Guard clause title = title_from_full_title(page.title) try: - uuid_from_title = get_uuid(title) + uuid_from_title = get_uuid_from_osw_id(title) except ValueError: print( f"Error: UUID could not be determined from title: '{title}', " diff --git a/src/osw/utils/wiki.py b/src/osw/utils/wiki.py index 054c3f77..9140f920 100644 --- a/src/osw/utils/wiki.py +++ b/src/osw/utils/wiki.py @@ -1,9 +1,18 @@ +import re from copy import deepcopy from uuid import UUID # Legacy imports: from opensemantic.v1 import get_full_title, get_namespace, get_title # noqa: F401 +OSW_ID_PATTERN = re.compile( + r"^(?:OSW)?" # the prefix, absent when a bare uuid is passed + r"([0-9a-f]{8}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{12})" + r"(?:\.[\w-]+)*$", # file suffixes, e.g. '.png' or '.drawio.png' + re.IGNORECASE, +) +"""Matches an OSW-ID with an optional prefix and any number of file suffixes""" + def get_osw_id(uuid: UUID) -> str: """Generates a OSW-ID based on the given uuid by prefixing "OSW" and removing @@ -21,19 +30,33 @@ def get_osw_id(uuid: UUID) -> str: return "OSW" + str(uuid).replace("-", "") -def get_uuid(osw_id) -> UUID: - """Returns the uuid for a given OSW-ID. Duplicate of OSW.get_uuid() from src/sw/core/osw.py +def get_uuid(osw_id: str) -> UUID: + """Returns the uuid for a given OSW-ID. The single implementation, wrapped by + OSW.get_uuid() from src/osw/core.py + + A file page keeps its file extension in the title, so the OSW-ID of a file is + followed by one or more suffixes that are not part of the uuid. These are + ignored, as is the OSW prefix. Parameters ---------- osw_id - OSW-ID string, e.g., OSW2ea5b605c91f4e5a95593dff79fdd4a5 + OSW-ID string, e.g., OSW2ea5b605c91f4e5a95593dff79fdd4a5, with or without + file suffixes, e.g., OSW2ea5b605c91f4e5a95593dff79fdd4a5.drawio.png Returns ------- uuid object, e.g., UUID("2ea5b605-c91f-4e5a-9559-3dff79fdd4a5") + + Raises + ------ + ValueError + If no OSW-ID can be read from the given string. """ - return UUID(osw_id.replace("OSW", "")) + match = OSW_ID_PATTERN.match(osw_id) + if match is None: + raise ValueError(f"No OSW-ID could be read from '{osw_id}'") + return UUID(match.group(1)) def namespace_from_full_title(full_title: str) -> str: diff --git a/tests/utils/utils_test.py b/tests/utils/utils_test.py index c9519c4c..8241b021 100644 --- a/tests/utils/utils_test.py +++ b/tests/utils/utils_test.py @@ -1,6 +1,9 @@ import uuid +import pytest + import osw.model.entity as model +from osw.core import OSW from osw.utils.regex import count_match_groups from osw.utils.strings import camel_case, pascal_case from osw.utils.wiki import ( @@ -28,6 +31,49 @@ def test_get_uuid(): assert get_uuid(osw_id) == uuid_ +@pytest.mark.parametrize( + "suffix", ["", ".png", ".drawio.png", ".tar.gz", ".jpg-2", ".a.b.c.d"] +) +def test_get_uuid_ignores_file_suffixes(suffix): + """File pages carry their extension in the title, ahead of the OSW-ID.""" + uuid_ = uuid.uuid4() + osw_id = f"OSW{str(uuid_).replace('-', '')}{suffix}" + + assert get_uuid(osw_id) == uuid_ + + +@pytest.mark.parametrize("prefix", ["", "OSW", "osw"]) +def test_get_uuid_accepts_a_bare_uuid(prefix): + uuid_ = uuid.uuid4() + + assert get_uuid(f"{prefix}{str(uuid_).replace('-', '')}") == uuid_ + assert get_uuid(f"{prefix}{uuid_}") == uuid_ # dashed + + +@pytest.mark.parametrize( + "osw_id", + [ + "", + "OSW", + "Category:OSW2ea5b605c91f4e5a95593dff79fdd4a5", # full title, not an ID + "OSW2ea5b605c91f4e5a95593dff79fdd4a", # one character short + "OSW2ea5b605c91f4e5a95593dff79fdd4a5x", + "OSW2ea5b605c91f4e5a95593dff79fdd4a5.", # empty suffix + "OSWzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + ], +) +def test_get_uuid_rejects_what_is_not_an_osw_id(osw_id): + with pytest.raises(ValueError): + get_uuid(osw_id) + + +def test_osw_get_uuid_delegates(): + """OSW.get_uuid is a wrapper, so it must handle suffixes just the same.""" + osw_id = "OSW2ea5b605c91f4e5a95593dff79fdd4a5.drawio.png" + + assert OSW.get_uuid(osw_id) == get_uuid(osw_id) + + def test_get_entity_namespace(): class DummyClass(model.Entity): pass From 186e9e32042efdc8c8aabe374923d9cd99fa1bd7 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Wed, 2 Sep 2026 08:51:02 +0200 Subject: [PATCH 5/6] refactor: match the OSW-ID as the 32 hex characters it is - drop the dashed-uuid alternation, an OSW-ID never carries dashes - use [0-9a-fA-F]{32} as regex_pattern.py already does - drop IGNORECASE, the prefix was never case insensitive before --- src/osw/utils/wiki.py | 5 ++--- tests/utils/utils_test.py | 9 +++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/osw/utils/wiki.py b/src/osw/utils/wiki.py index 9140f920..ef7fe0dd 100644 --- a/src/osw/utils/wiki.py +++ b/src/osw/utils/wiki.py @@ -7,9 +7,8 @@ OSW_ID_PATTERN = re.compile( r"^(?:OSW)?" # the prefix, absent when a bare uuid is passed - r"([0-9a-f]{8}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{12})" - r"(?:\.[\w-]+)*$", # file suffixes, e.g. '.png' or '.drawio.png' - re.IGNORECASE, + r"([0-9a-fA-F]{32})" # the uuid, in the hex form an OSW-ID carries it + r"(?:\.[\w-]+)*$" # file suffixes, e.g. '.png' or '.drawio.png' ) """Matches an OSW-ID with an optional prefix and any number of file suffixes""" diff --git a/tests/utils/utils_test.py b/tests/utils/utils_test.py index 8241b021..fab0b499 100644 --- a/tests/utils/utils_test.py +++ b/tests/utils/utils_test.py @@ -42,12 +42,11 @@ def test_get_uuid_ignores_file_suffixes(suffix): assert get_uuid(osw_id) == uuid_ -@pytest.mark.parametrize("prefix", ["", "OSW", "osw"]) -def test_get_uuid_accepts_a_bare_uuid(prefix): +def test_get_uuid_accepts_a_bare_uuid(): + """The prefix is optional, as it was when it was stripped by str.replace.""" uuid_ = uuid.uuid4() - assert get_uuid(f"{prefix}{str(uuid_).replace('-', '')}") == uuid_ - assert get_uuid(f"{prefix}{uuid_}") == uuid_ # dashed + assert get_uuid(str(uuid_).replace("-", "")) == uuid_ @pytest.mark.parametrize( @@ -60,6 +59,8 @@ def test_get_uuid_accepts_a_bare_uuid(prefix): "OSW2ea5b605c91f4e5a95593dff79fdd4a5x", "OSW2ea5b605c91f4e5a95593dff79fdd4a5.", # empty suffix "OSWzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + "osw2ea5b605c91f4e5a95593dff79fdd4a5", # prefix is case sensitive + "2ea5b605-c91f-4e5a-9559-3dff79fdd4a5", # an OSW-ID carries no dashes ], ) def test_get_uuid_rejects_what_is_not_an_osw_id(osw_id): From 3a7b6eb3f5bbb14d3369c52f0bac7cb256d85f78 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Wed, 2 Sep 2026 18:49:09 +0200 Subject: [PATCH 6/6] perf(file): cache the wiki's allowed file extensions for 24 h - WtSite.get_allowed_file_extensions() caches the siteinfo lookup for ALLOWED_FILE_EXTENSIONS_TTL, so a failed upload no longer re-queries - clear_allowed_file_extensions_cache() resets it explicitly; the page cache and clear_cache() stay independent - a failed or malformed lookup is never cached and never propagates - the rejected-extension ValueError now names the fetch time, the cache lifetime and the reset call - reraise_upload_error takes the WtSite instead of the raw mwclient site --- src/osw/controller/file/wiki.py | 36 ++--- src/osw/wtsite.py | 71 ++++++++- tests/test_wiki_file_upload_errors.py | 68 ++++++-- tests/test_wtsite_allowed_file_extensions.py | 154 +++++++++++++++++++ 4 files changed, 294 insertions(+), 35 deletions(-) create mode 100644 tests/test_wtsite_allowed_file_extensions.py diff --git a/src/osw/controller/file/wiki.py b/src/osw/controller/file/wiki.py index de6b2a79..1db1a582 100644 --- a/src/osw/controller/file/wiki.py +++ b/src/osw/controller/file/wiki.py @@ -11,21 +11,21 @@ from osw.wtsite import WtSite -def get_allowed_file_extensions(mw_site) -> Optional[List[str]]: - """Queries the file extensions the wiki accepts, None if unavailable""" - try: - result = mw_site.api( - "query", meta="siteinfo", siprop="fileextensions", formatversion=2 - ) - except Exception: - # only used to enrich an error message, never worth failing over - return None - extensions = result.get("query", {}).get("fileextensions", []) - return [entry["ext"] for entry in extensions if "ext" in entry] +def format_allowed_extensions(allowed: WtSite.AllowedFileExtensionsResult) -> str: + """Renders the allowed-extensions hint for a rejected-upload error message""" + if not allowed.extensions or allowed.fetched_at is None: + return "" + ttl_hours = int(WtSite.ALLOWED_FILE_EXTENSIONS_TTL.total_seconds() // 3600) + return ( + f" Extensions allowed there, read {allowed.fetched_at:%Y-%m-%d %H:%M:%S} and " + f"cached for {ttl_hours} h: {', '.join(sorted(allowed.extensions))}. " + "Call osw.site.clear_allowed_file_extensions_cache() to re-read the list " + "if the wiki configuration changed since." + ) def reraise_upload_error( - error: mwclient.errors.APIError, mw_site, title: str, suffix: Optional[str] + error: mwclient.errors.APIError, site: WtSite, title: str, suffix: Optional[str] ) -> None: """Turns a rejected file extension into a readable error, re-raises the rest @@ -34,15 +34,11 @@ def reraise_upload_error( """ if "filetype" not in str(getattr(error, "code", "")): raise error - allowed = get_allowed_file_extensions(mw_site) - hint = ( - f" Extensions allowed on this wiki: {', '.join(sorted(allowed))}." - if allowed - else "" - ) + allowed = site.get_allowed_file_extensions() + hint = format_allowed_extensions(allowed) raise ValueError( f"Upload of '{title}' was rejected because the file extension " - f"'{suffix}' is not allowed on {mw_site.host}.{hint}" + f"'{suffix}' is not allowed on {site.mw_site.host}.{hint}" ) from error @@ -216,7 +212,7 @@ def put(self, file: IO, **kwargs: Dict[str, Any]): ignore=True, ) except mwclient.errors.APIError as e: - reraise_upload_error(e, self.osw.mw_site, self.title, self.suffix) + reraise_upload_error(e, self.osw.site, self.title, self.suffix) assert_upload_success(result, self.title, self.osw.mw_site.host) self.osw.store_entity( OSW.StoreEntityParam( diff --git a/src/osw/wtsite.py b/src/osw/wtsite.py index 2c49b787..650c062b 100644 --- a/src/osw/wtsite.py +++ b/src/osw/wtsite.py @@ -10,7 +10,7 @@ import warnings import xml.etree.ElementTree as et from copy import deepcopy -from datetime import datetime +from datetime import datetime, timedelta from io import StringIO from pathlib import Path from pprint import pprint @@ -79,6 +79,9 @@ class WtSiteLegacyConfig(OswBaseModel): class Config: arbitrary_types_allowed = True + ALLOWED_FILE_EXTENSIONS_TTL = timedelta(hours=24) + """how long a fetched list of accepted file extensions stays valid""" + def __init__(self, config: Union[WtSiteConfig, WtSiteLegacyConfig]): """creates a new WtSite instance from a WtSiteConfig @@ -161,6 +164,10 @@ def __init__(self, config: Union[WtSiteConfig, WtSiteLegacyConfig]): self._page_cache = {} self._cache_enabled = False + # The file extensions the wiki accepts, read on demand and reused for + # ALLOWED_FILE_EXTENSIONS_TTL + self._allowed_file_extensions = None + def _get_session_lock(self) -> threading.RLock: """Return the session lock, lazily creating it if absent. @@ -492,6 +499,68 @@ def clear_cache(self): del self._page_cache self._page_cache = {} + class AllowedFileExtensionsResult(OswBaseModel): + """The file extensions a wiki accepts and when that list was read""" + + extensions: Optional[List[str]] + """the accepted extensions, None if the lookup failed""" + fetched_at: Optional[datetime] + """when the list was read from the wiki, None if the lookup failed""" + + def get_allowed_file_extensions( + self, refresh: bool = False + ) -> AllowedFileExtensionsResult: + """Returns the file extensions the wiki accepts, from cache if possible + + The list only changes when the wiki configuration changes, so it is + cached for ALLOWED_FILE_EXTENSIONS_TTL instead of being read on every + call. A failed lookup is never cached, so the next call retries; a + failed refresh leaves any previously cached list in place. + + Parameters + ---------- + refresh: + if True, bypasses the cache and re-reads the list from the wiki + + Returns + ------- + an AllowedFileExtensionsResult with the accepted extensions and + when they were read, or with both fields None if the lookup failed + """ + cached = getattr(self, "_allowed_file_extensions", None) + if ( + cached is not None + and not refresh + and cached.fetched_at is not None + and datetime.now() - cached.fetched_at < self.ALLOWED_FILE_EXTENSIONS_TTL + ): + return cached + try: + result = self.mw_site.api( + "query", meta="siteinfo", siprop="fileextensions", formatversion=2 + ) + extensions = [ + entry["ext"] + for entry in result.get("query", {}).get("fileextensions", []) + if "ext" in entry + ] + except Exception: + # only used to enrich an error message, never worth failing over + return WtSite.AllowedFileExtensionsResult(extensions=None, fetched_at=None) + self._allowed_file_extensions = WtSite.AllowedFileExtensionsResult( + extensions=extensions, fetched_at=datetime.now() + ) + return self._allowed_file_extensions + + def clear_allowed_file_extensions_cache(self): + """Clears the cached list of file extensions the wiki accepts + + The next get_allowed_file_extensions() call reads the list from the + wiki again. Use this when the wiki's upload configuration changed + within ALLOWED_FILE_EXTENSIONS_TTL. + """ + self._allowed_file_extensions = None + def _clear_cookies(self): # see https://github.com/mwclient/mwclient/issues/221 # Iterate a snapshot (list(...)) so mutating the jar mid-loop is safe, and diff --git a/tests/test_wiki_file_upload_errors.py b/tests/test_wiki_file_upload_errors.py index 211cf1b8..bef59575 100644 --- a/tests/test_wiki_file_upload_errors.py +++ b/tests/test_wiki_file_upload_errors.py @@ -13,25 +13,38 @@ from osw.controller.file.wiki import ( assert_upload_success, - get_allowed_file_extensions, + format_allowed_extensions, reraise_upload_error, store_params_for_upload, ) +from osw.wtsite import WtSite -class _FakeSite: +class _FakeMwSite: + """Stands in for the mwclient.Site behind WtSite.mw_site.""" + host = "wiki.example.org" def __init__(self, extensions=None, fail=False): - self._extensions = extensions + self._extensions = extensions or [] self._fail = fail + self.api_calls = 0 def api(self, *args, **kwargs): + self.api_calls += 1 if self._fail: raise RuntimeError("siteinfo unavailable") return {"query": {"fileextensions": [{"ext": e} for e in self._extensions]}} +def _fake_site(extensions=None, fail=False): + """Builds a minimal WtSite carrying a fake mwclient site, bypassing __init__.""" + ws = WtSite.__new__(WtSite) + ws._site = _FakeMwSite(extensions=extensions, fail=fail) + ws._allowed_file_extensions = None + return ws + + def _api_error(code): return mwclient.errors.APIError(code, "info", {}) @@ -40,7 +53,7 @@ def _api_error(code): "code", ["filetype-banned", "filetype-banned-type", "filetype-badtype"] ) def test_rejected_extension_names_the_extension(code): - site = _FakeSite(extensions=["png", "pdf"]) + site = _fake_site(extensions=["png", "pdf"]) with pytest.raises(ValueError) as exc_info: reraise_upload_error(_api_error(code), site, "OSW123.exe", ".exe") @@ -53,7 +66,7 @@ def test_rejected_extension_names_the_extension(code): def test_original_error_is_chained(): - site = _FakeSite(extensions=["png"]) + site = _fake_site(extensions=["png"]) original = _api_error("filetype-banned") with pytest.raises(ValueError) as exc_info: @@ -63,7 +76,7 @@ def test_original_error_is_chained(): def test_unrelated_api_error_is_reraised_unchanged(): - site = _FakeSite(extensions=["png"]) + site = _fake_site(extensions=["png"]) original = _api_error("readapidenied") with pytest.raises(mwclient.errors.APIError) as exc_info: @@ -74,23 +87,50 @@ def test_unrelated_api_error_is_reraised_unchanged(): def test_message_omits_the_hint_when_siteinfo_fails(): """A failing siteinfo lookup must not mask the upload error.""" - site = _FakeSite(fail=True) + site = _fake_site(fail=True) + + with pytest.raises(ValueError) as exc_info: + reraise_upload_error(_api_error("filetype-banned"), site, "a.exe", ".exe") + + message = str(exc_info.value) + assert "Extensions allowed" not in message + assert "clear_allowed_file_extensions_cache" not in message + assert ".exe" in message + + +def test_message_names_the_fetch_time_and_the_reset_hint(): + site = _fake_site(extensions=["png", "pdf"]) with pytest.raises(ValueError) as exc_info: reraise_upload_error(_api_error("filetype-banned"), site, "a.exe", ".exe") - assert "allowed on this wiki" not in str(exc_info.value) - assert ".exe" in str(exc_info.value) + message = str(exc_info.value) + fetched_at = site.get_allowed_file_extensions().fetched_at + assert f"{fetched_at:%Y-%m-%d %H:%M:%S}" in message + assert "osw.site.clear_allowed_file_extensions_cache()" in message + + +def test_a_second_rejected_upload_does_not_query_the_wiki_again(): + site = _fake_site(extensions=["png", "pdf"]) + + with pytest.raises(ValueError): + reraise_upload_error(_api_error("filetype-banned"), site, "a.exe", ".exe") + with pytest.raises(ValueError): + reraise_upload_error(_api_error("filetype-banned"), site, "b.exe", ".exe") + + assert site.mw_site.api_calls == 1 + +def test_format_allowed_extensions_is_empty_when_extensions_is_none(): + allowed = WtSite.AllowedFileExtensionsResult(extensions=None, fetched_at=None) -def test_get_allowed_file_extensions_returns_none_on_failure(): - assert get_allowed_file_extensions(_FakeSite(fail=True)) is None + assert format_allowed_extensions(allowed) == "" -def test_get_allowed_file_extensions_reads_siteinfo(): - site = _FakeSite(extensions=["png", "jpg"]) +def test_format_allowed_extensions_is_empty_when_extensions_is_empty(): + allowed = WtSite.AllowedFileExtensionsResult(extensions=[], fetched_at=None) - assert get_allowed_file_extensions(site) == ["png", "jpg"] + assert format_allowed_extensions(allowed) == "" def test_successful_upload_passes(): diff --git a/tests/test_wtsite_allowed_file_extensions.py b/tests/test_wtsite_allowed_file_extensions.py new file mode 100644 index 00000000..24aca44e --- /dev/null +++ b/tests/test_wtsite_allowed_file_extensions.py @@ -0,0 +1,154 @@ +"""Unit tests for WtSite.get_allowed_file_extensions and its cache. + +These tests construct a WtSite without going through __init__ (which would +require network / credentials) and exercise the cache mechanics directly +against a fake mwclient site. +""" + +from datetime import datetime, timedelta + +from osw.wtsite import WtSite + + +class _FakeMwSite: + """Stands in for the mwclient.Site behind WtSite.mw_site.""" + + host = "wiki.example.org" + + def __init__(self, extensions=None, fail=False, payload=None): + self._extensions = extensions or [] + self._fail = fail + self._payload = payload + self.api_calls = 0 + self.calls = [] + """the (args, kwargs) of every api() call, to check the request sent""" + + def api(self, *args, **kwargs): + self.api_calls += 1 + self.calls.append((args, kwargs)) + if self._fail: + raise RuntimeError("siteinfo unavailable") + if self._payload is not None: + return self._payload + return {"query": {"fileextensions": [{"ext": e} for e in self._extensions]}} + + +def _fake_site(extensions=None, fail=False, payload=None): + """Builds a minimal WtSite carrying a fake mwclient site, bypassing __init__.""" + ws = WtSite.__new__(WtSite) + ws._site = _FakeMwSite(extensions=extensions, fail=fail, payload=payload) + ws._allowed_file_extensions = None + return ws + + +def test_reads_the_extensions_from_siteinfo_and_returns_a_fetched_at(): + site = _fake_site(extensions=["png", "pdf"]) + + result = site.get_allowed_file_extensions() + + assert result.extensions == ["png", "pdf"] + assert isinstance(result.fetched_at, datetime) + + +def test_a_second_call_returns_the_cached_value_without_querying_the_wiki_again(): + site = _fake_site(extensions=["png", "pdf"]) + + first = site.get_allowed_file_extensions() + second = site.get_allowed_file_extensions() + + assert second is first + assert site.mw_site.api_calls == 1 + + +def test_refresh_true_re_reads_and_updates_fetched_at(): + site = _fake_site(extensions=["png"]) + + first = site.get_allowed_file_extensions() + second = site.get_allowed_file_extensions(refresh=True) + + assert site.mw_site.api_calls == 2 + assert second.fetched_at >= first.fetched_at + + +def test_an_entry_older_than_the_ttl_is_re_read(): + site = _fake_site(extensions=["png"]) + site.get_allowed_file_extensions() + site._allowed_file_extensions.fetched_at = datetime.now() - timedelta(hours=25) + + site.get_allowed_file_extensions() + + assert site.mw_site.api_calls == 2 + + +def test_an_entry_23_hours_old_is_still_served_from_the_cache(): + site = _fake_site(extensions=["png"]) + site.get_allowed_file_extensions() + site._allowed_file_extensions.fetched_at = datetime.now() - timedelta(hours=23) + + site.get_allowed_file_extensions() + + assert site.mw_site.api_calls == 1 + + +def test_clear_allowed_file_extensions_cache_forces_a_re_read(): + site = _fake_site(extensions=["png"]) + site.get_allowed_file_extensions() + + site.clear_allowed_file_extensions_cache() + site.get_allowed_file_extensions() + + assert site.mw_site.api_calls == 2 + + +def test_a_failing_lookup_returns_none_and_is_not_cached(): + """A failed lookup must not poison the cache: a later successful call must + still return the real list, not the failure.""" + site = _fake_site(fail=True) + + failed = site.get_allowed_file_extensions() + + assert failed.extensions is None + assert failed.fetched_at is None + + site.mw_site._fail = False + site.mw_site._extensions = ["png"] + succeeded = site.get_allowed_file_extensions() + + assert succeeded.extensions == ["png"] + + +def test_a_wtsite_without_the_attribute_set_at_all_still_works(): + """A WtSite built via __new__ may not have _allowed_file_extensions set, + the getattr fallback in get_allowed_file_extensions must handle that.""" + ws = WtSite.__new__(WtSite) + ws._site = _FakeMwSite(extensions=["png"]) + + result = ws.get_allowed_file_extensions() + + assert result.extensions == ["png"] + + +def test_the_request_sent_matches_the_siteinfo_fileextensions_query(): + """A typo in the action, meta or siprop would otherwise pass unnoticed.""" + site = _fake_site(extensions=["png"]) + + site.get_allowed_file_extensions() + + args, kwargs = site.mw_site.calls[0] + assert args == ("query",) + assert kwargs == { + "meta": "siteinfo", + "siprop": "fileextensions", + "formatversion": 2, + } + + +def test_a_malformed_siteinfo_payload_returns_none_instead_of_raising(): + """fileextensions entries that are not mappings must not crash the lookup, + since it only ever enriches an error message.""" + site = _fake_site(payload={"query": {"fileextensions": [1, 2, 3]}}) + + result = site.get_allowed_file_extensions() + + assert result.extensions is None + assert result.fetched_at is None