From c1bf29fe131e1dd696c73d2b6d3b1f18ee5d8476 Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Mon, 15 Sep 2025 17:55:38 +0530 Subject: [PATCH 1/9] plugin module from breaking on invalid external_view destination --- .../core_api/routes/public/plugins.py | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index fe700178f6bf4..c68cdc64e1a16 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -17,7 +17,7 @@ from __future__ import annotations -from typing import cast +import logging from fastapi import Depends @@ -32,6 +32,8 @@ ) from airflow.api_fastapi.core_api.security import requires_access_view +logger = logging.getLogger(__name__) + plugins_router = AirflowRouter(tags=["Plugin"], prefix="/plugins") @@ -44,8 +46,22 @@ def get_plugins( offset: QueryOffset, ) -> PluginCollectionResponse: plugins_info = sorted(plugins_manager.get_plugin_info(), key=lambda x: x["name"]) + valid_plugins: list[PluginResponse] = [] + for plugin_dict in plugins_info[offset.value :][: limit.value]: + try: + # Validate each plugin individually + plugin = PluginResponse.model_validate(plugin_dict) + valid_plugins.append(plugin) + except Exception as e: + logger.warning( + "Skipping invalid plugin %s due to error: %s", + plugin_dict.get("name", ""), + e, + ) + continue + return PluginCollectionResponse( - plugins=cast("list[PluginResponse]", plugins_info[offset.value :][: limit.value]), + plugins=valid_plugins, total_entries=len(plugins_info), ) From 605fb8af1af9bce1f70ebc8f798933215d8da77c Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Tue, 16 Sep 2025 12:25:38 +0530 Subject: [PATCH 2/9] Added Test Case - For invalid plugin --- .../core_api/routes/public/plugins.py | 3 ++- .../core_api/routes/public/test_plugins.py | 26 ++++++++++++++++--- .../tests/unit/plugins/test_plugin.py | 15 +++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index c68cdc64e1a16..3d331511dbd36 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -20,6 +20,7 @@ import logging from fastapi import Depends +from pydantic import ValidationError from airflow import plugins_manager from airflow.api_fastapi.auth.managers.models.resource_details import AccessView @@ -52,7 +53,7 @@ def get_plugins( # Validate each plugin individually plugin = PluginResponse.model_validate(plugin_dict) valid_plugins.append(plugin) - except Exception as e: + except ValidationError as e: logger.warning( "Skipping invalid plugin %s due to error: %s", plugin_dict.get("name", ""), diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py index c63a7b19db82f..d98095a638d49 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py @@ -33,7 +33,7 @@ class TestGetPlugins: # Filters ( {}, - 13, + 14, [ "MetadataCollectionPlugin", "OpenLineageProviderPlugin", @@ -52,10 +52,10 @@ class TestGetPlugins: ), ( {"limit": 3, "offset": 2}, - 13, + 14, ["databricks_workflow", "decreasing_priority_weight_strategy_plugin", "edge_executor"], ), - ({"limit": 1}, 13, ["MetadataCollectionPlugin"]), + ({"limit": 1}, 14, ["MetadataCollectionPlugin"]), ], ) def test_should_respond_200( @@ -119,6 +119,26 @@ def test_should_response_403(self, unauthorized_test_client): response = unauthorized_test_client.get("/plugins") assert response.status_code == 403 + def test_invalid_external_view_destination_should_log_warning_and_continue(self, test_client, caplog): + pytest.importorskip("flask_appbuilder") # Remove after upgrading to FAB5 + + caplog.set_level("WARNING", "airflow.api_fastapi.core_api.routes.public.plugins") + + response = test_client.get("/plugins") + assert response.status_code == 200 + + body = response.json() + plugin_names = [plugin["name"] for plugin in body["plugins"]] + + # Ensure our invalid plugin is skipped from the valid list + assert "test_plugin_invalid" not in plugin_names + + # Verify warning was logged + assert any( + "Skipping invalid plugin test_plugin_invalid due to error:" in rec.message + for rec in caplog.records + ) + @skip_if_force_lowest_dependencies_marker class TestGetPluginImportErrors: diff --git a/airflow-core/tests/unit/plugins/test_plugin.py b/airflow-core/tests/unit/plugins/test_plugin.py index def446620e94b..62e642955ef6e 100644 --- a/airflow-core/tests/unit/plugins/test_plugin.py +++ b/airflow-core/tests/unit/plugins/test_plugin.py @@ -183,3 +183,18 @@ class AirflowTestOnLoadPlugin(AirflowPlugin): def on_load(self, *args, **kwargs): self.name = "postload" + + +# Example external view with invalid destination +external_view_with_invalid_destination = { + "name": "Invalid External View", + "href": "https://example.com/invalid", + "url_route": "invalid_external_view", + "destination": "Assets", # <-- invalid destination + "icon": "book", +} + + +class AirflowTestPluginInvalid(AirflowPlugin): + name = "test_plugin_invalid" + external_views = [external_view_with_invalid_destination] From 7796b1176eb97ba4678079f9890e2c68ee7921c7 Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Tue, 16 Sep 2025 15:24:48 +0530 Subject: [PATCH 3/9] Suggested Change Co-authored-by: Ash Berlin-Taylor --- .../src/airflow/api_fastapi/core_api/routes/public/plugins.py | 1 - 1 file changed, 1 deletion(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index 3d331511dbd36..28ab5ea7a3c14 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -17,7 +17,6 @@ from __future__ import annotations -import logging from fastapi import Depends from pydantic import ValidationError From 7866baa21071d36db48f6591fc02220a8dabaf65 Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Tue, 16 Sep 2025 15:24:59 +0530 Subject: [PATCH 4/9] Suggested Change Co-authored-by: Ash Berlin-Taylor --- .../src/airflow/api_fastapi/core_api/routes/public/plugins.py | 1 + 1 file changed, 1 insertion(+) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index 28ab5ea7a3c14..91dae977dcda5 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -18,6 +18,7 @@ from __future__ import annotations +import structlog from fastapi import Depends from pydantic import ValidationError From 2b4a9372ac56e294481d59576b3fc88d7e139ad9 Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Tue, 16 Sep 2025 15:25:14 +0530 Subject: [PATCH 5/9] Suggested Change Co-authored-by: Ash Berlin-Taylor --- .../src/airflow/api_fastapi/core_api/routes/public/plugins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index 91dae977dcda5..6f40655454b4e 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -33,7 +33,7 @@ ) from airflow.api_fastapi.core_api.security import requires_access_view -logger = logging.getLogger(__name__) +logger = structlog.get_logger(__name__) plugins_router = AirflowRouter(tags=["Plugin"], prefix="/plugins") From 8ab8972cb6aeb188d22260b4b22f9c8dcdc28cff Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Tue, 16 Sep 2025 15:25:29 +0530 Subject: [PATCH 6/9] Suggested Change Co-authored-by: Ash Berlin-Taylor --- .../airflow/api_fastapi/core_api/routes/public/plugins.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index 6f40655454b4e..c11b525839452 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -55,9 +55,9 @@ def get_plugins( valid_plugins.append(plugin) except ValidationError as e: logger.warning( - "Skipping invalid plugin %s due to error: %s", - plugin_dict.get("name", ""), - e, + "Skipping invalid plugin due to error", + plugin_name=plugin_dict.get("name", ""), + error=str(e), ) continue From 879f16e609dacee715559b93ea87a6b4255b558d Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Tue, 16 Sep 2025 15:52:38 +0530 Subject: [PATCH 7/9] suggested change as per feedback --- .../unit/api_fastapi/core_api/routes/public/test_plugins.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py index d98095a638d49..b88eaabe34980 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py @@ -134,10 +134,7 @@ def test_invalid_external_view_destination_should_log_warning_and_continue(self, assert "test_plugin_invalid" not in plugin_names # Verify warning was logged - assert any( - "Skipping invalid plugin test_plugin_invalid due to error:" in rec.message - for rec in caplog.records - ) + assert any("Skipping invalid plugin due to error" in rec.message for rec in caplog.records) @skip_if_force_lowest_dependencies_marker From ccd8ffc6ec5cda6b4f326f30876661f0d01ea7ba Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Wed, 17 Sep 2025 11:17:56 +0530 Subject: [PATCH 8/9] Modified the total plugin count in testcases --- airflow-core/tests/unit/plugins/test_plugins_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airflow-core/tests/unit/plugins/test_plugins_manager.py b/airflow-core/tests/unit/plugins/test_plugins_manager.py index 0df2e0d05053d..eaf799ff12c13 100644 --- a/airflow-core/tests/unit/plugins/test_plugins_manager.py +++ b/airflow-core/tests/unit/plugins/test_plugins_manager.py @@ -94,7 +94,7 @@ def test_loads_filesystem_plugins(self, caplog): with mock.patch("airflow.plugins_manager.plugins", []): plugins_manager.load_plugins_from_plugin_directory() - assert len(plugins_manager.plugins) == 9 + assert len(plugins_manager.plugins) == 10 for plugin in plugins_manager.plugins: if "AirflowTestOnLoadPlugin" in str(plugin): assert plugin.name == "postload" From 2ad23274ebac7ed4c7ad054f22dd06632be606d3 Mon Sep 17 00:00:00 2001 From: Brunda10 Date: Thu, 18 Sep 2025 13:01:40 +0530 Subject: [PATCH 9/9] validation done before pagination --- .../core_api/routes/public/plugins.py | 11 +++++++---- .../core_api/routes/public/test_plugins.py | 18 +++++++++++++++--- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py index c11b525839452..3b5368f50d232 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py @@ -17,7 +17,6 @@ from __future__ import annotations - import structlog from fastapi import Depends from pydantic import ValidationError @@ -48,7 +47,7 @@ def get_plugins( ) -> PluginCollectionResponse: plugins_info = sorted(plugins_manager.get_plugin_info(), key=lambda x: x["name"]) valid_plugins: list[PluginResponse] = [] - for plugin_dict in plugins_info[offset.value :][: limit.value]: + for plugin_dict in plugins_info: try: # Validate each plugin individually plugin = PluginResponse.model_validate(plugin_dict) @@ -61,9 +60,13 @@ def get_plugins( ) continue + offset_value = offset.value or 0 + limit_value = limit.value if limit.value is not None else len(valid_plugins) + + paginated_plugins = valid_plugins[offset_value : offset_value + limit_value] return PluginCollectionResponse( - plugins=valid_plugins, - total_entries=len(plugins_info), + plugins=paginated_plugins, + total_entries=len(valid_plugins), ) diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py index b88eaabe34980..36312e710abff 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py @@ -33,7 +33,7 @@ class TestGetPlugins: # Filters ( {}, - 14, + 13, [ "MetadataCollectionPlugin", "OpenLineageProviderPlugin", @@ -52,10 +52,10 @@ class TestGetPlugins: ), ( {"limit": 3, "offset": 2}, - 14, + 13, ["databricks_workflow", "decreasing_priority_weight_strategy_plugin", "edge_executor"], ), - ({"limit": 1}, 14, ["MetadataCollectionPlugin"]), + ({"limit": 1}, 13, ["MetadataCollectionPlugin"]), ], ) def test_should_respond_200( @@ -136,6 +136,18 @@ def test_invalid_external_view_destination_should_log_warning_and_continue(self, # Verify warning was logged assert any("Skipping invalid plugin due to error" in rec.message for rec in caplog.records) + response = test_client.get("/plugins", params={"limit": 5, "offset": 9}) + assert response.status_code == 200 + + body = response.json() + plugins_page = body["plugins"] + + # Even though limit=5, only 4 valid plugins should come back + assert len(plugins_page) == 4 + assert "test_plugin_invalid" not in [p["name"] for p in plugins_page] + + assert body["total_entries"] == 13 + @skip_if_force_lowest_dependencies_marker class TestGetPluginImportErrors: