Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def _build_email_content(self, smtp: SmtpHook, context: Context):
if smtp.from_email is not None:
self.from_email = smtp.from_email
else:
raise ValueError("You should provide `from_email` or define it in the connection")
self.from_email = conf.get("email", "from_email", fallback="airflow@airflow")
fields_to_re_render.append("from_email")
if self.subject is None:
smtp_default_templated_subject_path: str
Expand Down
66 changes: 66 additions & 0 deletions providers/smtp/tests/unit/smtp/notifications/test_smtp.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ def test_notifier_with_defaults(self, mock_smtphook_hook, create_dag_without_db,
def test_notifier_with_nondefault_connection_extra(
self, mock_smtphook_hook, create_dag_without_db, mock_task_instance
):
"""When the connection extra defines `from_email` (and no config `[email] from_email` is set),
the connection's from_email is used, including template rendering."""
# TODO: we can use create_runtime_ti fixture in place of mock_task_instance once provider has minimum AF to Airflow 3.0+
ti = mock_task_instance(
dag_id=TEST_DAG_ID,
Expand Down Expand Up @@ -211,6 +213,70 @@ def test_notifier_with_nondefault_connection_extra(
**DEFAULT_EMAIL_PARAMS,
)

@mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook")
def test_notifier_from_email_falls_back_to_config(self, mock_smtphook_hook, create_dag_without_db):
"""When config [email] from_email is set and the SMTP connection extra `from_email` is NOT set, `[email] from_email` config is used."""
mock_smtphook_hook.return_value.__enter__.return_value.from_email = None
mock_smtphook_hook.return_value.__enter__.return_value.subject_template = None
mock_smtphook_hook.return_value.__enter__.return_value.html_content_template = None

with conf_vars({("email", "from_email"): "config@example.com"}):
notifier = SmtpNotifier(to=TEST_RECEIVER, subject=TEST_SUBJECT, html_content=TEST_BODY)
notifier({"dag": create_dag_without_db(TEST_DAG_ID)})

mock_smtphook_hook.return_value.__enter__().send_email_smtp.assert_called_once_with(
from_email="config@example.com",
to=TEST_RECEIVER,
subject=TEST_SUBJECT,
html_content=TEST_BODY,
smtp_conn_id=SMTP_CONN_ID,
**DEFAULT_EMAIL_PARAMS,
)

@mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook")
def test_notifier_connection_extra_takes_precedence_over_config(
self, mock_smtphook_hook, create_dag_without_db
):
"""The SMTP connection extra `from_email` must win over the `[email] from_email` config."""
mock_smtphook_hook.return_value.__enter__.return_value.from_email = "conn@example.com"
mock_smtphook_hook.return_value.__enter__.return_value.subject_template = None
mock_smtphook_hook.return_value.__enter__.return_value.html_content_template = None

with conf_vars({("email", "from_email"): "config@example.com"}):
notifier = SmtpNotifier(to=TEST_RECEIVER, subject=TEST_SUBJECT, html_content=TEST_BODY)
notifier({"dag": create_dag_without_db(TEST_DAG_ID)})

mock_smtphook_hook.return_value.__enter__().send_email_smtp.assert_called_once_with(
from_email="conn@example.com",
to=TEST_RECEIVER,
subject=TEST_SUBJECT,
html_content=TEST_BODY,
smtp_conn_id=SMTP_CONN_ID,
**DEFAULT_EMAIL_PARAMS,
)

@mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook")
def test_notifier_from_email_default_when_nothing_configured(
self, mock_smtphook_hook, create_dag_without_db
):
"""Falls back to the hard-coded default when neither the connection nor config define from_email."""
mock_smtphook_hook.return_value.__enter__.return_value.from_email = None
mock_smtphook_hook.return_value.__enter__.return_value.subject_template = None
mock_smtphook_hook.return_value.__enter__.return_value.html_content_template = None

with conf_vars({("email", "from_email"): None}):
notifier = SmtpNotifier(to=TEST_RECEIVER, subject=TEST_SUBJECT, html_content=TEST_BODY)
notifier({"dag": create_dag_without_db(TEST_DAG_ID)})

mock_smtphook_hook.return_value.__enter__().send_email_smtp.assert_called_once_with(
from_email="airflow@airflow",
to=TEST_RECEIVER,
subject=TEST_SUBJECT,
html_content=TEST_BODY,
smtp_conn_id=SMTP_CONN_ID,
**DEFAULT_EMAIL_PARAMS,
)

@mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook")
def test_notifier_with_custom_smtp_conn_id(self, mock_smtphook_hook, create_dag_without_db):
"""Test that a custom smtp_conn_id is correctly passed to SmtpHook."""
Expand Down
2 changes: 1 addition & 1 deletion task-sdk/src/airflow/sdk/execution_time/task_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -2058,7 +2058,7 @@ def _send_error_email_notification(
to=to_emails,
subject=subject,
html_content=html_content,
from_email=conf.get("email", "from_email", fallback="airflow@airflow"),
from_email=None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other callers that still resolve from_email themselves before constructing the notifier? Please check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SameerMesiah97

Good question — I checked. SmtpNotifier is only constructed in one production call site outside its own module, which is _send_error_email_notification in task_runner.py, and that's already updated in this PR to pass from_email=None. I didn't find any other operator or utility in the repo that resolves from_email from config before building the notifier, so this should cover all callers.

)
notifier(email_context)
except Exception:
Expand Down
6 changes: 3 additions & 3 deletions task-sdk/tests/task_sdk/execution_time/test_task_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -3896,7 +3896,7 @@ def execute(self, context):
else:
mock_smtp_notifier.assert_called_once()
kwargs = mock_smtp_notifier.call_args.kwargs
assert kwargs["from_email"] == self.FROM
assert kwargs["from_email"] is None
assert kwargs["to"] == emails
assert (
kwargs["html_content"]
Expand Down Expand Up @@ -3954,7 +3954,7 @@ def execute(self, context):
else:
mock_smtp_notifier.assert_called_once()
kwargs = mock_smtp_notifier.call_args.kwargs
assert kwargs["from_email"] == self.FROM
assert kwargs["from_email"] is None
assert kwargs["to"] == emails
assert (
kwargs["html_content"]
Expand Down Expand Up @@ -4006,7 +4006,7 @@ def execute(self, context):
kwargs["html_content"]
== "<h1>Custom Template</h1><p>Task: {{ti.task_id}}</p><p>Error: {{exception_html}}</p>"
)
assert kwargs["from_email"] == self.FROM
assert kwargs["from_email"] is None

@pytest.mark.enable_redact
def test_rendered_templates_mask_secrets(self, create_runtime_ti, mock_supervisor_comms):
Expand Down