Skip to content

Return only the latest Dag version from iter_all_latest_version_dags - #70935

Open
Eason09053360 wants to merge 2 commits into
apache:mainfrom
Eason09053360:fix-iter-all-latest-version-dags
Open

Return only the latest Dag version from iter_all_latest_version_dags#70935
Eason09053360 wants to merge 2 commits into
apache:mainfrom
Eason09053360:fix-iter-all-latest-version-dags

Conversation

@Eason09053360

Copy link
Copy Markdown
Contributor

The method promised latest-version dags in its name and docstring but read every serialized_dag row, so FAB permission syncing processed each Dag once per version and could apply an outdated access_control last.

DBDagBag.iter_all_latest_version_dags promises latest-version Dags in both its
name and its docstring, but it read every serialized_dag row without filtering
by version.

Its only production caller is FAB's create_dag_specific_permissions, so DAG-level
permission syncing iterated each Dag once per version. Besides the wasted work, the
query has no ORDER BY, so when versions carry different access_control the one
applied last is undefined and an outdated policy can win.

SerializedDagModel._latest_by_version_select() already supported "all dags" via
dag_ids=None; this exposes that through the public get_latest_serialized_dags()
wrapper, which keeps the existing load_op_links hook in the loop intact. All five
existing callers pass dag_ids= explicitly, so the signature change is backwards
compatible.

The existing test mocked session.scalars, so it could not catch a wrong query.
Added a regression test that writes two real versions to the database.


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

Generated-by: Claude Code (Opus 5) following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

The method promised latest-version dags in its name and docstring but read
every serialized_dag row, so FAB permission syncing processed each Dag once
per version and could apply an outdated access_control last.
@Eason09053360
Eason09053360 marked this pull request as ready for review August 2, 2026 08:36
@Eason09053360
Eason09053360 force-pushed the fix-iter-all-latest-version-dags branch from 3b3e4a6 to b40af4f Compare August 2, 2026 17:29

@SameerMesiah97 SameerMesiah97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment on the tests.

db.clear_db_runs()
db.clear_db_dags()
db.clear_db_serialized_dags()
db.clear_db_dag_bundles()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add a regression covering multiple DAG IDs with multiple versions, to explicitly verify we return the latest version per DAG rather than just the latest serialized row overall?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. With a single Dag the test couldn't distinguish a per-Dag max from a
global one, so group_by(dag_id) was never exercised .
I confirmed the old test still passed with that group_by removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants