diff --git a/scripts/ci/prek/check_provider_conn_fields.py b/scripts/ci/prek/check_provider_conn_fields.py new file mode 100644 index 0000000000000..ed4faf157c553 --- /dev/null +++ b/scripts/ci/prek/check_provider_conn_fields.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Validation helpers for the conn-fields ↔ get_connection_form_widgets() check. + +These functions have no third-party dependencies so they can be unit-tested +outside of the Breeze container without any stubbing. + +Used by ``scripts/in_container/run_provider_yaml_files_check.py``. +""" + +from __future__ import annotations + +from collections.abc import Callable + + +def check_conn_fields_for_entry( + conn_type_entry: dict, + yaml_file_path: str, + get_widget_keys: Callable[[str], set[str] | None], +) -> list[str]: + """ + Validate a single connection-type entry. Returns a (possibly empty) list of error strings. + + *get_widget_keys(hook_class_name)* is a callable supplied by the caller that: + + - returns the set of field keys from ``get_connection_form_widgets()`` on success, + - returns ``None`` to signal that the hook could not be imported, its UI + dependencies are absent, or it does not implement ``get_connection_form_widgets()`` + at all — the entry is then skipped entirely (no ``conn-fields`` check), or + - raises any other ``Exception`` to signal an unexpected failure (converted + here into an error string so callers never need to catch it). + """ + hook_class_name: str = conn_type_entry["hook-class-name"] + connection_type: str = conn_type_entry.get("connection-type", "?") + + try: + widget_keys = get_widget_keys(hook_class_name) + except Exception as exc: + return [ + f"Failed to call `{hook_class_name}.get_connection_form_widgets()` " + f"while checking {yaml_file_path}: {exc}" + ] + + if widget_keys is None: + return [] + + conn_fields = conn_type_entry.get("conn-fields") + if conn_fields is None: + # No conn-fields declared: the new UI simply exposes no custom fields for this + # connection type, which is intentional. Nothing to validate. + return [] + + error = build_mismatch_error( + set(conn_fields.keys()), widget_keys, connection_type, yaml_file_path, hook_class_name + ) + return [error] if error else [] + + +def build_mismatch_error( + yaml_keys: set[str], + hook_keys: set[str], + connection_type: str, + yaml_file_path: str, + hook_class_name: str, +) -> str | None: + """ + Check that every key declared in ``conn-fields`` exists in + ``get_connection_form_widgets()``. + + ``conn-fields`` is the new React UI's view of a connection type and is + intentionally a *subset* of the Flask form widgets — fields can be omitted + from ``conn-fields`` on purpose. We therefore only flag keys that appear in + ``conn-fields`` but are absent from the hook's form (invalid / stale + declarations). The reverse direction (hook fields not listed in + ``conn-fields``) is not an error. + + Return an error string when stale keys are found, or ``None`` when the + declared keys are all valid. + """ + only_in_yaml = yaml_keys - hook_keys + + if not only_in_yaml: + return None + + lines = [ + f"Mismatch between `conn-fields` in {yaml_file_path} and " + f"`{hook_class_name}.get_connection_form_widgets()` " + f"for connection-type '{connection_type}':" + ] + lines.append( + " Fields in provider.yaml conn-fields but NOT in get_connection_form_widgets(): " + + ", ".join(sorted(only_in_yaml)) + ) + lines.append("[yellow]How to fix it[/]: Remove the stale key(s) from conn-fields in provider.yaml.") + return "\n".join(lines) diff --git a/scripts/in_container/run_provider_yaml_files_check.py b/scripts/in_container/run_provider_yaml_files_check.py index db664a66e011c..922e84b710bc4 100755 --- a/scripts/in_container/run_provider_yaml_files_check.py +++ b/scripts/in_container/run_provider_yaml_files_check.py @@ -50,6 +50,11 @@ from airflow.exceptions import AirflowOptionalProviderFeatureException, AirflowProviderDeprecationWarning from airflow.providers_manager import ProvidersManager +# check_provider_conn_fields lives in scripts/ci/prek/ which is not on sys.path when +# this script runs inside Breeze; resolve it relative to this file. +sys.path.insert(0, str(pathlib.Path(__file__).parent.parent / "ci" / "prek")) +from check_provider_conn_fields import check_conn_fields_for_entry + # Those are deprecated modules that contain removed Hooks/Sensors/Operators that we left in the code # so that users can get a very specific error message when they try to use them. @@ -473,6 +478,65 @@ def check_hook_class_name_entries_in_connection_types(yaml_files: dict[str, dict return num_connection_types, num_errors +@run_check("Checking that conn-fields in provider.yaml match get_connection_form_widgets() of the hook class") +def check_conn_fields_match_form_widgets(yaml_files: dict[str, dict]) -> tuple[int, int]: + """ + For every connection-type entry whose hook declares ``conn-fields``, + verify that every key in ``conn-fields`` also exists in the hook's + ``get_connection_form_widgets()``. + + ``conn-fields`` is optional and is intentionally allowed to be a *subset* + of the hook's form widgets (the new React UI may expose fewer fields than + the legacy Flask form), so extra hook widgets are not flagged — only + ``conn-fields`` keys absent from the hook are reported as stale. + """ + num_checks = 0 + num_errors = 0 + + for yaml_file_path, provider_data in yaml_files.items(): + for conn_type_entry in provider_data.get("connection-types", []): + num_checks += 1 + for error in check_conn_fields_for_entry(conn_type_entry, yaml_file_path, _get_widget_keys): + errors.append(error) + num_errors += 1 + + return num_checks, num_errors + + +def _get_widget_keys(hook_class_name: str) -> set[str] | None: + """ + Import *hook_class_name* and return the keys of ``get_connection_form_widgets()``. + + Returns ``None`` when the hook or its UI dependencies cannot be imported, + or when the hook does not override ``get_connection_form_widgets()`` (meaning it + has no custom connection fields and the conn-fields check should be skipped). + Raises for unexpected errors so ``check_conn_fields_for_entry`` can convert them + to an error string. + """ + try: + module_name, class_name = hook_class_name.rsplit(".", maxsplit=1) + with warnings.catch_warnings(record=True): + hook_class = getattr(importlib.import_module(module_name), class_name) + except (ImportError, AirflowOptionalProviderFeatureException, AttributeError): + return None + + # Only validate hooks that override get_connection_form_widgets() in their own __dict__, + # because that method is the source-of-truth for what conn-fields should be declared. + # Hooks that inherit it without overriding have no provider-specific widget definition + # to diff against, so the check is intentionally skipped for them. As of writing this + # includes HttpHook, the common/ai hooks, and AzureComputeHook — any provider whose hook + # falls into this category will NOT be validated here, even if it declares conn-fields. + if "get_connection_form_widgets" not in hook_class.__dict__: + return None + + with warnings.catch_warnings(record=True): + try: + form_widgets: dict[str, Any] = hook_class.get_connection_form_widgets() + return set(form_widgets.keys()) + except (ImportError, AirflowOptionalProviderFeatureException, AttributeError): + return None + + @run_check("Checking that hook classes defining conn_type are registered in connection-types") def check_hook_classes_with_conn_type_are_registered(yaml_files: dict[str, dict]) -> tuple[int, int]: """Find Hook subclasses that define conn_type but are not listed in connection-types.""" @@ -1046,6 +1110,7 @@ def check_providers_have_all_documentation_files(yaml_files: dict[str, dict]): check_completeness_of_list_of_transfers(all_parsed_yaml_files) check_hook_class_name_entries_in_connection_types(all_parsed_yaml_files) + check_conn_fields_match_form_widgets(all_parsed_yaml_files) check_hook_classes_with_conn_type_are_registered(all_parsed_yaml_files) check_executor_classes(all_parsed_yaml_files) check_queue_classes(all_parsed_yaml_files) diff --git a/scripts/tests/ci/prek/test_check_conn_fields_match_form_widgets.py b/scripts/tests/ci/prek/test_check_conn_fields_match_form_widgets.py new file mode 100644 index 0000000000000..caa2b2fc35264 --- /dev/null +++ b/scripts/tests/ci/prek/test_check_conn_fields_match_form_widgets.py @@ -0,0 +1,124 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import pytest +from check_provider_conn_fields import ( + build_mismatch_error, + check_conn_fields_for_entry, +) + +YAML_PATH = "providers/my_provider/provider.yaml" +HOOK_CLASS = "my_provider.hooks.my_hook.MyHook" +CONN_TYPE = "my_conn_type" + + +def _entry(conn_fields: list[str] | None) -> dict: + entry: dict = {"hook-class-name": HOOK_CLASS, "connection-type": CONN_TYPE} + if conn_fields is not None: + entry["conn-fields"] = {k: {} for k in conn_fields} + return entry + + +def _get_keys(*keys: str): + """Return a get_widget_keys callable that always returns the given keys.""" + return lambda _hook_class_name: set(keys) + + +def _skip(_hook_class_name: str) -> None: + """get_widget_keys callable that signals 'skip silently'.""" + return None + + +def _raise(_hook_class_name: str) -> None: + raise RuntimeError("boom") + + +class TestBuildMismatchError: + @pytest.mark.parametrize( + "yaml_keys, hook_keys", + [ + pytest.param({"a", "b"}, {"a", "b"}, id="matching-keys"), + pytest.param(set(), set(), id="empty-sets"), + # Hook may have more keys than conn-fields — that is intentional (subset allowed). + pytest.param({"a"}, {"a", "extra_hook"}, id="hook-has-extra-keys-no-error"), + ], + ) + def test_no_mismatch_returns_none(self, yaml_keys, hook_keys): + assert build_mismatch_error(yaml_keys, hook_keys, CONN_TYPE, YAML_PATH, HOOK_CLASS) is None + + @pytest.mark.parametrize( + "yaml_keys, hook_keys, expected_in_error, not_expected_in_error", + [ + pytest.param( + {"a", "extra_yaml"}, + {"a"}, + ["extra_yaml", "NOT in get_connection_form_widgets"], + ["NOT in provider.yaml conn-fields"], + id="extra-in-yaml", + ), + # only_in_hook no longer triggers an error — only only_in_yaml does + pytest.param( + {"a", "only_yaml"}, + {"a", "only_hook"}, + [ + "only_yaml", + "NOT in get_connection_form_widgets", + ], + ["only_hook", "NOT in provider.yaml conn-fields"], + id="both-sides-only-yaml-reported", + ), + ], + ) + def test_mismatch_error_content(self, yaml_keys, hook_keys, expected_in_error, not_expected_in_error): + error = build_mismatch_error(yaml_keys, hook_keys, CONN_TYPE, YAML_PATH, HOOK_CLASS) + assert error is not None + for expected in expected_in_error: + assert expected in error + for not_expected in not_expected_in_error: + assert not_expected not in error + + +class TestCheckConnFieldsForEntry: + @pytest.mark.parametrize( + "conn_fields, get_keys", + [ + pytest.param(["a", "b"], _get_keys("a", "b"), id="matching-keys"), + pytest.param([], _get_keys(), id="empty-on-both-sides"), + pytest.param(["a"], _skip, id="skip-hook-without-get-connection-form-widgets"), + pytest.param(None, _skip, id="skip-missing-conn-fields-when-hook-has-no-widgets"), + # Hook with widgets but no conn-fields is allowed: new UI intentionally omits custom fields. + pytest.param(None, _get_keys("field_a"), id="no-conn-fields-with-hook-widgets-is-ok"), + # Hook has extra keys not in conn-fields — allowed (conn-fields is a valid subset). + pytest.param(["a"], _get_keys("a", "extra_hook"), id="hook-extra-keys-no-error"), + ], + ) + def test_no_errors(self, conn_fields, get_keys): + assert check_conn_fields_for_entry(_entry(conn_fields), YAML_PATH, get_keys) == [] + + @pytest.mark.parametrize( + "conn_fields, get_keys, expected_in_error", + [ + pytest.param(["a", "extra"], _get_keys("a"), "extra", id="extra-key-in-yaml"), + pytest.param(["a"], _raise, "boom", id="unexpected-exception-message"), + pytest.param(["a"], _raise, HOOK_CLASS, id="unexpected-exception-mentions-hook-class"), + ], + ) + def test_one_error_containing(self, conn_fields, get_keys, expected_in_error): + errors = check_conn_fields_for_entry(_entry(conn_fields), YAML_PATH, get_keys) + assert len(errors) == 1 + assert expected_in_error in errors[0]