You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kyupark (via GitHub)" <gi...@apache.org> on 2026/05/13 10:11:28 UTC
[PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
kyupark opened a new pull request, #66850:
URL: https://github.com/apache/airflow/pull/66850
Add an `overwrite` parameter to the IMAP hook's `download_mail_attachments()` method.
When `overwrite=True` (default), duplicate filenames are overwritten (current behavior).
When `overwrite=False`, unique filenames are generated by appending a suffix (e.g. `report_1.csv`, `report_2.csv`).
The non-overwrite path uses exclusive create mode (`"xb"`) to avoid TOCTOU races, and checks both in-memory state and the filesystem to handle files left over from previous runs.
closes: #65870
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 4.7)
Generated-by: Claude Code (Opus 4.7) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "kyupark (via GitHub)" <gi...@apache.org>.
kyupark commented on PR #66850:
URL: https://github.com/apache/airflow/pull/66850#issuecomment-4486110932
@phanikumv I added the focused FileExistsError retry test you suggested: the first `open(..., "xb")` for `report.csv` raises `FileExistsError`, then the retry succeeds with `report_1.csv`.
Local checks passed and CI is green. I’ve marked the thread resolved. Could you take a final look when you have a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #66850:
URL: https://github.com/apache/airflow/pull/66850#issuecomment-4894934647
Fixed in https://github.com/apache/airflow/pull/68838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #66850:
URL: https://github.com/apache/airflow/pull/66850#issuecomment-4798268714
@kyupark — this PR has 2 unresolved review thread(s) that still look like they need your attention. Once you've addressed them (push changes and/or reply in-thread), please resolve the threads or reply to confirm, and give the reviewer a nudge for another look. Thanks!
See the [PR quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
<sub>Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. ([why automated](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md))</sub>
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #66850:
URL: https://github.com/apache/airflow/pull/66850#issuecomment-4483041916
@kyupark — There is 1 unresolved review thread on this PR from `phanikumv`, and you have engaged with it (post-review commits and/or in-thread replies). Could you confirm whether you believe the feedback is fully addressed and the PR is ready for maintainer review confirmation?
If yes, please mark the thread as resolved and ping the reviewer (`phanikumv`) for a final look. They will either label the PR ready for maintainer review or follow up with additional feedback.
If you are still working on the thread, please reply with what is outstanding so the threads stay unresolved on purpose.
---
_Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "kyupark (via GitHub)" <gi...@apache.org>.
kyupark commented on code in PR #66850:
URL: https://github.com/apache/airflow/pull/66850#discussion_r3253387439
##########
providers/imap/tests/unit/imap/hooks/test_imap.py:
##########
@@ -509,3 +509,117 @@ def test_has_mail_attachment_with_max_mails(self, mock_imaplib):
assert result is True
assert 1 <= mock_conn.fetch.call_count <= 2
+
+ @patch(open_string, new_callable=mock_open)
+ @patch(imaplib_string)
+ def test_download_attachments_overwrite_default(self, mock_imaplib, mock_open_method):
Review Comment:
Thanks, good point. Added a focused test for this path: the first `open(..., "xb")` for `report.csv` raises `FileExistsError`, then the retry succeeds with `report_1.csv`.
Local checks passed: ruff format/check on `test_imap.py`, plus the full IMAP hook test file (`41 passed`). CI is green as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #66850:
URL: https://github.com/apache/airflow/pull/66850#issuecomment-4668633663
@kyupark can you please address the open comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "kyupark (via GitHub)" <gi...@apache.org>.
kyupark commented on PR #66850:
URL: https://github.com/apache/airflow/pull/66850#issuecomment-4467650842
Addressed the review comment about the untested FileExistsError retry path. I added a focused unit test where exclusive create first raises FileExistsError and then succeeds with the next generated filename.\n\nLocal checks run:\n- uv run --project providers/imap ruff format providers/imap/tests/unit/imap/hooks/test_imap.py --check\n- uv run --project providers/imap ruff check providers/imap/tests/unit/imap/hooks/test_imap.py\n- uv run --project providers/imap pytest providers/imap/tests/unit/imap/hooks/test_imap.py -q\n\nResult: 41 passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "phanikumv (via GitHub)" <gi...@apache.org>.
phanikumv commented on code in PR #66850:
URL: https://github.com/apache/airflow/pull/66850#discussion_r3252688464
##########
providers/imap/tests/unit/imap/hooks/test_imap.py:
##########
@@ -509,3 +509,117 @@ def test_has_mail_attachment_with_max_mails(self, mock_imaplib):
assert result is True
assert 1 <= mock_conn.fetch.call_count <= 2
+
+ @patch(open_string, new_callable=mock_open)
+ @patch(imaplib_string)
+ def test_download_attachments_overwrite_default(self, mock_imaplib, mock_open_method):
Review Comment:
The `FileExistsError` retry is exactly the path these 6 tests don't cover. Recommend one test where `mock_open` raises `FileExistsError` on the first call and succeeds on the next.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck closed pull request #66850: Add overwrite parameter to IMAP hook download_mail_attachments
URL: https://github.com/apache/airflow/pull/66850
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add overwrite parameter to IMAP hook download_mail_attachments [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #66850:
URL: https://github.com/apache/airflow/pull/66850#discussion_r3344564677
##########
providers/imap/src/airflow/providers/imap/hooks/imap.py:
##########
@@ -287,14 +292,54 @@ def _check_mail_body(
return mail.get_attachments_by_name(name, check_regex, find_first=latest_only)
return []
- def _create_files(self, mail_attachments: list, local_output_directory: str) -> None:
+ def _create_files(
+ self, mail_attachments: list, local_output_directory: str, *, overwrite: bool = True
+ ) -> None:
+ seen: dict[str, int] = {}
for name, payload in mail_attachments:
if self._is_symlink(name):
self.log.error("Can not create file because it is a symlink!")
elif self._is_escaping_current_directory(name):
self.log.error("Can not create file because it is escaping the current directory!")
else:
- self._create_file(name, payload, local_output_directory)
+ if not overwrite:
+ file_name = self._unique_name(name, seen, local_output_directory)
+ file_path = self._correct_path(file_name, local_output_directory)
+ while True:
+ try:
+ with open(file_path, "xb") as f:
+ f.write(payload)
+ break
+ except FileExistsError:
+ seen[file_name] = 0
+ file_name = self._unique_name(name, seen, local_output_directory)
+ file_path = self._correct_path(file_name, local_output_directory)
+ else:
+ file_name = name
+ self._create_file(file_name, payload, local_output_directory)
+
+ def _unique_name(self, name: str, seen: dict[str, int], local_output_directory: str) -> str:
Review Comment:
Names of methods should start with a verb (`_generate_unique_name`)
##########
providers/imap/src/airflow/providers/imap/hooks/imap.py:
##########
@@ -287,14 +292,54 @@ def _check_mail_body(
return mail.get_attachments_by_name(name, check_regex, find_first=latest_only)
return []
- def _create_files(self, mail_attachments: list, local_output_directory: str) -> None:
+ def _create_files(
+ self, mail_attachments: list, local_output_directory: str, *, overwrite: bool = True
+ ) -> None:
+ seen: dict[str, int] = {}
for name, payload in mail_attachments:
if self._is_symlink(name):
self.log.error("Can not create file because it is a symlink!")
elif self._is_escaping_current_directory(name):
self.log.error("Can not create file because it is escaping the current directory!")
else:
- self._create_file(name, payload, local_output_directory)
+ if not overwrite:
+ file_name = self._unique_name(name, seen, local_output_directory)
+ file_path = self._correct_path(file_name, local_output_directory)
+ while True:
+ try:
+ with open(file_path, "xb") as f:
+ f.write(payload)
+ break
+ except FileExistsError:
+ seen[file_name] = 0
Review Comment:
Might be redundant considering the logic below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org