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