-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Migrate FAB POST /users to FastAPI #57480
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
1924364 to
6374f0d
Compare
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.
One question as well: what about the old APIs we are replacing? Are you planning to delete them once all the APIs are implemented?
| resource: ResourceResponse | ||
|
|
||
|
|
||
| class RoleRef(BaseModel): |
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.
| class RoleRef(BaseModel): | |
| class Role(BaseModel): |
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 also realized in this file we have models such as ActionResourceResponse that is used in responses but requests as well. In that case could you rename them ActionResource. Same for ActionResponse and ResourceResponse
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 also realized in this file we have models such as
ActionResourceResponsethat is used in responses but requests as well. In that case could you rename themActionResource. Same forActionResponseandResourceResponse
may i fix this in the next PR? I want to keep the scope of this PR to /users API
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.
For sure 👌 Please do not forget, that's the only thing :)
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.
here's the PR: #58069
@vincbeck, yes that's the plan! |
6374f0d to
c8da155
Compare
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.
The latest commit fixes a MyPy error:
Run mypy for dev.................................................................Failed
- hook id: mypy-dev
- exit code: 1
Using 'uv' to install Airflow
Using airflow version from current sources
scripts/in_container/install_airflow_and_providers.py:438: error: Item "None"
of "Match[str] | None" has no attribute "groups" [union-attr]
owner, repo, branch = repo_match.groups()
^~~~~~~~~~~~~~~~~
Found 1 error in 1 file (checked 45 source files)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! Although this is a small change but it would be better to split it into another PR.
Since it will be much more easier to backport. Thanks!
|
Some conflicts |
c8da155 to
7549439
Compare
7549439 to
02b7b5e
Compare
jason810496
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.
Nice! LGTM overall.
| if TYPE_CHECKING: | ||
| from airflow.providers.fab.auth_manager.api_fastapi.datamodels.roles import Role |
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.
It seems we don't need the TYPE_CHECKING as we already imported in line 25.
| if TYPE_CHECKING: | |
| from airflow.providers.fab.auth_manager.api_fastapi.datamodels.roles import Role |
| created_on: datetime | None = None | ||
| changed_on: datetime | None = None | ||
|
|
||
| @field_validator("created_on", "changed_on") | ||
| @classmethod | ||
| def _coerce_tzaware(cls, v: datetime | None) -> datetime | None: | ||
| if v is None: | ||
| return None | ||
| return v if (v.tzinfo and v.utcoffset() is not None) else v.replace(tzinfo=timezone.utc) |
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.
Would it be possible to leverage UtcDateTime ? Then we don't need to handle the common timezone validation for this case.
airflow/airflow-core/src/airflow/api_fastapi/execution_api/datamodels/taskinstance.py
Lines 282 to 283 in bc3a750
| timestamp: UtcDateTime | |
| if TYPE_CHECKING: | ||
| from airflow.providers.fab.auth_manager.api_fastapi.datamodels.users import UserBody, UserResponse |
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.
| if TYPE_CHECKING: | |
| from airflow.providers.fab.auth_manager.api_fastapi.datamodels.users import UserBody, UserResponse |
| if TYPE_CHECKING: | ||
| from airflow.providers.fab.auth_manager.api_fastapi.datamodels.users import UserBody, UserResponse | ||
|
|
||
| users_router = AirflowRouter(prefix="/fab/v1", tags=["FabAuthManager"]) |
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.
Non-blocking nit, can be done in follow-up PR:
How about adding a common root router for all providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/<entity>.py? Then we could specified the prefix and tags in single source.
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! Although this is a small change but it would be better to split it into another PR.
Since it will be much more easier to backport. Thanks!
|
Are you planning to update this PR? |
|
Completed in #60523 |
Why
POST /auth/fab/v1/users.How
Some differences from original implementation:
created_on/changed_onto UTC tz-aware on output to avoid mixed-aware states. This intentionally differs from the oldmarshmallowbehavior (which validates timezone and may reject naive datetimes unless configured) and aligns with Airflow’s “store in UTC” policy; implemented via apydanticfield_validator(default after parsing).None.^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.