From 2e523d6c2356bcd4e79baeef1cbb83e73fe08240 Mon Sep 17 00:00:00 2001 From: henry3260 Date: Mon, 13 Jul 2026 00:49:09 +0800 Subject: [PATCH 1/3] Reject connection secrets without a connection type Prevent malformed JSON secrets from being reported as missing connections at runtime. --- .../airflow_shared/secrets_backend/base.py | 6 ++++ .../tests/secrets_backend/test_base.py | 33 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py b/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py index e61197d2baa7a..3106989a2ea7f 100644 --- a/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py +++ b/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py @@ -17,6 +17,7 @@ from __future__ import annotations import inspect +import json from abc import ABC from collections.abc import Callable @@ -122,6 +123,11 @@ def _get_connection_class(self) -> type: def _deserialize_connection_value(conn_class: type, conn_id: str, value: str): value = value.strip() if value[0] == "{": + connection_data = json.loads(value) + conn_type = connection_data.get("conn_type") + uri = connection_data.get("uri") + if not (isinstance(conn_type, str) and conn_type.strip() or isinstance(uri, str) and uri.strip()): + raise ValueError("Connection secret JSON must include a non-empty 'conn_type' or 'uri'.") return conn_class.from_json(value=value, conn_id=conn_id) # type: ignore[attr-defined] # TODO: Only sdk has from_uri defined on it. Is it worthwhile developing the core path or not? diff --git a/shared/secrets_backend/tests/secrets_backend/test_base.py b/shared/secrets_backend/tests/secrets_backend/test_base.py index e14e620424392..d13a8dfc68971 100644 --- a/shared/secrets_backend/tests/secrets_backend/test_base.py +++ b/shared/secrets_backend/tests/secrets_backend/test_base.py @@ -124,6 +124,39 @@ def test_deserialize_connection_json(self, sample_conn_json): assert conn.conn_id == "test_conn" assert conn._kwargs["conn_type"] == "mysql" + @pytest.mark.parametrize( + "value", + [ + '{"host": "example.com"}', + '{"conn_type": null}', + '{"conn_type": ""}', + '{"conn_type": " "}', + ], + ) + def test_deserialize_connection_json_requires_conn_type(self, value): + backend = _TestBackend() + backend._set_connection_class(MockConnection) + + with pytest.raises( + ValueError, match="Connection secret JSON must include a non-empty 'conn_type' or 'uri'." + ): + backend.deserialize_connection("test_conn", value) + + @pytest.mark.parametrize( + "value", + [ + '{"uri": "http://example.com"}', + '{"conn_type": null, "uri": "http://example.com"}', + ], + ) + def test_deserialize_connection_json_with_uri_does_not_require_conn_type(self, value): + backend = _TestBackend() + backend._set_connection_class(MockConnection) + + conn = backend.deserialize_connection("test_conn", value) + + assert conn.uri == "http://example.com" + def test_deserialize_connection_uri(self, sample_conn_uri): """Test deserialize_connection with URI format through _TestBackend.""" backend = _TestBackend() From 6ba6ad82937d39fc756a44223094f53869f3cd7b Mon Sep 17 00:00:00 2001 From: henry3260 Date: Wed, 22 Jul 2026 17:22:01 +0800 Subject: [PATCH 2/3] Keep Airflow 2-era connection secrets working and document why Review on the PR pointed out that rejecting JSON connection secrets without a conn_type would regress the Airflow 2 -> 3 migration compatibility deliberately established in #61728: secrets stored by Airflow 2-era backends (e.g. AWS Secrets Manager) commonly omit both conn_type and uri, and the worker-local secrets backend path must keep resolving them. Replace the rejection with a comment at the deserialization site and a regression test, so the compatibility guarantee is visible to future readers and enforced by CI. Co-Authored-By: Claude Fable 5 --- .../airflow_shared/secrets_backend/base.py | 12 +++--- .../tests/secrets_backend/test_base.py | 39 ++++++------------- 2 files changed, 17 insertions(+), 34 deletions(-) diff --git a/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py b/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py index 3106989a2ea7f..0e785f968177d 100644 --- a/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py +++ b/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py @@ -17,7 +17,6 @@ from __future__ import annotations import inspect -import json from abc import ABC from collections.abc import Callable @@ -123,11 +122,12 @@ def _get_connection_class(self) -> type: def _deserialize_connection_value(conn_class: type, conn_id: str, value: str): value = value.strip() if value[0] == "{": - connection_data = json.loads(value) - conn_type = connection_data.get("conn_type") - uri = connection_data.get("uri") - if not (isinstance(conn_type, str) and conn_type.strip() or isinstance(uri, str) and uri.strip()): - raise ValueError("Connection secret JSON must include a non-empty 'conn_type' or 'uri'.") + # JSON secrets stored by Airflow 2-era backends may lack both "conn_type" + # and "uri" (e.g. {"host": ..., "login": ..., "password": ...}). This is + # valid: conn_type is intentionally optional on the SDK Connection model + # for Airflow 2 -> 3 migration compatibility. Do not add validation here + # that rejects such secrets; see + # https://github.com/apache/airflow/pull/61728 return conn_class.from_json(value=value, conn_id=conn_id) # type: ignore[attr-defined] # TODO: Only sdk has from_uri defined on it. Is it worthwhile developing the core path or not? diff --git a/shared/secrets_backend/tests/secrets_backend/test_base.py b/shared/secrets_backend/tests/secrets_backend/test_base.py index d13a8dfc68971..79b413720ba9b 100644 --- a/shared/secrets_backend/tests/secrets_backend/test_base.py +++ b/shared/secrets_backend/tests/secrets_backend/test_base.py @@ -124,38 +124,21 @@ def test_deserialize_connection_json(self, sample_conn_json): assert conn.conn_id == "test_conn" assert conn._kwargs["conn_type"] == "mysql" - @pytest.mark.parametrize( - "value", - [ - '{"host": "example.com"}', - '{"conn_type": null}', - '{"conn_type": ""}', - '{"conn_type": " "}', - ], - ) - def test_deserialize_connection_json_requires_conn_type(self, value): - backend = _TestBackend() - backend._set_connection_class(MockConnection) - - with pytest.raises( - ValueError, match="Connection secret JSON must include a non-empty 'conn_type' or 'uri'." - ): - backend.deserialize_connection("test_conn", value) + def test_deserialize_connection_json_without_conn_type(self): + """Airflow 2-era JSON secrets without conn_type or uri must keep deserializing. - @pytest.mark.parametrize( - "value", - [ - '{"uri": "http://example.com"}', - '{"conn_type": null, "uri": "http://example.com"}', - ], - ) - def test_deserialize_connection_json_with_uri_does_not_require_conn_type(self, value): + Guards the Airflow 2 -> 3 migration compatibility established in + https://github.com/apache/airflow/pull/61728. + """ backend = _TestBackend() backend._set_connection_class(MockConnection) - conn = backend.deserialize_connection("test_conn", value) - - assert conn.uri == "http://example.com" + conn = backend.deserialize_connection( + "test_conn", '{"host": "example.com", "login": "admin", "password": "secret"}' + ) + assert isinstance(conn, MockConnection) + assert conn.conn_id == "test_conn" + assert conn._kwargs == {"host": "example.com", "login": "admin", "password": "secret"} def test_deserialize_connection_uri(self, sample_conn_uri): """Test deserialize_connection with URI format through _TestBackend.""" From 5455062a3dcecd13b67b899bc6a3019cacc37437 Mon Sep 17 00:00:00 2001 From: henry3260 Date: Thu, 23 Jul 2026 15:31:55 +0800 Subject: [PATCH 3/3] Simplify Airflow 2 compatibility comments and add removal TODO Apply review suggestions: tighten the migration-compatibility comment and test docstring, and mark the compatibility path for removal once the minimum supported Airflow version in providers is 3.0. --- .../src/airflow_shared/secrets_backend/base.py | 9 +++++---- .../secrets_backend/tests/secrets_backend/test_base.py | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py b/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py index 0e785f968177d..77c08be45f1bd 100644 --- a/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py +++ b/shared/secrets_backend/src/airflow_shared/secrets_backend/base.py @@ -122,12 +122,13 @@ def _get_connection_class(self) -> type: def _deserialize_connection_value(conn_class: type, conn_id: str, value: str): value = value.strip() if value[0] == "{": - # JSON secrets stored by Airflow 2-era backends may lack both "conn_type" + # JSON secrets stored by Airflow 2 backends may lack both "conn_type" # and "uri" (e.g. {"host": ..., "login": ..., "password": ...}). This is # valid: conn_type is intentionally optional on the SDK Connection model - # for Airflow 2 -> 3 migration compatibility. Do not add validation here - # that rejects such secrets; see - # https://github.com/apache/airflow/pull/61728 + # for Airflow 2 -> 3 migration compatibility. + # Check: https://github.com/apache/airflow/pull/61728 + # TODO: Remove this compatibility once the minimum supported Airflow + # version in providers is 3.0. return conn_class.from_json(value=value, conn_id=conn_id) # type: ignore[attr-defined] # TODO: Only sdk has from_uri defined on it. Is it worthwhile developing the core path or not? diff --git a/shared/secrets_backend/tests/secrets_backend/test_base.py b/shared/secrets_backend/tests/secrets_backend/test_base.py index 79b413720ba9b..32d9993ae1ee5 100644 --- a/shared/secrets_backend/tests/secrets_backend/test_base.py +++ b/shared/secrets_backend/tests/secrets_backend/test_base.py @@ -125,8 +125,7 @@ def test_deserialize_connection_json(self, sample_conn_json): assert conn._kwargs["conn_type"] == "mysql" def test_deserialize_connection_json_without_conn_type(self): - """Airflow 2-era JSON secrets without conn_type or uri must keep deserializing. - + """ Guards the Airflow 2 -> 3 migration compatibility established in https://github.com/apache/airflow/pull/61728. """