-
Notifications
You must be signed in to change notification settings - Fork 164
refactor(BA-2966): Explicitly raise error when operation fails in auth repository #7177
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: octodog <[email protected]>
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.
Pull request overview
This PR refactors the authentication repository layer to explicitly raise exceptions when operations fail, replacing the previous pattern of returning None or False to indicate failure. This makes error handling more explicit and moves it closer to the source of the error.
Key Changes
- Modified
check_credential_with_migrationandcheck_credential_without_migrationto raiseAuthorizationFailedinstead of returningNone - Changed
update_user_full_nameto raiseUserNotFoundinstead of returningFalse - Updated
get_user_row_by_uuidto raiseUserNotFoundinstead of returningNone - Removed redundant null checks in the service layer since exceptions are now raised at the repository level
- Updated all tests to expect exceptions using
pytest.raises()and mockside_effectinstead of return values
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/manager/models/user.py | Modified check_credential_with_migration and check_credential to raise AuthorizationFailed instead of returning None. Updated docstrings to document the new exception behavior. Removed the query_user_by_uuid method. |
| src/ai/backend/manager/repositories/auth/db_source/db_source.py | Added new imports (Any, joinedload, selectinload, UserNotFound). Updated modify_user_full_name to raise UserNotFound instead of returning bool. Moved fetch_user_row_by_uuid implementation inline (from removed UserRow.query_user_by_uuid) and added exception raising. Changed return type annotations from Optional to non-optional. |
| src/ai/backend/manager/repositories/auth/repository.py | Updated method signatures to remove Optional return types for update_user_full_name, check_credential_with_migration, check_credential_without_migration, and get_user_row_by_uuid. |
| src/ai/backend/manager/services/auth/service.py | Removed UserNotFound import. Removed null checks after credential checks and user queries since exceptions are now raised. Updated update_password to use try-except for credential check. Removed redundant error handling in authorize, signout, update_full_name, and update_password_no_auth. |
| src/ai/backend/manager/api/auth.py | Removed error handling for update_full_name since exceptions are now propagated from the service layer. |
| tests/manager/services/auth/test_authorize.py | Updated to use side_effect with AuthorizationFailed exception instead of return_value = None. |
| tests/manager/services/auth/test_signout.py | Updated to expect AuthorizationFailed exception instead of GenericBadRequest. Changed mock to use side_effect instead of return_value. |
| tests/manager/services/auth/test_update_full_name.py | Updated test to expect UserNotFound exception. Changed mock to use side_effect. Removed @pytest.mark.asyncio decorator (issue identified). |
| tests/manager/services/auth/test_update_password.py | Updated to use side_effect with AuthorizationFailed exception. |
| tests/manager/services/auth/test_update_password_no_auth.py | Updated to use side_effect with AuthorizationFailed exception. |
| tests/manager/repositories/auth/test_auth_repository.py | Added new test cases for error scenarios: test_update_user_full_name_not_found and test_get_user_row_by_uuid_not_found. Added redundant local import of UserNotFound (issue identified). |
| changes/7177.enhance.md | Added changelog entry for this enhancement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @pytest.mark.asyncio | ||
| async def test_get_user_row_by_uuid_not_found(self, auth_repository: AuthRepository) -> None: | ||
| """Test getting user row by UUID when user doesn't exist""" | ||
| from ai.backend.common.exception import UserNotFound |
Copilot
AI
Dec 8, 2025
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.
Redundant import. UserNotFound is already imported at the top of the file (line 14), so this local import is unnecessary.
| from ai.backend.common.exception import UserNotFound |
| ) | ||
| assert result.success is True | ||
|
|
||
|
|
Copilot
AI
Dec 8, 2025
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.
Missing @pytest.mark.asyncio decorator. This async test function requires the decorator to be properly executed by pytest. Without it, the test will not run correctly.
| @pytest.mark.asyncio |
resolves #6639 (BA-2966)
Checklist: (if applicable)
ai.backend.testdocsdirectory