pierrejeambrun commented on code in PR #62583:
URL: https://github.com/apache/airflow/pull/62583#discussion_r3023041560
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_deadlines.py:
##########
@@ -310,3 +312,207 @@ def test_should_response_401(self,
unauthenticated_test_client):
def test_should_response_403(self, unauthorized_test_client):
response =
unauthorized_test_client.get(f"/dags/{DAG_ID}/dagRuns/{RUN_EMPTY}/deadlines")
assert response.status_code == 403
+
+
+class TestGetDeadlines:
+ """Tests for GET /dags/{dag_id}/dagRuns/{dag_run_id}/deadlines with ~
wildcards."""
+
+ # ------------------------------------------------------------------
+ # 200 – happy paths
+ # ------------------------------------------------------------------
+
+ def test_returns_all_deadlines(self, test_client):
+ """All deadlines across all runs are returned when both dag_id and
dag_run_id are ~."""
+ response = test_client.get("/dags/~/dagRuns/~/deadlines")
+ assert response.status_code == 200
+ data = response.json()
+ # Fixture creates: 1 (run_single) + 1 (run_missed) + 1 (run_alert) + 3
(run_multi) + 1 (run_other) = 7
+ assert data["total_entries"] == 7
+ assert len(data["deadlines"]) == 7
+
+ def test_response_includes_dag_and_run_ids(self, test_client):
+ """Each deadline includes dag_id and dag_run_id for linking."""
+ response = test_client.get(f"/dags/{DAG_ID}/dagRuns/~/deadlines",
params={"limit": 1})
+ assert response.status_code == 200
+ dl = response.json()["deadlines"][0]
+ assert dl["dag_id"] == DAG_ID
+ assert "dag_run_id" in dl
+ assert "id" in dl
+ assert "deadline_time" in dl
+ assert "missed" in dl
Review Comment:
merge this with previous test. No need to have 1 test for payload 1 test for
number of responses etc...
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -44,3 +44,58 @@ class DeadlineCollectionResponse(BaseModel):
deadlines: Iterable[DeadlineResponse]
total_entries: int
+
+
+class DeadlineWithDagRunResponse(BaseModel):
+ """Deadline serializer for responses that includes DAG and DAG run
identifiers."""
+
+ id: UUID
+ deadline_time: datetime
+ missed: bool
+ created_at: datetime
+ dag_id: str = Field(validation_alias=AliasPath("dagrun", "dag_id"))
+ dag_run_id: str = Field(validation_alias=AliasPath("dagrun", "run_id"))
+ alert_name: str | None =
Field(validation_alias=AliasPath("deadline_alert", "name"), default=None)
+ alert_description: str | None = Field(
+ validation_alias=AliasPath("deadline_alert", "description"),
default=None
+ )
Review Comment:
Why do we duplicate the serializer ? Can't we just adds necessary stuff to
`DeadlineResponse` and use that only?
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_deadlines.py:
##########
@@ -310,3 +312,207 @@ def test_should_response_401(self,
unauthenticated_test_client):
def test_should_response_403(self, unauthorized_test_client):
response =
unauthorized_test_client.get(f"/dags/{DAG_ID}/dagRuns/{RUN_EMPTY}/deadlines")
assert response.status_code == 403
+
+
+class TestGetDeadlines:
+ """Tests for GET /dags/{dag_id}/dagRuns/{dag_run_id}/deadlines with ~
wildcards."""
+
+ # ------------------------------------------------------------------
+ # 200 – happy paths
+ # ------------------------------------------------------------------
+
+ def test_returns_all_deadlines(self, test_client):
+ """All deadlines across all runs are returned when both dag_id and
dag_run_id are ~."""
+ response = test_client.get("/dags/~/dagRuns/~/deadlines")
+ assert response.status_code == 200
+ data = response.json()
+ # Fixture creates: 1 (run_single) + 1 (run_missed) + 1 (run_alert) + 3
(run_multi) + 1 (run_other) = 7
+ assert data["total_entries"] == 7
+ assert len(data["deadlines"]) == 7
Review Comment:
There is only 1 dag in the test. so there is no cross dag filtering. For
this test to be relevant we need another 'dag' and dealines associatied, to
make sure deadlines for all dags are returned
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_deadlines.py:
##########
@@ -310,3 +312,207 @@ def test_should_response_401(self,
unauthenticated_test_client):
def test_should_response_403(self, unauthorized_test_client):
response =
unauthorized_test_client.get(f"/dags/{DAG_ID}/dagRuns/{RUN_EMPTY}/deadlines")
assert response.status_code == 403
+
+
+class TestGetDeadlines:
+ """Tests for GET /dags/{dag_id}/dagRuns/{dag_run_id}/deadlines with ~
wildcards."""
+
+ # ------------------------------------------------------------------
+ # 200 – happy paths
+ # ------------------------------------------------------------------
+
Review Comment:
Nit, remove all similar comments. (401, 403 etc....)
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -44,3 +44,58 @@ class DeadlineCollectionResponse(BaseModel):
deadlines: Iterable[DeadlineResponse]
total_entries: int
+
+
+class DeadlineWithDagRunResponse(BaseModel):
+ """Deadline serializer for responses that includes DAG and DAG run
identifiers."""
+
+ id: UUID
+ deadline_time: datetime
+ missed: bool
+ created_at: datetime
+ dag_id: str = Field(validation_alias=AliasPath("dagrun", "dag_id"))
+ dag_run_id: str = Field(validation_alias=AliasPath("dagrun", "run_id"))
+ alert_name: str | None =
Field(validation_alias=AliasPath("deadline_alert", "name"), default=None)
+ alert_description: str | None = Field(
+ validation_alias=AliasPath("deadline_alert", "description"),
default=None
+ )
+
+
+class DeadlineWithDagRunCollectionResponse(BaseModel):
+ """Deadline Collection serializer for responses that includes DAG and DAG
run identifiers."""
+
+ deadlines: Iterable[DeadlineWithDagRunResponse]
+ total_entries: int
+
+
+class DeadlineAlertResponse(BaseModel):
+ """DeadlineAlert serializer for responses."""
+
+ id: UUID
+ name: str | None = None
+ description: str | None = None
+ reference_type: str
+ interval: float
Review Comment:
Nit: interval Field Unit is Ambiguous
The DeadlineAlertResponse.interval field is a float with no documentation of
its unit. The test asserts alert["interval"] == 3600.0 which suggests seconds,
but the API contract doesn't state this. A docstring or field description
(e.g., Field(description="Interval in seconds")) would clarify the contract for
API consumers.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -44,3 +44,58 @@ class DeadlineCollectionResponse(BaseModel):
deadlines: Iterable[DeadlineResponse]
total_entries: int
+
+
+class DeadlineWithDagRunResponse(BaseModel):
+ """Deadline serializer for responses that includes DAG and DAG run
identifiers."""
+
+ id: UUID
+ deadline_time: datetime
+ missed: bool
+ created_at: datetime
+ dag_id: str = Field(validation_alias=AliasPath("dagrun", "dag_id"))
+ dag_run_id: str = Field(validation_alias=AliasPath("dagrun", "run_id"))
+ alert_name: str | None =
Field(validation_alias=AliasPath("deadline_alert", "name"), default=None)
+ alert_description: str | None = Field(
+ validation_alias=AliasPath("deadline_alert", "description"),
default=None
+ )
+
+
+class DeadlineWithDagRunCollectionResponse(BaseModel):
+ """Deadline Collection serializer for responses that includes DAG and DAG
run identifiers."""
+
+ deadlines: Iterable[DeadlineWithDagRunResponse]
+ total_entries: int
+
+
+class DeadlineAlertResponse(BaseModel):
+ """DeadlineAlert serializer for responses."""
+
+ id: UUID
+ name: str | None = None
+ description: str | None = None
+ reference_type: str
+ interval: float
+ created_at: datetime
+
+ @model_validator(mode="before")
+ @classmethod
+ def extract_reference_type(cls, data):
+ """Extract reference_type from the JSON reference column when
validating from an ORM object."""
+ if not isinstance(data, dict) and hasattr(data, "reference"):
+ return {
+ "id": data.id,
+ "name": data.name,
+ "description": data.description,
+ "reference_type": data.reference.get("reference_type", "") if
data.reference else "",
+ "interval": data.interval,
+ "created_at": data.created_at,
+ }
+ return data
Review Comment:
A more robust approach would be to use a computed field or a single
@field_validator for just reference_type, letting Pydantic's from_attributes
handle the other fields normally. For example, using
validation_alias=AliasPath("reference", "reference_type") (which is how the
existing DeadlineWithDagRunResponse handles nested fields like dag_id).
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/__init__.py:
##########
@@ -49,4 +48,5 @@
ui_router.include_router(grid_router)
ui_router.include_router(gantt_router)
ui_router.include_router(calendar_router)
+ui_router.include_router(partitioned_dag_runs_router)
Review Comment:
Why this re-order? This creates a bigger code diff. Is there route
shadowing?
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_deadlines.py:
##########
@@ -310,3 +312,207 @@ def test_should_response_401(self,
unauthenticated_test_client):
def test_should_response_403(self, unauthorized_test_client):
response =
unauthorized_test_client.get(f"/dags/{DAG_ID}/dagRuns/{RUN_EMPTY}/deadlines")
assert response.status_code == 403
+
+
+class TestGetDeadlines:
+ """Tests for GET /dags/{dag_id}/dagRuns/{dag_run_id}/deadlines with ~
wildcards."""
+
+ # ------------------------------------------------------------------
+ # 200 – happy paths
+ # ------------------------------------------------------------------
+
+ def test_returns_all_deadlines(self, test_client):
+ """All deadlines across all runs are returned when both dag_id and
dag_run_id are ~."""
+ response = test_client.get("/dags/~/dagRuns/~/deadlines")
+ assert response.status_code == 200
+ data = response.json()
+ # Fixture creates: 1 (run_single) + 1 (run_missed) + 1 (run_alert) + 3
(run_multi) + 1 (run_other) = 7
+ assert data["total_entries"] == 7
+ assert len(data["deadlines"]) == 7
+
+ def test_response_includes_dag_and_run_ids(self, test_client):
+ """Each deadline includes dag_id and dag_run_id for linking."""
+ response = test_client.get(f"/dags/{DAG_ID}/dagRuns/~/deadlines",
params={"limit": 1})
+ assert response.status_code == 200
+ dl = response.json()["deadlines"][0]
+ assert dl["dag_id"] == DAG_ID
+ assert "dag_run_id" in dl
+ assert "id" in dl
+ assert "deadline_time" in dl
+ assert "missed" in dl
+
+ def test_filter_by_dag_id(self, test_client):
+ """Specifying a dag_id path param (with ~ for dag_run_id) returns only
that DAG's deadlines."""
+ response = test_client.get(f"/dags/{DAG_ID}/dagRuns/~/deadlines")
+ assert response.status_code == 200
+ data = response.json()
+ assert data["total_entries"] == 7
+ for dl in data["deadlines"]:
+ assert dl["dag_id"] == DAG_ID
Review Comment:
We need a second dag with it's set of deadlines to make sure filtering is
working.
--
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]
