Andrushika commented on code in PR #71003:
URL: https://github.com/apache/airflow/pull/71003#discussion_r3961124664
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/common.py:
##########
@@ -55,6 +56,10 @@ def handle_request(self) -> BulkResponse:
if action.action == BulkAction.CREATE:
self.handle_bulk_create(action, results[action.action.value])
elif action.action == BulkAction.UPDATE:
+ if action.entities:
+ action.update_mask = validate_update_mask(
+ cast("type[BaseModel]", type(action.entities[0])),
action.update_mask
+ )
Review Comment:
Currently a bad mask fails the whole bulk request with 400, but we expect
each action to report its own errors in `results.errors`, like the immutable
field check in `apply_patch_with_update_mask` ends up doing.
Would it make sense to move this into the per-action try block, so bulk
clients see one shape?
##########
airflow-core/src/airflow/api_fastapi/common/parameters.py:
##########
@@ -2056,3 +2056,56 @@ def _optional_boolean(value: bool | None) -> bool | None:
)
),
]
+
+
+# Update mask
+def validate_update_mask(patch_body_type: type[BaseModel], update_mask:
list[str] | None) -> list[str] | None:
+ """
+ Reject ``update_mask`` entries that name no field of ``patch_body_type``.
+
+ Every caller narrows the patch down with ``set(update_mask)``, which drops
an entry matching
+ nothing -- so a typo used to make the whole request a no-op that still
answered ``200``.
+ Aliases count as known names because that is what a caller sends in the
body and reads back in
+ the response; which of the two a given endpoint acts on is left untouched
here. Surrounding
+ whitespace is stripped so a stray space selects the field instead of
silently selecting nothing.
+
+ Routes take the mask from the query string through
:func:`update_mask_param_factory`; this is
+ for the bulk endpoints, which carry it in the request body instead.
+
+ :param patch_body_type: the request body type the mask selects fields from.
+ :param update_mask: the requested field names, or ``None``.
+ :return: the mask with whitespace stripped, or ``None``.
+ :raises HTTPException: 400 if any entry names no field.
+ """
+ if not update_mask:
+ return update_mask
+
+ fields = patch_body_type.model_fields
+ known = set(fields) | {
+ alias
+ for field in fields.values()
+ for alias in (field.alias, field.serialization_alias,
field.validation_alias)
Review Comment:
I think we should exclude `serialization_alias` here, it is too wide. It is
the output name, not a name the body can send.
For example: `PATCH /variables/k?update_mask=val` passes this check
(`VariableBody.value` has `serialization_alias="val"`), but model_fields_set
only has value, so it is a 200 no-op again.
validation_alias alone should be enough, since alias fills it too.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py:
##########
@@ -633,6 +633,33 @@ def test_patch_with_update_mask_description_only(self,
test_client, session):
assert response.json()["description"] == "updated description"
assert response.json()["key"] == TEST_VARIABLE_KEY
+ @pytest.mark.parametrize(
+ "unknown",
+ ["valu", "definitely_not_a_field", ""], # codespell:ignore valu
+ )
+ def test_patch_unknown_update_mask_field_returns_400(self, test_client,
session, unknown):
+ self.create_variables()
+ response = test_client.patch(
+ f"/variables/{TEST_VARIABLE_KEY}",
+ json={"key": TEST_VARIABLE_KEY, "value": "new_value",
"description": "new description"},
+ params={"update_mask": [unknown]},
+ )
+ assert response.status_code == 400
+ assert f"Unknown field(s) in update_mask: {unknown!r}" in
response.json()["detail"]
+ # The request must not have been applied
+ stored = session.scalar(select(Variable).where(Variable.key ==
TEST_VARIABLE_KEY))
+ assert stored.val == TEST_VARIABLE_VALUE
+
+ def test_patch_update_mask_field_tolerates_surrounding_whitespace(self,
test_client, session):
+ self.create_variables()
+ response = test_client.patch(
+ f"/variables/{TEST_VARIABLE_KEY}",
+ json={"key": TEST_VARIABLE_KEY, "value": "new_value",
"description": "unchanged"},
+ params={"update_mask": [" value "]},
+ )
+ assert response.status_code == 200
+ assert response.json()["value"] == "new_value"
+
Review Comment:
Only `variables` and `dag_run` have a 400 case now, and each route wires the
dependency on its own line, so dropping one is not caught. Let's add test
coverage for each endpoint to lock the wiring.
--
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]
