You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2023/06/30 18:51:21 UTC
[GitHub] [airflow] amoghrajesh opened a new pull request, #32292: Strip whitespaces from airflow connections form
amoghrajesh opened a new pull request, #32292:
URL: https://github.com/apache/airflow/pull/32292
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
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](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1627706815
@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
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1623798932
@hussein-awala can you take a look at this PR once you have some time?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1255276863
##########
tests/www/views/test_views_connection.py:
##########
@@ -58,16 +58,23 @@ def test_create_connection(admin_client, session):
_check_last_log(session, dag_id=None, event="connection.create", execution_date=None)
-def test_invalid_connection_id_trailing_blanks(admin_client, session):
- invalid_conn_id = "conn_id_with_trailing_blanks "
- invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
- resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
+def test_connection_id_with_trailing_blanks(admin_client, session):
+ conn_id_with_blanks = "conn_id_with_trailing_blanks "
+
+ conn = Connection(
+ conn_id=conn_id_with_blanks,
+ conn_type="Google Cloud",
)
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add(conn)
+ session.commit()
+
+ response = {conn[0] for conn in session.query(Connection.conn_id).all()}
+ assert len(response) == 1
+ assert "conn_id_with_trailing_blanks" in list(response)[0]
+
Review Comment:
Thanks for the feedback. Pushing the changes to fix this
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1617505825
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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1252344839
##########
tests/www/views/test_views_connection.py:
##########
@@ -62,11 +62,8 @@ def test_invalid_connection_id_trailing_blanks(admin_client, session):
invalid_conn_id = "conn_id_with_trailing_blanks "
invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
- )
+ # all the whitespaces get stripped off
+ check_content_in_response("Added Row", resp)
Review Comment:
You dont seem to check the blanks have actually been stripped? Also, the test name needs to be updated
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1253198344
##########
tests/www/views/test_views_connection.py:
##########
@@ -62,11 +62,8 @@ def test_invalid_connection_id_trailing_blanks(admin_client, session):
invalid_conn_id = "conn_id_with_trailing_blanks "
invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
- )
+ # all the whitespaces get stripped off
+ check_content_in_response("Added Row", resp)
Review Comment:
You can fetch connections the same way it is done in the test `test_duplicate_connection` in this file:
```python
response = {conn[0] for conn in session.query(Connection.conn_id).all()}
```
By doing this you fetch all connections, you just need then to check the connection `conn_id_with_trailing_blanks` (with to blanks) is 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1625308960
@hussein-awala addressed your concerns here. Can you take a look when you have some time?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1623076509
@vincbeck I think I was able to do it. Can you have a look again when you have some time?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1254034786
##########
airflow/www/forms.py:
##########
@@ -204,6 +204,12 @@ def _iter_connection_types() -> Iterator[tuple[str, str]]:
yield (connection_type, provider_info.hook_name)
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):
Review Comment:
```suggestion
if isinstance(getattr(field, "data", None), str):
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1617661233
> Yeah to me too @pankajkoti i think it will be nice to hear what others have to say about it.
>
> 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 :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1250354344
##########
airflow/www/forms.py:
##########
@@ -220,9 +220,9 @@ class ConnectionForm(DynamicForm):
),
)
description = StringField(lazy_gettext("Description"), widget=BS3TextAreaFieldWidget())
- host = StringField(lazy_gettext("Host"), widget=BS3TextFieldWidget())
+ 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])
Review 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 :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1254072035
##########
airflow/www/forms.py:
##########
@@ -220,9 +220,9 @@ class ConnectionForm(DynamicForm):
),
)
description = StringField(lazy_gettext("Description"), widget=BS3TextAreaFieldWidget())
- host = StringField(lazy_gettext("Host"), widget=BS3TextFieldWidget())
+ 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])
Review 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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1255789477
##########
tests/www/views/test_views_connection.py:
##########
@@ -59,14 +59,25 @@ def test_create_connection(admin_client, session):
def test_invalid_connection_id_trailing_blanks(admin_client, session):
Review Comment:
Good catch. Made changes
##########
tests/www/views/test_views_connection.py:
##########
@@ -59,14 +59,25 @@ def test_create_connection(admin_client, session):
def test_invalid_connection_id_trailing_blanks(admin_client, session):
- invalid_conn_id = "conn_id_with_trailing_blanks "
- invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
- resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
- )
+ conn_id_with_blanks = "conn_id_with_trailing_blanks "
+ conn = {**CONNECTION, "conn_id": conn_id_with_blanks}
+ resp = admin_client.post("/connection/add", data=conn, follow_redirects=True)
+ check_content_in_response("Added Row", resp)
+
+ response = {conn[0] for conn in session.query(Connection.conn_id).all()}
+ assert len(response) == 1
+ assert "conn_id_with_trailing_blanks" == list(response)[0]
+
+
+def test_invalid_connection_id_leading_blanks(admin_client, session):
Review Comment:
Made the changes
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1625384148
@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?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1254072035
##########
airflow/www/forms.py:
##########
@@ -220,9 +220,9 @@ class ConnectionForm(DynamicForm):
),
)
description = StringField(lazy_gettext("Description"), widget=BS3TextAreaFieldWidget())
- host = StringField(lazy_gettext("Host"), widget=BS3TextFieldWidget())
+ 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])
Review Comment:
It shouldn’t, since no spaces are technically allowed in any of the text fields anyway (except descriptions). It would be a bug elsewhere if a connection requires a space to work in any of those fields.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala merged pull request #32292: Strip whitespaces from airflow connections form
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #32292:
URL: https://github.com/apache/airflow/pull/32292
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1252574645
##########
tests/www/views/test_views_connection.py:
##########
@@ -62,11 +62,8 @@ def test_invalid_connection_id_trailing_blanks(admin_client, session):
invalid_conn_id = "conn_id_with_trailing_blanks "
invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
- )
+ # all the whitespaces get stripped off
+ check_content_in_response("Added Row", resp)
Review Comment:
Sure I will make the changes for test name.
@vincbeck I am unable to figure out how to check if the blanks have been stripped off using unit tests..
Any hint would be really 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1617388834
@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
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32292:
URL: https://github.com/apache/airflow/pull/32292#issuecomment-1617651141
Yeah to me too
@pankajkoti i think it will be nice to hear what others have to say about it.
IMO, this is followed by applications historcially
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1250256796
##########
airflow/www/forms.py:
##########
@@ -220,9 +220,9 @@ class ConnectionForm(DynamicForm):
),
)
description = StringField(lazy_gettext("Description"), widget=BS3TextAreaFieldWidget())
- host = StringField(lazy_gettext("Host"), widget=BS3TextFieldWidget())
+ 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])
Review 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?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1254911180
##########
tests/www/views/test_views_connection.py:
##########
@@ -58,16 +58,23 @@ def test_create_connection(admin_client, session):
_check_last_log(session, dag_id=None, event="connection.create", execution_date=None)
-def test_invalid_connection_id_trailing_blanks(admin_client, session):
- invalid_conn_id = "conn_id_with_trailing_blanks "
- invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
- resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
+def test_connection_id_with_trailing_blanks(admin_client, session):
+ conn_id_with_blanks = "conn_id_with_trailing_blanks "
+
+ conn = Connection(
+ conn_id=conn_id_with_blanks,
+ conn_type="Google Cloud",
)
+ with create_session() as session:
+ session.query(Connection).delete()
+ session.add(conn)
+ session.commit()
+
+ response = {conn[0] for conn in session.query(Connection.conn_id).all()}
+ assert len(response) == 1
+ assert "conn_id_with_trailing_blanks" in list(response)[0]
+
Review Comment:
How creating a connection via the Connection model could test test connection form?
IMO this test is useless, you need to create a connection using the connection form, to do that you can sens a post request to the route `/connection/add` as we did in the test you are replacing...
Another remark, you **should** use `=` when you can in the assert statements, where your current is the following:
```python
assert "conn_id_with_trailing_blanks" in "conn_id_with_trailing_blanks "
```
which is `True`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1248883699
##########
airflow/www/forms.py:
##########
@@ -220,9 +220,9 @@ class ConnectionForm(DynamicForm):
),
)
description = StringField(lazy_gettext("Description"), widget=BS3TextAreaFieldWidget())
- host = StringField(lazy_gettext("Host"), widget=BS3TextFieldWidget())
+ 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])
Review 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 :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1254043250
##########
airflow/www/forms.py:
##########
@@ -204,6 +204,12 @@ def _iter_connection_types() -> Iterator[tuple[str, str]]:
yield (connection_type, provider_info.hook_name)
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):
Review Comment:
Good suggestion. Optimises the code readability.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #32292: Strip whitespaces from airflow connections form
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #32292:
URL: https://github.com/apache/airflow/pull/32292#discussion_r1255721499
##########
tests/www/views/test_views_connection.py:
##########
@@ -59,14 +59,25 @@ def test_create_connection(admin_client, session):
def test_invalid_connection_id_trailing_blanks(admin_client, session):
- invalid_conn_id = "conn_id_with_trailing_blanks "
- invalid_connection = {**CONNECTION, "conn_id": invalid_conn_id}
- resp = admin_client.post("/connection/add", data=invalid_connection, follow_redirects=True)
- check_content_in_response(
- f"The key '{invalid_conn_id}' has to be made of alphanumeric characters, "
- + "dashes, dots and underscores exclusively",
- resp,
- )
+ conn_id_with_blanks = "conn_id_with_trailing_blanks "
+ conn = {**CONNECTION, "conn_id": conn_id_with_blanks}
+ resp = admin_client.post("/connection/add", data=conn, follow_redirects=True)
+ check_content_in_response("Added Row", resp)
+
+ response = {conn[0] for conn in session.query(Connection.conn_id).all()}
+ assert len(response) == 1
+ assert "conn_id_with_trailing_blanks" == list(response)[0]
+
+
+def test_invalid_connection_id_leading_blanks(admin_client, session):
Review Comment:
same here
```suggestion
def test_connection_id_leading_blanks(admin_client, session):
```
##########
tests/www/views/test_views_connection.py:
##########
@@ -59,14 +59,25 @@ def test_create_connection(admin_client, session):
def test_invalid_connection_id_trailing_blanks(admin_client, session):
Review Comment:
the connection id is not invalid anymore, we should rename the test:
```suggestion
def test_connection_id_trailing_blanks(admin_client, session):
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org