Fix VaultBackend.get_connection() breaking PythonVirtualenvOperator in Airflow 3 by seanmuth · Pull Request #68305 · apache/airflow

seanmuth · 2026-06-09T19:03:08Z

VaultBackend.get_connection() constructed the returned Connection using
airflow.models.connection.Connection — the SQLAlchemy ORM model. In Airflow 3,
instantiating this model triggers lazy mapper initialisation for the entire
Airflow model registry. In PythonVirtualenvOperator subprocesses (and other
isolated task contexts), DagModel has not been imported when the secrets
backend is consulted, so DagScheduleAssetNameReference cannot resolve its
'DagModel' relationship string, raising:

sqlalchemy.exc.InvalidRequestError: One or more mappers failed to initialize …
Triggering mapper: 'mapped class DagScheduleAssetNameReference->dag_schedule_asset_name_reference'.
Original exception was: … expression 'DagModel' failed to locate a name ('DagModel').

Because context.py's secrets-backend loop catches all exceptions at DEBUG
level (without exc_info), this error is silently swallowed. The connection
appears undefined even when Vault returns a valid 200 response with the
correct data.

Fix: switch to airflow.providers.common.compat.sdk.Connection, which
resolves to the Pydantic SDK Connection in Airflow 3 (no SQLAlchemy mapper
involvement) and falls back to the SQLAlchemy model in Airflow 2. For the
URI path, use Connection.from_uri() guarded by AIRFLOW_V_3_0_PLUS to
handle both Airflow versions correctly.

Two new regression tests verify that get_connection() returns an instance of
the SDK Connection type rather than the SQLAlchemy ORM model.

Was generative AI tooling used to co-author this PR?

Yes — Claude Code (Sonnet 4.6)

Generated-by: Claude Code (Sonnet 4.6) following the guidelines

Important

🛠️ Maintainer triage note for @seanmuth · by @potiuk · 2026-06-17 18:53 UTC

Some review feedback from kaxil is waiting on you:

Please respond to the open review thread(s) — reply or push a fix in each, then mark them resolved.
See the Pull Request quality criteria.

The ball is in your court — you've been assigned to this PR. Reply or push a fix in each thread, then mark them resolved.

_{Automated triage — may be imperfect; a maintainer takes the next look.}

seanmuth · 2026-06-09T20:56:46Z

Force-pushed twice to update this branch after rebasing onto main. Both times the pre-push hook ran against all files that diverged between the remote and local tip (including upstream commits pulled in by the rebase), hitting pre-existing failures in unrelated files (providers/common/ai, etc.) — not caused by changes in this PR.

Drafted-by: Claude Code (Sonnet 4.6); reviewed by @seanmuth before posting

amoghrajesh

One comment, otherwise the root case is on point and the compat sdk swap is the right fix. Preemptive approval once kaxil's concerns are handled

…ualenv tasks Constructing airflow.models.connection.Connection (SQLAlchemy ORM) triggers lazy mapper initialisation for the entire Airflow model registry. In PythonVirtualenvOperator subprocesses, DagModel has not been imported so DagScheduleAssetNameReference cannot resolve its 'DagModel' relationship, raising sqlalchemy.exc.InvalidRequestError. This exception is silently swallowed in context.py's secrets-backend loop, making the connection appear undefined even when Vault returns a valid 200 response. Switch to airflow.providers.common.compat.sdk.Connection (Pydantic in Airflow 3, SQLAlchemy in Airflow 2) which does not trigger mapper init. Also update call sites to use keyword-only conn_id= to match the SDK Connection's constructor signature.

…ri= calls from_uri is a defined classmethod on airflow.sdk.Connection, and the uri= overload is visible via the manual __init__ overloads — neither suppression is needed.

Pydantic or future attrs validators can raise ValueError; catching only TypeError would let those slip through silently the same way this PR fixes the original silent failure.

seanmuth · 2026-06-12T16:53:34Z

thought we fixed that test already, looking

The attrs mypy plugin overrides the manually-written __init__ overloads, so mypy sees only the attrs-generated __init__ (no uri= param). The ignore is required on the Airflow 2 / pre-3.2 fallback path.

amoghrajesh

Requesting changes to avoid accidental merge, @kaxil's suggestion is right on this one. We can merge once that's done.

@seanmuth pls validate if the fix works practically too, theory wise it seems perfect.

potiuk · 2026-06-25T10:29:48Z

@seanmuth — this PR has 2 unresolved review thread(s) that still look like they need your attention. Once you've addressed them (push changes and/or reply in-thread), please resolve the threads or reply to confirm, and give the reviewer a nudge for another look. Thanks!

See the PR quality criteria.

_{Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated)}

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

seanmuth · 2026-06-26T15:05:34Z

Requesting changes to avoid accidental merge, @kaxil's suggestion is right on this one. We can merge once that's done.

@seanmuth pls validate if the fix works practically too, theory wise it seems perfect.

On it now, last week was an on-call week for me, always causes stuff to fall of my plate.

Will also add a quick confirmation that this works the way we're expecting it does 🫡

Replace the get_connection() override (which hard-coded the compat SDK Connection) with get_conn_value(), returning a URI string for conn_uri secrets or a JSON-serialized field dict otherwise. The base-class get_connection() then deserializes using _get_connection_class(), which the framework populates with the ORM Connection on the server and the SDK Connection in workers — fixing the mapper-init regression without breaking server-side commands like airflow connections get. Tests call _set_connection_class(SdkConnection) to simulate the framework injection, matching how initialize_secrets_backends() wires things up at runtime.

