stantheman0128 commented on issue #69935:
URL: https://github.com/apache/airflow/issues/69935#issuecomment-5209518293
I looked into whether the TODO this issue quotes can still be acted on, and
I think its
premise has gone stale. Posting the findings in case they save someone the
same dig.
The TODO was added in #44899 in December 2024, when the Execution API
`DagRun` schema was
only ever returned nested inside `TIRunContext`. The same class has since
picked up
standalone return positions: `GET /dag-runs/{dag_id}/previous` in the
2025-08-10 version
change, and `GET /dag-runs/{dag_id}/{run_id}` plus `GET /dag-runs/previous`
in 2026-04-06.
In those the fields duplicate nothing.
The part that surprised me is that the nested case cannot be fixed by a
converter either.
`TIRunContext` has no `ti` field, only `dag_run`, `task_reschedule_count`,
`max_tries`,
`variables`, `connections`, `next_method`, `next_kwargs`,
`xcom_keys_to_clear`,
`should_retry` and `start_date`, and the route's only path parameter is the
task instance
UUID. So there is no sibling `TaskInstance` in the payload to fall back on.
On
`/dag-runs/previous` it is worse: the `run_id` returned belongs to a
different run from
the caller's and appears in neither the query parameters nor the body, and
it is not
derivable from `logical_date` because `generate_run_id` is overridable and
`TriggerDagRunOperator` can supply an arbitrary `run_id`.
That matters because the generated client sets `extra="forbid"` and marks
both fields
required, so a worker pinned to 2026-06-30 or earlier raises a
ValidationError rather
than degrading.
The fields are also more load-bearing than they look. `DagRunProtocol`
declares both,
`dag_run` goes straight into the template context so `{{ dag_run.run_id }}`
resolves
against it, `AIRFLOW_CONTEXT_DAG_RUN_ID` and `task_instance_key_str` are
built from
`run_id`, and `AssetEventSourceTaskInstance` exposes `dag_id`/`run_id` as
properties over
a nested `DagRun` that it then passes as the `ti_key` to `XCom.get_value`.
That last one
is a functional break in asset event `xcom_pull`, not just a templating
change.
Since `schema(DagRun).field(...)` applies to the class rather than to a
route, the fields
cannot be dropped for the nested case while kept on the standalone ones
without a second
schema class, and the seven existing `schema(DagRun)` instructions across
four version
modules would each need retro-fitting onto it.
My read is that the honest version of this issue is not "delete two
redundant fields" but
"what should the `ti_run` response contain", which is an AIP-72 design
question rather
than a cleanup. If that is not something you want to open right now, I am
happy to send a
small patch that just updates the TODO to record why it is stale, so the
next person does
not walk into the same wall.
_Disclosure: this analysis was put together with Gen-AI assistance (Claude
Code). Every file, line number and field list above was verified by hand
against `main` at `299ca2c8` 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]
