Copilot commented on code in PR #64089:
URL: https://github.com/apache/airflow/pull/64089#discussion_r3025378197
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/backfills.py:
##########
@@ -36,6 +36,7 @@
from airflow.api_fastapi.core_api.openapi.exceptions import (
create_openapi_http_exception_doc,
)
+from airflow.api_fastapi.core_api.routes.public.backfills import
_enrich_backfill_responses
Review Comment:
The UI router is importing `_enrich_backfill_responses` from the public
router module. Sharing logic via a private helper in another router creates a
brittle cross-route dependency (and makes refactors/routing changes riskier).
Consider moving the enrichment function into a shared module (e.g. a
`services/backfills.py` or `api_fastapi/core_api/common/backfills.py`) and
importing it from both routers.
##########
airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml:
##########
@@ -1483,6 +1483,16 @@ components:
dag_display_name:
type: string
title: Dag Display Name
+ num_runs:
+ type: integer
+ title: Num Runs
+ default: 0
+ dag_run_state_counts:
+ additionalProperties:
+ type: integer
+ type: object
+ title: Dag Run State Counts
+ default: {}
Review Comment:
This PR updates `_private_ui.yaml` (an OpenAPI artifact) directly. Please
ensure this file is regenerated via the repo’s OpenAPI generation process
rather than hand-edited, to keep generated specs consistent and reproducible.
##########
airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml:
##########
@@ -9296,6 +9296,16 @@ components:
dag_display_name:
type: string
title: Dag Display Name
+ num_runs:
+ type: integer
+ title: Num Runs
+ default: 0
+ dag_run_state_counts:
+ additionalProperties:
+ type: integer
+ type: object
+ title: Dag Run State Counts
+ default: {}
Review Comment:
This PR updates `v2-rest-api-generated.yaml` directly. In Airflow these
OpenAPI specs are generated artifacts; please ensure they’re produced via the
repo’s OpenAPI generation workflow rather than hand-edited, so the change is
reproducible and stays in sync with the source models.
```suggestion
```
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/backfills.py:
##########
@@ -54,6 +54,8 @@ class BackfillResponse(BaseModel):
completed_at: datetime | None
updated_at: datetime
dag_display_name: str = Field(validation_alias=AliasPath("dag_model",
"dag_display_name"))
+ num_runs: int = 0
+ dag_run_state_counts: dict[str, int] = {}
Review Comment:
`dag_run_state_counts` is initialized with a mutable default `{}`. This can
lead to shared state between `BackfillResponse` instances if the dict is ever
mutated. Use `Field(default_factory=dict)` for this field (and similarly
consider `BackfillPostBody.dag_run_conf: dict = {}` which has the same issue).
##########
airflow-ctl/src/airflowctl/api/datamodels/generated.py:
##########
@@ -1135,6 +1135,8 @@ class BackfillResponse(BaseModel):
completed_at: Annotated[datetime | None, Field(title="Completed At")] =
None
updated_at: Annotated[datetime, Field(title="Updated At")]
dag_display_name: Annotated[str, Field(title="Dag Display Name")]
+ num_runs: Annotated[int | None, Field(title="Num Runs")] = 0
+ dag_run_state_counts: Annotated[dict[str, int] | None, Field(title="Dag
Run State Counts")] = {}
Review Comment:
In this generated model, `num_runs`/`dag_run_state_counts` are typed as
optional but default to non-None values, and `dag_run_state_counts` uses a
mutable default `{}`. This pattern is repeated elsewhere in this file (e.g.
`dag_run_conf = {}`), and can lead to confusing typing and shared mutable
state. If the generator can be configured, prefer generating non-optional
fields with `default_factory` for dict/list defaults to avoid these issues.
##########
airflow-core/src/airflow/ui/src/components/BackfillProgressBar.tsx:
##########
@@ -0,0 +1,63 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, HStack, Text } from "@chakra-ui/react";
+
+type Props = {
+ readonly stateCounts: Record<string, number>;
+ readonly total: number;
+ readonly trackColor?: string;
+};
+
+export const BackfillProgressBar = ({ stateCounts, total, trackColor =
"bg.emphasized" }: Props) => {
+ const successCount = stateCounts.success ?? 0;
+ const failedCount = stateCounts.failed ?? 0;
+ const successPct = (successCount / total) * 100;
+ const failedPct = (failedCount / total) * 100;
+ const remainingPct = 100 - successPct - failedPct;
+
Review Comment:
`successPct`/`failedPct` divide by `total` without guarding against `total
=== 0`. Current call sites check for `total === 0`, but this component is
exported and could be reused elsewhere; with `total=0` it will yield
`Infinity/NaN` widths. Add an internal guard (e.g., treat `total <= 0` as
0%/0%/100% and render `0/0`).
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/backfills.py:
##########
@@ -61,6 +62,40 @@
backfills_router = AirflowRouter(tags=["Backfill"], prefix="/backfills")
+def _enrich_backfill_responses(
+ backfills: list[BackfillResponse],
+ *,
+ session: Session,
+) -> list[BackfillResponse]:
+ """Populate num_runs and dag_run_state_counts on each backfill response."""
+ ids = [b.id for b in backfills]
+ if not ids:
+ return backfills
+ # Single query: get state counts per backfill, derive num_runs by summing
counts.
+ rows = session.execute(
+ select(
+ BackfillDagRun.backfill_id,
+ DagRun.state,
+ func.count().label("count"),
+ )
+ .join(DagRun, BackfillDagRun.dag_run_id == DagRun.id)
+ .where(
+ BackfillDagRun.backfill_id.in_(ids),
+ DagRun.backfill_id == BackfillDagRun.backfill_id,
+ )
+ .group_by(BackfillDagRun.backfill_id, DagRun.state)
+ ).all()
+ counts: dict[int, dict[str, int]] = defaultdict(dict)
+ num_runs: dict[int, int] = defaultdict(int)
+ for backfill_id, state, count in rows:
+ counts[backfill_id][state] = count
+ num_runs[backfill_id] += count
Review Comment:
`DagRun.state` is nullable (stored in `_state` with `nullable=True`), so
this aggregation can produce a NULL `state` group. That would populate
`dag_run_state_counts` with a `None` key (and potentially break JSON
serialization or UI expectations). Consider filtering out NULL states
(`DagRun.state.is_not(None)`) or mapping them to a stable string using
`func.coalesce` in the SELECT.
##########
airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts:
##########
@@ -147,6 +147,10 @@ export type BackfillResponse = {
completed_at: string | null;
updated_at: string;
dag_display_name: string;
+ num_runs?: number;
+ dag_run_state_counts?: {
+ [key: string]: (number);
Review Comment:
This file is under `openapi-gen/` and appears to be generated. Please
confirm it was regenerated using the project’s OpenAPI client generation
tooling (not manually edited), and consider documenting the regen command in
the PR description if it’s not already included.
```suggestion
[key: string]: number;
```
--
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]
