Add overwrite_file option to IMAP download_mail_attachments#67588
Add overwrite_file option to IMAP download_mail_attachments#67588Jualhosting wants to merge 7 commits into
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
The overall approach looks reasonable, but this is missing tests. I think we should cover the following scenarios:
- Overwriting an existing file
- Preserving an existing file when
overwrite_file=False - Suffix incrementing (
file_1,file_2, etc.) - Preserving file extensions so formats like
.csvare not corrupted accidentally during renaming
CI needs to be triggered as well.
|
Thanks for the feedback! I have added unit tests covering both overwrite_file=True and overwrite_file=False scenarios. This includes testing the suffix incrementing logic to ensure existing files (and their extensions) are preserved properly. The new tests have been pushed and should trigger the CI. |
23tae
left a comment
There was a problem hiding this comment.
Nice work, @Jualhosting!
I've left a minor inline comment regarding production robustness.
A quick note on PR guidelines:
As per the Airflow PR Quality Criteria, all PRs need a meaningful description. Could you please update the PR body to briefly explain why this feature is needed?
… avoid race conditions
|
Done! I've updated the PR body with the explanation and implemented the atomic file creation using \xb\ mode as suggested. Thanks for catching that! |
|
@Jualhosting It looks like your description was placed inside the HTML comment tags ( If you move your text outside of those comment tags, it will display correctly. |
|
@23tae Ah, you are absolutely right! The CLI tool I used to update the description accidentally swallowed the line breaks and pushed it inside the template comments. I've just updated the PR body again manually to remove the HTML comments and restore the proper formatting. It should be fully visible and readable now! Thanks for the heads-up! 🍻 |
|
Looks like this addresses #65870 — might be worth referencing it in the description. |
Description
This PR adds a new boolean argument
overwrite_filetoImapHook.download_mail_attachments.Previously,
download_mail_attachmentswould always overwrite local files if a downloaded attachment had the same filename as an existing file. By settingoverwrite_file=False, the hook now safely preserves existing files by appending a numeric suffix (e.g.,file_1.ext). The file creation uses exclusive creation mode ("xb") to ensure atomic, race-condition-free operation.Testing
overwrite_filelogic."xb"properly raisesFileExistsErrorand loops to the next suffix.Was generative AI tooling used to co-author this PR?
Generated-by: Antigravity (Gemini-based AI coding assistant) following the guidelines