ashb commented on code in PR #68700:
URL: https://github.com/apache/airflow/pull/68700#discussion_r3435318971
##########
airflow-core/tests/unit/always/test_secrets_local_filesystem.py:
##########
Review Comment:
I'm not sure this test adds anything -- the secrets backend just provides a
str/json value to the Connection object, all the logic we are testing here
lives inside connection.
I think remove the added secret backend tests
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -153,6 +153,17 @@ def test_get_should_respond_200(self, test_client,
testing_team, session):
assert body["conn_type"] == TEST_CONN_TYPE
assert body["team_name"] == testing_team.name
+ def test_get_should_return_existing_connection_with_invalid_port(self,
test_client, session):
+ session.execute(
+ Connection.__table__.insert().values(conn_id=TEST_CONN_ID,
conn_type=TEST_CONN_TYPE, port=0)
+ )
Review Comment:
Odd way of doing this:
```suggestion
session.add(
Connection(conn_id=TEST_CONN_ID, conn_type=TEST_CONN_TYPE,
port=0)
)
```
is more normal for what we do in Airflow.
##########
airflow-core/tests/unit/models/test_connection.py:
##########
@@ -540,3 +593,18 @@ def test_get_conn_id_to_team_name_mapping(self,
testing_team: Team, session: Ses
"test_conn2": None,
}
clear_db_connections()
+
+ @pytest.mark.db_test
+ def test_existing_connection_with_invalid_port_can_be_loaded(self,
session: Session):
+ clear_db_connections()
+ session.execute(
+
Connection.__table__.insert().values(conn_id="legacy_invalid_port",
conn_type="test", port=0)
Review Comment:
Same here -- `session.add`.
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -219,6 +222,32 @@ def _validate_extra(extra, conn_id) -> None:
raise ValueError(f"Encountered non-JSON in `extra` field for
connection {conn_id!r}.")
return None
+ @staticmethod
+ def _coerce_port(port: int | str | None) -> int | None:
+ if port is None:
+ return None
+ if isinstance(port, bool):
+ raise ValueError(f"Expected integer value for `port`, but got
{port!r} instead.")
+ if isinstance(port, int):
+ return port
+ if isinstance(port, str):
+ try:
+ return int(port)
+ except ValueError:
+ raise ValueError(f"Expected integer value for `port`, but got
{port!r} instead.") from None
+ raise ValueError(f"Expected integer value for `port`, but got {port!r}
instead.")
Review Comment:
Nit: Feels like a text-book example of where structual matching is suited.
```suggestion
match port:
case None:
return None
case bool():
raise ValueError(f"Expected integer value for `port`, but
got {port!r} instead.")
case int():
return port
case str():
try:
return int(port)
except ValueError:
raise ValueError(f"Expected integer value for `port`,
but got {port!r} instead.") from None
case _:
raise ValueError(f"Expected integer value for `port`, but got
{port!r} instead.")
```
##########
airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml:
##########
@@ -12771,6 +12771,8 @@ components:
port:
anyOf:
- type: integer
+ maximum: 65535.0
+ minimum: 1.0
Review Comment:
We have a bit of a problem here .. while this is "correct", by specifying
this in the schema, it is likely that clients generated from it will also
enforce this, and thus reject the invalid ports in any response they receive.
If that is a problem or not is the question (as any such port would have
been "invalid at use" anyway. Needs consideration
##########
airflow-core/tests/unit/models/test_connection.py:
##########
@@ -221,6 +223,57 @@ def test_parse_from_uri(
assert conn.schema == expected_schema
assert conn.extra_dejson == expected_extra_dict
+ @pytest.mark.parametrize(
+ ("port", "expected_port"),
+ [
+ (None, None),
+ (1, 1),
+ ("1", 1),
+ (65535, 65535),
+ ("65535", 65535),
+ ],
+ )
+ def test_connection_accepts_valid_ports(self, port, expected_port):
+ connection = Connection(conn_id="test_conn", conn_type="test",
port=port)
+
+ assert connection.port == expected_port
+
+ @pytest.mark.parametrize("port", [-1, 0, 65536, "0", "65536",
"not-a-port", True])
+ def test_connection_rejects_invalid_ports(self, port):
+ with pytest.raises(ValueError, match="port"):
+ Connection(conn_id="test_conn", conn_type="test", port=port)
+
+ def test_parse_from_uri_rejects_port_zero(self):
+ with pytest.raises(ValueError, match="between 1 and 65535"):
+ Connection(conn_id="test_conn", uri="type://host:0/schema")
+
Review Comment:
Duplicate case already covered in above test
```suggestion
```
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -219,6 +222,32 @@ def _validate_extra(extra, conn_id) -> None:
raise ValueError(f"Encountered non-JSON in `extra` field for
connection {conn_id!r}.")
return None
+ @staticmethod
+ def _coerce_port(port: int | str | None) -> int | None:
+ if port is None:
+ return None
+ if isinstance(port, bool):
+ raise ValueError(f"Expected integer value for `port`, but got
{port!r} instead.")
+ if isinstance(port, int):
+ return port
+ if isinstance(port, str):
+ try:
+ return int(port)
+ except ValueError:
+ raise ValueError(f"Expected integer value for `port`, but got
{port!r} instead.") from None
Review Comment:
How do you hit this str case?
--
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]
