rjgoyln commented on code in PR #73778:
URL: https://github.com/apache/airflow/pull/73778#discussion_r4115139894
##########
providers/ftp/src/airflow/providers/ftp/hooks/ftp.py:
##########
@@ -318,20 +318,18 @@ def get_conn(self) -> ftplib.FTP:
encoding = params.extra_dejson.get("encoding")
self.encoding = encoding
- if params.port:
- ftplib.FTP_TLS.port = params.port
-
# Construct FTP_TLS instance with SSL context to allow
certificates to be validated by default
context = ssl.create_default_context()
- params.host = cast("str", params.host)
- params.password = cast("str", params.password)
- params.login = cast("str", params.login)
if encoding:
- self.conn = ftplib.FTP_TLS(
- params.host, params.login, params.password,
context=context, encoding=encoding
- ) # nosec: B321
+ self.conn = ftplib.FTP_TLS(context=context, encoding=encoding)
# nosec: B321
else:
- self.conn = ftplib.FTP_TLS(params.host, params.login,
params.password, context=context) # nosec: B321
+ self.conn = ftplib.FTP_TLS(context=context) # nosec: B321
+ if params.host:
Review Comment:
Now that the FTPS path calls `connect()` itself, it is the only one of the
two hooks that connects silently — `FTPHook.get_conn` logs `Connecting via FTP
to %s:%d` at the equivalent point. Worth the same line here: the port an FTPS
connection actually dialled is precisely what this bug made impossible to tell
from the logs.
##########
providers/ftp/tests/unit/ftp/hooks/test_ftp.py:
##########
@@ -262,6 +263,26 @@ def test_ftp_custom_port_and_login(self, mock_ftp):
conn.login.assert_called_once_with("user", "pass123")
conn.set_pasv.assert_called_once_with(True)
+ @mock.patch("ftplib.FTP_TLS.connect")
+ @mock.patch("ftplib.FTP_TLS.login")
+ @mock.patch("ftplib.FTP_TLS.set_pasv")
+ @mock.patch("ftplib.FTP_TLS.prot_p")
+ def test_ftps_custom_port_and_login(self, mock_prot_p, mock_set_pasv,
mock_login, mock_connect):
+ from airflow.providers.ftp.hooks.ftp import FTPSHook
+
+ FTPSHook("ftp_custom_port_and_login").get_conn()
+
+ mock_connect.assert_called_once_with("localhost", 10000)
+ mock_login.assert_called_once_with("user", "pass123")
+ # The port must not leak into the default port of other FTP_TLS
connections
+ assert ftplib.FTP_TLS.port == ftplib.FTP_PORT
+
+ mock_connect.reset_mock()
+ FTPSHook("ftp_passive").get_conn()
+
+ mock_connect.assert_called_once_with("localhost", 0)
+ mock_login.assert_called_once()
Review Comment:
This last assertion is still matching the *first* hook's call, not the
second one. `ftp_passive` has no login, so `FTPSHook("ftp_passive").get_conn()`
never reaches `self.conn.login(...)`; printing the mock right here gives `1
[call('user', 'pass123')]` — the same call line 276 already checked. It reads
as "the second connection logged in too" while asserting nothing about the
second connection, so I'd drop it (or `mock_login.reset_mock()` before the
second `get_conn()` and assert `assert_not_called()`, if the intent was to pin
the credential-less path).
```suggestion
mock_connect.assert_called_once_with("localhost", 0)
```
##########
providers/ftp/src/airflow/providers/ftp/hooks/ftp.py:
##########
@@ -318,20 +318,18 @@ def get_conn(self) -> ftplib.FTP:
encoding = params.extra_dejson.get("encoding")
self.encoding = encoding
- if params.port:
- ftplib.FTP_TLS.port = params.port
-
# Construct FTP_TLS instance with SSL context to allow
certificates to be validated by default
context = ssl.create_default_context()
- params.host = cast("str", params.host)
- params.password = cast("str", params.password)
- params.login = cast("str", params.login)
if encoding:
- self.conn = ftplib.FTP_TLS(
- params.host, params.login, params.password,
context=context, encoding=encoding
- ) # nosec: B321
+ self.conn = ftplib.FTP_TLS(context=context, encoding=encoding)
# nosec: B321
else:
- self.conn = ftplib.FTP_TLS(params.host, params.login,
params.password, context=context) # nosec: B321
+ self.conn = ftplib.FTP_TLS(context=context) # nosec: B321
+ if params.host:
+ # Pass the port to connect() rather than setting FTP_TLS.port,
which would
+ # change the default port of every FTP_TLS connection in the
process.
+ self.conn.connect(params.host, params.port or 0)
Review Comment:
`or 0` is correct — `connect()` does `if port > 0: self.port = port`, so 0
leaves the instance on ftplib's 21 — but it reads as "connect to port 0", and
the test then has to assert `connect("localhost", 0)` for what is really the
default port. `ftplib.FTP_PORT` states it, and matches `FTPHook.get_conn` a few
lines up, which already spells the fallback out as `port: int =
int(ftplib.FTP_PORT)`.
```suggestion
self.conn.connect(params.host, params.port or
ftplib.FTP_PORT)
```
--
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]
