You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "bugraoz93 (via GitHub)" <gi...@apache.org> on 2024/10/26 11:02:05 UTC
[PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
bugraoz93 opened a new pull request, #43396:
URL: https://github.com/apache/airflow/pull/43396
<!--
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/
-->
closes: #42593
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1822739405
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
This test is not correct. This will only assert that "password" key is not in the response dict this is not enough. We actually want password and `extra` in the response, just the values should be redacted.
You need to check that the response has `"password": "***"` and `"extra": {"password": "***"}`
Also you might be missing a `@pytest.mark.enable_redact` in your test.
Cf `variables`.
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43396:
URL: https://github.com/apache/airflow/pull/43396#issuecomment-2442499350
> Should we handle the `password` as well ? How do we plan to set the connection password through the rest api ?
>
> (And check that in the response of the POST it is properly redacted).
>
> ATM this is done trough FAB in the `Adming/Connection` form that allow the `password` field. But this will be removed in airflow 3.0 so we should handle it in the rest API. Also the doc states `password` https://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/post_connection but it was not handled I believe.
Yes, we should. Indeed, the post and the patch include a password. Awesome one!
Let me make this work with the password as well as for the PATCH one. My mind ignored the password in the API. When I see passwords my mind just makes them disappear :smile:
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #43396:
URL: https://github.com/apache/airflow/pull/43396
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1823065055
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
One more thing to consider is including a new field to response data model doesn't seem like a breaking change since it can be ignored easily since no one is using the field due to not exist in the response, but it could be for a couple of users if they are tightly coupled with the response schema somehow and persisting somewhere the response.
In any case, maybe we should document this somewhere, either in the common api changes issue or so. What do you think?
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #43396:
URL: https://github.com/apache/airflow/pull/43396#issuecomment-2455353442
We need to rebase to solve conflicts, but ready to merge IMO.
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1823054504
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
I first included it like that will return `***` as redacted using similar approaches to the `variables` and `extra` field `connections`. Then I saw it is not even included in the reponse in the legacy api document.
[post_connection](https://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/post_connection). In the Patch PR comment, I know you have said we should reduct the returned `password`, but the document made me confused, and I pushed this to align with you first. I can easily get back to returning the password field and redacting.
Yeah, for the redacted unit test, indeed this should be missing that marking definition. Thanks for pointing out!
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43396:
URL: https://github.com/apache/airflow/pull/43396#issuecomment-2455440998
Good to hear that! Rebased and included the field into redact from the other PR comment :)
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1823387755
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
I think we should add the password, indeed adding a field is not a breaking change (We used to do that a lot for the legacy API)
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1823387755
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
I think we should add the password, indeed adding a field is not a breaking change and this is how we have been updating the legacy API.
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1825159918
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
Included the password to the response as redacted and adjusted the tests accordingly. Followed up a similar approach with variables. I thi. I will apply the same update to the patch. Many thanks! :)
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1828146946
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +35,16 @@ class ConnectionResponse(BaseModel):
login: str | None
schema_: str | None = Field(alias="schema")
port: int | None
+ password: str | None
extra: str | None
+ @field_validator("password", mode="after")
+ @classmethod
+ def redact_password(cls, v: str | None) -> str | None:
+ if v is None:
+ return None
+ return redact(v)
+
Review Comment:
yes like this. (Referencing my comment on the other PR)
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1822739405
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -169,3 +169,83 @@ def test_should_respond_200(
body = response.json()
assert body["total_entries"] == expected_total_entries
assert [connection["connection_id"] for connection in body["connections"]] == expected_ids
+
+
+class TestPostConnection(TestConnectionEndpoint):
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": None},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": "{}"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "extra": '{"key": "value"}'},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "description": "test_description",
+ "host": "test_host",
+ "login": "test_login",
+ "schema": "test_schema",
+ "port": 8080,
+ "extra": '{"key": "value"}',
+ },
+ ],
+ )
+ def test_post_should_respond_200(self, test_client, session, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "****", "conn_type": "test_type"},
+ {"connection_id": "test()", "conn_type": "test_type"},
+ {"connection_id": "this_^$#is_invalid", "conn_type": "test_type"},
+ {"connection_id": "iam_not@#$_connection_id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_400_for_invalid_conn_id(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 400
+ connection_id = body["connection_id"]
+ assert response.json() == {
+ "detail": f"The key '{connection_id}' has to be made of "
+ "alphanumeric characters, dashes, dots and underscores exclusively",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type"},
+ ],
+ )
+ def test_post_should_respond_already_exist(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ # Another request
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 409
+ assert response.json() == {
+ "detail": "Connection with connection_id: `test-connection-id` already exists",
+ }
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "test-password"},
+ {"connection_id": "test-connection-id", "conn_type": "test_type", "password": "?>@#+!_%()#"},
+ {
+ "connection_id": "test-connection-id",
+ "conn_type": "test_type",
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_post_should_response_201_redacted_password(self, test_client, body):
+ response = test_client.post("/public/connections/", json=body)
+ assert response.status_code == 201
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
This test is not correct. This will only assert that "password" key is not in the response dict this is not enough. We actually want `password` and `extra` in the response, just the values should be redacted.
You need to check that the response has `"password": "***"` and `"extra": {"password": "***"}`
Also you might be missing a `@pytest.mark.enable_redact` in your test.
Cf `variables`.
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43396:
URL: https://github.com/apache/airflow/pull/43396#issuecomment-2455618637
Something is off with the tests. These should have been caught by the CI/CD already.
`FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py`
I think those parts are deleted maybe :thinking: it should be fixed in #43654.
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1817924444
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,9 +24,12 @@
from airflow.utils.log.secrets_masker import redact
+# Response Models
class ConnectionResponse(BaseModel):
"""Connection serializer for responses."""
+ """Connection serializer for responses."""
+
Review Comment:
Copy & Paste Error?
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43396:
URL: https://github.com/apache/airflow/pull/43396#discussion_r1817958506
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,9 +24,12 @@
from airflow.utils.log.secrets_masker import redact
+# Response Models
class ConnectionResponse(BaseModel):
"""Connection serializer for responses."""
+ """Connection serializer for responses."""
+
Review Comment:
Yes, exactly. I split the models first in #43102 which has the same mistake. :facepalm: You saved me twice :pray: going to fix that PR as well.
--
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
Re: [PR] AIP-84 Migrate post a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43396:
URL: https://github.com/apache/airflow/pull/43396#issuecomment-2452697542
This still needs some work. I am looking
--
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