feat(plugins): Add thorough type hinting for AirflowPlugin attributes#62455
feat(plugins): Add thorough type hinting for AirflowPlugin attributes#62455SakshamSinghal20 wants to merge 2 commits intoapache:mainfrom
AirflowPlugin attributes#62455Conversation
There was a problem hiding this comment.
Pull request overview
This PR significantly improves type safety for Apache Airflow plugin configuration by introducing Pydantic validation models for plugin attributes. The changes enable better IDE support, earlier error detection, and clearer documentation of expected plugin schemas.
Changes:
- Added Pydantic config models (
ExternalViewConfig,FastAPIAppConfig,AppBuilderViewConfig, etc.) inairflow-core/src/airflow/plugins_manager.pyfor validating plugin attribute dictionaries - Enhanced
AirflowPlugindocstring with detailed attribute descriptions in the shared module - Integrated validation into plugin loading via
validate_plugin_attributes(), with errors reported as import errors - Updated plugin info serialization to handle both dict and Pydantic model instances transparently
- Added comprehensive unit tests for config models and validation logic
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| airflow-core/src/airflow/plugins_manager.py | Core implementation: defines 6 Pydantic config models, validation logic, and helper functions for transparent dict/model access |
| shared/plugins_manager/src/airflow_shared/plugins_manager/plugins_manager.py | Enhanced docstring documenting all plugin attributes with expected types and usage patterns |
| airflow-core/src/airflow/api_fastapi/app.py | Updated FastAPI plugin initialization to use _get_attr helper for accessing plugin attributes |
| airflow-core/tests/unit/plugins/test_plugins_manager.py | Added 346 lines of tests covering config models, validation, and helper functions |
| """ | ||
|
|
||
| name: str = Field(..., description="Display name for the React app.") | ||
| bundle_url: str | None = Field(default=None, description="URL to the React app's JavaScript bundle.") |
There was a problem hiding this comment.
The ReactAppConfig model makes bundle_url optional (default=None), but the corresponding API response model ReactAppResponse in airflow/api_fastapi/core_api/datamodels/plugins.py (line 101) requires bundle_url as a mandatory field. This mismatch could allow invalid plugin configurations that would fail during API serialization.
Consider making bundle_url required in ReactAppConfig to match the API contract and ensure plugins are validated at load time rather than at API response time.
| bundle_url: str | None = Field(default=None, description="URL to the React app's JavaScript bundle.") | |
| bundle_url: str = Field(..., description="URL to the React app's JavaScript bundle.") |
| fastapi_apps : list[dict | FastAPIAppConfig] | ||
| FastAPI sub-applications to mount on the Airflow API. Each item | ||
| must contain ``app`` (a ``FastAPI`` instance), ``url_prefix`` | ||
| (non-empty string), and ``name``. | ||
| fastapi_root_middlewares : list[dict | FastAPIRootMiddlewareConfig] | ||
| Root-level ASGI middlewares. Each item must contain | ||
| ``middleware`` (class/factory) and ``name``. | ||
| external_views : list[dict | ExternalViewConfig] | ||
| External views rendered as iframes. Expected keys: ``name``, | ||
| ``href``, ``url_route``, ``icon``, ``icon_dark_mode``, | ||
| ``category``, ``destination``. | ||
| react_apps : list[dict | ReactAppConfig] | ||
| React micro-frontends. Expected keys: ``name``, | ||
| ``bundle_url``, ``url_route``, ``icon``, ``destination``, | ||
| ``category``. | ||
| menu_links : list[Any] | ||
| *Deprecated.* Legacy Flask-Admin menu links. | ||
| appbuilder_views : list[dict | AppBuilderViewConfig] | ||
| Flask AppBuilder views. Each item should contain ``view`` | ||
| (a ``BaseView`` instance), ``name``, ``category``, and | ||
| optionally ``label``. | ||
| appbuilder_menu_items : list[dict | AppBuilderMenuItemConfig] |
There was a problem hiding this comment.
The docstring refers to type names (FastAPIRootMiddlewareConfig, ExternalViewConfig, ReactAppConfig, AppBuilderViewConfig, AppBuilderMenuItemConfig) that don't exist in this shared module. These types are only defined in airflow-core, but the shared module can be used without airflow-core being installed.
Update the type annotations in the docstring to use more generic descriptions or clarify that these specific type names are only available in airflow-core. For consistency with the implementation, consider using dict or Any for the type hints in the attribute descriptions.
| fastapi_apps : list[dict | FastAPIAppConfig] | |
| FastAPI sub-applications to mount on the Airflow API. Each item | |
| must contain ``app`` (a ``FastAPI`` instance), ``url_prefix`` | |
| (non-empty string), and ``name``. | |
| fastapi_root_middlewares : list[dict | FastAPIRootMiddlewareConfig] | |
| Root-level ASGI middlewares. Each item must contain | |
| ``middleware`` (class/factory) and ``name``. | |
| external_views : list[dict | ExternalViewConfig] | |
| External views rendered as iframes. Expected keys: ``name``, | |
| ``href``, ``url_route``, ``icon``, ``icon_dark_mode``, | |
| ``category``, ``destination``. | |
| react_apps : list[dict | ReactAppConfig] | |
| React micro-frontends. Expected keys: ``name``, | |
| ``bundle_url``, ``url_route``, ``icon``, ``destination``, | |
| ``category``. | |
| menu_links : list[Any] | |
| *Deprecated.* Legacy Flask-Admin menu links. | |
| appbuilder_views : list[dict | AppBuilderViewConfig] | |
| Flask AppBuilder views. Each item should contain ``view`` | |
| (a ``BaseView`` instance), ``name``, ``category``, and | |
| optionally ``label``. | |
| appbuilder_menu_items : list[dict | AppBuilderMenuItemConfig] | |
| fastapi_apps : list[dict] | |
| FastAPI sub-applications to mount on the Airflow API. Each item | |
| must contain ``app`` (a ``FastAPI`` instance), ``url_prefix`` | |
| (non-empty string), and ``name``. | |
| fastapi_root_middlewares : list[dict] | |
| Root-level ASGI middlewares. Each item must contain | |
| ``middleware`` (class/factory) and ``name``. | |
| external_views : list[dict] | |
| External views rendered as iframes. Expected keys: ``name``, | |
| ``href``, ``url_route``, ``icon``, ``icon_dark_mode``, | |
| ``category``, ``destination``. | |
| react_apps : list[dict] | |
| React micro-frontends. Expected keys: ``name``, | |
| ``bundle_url``, ``url_route``, ``icon``, ``destination``, | |
| ``category``. | |
| menu_links : list[Any] | |
| *Deprecated.* Legacy Flask-Admin menu links. | |
| appbuilder_views : list[dict] | |
| Flask AppBuilder views. Each item should contain ``view`` | |
| (a ``BaseView`` instance), ``name``, ``category``, and | |
| optionally ``label``. | |
| appbuilder_menu_items : list[dict] |
| layer. | ||
| """ | ||
| if isinstance(item, _PluginConfigBase): | ||
| return item.model_dump(exclude_unset=True) |
There was a problem hiding this comment.
The _to_dict function uses model_dump(exclude_unset=True), which omits fields that were not explicitly set, even if they have default values. For example, if an ExternalViewConfig is created with only name="V", the resulting dict won't include the destination key, even though the model has a default value of "nav".
This could break existing code that expects certain keys to always be present in the plugin info dictionaries. Consider using model_dump() without exclude_unset=True to ensure default values are always included, or add a test to verify this behavior is intentional and document it clearly.
| return item.model_dump(exclude_unset=True) | |
| return item.model_dump() |
| assert isinstance(result, dict) | ||
| assert result["name"] == "V" | ||
| assert result["href"] == "https://test.com" | ||
|
|
There was a problem hiding this comment.
The test verifies that _to_dict converts a model to a dict, but doesn't verify the behavior of exclude_unset=True. Specifically, it doesn't test whether fields with default values that weren't explicitly set are included or excluded from the output.
Add a test case like:
cfg = ExternalViewConfig(name="V")
result = _to_dict(cfg)
assert "destination" not in result # or assert result["destination"] == "nav" if you want defaults includedThis will make the intended behavior explicit and catch any regressions.
| def test_to_dict_exclude_unset_behavior(self): | |
| from airflow.plugins_manager import ExternalViewConfig, _to_dict | |
| # Ensure that fields with default values that are not explicitly set | |
| # are handled consistently by _to_dict (exclude_unset behavior). | |
| cfg = ExternalViewConfig(name="V") | |
| result = _to_dict(cfg) | |
| # With exclude_unset=True, defaulted-but-unset fields like "destination" | |
| # should not be present in the output dict. | |
| assert "destination" not in result |
| """ | ||
|
|
||
| name: str = Field(..., description="Display name for the external view.") | ||
| href: str | None = Field(default=None, description="URL the external view iframe points to.") |
There was a problem hiding this comment.
The ExternalViewConfig model makes href optional (default=None), but the corresponding API response model ExternalViewResponse in airflow/api_fastapi/core_api/datamodels/plugins.py (line 92) requires href as a mandatory field. This inconsistency could allow plugins to be registered with external views that lack an href, which would then fail when serializing the API response.
Consider either:
- Making
hrefrequired inExternalViewConfigto match the API contract, or - Making
hrefoptional inExternalViewResponseand handling None values appropriately in the UI
| href: str | None = Field(default=None, description="URL the external view iframe points to.") | |
| href: str = Field(..., description="URL the external view iframe points to.") |
| name: str = Field(..., description="Display name for the menu item.") | ||
| href: str = Field(..., description="URL the menu item links to.") | ||
| category: str | None = Field(default=None, description="Navigation category for grouping.") | ||
| label: str | None = Field(default=None, description="Label override for the menu item.") |
There was a problem hiding this comment.
The AppBuilderMenuItemConfig includes a label field (line 169) that is not present in the corresponding AppBuilderMenuItemResponse model (which only has name, href, and category). This inconsistency means that if a plugin developer provides a label, it will be silently ignored in the API response.
Either add label to AppBuilderMenuItemResponse, or remove it from AppBuilderMenuItemConfig and update the documentation to clarify that label is not supported for menu items.
| following class-level attributes. Each attribute accepts a list of | ||
| items; items may be supplied as plain ``dict`` objects or, when | ||
| ``airflow-core`` is available, as strongly-typed Pydantic config | ||
| model instances (see ``airflow.plugins_manager``). |
There was a problem hiding this comment.
The docstring references types like FastAPIAppConfig, FastAPIRootMiddlewareConfig, ExternalViewConfig, ReactAppConfig, and AppBuilderMenuItemConfig which are not defined in the shared plugins_manager module. These types only exist in airflow-core/src/airflow/plugins_manager.py, but this docstring is in the shared module that can be used without airflow-core.
The docstring should clarify that these strongly-typed models are only available when airflow-core is installed, or the references should be updated to reflect that these types may not be available in all contexts. For example: "dict objects or, when airflow-core is available, as strongly-typed Pydantic config model instances".
| model instances (see ``airflow.plugins_manager``). | |
| model instances (see the corresponding config models in | |
| ``airflow.plugins_manager`` when ``airflow-core`` is installed). |
| errors = validate_plugin_attributes(plugin) | ||
| assert errors == [], f"Backward compatibility failure: {errors}" | ||
|
|
||
|
|
There was a problem hiding this comment.
While there are comprehensive unit tests for validate_plugin_attributes in isolation (lines 562-708), there's no integration test verifying that validation errors are properly collected as import errors during the actual plugin loading process in __register_plugins (lines 475-486).
Add an integration test using mock_plugin_manager to verify that when a plugin with invalid attributes is loaded, the validation errors appear in the import_errors dict returned by _get_plugins(). This would ensure the validation logic is properly wired into the plugin loading flow.
| class TestPluginValidationIntegration: | |
| def test_validation_errors_collected_in_import_errors(self): | |
| from airflow.plugins_manager import _get_plugins, validate_plugin_attributes | |
| class InvalidPlugin(AirflowPlugin): | |
| name = "invalid_plugin_with_bad_external_view" | |
| # Missing required fields in external_views entry to trigger validation errors. | |
| external_views = [ | |
| { | |
| # Intentionally omit required keys such as "destination" or others | |
| # to ensure validate_plugin_attributes reports an error. | |
| "name": "Invalid External View", | |
| } | |
| ] | |
| # Sanity-check that the plugin is actually invalid and produces validation errors. | |
| expected_errors = validate_plugin_attributes(InvalidPlugin) | |
| assert expected_errors, "Expected validate_plugin_attributes to report errors for InvalidPlugin" | |
| with mock_plugin_manager(plugins=[InvalidPlugin]): | |
| plugins, import_errors = _get_plugins() | |
| # The invalid plugin should not be returned as a successfully loaded plugin. | |
| assert not any( | |
| getattr(p, "name", None) == InvalidPlugin.name for p in plugins | |
| ), "Invalid plugin should not appear in the loaded plugins list" | |
| # Validation errors should be present in import_errors. | |
| assert import_errors, "Expected import_errors to contain validation errors" | |
| # Flatten all collected errors and ensure they include the validation errors we observed. | |
| collected_errors = [] | |
| for errs in import_errors.values(): | |
| if isinstance(errs, (list, tuple)): | |
| collected_errors.extend(errs) | |
| else: | |
| collected_errors.append(errs) | |
| for err in expected_errors: | |
| assert err in collected_errors |
| log.warning( | ||
| "Plugin '%s' has an external view that is not a dictionary. The view will not be loaded.", | ||
| plugin.name, |
There was a problem hiding this comment.
The warning message says "is not a dictionary" but the check on line 527 now also accepts _PluginConfigBase instances. The message should be updated to reflect this, e.g., "Plugin '%s' has an external view that is not a valid type (dict or config model). The view will not be loaded."
| log.warning( | ||
| "Plugin '%s' has a React App that is not a dictionary. The React App will not be loaded.", | ||
| plugin.name, |
There was a problem hiding this comment.
The warning message says "is not a dictionary" but the check on line 551 now also accepts _PluginConfigBase instances. The message should be updated to reflect this, e.g., "Plugin '%s' has a React App that is not a valid type (dict or config model). The React App will not be loaded."
Type hinting has been significantly improved for
AirflowPluginattributes to enhance developer experience and runtime safety.Previously, attributes such as
appbuilder_menu_items,external_views, andfastapi_appswere typed asList[Any]or generic lists. This lack of structure made it difficult for plugin developers to know the expected schema and allowed invalid configurations to pass until they caused runtime errors.Changes in this PR:
ExternalViewConfigAppBuilderViewConfigAppBuilderMenuItemConfigFastAPIAppConfigFastAPIMiddlewareConfigAirflowPlugin: Refactored the class to useSequence[Model]type hints instead ofList[Any].AirflowPlugin.validate()method to rigorously check provided values against these schemas.These changes allow IDEs to provide better autocompletion and catch configuration errors early during plugin loading.
closes: #62222
Was generative AI tooling used to co-author this PR?