Repository navigation
Warn at provider discovery about an unresolvable connection type - #72904
Merged
Merged
Conversation
A hook registers under the connection-type string verbatim, while a connection read from a URI, from JSON, or rebuilt from the secrets cache presents the decoded form of it: get_uri() lowercases the type and encodes '_' as '-', because RFC 3986 forbids '_' in a URI scheme, and reading a connection back decodes it again. Where the declaration and the decoded name differ, the hook is unreachable for those connections. A connection created through the UI, the REST API or the CLI keeps the declared spelling and does resolve, which is what lets the mismatch go unnoticed until a connection is served from somewhere else. Where two providers declare two spellings of one name it is worse than unreachable. A connection for either resolves whichever hook holds the decoded name, so it can be given the other provider's hook, and because that resolves rather than failing there is no error anywhere to carry the explanation. Discovery is the only place either can be reported. The authoring schema validates the providers in this repository alone, so a third-party distribution never passes through it, and Connection.get_hook() is reached only once a lookup has already failed, which the collision never does. Discovery already warns there about a connection type registered twice, so this is that check against the decoded names. The decoded name comes from Connection._normalize_conn_type applied to the parsed scheme, rather than from restating the separator rule, so it follows the aliases that decoder applies and reports a type a URI cannot carry at all. A test asserts it against a real get_uri/from_uri round trip so the two cannot drift. The existing inconsistency test declares 'wrong-connection-type', which is read back under another name in its own right, so it now asserts both warnings rather than a single one.
kaxil
marked this pull request as ready for review
September 10, 2026 22:16
kaxil
force-pushed
the
warn-unreachable-connection-types
branch
from
September 10, 2026 22:16
c5616ba to
3c1254f
Compare
jason810496
reviewed
Sep 14, 2026
jason810496
left a comment
Member
There was a problem hiding this comment.
Thanks for the fix.
No concern from manually review in the first place.
Here're some minor nits pointed out by agents that makes sense to me.
All three warnings now carry connection_types/packages, so a query keyed on those fields matches the single-declaration case too. The single-group skip states its invariant directly rather than leaving it to be derived from the keys being distinct. In the tests, the entry check in test_warning_logs_generated now pins one known pattern per entry, the two-provider case asserts the package/spelling pairing rather than sorting it away, and the round-trip test's Connection import moves to the top of the file. There is no cycle there; the one in the production method is a real cycle and stays. Also narrows the docstring's claim about the CLI: connections add --conn-uri builds Connection(uri=...) and --conn-json goes through from_json, both of which decode the type, so only --conn-type keeps the declared spelling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A hook registers under the
connection-typestring verbatim. A connection read from a URI, from JSON, or rebuilt from the secrets cache presents the decoded form of that string:get_uri()lowercases the type and encodes_as-, because RFC 3986 forbids_in a URI scheme, and reading a connection back decodes it again.Where the declaration and the decoded name differ, the hook is unreachable for every such connection. A connection created through the UI, the REST API or
connections add --conn-typekeeps the declared spelling and does resolve, which is what lets the mismatch go unnoticed until a connection is served from somewhere else. That is #72316.Provider discovery now says so once, naming the package that declared it.
The case that never fails
Where two providers declare the two spellings of one name, a connection for either resolves whichever hook holds the decoded name:
Neither call raises. The connection is handed the other provider's hook, and because nothing fails there is no error anywhere that could carry the explanation.
Why discovery rather than somewhere else
The authoring schema validates the providers in this repository alone, so a third-party distribution shipping
get_provider_info()never passes through it. The runtimeprovider_info.schema.jsonis deliberately lenient so installs of already-released providers keep working, which is why #72854 left it alone and #71104 declined the same tightening.Connection.get_hook()is reached only after a lookup has already failed, so it cannot reach the collision above, which resolves.Discovery sees every declared type together with the package that declared it, and it already warns there when one connection type is registered twice. This is that check against the decoded names rather than the literal ones.
What it logs
Design rationale
The decoded name is asked of the decoder,
Connection._normalize_conn_typeapplied to the parsed scheme, rather than restated as a separator swap. Restating it misses what that decoder actually does:postgresqlis aliased topostgres, so a provider declaringpostgresqlis unreachable while a hand-rolled rule calls it fine, and a type that cannot be a URI scheme at all, such as one containing a space, is lost rather than re-spelled and needs its own message. A test asserts the derived name against a realget_uri/from_uriround trip, so the two cannot drift apart.The import is inside the function because
airflow.sdk.definitions.connectionimports this module at module level, following the commented precedent already in this file.Note on log style
These warnings pass structlog keyword arguments rather than the
%spositional arguments used by the neighbouring warnings in this file. A positional argument is interpolated into the message and then dropped, so the connection type and the package name would exist only inside the rendered string. As keyword arguments they stay separate fields in the JSON event, which is what makes them queryable and what makes one field schema across the three messages worth having.Related
#72855 improves what
Connection.get_hook()says when a lookup fails. It is independent of this change and touches different files; this one covers the case that does not fail.