You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/12/06 16:03:00 UTC
[GitHub] [airflow] ashb commented on a change in pull request #12837: Customizable javascript configuration in connnection form
ashb commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537057669
##########
File path: airflow/hooks/base_hook.py
##########
@@ -18,14 +18,82 @@
"""Base class for all hooks"""
import logging
import warnings
-from typing import Any, List
+from typing import Any, Dict, List
+
+from wtforms import Field
from airflow.models.connection import Connection
+from airflow.typing_compat import Protocol
from airflow.utils.log.logging_mixin import LoggingMixin
log = logging.getLogger(__name__)
+class DiscoverableHook(Protocol):
+ """
+ This protocol should be implemented by Hook classes that want to be discovered by ProvidersManager
+
+
+ The conn_name_attr, default_conn_name, conn_type should be implemented by those
+ Hooks that want to be automatically mapped from the connection_type -> Hook when get_hook method
+ is called with connection_type.
+
+ Additional - hook_name should be set when you want the hook to have a custom name in the UI selection
Review comment:
```suggestion
Additionally hook_name should be set when you want the hook to have a custom name in the UI selection
```
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
`.text()` on a hidden input returns nothing. You wanted `.val()`.
##########
File path: airflow/providers/cloudant/hooks/cloudant.py
##########
@@ -37,6 +40,17 @@ class CloudantHook(BaseHook):
conn_type = 'cloudant'
hook_name = 'Cloudant'
+ @staticmethod
+ def get_custom_fields() -> Dict:
+ return {
+ "hidden_fields": ['port', 'extra'],
+ "relabeling": {'host': 'Account', 'login': 'Username (or API Key)', 'schema': 'Database'},
+ }
+
+ @staticmethod
+ def get_connection_form_widgets() -> Dict[str, Field]:
Review comment:
So in cncf.kube.hooks.kube you don't implement this method, but here you do.
Do we _need_ to have both methods? Lets be consistent, and either remove this here, or add it so that when we have one we have both.
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -109,6 +111,17 @@ class SparkSubmitHook(BaseHook, LoggingMixin):
conn_type = 'spark'
hook_name = 'Spark'
+ @staticmethod
+ def get_connection_form_widgets() -> Dict[str, Field]:
+ return {}
+
+ @staticmethod
+ def get_custom_fields() -> Dict:
Review comment:
How about `get_ui_field_behaviour` as a name?
`custom_fields` sort of reads like this could return WTForms.Field instances, but that is not what we are doing here.
##########
File path: airflow/www/forms.py
##########
@@ -213,6 +215,10 @@ def _get_connection_types():
return _connection_types
+def _get_field_customizations() -> Dict[str, Dict]:
+ return ProvidersManager().field_customizations
Review comment:
Single use one line function -- why not "inline" it direct?
##########
File path: airflow/www/forms.py
##########
@@ -230,14 +236,14 @@ class ConnectionForm(DynamicForm):
port = IntegerField(lazy_gettext('Port'), validators=[Optional()], widget=BS3TextFieldWidget())
extra = TextAreaField(lazy_gettext('Extra'), widget=BS3TextAreaFieldWidget())
- # You should add connection form fields in the hooks via get_connection_form_widgets() method
- # See for example airflow/providers/jdbc/hooks/jdbc.py
- # It is esed to customized the form, the forms elements get rendered
- # and results are stored in the extra field as json. All of these
- # need to be prefixed with extra__ and then the conn_type ___ as in
- # extra__{conn_type}__name. You can also hide form elements and rename
- # others from the connection_form.js file
+ # Used to store a dictionary of field customizations used to dynamically change available
+ # fields in ConnectionForm based on type of connection chosen
+ field_parameters = HiddenField(
+ lazy_gettext('field_parameters'), default=json.dumps(_get_field_customizations())
+ )
Review comment:
Making this a field means this gets submitted back to the server which isn't needed.
Please find another way to get this in to the front end.
##########
File path: airflow/hooks/base_hook.py
##########
@@ -18,14 +18,82 @@
"""Base class for all hooks"""
import logging
import warnings
-from typing import Any, List
+from typing import Any, Dict, List
+
+from wtforms import Field
from airflow.models.connection import Connection
+from airflow.typing_compat import Protocol
from airflow.utils.log.logging_mixin import LoggingMixin
log = logging.getLogger(__name__)
+class DiscoverableHook(Protocol):
+ """
+ This protocol should be implemented by Hook classes that want to be discovered by ProvidersManager
+
+
+ The conn_name_attr, default_conn_name, conn_type should be implemented by those
+ Hooks that want to be automatically mapped from the connection_type -> Hook when get_hook method
+ is called with connection_type.
+
+ Additional - hook_name should be set when you want the hook to have a custom name in the UI selection
+ Name. If not specified, conn_name will be used.
+
+ The "get_custom_fields" and "get_connection_form_widgets" are optional - override them if you want
+ to customize the Connection Form screen. You can add extra widgets to parse your extra fields via the
+ get_connection_form_widgets method as well as hide or relabel the fields or pre-fill
+ them with placeholders via get_custom_fields method.
+
+ Note that the "get_custom_fields" and "get_connection_form_widgets" need to be set by each class in the
+ class hierarchy in order to apply widget customizations.
Review comment:
Add;
```rst
For example, even if you want to use the fields from your parent class, you must explicitly have a method on *your* class:
.. code-block:: python
@classmethod
def get_custom_fields(cls):
return super().get_custom_fields()
```
##########
File path: airflow/providers/cloudant/hooks/cloudant.py
##########
@@ -16,13 +16,16 @@
# specific language governing permissions and limitations
# under the License.
"""Hook for Cloudant"""
+from typing import Dict
+
from cloudant import cloudant
+from wtforms import Field
Review comment:
I really don't like importing just for types. _Especially_ as we return `{}`
Please use `typing.TYPE_CHECKING` here.
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -303,7 +303,7 @@ def web_identity_token_loader():
return web_identity_token_loader
-class AwsBaseHook(BaseHook):
+class AwsBaseHook(BaseHook, DiscoverableHook):
Review comment:
Three things:
1. Protocols are not _normally_ explicit base classes -- they are more normally used as "duck-typing" only. Check out `airflow.models.crypto.FernetProtocol` -- it is not a subclass, but just a type.
2. This class _doesn't_ implement the discoverable protocol, it doesn't have `get_connection_form_widgets` or `get_custom_fields` methods.
Additionally a result of the sublclassing, the type is wrong.
```python console
>>> from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
>>> AwsBaseHook.get_custom_fields
<function airflow.hooks.base_hook.DiscoverableHook.get_custom_fields() -> Dict>
>>> AwsBaseHook.get_custom_fields()
>>> type(AwsBaseHook.get_custom_fields())
NoneType
```
(Yes, the view wouldn't use this because of the checks I know.)
My preferred solution to this would be to never inherit from the protocol.
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -39,6 +40,20 @@ class DockerHook(BaseHook, LoggingMixin):
conn_type = 'docker'
hook_name = 'Docker Registry'
+ @staticmethod
+ def get_custom_fields() -> Dict:
+ return {
+ "hidden_fields": ['port', 'schema'],
Review comment:
But we use port:
```
if conn.port:
self.__registry = f"{conn.host}:{conn.port}"
```
##########
File path: airflow/providers_manager.py
##########
@@ -289,6 +307,28 @@ def _add_widgets(self, package_name: str, hook_class: type, widgets: Dict[str, F
hook_class.__name__, package_name, field
)
+ 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 in self._field_customizations:
+ log.warning(
+ "The connection_type %s from package %s and class %s has already been added "
+ "by another provider. Ignoring it.",
+ connection_type,
+ package_name,
+ hook_class.__name__,
+ )
+ return
+ self._field_customizations[connection_type] = customized_fields
+ except Exception as e: # noqa pylint: disable=broad-except
+ log.warning(
+ "Error when loading customized fields from package '%s' " "hook class '%s': %s",
Review comment:
```suggestion
"Error when loading customized fields from package '%s' hook class '%s': %s",
```
##########
File path: airflow/providers_manager.py
##########
@@ -325,12 +365,17 @@ def hooks(self) -> Dict[str, HookInfo]:
"""Returns dictionary of connection_type-to-hook mapping"""
return self._hooks_dict
- @property
- def extra_links_class_names(self):
- """Returns set of extra link class names."""
- return sorted(list(self._extra_link_class_name_set))
-
@property
def connection_form_widgets(self) -> Dict[str, ConnectionFormWidgetInfo]:
"""Returns widgets for connection forms."""
return self._connection_form_widgets
+
+ @property
+ def field_customizations(self) -> Dict[str, Dict]:
+ """Returns dictionary with field customizations for connection types."""
+ return self._field_customizations
+
+ @property
+ def extra_links_class_names(self):
+ """Returns set of extra link class names."""
+ return sorted(list(self._extra_link_class_name_set))
Review comment:
```suggestion
return sorted(self._extra_link_class_name_set)
```
##########
File path: airflow/providers_manager.py
##########
@@ -244,7 +256,13 @@ def _add_hook(self, hook_class_name: str, provider_package: str) -> None:
# 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()
- self._add_widgets(provider_package, hook_class, widgets)
+ if len(widgets.keys()) > 0:
+ self._add_widgets(provider_package, hook_class, widgets)
+ if 'get_custom_fields' in hook_class.__dict__:
+ customized_fields = hook_class.get_custom_fields()
+ if len(customized_fields.keys()) > 0:
Review comment:
```suggestion
if customized_fields:
```
##########
File path: airflow/www/forms.py
##########
@@ -230,14 +236,14 @@ class ConnectionForm(DynamicForm):
port = IntegerField(lazy_gettext('Port'), validators=[Optional()], widget=BS3TextFieldWidget())
extra = TextAreaField(lazy_gettext('Extra'), widget=BS3TextAreaFieldWidget())
- # You should add connection form fields in the hooks via get_connection_form_widgets() method
- # See for example airflow/providers/jdbc/hooks/jdbc.py
- # It is esed to customized the form, the forms elements get rendered
- # and results are stored in the extra field as json. All of these
- # need to be prefixed with extra__ and then the conn_type ___ as in
- # extra__{conn_type}__name. You can also hide form elements and rename
- # others from the connection_form.js file
+ # Used to store a dictionary of field customizations used to dynamically change available
+ # fields in ConnectionForm based on type of connection chosen
+ field_parameters = HiddenField(
+ lazy_gettext('field_parameters'), default=json.dumps(_get_field_customizations())
+ )
Review comment:
I think you can do this with a custom widget:
```python
class provider_field_widget(field, **kwargs):
return Markup('<script id="{}" type="text/json">{}</script>').format(
field.name,
json.dumps(ProvidersManager().field_customizations)
)
```
And then:
```suggestion
field_parameters = HiddenField(
lazy_gettext('field_parameters'), widget=provider_field_widget
)
```
(And `$("#field_parameters").text()` would continue to work)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org