Re: [I] Remove duplicated dag_id and run_id fields from Execution API DagRun schema [airflow]

["stantheman0128 (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org