Strip whitespaces from airflow connections form#32292
Conversation
hussein-awala
left a comment
hussein-awala
left a comment
There was a problem hiding this comment.
IMO we should add the strip processing to all the fields (including the fields added in the hooks classes) or none of them.
To make it easier an implicit, I think you can update the form class and override the process method, you can test something like:
class ConnectionForm(DynamicForm):
def process(self, formdata=None, obj=None, **kwargs):
super().process(formdata=formdata, obj=obj, **kwargs)
for field in self._fields.values():
if hasattr(field, 'data') and isinstance(field.data, str):
field.data = field.data.strip()
| host = StringField(lazy_gettext("Host"), widget=BS3TextFieldWidget(), filters=[strip_filter]) | ||
| schema = StringField(lazy_gettext("Schema"), widget=BS3TextFieldWidget()) | ||
| login = StringField(lazy_gettext("Login"), widget=BS3TextFieldWidget()) | ||
| login = StringField(lazy_gettext("Login"), widget=BS3TextFieldWidget(), filters=[strip_filter]) |
There was a problem hiding this comment.
I get the fact that most systems may not allow hosts to contain leading or trailing whitespaces, but thinking critically and loud here in the case of login, would some users like to have intentionally leading or trailing whitespaces? I am not sure, just wanted to discuss this once :)
There was a problem hiding this comment.
I dont think so, it is not a common practice to have trailing or leading spaces. In between the login name is usually accepted.
In my opinion, we can perform a strip and then if need be try to support leading/trailing if needed. WDYT?
There was a problem hiding this comment.
Well, I am not sure if stripping by default for all the fields would cause issues for someone's connections or not. If we're sure that it would not cause any issues then it is fine :)
There was a problem hiding this comment.
It shouldn’t, since no spaces are technically allowed in any of the text fields anyway (except description). It would be a bug elsewhere if a connection requires a space to work in any of those fields.
|
@hussein-awala tried the exact snippet provided by you and seems that things are working. Performed similar test as earlier - adding trailing and leading spaces to all the fields for a test connection. They get stripped when we save the connection |
|
Automatically stripping all fields makes sense to me. IIRC it is the default behaviour for Django for most (probably not all? I don’t quite remember) fields. |
|
Yeah to me too IMO, this is followed by applications historcially |
Sounds good @amoghrajesh I agree it is a nice change to have if we see no potential issues :) |
|
@vincbeck I think I was able to do it. Can you have a look again when you have some time? |
Co-authored-by: Tzu-ping Chung <[email protected]>
|
@hussein-awala can you take a look at this PR once you have some time? |
|
@hussein-awala addressed your concerns here. Can you take a look when you have some time? |
hussein-awala
left a comment
hussein-awala
left a comment
There was a problem hiding this comment.
The change made in the PR removes the trailing and leading blanks from all the connection fields, but the unit tests checks only the connection id field, could you add a check on the other fields? and I wonder if we can test with a custom widget configured in a test hook 🤔
|
@hussein-awala So, I added a new test case that adds a connection with all fields with spaces and then queries to see if the spaces were stripped or not. I think I covered breadth here. Can you take a look again? |
|
@hussein-awala following up for a review on this PR. I have addressed the comments, can you take a look at it when possible? Trying to get consensus on this one |
hussein-awala
left a comment
hussein-awala
left a comment
There was a problem hiding this comment.
I refactored the tests to improve their performance (a single query instead of multiple ones) and readability, I will merge once the CI is green
7 participants
PR intends to add filters to the UI fields to strip off the leading and trailing whitespaces which could be entered by mistake into the connection form.
Results:
Having spaces in the end for host:
Having leading spaces in start of username:
After saving, spaces trimmed off
closes: #25240
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.