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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
