Dev-iL commented on PR #61532:
URL: https://github.com/apache/airflow/pull/61532#issuecomment-3875768634
@jason810496 Please see below Claude's analysis of the point you raised. I
am going to add the extra test. Please let me know if you think this is
insufficient.
------------
## Reviewer concern: `field_serializer` for UUID fields
The reviewer suggests adding `field_serializer` to UUID fields in Pydantic
response models to keep responses compatible with `airflow-client-python`
(which is generated from the OpenAPI YAML spec).
**Is the concern justified?** Partially — there IS an observable schema
change, but the suggested fix doesn't address it:
1. **JSON wire format (NOT affected):** Pydantic v2 already serializes
`UUID` objects to dashed strings (`"019c39c7-..."`) by default. A
`field_serializer` returning `str(id_value)` is redundant — it does exactly
what Pydantic already does.
2. **OpenAPI schema (IS affected):** Changing `id: str` → `id: UUID` causes
the generated YAML spec to add `format: uuid`:
```yaml
# Before (str) # After (UUID)
id: id:
type: string type: string
title: Id format: uuid # ← NEW
title: Id
```
This is already reflected in both `v2-rest-api-generated.yaml` (line
12585) and `_private_ui.yaml` (line 2548).
3. **`field_serializer` does NOT fix the schema:** In Pydantic v2,
`@field_serializer` only affects **runtime** serialization, not JSON schema
generation. The schema is derived from the type annotation (`UUID`), so
`format: uuid` appears regardless of any serializer. To suppress `format: uuid`
in the schema, you'd need `Annotated[UUID, PlainSerializer(str,
return_type=str)]` or similar — a fundamentally different approach.
4. **Precedent already exists:** `DagVersionResponse.id` was already `UUID`
typed and already had `format: uuid` in the OpenAPI spec **before this PR**.
The pattern is established.
5. **`format: uuid` is non-breaking for clients:** OpenAPI's `format` is a
hint. The base type is still `string`. Well-behaved client generators treat
`format: uuid` as validation/documentation, not as a different wire type.
### Recommendation: Add a serialization regression test (no
`field_serializer`)
The `field_serializer` is unnecessary since Pydantic already handles the
conversion. However, a **unit test** documenting the expected serialization
behavior IS valuable as a regression guard — it protects against future
Pydantic upgrades or config changes accidentally breaking UUID→string
serialization.
**Test to add** in a new or existing test file (e.g.,
`airflow-core/tests/unit/api_fastapi/core_api/datamodels/test_task_instances.py`):
```python
from uuid import UUID
from airflow.api_fastapi.core_api.datamodels.task_instances import
TaskInstanceResponse
def test_uuid_serialized_as_dashed_string():
"""Ensure UUID fields serialize to dashed string format in JSON
responses.
This guards against regressions that could break airflow-client-python
and other consumers of the Public API.
See: https://github.com/apache/airflow/pull/XXXXX
"""
test_uuid = UUID("019c39c7-4fea-76b6-891d-ebb3267d427d")
# TaskInstanceResponse requires many fields; construct minimally via
model_construct
response = TaskInstanceResponse.model_construct(id=test_uuid)
json_data = response.model_dump(mode="json")
assert json_data["id"] == "019c39c7-4fea-76b6-891d-ebb3267d427d"
assert isinstance(json_data["id"], str)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
