ColtenOuO commented on code in PR #68700:
URL: https://github.com/apache/airflow/pull/68700#discussion_r3863779819
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -187,7 +190,7 @@ def __init__(
self.login = login
self.password = password
self.schema = schema
- self.port = port
+ self.port = self._normalize_port(port) if _validate_port else
self._coerce_port(port)
Review Comment:
Would it be safer to enforce this invariant on attribute assignment rather
than only during initialization?
As currently implemented, validation can be bypassed after construction:
```python
connection = Connection(conn_id="test", port=5432)
connection.port = 0
```
This could allow an invalid port to be persisted later and makes the
invariant easier to accidentally break during future maintenance.
Could we use SQLAlchemy's `@validates("port")`, or equivalent
attribute-level validation, so every normal assignment goes through
_normalize_port()?
It would also be useful to add a regression test covering assignment after
construction, for example:
```python
connection = Connection(conn_id="test", port=5432)
with pytest.raises(ValueError, match="port"):
connection.port = 0
assert connection.port == 5432
```
This would ensure an invalid port fails immediately instead of being
discovered at flush time or when the connection is eventually used.
--
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]
