You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Brunda10 (via GitHub)" <gi...@apache.org> on 2025/09/15 12:39:13 UTC
[PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Brunda10 opened a new pull request, #55673:
URL: https://github.com/apache/airflow/pull/55673
This PR fixes an issue where an invalid plugin definition (e.g., an unsupported destination in external_views) caused the entire /plugins API endpoint to fail with a 500.
Closes : https://github.com/apache/airflow/issues/55571
With this change:
Added validation when constructing PluginResponse objects.
Only valid plugins are returned in the /api/v2/plugins API response.
Invalid plugins are skipped gracefully.
A warning is logged in the terminal with the plugin name and the error details.
This ensures the UI remains functional even if some plugins are misconfigured.
Impact:
- Prevents the entire /plugins API from failing due to a single invalid plugin.
- Users will continue to see all valid plugins in the UI, while still being informed about invalid ones through logs.
Attached screenshot for reference
<img width="1438" height="1102" alt="Screenshot from 2025-09-15 17-49-50" src="https://github.com/user-attachments/assets/63fab837-b78f-4e8f-90ca-8be91d320c7e" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #55673:
URL: https://github.com/apache/airflow/pull/55673#issuecomment-3307029639
CI is failing, I'll rebase the PR to trigger it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #55673:
URL: https://github.com/apache/airflow/pull/55673#issuecomment-3307624705
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "Brunda10 (via GitHub)" <gi...@apache.org>.
Brunda10 commented on PR #55673:
URL: https://github.com/apache/airflow/pull/55673#issuecomment-3305844147
Thanks for the feedback @pierrejeambrun ,
I’ve updated the implementation to:
- First validate all plugins and build the valid_plugins list.
- Then apply pagination and total_entries only on the valid set.
- Added a test case to validate pagination when invalid plugins are present, ensuring page size and offsets behave correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #55673:
URL: https://github.com/apache/airflow/pull/55673#discussion_r2349307071
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py:
##########
@@ -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", "<unknown>"),
+ e,
+ )
+ continue
+
Review Comment:
Also could you add a relevant test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #55673:
URL: https://github.com/apache/airflow/pull/55673#issuecomment-3308232721
Adding to 3.1 milestone. This feels like a useful bug fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #55673:
URL: https://github.com/apache/airflow/pull/55673#discussion_r2358158106
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py:
##########
@@ -44,9 +46,27 @@ 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:
+ try:
+ # Validate each plugin individually
+ plugin = PluginResponse.model_validate(plugin_dict)
+ valid_plugins.append(plugin)
+ except ValidationError as e:
+ logger.warning(
+ "Skipping invalid plugin due to error",
+ plugin_name=plugin_dict.get("name", "<unknown>"),
+ error=str(e),
+ )
+ continue
+
+ offset_value = offset.value or 0
Review Comment:
By taking a look at the defiition of `offset_value` and `limit_value` those can't be None. You're having type error because mypy is having a hard time understanding how fastapi validation works I guess. But that's fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "Brunda10 (via GitHub)" <gi...@apache.org>.
Brunda10 commented on code in PR #55673:
URL: https://github.com/apache/airflow/pull/55673#discussion_r2358240563
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py:
##########
@@ -44,9 +46,27 @@ 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:
+ try:
+ # Validate each plugin individually
+ plugin = PluginResponse.model_validate(plugin_dict)
+ valid_plugins.append(plugin)
+ except ValidationError as e:
+ logger.warning(
+ "Skipping invalid plugin due to error",
+ plugin_name=plugin_dict.get("name", "<unknown>"),
+ error=str(e),
+ )
+ continue
+
+ offset_value = offset.value or 0
Review Comment:
Yes, The values coming from FastAPI validation won’t actually be None, but I added the default handling mainly because pre-commit/mypy was flagging it as a type error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #55673:
URL: https://github.com/apache/airflow/pull/55673
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #55673:
URL: https://github.com/apache/airflow/pull/55673#discussion_r2349299789
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py:
##########
@@ -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", "<unknown>"),
+ e,
+ )
+ continue
+
Review Comment:
Please only catch `ValidationError` from pydantic. Not all exceptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Validate plugins in /plugins API and skip invalid ones with warnings [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #55673:
URL: https://github.com/apache/airflow/pull/55673#discussion_r2355962206
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/plugins.py:
##########
@@ -44,8 +47,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 ValidationError as e:
+ logger.warning(
+ "Skipping invalid plugin due to error",
+ plugin_name=plugin_dict.get("name", "<unknown>"),
+ error=str(e),
+ )
+ continue
Review Comment:
Just realized you can't do that. You need to apply the validation before the 'virtual' pagination.
Otherwise the returned page could be 'partial' which isn't correct.
Filter all `plugins_info` and then apply pagination and `total_entries`.
|
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_plugins.py:
##########
@@ -119,6 +119,23 @@ 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 due to error" in rec.message for rec in caplog.records)
Review Comment:
To verify my comment above you could also check the size of items returned, with a small page count and the appropriate offset (just before the invalid items) your page size could be smaller that the expected one (even if this is not the last page)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org