Skip to content

Reject unknown update_mask fields instead of silently ignoring them - #71003

Open
ColtenOuO wants to merge 1 commit into
apache:mainfrom
ColtenOuO:api-reject-unknown-update-mask
Open

Reject unknown update_mask fields instead of silently ignoring them#71003
ColtenOuO wants to merge 1 commit into
apache:mainfrom
ColtenOuO:api-reject-unknown-update-mask

Conversation

@ColtenOuO

Copy link
Copy Markdown
Contributor

Summary

update_mask lets a PATCH caller say "only apply these fields". Every endpoint
implements it by intersecting the mask with the fields present in the body:

fields_to_update = patch_body.model_fields_set.intersection(update_mask)

An entry that matches no field simply falls out of the intersection. Nothing
notices, so the request becomes a no-op — and still answers 200, echoing back
the untouched record.

A typo is therefore indistinguishable from success:

$ curl -X PATCH '.../variables/pk?update_mask=value' -d '{"key":"pk","value":"CHANGED",...}'
200   # value -> "CHANGED"

$ curl -X PATCH '.../variables/pk?update_mask=valu' -d '{"key":"pk","value":"CHANGED",...}'
200   # value still "original"

$ curl -X PATCH '.../pools/p?update_mask=slot' -d '{"pool":"p","slots":99,...}'
200   # slots still 1

A stray space does the same thing, since nothing trims the entries:

$ curl -X PATCH '.../variables/pk?update_mask=%20value' ...
200   # value still "original"

A client that checks the status code has no way to learn its patch never landed.

dags.py already guards against this (if update_mask != ["is_paused"]: raise 400),
so rejecting an unusable mask is an established expectation in this API — it was
just missing from the other five places that accept one.

Before / after

Request Before After
?update_mask=value 200, applied unchanged
?update_mask=valu 200, not applied 400 naming the unknown field
?update_mask=%20value 200, not applied 200, applied

The error names both the offender and the accepted fields:

Unknown field(s) in update_mask: 'slot'.
Valid fields are: description, include_deferred, name, slots, team_name.

The change

A single validate_update_mask() helper in services/public/common.py, called from
the five places that accept a mask: apply_patch_with_update_mask (covers variables
and pools, including their bulk actions), the connections route, the dag-run route,
and the task-instance service. dags.py is left alone — it already validates.

Two decisions worth flagging:

Aliases count as known names. Validating against field names alone broke an
existing connections test: ConnectionBody.schema_ is aliased to schema, and
update_mask=["schema","extra"] is a working request today. A caller sends the alias
in the body and reads it back in the response, so it has to be accepted. This PR only
validates the mask — which of the two names a given endpoint acts on is deliberately
left untouched.

Whitespace is trimmed rather than rejected. pools.py already compares with
mask.strip(), and the intent of " value" is not in doubt.

Note on an existing test

test_patch_dag_run_with_update_mask had a case asserting that
{"update_mask": ["random"]} returns 200 — the buggy behaviour written down as an
expectation. It now expects 400 and the error message.

Tests

Four new cases on variables: three unknown-field shapes (a typo, a name that exists
nowhere, an empty string) assert 400 and that the row is unchanged, plus one
asserting a mask with surrounding whitespace still applies.

test_variables + test_dag_run: 443 passed.
test_pools + test_connections + test_task_instances + test_dags: 684 passed.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

@ColtenOuO
ColtenOuO force-pushed the api-reject-unknown-update-mask branch from c6c2495 to 8344c90 Compare August 3, 2026 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant