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 2021/01/10 12:00:59 UTC
[GitHub] [airflow] soltanianalytics opened a new issue #13597: Extra field widgets of custom connections do not properly save data
soltanianalytics opened a new issue #13597:
URL: https://github.com/apache/airflow/issues/13597
**Apache Airflow version**: 2.0.0
**Environment**: Docker image `apache/airflow:2.0.0-python3.8` on Win10 with WSL
**What happened**:
Ich built a custom provider with a number of custom connections.
This works:
- The connections are properly registered
- The UI does not show hidden fields as per `get_ui_field_behaviour`
- The UI correctly relabels fields as per `get_ui_field_behaviour`
- The UI correctly shows added widgets as per `get_connection_form_widgets` (well, mostly)
What does not work:
- The UI does not save values entered for additional widgets
I used the [JDBC example](https://github.com/apache/airflow/blob/master/airflow/providers/jdbc/hooks/jdbc.py) to string myself along by copying it and pasting it as a hook into my custom provider package. (I did not install the JDBC provider package, unless it is installed in the image I use - but if I don't add it in my own provider package, I don't have the connection type in the UI, so I assume it is not). Curiously, The JDBC hook works just fine. I then created the following file:
```Python
"""
You find two child classes of DbApiHook in here. One is the exact copy of the JDBC provider hook, minus some irrelevant logc (I only care about the UI stuff here). The other is the exact same thing, except I added an "x" behind every occurance of "jdbc" in strings and names.
"""
from typing import Any, Dict, Optional
from airflow.hooks.dbapi import DbApiHook
# from airflow.models.connection import Connection
class JdbcXHook(DbApiHook):
"""
Copy of JdbcHook below. Added an "x" at various places, including the class name.
"""
conn_name_attr = 'jdbcx_conn_id' # added x
default_conn_name = 'jdbcx_default' # added x
conn_type = 'jdbcx' # added x
hook_name = 'JDBCx Connection' # added x
supports_autocommit = True
@staticmethod
def get_connection_form_widgets() -> Dict[str, Any]:
"""Returns connection widgets to add to connection form"""
from flask_appbuilder.fieldwidgets import BS3TextFieldWidget
from flask_babel import lazy_gettext
from wtforms import StringField
# added an x in the keys
return {
"extra__jdbcx__drv_path": StringField(lazy_gettext('Driver Path'), widget=BS3TextFieldWidget()),
"extra__jdbcx__drv_clsname": StringField(
lazy_gettext('Driver Class'), widget=BS3TextFieldWidget()
),
}
@staticmethod
def get_ui_field_behaviour() -> Dict:
"""Returns custom field behaviour"""
return {
"hidden_fields": ['port', 'schema', 'extra'],
"relabeling": {'host': 'Connection URL'},
}
class JdbcHook(DbApiHook):
"""
General hook for jdbc db access.
JDBC URL, username and password will be taken from the predefined connection.
Note that the whole JDBC URL must be specified in the "host" field in the DB.
Raises an airflow error if the given connection id doesn't exist.
"""
conn_name_attr = 'jdbc_conn_id'
default_conn_name = 'jdbc_default'
conn_type = 'jdbc'
hook_name = 'JDBC Connection plain'
supports_autocommit = True
@staticmethod
def get_connection_form_widgets() -> Dict[str, Any]:
"""Returns connection widgets to add to connection form"""
from flask_appbuilder.fieldwidgets import BS3TextFieldWidget
from flask_babel import lazy_gettext
from wtforms import StringField
return {
"extra__jdbc__drv_path": StringField(lazy_gettext('Driver Path'), widget=BS3TextFieldWidget()),
"extra__jdbc__drv_clsname": StringField(
lazy_gettext('Driver Class'), widget=BS3TextFieldWidget()
),
}
@staticmethod
def get_ui_field_behaviour() -> Dict:
"""Returns custom field behaviour"""
return {
"hidden_fields": ['port', 'schema', 'extra'],
"relabeling": {'host': 'Connection URL'},
}
```
**What you expected to happen**:
After doing the above, I expected
- Seeing both in the add connection UI
- Being able to use both the same way
What actually happenes
- I _do_ see both in the UI (Screenshot 1)
- For some reason, the "normal" hook has BOTH extra fields - not just his own two? (Screenshot 2)
- If I add the connection as in Screenshot 2, they are saved in the four fields (his own two + the two for the "x" hook) properly as shown in Screenshot 3
- If I seek to edit the connection again, they are also they - all four fields - with the correct values in the UI
- If I add the connection for the "x" type as in Screenshot 4, it ostensibly saves it - with two fields as defined in the code
- You can see in screenshot 5, that the extra is saved as an empty string?!
- When trying to edit the connection in the UI, you also see that there is no data saved for two extra widgets?!
- I added a few more screenshots of airflow providers CLI command results (note that the package `ewah` has a number of other custom hooks, and the issue above occurs for *all* of them)
*Screenshot 1:*
*Screenshot 2:*
*Screenshot 3:*
*Screenshot 4:*
*Screenshot 5:*
*Screenshot 6 - airflow providers behaviours:*
*Screenshot 7 - airflow providers get:*
(Note: This error occurs with pre-installed providers as well)
*Screenshot 8 - airflow providers hooks:*
*Screenshot 9 - aorflow providers list:*
*Screenshot 10 - airflow providers widgets:*
**How to reproduce it**:
- create a custom provider package
- add the code snippet pasted above somewhere
- add the two classes to the `hook-class-names` list in the provider info
- install the provider package
- do what I described above
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-758852411
Hey @soltanianalytics - I implemented fixes for all the problem in #13640. I tested it for built-in providers and I have a reason to believe they should also work for custom ones.
Would it be possible that you apply those changes in your installation and see if they work es expected ?
I'd greatly appreciate it.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-757507757
(Note - the javascript was there in 1.10 but we simply did not have such 'overlapping' connection names :)
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk edited a comment on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-757507585
Can you try one more thing ? I think I know what could be the problem. The problem is lilkely in the javascript which might not be ready for having two connections where name of one connection (jdbc) is substring of the other (jdbcx). Could you please change the name of the `jdbcx` to `jdxbc` everywhere ? That could help us to narrow-down the problem and then we can likely find a fix for that.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-757507066
Interesting. The 'version' error has already been fixed in master for the upcoming 2.0.1 (merged 2 days ago).
https://github.com/apache/airflow/commit/f49f36b6a0c1332e141200d1a0c900991b9cdaef#diff-c56df299b30e60d690494057a4e8721d36406c0cca266961ff2ae6504993c8cbL159
I do not think it was causing the problem but maybe installing master version and trying out might be something you could try before I dig deeper @soltanianalytics ?
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-758831582
OK. Sorry I have not seen your answer - good that we know about the duplication. That should be easy fix :).
And I think I know where the problem is. There is one more place I missed in `views.py`:
```
def process_form(self, form, is_created):
"""Process form data."""
formdata = form.data
if formdata['conn_type'] in ['jdbc', 'google_cloud_platform', 'grpc', 'yandexcloud', 'kubernetes']:
extra = {key: formdata[key] for key in self.extra_fields if key in formdata}
form.extra.data = json.dumps(extra)
```
Both are to be fixed in 2.0.1 then !
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk edited a comment on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-758832597
Thanks for being so thorough!. You can test it by locally modifying the `airflow/www/views.py` and adding the connection_type there !
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-757507585
Can you try one more thing ? I think I know what could be the problem. The problem is lilkely in the javascript which might not be ready for having two connections where name of one connection (jdbc) is substring of the other (jdbcx). Could you please change the name of the `jdbcx` to `jdxbc` ? That could help us to narrow-down the problem and then we can likely find a fix for that.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-758837279
BTW. I realized that the original implementation is somewhat wrong as it stores all the extras from all the connection in extra of any connection (even if it is empty). Not nice :).
----------------------------------------------------------------
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
[GitHub] [airflow] soltanianalytics commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-757524098
Can you point me at where the extra is saved to the metadata database? It seems to me that the issue may be arising there. Saving non-extra fields (login, password, ...) works just fine. It's only the extra fields that don't work because for some reason, it saves an empty string instead of the full extra. It seems that when it is working correctly, the extra saved a dict with values for _all_ custom widgets, no matter what the connection is - and this fails for some reason for my custom connection types, except for the copied jdbc one, strangely enough.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk edited a comment on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-758852411
Hey @soltanianalytics - I implemented fixes for all the problems in #13640. I tested it for built-in providers and I have a reason to believe they should also work for custom ones.
Would it be possible that you apply those changes in your installation and see if they work es expected ?
I'd greatly appreciate it.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-758832597
Thanks for being so thorough!. You can test it by locally modifying the `airflow/www/views.py` and adding the connection_form there !
----------------------------------------------------------------
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
[GitHub] [airflow] soltanianalytics commented on issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on issue #13597:
URL: https://github.com/apache/airflow/issues/13597#issuecomment-757520579
The naming change seems to fix the duplicate fields issue:
However, this does not fix that the extra is an empty string for the "x" version:
The extra is also not encrypted in the metadata db, if that information helps.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk closed issue #13597: Extra field widgets of custom connections do not properly save data
Posted by GitBox <gi...@apache.org>.
potiuk closed issue #13597:
URL: https://github.com/apache/airflow/issues/13597
----------------------------------------------------------------
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