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/17 00:08:20 UTC
[PR] AIP-84 Migrate patch a connection to FastAPI API [airflow]
bugraoz93 opened a new pull request, #43102:
URL: https://github.com/apache/airflow/pull/43102
<!--
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: #42592.
Include, making fields nullable for patch endpoints for backward compatibility.
<!-- 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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1809255742
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @pierrejeambrun,
Thanks for the details! I forgot to add TLDR into my previous message (it was an essay) my bad :sweat:
I saw the way along with tests and implemented everything similar to those. Then this bugged me when I saw `legacy api` tests because I thought everything in Public API would be backwards compatible. If we can do breaking changes, all good.
I will convert the `update_mask` parameters again into `list[str]`. I can document these changes into a `significant` fragment.
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e `client.get(..., params={"q": [item1, item2]})`.
Or you can do that manually (don't recommend) by doing `?q=item1&q=item2`.
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1828138447
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +36,23 @@ 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, field_info: ValidationInfo) -> str | None:
+ if v is None:
+ return None
+ try:
+ redacted_dict = json.loads(
+ json.dumps(redact(value={field_info.field_name: v}, name=field_info.field_name))
+ )
+ redacted_password = redacted_dict.get(field_info.field_name, "***")
+ return redacted_password
+ except (json.JSONDecodeError, ValueError) as ex:
+ raise ex
Review Comment:
I think all of that was necessary because the `val` of a Variable is a dictionary with some key / values to redact.
In this case we just need to redact the value, why can't we just `redact(self.val, "password")` ?
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1825334322
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -376,10 +376,28 @@ def test_patch_should_respond_400(self, test_client, payload):
],
)
def test_patch_should_respond_404(self, test_client, payload):
- payload = {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "port": 90}
- response = test_client.patch(f"/public/connections/{TEST_CONN_ID}", json=payload)
+ response = test_client.patch(f"/public/connections/{payload['connection_id']}", json=payload)
assert response.status_code == 404
- print(response.json())
assert {
- "detail": f"The Connection with connection_id: `{TEST_CONN_ID}` was not found",
+ "detail": f"The Connection with connection_id: `{payload['connection_id']}` was not found",
} == response.json()
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "test-password"},
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_LOGIN_2, "password": "?>@#+!_%()#"},
+ {
+ "connection_id": TEST_CONN_ID,
+ "conn_type": TEST_CONN_TYPE,
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_patch_should_response_201_redacted_password(self, test_client, body):
+ self.create_connection()
+ response = test_client.patch(f"/public/connections/{body['connection_id']}", json=body)
+ assert response.status_code == 200
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
I will check this out tomorrow
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
TLDR: Yes fastapi use the `exploded=True` way for passing list, and we should use that and not try to pass comma separated value as before `q=item1,item2`.
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e `client.get(..., params={"q": [item1, item2]})`.
Or you can do that manually (don't recommend) by doing "`?q=item1&q=item2`".
More info on the spec here:
https://swagger.io/docs/specification/v3_0/serialization/
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
TLDR: Yes fastapi use the `exploded=True` way for passing list, and we should use that and not try to pass comma separated value as before `q=item1,item2`. That's a breaking change and we can document it.
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e `client.get(..., params={"q": [item1, item2]})`.
Or you can do that manually (don't recommend) by doing "`?q=item1&q=item2`".
More info on the spec here:
https://swagger.io/docs/specification/v3_0/serialization/
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1807420546
##########
airflow/api_fastapi/core_api/routes/public/connections.py:
##########
@@ -104,3 +105,36 @@ async def get_connections(
],
total_entries=total_entries,
)
+
+
+@connections_router.patch(
+ "/{connection_id}",
+ responses=create_openapi_http_exception_doc([400, 401, 403, 404]),
+)
+async def patch_connection(
+ connection_id: str,
+ patch_body: ConnectionBody,
+ session: Annotated[Session, Depends(get_session)],
+ update_mask: list[str] | None = Query(None),
+) -> ConnectionResponse:
+ """Update a connection entry."""
+ if patch_body.connection_id != connection_id:
+ raise HTTPException(400, "The connection_id in the request body does not match the URL parameter")
+
+ non_update_fields = ["connection_id", "conn_id"]
+ connection = session.scalar(select(Connection).filter_by(conn_id=connection_id).limit(1))
+
+ if connection is None:
+ raise HTTPException(404, f"The Connection with connection_id: `{connection_id}` was not found")
+
+ if update_mask:
+ data = patch_body.model_dump(include=set(update_mask) - set(non_update_fields))
+ else:
+ data = patch_body.model_dump(exclude=set(non_update_fields))
+
+ for key, val in data.items():
+ setattr(connection, key, val)
+ session.add(connection)
+ session.commit()
Review Comment:
Yes, I missed the scope and added these. I also deleted the same part in Variable Patch.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1823066135
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -376,10 +376,28 @@ def test_patch_should_respond_400(self, test_client, payload):
],
)
def test_patch_should_respond_404(self, test_client, payload):
- payload = {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "port": 90}
- response = test_client.patch(f"/public/connections/{TEST_CONN_ID}", json=payload)
+ response = test_client.patch(f"/public/connections/{payload['connection_id']}", json=payload)
assert response.status_code == 404
- print(response.json())
assert {
- "detail": f"The Connection with connection_id: `{TEST_CONN_ID}` was not found",
+ "detail": f"The Connection with connection_id: `{payload['connection_id']}` was not found",
} == response.json()
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "test-password"},
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_LOGIN_2, "password": "?>@#+!_%()#"},
+ {
+ "connection_id": TEST_CONN_ID,
+ "conn_type": TEST_CONN_TYPE,
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_patch_should_response_201_redacted_password(self, test_client, body):
+ self.create_connection()
+ response = test_client.patch(f"/public/connections/{body['connection_id']}", json=body)
+ assert response.status_code == 200
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
Thanks for swift feedback! Replied from 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
Re: [PR] AIP-84 Migrate patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2441643558
I think we have the same issue here, we cannot `update` the `password` field. This should be possible, and the returned password should be redacted.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2455619562
Tests should be fixed in https://github.com/apache/airflow/pull/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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1803935867
##########
airflow/api_fastapi/core_api/serializers/variables.py:
##########
@@ -31,7 +31,7 @@ class VariableBase(BaseModel):
model_config = ConfigDict(populate_by_name=True)
key: str
- description: str | None
+ description: str | None = Field(default=None)
Review Comment:
@pierrejeambrun, I assign these as `Field` and set their `default` to `None` for backward compatibility, because these fields are nullable for `POST` and `PATCH` operations. It is a bit repetitive.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1807421331
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Agree. We have no other choice. Having the distinction would also help with the model/database distinctions, for example, for variables. I have also adjusted this approach for the Variable model and realised that it is flaky if we don't provide a separate strategy for aliases for request and response due to the Pydandic model_dump only generating the proper attributes to update in this case.
In addition, while doing these changes and testing the `update_mask` parameter in the Patch endpoints, I realised that some updates are needed for the update_mask field. Even though it is stated as a comma-separated string list, it is behaving differently in FastAPI. This is because the distinction in passing list from query param is to use the following HTTP call string.
HTTP Call String
`http://localhost:8000/items/?q=foo&q=bar`
https://fastapi.tiangolo.com/tutorial/query-params-str-validations/#query-parameter-list-multiple-values
I believe this is not the case for our legacy api. Please check the test case for connection. I think indeed it is properly cast as a sequence in legacy api but when we use this approach in FastAPI, the value is going as a single-item list rather than a distinct list where it includes all the items. Otherwise, the FastAPI example should be used in the request stated above.
https://github.com/apache/airflow/blob/main/tests/api_connexion/endpoints/test_connection_endpoint.py#L386
@pierrejeambrun This is why It took me some time to make the changes. I had thoughtful times for making everyone happy in pre-commit and FastAPI :sweat_smile: What do you think? Please correct me if I am missing something.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1807421462
##########
airflow/api_fastapi/core_api/serializers/variables.py:
##########
@@ -31,7 +31,7 @@ class VariableBase(BaseModel):
model_config = ConfigDict(populate_by_name=True)
key: str
- description: str | None
+ description: str | None = Field(default=None)
Review Comment:
Yeap, I have included these to Body models.
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1828138447
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +36,23 @@ 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, field_info: ValidationInfo) -> str | None:
+ if v is None:
+ return None
+ try:
+ redacted_dict = json.loads(
+ json.dumps(redact(value={field_info.field_name: v}, name=field_info.field_name))
+ )
+ redacted_password = redacted_dict.get(field_info.field_name, "***")
+ return redacted_password
+ except (json.JSONDecodeError, ValueError) as ex:
+ raise ex
Review Comment:
I think all of that was necessary because the `val` was a serialized string representation of a dict in the `VariableResponse` schema, with some key/values pair to redact.
In this case we have a string, we just need to redact the value, why can't we just `redact(v, "password")` ?
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +36,23 @@ 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, field_info: ValidationInfo) -> str | None:
+ if v is None:
+ return None
+ try:
+ redacted_dict = json.loads(
+ json.dumps(redact(value={field_info.field_name: v}, name=field_info.field_name))
+ )
+ redacted_password = redacted_dict.get(field_info.field_name, "***")
+ return redacted_password
+ except (json.JSONDecodeError, ValueError) as ex:
+ raise ex
Review Comment:
I think all of that was necessary because the `val` was a serialized string representation of a dict in the `VariableResponse` schema, with some key/values pair to redact.
In this case we have a string, we just need to redact the value, can we `redact(v, "password")` ?
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1826398908
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -376,10 +376,28 @@ def test_patch_should_respond_400(self, test_client, payload):
],
)
def test_patch_should_respond_404(self, test_client, payload):
- payload = {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "port": 90}
- response = test_client.patch(f"/public/connections/{TEST_CONN_ID}", json=payload)
+ response = test_client.patch(f"/public/connections/{payload['connection_id']}", json=payload)
assert response.status_code == 404
- print(response.json())
assert {
- "detail": f"The Connection with connection_id: `{TEST_CONN_ID}` was not found",
+ "detail": f"The Connection with connection_id: `{payload['connection_id']}` was not found",
} == response.json()
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "test-password"},
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_LOGIN_2, "password": "?>@#+!_%()#"},
+ {
+ "connection_id": TEST_CONN_ID,
+ "conn_type": TEST_CONN_TYPE,
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_patch_should_response_201_redacted_password(self, test_client, body):
+ self.create_connection()
+ response = test_client.patch(f"/public/connections/{body['connection_id']}", json=body)
+ assert response.status_code == 200
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
@pierrejeambrun this is working fine though I didn't like the hacky `JSON` parsing back and forth but I could only make `mypy airflow` happy otherwise. I was surprised by how the first commit worked in `POST` #43396 after digging deeper into the `redact()` method.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2457964395
> Looking good.
>
> Need rebasing to fix conflicts, we can merge after.
Many thanks for rebasing and merging the other PR! You saved me from facing the conflict twice :pray: Rebased.
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
TLDR: Yes fastapi use the `exploded=True` way for passing list, and we should use that and not try to pass comma separated value as before `q=item1,item2`. That's a breaking change and we can document it.
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e `client.get(..., params={"q": [item1, item2]})`.
Or you can do that manually (don't recommend) by doing "`?q=item1&q=item2`".
More info on the spec here:
https://swagger.io/docs/specification/v3_0/serialization/
I can add a `significant` newsfragment to warn users that this changed.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2444749527
> For 'common' behavior of null values we can do that in another PR. That is not really specific to this one. I think we can just handle the 'password', then merge it, maybe you can take care of making all `PATCH` consistent regarding none values in a follow up, if that sounds good ?
Yeap, it sounds perfect! I am going to take care of that after finishing the `password` part. There is a small work left for password. I couldn't make redact work in the tests, which I will check it out today and push it to both PRs related to 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
Re: [PR] AIP-84 Migrate patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1822746805
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -376,10 +376,28 @@ def test_patch_should_respond_400(self, test_client, payload):
],
)
def test_patch_should_respond_404(self, test_client, payload):
- payload = {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "port": 90}
- response = test_client.patch(f"/public/connections/{TEST_CONN_ID}", json=payload)
+ response = test_client.patch(f"/public/connections/{payload['connection_id']}", json=payload)
assert response.status_code == 404
- print(response.json())
assert {
- "detail": f"The Connection with connection_id: `{TEST_CONN_ID}` was not found",
+ "detail": f"The Connection with connection_id: `{payload['connection_id']}` was not found",
} == response.json()
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "test-password"},
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_LOGIN_2, "password": "?>@#+!_%()#"},
+ {
+ "connection_id": TEST_CONN_ID,
+ "conn_type": TEST_CONN_TYPE,
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_patch_should_response_201_redacted_password(self, test_client, body):
+ self.create_connection()
+ response = test_client.patch(f"/public/connections/{body['connection_id']}", json=body)
+ assert response.status_code == 200
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
Same comment https://github.com/apache/airflow/pull/43396#discussion_r1822739405
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1825224073
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -376,10 +376,28 @@ def test_patch_should_respond_400(self, test_client, payload):
],
)
def test_patch_should_respond_404(self, test_client, payload):
- payload = {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "port": 90}
- response = test_client.patch(f"/public/connections/{TEST_CONN_ID}", json=payload)
+ response = test_client.patch(f"/public/connections/{payload['connection_id']}", json=payload)
assert response.status_code == 404
- print(response.json())
assert {
- "detail": f"The Connection with connection_id: `{TEST_CONN_ID}` was not found",
+ "detail": f"The Connection with connection_id: `{payload['connection_id']}` was not found",
} == response.json()
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "test-password"},
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_LOGIN_2, "password": "?>@#+!_%()#"},
+ {
+ "connection_id": TEST_CONN_ID,
+ "conn_type": TEST_CONN_TYPE,
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_patch_should_response_201_redacted_password(self, test_client, body):
+ self.create_connection()
+ response = test_client.patch(f"/public/connections/{body['connection_id']}", json=body)
+ assert response.status_code == 200
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
Checking the tests.
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1828138447
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +36,23 @@ 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, field_info: ValidationInfo) -> str | None:
+ if v is None:
+ return None
+ try:
+ redacted_dict = json.loads(
+ json.dumps(redact(value={field_info.field_name: v}, name=field_info.field_name))
+ )
+ redacted_password = redacted_dict.get(field_info.field_name, "***")
+ return redacted_password
+ except (json.JSONDecodeError, ValueError) as ex:
+ raise ex
Review Comment:
I think all of that was necessary because the `val` of a Variable is a dictionary with some values to redact.
In this case we just need to redact the value, why can't we just `redact(self.val, "password")` ?
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1804350465
##########
airflow/api_fastapi/core_api/routes/public/connections.py:
##########
@@ -104,3 +105,36 @@ async def get_connections(
],
total_entries=total_entries,
)
+
+
+@connections_router.patch(
+ "/{connection_id}",
+ responses=create_openapi_http_exception_doc([400, 401, 403, 404]),
+)
+async def patch_connection(
+ connection_id: str,
+ patch_body: ConnectionBody,
+ session: Annotated[Session, Depends(get_session)],
+ update_mask: list[str] | None = Query(None),
+) -> ConnectionResponse:
+ """Update a connection entry."""
+ if patch_body.connection_id != connection_id:
+ raise HTTPException(400, "The connection_id in the request body does not match the URL parameter")
+
+ non_update_fields = ["connection_id", "conn_id"]
+ connection = session.scalar(select(Connection).filter_by(conn_id=connection_id).limit(1))
+
+ if connection is None:
+ raise HTTPException(404, f"The Connection with connection_id: `{connection_id}` was not found")
+
+ if update_mask:
+ data = patch_body.model_dump(include=set(update_mask) - set(non_update_fields))
+ else:
+ data = patch_body.model_dump(exclude=set(non_update_fields))
+
+ for key, val in data.items():
+ setattr(connection, key, val)
+ session.add(connection)
+ session.commit()
Review Comment:
I don't think we need that. The object is already in the session. The context manager for the session dependency comits on exit
```suggestion
```
##########
airflow/api_fastapi/core_api/serializers/variables.py:
##########
@@ -31,7 +31,7 @@ class VariableBase(BaseModel):
model_config = ConfigDict(populate_by_name=True)
key: str
- description: str | None
+ description: str | None = Field(default=None)
Review Comment:
Yes we need to do that for all our `Body` models, to respect the `legacy API` contract. (if fields could be missing, It should be possible in the new API to omit them)
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Unfortunately I don't think they can share the same base.
Because that works for the `ConnectionBody`, but it doesn't for the Response. Here in the documentation, all those field will appear as `optional`, like `they could be missing from the body` but that's not possible for the Response.
I think we have no other choice than having 2 full distinct Schema `ConnectionResponse` (no fields are missing, but nullable) `ConnectionBody` (most fields can be missing from the body)
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e `client.get(..., params={"q": [item1, item2]})`.
Or you can do that manually (don't recommend) by doing "`?q=item1&q=item2`".
More info on the spec here:
https://swagger.io/docs/specification/v3_0/serialization/
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1815559254
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
I have adjusted the code accordingly and included the `significant` fragment. Please check again when you have 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
Re: [PR] AIP-84 Migrate patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1816757546
##########
newsfragments/43102.significant.rst:
##########
@@ -0,0 +1,5 @@
+Change in update model endpoints (PATCH) ``update_mask``
+
+The ``update_mask`` parameter in the PATCH endpoints has been updated to explode (true) by default.
+Before: ``http://<URL>:<PORT>/<PATH>?update_mask=item1,item2``
Review Comment:
I don't think this should be specific to the `PATCH` endpoint. This is how fast API works, and all list parameters in the API will be impacted.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1828225648
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +36,23 @@ 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, field_info: ValidationInfo) -> str | None:
+ if v is None:
+ return None
+ try:
+ redacted_dict = json.loads(
+ json.dumps(redact(value={field_info.field_name: v}, name=field_info.field_name))
+ )
+ redacted_password = redacted_dict.get(field_info.field_name, "***")
+ return redacted_password
+ except (json.JSONDecodeError, ValueError) as ex:
+ raise ex
Review Comment:
This worked smoothly. Indeed, my mind just didn't accept the fact the same approach wouldn't work with the same structure. I totally missed this part where we can pass the name for a pre-defined hidden key like a password. :facepalm: Thanks! :) Rebased, passed the `field_name` ("password") to `redact()`
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e `client.get(..., params={"q": [item1, item2]})`.
Or you can do that manually (don't recommend) by doing "`?q=item1&q=item2`".
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing).
Or you can do that manually (don't recommend) by doing `?q=item1&q=item2`.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2442465240
> I just pushed one commit to remove a remnent of old code `_parse_comma_separated_query_params`. PR rebased. Should be good to merge.
I totally missed this one, thanks for 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
Re: [PR] AIP-84 Migrate patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1828138447
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -34,8 +36,23 @@ 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, field_info: ValidationInfo) -> str | None:
+ if v is None:
+ return None
+ try:
+ redacted_dict = json.loads(
+ json.dumps(redact(value={field_info.field_name: v}, name=field_info.field_name))
+ )
+ redacted_password = redacted_dict.get(field_info.field_name, "***")
+ return redacted_password
+ except (json.JSONDecodeError, ValueError) as ex:
+ raise ex
Review Comment:
I think all of that was necessary because the `val` of a Variable is a dictionary with some key / values to redact.
In this case we just need to redact the value, why can't we just `redact(v, "password")` ?
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2443576945
For 'common' behavior of null values we can do that in another PR. That is not really specific to this one. I think we can just handle the 'password', then merge it, maybe you can take care of making all `PATCH` consistent regarding none values in a follow up, if that sounds good ?
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1817441750
##########
newsfragments/43102.significant.rst:
##########
@@ -0,0 +1,5 @@
+Change in update model endpoints (PATCH) ``update_mask``
+
+The ``update_mask`` parameter in the PATCH endpoints has been updated to explode (true) by default.
+Before: ``http://<URL>:<PORT>/<PATH>?update_mask=item1,item2``
Review Comment:
Good point! I originally focused on this as a specific case for `update_mask`, which is why it read more like a deployment guide. I agree, this should be written from a broader perspective. Thanks for the feedback! I updated accordingly, I hope the new one works 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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1816757546
##########
newsfragments/43102.significant.rst:
##########
@@ -0,0 +1,5 @@
+Change in update model endpoints (PATCH) ``update_mask``
+
+The ``update_mask`` parameter in the PATCH endpoints has been updated to explode (true) by default.
+Before: ``http://<URL>:<PORT>/<PATH>?update_mask=item1,item2``
Review Comment:
I don't think this should be specific to the `PATCH` endpoint. This is how fast API works, and all list parameters in the API will be impacted, where this suggests that only this specific query parameter is impacted.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2442472690
> I think we should also make consistant the behavior for `PATCH` regarding `None` value.
>
> Here when there is a mask you take them. And when there is no mask, you filter them out.
>
> I did something different for `Pool` which I think can be extented. When there is a mask, same thing, we use values. When there is no mask, we validate that the payload is valide by it's own, i.e it's a valid db entity by its own, and we use everything. (none or not).
>
> The idea is to allow someone to not specify `mask` and reset some fields to `None` if needed.
I think I looked from the lazy path when no parameter, no value to you :) Saw your changes. Looks good and makes sense! I agree, we should have a common approach for common methods because what we are doing at the end is pretty similar.
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #43102:
URL: https://github.com/apache/airflow/pull/43102#issuecomment-2441320841
I just pushed one commit to remove a remnent of old code `_parse_comma_separated_query_params`. Should be good to merge.
--
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 patch a connection to FastAPI API [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1825170663
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -376,10 +376,28 @@ def test_patch_should_respond_400(self, test_client, payload):
],
)
def test_patch_should_respond_404(self, test_client, payload):
- payload = {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "port": 90}
- response = test_client.patch(f"/public/connections/{TEST_CONN_ID}", json=payload)
+ response = test_client.patch(f"/public/connections/{payload['connection_id']}", json=payload)
assert response.status_code == 404
- print(response.json())
assert {
- "detail": f"The Connection with connection_id: `{TEST_CONN_ID}` was not found",
+ "detail": f"The Connection with connection_id: `{payload['connection_id']}` was not found",
} == response.json()
+
+ @pytest.mark.parametrize(
+ "body",
+ [
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "test-password"},
+ {"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_LOGIN_2, "password": "?>@#+!_%()#"},
+ {
+ "connection_id": TEST_CONN_ID,
+ "conn_type": TEST_CONN_TYPE,
+ "password": "A!rF|0wi$aw3s0m3",
+ "extra": '{"password": "test-password"}',
+ },
+ ],
+ )
+ def test_patch_should_response_201_redacted_password(self, test_client, body):
+ self.create_connection()
+ response = test_client.patch(f"/public/connections/{body['connection_id']}", json=body)
+ assert response.status_code == 200
+ connection = response.json()
+ assert "password" not in connection
Review Comment:
Include the password in this 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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #43102:
URL: https://github.com/apache/airflow/pull/43102
--
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 patch a connection to FastAPI API [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43102:
URL: https://github.com/apache/airflow/pull/43102#discussion_r1808447157
##########
airflow/api_fastapi/core_api/serializers/connections.py:
##########
@@ -24,19 +24,24 @@
from airflow.utils.log.secrets_masker import redact
-class ConnectionResponse(BaseModel):
+class ConnectionBase(BaseModel):
"""Connection serializer for responses."""
- connection_id: str = Field(serialization_alias="connection_id", validation_alias="conn_id")
conn_type: str
- description: str | None
- host: str | None
- login: str | None
- schema_: str | None = Field(alias="schema")
- port: int | None
- extra: str | None
-
- @field_validator("extra", mode="before")
+ description: str | None = Field(default=None)
+ host: str | None = Field(default=None)
+ login: str | None = Field(default=None)
+ schema_: str | None = Field(None, alias="schema")
+ port: int | None = Field(default=None)
+ extra: str | None = Field(default=None)
+
+
+class ConnectionResponse(ConnectionBase):
Review Comment:
Hello @bugraoz93,
Indeed fastapi use the `exploded=True` form where the legacy one does not. You can take a look at the other tests that implements tests on the `update_mask` in fastapi. You can either pass an object with a list to the `params` parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing).
Or you can do that manually (don't recommend) by doing `?q=item1&q=item2`.
--
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