Skip to content

Validate connection ports in core model - #74461

Merged
potiuk merged 6 commits into
apache:mainfrom
MuhammadMaazA:fix-connection-port-core
Oct 19, 2026
Merged

potiuk merged 6 commits into
apache:mainfrom
MuhammadMaazA:fix-connection-port-core

Conversation

@MuhammadMaazA

@MuhammadMaazA MuhammadMaazA commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Part of the split implementation for #68382.\n\nAdds range and type validation for Connection.port at the core model boundary, including JSON and assignment coverage. This PR intentionally contains only the core model and its tests; it does not close the issue by itself.

@MuhammadMaazA
MuhammadMaazA force-pushed the fix-connection-port-core branch from 3b20222 to ab0c501 Compare October 8, 2026 12:43

@fat-catTW fat-catTW left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

I think this needs one more change because the stricter core validation regresses the existing CLI path.

--conn-port is currently parsed as a string, and connections_add() passes args.conn_port directly into Connection(...). With this PR, Connection(port="9083") now raises ValueError, so the existing airflow connections add ... --conn-port=9083 path breaks.

I confirmed this with the existing CLI test:

airflow-core/tests/unit/cli/commands/test_connection_command.py::TestCliAddConnections::test_cli_connection_add[individual-parts]

It fails with:

ValueError: Expected integer value for port, but got '9083' instead.

Could you coerce/validate the CLI value before constructing Connection? For example, parse ARG_CONN_PORT as int, or otherwise reuse the same port coercion logic before passing it to the model.

@MuhammadMaazA

Copy link
Copy Markdown
Contributor Author

Correction: fixed in commit abd1b2c. The --conn-port CLI argument is now parsed as an integer before constructing Connection, so the existing airflow connections add ... --conn-port=9083 path no longer passes a string into the stricter core validation.

Comment thread airflow-core/src/airflow/models/connection.py Outdated
MuhammadMaazA and others added 4 commits October 18, 2026 16:28
Rejecting them would break callers that pass the port as a string (env vars, config values, custom secrets backends); from_json already converts them.

Generated-by: Claude Opus 5

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I pushed a small fixup on top: @validates("port") now converts string ports the same way from_json does (via _coerce_port), so Connection(port="5432") and assignments like conn.port = "5432" keep working for callers that read the port from env vars, config or a custom secrets backend. Out-of-range values and non-numeric strings are still rejected. The tests now cover the accepted strings ("123", "0", "") next to the rejected ones ("abc", "65536"). With that, this stays a validation improvement rather than an API break, so no newsfragment is needed. Will merge once CI is green.


Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit 923a096 into apache:main Oct 19, 2026
149 of 150 checks passed
@boring-cyborg

boring-cyborg Bot commented Oct 19, 2026

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

potiuk added a commit that referenced this pull request Oct 19, 2026
…5119)

Core Connection now validates the port when it is set (#74461), so the
test failed while building the connection, before the hook's own
beeline-parameter check, which still guards older Airflow cores, could
run.

Generated-by: Claude Opus 5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants