jedcunningham commented on code in PR #22607:
URL: https://github.com/apache/airflow/pull/22607#discussion_r856522719
##########
newsfragments/22607.significant.rst:
##########
@@ -0,0 +1,5 @@
+Remove requirement that custom connection UI fields be prefixed
+
+Hooks can define custom connection fields their connection type by
implementing method ``get_connection_form_widgets``. These custom fields are
presented in the web UI as additional connection attributes, but internally
they are stored in the connection ``extra`` dict. For technical reasons,
previously custom field when stored in ``extra`` had to be named with a prefix
``extra__<conn type>__<field name>``. This had the consequence of making it
more cumbersome to define connections outside of the UI, since the prefix is
tougher to read and work with. With #22607, we make it so that you can now
define custom fields such that they can be read from and stored in ``extra``
without the prefix.
Review Comment:
```suggestion
Hooks can define custom connection fields for their connection type by
implementing method ``get_connection_form_widgets``. These custom fields are
presented in the web UI as additional connection attributes, but internally
they are stored in the connection ``extra`` dict. For technical reasons,
previously custom field when stored in ``extra`` had to be named with a prefix
``extra__<conn type>__<field name>``. This had the consequence of making it
more cumbersome to define connections outside of the UI, since the prefix is
tougher to read and work with. With #22607, we make it so that you can now
define custom fields such that they can be read from and stored in ``extra``
without the prefix.
```
##########
tests/www/views/test_views_connection.py:
##########
@@ -118,9 +199,19 @@ def test_process_form_extras():
cmv = ConnectionModelView()
cmv.extra_fields = ["extra__test4__custom_field"] # Custom field
+
+ # this is set by `lazy_add_provider_discovered_options_to_connection_form
Review Comment:
```suggestion
# this is set by
`lazy_add_provider_discovered_options_to_connection_form`
```
##########
tests/core/test_providers_manager.py:
##########
@@ -142,6 +145,49 @@ def test_connection_form_widgets(self):
connections_form_widgets =
list(provider_manager.connection_form_widgets.keys())
assert len(connections_form_widgets) > 29
+ @pytest.mark.parametrize(
+ 'scenario',
+ [
+ 'prefix',
+ 'no_prefix',
+ 'both_1',
+ 'both_2',
+ ],
+ )
+ def test_connection_form__add_widgets_prefix_backcompat(self, scenario):
+ """
+ When field name prefixed, the field name should be used as is.
Review Comment:
```suggestion
When the field name is prefixed, it should be used as is.
```
##########
newsfragments/22607.significant.rst:
##########
@@ -0,0 +1,5 @@
+Remove requirement that custom connection UI fields be prefixed
+
+Hooks can define custom connection fields their connection type by
implementing method ``get_connection_form_widgets``. These custom fields are
presented in the web UI as additional connection attributes, but internally
they are stored in the connection ``extra`` dict. For technical reasons,
previously custom field when stored in ``extra`` had to be named with a prefix
``extra__<conn type>__<field name>``. This had the consequence of making it
more cumbersome to define connections outside of the UI, since the prefix is
tougher to read and work with. With #22607, we make it so that you can now
define custom fields such that they can be read from and stored in ``extra``
without the prefix.
+
+To enable this, update the dict returned by the
``get_connection_form_widgets`` to remove the prefix from the keys.
Internally, the providers manager will still use a prefix to ensure each custom
field is globally unique, but the absence of a prefix in the returned widget
dict will signal to the Web UI to read and store custom fields without the
prefix. Note that this is only a change to the Web UI behavior; when updating
your hook in this way, you must make sure that when you *hook* reads the
``extra`` field, it will also check for the prefixed value for backward
compatibility.
Review Comment:
```suggestion
To enable this, update the dict returned by the
``get_connection_form_widgets`` method to remove the prefix from the keys.
Internally, the providers manager will still use a prefix to ensure each custom
field is globally unique, but the absence of a prefix in the returned widget
dict will signal to the Web UI to read and store custom fields without the
prefix. Note that this is only a change to the Web UI behavior; when updating
your hook in this way, you must make sure that when your *hook* reads the
``extra`` field, it will also check for the prefixed value for backward
compatibility.
```
##########
tests/www/views/test_views_connection.py:
##########
@@ -51,16 +53,54 @@ def test_create_connection(admin_client):
def test_prefill_form_null_extra():
mock_form = mock.Mock()
- mock_form.data = {"conn_id": "test", "extra": None}
+ mock_form.data = {"conn_id": "test", "extra": None, "conn_type": "test"}
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
-def test_process_form_extras():
[email protected](
+ 'extras, expected',
+ [
+ param({"extra__test__my_param": "this_val"}, "this_val",
id='conn_not_upgraded'),
+ param({"my_param": "my_val"}, "my_val", id='conn_upgraded'),
+ param(
+ {"extra__test__my_param": "this_val", "my_param": "my_val"},
+ "my_val",
+ id='conn_upgraded_old_val_present',
+ ),
+ ],
+)
+def test_prefill_form_backcompat(extras, expected):
+ """
+ When populating custom fields in connection form we should first check for
the non-prefixed
+ value (since prefixes in extra are deprecated) and then fallback to the
prefixed value.
+
+ Either way, the field is known internally to the model view as the
prefixed value.
+ """
+ mock_form = mock.Mock()
+ mock_form.data = {"conn_id": "test", "extra": json.dumps(extras),
"conn_type": "test"}
+ cmv = ConnectionModelView()
+ cmv.extra_fields = ['extra__test__my_param']
+
+ # this is set by `lazy_add_provider_discovered_options_to_connection_form
Review Comment:
```suggestion
# this is set by
`lazy_add_provider_discovered_options_to_connection_form`
```
##########
tests/www/views/test_views_connection.py:
##########
@@ -51,16 +53,54 @@ def test_create_connection(admin_client):
def test_prefill_form_null_extra():
mock_form = mock.Mock()
- mock_form.data = {"conn_id": "test", "extra": None}
+ mock_form.data = {"conn_id": "test", "extra": None, "conn_type": "test"}
cmv = ConnectionModelView()
cmv.prefill_form(form=mock_form, pk=1)
-def test_process_form_extras():
[email protected](
+ 'extras, expected',
+ [
+ param({"extra__test__my_param": "this_val"}, "this_val",
id='conn_not_upgraded'),
+ param({"my_param": "my_val"}, "my_val", id='conn_upgraded'),
+ param(
+ {"extra__test__my_param": "this_val", "my_param": "my_val"},
+ "my_val",
+ id='conn_upgraded_old_val_present',
+ ),
+ ],
+)
+def test_prefill_form_backcompat(extras, expected):
+ """
+ When populating custom fields in connection form we should first check for
the non-prefixed
Review Comment:
```suggestion
When populating custom fields in the connection form we should first
check for the non-prefixed
```
##########
tests/www/views/test_views_connection.py:
##########
@@ -72,14 +112,27 @@ def test_process_form_extras():
}
cmv = ConnectionModelView()
+
+ # this is set by `lazy_add_provider_discovered_options_to_connection_form
Review Comment:
```suggestion
# this is set by
`lazy_add_provider_discovered_options_to_connection_form`
```
##########
tests/www/views/test_views_connection.py:
##########
@@ -103,11 +169,26 @@ def test_process_form_extras():
cmv = ConnectionModelView()
cmv.extra_fields = ["extra__test3__custom_field"] # Custom field
+
+ # this is set by `lazy_add_provider_discovered_options_to_connection_form
Review Comment:
```suggestion
# this is set by
`lazy_add_provider_discovered_options_to_connection_form`
```
##########
tests/core/test_providers_manager.py:
##########
@@ -142,6 +145,49 @@ def test_connection_form_widgets(self):
connections_form_widgets =
list(provider_manager.connection_form_widgets.keys())
assert len(connections_form_widgets) > 29
+ @pytest.mark.parametrize(
+ 'scenario',
+ [
+ 'prefix',
+ 'no_prefix',
+ 'both_1',
+ 'both_2',
+ ],
+ )
+ def test_connection_form__add_widgets_prefix_backcompat(self, scenario):
+ """
+ When field name prefixed, the field name should be used as is.
+ When not prefixed, we should add the prefix defived using the conn
type.
Review Comment:
I'm not sure what "defived" means?
--
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]
