feat(file): report upload failures clearly and verify before storing - #153
Open
LukasGold wants to merge 3 commits into
Open
feat(file): report upload failures clearly and verify before storing#153LukasGold wants to merge 3 commits into
LukasGold wants to merge 3 commits into
Conversation
- 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
Contributor
Release previewMerging this PR would release v2.1.0 (current: Changelog preview (truncated)## v2.1.0 (2026-09-01)
### Bug Fixes
- **file**: Verify the upload before storing the file entity
([`4a09bf5`](https://github.com/OpenSemanticLab/osw-python/commit/4a09bf541a3ea404dcd2e1cd41ea36b8c53ce141))
- **file**: Write the metadata onto the page the upload created
([`eb9e0e1`](https://github.com/OpenSemanticLab/osw-python/commit/eb9e0e112e8b75128c8a3fe5c8d5943531c16d6a))
### Features
- **file**: Report a rejected file extension clearly on upload
([`bb49392`](https://github.com/OpenSemanticLab/osw-python/commit/bb49392a4fc81801d3662613faae96944dd8ed91))
### Testing
- Rename oold.py to oold_test.py so its tests are collected
([`20072a9`](https://github.com/OpenSemanticLab/osw-python/commit/20072a9249cd97126a222c62a70f84e0433343ef))
Preview via python-semantic-release and conventional commits. |
- 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
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51.
Changes
reraise_upload_error: translate MediaWiki'sfiletype-banned,filetype-banned-typeandfiletype-badtypeinto aValueErrornaming the offending extension and listing what the wiki accepts. Every otherAPIErroris re-raised unchanged.get_allowed_file_extensions: read that list fromsiteinfo/fileextensions, returningNonewhen unavailable so a broken lookup never masks the upload error.assert_upload_success: raise unless the upload API reportedresult: "Success", quoting any returned warnings.WikiFileController.put: upload, verify, thenstore_entity, instead of storing first.store_params_for_upload: passoverwrite="replace remote"when the file page did not exist before the upload.tests/test_wiki_file_upload_errors.py.Rationale
mwclient.Site.handle_api_resultraises only when the response carries anerrorkey.Site.uploadinspects the returned warnings for exactly one case (exists) and only whenignore=False, andputpassesignore=Trueand discarded the return value entirely. So an upload MediaWiki declined for any reason short of an API error returned normally, andputreported success having stored nothing. mwclient names this same class of problem "silent failure" in its own workaround for mwclient/mwclient#211.The page edit and the file upload are separate writes with no transaction across them, so ordering only decides which half-state survives a failure. Uploading first is preferred because the leftover, a file page without metadata, is visibly incomplete, whereas the previous leftover, an entity carrying metadata but no binary, resolves in queries and looks valid until someone tries to download it. It is also the cheap half that gets retried rather than the expensive one.
Reordering is safe with respect to the title:
_init()setsself.titlebefore either call, andstore_entityoperates onself.cast(model.WikiFile, ...), a new instance built from_raw_dict(), so the store never mutatesself.title.Why the overwrite policy had to change with the order
The upload creates the file page when it is not already there.
store_entitythen finds an existing page and, under its defaultkeep existing, returns without writing anything (https://github.com/OpenSemanticLab/osw-python/blob/main/src/osw/core.py#L1422-L1430), leaving a binary with no jsondata behind it. The integration suite caught exactly that:load_entityreturnedNoneand the test died onAttributeError: 'NoneType' object has no attribute 'cast'.Reading
page.existsbefore the upload and forcingreplace remotefor a page that was not there routes the store back through the branch it used to take, which writes the full jsondata (https://github.com/OpenSemanticLab/osw-python/blob/main/src/osw/core.py#L1409-L1420). A page that already existed keeps whatever the caller asked for, so re-uploads behave as before.Behaviour change
Successresult now raisesValueErrorinstead of returning silently.replace remoterather than the caller's policy. There is nothing to preserve on a page the upload created moments earlier.Known cosmetic side effect
Because the page now exists by the time
store_entityruns, its uuid validator reaches the branch at https://github.com/OpenSemanticLab/osw-python/blob/main/src/osw/core.py#L1348-L1357 and printsError: UUID could not be determined from title ..., sinceget_uuiddoes not strip the file suffix. It prints and returns, so the store proceeds. Looks like a pre-existing gap in that validator for File pages, left alone here.Not covered
Real atomicity would need MediaWiki's upload stash: stash the file, store the entity, then publish by
filekey, with an abandoned stash expiring on its own and needing no delete rights.mwclient.Site.uploadacceptsfilekeybut exposes nostashparameter, so the stashing call would have to be made against the raw API. Out of scope here.Verification
tests/test_wiki_file_upload_errors.pypasses offline (16 tests).tests/integration/test_file_controller.pycovers the reordering against the test wiki; both the Main and the Integration workflow are green on the final commit.