Skip to content

Conversation

@vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Apr 3, 2025

Resolves #48724.


^ 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 airflow-core/newsfragments.

@tirkarthi
Copy link
Contributor

tirkarthi commented Apr 3, 2025

Is it a good idea to change the signature too to User | None for correctness so that mypy can catch other places where None needs to be handled but I guess it will have more effort in changing all functions that use it.

def deserialize_user(self, token: dict[str, Any]) -> User:

@vincbeck
Copy link
Contributor Author

vincbeck commented Apr 3, 2025

Is it a good idea to change the signature too to User | None for correctness so that mypy can catch other places where None needs to be handled but I guess it will have more effort in changing all functions that use it.

def deserialize_user(self, token: dict[str, Any]) -> User:

This is a very edge case of FAB auth manager. I am not willing to update the auth manager interface and therefore all auth managers for this very specific edge case very specific to FAB auth manager.

@vincbeck vincbeck merged commit a321935 into apache:main Apr 3, 2025
63 checks passed
@vincbeck vincbeck deleted the vincbeck/fab_delete_user branch April 3, 2025 21:17
@tirkarthi
Copy link
Contributor

Is it a good idea to change the signature too to User | None for correctness so that mypy can catch other places where None needs to be handled but I guess it will have more effort in changing all functions that use it.

def deserialize_user(self, token: dict[str, Any]) -> User:

This is a very edge case of FAB auth manager. I am not willing to update the auth manager interface and therefore all auth managers for this very specific edge case very specific to FAB auth manager.

Ok, the suggestion and link was to change it in fab auth manager only for deserialize_user .

@vincbeck
Copy link
Contributor Author

vincbeck commented Apr 3, 2025

deserialize_user

The issue is, deserialize_user is defined in the auth manager interface BaseAuthManager. So if I change the signature of this method in FAB auth manager, I need to update it in BaseAuthManager as well otherwise mypy will complain. And if I change it there, then all auth managers need to reflect this change. I agree with you, ideally we should update the type to reflect the reality but I do not think this is worth it given this is a very special case

@tirkarthi
Copy link
Contributor

Got it. I didn't know BaseAuthManager also needs to be updated for mypy. Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error when logged in user is removed in database using FAB manager

3 participants