-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Migrate Flask based user APIs to Fastapi #60973
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
Conversation
- Add GET /fab/v1/users (list with pagination/ordering)
- Add GET /fab/v1/users/{username}
- Add PATCH /fab/v1/users/{username} with update_mask support
- Add DELETE /fab/v1/users/{username}
- Add UserPatchBody and UserCollectionResponse datamodels
- Add comprehensive tests for routes, services, and datamodels
closes apache#60946
|
Thanks for the PR! I just have one question: It seems user_endpoint.py hasn't been migrated to FastAPI yet, but the commit message says it has. Did we miss this file? |
Thanks for the question! The new FastAPI endpoints have been created in
The old Connexion file ( The migration approach is:
|
vincbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Try running |
Thanks! Ran |
|
@vincbeck CI has 2 failing checks:
The error happens during test setup, suggesting a TestClient compatibility issue with Airflow 3.0.6, not my code. Could you confirm if these are known issues? Happy to investigate further if needed. |
henry3260
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the CI failed because returning None results in a null response body, but HTTP 204 responses must not have a body. Please return an empty Response object instead.
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/users.py
Show resolved
Hide resolved
henry3260
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix. LGTM.
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* changes
* Replace connexion with FastAPI: user_endpoint.py
- Add GET /fab/v1/users (list with pagination/ordering)
- Add GET /fab/v1/users/{username}
- Add PATCH /fab/v1/users/{username} with update_mask support
- Add DELETE /fab/v1/users/{username}
- Add UserPatchBody and UserCollectionResponse datamodels
- Add comprehensive tests for routes, services, and datamodels
closes apache#60946
This PR migrates the remaining user endpoints from Connexion to FastAPI as part of #56730.
What's Changed
New endpoints in
api_fastapi/routes/users.py:GET /fab/v1/users- List users with pagination and orderingGET /fab/v1/users/{username}- Get a single user by usernamePATCH /fab/v1/users/{username}- Update user withupdate_masksupportDELETE /fab/v1/users/{username}- Delete a userNew datamodels in
api_fastapi/datamodels/users.py:UserPatchBody- Request model for partial updates (all fields optional)UserCollectionResponse- Response model for paginated user listNew service methods in
api_fastapi/services/users.py:get_user()- Retrieve single userget_users()- List users with pagination/orderingupdate_user()- Partial update withupdate_masksupportdelete_user()- Remove userTests added:
UserPatchBodyandUserCollectionResponseNotes
roles.pyimplementationupdate_maskuses comma-separated string format (consistent with roles endpoint)closes #60946
Was generative AI tooling used to co-author this PR?
Generated-by: Claude following the guidelines
Claude was used for: