potiuk commented on code in PR #69473:
URL: https://github.com/apache/airflow/pull/69473#discussion_r3550328274
##########
scripts/in_container/run_provider_yaml_files_check.py:
##########
@@ -473,6 +478,58 @@ 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, verify that:
+
+ 1. ``conn-fields`` is present (all providers must declare it).
+ 2. The field names are exactly the keys returned by
+ ``get_connection_form_widgets()`` on the corresponding hook class,
+ and vice-versa.
+ """
+ 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 check if the hook itself (not a base class) defines
get_connection_form_widgets.
+ if "get_connection_form_widgets" not in hook_class.__dict__:
Review Comment:
Worth a note here (or in the module docstring): because this skips any hook
that doesn't define `get_connection_form_widgets()` in its own `__dict__`,
providers that declare `conn-fields` but whose hook has no such method are
**silently not validated at all** — in the current tree that's `http`
(`HttpHook`), the five `common/ai` hooks, and `AzureComputeHook`. That's a
defensible choice (there's no widget source-of-truth to diff against), but the
check's coverage is narrower than the PR description implies, so calling it out
prevents a future "why didn't this catch my stale http conn-field" surprise.
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
##########
scripts/in_container/run_provider_yaml_files_check.py:
##########
@@ -473,6 +478,58 @@ 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, verify that:
+
+ 1. ``conn-fields`` is present (all providers must declare it).
+ 2. The field names are exactly the keys returned by
+ ``get_connection_form_widgets()`` on the corresponding hook class,
+ and vice-versa.
Review Comment:
This docstring is stale and contradicts both the actual implementation and
the helper module's own docstring: `conn-fields` is **not** required (a
missing/`None` `conn-fields` is skipped), and the check is one-directional
**subset** validation, not "exactly the keys … and vice-versa" (extra hook
widgets are deliberately allowed). Suggested fix:
```suggestion
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.
```
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
