rjgoyln commented on code in PR #73780:
URL: https://github.com/apache/airflow/pull/73780#discussion_r4115425942
##########
providers/smtp/src/airflow/providers/smtp/hooks/smtp.py:
##########
@@ -46,6 +46,19 @@
from airflow.providers.common.compat.sdk import Connection
+def _get_bool_extra(extra: dict[str, Any], key: str) -> bool:
+ """
+ Read a boolean connection extra that defaults to ``False``.
+
+ Extras parsed from a connection URI (e.g.
``smtp://host?disable_tls=false``) are strings,
Review Comment:
Small correction to the description: `FTPHook` doesn't parse `passive`
either — it passes the raw value to `ftplib.set_pasv()`, so `passive=false`
from a URI is truthy there too. The closer in-tree precedents are
mongo/ssh/sftp, which explicitly parse the string value. FTP looks like the
same latent bug if someone wants to follow up.
##########
providers/smtp/tests/unit/smtp/hooks/test_smtp.py:
##########
@@ -374,6 +374,28 @@ def test_send_mime_nossl(self, mock_smtp, mock_smtp_ssl):
assert not mock_smtp_ssl.called
mock_smtp.assert_called_once_with(host=SMTP_HOST, port=NONSSL_PORT,
timeout=DEFAULT_TIMEOUT)
+ @pytest.mark.parametrize(
+ ("query", "expected_ssl", "expected_starttls"),
+ [
+ ("", True, True),
+ ("disable_ssl=true&disable_tls=true", False, False),
+ ("disable_ssl=false&disable_tls=false", True, True),
+ ("disable_ssl=False&disable_tls=0", True, True),
+ ("disable_ssl=1&disable_tls=false", False, True),
Review Comment:
Nothing here pins the behavior for unrecognized values. For example,
disable_tls=ture currently falls back to False, keeping STARTTLS enabled. Worth
adding one case if this fail-safe behavior is intentional.
```suggestion
("disable_ssl=1&disable_tls=false", False, True),
("disable_ssl=nope&disable_tls=ture", True, True),
```
##########
providers/smtp/src/airflow/providers/smtp/hooks/smtp.py:
##########
@@ -46,6 +46,19 @@
from airflow.providers.common.compat.sdk import Connection
+def _get_bool_extra(extra: dict[str, Any], key: str) -> bool:
+ """
+ Read a boolean connection extra that defaults to ``False``.
+
+ Extras parsed from a connection URI (e.g.
``smtp://host?disable_tls=false``) are strings,
+ and a non-empty string such as ``"false"`` is truthy.
+ """
+ value = extra.get(key, False)
+ if isinstance(value, str):
+ return value.strip().lower() in ("true", "1", "yes", "on")
Review Comment:
There is one — `airflow.utils.strings.to_boolean`, used by Hashicorp,
OpenSearch, and Snowflake. Its `TRUE_LIKE_VALUES` also includes `t` and `y`, so
`disable_tls=t` currently reads as False here but True there.
I think a local parser is still the right choice: `to_boolean` doesn't
accept real bools directly, and `airflow.utils` isn't part of the documented
public interface. I'd just match its accepted values so the same extra doesn't
mean different things across providers.
```suggestion
return value.strip().lower() in ("on", "t", "true", "y", "yes", "1")
```
--
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]
