Skip to content

feat(file): report upload failures clearly and verify before storing - #153

Open
LukasGold wants to merge 3 commits into
mainfrom
fix/file-upload-extension-error
Open

feat(file): report upload failures clearly and verify before storing#153
LukasGold wants to merge 3 commits into
mainfrom
fix/file-upload-extension-error

Conversation

@LukasGold

@LukasGold LukasGold commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes #51.

Changes

  • reraise_upload_error: translate MediaWiki's filetype-banned, filetype-banned-type and filetype-badtype into a ValueError naming the offending extension and listing what the wiki accepts. Every other APIError is re-raised unchanged.
  • get_allowed_file_extensions: read that list from siteinfo/fileextensions, returning None when unavailable so a broken lookup never masks the upload error.
  • assert_upload_success: raise unless the upload API reported result: "Success", quoting any returned warnings.
  • WikiFileController.put: upload, verify, then store_entity, instead of storing first.
  • store_params_for_upload: pass overwrite="replace remote" when the file page did not exist before the upload.
  • 16 offline unit tests in tests/test_wiki_file_upload_errors.py.

Rationale

mwclient.Site.handle_api_result raises only when the response carries an error key. Site.upload inspects the returned warnings for exactly one case (exists) and only when ignore=False, and put passes ignore=True and discarded the return value entirely. So an upload MediaWiki declined for any reason short of an API error returned normally, and put reported 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() sets self.title before either call, and store_entity operates on self.cast(model.WikiFile, ...), a new instance built from _raw_dict(), so the store never mutates self.title.

Why the overwrite policy had to change with the order

The upload creates the file page when it is not already there. store_entity then finds an existing page and, under its default keep 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_entity returned None and the test died on AttributeError: 'NoneType' object has no attribute 'cast'.

Reading page.exists before the upload and forcing replace remote for 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

  • An upload returning a non-Success result now raises ValueError instead of returning silently.
  • Uploading onto a file page that did not exist yet writes the metadata with replace remote rather 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_entity runs, its uuid validator reaches the branch at https://github.com/OpenSemanticLab/osw-python/blob/main/src/osw/core.py#L1348-L1357 and prints Error: UUID could not be determined from title ..., since get_uuid does 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.upload accepts filekey but exposes no stash parameter, so the stashing call would have to be made against the raw API. Out of scope here.

Verification

tests/test_wiki_file_upload_errors.py passes offline (16 tests). tests/integration/test_file_controller.py covers the reordering against the test wiki; both the Main and the Integration workflow are green on the final commit.

- 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
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Release preview

Merging this PR would release v2.1.0 (current: v2.0.2).

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
@LukasGold LukasGold changed the title feat(file): report a rejected file extension clearly on upload feat(file): report upload failures clearly and verify before storing Sep 1, 2026
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add error message if file upload fails due to not allowed extension

1 participant