Skip to content

Tidy DagVersion/DagCode metadata helpers#68627

Open
ephraimbuddy wants to merge 2 commits into
apache:mainfrom
astronomer:dag-version-metadata-hygiene
Open

Tidy DagVersion/DagCode metadata helpers#68627
ephraimbuddy wants to merge 2 commits into
apache:mainfrom
astronomer:dag-version-metadata-hygiene

Conversation

@ephraimbuddy

Copy link
Copy Markdown
Contributor
  • get_version: use 'if version_number is not None' so version_number=0 filters (and 404s) instead of falling through to the latest version.
  • bundle_url: drop the dead hasattr(self.bundle, 'signed_url_template') guard (always true on the ORM model). Behavior is unchanged: a loaded bundle uses render_url(), and the deprecated manager lookup is used only when there is no bundle row -- not on every call for bundles without a URL template.
  • DagCode.update_source_code: refresh fileloc alongside the source, and drop the redundant session.merge() on the already-session-attached row.

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

claude opus 4.8

Comment thread airflow-core/src/airflow/models/dagcode.py

Copilot AI 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.

Pull request overview

This PR tidies up Airflow’s DAG version/code metadata helpers by fixing edge-case filtering behavior, simplifying bundle URL resolution logic, and ensuring DagCode.update_source_code() keeps related metadata consistent.

Changes:

  • Fix DagVersion.get_version() so version_number=0 is treated as a real filter (instead of falling back to “latest”).
  • Simplify DagVersion.bundle_url by relying on DagBundleModel.render_url() when a bundle row is present and only using the deprecated manager path when no bundle row exists.
  • Update DagCode.update_source_code() to refresh fileloc when the source is refreshed, and add regression coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
airflow-core/src/airflow/models/dag_version.py Fixes get_version zero-filter behavior; refines bundle_url selection logic.
airflow-core/src/airflow/models/dagcode.py Updates update_source_code to keep fileloc aligned when refreshing stored source.
airflow-core/tests/unit/models/test_dag_version.py Adds regression test ensuring version_number=0 filters rather than returning latest.
airflow-core/tests/unit/models/test_dagcode.py Adds regression test verifying update_source_code refreshes stale fileloc on update path.

Comment thread airflow-core/src/airflow/models/dag_version.py Outdated

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread airflow-core/src/airflow/models/dagcode.py
@ephraimbuddy ephraimbuddy self-assigned this Jun 17, 2026

@phanikumv phanikumv 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.

CI is red on all four core DB jobs because of the bundle_url guard removal, a
test that was dependent on this logic needs fixing

Suggested replacement for airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_versions.py here

@pytest.mark.usefixtures("make_dag_with_multiple_versions")
@mock.patch("airflow.models.dagbundle.DagBundleModel.render_url", return_value=None)
def test_get_dag_version_with_unconfigured_bundle(self, mock_render_url, test_client):
    """A bundle row with no URL template yields an empty bundle_url."""
    response = test_client.get("/dags/dag_with_multiple_versions/dagVersions/1")
    assert response.status_code == 200
    assert not response.json()["bundle_url"]

- get_version: use 'if version_number is not None' so version_number=0 filters
  (and 404s) instead of falling through to the latest version.
- bundle_url: drop the dead hasattr(self.bundle, 'signed_url_template') guard
  (always true on the ORM model). Behavior is unchanged: a loaded bundle uses
  render_url(), and the deprecated manager lookup is used only when there is no
  bundle row -- not on every call for bundles without a URL template.
- DagCode.update_source_code: refresh fileloc alongside the source, and drop the
  redundant session.merge() on the already-session-attached row.

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@phanikumv phanikumv 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.

The PR tests the internal function get_version(), users never call it directly . Worth adding a test that calls the actual API endpoint GET /dagSources/{dag_id}?version_number=0, and asserts that it gets a 404

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants