Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AIP-84 Fix: Allow Null Values for end_date Field in Dashboard Endpint in FastAPI #44043

Conversation

bugraoz93
Copy link
Collaborator

@bugraoz93 bugraoz93 commented Nov 14, 2024

related: #43846

  • Allow safe_date_time to return None if the input is None
  • Handle None in queries/endpoints
  • Simplify unit tests

I have created a separate PR because write permissions don't allow me to push to the existing PR.
cc: @bbovenzi @pierrejeambrun @tirkarthi


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@bugraoz93 bugraoz93 changed the title Allow safe_date_time to return none if input none, handle None in que… AIP-84 Fix: Allow Null Values for Dashboard Endpint in FastAPI Nov 14, 2024
@bugraoz93 bugraoz93 changed the title AIP-84 Fix: Allow Null Values for Dashboard Endpint in FastAPI AIP-84 Fix: Allow Null Values for end_date Field in Dashboard Endpint in FastAPI Nov 14, 2024
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Thanks a few suggestions

airflow/api_fastapi/common/parameters.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/ui/dashboard.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/ui/dashboard.py Outdated Show resolved Hide resolved
@tirkarthi
Copy link
Contributor

The following were added in #43934 . The changes broke dashboard endpoint probably due str changed to datetime . During resolving merge conflicts maybe OptionalDateTimeQuery could be used for end_date which hopefully resolves this PR's use case.

DateTimeQuery = Annotated[datetime, AfterValidator(_safe_parse_datetime)]
OptionalDateTimeQuery = Annotated[Union[datetime, None], AfterValidator(_safe_parse_datetime_optional)]

https://github.com/apache/airflow/pull/43934/files#diff-183a76763667e545ddd8c2aadf6858fdd7a80fecdd04b71e61eaa4ff344239cdR638-R639

@bugraoz93 bugraoz93 force-pushed the bugfix/43846/allow-null-end-date-dashboard branch from a4992ad to ac48b0b Compare November 16, 2024 16:36
@bugraoz93
Copy link
Collaborator Author

Thanks a few suggestions

I’ve made the adjustments in the methods and let FastAPI handle it as a 422. I followed a similar approach to the changes you made in the other PR.

The following were added in #43934 . The changes broke dashboard endpoint probably due str changed to datetime . During resolving merge conflicts maybe OptionalDateTimeQuery could be used for end_date which hopefully resolves this PR's use case.

DateTimeQuery = Annotated[datetime, AfterValidator(_safe_parse_datetime)]
OptionalDateTimeQuery = Annotated[Union[datetime, None], AfterValidator(_safe_parse_datetime_optional)]

https://github.com/apache/airflow/pull/43934/files#diff-183a76763667e545ddd8c2aadf6858fdd7a80fecdd04b71e61eaa4ff344239cdR638-R639

I haven’t updated the annotation to datetime yet because, in airflow/utils/timezone.py, we’re parsing the date as a pendulum object to ensure consistent timezone handling, and it currently accepts a string. If we want to validate the datetime type directly in FastAPI and ensure it works with pendulum for proper timezone handling, we could introduce a new implementation using something like pendulum.instance(). However, I’m uncertain if adding that new capability is necessary. If it is, I am up for the change.

@bugraoz93
Copy link
Collaborator Author

FastAPI itself returns str for datetime objects in both requests and responses. As mentioned earlier, I’m not certain if there would be a meaningful difference since pendulum.parse() already supports the ISO 8601 format.

  • datetime.datetime:
    A Python datetime.datetime.
    In requests and responses will be represented as a str in ISO 8601 format, like: 2008-09-15T15:53:00+05:00.

https://fastapi.tiangolo.com/tutorial/extra-data-types/?h=datet#other-data-types

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looks good, let us just merge again the PR introducing OptionalDateTimeQuery (it got reverted because of CI issues not catching tests etc...) and we should be good to go

airflow/api_fastapi/core_api/routes/ui/dashboard.py Outdated Show resolved Hide resolved
@bugraoz93 bugraoz93 force-pushed the bugfix/43846/allow-null-end-date-dashboard branch from ac48b0b to 20484b1 Compare November 18, 2024 22:53
@bugraoz93
Copy link
Collaborator Author

Looks good, let us just merge again the PR introducing OptionalDateTimeQuery (it got reverted because of CI issues not catching tests etc...) and we should be good to go

Thanks for the quick reviews and for merging the changes again so promptly! I aimed to replicate the changes and test the endpoint accordingly. I also followed up on the CI failure, which helped me realize I needed to apply a similar fix to the CI script updates in another PR later on :)

@pierrejeambrun
Copy link
Member

Restarting failed job, ready to merge when CI is green.

@pierrejeambrun pierrejeambrun merged commit 8d2e96f into apache:main Nov 19, 2024
45 checks passed
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