RehanAhmad25 commented on PR #72499: URL: https://github.com/apache/airflow/pull/72499#issuecomment-5556952361
Both are real, thanks for catching these. Verified in source before fixing: **version caching:** I Confirmed `get_dag_for_run()` resolves via `dag_run.created_dag_version_id` (falling back to latest only when a run has no version of its own), so my `dag_id`-only cache was genuinely wrong, exactly as you described. Fixed by dropping the cache entirely: `dag_bag` already caches internally by `dag_version_id` (`self._dags: MutableMapping[UUID | str, _CacheEntry]`), so calling `get_dag_for_run()` per run stays cheap without an extra, incorrect layer on top. Added the regression test you suggested (two runs of the same `dag_id`, different Dag versions, only the second declares `format="password"`). One thing worth flagging in case anyone else writes a similar test here: `dag_maker` defaults `bundle_version=None`, and `DBDagBag._version_from_dag_run()` falls back to the *latest* Dag version whenever `bundle_version` is falsy, regardless of `created_dag_version_id`. My first pass at the test used the default and got a false failure that looked like the fix itself was broken, both runs resolved to the latest version rather than their own. Fixed by explicitly pinning `created_dag_version_id` and `bundle_version` on each run in the test. This correctness fix does add a small, bounded number of extra DB queries per request (one Dag-version resolve+fetch per distinct version among the returned runs, not per row, thanks to `dag_bag`'s own internal cache), which I've reflected in the existing `assert_queries_count` baselines in this file along with comments explaining the delta. **trigger endpoint:** It is also correct, and while auditing every response-construction site in the file to make sure nothing else was missed, found a second unmasked spot: `patch_dag_run` (the PATCH endpoint used for state/note updates) also returned the raw ORM object directly. Fixed both: - `trigger_dag_run` now masks using `context_dag`, which was already resolved in scope (accounts for `bundle_version` override, since that's the exact Dag version `body.validate_context()` validated against). - `patch_dag_run` now masks using the `dag` already resolved earlier in the function. - Also fixed `clear_dag_runs` (bulk clear), which had the same unmasked-raw-return pattern, reusing the existing `_build_masked_dag_run_responses` helper. Added tests for the trigger and patch response paths too. Full test file passes clean (376 passed). Pushed as a follow-up commit. -- 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]
