kaxil commented on code in PR #63355:
URL: https://github.com/apache/airflow/pull/63355#discussion_r3416871687
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -1971,6 +1971,62 @@ def test_ti_update_state_not_running(self, client,
session, create_task_instance
session.refresh(ti)
assert ti.state == State.SUCCESS
+ def test_ti_update_state_same_state_is_idempotent(self, client, session,
create_task_instance):
+ """Test that setting a TI to its current state is treated as an
idempotent no-op."""
+ ti = create_task_instance(
+ task_id="test_ti_update_state_same_state_is_idempotent",
+ state=State.SUCCESS,
+ session=session,
+ start_date=DEFAULT_START_DATE,
+ )
+ ti.end_date = DEFAULT_END_DATE
+ session.commit()
+
+ response = client.patch(
+ f"/execution/task-instances/{ti.id}/state",
+ json={
+ "state": "success",
+ "end_date": timezone.parse("2024-10-31T13:00:00Z").isoformat(),
+ },
+ )
+ assert response.status_code == 200
+ assert response.content == b""
+
+ session.refresh(ti)
+ assert ti.state == State.SUCCESS
+ assert ti.end_date == DEFAULT_END_DATE
Review Comment:
This sends a different `end_date` (13:00) than the TI's stored value
(`DEFAULT_END_DATE`, 12:00) and then asserts the stored value wins (line 1997),
so it codifies "silently drop a conflicting payload field" as the contract. But
the duplicate-SUCCESS source this actually fixes (supervisor replay / httpx
retry) re-sends the byte-identical payload, so this documents a case the bug
can't produce. Sending the same `end_date` here would document the
idempotent-replay path you're fixing -- and would still pass.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -335,16 +335,12 @@ def ti_run(
@ti_id_router.patch(
"/{task_instance_id}/state",
status_code=status.HTTP_204_NO_CONTENT,
- responses=create_openapi_http_exception_doc(
- [
- (status.HTTP_404_NOT_FOUND, "Task Instance not found"),
- (
- status.HTTP_409_CONFLICT,
- "The TI is already in the requested state",
- ),
- (HTTP_422_UNPROCESSABLE_CONTENT, "Invalid payload for the state
transition"),
- ]
- ),
+ responses={
+ status.HTTP_200_OK: {"description": "The TI was already in the
requested state"},
+ status.HTTP_404_NOT_FOUND: {"description": "Task Instance not found"},
+ status.HTTP_409_CONFLICT: {"description": "The TI is not in a valid
state for this transition"},
+ HTTP_422_UNPROCESSABLE_CONTENT: {"description": "Invalid payload for
the state transition"},
+ },
Review Comment:
This is now the only route in the file that hand-writes `responses=` instead
of going through `create_openapi_http_exception_doc(...)` (the helper is used
at lines 118, 777, 840, 963, 997, 1039, 1307). The helper attaches `model:
HTTPExceptionResponse` to each error code; this literal dict carries only
`description`, so 404/409/422 lose their response schema in the generated
OpenAPI spec (and the codegen'd execution-API client). Since the helper only
emits error models, you can keep it for the error codes and merge the 200 in:
```python
responses={
status.HTTP_200_OK: {"description": "The TI was already in the requested
state"},
**create_openapi_http_exception_doc(
[
(status.HTTP_404_NOT_FOUND, "Task Instance not found"),
(status.HTTP_409_CONFLICT, "The TI is not in a valid state for
this transition"),
(HTTP_422_UNPROCESSABLE_CONTENT, "Invalid payload for the state
transition"),
]
),
},
```
--
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]