_set_connection_class was added after 3.0/2.11; guard test calls with hasattr and skip the two isinstance(SdkConnection) tests on older Airflow where class injection is unavailable.

…ersions On Airflow 3.0/2.11, BaseSecretsBackend.get_connection() did not accept team_name. Without a VaultBackend override, calling get_connection with team_name raised TypeError. The override delegates to get_conn_value() then uses deserialize_connection() when available (modern Airflow, class injection works), and falls back to the compat SDK Connection on older Airflow that predates _get_connection_class / deserialize_connection.

amoghrajesh

Sorry for the back and forth, @seanmuth. I guess we could just stick to what @kaxil mentioned the keep the diff manageable and scoped here, I think the diff with his suggestion would be close to:

def get_conn_value(self, conn_id: str, team_name: str | None = None) -> str | None:
    response = self._get_team_or_global_secret(self.connections_path, team_name, conn_id)
    if response is None:
        return None
    uri = response.get("conn_uri")
    if uri:
        return uri
    return json.dumps(response)

Return conn_uri verbatim or json.dumps(secret_dict) from get_conn_value() so the base-class get_connection() deserializes using the Connection class injected per execution context — fixing the mapper-init regression in workers without regressing server-side CLI commands. Tests assert on get_conn_value() string output directly, removing the _set_connection_class injection pattern. Add both conn_uri and field-based JWT auth tests to cover URI secrets.

kaxil

A couple of comments & PR title revision, then it looks good

amoghrajesh

LGTM apart from the open comments.

Hoist the repeated in-test json import to module level and qualify the get_conn_value docstring so the framework-injected Connection class behavior is scoped to Airflow 3.2+, where per-process injection exists (2.11/3.0/3.1 still import the ORM Connection directly).

seanmuth · 2026-07-08T21:30:23Z

I think I've addressed everything in the comments? Ready for another pass @kaxil @amoghrajesh

amoghrajesh

All prior review feedback has been addressed, the get_conn_value() rework correctly delegates to _get_connection_class() injection, and the docstring caveat is in place and correct too. LGTM.

seanmuth requested a review from hussein-awala as a code owner June 9, 2026 19:03

boring-cyborg Bot added area:providers area:secrets provider:hashicorp Hashicorp provider related issues labels Jun 9, 2026

seanmuth force-pushed the fix-vault-connection-sdk-compat branch from 380aef2 to ba7707d Compare June 9, 2026 19:17

kaxil reviewed Jun 9, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

kaxil requested a review from amoghrajesh June 9, 2026 20:18

seanmuth force-pushed the fix-vault-connection-sdk-compat branch from ba7707d to 105c7b2 Compare June 9, 2026 20:31

seanmuth force-pushed the fix-vault-connection-sdk-compat branch from 105c7b2 to 39285a0 Compare June 9, 2026 21:15

kaxil reviewed Jun 9, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

kaxil approved these changes Jun 9, 2026

View reviewed changes

kaxil reviewed Jun 9, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

amoghrajesh approved these changes Jun 10, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

seanmuth added 3 commits June 11, 2026 00:53

Remove unnecessary type: ignore comments on Connection.from_uri and u…

53250e3

…ri= calls from_uri is a defined classmethod on airflow.sdk.Connection, and the uri= overload is visible via the manual __init__ overloads — neither suppression is needed.

Widen except clause to catch ValueError in addition to TypeError

8e682f9

Pydantic or future attrs validators can raise ValueError; catching only TypeError would let those slip through silently the same way this PR fixes the original silent failure.

kaxil force-pushed the fix-vault-connection-sdk-compat branch from 9c4918b to 8e682f9 Compare June 10, 2026 23:53

potiuk assigned seanmuth Jun 12, 2026

Merge branch 'main' into fix-vault-connection-sdk-compat

5a14ded

Restore type: ignore[call-arg] on Connection(uri=) fallback

e03b429

The attrs mypy plugin overrides the manually-written __init__ overloads, so mypy sees only the attrs-generated __init__ (no uri= param). The ignore is required on the Airflow 2 / pre-3.2 fallback path.

kaxil reviewed Jun 15, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

amoghrajesh requested changes Jun 18, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

seanmuth added 3 commits June 26, 2026 10:20

Guard _set_connection_class calls for Airflow 3.0/2.11 compat

d35144c

_set_connection_class was added after 3.0/2.11; guard test calls with hasattr and skip the two isinstance(SdkConnection) tests on older Airflow where class injection is unavailable.

kaxil reviewed Jun 26, 2026

View reviewed changes

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py Outdated

Comment thread providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py Outdated

amoghrajesh reviewed Jun 29, 2026

View reviewed changes

kaxil reviewed Jun 30, 2026

View reviewed changes

Comment thread providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py Outdated

Comment thread providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py

kaxil approved these changes Jun 30, 2026

View reviewed changes

amoghrajesh approved these changes Jul 1, 2026

View reviewed changes

amoghrajesh approved these changes Jul 14, 2026

View reviewed changes

amoghrajesh merged commit 4112a4a into apache:main Jul 14, 2026
79 checks passed

Uh oh!

Conversation

seanmuth commented Jun 9, 2026 • edited by potiuk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Was generative AI tooling used to co-author this PR?

Uh oh!

Uh oh!

Uh oh!

seanmuth commented Jun 9, 2026

Uh oh!

Uh oh!

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanmuth commented Jun 12, 2026

Uh oh!

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

potiuk commented Jun 25, 2026

Uh oh!

seanmuth commented Jun 26, 2026

Uh oh!

Uh oh!

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

seanmuth commented Jul 8, 2026

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seanmuth commented Jun 9, 2026 •

edited by potiuk

Loading