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/30 16:22:06 UTC
[GitHub] [airflow] yjwong opened a new issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
yjwong opened a new issue #13985:
URL: https://github.com/apache/airflow/issues/13985
**Apache Airflow version**: 2.0.0 with the following patch: https://github.com/apache/airflow/pull/13640
**Kubernetes version (if you are using kubernetes)** (use `kubectl version`): N/A
**Environment**:
- **Cloud provider or hardware configuration**: AMD Ryzen 3900X (12C/24T), 64GB RAM
- **OS** (e.g. from /etc/os-release): Ubuntu 20.04.1 LTS
- **Kernel** (e.g. `uname -a`): 5.9.8-050908-generic
- **Install tools**: N/A
- **Others**: N/A
**What happened**:
If there are custom hooks that implement the `get_connection_form_widgets` method that return fields using the `InputRequired` validator, saving breaks for all types of connections on the "Edit Connections" page.
In Chrome, the following message is logged to the browser console:
```
An invalid form control with name='extra__hook_name__field_name' is not focusable.
```
This happens because the field is marked as `<input required>` but is hidden using CSS when the connection type exposed by the custom hook is not selected.
**What you expected to happen**:
Should be able to save other types of connections.
**How to reproduce it**:
Create a provider, and add a hook with something like:
```python
@staticmethod
def get_connection_form_widgets() -> Dict[str, Any]:
"""Returns connection widgets to add to connection form."""
return {
'extra__my_hook__client_id': StringField(
lazy_gettext('OAuth2 Client ID'),
widget=BS3TextFieldWidget(),
validators=[wtforms.validators.InputRequired()],
),
}
```
Go to the Airflow Web UI, click the "Add" button in the connection list page, then choose a connection type that's not the type exposed by the custom hook. Fill in details and click "Save".
**Anything else we need to know**: N/A
----------------------------------------------------------------
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] yjwong commented on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
yjwong commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-773795228
Flask-AppBuilder also runs validations for every field when it's submitted. I was not able to find a way to conditionally add/remove fields depending on the submitted `conn_type`, so that means that my PR alone would not work.
However, it's possible for a provider to get it to work using a validator that's conditional on the `conn_type`:
```
class InputRequired(wtforms.validators.InputRequired):
field_flags = ('required', )
def __call__(self, form, field):
conn_type = form.conn_type.data
if conn_type == MyHook.conn_type:
super().__call__(form, field)
```
----------------------------------------------------------------
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 #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-772873155
I see, yeah I see
----------------------------------------------------------------
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] yjwong commented on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
yjwong commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-773795228
Flask-AppBuilder also runs validations for every field when it's submitted. I was not able to find a way to conditionally add/remove fields depending on the submitted `conn_type`, so that means that my PR alone would not work.
However, it's possible for a provider to get it to work using a validator that's conditional on the `conn_type`:
```
class InputRequired(wtforms.validators.InputRequired):
field_flags = ('required', )
def __call__(self, form, field):
conn_type = form.conn_type.data
if conn_type == MyHook.conn_type:
super().__call__(form, field)
```
----------------------------------------------------------------
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] boring-cyborg[bot] commented on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-770237942
Thanks for opening your first issue here! Be sure to follow the issue template!
----------------------------------------------------------------
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] yjwong edited a comment on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
yjwong edited a comment on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-773795228
Flask-AppBuilder also runs validations for every field when it's submitted. I was not able to find a way to conditionally add/remove fields depending on the submitted `conn_type`, so that means that my PR alone would not work.
However, it's possible for a provider to get it to work using a validator that's conditional on the `conn_type`:
```python
class InputRequired(wtforms.validators.InputRequired):
field_flags = ('required', )
def __call__(self, form, field):
conn_type = form.conn_type.data
if conn_type == MyHook.conn_type:
super().__call__(form, field)
```
----------------------------------------------------------------
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 #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-770435358
> If I understand the way it currently works, the entire set of fields, regardless of the selected connection, is POSTed to the controller.
Yep. My understanding as well.
> Haven't read in detail yet, but if the controller can handle fields that are omitted in the POST request without negative side effects, `connection_form.js` could be modified to save the DOM nodes on initial load, then dynamically add/remove them when different connection types are selected.
Yep. my understing that this would work the way. Currently this code is rather simple (simply changing attributes of the fields to be visible/hidden). Another option would be to remove/restore `required` property of the "required" fields. Not sure how the controller would behave in this case, but likely this has even more chances of not having side effects and might be a bit simpler (no need to add the fields on submit() likely).
> Also worth noting that this is also an issue not just for required fields, but also where someone tries to return an `IntegerField` instead of a `StringField`.
Indeed. Those could be prefilled with 0s I guess (not perfect and I can imagine scenarios where such fields are visible, change, hidden and THEN submitted. Not sure how the controller would behave.
----------------------------------------------------------------
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 #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-770421661
I do not think 'InputRequired' is used by any built-in connection, that's why it has never been an issue.
I am not sure if we should implement it or simply disallow InputRequired (also DataRequired and Required I think). We cannot simply delete the field instead of hiding, because this is done dynamically in javascript by selecting connection type and we would have to move it out form/add back if necessary rather than delete.
This however has the potential of breaking the whole connection form by a "rogue" provider that can add such field so we would have to check it when we call the `get_connection_form_widgets()` method.
@ryanahamilton @ashb @kaxil - WDYT?
----------------------------------------------------------------
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] yjwong edited a comment on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
yjwong edited a comment on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-773795228
Flask-AppBuilder also runs validations for every field when it's submitted. I was not able to find a way to conditionally add/remove fields depending on the submitted `conn_type`, so that means that my PR alone would not work.
However, it's possible for a provider to get it to work using a validator that's conditional on the `conn_type`:
```python
class InputRequired(wtforms.validators.InputRequired):
field_flags = ('required', )
def __call__(self, form, field):
conn_type = form.conn_type.data
if conn_type == MyHook.conn_type:
super().__call__(form, field)
```
----------------------------------------------------------------
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] ryanahamilton closed issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
ryanahamilton closed issue #13985:
URL: https://github.com/apache/airflow/issues/13985
----------------------------------------------------------------
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] yjwong commented on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
yjwong commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-772816133
@potiuk I have a first draft PR above using the approach of adding/removing DOM elements, however, seems like the form still isn't being saved properly when some field has `InputRequired` (getting redirected back to the form itself). Will have to look into the server-side code - seems like there's some validation done there too.
----------------------------------------------------------------
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] yjwong commented on issue #13985: Can't save any connection if provider-provided connection form widgets have fields marked as InputRequired
Posted by GitBox <gi...@apache.org>.
yjwong commented on issue #13985:
URL: https://github.com/apache/airflow/issues/13985#issuecomment-770424026
Looking at `connection_form.js`, I can take a stab at supporting these. However, I'm currently limited by how much I understand how Flask-AppBuilder works.
If I understand the way it currently works, the entire set of fields, regardless of the selected connection, is POSTed to the controller.
Haven't read in detail yet, but if the controller can handle fields that are omitted in the POST request without negative side effects, `connection_form.js` could be modified to save the DOM nodes on initial load, then dynamically add/remove them when different connection types are selected. If there are negative side effects, we can also attach an `submit` event handler on the form to re-add the nodes right before submission.
Also worth noting that this is also an issue not just for required fields, but also where someone tries to return an `IntegerField` instead of a `StringField`.
----------------------------------------------------------------
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