Fix OneDrive upload service to report all missing files at once#169221
Conversation
Previously _read_file_contents raised on the first missing file, leaving subsequent missing files unreported. Collect all missing filenames and raise a single HomeAssistantError with all of them listed in the translation placeholder
home-assistant
Bot
added
bugfix
cla-signed
has-tests
integration: onedrive
small-pr
|
Hey there @zweckj, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR improves the OneDrive upload service’s error reporting so that when multiple filenames are provided, all missing files are reported in a single translated HomeAssistantError rather than failing on the first missing path.
Changes:
- Update
_read_file_contentsto collect all missing filenames before raising an error. - Add a new
filenames_do_not_existtranslation key for multi-file missing errors. - Extend test coverage to validate that multiple missing files are surfaced together.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
homeassistant/components/onedrive/services.py |
Collects missing filenames during upload preparation and raises a single translated error after processing inputs. |
homeassistant/components/onedrive/strings.json |
Adds a new exception translation message for multiple missing filenames. |
tests/components/onedrive/test_services.py |
Updates existing missing-file test assertions and adds a new test for multiple missing filenames. |
- Defer stat/read I/O until after all missing filenames are collected, avoiding unnecessary filesystem work when any file is absent - Cache Path.stat().st_size in a local variable to avoid a double syscall and a subtle race in the file_too_large error path - Route single missing file through the existing filename_does_not_exist key for more natural phrasing; two or more use filenames_do_not_exist - Fix "file(s)" wording to "files" in the filenames_do_not_exist message - Add the missing filenames_do_not_exist entry to translations/en.json - Update test_filename_does_not_exist to assert the singular key and placeholder
| if len(missing) == 1: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="filename_does_not_exist", | ||
| translation_placeholders={"filename": missing[0]}, | ||
| ) | ||
| if missing: |
There was a problem hiding this comment.
I wouldn't overcomplicate, just having the second error messages (which only show one file), should be understandable enough
| if len(missing) == 1: | |
| raise HomeAssistantError( | |
| translation_domain=DOMAIN, | |
| translation_key="filename_does_not_exist", | |
| translation_placeholders={"filename": missing[0]}, | |
| ) | |
| if missing: |
There was a problem hiding this comment.
I wouldn't overcomplicate, just having the second error messages (which only show one file), should be understandable enough
Just to see if I understood your request correctly: you suggest removing singular/plural branching, using filenames_do_not_exist for both 1 and many, right?
If that was what you were thinking, I agree. I made this separation due to concerns about translations of singular/plural strings, but personally, I do not like it either.
There was a problem hiding this comment.
If you confirm this, we should also adjust this same condition in #168064
- Remove the singular error handling for a single missing file, consolidating it to report all missing files at once. - Update translation keys to reflect the change from "filename_does_not_exist" to "filenames_do_not_exist" for consistency. - Adjust tests to validate the new error handling behavior for multiple missing files.
When uploading multiple files, if more than one file does not exist, only the first missing file was reported and the rest were silently ignored. This was because
_read_file_contentsraised aHomeAssistantErrorimmediately on the firstPath.exists()failure. This PR collects all missing filenames before raising, then surfaces all of them in a singleHomeAssistantErrorusing the newfilenames_do_not_existtranslation key.Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: