amoghrajesh commented on code in PR #75414: URL: https://github.com/apache/airflow/pull/75414#discussion_r4384657548
########## airflow-core/tests/unit/utils/test_email.py: ########## @@ -17,404 +17,71 @@ # under the License. from __future__ import annotations -import json -import os -from email.mime.application import MIMEApplication from email.mime.multipart import MIMEMultipart -from email.mime.text import MIMEText -from smtplib import SMTPServerDisconnected from unittest import mock import pytest -from airflow.configuration import conf +from airflow.providers.smtp.utils import emailer from airflow.utils import email +from airflow.utils.deprecation_tools import DeprecatedImportWarning -from tests_common.test_utils.config import conf_vars +DEPRECATED_NAMES = [ + "send_email", + "send_email_smtp", + "build_mime_message", + "send_mime_email", + "get_email_address_list", +] -EMAILS = ["[email protected]", "[email protected]"] -send_email_test = mock.MagicMock() [email protected]("name", DEPRECATED_NAMES) +def test_deprecated_names_resolve_to_provider(name): + with pytest.warns( + DeprecatedImportWarning, match=f"airflow.providers.smtp.utils.emailer.{name}" + ) as record: + assert getattr(email, name) is getattr(emailer, name) + assert "BaseHook.get_connection" in str(record[0].message) -class TestEmail: - def setup_method(self): - conf.remove_option("email", "EMAIL_BACKEND") - conf.remove_option("email", "EMAIL_CONN_ID") - def test_get_email_address_single_email(self): - emails_string = "[email protected]" +def test_star_import_exposes_deprecated_names(): + namespace: dict = {} + with pytest.warns(DeprecatedImportWarning): + exec("from airflow.utils.email import *", namespace) - assert email.get_email_address_list(emails_string) == [emails_string] - - def test_get_email_address_comma_sep_string(self): - emails_string = "[email protected], [email protected]" - - assert email.get_email_address_list(emails_string) == EMAILS - - def test_get_email_address_colon_sep_string(self): - emails_string = "[email protected]; [email protected]" - - assert email.get_email_address_list(emails_string) == EMAILS - - def test_get_email_address_list(self): - emails_list = ["[email protected]", "[email protected]"] - - assert email.get_email_address_list(emails_list) == EMAILS - - def test_get_email_address_tuple(self): - emails_tuple = ("[email protected]", "[email protected]") - - assert email.get_email_address_list(emails_tuple) == EMAILS - - def test_get_email_address_invalid_type(self): - emails_string = 1 - - with pytest.raises(TypeError): - email.get_email_address_list(emails_string) - - def test_get_email_address_invalid_type_in_iterable(self): - emails_list = ["[email protected]", 2] - - with pytest.raises(TypeError): - email.get_email_address_list(emails_list) - - @mock.patch("airflow.utils.email.send_email") - def test_default_backend(self, mock_send_email): - res = email.send_email("to", "subject", "content") - mock_send_email.assert_called_once_with("to", "subject", "content") - assert mock_send_email.return_value == res - - @mock.patch("airflow.utils.email.send_email_smtp") - def test_custom_backend(self, mock_send_email): - with conf_vars( - { - ("email", "email_backend"): "unit.utils.test_email.send_email_test", - ("email", "email_conn_id"): "smtp_default", - } - ): - email.send_email("to", "subject", "content") - send_email_test.assert_called_once_with( - "to", - "subject", - "content", - files=None, - dryrun=False, - cc=None, - bcc=None, - mime_charset="utf-8", - mime_subtype="mixed", - conn_id="smtp_default", - from_email=None, - custom_headers=None, - ) - assert not mock_send_email.called - - @mock.patch("airflow.utils.email.send_email_smtp") - @conf_vars( - { - ("email", "email_backend"): "unit.utils.test_email.send_email_test", - ("email", "email_conn_id"): "smtp_default", - ("email", "from_email"): "[email protected]", - } - ) - def test_custom_backend_sender(self, mock_send_email_smtp): - email.send_email("to", "subject", "content") - _, call_kwargs = send_email_test.call_args - assert call_kwargs["from_email"] == "[email protected]" - assert not mock_send_email_smtp.called - - def test_build_mime_message(self): - mail_from = "[email protected]" - mail_to = "[email protected]" - subject = "test subject" - html_content = "<html>Test</html>" - custom_headers = {"Reply-To": "[email protected]"} - - msg, recipients = email.build_mime_message( - mail_from=mail_from, - to=mail_to, - subject=subject, - html_content=html_content, - custom_headers=custom_headers, - ) - - assert "From" in msg - assert "To" in msg - assert "Subject" in msg - assert "Reply-To" in msg - assert [mail_to] == recipients - assert msg["To"] == ",".join(recipients) - - def test_build_mime_message_escapes_attachment_filename(self, tmp_path): - # A quote in the filename must not break out of the quoted - # Content-Disposition value and inject extra parameters. - malicious = 'report.txt"; x-evil="1' - attachment = tmp_path / malicious - attachment.write_bytes(b"data") - - msg, _ = email.build_mime_message( - mail_from="[email protected]", - to="[email protected]", - subject="subject", - html_content="<html></html>", - files=[os.fspath(attachment)], - ) - - part = msg.get_payload()[-1] - assert part.get_filename() == malicious - assert "x-evil" not in dict(part.get_params(header="Content-Disposition")) + assert {name: namespace.get(name) for name in DEPRECATED_NAMES} == { + name: getattr(emailer, name) for name in DEPRECATED_NAMES + } @pytest.mark.db_test -class TestEmailSmtp: - @pytest.fixture(autouse=True) - def setup_test_cases(self, monkeypatch): - monkeypatch.setenv( # Set the default smtp connection for all test cases - "AIRFLOW_CONN_SMTP_DEFAULT", - json.dumps({"conn_type": "smtp", "login": "user", "password": "p@$$word"}), - ) - - @mock.patch("airflow.utils.email.send_mime_email") - def test_send_smtp(self, mock_send_mime, tmp_path): - path = tmp_path / "testfile" - path.write_text("attachment") - email.send_email_smtp("to", "subject", "content", files=[os.fspath(path)]) - assert mock_send_mime.called - _, call_args = mock_send_mime.call_args - assert conf.get("smtp", "SMTP_MAIL_FROM") == call_args["e_from"] - assert call_args["e_to"] == ["to"] - msg = call_args["mime_msg"] - assert msg["Subject"] == "subject" - assert conf.get("smtp", "SMTP_MAIL_FROM") == msg["From"] - assert len(msg.get_payload()) == 2 - filename = f'attachment; filename="{path.name}"' - assert filename == msg.get_payload()[-1].get("Content-Disposition") - mimeapp = MIMEApplication("attachment") - assert mimeapp.get_payload() == msg.get_payload()[-1].get_payload() - - @mock.patch("airflow.utils.email.send_mime_email") - def test_send_smtp_with_multibyte_content(self, mock_send_mime): - email.send_email_smtp("to", "subject", "🔥", mime_charset="utf-8") - assert mock_send_mime.called - _, call_args = mock_send_mime.call_args - msg = call_args["mime_msg"] - mimetext = MIMEText("🔥", "mixed", "utf-8") - assert mimetext.get_payload() == msg.get_payload()[0].get_payload() - - @mock.patch("airflow.utils.email.send_mime_email") - def test_send_bcc_smtp(self, mock_send_mime, tmp_path): - path = tmp_path / "testfile" - path.write_text("attachment") - email.send_email_smtp( - "to", - "subject", - "content", - files=[os.fspath(path)], - cc="cc", - bcc="bcc", - custom_headers={"Reply-To": "[email protected]"}, - ) - assert mock_send_mime.called - _, call_args = mock_send_mime.call_args - assert conf.get("smtp", "SMTP_MAIL_FROM") == call_args["e_from"] - assert call_args["e_to"] == ["to", "cc", "bcc"] - msg = call_args["mime_msg"] - assert msg["Subject"] == "subject" - assert conf.get("smtp", "SMTP_MAIL_FROM") == msg["From"] - assert len(msg.get_payload()) == 2 - assert f'attachment; filename="{path.name}"' == msg.get_payload()[-1].get("Content-Disposition") - mimeapp = MIMEApplication("attachment") - assert mimeapp.get_payload() == msg.get_payload()[-1].get_payload() - assert msg["Reply-To"] == "[email protected]" - - @mock.patch("smtplib.SMTP_SSL") - @mock.patch("smtplib.SMTP") - def test_send_mime_airflow_config(self, mock_smtp, mock_smtp_ssl, monkeypatch): - monkeypatch.delenv("AIRFLOW_CONN_SMTP_DEFAULT", raising=False) - mock_smtp.return_value = mock.Mock() - msg = MIMEMultipart() - email.send_mime_email("from", "to", msg, dryrun=False) - mock_smtp.assert_called_once_with( - host=conf.get("smtp", "SMTP_HOST"), - port=conf.getint("smtp", "SMTP_PORT"), - timeout=conf.getint("smtp", "SMTP_TIMEOUT"), - ) - assert not mock_smtp_ssl.called - assert mock_smtp.return_value.starttls.called - mock_smtp.return_value.sendmail.assert_called_once_with("from", "to", msg.as_string()) - assert mock_smtp.return_value.quit.called - - @mock.patch("smtplib.SMTP") - def test_send_mime_conn_id(self, mock_smtp, monkeypatch): - monkeypatch.setenv( - "AIRFLOW_CONN_SMTP_TEST_CONN", - json.dumps({"conn_type": "smtp", "login": "test-user", "password": "test-p@$$word"}), - ) - msg = MIMEMultipart() - email.send_mime_email("from", "to", msg, dryrun=False, conn_id="smtp_test_conn") - mock_smtp.return_value.login.assert_called_once_with("test-user", "test-p@$$word") - mock_smtp.return_value.sendmail.assert_called_once_with("from", "to", msg.as_string()) - assert mock_smtp.return_value.quit.called - - @mock.patch("smtplib.SMTP_SSL") - @mock.patch("smtplib.SMTP") - def test_send_mime_ssl_none_context(self, mock_smtp, mock_smtp_ssl): - mock_smtp_ssl.return_value = mock.Mock() - with conf_vars({("smtp", "smtp_ssl"): "True", ("email", "ssl_context"): "none"}): - email.send_mime_email("from", "to", MIMEMultipart(), dryrun=False) - assert not mock_smtp.called - mock_smtp_ssl.assert_called_once_with( - host=conf.get("smtp", "SMTP_HOST"), - port=conf.getint("smtp", "SMTP_PORT"), - timeout=conf.getint("smtp", "SMTP_TIMEOUT"), - context=None, - ) - - @mock.patch("smtplib.SMTP_SSL") - @mock.patch("smtplib.SMTP") - @mock.patch("ssl.create_default_context") - def test_send_mime_ssl_default_context_if_not_set(self, create_default_context, mock_smtp, mock_smtp_ssl): - mock_smtp_ssl.return_value = mock.Mock() - with conf_vars({("smtp", "smtp_ssl"): "True"}): - email.send_mime_email("from", "to", MIMEMultipart(), dryrun=False) - assert not mock_smtp.called - assert create_default_context.called - mock_smtp_ssl.assert_called_once_with( - host=conf.get("smtp", "SMTP_HOST"), - port=conf.getint("smtp", "SMTP_PORT"), - timeout=conf.getint("smtp", "SMTP_TIMEOUT"), - context=create_default_context.return_value, - ) - - @mock.patch("smtplib.SMTP_SSL") - @mock.patch("smtplib.SMTP") - @mock.patch("ssl.create_default_context") - def test_send_mime_ssl_default_context_with_value_set_to_default( - self, create_default_context, mock_smtp, mock_smtp_ssl - ): - mock_smtp_ssl.return_value = mock.Mock() - with conf_vars({("smtp", "smtp_ssl"): "True", ("email", "ssl_context"): "default"}): - email.send_mime_email("from", "to", MIMEMultipart(), dryrun=False) - assert not mock_smtp.called - assert create_default_context.called - mock_smtp_ssl.assert_called_once_with( - host=conf.get("smtp", "SMTP_HOST"), - port=conf.getint("smtp", "SMTP_PORT"), - timeout=conf.getint("smtp", "SMTP_TIMEOUT"), - context=create_default_context.return_value, - ) - - @mock.patch("smtplib.SMTP_SSL") - @mock.patch("smtplib.SMTP") - def test_send_mime_noauth(self, mock_smtp, mock_smtp_ssl): - mock_smtp.return_value = mock.Mock() - with conf_vars( - { - ("smtp", "smtp_user"): None, - ("smtp", "smtp_password"): None, - } - ): - email.send_mime_email("from", "to", MIMEMultipart(), dryrun=False) - assert not mock_smtp_ssl.called - mock_smtp.assert_called_once_with( - host=conf.get("smtp", "SMTP_HOST"), - port=conf.getint("smtp", "SMTP_PORT"), - timeout=conf.getint("smtp", "SMTP_TIMEOUT"), - ) - assert not mock_smtp.login.called - - @mock.patch("smtplib.SMTP_SSL") - @mock.patch("smtplib.SMTP") - def test_send_mime_dryrun(self, mock_smtp, mock_smtp_ssl): - email.send_mime_email("from", "to", MIMEMultipart(), dryrun=True) - assert not mock_smtp.called - assert not mock_smtp_ssl.called +class TestDeprecatedSendFromServerProcess: Review Comment: Non-blocking; can come in a follow-up. This test sets `_AIRFLOW_PROCESS_CONTEXT=server`, so it covers the case where the database is still searched. That case did not change here. The test passes on main too. The case that changed is the third branch of `ensure_secrets_backend_loaded`, ie: no `SUPERVISOR_COMMS` and no env var means environment variables only, no MetastoreBackend. A plain script can no longer find a Connection that exists only in the database, and send falls back to no auth. That is what the newsfragment describes at lines 14-16. Can you add a second test with the same fixture, without setenv, asserting that login is never called? Right now, that behaviour is only written down, but we should add a test for it too. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
