pierrejeambrun commented on code in PR #44043:
URL: https://github.com/apache/airflow/pull/44043#discussion_r1843745093
##########
airflow/api_fastapi/core_api/routes/ui/dashboard.py:
##########
@@ -49,12 +49,18 @@ def historical_metrics(
session: Annotated[Session, Depends(get_session)],
) -> HistoricalMetricDataResponse:
"""Return cluster activity historical metrics."""
+ if start_date is None:
Review Comment:
`DateTimeQuery` need to be adjusted, because by definition, it cannot return
`str | None`, only `str`:
```
DateTimeQuery = Annotated[str, AfterValidator(_safe_parse_datetime)]
```
Maybe create a `OptionalDateTimeQuery = Annotated[str | None,
AfterValidator(_safe_parse_datetime)]`, or maybe `DateTimeQuery | None` already
works
##########
airflow/api_fastapi/core_api/routes/ui/dashboard.py:
##########
@@ -49,12 +49,18 @@ def historical_metrics(
session: Annotated[Session, Depends(get_session)],
) -> HistoricalMetricDataResponse:
"""Return cluster activity historical metrics."""
+ if start_date is None:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail="start_date parameter is required in the request",
+ )
+
Review Comment:
This should be handled automatically by FastAPI. If the Query parameter is
properly defined, we shouldn't allow `None`. No need to handle that manually
here. (Also that would be a 422 and not a 400, because this is how fastapi
handles validation errors)
##########
airflow/api_fastapi/common/parameters.py:
##########
@@ -365,14 +365,16 @@ def depends(self, tag_name_pattern: str | None = None) ->
_DagTagNamePatternSear
return self.set_value(tag_name_pattern)
-def _safe_parse_datetime(date_to_check: str) -> datetime:
+def _safe_parse_datetime(date_to_check: str) -> datetime | None:
Review Comment:
argument type need to be adjusted
--
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]
