pierrejeambrun commented on code in PR #46018:
URL: https://github.com/apache/airflow/pull/46018#discussion_r1928975110
##########
airflow/api_fastapi/core_api/routes/public/task_instances.py:
##########
@@ -717,34 +705,123 @@ def patch_task_instance(
fields_to_update = body.model_fields_set
if update_mask:
fields_to_update = fields_to_update.intersection(update_mask)
- data = body.model_dump(include=fields_to_update, by_alias=True)
else:
try:
PatchTaskInstanceBody.model_validate(body)
except ValidationError as e:
raise RequestValidationError(errors=e.errors())
- data = body.model_dump(by_alias=True)
+
+ return dag, ti, body.model_dump(include=fields_to_update, by_alias=True)
Review Comment:
I think we should always only reqly on `fields_to_update`, i.e
`body.model_fields_set` field actually set by the user.
For instance It feels weird when sending `{"note": "something"}` to have
`state` filled in as None, and then setting the state as None, while this does
not look the intend of the user. Problem appears when patching an instance with
a partial payload but without specifying the `update_mask`, maybe we should
error in this case. @rawwar what do you think, should I update other `patch` as
well to use `fields_to_update` to avoid this edge case ?
--
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]
