From 192dfbd23c50c79936dc57393011aecff18c3805 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Tue, 11 Oct 2022 11:20:14 -0700 Subject: [PATCH] No extra__conn_type__ prefix required for UI behaviors After making it so we can omit the prefix in `get_connection_form_widgets`, we left `get_ui_field_behaviour` alone because, even though maybe it's unlikely and probably not a great idea, it's *possible* that a user would want to add an extra field that shares a name with an existing conn attr _and_ want to provide a placeholder for that field. But, after turning my attention back to this area, I think I arrived a at a reasonable way to solve this, which should make things easier for users when trying to implement extra fields for hooks. What we do is, check the placeholders when processing the hook in the providers manager, and if there are any fields which are not conn attrs and are not prefixed already, we add the prefix. We don't need to worry about "hidden fields" or "relabeling" because these are only used with the built-in conn attrs. --- airflow/providers_manager.py | 33 ++++++++++++++++++++++++ docs/apache-airflow/howto/connection.rst | 9 ++++--- tests/core/test_providers_manager.py | 25 ++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/airflow/providers_manager.py b/airflow/providers_manager.py index b6b599e13f328..2e879fbc11514 100644 --- a/airflow/providers_manager.py +++ b/airflow/providers_manager.py @@ -49,6 +49,33 @@ "apache-airflow-providers-celery": "2.1.0", } + +def _ensure_prefix_for_placeholders(field_behaviors: dict[str, Any], conn_type: str): + """ + If the given field_behaviors dict contains a placeholders node, and there + are placeholders for extra fields (i.e. anything other than the built-in conn + attrs), and if those extra fields are unprefixed, then add the prefix. + + The reason we need to do this is, all custom conn fields live in the same dictionary, + so we need to namespace them with a prefix internally. But for user convenience, + and consistency between the `get_ui_field_behaviour` method and the extra dict itself, + we allow users to supply the unprefixed name. + """ + conn_attrs = {'host', 'schema', 'login', 'password', 'port', 'extra'} + + def ensure_prefix(field): + if field not in conn_attrs and not field.startswith('extra__'): + return f"extra__{conn_type}__{field}" + else: + return field + + if 'placeholders' in field_behaviors: + placeholders = field_behaviors['placeholders'] + field_behaviors['placeholders'] = {ensure_prefix(k): v for k, v in placeholders.items()} + + return field_behaviors + + if TYPE_CHECKING: from airflow.decorators.base import TaskDecorator from airflow.hooks.base import BaseHook @@ -740,6 +767,7 @@ def _import_hook( # hierarchy and we add it only from the parent hook that provides those! if 'get_connection_form_widgets' in hook_class.__dict__: widgets = hook_class.get_connection_form_widgets() + if widgets: for widget in widgets.values(): if widget.field_class not in allowed_field_classes: @@ -821,7 +849,12 @@ def _add_widgets(self, package_name: str, hook_class: type, widgets: dict[str, A def _add_customized_fields(self, package_name: str, hook_class: type, customized_fields: dict): try: connection_type = getattr(hook_class, "conn_type") + self._customized_form_fields_schema_validator.validate(customized_fields) + + if connection_type: + customized_fields = _ensure_prefix_for_placeholders(customized_fields, connection_type) + if connection_type in self._field_behaviours: log.warning( "The connection_type %s from package %s and class %s has already been added " diff --git a/docs/apache-airflow/howto/connection.rst b/docs/apache-airflow/howto/connection.rst index f28392bf32e3f..17ed4eae18a88 100644 --- a/docs/apache-airflow/howto/connection.rst +++ b/docs/apache-airflow/howto/connection.rst @@ -274,12 +274,15 @@ An example: "relabeling": {}, "placeholders": { "password": "Asana personal access token", - "extra__my_conn_type__workspace": "My workspace gid", - "extra__my_conn_type__project": "My project gid", + "workspace": "My workspace gid", + "project": "My project gid", }, } -Note here that *here* (in contrast with ``get_connection_form_widgets``) we must add the prefix ``extra____`` when referencing a custom field. This is because it's possible to create a custom field whose name overlaps with a built-in field and we need to be able to reference it unambiguously. +.. note:: + + If you want to add a form placeholder for an ``extra`` field whose name conflicts with a standard connection attribute (i.e. login, password, host, scheme, port, extra) then + you must prefix it with ``extra____``. E.g. ``extra__myservice__password``. Take a look at providers for examples of what you can do, for example :py:class:`~airflow.providers.jdbc.hooks.jdbc.JdbcHook` and :py:class:`~airflow.providers.asana.hooks.jdbc.AsanaHook` both make use of this feature. diff --git a/tests/core/test_providers_manager.py b/tests/core/test_providers_manager.py index c4c04a841c48a..9ebecdc3dc124 100644 --- a/tests/core/test_providers_manager.py +++ b/tests/core/test_providers_manager.py @@ -189,6 +189,31 @@ class MyHook: ) assert provider_manager.connection_form_widgets['extra__test__my_param'].field == widget_field + def test_connection_field_behaviors_placeholders_prefix(self): + class MyHook: + conn_type = 'test' + + @classmethod + def get_ui_field_behaviour(cls): + return { + "hidden_fields": ['host', 'schema'], + "relabeling": {}, + "placeholders": {"abc": "hi", "extra__anything": "n/a", "password": "blah"}, + } + + provider_manager = ProvidersManager() + provider_manager._add_customized_fields( + package_name='abc', + hook_class=MyHook, + customized_fields=MyHook.get_ui_field_behaviour(), + ) + expected = { + "extra__test__abc": "hi", # prefix should be added, since `abc` is not reserved + "extra__anything": "n/a", # no change since starts with extra + "password": "blah", # no change since it's a conn attr + } + assert provider_manager.field_behaviours['test']['placeholders'] == expected + def test_connection_form_widgets_fields_order(self): """Check that order of connection for widgets preserved by original Hook order.""" test_conn_type = 'test'