pierrejeambrun commented on code in PR #44619:
URL: https://github.com/apache/airflow/pull/44619#discussion_r1871451479
##########
airflow/api_fastapi/core_api/routes/public/variables.py:
##########
@@ -130,14 +132,24 @@ def patch_variable(
raise HTTPException(
status.HTTP_404_NOT_FOUND, f"The Variable with key:
`{variable_key}` was not found"
)
+
+ fields_to_update = patch_body.model_fields_set
if update_mask:
+ fields_to_update = fields_to_update.intersection(update_mask)
data = patch_body.model_dump(
- include=set(update_mask) - non_update_fields, by_alias=True,
exclude_none=True
+ include=fields_to_update - non_update_fields, by_alias=True,
exclude_none=True
)
else:
+ try:
+ VariableBody(**patch_body.model_dump())
+ except ValidationError as e:
+ raise RequestValidationError(errors=e.errors())
data = patch_body.model_dump(exclude=non_update_fields, by_alias=True,
exclude_none=True)
+
for key, val in data.items():
setattr(variable, key, val)
+
+ session.commit()
Review Comment:
`commit` is automatically done on session close. I don't think we need to do
that manually here.
##########
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -140,9 +142,16 @@ def patch_dag_run(
if not dag:
raise HTTPException(status.HTTP_404_NOT_FOUND, f"Dag with id {dag_id}
was not found")
+ fields_to_update = patch_body.model_fields_set
+
if update_mask:
- data = patch_body.model_dump(include=set(update_mask))
+ fields_to_update = fields_to_update.intersection(update_mask)
+ data = patch_body.model_dump(include=fields_to_update)
else:
+ try:
+ DAGRunPatchBody(**patch_body.model_dump())
+ except ValidationError as e:
+ raise RequestValidationError(errors=e.errors())
data = patch_body.model_dump()
Review Comment:
Some of them are doing that, and some others have `exclude_none=True`
##########
tests/api_fastapi/core_api/routes/public/test_dags.py:
##########
@@ -193,7 +193,7 @@ class TestPatchDag(TestDagEndpoint):
"query_params, dag_id, body, expected_status_code, expected_is_paused",
[
({}, "fake_dag_id", {"is_paused": True}, 404, None),
- ({"update_mask": ["field_1", "is_paused"]}, DAG1_ID, {"is_paused":
True}, 400, None),
+ ({"update_mask": ["field_1", "is_paused"]}, DAG1_ID, {"is_paused":
True}, 200, True),
Review Comment:
This should still raise `400`. Only `is_paused` is updatable, anything else
should error and warn the client that the field is not editable.
##########
tests/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -522,8 +522,8 @@ def test_patch_should_respond_200(self, test_client, body,
session):
"conn_type": TEST_CONN_TYPE,
"extra": None,
"host": TEST_CONN_HOST,
- "login": None,
- "port": None,
+ "login": TEST_CONN_LOGIN,
+ "port": TEST_CONN_PORT,
Review Comment:
Why was this test updated ? It's still valid to actually send `None` to the
API.
##########
airflow/api_fastapi/core_api/routes/public/task_instances.py:
##########
@@ -671,6 +673,11 @@ def patch_task_instance(
fields_to_update = body.model_fields_set
if update_mask:
fields_to_update = fields_to_update.intersection(update_mask)
+ else:
+ try:
+ PatchTaskInstanceBody.model_validate(body)
+ except ValidationError as e:
+ raise RequestValidationError(errors=e.errors())
Review Comment:
Task instance is not doing exacly the same as the rest I believe.
We are not replacing `data` by the validated entity, therefore we are still
using only `fields_to_update = body.model_fields_set` even when the
`update_mask` is not provided.
Because for other endpoints we do:
```
data = patch_body.model_dump(exclude=non_update_fields, by_alias=True,
exclude_none=True)
```
I think we should do the same.
##########
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -165,6 +174,7 @@ def patch_dag_run(
dag_run.dag_run_note.content = attr_value
dag_run = session.get(DagRun, dag_run.id)
+ session.commit()
Review Comment:
Same
##########
tests/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -887,7 +887,7 @@ def test_patch_dag_run(self, test_client, dag_id, run_id,
patch_body, response_b
{"note": "new_note2", "state": "failed"},
200,
),
- ({"update_mask": ["note"]}, {}, {"state": "success", "note":
None}, 200),
+ ({"update_mask": ["note"]}, {}, {"state": "success", "note":
"test_note"}, 200),
Review Comment:
Same this was updated but is actually a valid test. We should be able to
reset the note to `None`
##########
airflow/api_fastapi/core_api/routes/public/variables.py:
##########
@@ -130,14 +132,24 @@ def patch_variable(
raise HTTPException(
status.HTTP_404_NOT_FOUND, f"The Variable with key:
`{variable_key}` was not found"
)
+
+ fields_to_update = patch_body.model_fields_set
if update_mask:
+ fields_to_update = fields_to_update.intersection(update_mask)
data = patch_body.model_dump(
- include=set(update_mask) - non_update_fields, by_alias=True,
exclude_none=True
+ include=fields_to_update - non_update_fields, by_alias=True,
exclude_none=True
)
else:
+ try:
+ VariableBody(**patch_body.model_dump())
+ except ValidationError as e:
+ raise RequestValidationError(errors=e.errors())
data = patch_body.model_dump(exclude=non_update_fields, by_alias=True,
exclude_none=True)
+
for key, val in data.items():
setattr(variable, key, val)
+
+ session.commit()
Review Comment:
You can `flush` if you need auto-generated id/ or anything else from the db
side.
##########
tests/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -887,7 +887,7 @@ def test_patch_dag_run(self, test_client, dag_id, run_id,
patch_body, response_b
{"note": "new_note2", "state": "failed"},
200,
),
- ({"update_mask": ["note"]}, {}, {"state": "success", "note":
None}, 200),
+ ({"update_mask": ["note"]}, {}, {"state": "success", "note":
"test_note"}, 200),
Review Comment:
You could add another one like:
```
({"update_mask": ["note"]}, {"note": None}, {"state": "success", "note":
"test_note"}, 200),
```
--
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]
