Revanth14 commented on PR #68700:
URL: https://github.com/apache/airflow/pull/68700#issuecomment-4753497142
> I'm not sure that `Connection(port="1024")` is "worth" supporting.
>
> The cli already converts `--conn-port` to an integer in this change, and I
don't think anyone creating a port with a string is "valid"? Not sure, there's
maybe arguments to be said we should continue supporting it from string for
back compat but I'm not sure how valid or widespread a use case that is.
That's fair. I kept string coercion because this PR is trying to add range
validation without also tightening the accepted input-type contract.
Today `Connection(port="1024")` works, and `from_json`/secrets-style paths
can naturally carry JSON/string values before normalization e.g. a
Vault/secrets entry stored as `{"conn_type": "mysql", "port": "5432"}`, which
the previous `from_json` already coerced via `int(port)`. The CLI/API now pass
ints, but direct construction and deserialization are still public-ish
surfaces, so accepting numeric strings keeps this change backward compatible
while still rejecting bools, non-integer strings, and out-of-range values.
That said, I don’t feel strongly about preserving it if you’d rather make
the boundary stricter here. I mainly wanted to avoid sneaking an input-type
behavior change into a range-validation PR.
Happy to modify or adjust this if you think stricter boundary is a better
trade-off.
--
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]
