potiuk commented on code in PR #70328:
URL: https://github.com/apache/airflow/pull/70328#discussion_r3682287539
##########
airflow-ctl/tests/airflow_ctl/api/test_operations.py:
##########
@@ -1157,6 +1157,36 @@ def handle_request(request: httpx.Request) ->
httpx.Response:
response = client.dags.trigger(dag_id=self.dag_id,
trigger_dag_run=self.trigger_dag_run)
assert response == self.dag_run_response
+ def test_trigger_excludes_none_fields_from_request_body(self):
+ """Regression test: trigger() must exclude unset (None) fields from the
+ request body. Older API servers reject unrecognized fields (e.g.
+ partition_key, bundle_version, added in a later API version) with a
+ 422 extra_forbidden error if they're present at all, even as null.
+ """
+ captured_body = {}
+
+ def handle_request(request: httpx.Request) -> httpx.Response:
+ nonlocal captured_body
+ captured_body = json.loads(request.content)
+ return httpx.Response(200,
json=json.loads(self.dag_run_response.model_dump_json()))
+
+ client = make_api_client(transport=httpx.MockTransport(handle_request))
+ client.dags.trigger(dag_id=self.dag_id,
trigger_dag_run=self.trigger_dag_run)
+
+ # conf is explicitly defaulted to {} by trigger() when unset, so it's
expected.
+ assert captured_body == {"conf": {}}
+ for field in (
Review Comment:
This asserts `"logical_date" not in captured_body`, which codifies the
behaviour I think is the bug — so the test would keep passing after a fix that
reintroduces the field.
More broadly, `httpx.MockTransport` returns a canned 200 without validating
the body against the server model, so no test here can catch "the server would
have rejected this". A test that round-trips the body through
`airflow.api_fastapi.core_api.datamodels.dag_run.TriggerDAGRunPostBody.model_validate(captured_body)`
would have caught the missing required field immediately, and is worth adding
whatever direction the fix takes.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -603,7 +603,7 @@ def trigger(
trigger_dag_run.conf = {}
try:
self.response = self.client.post(
- f"dags/{dag_id}/dagRuns",
json=trigger_dag_run.model_dump(mode="json")
+ f"dags/{dag_id}/dagRuns",
json=trigger_dag_run.model_dump(mode="json", exclude_none=True)
Review Comment:
`exclude_none=True` drops **every** null field, including `logical_date`,
which the server requires to be present (see the body of my review). It also
can't distinguish "never provided" from "explicitly set to null", so a user who
deliberately passes a null value has it silently dropped.
The stated problem is narrower than this: older servers reject *fields they
don't know about*. So the targeted fix is to omit only those, e.g.
`exclude={"partition_key", "bundle_version"}` when talking to an older server,
ideally gated on a negotiated API version rather than applied unconditionally.
If you'd rather keep it general, `exclude_unset=True` is at least
semantically honest about "the caller didn't set this" — but it has the same
`logical_date` problem, so it doesn't avoid the regression on its own.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
