Improve exception handling and unit tests for auth, user, groups, expenses #79
Conversation
…expenses services
|
""" WalkthroughThis update introduces comprehensive and specific exception handling, error logging, and consistent HTTP error responses across the authentication, expenses, groups, and user service modules. All generic exception handling is replaced with targeted exception types and appropriate status codes. Corresponding unit tests are added or updated to verify error handling and input validation, including new tests for invalid ObjectId scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService
participant Database
participant Logger
participant Firebase
Client->>AuthService: create_user_with_email(email, password, name)
AuthService->>Database: Insert user
alt DuplicateKeyError
AuthService->>Logger: Log warning
AuthService-->>Client: HTTP 400 error (user exists)
else PyMongoError
AuthService->>Logger: Log error
AuthService-->>Client: HTTP 500 error
else Success
AuthService->>AuthService: _create_refresh_token_record(user_id)
alt Error in refresh token
AuthService->>Logger: Log error
AuthService-->>Client: HTTP 500 error
else
AuthService-->>Client: Return user info and tokens
end
end
sequenceDiagram
participant Client
participant ExpenseService
participant Database
participant Logger
Client->>ExpenseService: update_expense(expense_id, ...)
ExpenseService->>Database: Find expense by ID
alt InvalidId
ExpenseService->>Logger: Log warning
ExpenseService-->>Client: HTTP 400 error
else NotFound
ExpenseService->>Logger: Log warning
ExpenseService-->>Client: HTTP 404 error
else Unauthorized
ExpenseService->>Logger: Log warning
ExpenseService-->>Client: HTTP 403 error
else Success
ExpenseService->>Database: Update expense
ExpenseService-->>Client: Return updated expense
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for splitwizer failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
backend/tests/auth/test_auth_service.py (1)
651-661: Remove or relocate misplaced test.This test for
validate_object_idseems out of place in the auth service tests, and theservicevariable is unused. Consider removing this test since the function itself is barely used.-@pytest.mark.asyncio -async def test_create_refresh_token_record_invalid_user_id(): - service = AuthService() - invalid_user_id = "not-a-valid-objectid" - - with pytest.raises(HTTPException) as exc_info: - validate_object_id(invalid_user_id, field_name="user ID") - - assert exc_info.value.status_code == 400 - assert exc_info.value.detail == "Invalid user ID"
🧹 Nitpick comments (22)
backend/app/groups/service.py (1)
3-3: Good logging setup, but remove unused imports.The addition of specific exception handling imports and logging is excellent for improved error traceability.
However, please remove the unused imports flagged by static analysis:
-from typing import Optional, Dict, Any, List +from typing import Optional, ListAlso applies to: 8-8, 10-10
backend/app/user/service.py (1)
3-3: Good logging setup, remove unused imports.The logging setup and specific exception handling imports are well-implemented and consistent with the groups service.
Please remove the unused imports:
-from typing import Optional, Dict, Any +from typing import OptionalAlso applies to: 6-6, 8-8
backend/tests/expenses/test_expense_service.py (10)
107-125: Well-structured exception handling tests.The updated tests properly validate the different HTTPException scenarios:
- Case 1: Invalid ObjectId format → 400 status code
- Case 2: Valid ObjectId but unauthorized access → 403 status code
This aligns perfectly with the service layer changes shown in the relevant code snippets.
Consider removing the commented-out old test code to improve readability:
- '''# Test with invalid ObjectId format - with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_expense("invalid_group", expense_request, "user_a") - - # Test with valid ObjectId format but non-existent group - with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_expense("65f1a2b3c4d5e6f7a8b9c0d0", expense_request, "user_a")'''
322-338: Correct exception handling update.The test properly validates HTTPException with status code 403 for unauthorized expense updates, consistent with the service layer implementation.
Consider removing the commented-out old test code for cleaner code:
- '''with pytest.raises(ValueError, match="Expense not found or not authorized to edit"): - await expense_service.update_expense( - "group_id", - "65f1a2b3c4d5e6f7a8b9c0d1", - update_request, - "unauthorized_user" - )'''
451-461: Appropriate 404 error handling validation.The test correctly validates HTTPException with status code 404 for missing expenses, matching the service layer implementation.
Consider removing the commented-out old test code:
- ''' with pytest.raises(ValueError, match="Expense not found"): - await expense_service.get_expense_by_id("65f1a2b3c4d5e6f7a8b9c0d0", "65f1a2b3c4d5e6f7a8b9c0d1", "user_a")'''
678-688: Correct authorization error validation.The test properly validates HTTPException with status code 403 for unauthorized deletion attempts, with the exact error message matching the service implementation.
Consider removing the commented-out old test code for better readability:
- '''with pytest.raises(ValueError, match="Expense not found or not authorized to delete"): - await expense_service.delete_expense(group_id, expense_id, user_id) - - mock_db.settlements.delete_many.assert_not_called() - mock_db.expenses.delete_one.assert_not_called()'''
808-817: Proper group access validation.The test correctly validates HTTPException with status code 403 for group access denied scenarios, matching the service layer implementation exactly.
Consider removing the commented-out old test code:
- '''with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_manual_settlement(group_id, settlement_request, user_id) - - mock_db.settlements.insert_one.assert_not_called()'''
922-929: Consistent group access error handling.The test properly validates HTTPException with status code 403 for group access denied, maintaining consistency with other group access validation tests.
Consider removing the commented-out old test code for cleaner implementation:
- '''with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_group_settlements(group_id, user_id)'''
986-993: Appropriate 404 handling for missing settlements.The test correctly validates HTTPException with status code 404 for settlement not found scenarios, consistent with the service layer implementation.
Consider removing the commented-out old test code:
- '''with pytest.raises(ValueError, match="Settlement not found"): - await expense_service.get_settlement_by_id(group_id, settlement_id_str, user_id)'''
1008-1015: Consistent group access validation.The test properly validates HTTPException with status code 403 for group access denied, maintaining consistency across all group access tests.
Consider removing the commented-out old test code:
- '''with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_settlement_by_id(group_id, settlement_id_str, user_id)'''
1093-1104: Proper 404 handling for settlement updates.The test correctly validates HTTPException with status code 404 for settlement not found during status updates, consistent with the service implementation.
Consider removing the commented-out old test code:
- '''with pytest.raises(ValueError, match="Settlement not found"): - await expense_service.update_settlement_status( - group_id, settlement_id_str, new_status - )'''
1174-1181: Consistent exception handling across remaining tests.The remaining test updates properly implement HTTPException validation with appropriate status codes:
- Lines 1174-1181: 403 for delete settlement access denied
- Lines 1287-1296: 403 for user balance access denied
- Lines 1742-1749: 403 for group analytics access denied
All changes align perfectly with the service layer implementations.
Consider removing all commented-out old test code throughout these methods for cleaner code maintenance.
Also applies to: 1287-1296, 1742-1749
backend/tests/groups/test_groups_service.py (3)
370-376: Add assertion to verify no database calls are made.The test should verify that no database operations occur when an invalid ObjectId is provided, ensuring early validation prevents unnecessary DB interaction.
@pytest.mark.asyncio async def test_get_group_by_id_invalid_objectid(self): """Test get_group_by_id with invalid ObjectId format""" - with patch.object(self.service, 'get_db'): + mock_db = AsyncMock() + with patch.object(self.service, 'get_db', return_value=mock_db): result = await self.service.get_group_by_id("invalid-id", "user123") assert result is None + # Verify no DB calls were made + mock_db.groups.find_one.assert_not_called()
377-383: Add assertion to verify no database calls are made.Similar to the previous test, verify that invalid ObjectId prevents database operations.
@pytest.mark.asyncio async def test_update_group_invalid_objectid(self): """Test update_group with invalid ObjectId""" - with patch.object(self.service, 'get_db'): + mock_db = AsyncMock() + with patch.object(self.service, 'get_db', return_value=mock_db): result = await self.service.update_group("invalid-id", {"name": "test"}, "user123") assert result is None + # Verify no DB calls were made + mock_db.groups.find_one.assert_not_called()
384-390: Add assertion to verify no database calls are made.Ensure consistency across all invalid ObjectId tests by verifying no database operations occur.
@pytest.mark.asyncio async def test_delete_group_invalid_objectid(self): """Test delete_group with invalid ObjectId""" - with patch.object(self.service, 'get_db'): + mock_db = AsyncMock() + with patch.object(self.service, 'get_db', return_value=mock_db): result = await self.service.delete_group("invalid-id", "user123") assert result is False + # Verify no DB calls were made + mock_db.groups.find_one.assert_not_called() + mock_db.groups.delete_one.assert_not_called()backend/app/expenses/service.py (3)
42-47: Use exception chaining for better error traceability.When re-raising exceptions, use
fromto preserve the original exception context.except errors.InvalidId: #Incorrect ObjectId format logger.warning(f"Invalid group ID format: {group_id}") - raise HTTPException(status_code=400, detail="Invalid group ID") + raise HTTPException(status_code=400, detail="Invalid group ID") from None except Exception as e: logger.error(f"Unexpected error parsing groupId: {e}") - raise HTTPException(status_code=500, detail="Failed to process group ID") + raise HTTPException(status_code=500, detail="Failed to process group ID") from e
210-216: Use exception chaining for better error traceability.Apply consistent exception chaining pattern throughout the codebase.
except errors.InvalidId: #Incorrect ObjectId format for group_id or expense_id logger.warning(f"Invalid ObjectId(s): group_id={group_id}, expense_id={expense_id}") - raise HTTPException(status_code=400, detail="Invalid group ID or expense ID") + raise HTTPException(status_code=400, detail="Invalid group ID or expense ID") from None except Exception as e: logger.error(f"Unexpected error parsing IDs: {e}") - raise HTTPException(status_code=500, detail="Unable to process IDs") + raise HTTPException(status_code=500, detail="Unable to process IDs") from e
369-378: Remove redundant traceback printing.
logger.exception()already includes the full traceback, makingtraceback.print_exc()redundant.#Allowing FastAPI exception to bubble up for proper handling except HTTPException: raise except ValueError as ve: - raise HTTPException(status_code=400, detail=str(ve)) + raise HTTPException(status_code=400, detail=str(ve)) from ve except Exception as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback logger.exception(f"Unhandled error in update_expense for expense {expense_id}: {e}") - import traceback - traceback.print_exc() raise Exception(f"Database error during expense update: {str(e)}")backend/tests/auth/test_auth_service.py (1)
507-510: Combine nested with statements.Use a single with statement for cleaner code.
- with patch("app.auth.service.generate_reset_token", return_value="mocktoken"): - with caplog.at_level(logging.INFO): #caplog captures the log messages (previously print statements) - result = await service.request_password_reset("[email protected]") + with patch("app.auth.service.generate_reset_token", return_value="mocktoken"), \ + caplog.at_level(logging.INFO): #caplog captures the log messages (previously print statements) + result = await service.request_password_reset("[email protected]")backend/app/auth/service.py (3)
184-190: Good specific exception handling for Firebase token validation.The specific handling of
InvalidIdTokenErrorwith appropriate status code is excellent. Consider adding exception chaining.- raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid Google ID token" - ) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid Google ID token" + ) from None
366-374: Good security logging for JWT verification failures.The warning log for JWT verification failures is appropriate for security monitoring. Add exception chaining for better debugging.
- raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid token" - ) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid token" + ) from e
497-504: Add exception chaining to generic handler.The HTTPException re-raising is correct, but the generic exception handler should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error during password reset" - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error during password reset" + ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/auth/service.py(15 hunks)backend/app/expenses/service.py(16 hunks)backend/app/groups/service.py(6 hunks)backend/app/user/service.py(5 hunks)backend/tests/auth/test_auth_service.py(1 hunks)backend/tests/expenses/test_expense_service.py(13 hunks)backend/tests/groups/test_groups_service.py(1 hunks)backend/tests/user/test_user_service.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/app/expenses/service.py (2)
backend/app/expenses/schemas.py (1)
ExpenseUpdateRequest(37-55)backend/app/expenses/routes.py (2)
update_expense(73-89)debug_expense(375-418)
backend/tests/expenses/test_expense_service.py (2)
backend/app/expenses/service.py (11)
create_expense(36-93)update_expense(253-378)get_expense_by_id(203-251)delete_expense(380-398)create_manual_settlement(534-572)get_group_settlements(605-643)get_settlement_by_id(645-667)update_settlement_status(669-695)delete_settlement(697-713)get_user_balance_in_group(715-818)get_group_analytics(1012-1118)backend/app/expenses/routes.py (5)
create_expense(21-33)update_expense(73-89)delete_expense(92-106)get_group_settlements(180-225)delete_settlement(261-279)
backend/tests/groups/test_groups_service.py (3)
backend/app/groups/service.py (4)
get_group_by_id(150-178)update_group(180-205)delete_group(207-228)transform_group_document(84-103)backend/app/groups/routes.py (1)
delete_group(62-70)backend/tests/groups/test_groups_routes.py (2)
test_update_group_metadata(131-152)test_get_group_details(96-116)
backend/app/auth/service.py (4)
backend/app/auth/security.py (3)
verify_password(19-30)verify_token(76-91)get_password_hash(32-42)backend/app/auth/routes.py (3)
refresh_token(157-198)verify_token(201-221)login_with_email(90-122)backend/app/user/service.py (1)
get_db(14-15)backend/tests/auth/test_auth_routes.py (2)
test_signup_with_email_success(19-49)test_login_with_email_success(120-163)
🪛 Ruff (0.12.2)
backend/app/groups/service.py
5-5: typing.Dict imported but unused
Remove unused import
(F401)
5-5: typing.Any imported but unused
Remove unused import
(F401)
backend/app/user/service.py
5-5: typing.Dict imported but unused
Remove unused import
(F401)
5-5: typing.Any imported but unused
Remove unused import
(F401)
backend/app/expenses/service.py
1-1: fastapi.status imported but unused
Remove unused import: fastapi.status
(F401)
2-2: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
8-8: app.expenses.schemas.SplitType imported but unused
Remove unused import: app.expenses.schemas.SplitType
(F401)
10-10: asyncio imported but unused
Remove unused import: asyncio
(F401)
11-11: collections.deque imported but unused
Remove unused import: collections.deque
(F401)
44-44: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
212-212: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
215-215: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
262-262: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
373-373: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/tests/auth/test_auth_service.py
9-9: app.auth.security.get_password_hash imported but unused
Remove unused import
(F401)
9-9: app.auth.security.create_refresh_token imported but unused
Remove unused import
(F401)
9-9: app.auth.security.verify_password imported but unused
Remove unused import
(F401)
10-10: Redefinition of unused ObjectId from line 4
Remove definition: ObjectId
(F811)
19-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
507-508: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
558-559: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
653-653: Local variable service is assigned to but never used
Remove assignment to unused variable service
(F841)
backend/app/auth/service.py
123-123: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
125-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
144-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
161-164: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
187-190: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
213-216: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
252-255: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
262-265: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
303-306: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
319-322: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
334-337: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
371-374: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
388-391: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
413-413: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
434-434: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
501-504: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
533-536: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (21)
backend/app/groups/service.py (5)
53-64: Excellent specific exception handling with appropriate fallbacks.The addition of specific
InvalidIdexception handling with graceful fallback behavior is well-implemented. The logging levels are appropriate (warning for invalid IDs, error for unexpected exceptions), and the fallback maintains consistent data structure.Also applies to: 66-66
90-91: Good exception handling with appropriate logging.The enhanced exception handling for document transformation with warning-level logging is well-implemented and maintains consistent behavior.
155-160: Excellent layered exception handling with early returns.The specific handling of
InvalidIdexceptions with warning logging, followed by general exception handling with error logging, is well-structured. Early returns prevent unnecessary database operations on invalid inputs.
185-190: Consistent and robust exception handling pattern.The exception handling follows the same excellent pattern established in other methods: specific
InvalidIdhandling with warnings, general exception handling with error logging, and early returns to prevent invalid database operations.
212-217: Maintains consistent exception handling pattern.The exception handling implementation is consistent with the established pattern throughout the service, with appropriate return value (False) for the boolean method signature.
backend/app/user/service.py (4)
31-31: Excellent specific exception handling refinement.The enhancement from generic exception handling to specific
KeyErrorandTypeErrorcatching is well-reasoned - these are the expected failure modes for document format issues. The logging levels are appropriate (warning for datetime conversion, error for document format).Also applies to: 36-37
53-54: Consistent InvalidId exception handling.The specific
InvalidIdexception handling with warning logging and early return is consistent with the pattern established in the groups service and prevents unnecessary database operations.
63-64: Maintains consistent exception handling pattern.The
InvalidIdexception handling is consistent with the established pattern and appropriately returnsNonefor the method signature.
81-82: Consistent exception handling with appropriate return value.The
InvalidIdexception handling maintains the established pattern with appropriate boolean return value (False) for the delete operation.backend/tests/expenses/test_expense_service.py (1)
2-2: LGTM! Necessary import for updated exception handling.The addition of
HTTPExceptionimport is required for the test updates that replaceValueErrorassertions withHTTPExceptionassertions.backend/tests/groups/test_groups_service.py (1)
391-402: LGTM!Good test coverage for partial document transformation with proper verification of default values.
backend/app/expenses/service.py (1)
1-1121: Excellent improvement to exception handling!The refactoring from generic exceptions to specific HTTPException with appropriate status codes greatly improves API clarity and debugging. The consistent logging patterns throughout the service will help with monitoring and troubleshooting.
backend/tests/user/test_user_service.py (3)
162-172: LGTM!Excellent test implementation that properly verifies no database calls are made for invalid ObjectId inputs.
222-232: LGTM!Consistent test pattern that properly validates invalid ObjectId handling.
258-267: LGTM!Well-implemented test maintaining consistency across all user service operations.
backend/tests/auth/test_auth_service.py (2)
24-100: Excellent test coverage for user creation!Comprehensive test cases covering success path, duplicate users, token generation failures, and database errors.
1-661: Outstanding test coverage for AuthService!This comprehensive test suite excellently validates all authentication workflows including error handling paths. The tests align perfectly with the PR objectives of improving exception handling and achieving high test coverage. Once the minor code quality issues are addressed, this will be an exemplary test suite.
backend/app/auth/service.py (4)
3-17: Good addition of specific exception imports and logging setup.The imports for
PyMongoError,DuplicateKeyError,JWTError, and the logging module are appropriate for the enhanced exception handling. The logger initialization using__name__follows Python best practices.
271-272: Correct pattern for re-raising HTTPException.Good use of re-raising pattern to avoid double-logging HTTPExceptions.
340-348: Good design decision on error handling for token revocation.Excellent decision not to raise an exception when revoking the old token fails, since the new token is already safely issued. The comment clearly explains the reasoning.
461-495: Well-structured error handling for password reset flow.The comprehensive error handling with appropriate logging levels (warning for invalid tokens, info for success) is excellent. The transaction-like approach ensures all related operations succeed or fail together.
| except Exception as e: | ||
| logger.exception("Unexpected error while creating user with email") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Internal server error" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling with proper chaining and logging.
The generic exception handler should include the exception details in the log and use proper exception chaining.
- except Exception as e:
- logger.exception("Unexpected error while creating user with email")
- raise HTTPException(
- status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
- detail="Internal server error"
- )
+ except Exception as e:
+ logger.exception(f"Unexpected error while creating user with email: {str(e)}")
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail="Internal server error"
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.exception("Unexpected error while creating user with email") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Internal server error" | |
| ) | |
| except Exception as e: | |
| logger.exception(f"Unexpected error while creating user with email: {str(e)}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Internal server error" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
123-123: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
125-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 123 to 128, the generic exception
handler logs a message without including the exception details and does not use
proper exception chaining. Modify the logger.exception call to include the
exception details automatically by passing the exception object or using
logger.exception correctly, and raise the HTTPException using "from e" to
maintain the exception chain for better debugging.
| try: | ||
| user = await db.users.find_one({"email": email}) | ||
| except PyMongoError as e: | ||
| logger.error(f"Database error during user lookup: {e}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Internal server error" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining for proper error context.
Add proper exception chaining to maintain the error stack trace.
- raise HTTPException(
- status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
- detail="Internal server error"
- )
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail="Internal server error"
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| user = await db.users.find_one({"email": email}) | |
| except PyMongoError as e: | |
| logger.error(f"Database error during user lookup: {e}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Internal server error" | |
| ) | |
| try: | |
| user = await db.users.find_one({"email": email}) | |
| except PyMongoError as e: | |
| logger.error(f"Database error during user lookup: {e}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Internal server error" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
144-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 140 to 147, the except block catches
a PyMongoError and raises an HTTPException but does not use exception chaining,
which loses the original error context. Fix this by adding "from e" to the raise
statement to chain the original exception properly, preserving the error stack
trace.
| try: | ||
| refresh_token = await self._create_refresh_token_record(str(user["_id"])) | ||
| except Exception as e: | ||
| logger.error(f"Failed to generate refresh token: {e}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to generate refresh token" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining and ensure consistent error messaging.
The error handling is good, but should use exception chaining for better debugging.
- raise HTTPException(
- status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
- detail="Failed to generate refresh token"
- )
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail="Failed to generate refresh token"
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| refresh_token = await self._create_refresh_token_record(str(user["_id"])) | |
| except Exception as e: | |
| logger.error(f"Failed to generate refresh token: {e}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Failed to generate refresh token" | |
| ) | |
| try: | |
| refresh_token = await self._create_refresh_token_record(str(user["_id"])) | |
| except Exception as e: | |
| logger.error(f"Failed to generate refresh token: {e}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Failed to generate refresh token" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
161-164: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 157 to 164, the exception handling
for generating the refresh token should use exception chaining to preserve the
original traceback. Modify the raise statement to include "from e" after the
HTTPException to chain the original exception. Also, ensure the error message in
the HTTPException detail remains consistent with the logged error.
| try: | ||
| user = await db.users.find_one({"$or": [ | ||
| {"email": email}, | ||
| {"firebase_uid": firebase_uid} | ||
| ]}) | ||
| except PyMongoError as e: | ||
| logger.error("Database error while checking user: %s", str(e)) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Internal server error" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception chaining to maintain error context.
The error handling structure is good, but all these exception handlers should use raise ... from e for proper exception chaining.
For each of these exception blocks, add from e to the raise statement to maintain the full error context for debugging.
Also applies to: 246-255, 258-265
🧰 Tools
🪛 Ruff (0.12.2)
213-216: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 206 to 216, the exception handling
for PyMongoError catches the error and raises an HTTPException but does not use
exception chaining. To fix this, modify the raise statement to use 'raise
HTTPException(...) from e' to preserve the original exception context. Apply the
same change to the exception blocks at lines 246-255 and 258-265 as well.
| try: | ||
| token_record = await db.refresh_tokens.find_one({ | ||
| "token": refresh_token, | ||
| "revoked": False, | ||
| "expires_at": {"$gt": datetime.now(timezone.utc)} | ||
| }) | ||
| except PyMongoError as e: | ||
| logger.error("Database error while validating refresh token: %s", str(e)) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Internal server error" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception chaining for better error tracking.
All these exception handlers should use raise ... from e pattern for proper exception chaining.
Also applies to: 315-322, 330-337
🧰 Tools
🪛 Ruff (0.12.2)
303-306: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 295 to 306, the exception handler
for PyMongoError does not use exception chaining, which hinders error tracking.
Modify the raise statement to use "raise HTTPException(...) from e" to preserve
the original exception context. Apply the same pattern to the exception handlers
in lines 315-322 and 330-337 as well.
backend/app/auth/service.py
Outdated
| try: | ||
| user = await db.users.find_one({"email": email}) | ||
| except PyMongoError as e: | ||
| logger.error(f"Database error while fetching user by email {email}: {str(e)}") | ||
| raise HTTPException(status_code=500, detail="Internal server error during user lookup.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception chaining for database errors.
Both exception handlers should use raise ... from e for proper exception chaining.
Also applies to: 423-434
🧰 Tools
🪛 Ruff (0.12.2)
413-413: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 409 to 413 and also lines 423 to
434, the exception handlers for database errors currently raise HTTPException
without chaining the original exception. Modify the raise statements to use
"raise HTTPException(...) from e" to preserve the original exception context for
better debugging and error tracing.
| try: | ||
| await db.refresh_tokens.insert_one({ | ||
| "token": refresh_token, | ||
| "user_id": ObjectId(user_id) if isinstance(user_id, str) else user_id, | ||
| "expires_at": expires_at, | ||
| "revoked": False, | ||
| "created_at": datetime.now(timezone.utc) | ||
| }) | ||
| except Exception as e: | ||
| logger.error(f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Failed to create refresh token: {str(e)}" | ||
| ) |
There was a problem hiding this comment.
Remove internal error details from client response.
Including the raw error message in the HTTPException detail could expose internal implementation details. Consider using a generic message for the client while keeping detailed logs.
- raise HTTPException(
- status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
- detail=f"Failed to create refresh token: {str(e)}"
- )
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail="Failed to create refresh token"
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| await db.refresh_tokens.insert_one({ | |
| "token": refresh_token, | |
| "user_id": ObjectId(user_id) if isinstance(user_id, str) else user_id, | |
| "expires_at": expires_at, | |
| "revoked": False, | |
| "created_at": datetime.now(timezone.utc) | |
| }) | |
| except Exception as e: | |
| logger.error(f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail=f"Failed to create refresh token: {str(e)}" | |
| ) | |
| try: | |
| await db.refresh_tokens.insert_one({ | |
| "token": refresh_token, | |
| "user_id": ObjectId(user_id) if isinstance(user_id, str) else user_id, | |
| "expires_at": expires_at, | |
| "revoked": False, | |
| "created_at": datetime.now(timezone.utc) | |
| }) | |
| except Exception as e: | |
| logger.error(f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Failed to create refresh token" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
533-536: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 523 to 536, the HTTPException detail
currently includes raw error messages which may expose internal implementation
details to clients. Modify the exception handling to log the detailed error
internally using logger.error, but replace the HTTPException detail message with
a generic, non-sensitive message like "Failed to create refresh token" before
raising it.
| import pytest | ||
| from unittest.mock import AsyncMock, patch, MagicMock | ||
| from fastapi import HTTPException, status | ||
| from bson import ObjectId | ||
| from jose import JWTError | ||
| from firebase_admin import auth as firebase_auth | ||
| from datetime import datetime, timedelta, timezone | ||
| from app.auth.service import AuthService | ||
| from app.auth.security import get_password_hash, create_refresh_token, verify_password | ||
| from bson import ObjectId | ||
| from bson.errors import InvalidId | ||
| from pymongo.errors import PyMongoError | ||
| import logging | ||
|
|
||
| def validate_object_id(id_str: str, field_name: str = "ID") -> ObjectId: | ||
| try: | ||
| return ObjectId(id_str) | ||
| except InvalidId: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=f"Invalid {field_name}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Clean up imports and remove unnecessary helper function.
Remove unused imports and the barely-used validate_object_id function to improve code clarity.
import pytest
from unittest.mock import AsyncMock, patch, MagicMock
from fastapi import HTTPException, status
from bson import ObjectId
from jose import JWTError
from firebase_admin import auth as firebase_auth
from datetime import datetime, timedelta, timezone
from app.auth.service import AuthService
-from app.auth.security import get_password_hash, create_refresh_token, verify_password
-from bson import ObjectId
from bson.errors import InvalidId
from pymongo.errors import PyMongoError
import logging
-def validate_object_id(id_str: str, field_name: str = "ID") -> ObjectId:
- try:
- return ObjectId(id_str)
- except InvalidId:
- raise HTTPException(
- status_code=status.HTTP_400_BAD_REQUEST,
- detail=f"Invalid {field_name}"
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import pytest | |
| from unittest.mock import AsyncMock, patch, MagicMock | |
| from fastapi import HTTPException, status | |
| from bson import ObjectId | |
| from jose import JWTError | |
| from firebase_admin import auth as firebase_auth | |
| from datetime import datetime, timedelta, timezone | |
| from app.auth.service import AuthService | |
| from app.auth.security import get_password_hash, create_refresh_token, verify_password | |
| from bson import ObjectId | |
| from bson.errors import InvalidId | |
| from pymongo.errors import PyMongoError | |
| import logging | |
| def validate_object_id(id_str: str, field_name: str = "ID") -> ObjectId: | |
| try: | |
| return ObjectId(id_str) | |
| except InvalidId: | |
| raise HTTPException( | |
| status_code=status.HTTP_400_BAD_REQUEST, | |
| detail=f"Invalid {field_name}" | |
| ) | |
| import pytest | |
| from unittest.mock import AsyncMock, patch, MagicMock | |
| from fastapi import HTTPException, status | |
| from bson import ObjectId | |
| from jose import JWTError | |
| from firebase_admin import auth as firebase_auth | |
| from datetime import datetime, timedelta, timezone | |
| from app.auth.service import AuthService | |
| from bson.errors import InvalidId | |
| from pymongo.errors import PyMongoError | |
| import logging |
🧰 Tools
🪛 Ruff (0.12.2)
9-9: app.auth.security.get_password_hash imported but unused
Remove unused import
(F401)
9-9: app.auth.security.create_refresh_token imported but unused
Remove unused import
(F401)
9-9: app.auth.security.verify_password imported but unused
Remove unused import
(F401)
10-10: Redefinition of unused ObjectId from line 4
Remove definition: ObjectId
(F811)
19-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/tests/auth/test_auth_service.py lines 1 to 23, remove all unused
imports such as logging and duplicate imports like ObjectId, and delete the
validate_object_id function entirely since it is barely used, to clean up the
code and improve clarity.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
backend/app/groups/service.py (1)
52-76: Eliminate code duplication in exception handlers.The fallback member creation logic is duplicated between the
InvalidIdand generic exception handlers. Consider extracting this into a helper method or reusing the same logic.except errors.InvalidId: #exception for invalid ObjectId logger.warning(f"Invalid ObjectId for userId: {member_user_id}") - enriched_members.append({ - "userId": member_user_id, - "role": member.get("role", "member"), - "joinedAt": member.get("joinedAt"), - "user": { - "name": f"User {member_user_id[-4:]}", - "email": f"{member_user_id}@example.com", - "avatar": None - } - }) except Exception as e: logger.error(f"Error enriching userId {member_user_id}: {e}") - # If user lookup fails, add member with basic info - enriched_members.append({ - "userId": member_user_id, - "role": member.get("role", "member"), - "joinedAt": member.get("joinedAt"), - "user": { - "name": f"User {member_user_id[-4:]}", - "email": f"{member_user_id}@example.com", - "avatar": None - } - }) + + # For both InvalidId and other exceptions, add member with basic info + enriched_members.append({ + "userId": member_user_id, + "role": member.get("role", "member"), + "joinedAt": member.get("joinedAt"), + "user": { + "name": f"User {member_user_id[-4:]}", + "email": f"{member_user_id}@example.com", + "avatar": None + } + })
♻️ Duplicate comments (3)
backend/app/expenses/service.py (1)
1-12: Remove unused imports.Several imports are not used in the code and should be removed to maintain clean code.
backend/app/auth/service.py (2)
427-427: Use timezone-aware datetime for consistency.Replace
datetime.utcnow()withdatetime.now(timezone.utc)to maintain consistency with the rest of the codebase."expires_at": reset_expires, "used": False, - "created_at": datetime.utcnow() + "created_at": datetime.now(timezone.utc) })
529-533: Avoid exposing internal error details to clients.The raw error message in the HTTPException detail could expose internal implementation details. Use a generic message for clients while keeping detailed logs.
except Exception as e: logger.error(f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=f"Failed to create refresh token: {str(e)}" + detail="Failed to create refresh token" )
🧹 Nitpick comments (2)
backend/app/groups/service.py (1)
260-263: Apply consistent error handling pattern.For consistency with other methods in this file, consider using the same error handling pattern with specific
errors.InvalidIdhandling and logging.try: obj_id = ObjectId(group_id) - except Exception: + except errors.InvalidId: + logger.warning(f"Invalid group_id: {group_id}") + return False + except Exception as e: + logger.error(f"Unexpected error converting group_id to ObjectId: {e}") return Falsebackend/app/expenses/service.py (1)
40-45: Use exception chaining for better error context.Add
from Noneorfrom eto maintain proper exception context.except errors.InvalidId: #Incorrect ObjectId format logger.warning(f"Invalid group ID format: {group_id}") - raise HTTPException(status_code=400, detail="Invalid group ID") + raise HTTPException(status_code=400, detail="Invalid group ID") from None except Exception as e: logger.error(f"Unexpected error parsing groupId: {e}") - raise HTTPException(status_code=500, detail="Failed to process group ID") + raise HTTPException(status_code=500, detail="Failed to process group ID") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/auth/service.py(14 hunks)backend/app/expenses/service.py(16 hunks)backend/app/groups/service.py(6 hunks)backend/app/user/service.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/user/service.py
🧰 Additional context used
📓 Path-based instructions (4)
backend/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend code must be implemented using FastAPI with Python 3.9+ and organized under the /backend/ directory.
Files:
backend/app/expenses/service.pybackend/app/auth/service.pybackend/app/groups/service.py
backend/app/{auth,user,groups,expenses}/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/{auth,user,groups,expenses}/**/*.py: Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
When adding a new API endpoint, add the route to the appropriate service router file in the backend.
Files:
backend/app/expenses/service.pybackend/app/auth/service.pybackend/app/groups/service.py
backend/app/expenses/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/expenses/**/*.py: Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Settlements must track debt resolution between users in the expense tracking logic.
Files:
backend/app/expenses/service.py
backend/app/auth/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
JWT authentication with refresh token rotation must be implemented in the backend.
Files:
backend/app/auth/service.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.290Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
backend/app/expenses/service.py (4)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.291Z
Learning: Applies to backend/app/expenses/**/*.py : Settlements must track debt resolution between users in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.291Z
Learning: Applies to backend/app/expenses/**/*.py : Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.290Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.290Z
Learning: Applies to backend/app/database.py : MongoDB must be used for persistent data storage in the backend, with connection logic in backend/app/database.py.
backend/app/auth/service.py (4)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.291Z
Learning: Applies to backend/app/auth/security.py : To troubleshoot authentication issues, check JWT token handling in app/auth/security.py.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.290Z
Learning: Applies to backend/app/auth/**/*.py : JWT authentication with refresh token rotation must be implemented in the backend.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.290Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.290Z
Learning: Applies to backend/app/database.py : MongoDB must be used for persistent data storage in the backend, with connection logic in backend/app/database.py.
🧬 Code Graph Analysis (1)
backend/app/groups/service.py (1)
backend/tests/groups/test_groups_service.py (1)
test_transform_group_document(22-40)
🪛 Ruff (0.12.2)
backend/app/expenses/service.py
1-1: fastapi.status imported but unused
Remove unused import: fastapi.status
(F401)
2-2: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
42-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
45-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
210-210: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
213-213: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
260-260: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
357-357: f-string without any placeholders
Remove extraneous f prefix
(F541)
371-371: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/auth/service.py
120-120: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
122-125: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
141-144: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
158-161: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
184-187: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
210-213: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-252: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
259-262: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
300-303: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
316-319: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
331-334: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
368-371: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
385-388: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
410-410: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
431-431: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
498-501: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
530-533: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/groups/service.py
6-6: typing.Dict imported but unused
Remove unused import
(F401)
6-6: typing.Any imported but unused
Remove unused import
(F401)
| except Exception: | ||
| logger.error(f"Warning: Failed to recalculate settlements",exc_info=True) | ||
| # Continue anyway, as the expense update succeeded |
There was a problem hiding this comment.
Fix f-string without placeholders and remove redundant logging.
The f-string on line 357 has no placeholders, and logger.error should be logger.warning to match the comment.
except Exception:
- logger.error(f"Warning: Failed to recalculate settlements",exc_info=True)
+ logger.warning("Failed to recalculate settlements", exc_info=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception: | |
| logger.error(f"Warning: Failed to recalculate settlements",exc_info=True) | |
| # Continue anyway, as the expense update succeeded | |
| except Exception: | |
| logger.warning("Failed to recalculate settlements", exc_info=True) | |
| # Continue anyway, as the expense update succeeded |
🧰 Tools
🪛 Ruff (0.12.2)
357-357: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 356 to 358, replace the f-string
in the logger.error call with a plain string since there are no placeholders,
and change the logging level from error to warning to align with the comment
indicating it is a warning. Remove the redundant f-string formatting and update
the method to logger.warning.
backend/app/expenses/service.py
Outdated
| except Exception as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback | ||
| logger.exception(f"Unhandled error in update_expense for expense {expense_id}: {e}") | ||
| import traceback | ||
| traceback.print_exc() | ||
| raise Exception(f"Database error during expense update: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant traceback printing.
The logger.exception() already includes the full traceback, making the explicit traceback.print_exc() redundant.
except Exception as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback
logger.exception(f"Unhandled error in update_expense for expense {expense_id}: {e}")
- import traceback
- traceback.print_exc()
raise Exception(f"Database error during expense update: {str(e)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback | |
| logger.exception(f"Unhandled error in update_expense for expense {expense_id}: {e}") | |
| import traceback | |
| traceback.print_exc() | |
| raise Exception(f"Database error during expense update: {str(e)}") | |
| except Exception as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback | |
| logger.exception(f"Unhandled error in update_expense for expense {expense_id}: {e}") | |
| raise Exception(f"Database error during expense update: {str(e)}") |
🧰 Tools
🪛 Ruff (0.12.2)
376-376: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 372 to 376, remove the explicit
call to traceback.print_exc() since logger.exception() already logs the full
traceback. Keep the logger.exception() call and the raise statement, but delete
the import traceback and traceback.print_exc() lines to avoid redundant
traceback output.
backend/app/groups/service.py
Outdated
| from app.config import logger | ||
| from bson import ObjectId, errors | ||
| from datetime import datetime, timezone | ||
| from typing import Optional, Dict, Any, List |
There was a problem hiding this comment.
Remove unused imports Dict and Any.
These types are imported but not used in the code.
-from typing import Optional, Dict, Any, List
+from typing import Optional, List📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import Optional, Dict, Any, List | |
| -from typing import Optional, Dict, Any, List | |
| +from typing import Optional, List |
🧰 Tools
🪛 Ruff (0.12.2)
6-6: typing.Dict imported but unused
Remove unused import
(F401)
6-6: typing.Any imported but unused
Remove unused import
(F401)
🤖 Prompt for AI Agents
In backend/app/groups/service.py at line 6, the imports Dict and Any are not
used anywhere in the file. Remove Dict and Any from the import statement to
clean up unused imports, leaving only Optional and List.
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
backend/app/auth/service.py (1)
281-288: Fix outer exception handler pattern.The outer exception handler should re-raise HTTPExceptions and handle unexpected errors properly.
except HTTPException: raise except Exception as e: logger.exception("Unexpected error during Google authentication") - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=f"Google authentication failed: {str(e)}", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Google authentication failed", + ) from eNote: Changed to HTTP 500 since unexpected errors are typically server errors, not client errors.
♻️ Duplicate comments (21)
backend/app/groups/service.py (1)
4-4: Address unused imports from past review.The past review comment indicated that
DictandAnyfrom typing are unused imports that should be removed.Based on the past review, remove the unused imports:
-from typing import Any, Dict, List, Optional +from typing import List, Optionalbackend/app/expenses/service.py (3)
17-17: Remove unused importstatus.The
statusmodule from fastapi is imported but not used in the code.-from fastapi import HTTPException, status +from fastapi import HTTPException
414-414: Fix f-string without placeholders.The f-string has no placeholders and should be a regular string.
- logger.error(f"Warning: Failed to recalculate settlements", exc_info=True) + logger.warning("Failed to recalculate settlements", exc_info=True)
437-443: Remove redundant traceback printing.The
logger.exception()already includes the full traceback, making the explicittraceback.print_exc()redundant.logger.exception( f"Unhandled error in update_expense for expense {expense_id}: {e}" ) - import traceback - - traceback.print_exc() raise Exception(f"Database error during expense update: {str(e)}")backend/tests/auth/test_auth_service.py (1)
7-7: Clean up imports and remove unnecessary helper function.Remove unused imports and the barely-used
validate_object_idfunction to improve code clarity.-from app.auth.security import create_refresh_token, get_password_hash, verify_password from app.auth.service import AuthServiceAlso remove the
validate_object_idfunction (lines 17-24) as it's only used once and adds unnecessary complexity.Also applies to: 17-24
backend/app/auth/service.py (16)
131-136: Add exception chaining and fix logger import.This exception handler lacks proper chaining and the logger is not imported.
except Exception as e: - logger.exception("Unexpected error while creating user with email") + logger.exception(f"Unexpected error while creating user with email: {str(e)}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error", - ) + ) from e
150-157: Add exception chaining for database error handling.Good use of specific
PyMongoErrorexception type, but missing exception chaining.except PyMongoError as e: logger.error(f"Database error during user lookup: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error", - ) + ) from e
167-174: Add exception chaining for refresh token generation.The exception handling structure is appropriate but needs proper chaining.
except Exception as e: logger.error(f"Failed to generate refresh token: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed to generate refresh token", - ) + ) from e
191-197: Add exception chaining for Firebase token verification.Good approach to catch Firebase-specific exceptions early, but needs proper chaining.
except firebase_auth.InvalidIdTokenError: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid Google ID token", - ) + ) from NoneNote: Using
from Nonehere since we want to suppress the original Firebase exception details for security.
214-223: Add exception chaining for user lookup.Proper use of
PyMongoErrorbut missing exception chaining.except PyMongoError as e: logger.error("Database error while checking user: %s", str(e)) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error", - ) + ) from e
253-263: Add exception chaining for user creation.except PyMongoError as e: logger.error( "Failed to create new Google user: %s", str(e)) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed to create user", - ) + ) from e
266-278: Add exception chaining for refresh token creation.except Exception as e: logger.error( "Failed to issue refresh token for Google login: %s", str( e) ) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed to generate refresh token", - ) + ) from e
305-319: Add exception chaining for refresh token validation.except PyMongoError as e: logger.error( "Database error while validating refresh token: %s", str(e)) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error", - ) + ) from e
328-335: Add exception chaining for user lookup.except PyMongoError as e: logger.error("Error while fetching user: %s", str(e)) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error", - ) + ) from e
342-351: Add exception chaining for new token creation.except Exception as e: logger.error("Failed to create new refresh token: %s", str(e)) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed to create refresh token", - ) + ) from e
379-386: Add exception chaining for JWT verification.Good use of specific
JWTErrorexception type.except JWTError as e: logger.warning("JWT verification failed: %s", str(e)) raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid token" - ) + ) from e
395-402: Use specific exception type and add chaining.Use
PyMongoErrorinstead of genericExceptionfor database operations.-except Exception as e: +except PyMongoError as e: logger.error("Error while verifying token: %s", str(e)) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error", - ) + ) from e
419-427: Add exception chaining for user lookup.except PyMongoError as e: logger.error( f"Database error while fetching user by email {email}: {str(e)}" ) raise HTTPException( status_code=500, detail="Internal server error during user lookup." - ) + ) from e
438-455: Add exception chaining and fix datetime usage.except PyMongoError as e: logger.error( f"Database error while storing reset token for user {email}: {str(e)}" ) raise HTTPException( status_code=500, detail="Internal server error during token storage." - ) + ) from eAlso, fix the datetime usage for consistency:
- "created_at": datetime.utcnow(), + "created_at": datetime.now(timezone.utc),
484-531: Add exception chaining and consider breaking down the try block.The exception handling structure is good, but needs proper chaining and could benefit from more granular error handling.
except HTTPException: raise # Raising HTTPException to avoid logging again except Exception as e: logger.exception( f"Unexpected error during password reset: {str(e)}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error during password reset", - ) + ) from eConsider breaking down the large try block into smaller, more specific exception handling blocks for each database operation (token lookup, password update, token marking, refresh token revocation) to provide more precise error messages.
552-572: Fix exception chaining and remove internal error details.The HTTPException detail includes raw error messages which could expose internal implementation details.
+except PyMongoError as e: + logger.error( + f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}" + ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to create refresh token", + ) from e except Exception as e: logger.error( f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}" ) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=f"Failed to create refresh token: {str(e)}", - ) + detail="Failed to create refresh token", + ) from e
🧹 Nitpick comments (6)
backend/tests/expenses/test_expense_service.py (5)
122-142: Remove commented code and excellent exception handling improvements.The transition from
ValueErrortoHTTPExceptionwith specific status codes (400 for invalid ObjectId, 403 for authorization issues) is excellent and aligns perfectly with the PR objectives.Remove the commented code blocks to improve readability:
- '''# Test with invalid ObjectId format - with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_expense( - "invalid_group", expense_request, "user_a" - ) - - # Test with valid ObjectId format but non-existent group - with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_expense("65f1a2b3c4d5e6f7a8b9c0d0", expense_request, "user_a")'''
488-498: Remove malformed comments and excellent 404 exception handling.The new HTTPException test with 404 status code is correctly implemented for the "expense not found" scenario.
Remove the malformed commented code:
- ''' with pytest.raises(ValueError, match="Expense not found"): - await expense_service.get_expense_by_id("65f1a2b3c4d5e6f7a8b9c0d0", "65f1a2b3c4d5e6f7a8b9c0d1", "user_a")'''The HTTPException implementation correctly validates the 404 status code and error message.
923-933: Remove malformed comments and approve group access validation.The HTTPException test correctly validates 403 status code for group membership failures.
Remove the malformed commented code:
- '''with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_manual_settlement(group_id, settlement_request, user_id) - - mock_db.settlements.insert_one.assert_not_called()'''The HTTPException implementation properly validates group access control.
1075-1082: Fix syntax errors and approve consistent exception handling.The HTTPException implementation maintains consistency across the test suite with proper 403 status codes.
Remove the malformed commented code:
- '''with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_group_settlements(group_id, user_id)'''The new exception handling correctly validates group membership for settlement retrieval.
1364-1371: Fix syntax errors and approve consistent authorization handling.The HTTPException implementation maintains consistency with 403 status codes for authorization failures.
Remove the malformed commented code:
- '''with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.delete_settlement(group_id, settlement_id_str, user_id)'''backend/tests/auth/test_auth_service.py (1)
584-587: Combine nested with statements for cleaner code.Use a single with statement with multiple contexts instead of nested with statements.
- with patch("app.auth.service.generate_reset_token", return_value="mocktoken"): - with caplog.at_level( - logging.INFO - ): + with patch("app.auth.service.generate_reset_token", return_value="mocktoken"), \ + caplog.at_level(logging.INFO):Also applies to: 639-640
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/auth/service.py(14 hunks)backend/app/expenses/service.py(16 hunks)backend/app/groups/service.py(6 hunks)backend/app/user/service.py(5 hunks)backend/tests/auth/test_auth_service.py(1 hunks)backend/tests/expenses/test_expense_service.py(13 hunks)backend/tests/groups/test_groups_service.py(1 hunks)backend/tests/user/test_user_service.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/user/service.py
- backend/tests/groups/test_groups_service.py
🧰 Additional context used
📓 Path-based instructions (5)
backend/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend code must be implemented using FastAPI with Python 3.9+ and organized under the /backend/ directory.
Files:
backend/app/expenses/service.pybackend/tests/auth/test_auth_service.pybackend/app/auth/service.pybackend/app/groups/service.pybackend/tests/expenses/test_expense_service.pybackend/tests/user/test_user_service.py
backend/app/{auth,user,groups,expenses}/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/{auth,user,groups,expenses}/**/*.py: Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
When adding a new API endpoint, add the route to the appropriate service router file in the backend.
Files:
backend/app/expenses/service.pybackend/app/auth/service.pybackend/app/groups/service.py
backend/app/expenses/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/expenses/**/*.py: Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Settlements must track debt resolution between users in the expense tracking logic.
Files:
backend/app/expenses/service.py
backend/tests/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend tests must be placed in the /backend/tests/ directory and run using pytest.
Files:
backend/tests/auth/test_auth_service.pybackend/tests/expenses/test_expense_service.pybackend/tests/user/test_user_service.py
backend/app/auth/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
JWT authentication with refresh token rotation must be implemented in the backend.
Files:
backend/app/auth/service.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
backend/app/expenses/service.py (4)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Settlements must track debt resolution between users in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : When adding a new API endpoint, add the route to the appropriate service router file in the backend.
backend/tests/auth/test_auth_service.py (1)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/auth/security.py : To troubleshoot authentication issues, check JWT token handling in app/auth/security.py.
backend/app/auth/service.py (4)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/auth/security.py : To troubleshoot authentication issues, check JWT token handling in app/auth/security.py.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/auth/**/*.py : JWT authentication with refresh token rotation must be implemented in the backend.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/database.py : MongoDB must be used for persistent data storage in the backend, with connection logic in backend/app/database.py.
backend/tests/expenses/test_expense_service.py (2)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Settlements must track debt resolution between users in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
🧬 Code Graph Analysis (4)
backend/tests/auth/test_auth_service.py (3)
backend/app/auth/security.py (3)
create_refresh_token(80-87)get_password_hash(37-47)verify_password(23-34)backend/app/auth/service.py (10)
AuthService(64-574)create_user_with_email(75-136)authenticate_user_with_email(138-175)authenticate_with_google(177-288)get_db(69-73)_create_refresh_token_record(533-574)refresh_access_token(290-362)verify_access_token(364-409)request_password_reset(411-464)confirm_password_reset(466-531)backend/tests/conftest.py (1)
mock_db(65-100)
backend/app/auth/service.py (4)
backend/app/auth/security.py (3)
verify_password(23-34)verify_token(90-107)get_password_hash(37-47)backend/app/auth/routes.py (3)
refresh_token(176-217)verify_token(221-240)login_with_email(90-122)backend/app/user/service.py (1)
get_db(13-14)backend/tests/auth/test_auth_routes.py (2)
test_signup_with_email_success(19-49)test_login_with_email_success(120-163)
backend/app/groups/service.py (1)
backend/app/database.py (1)
get_database(36-42)
backend/tests/user/test_user_service.py (1)
backend/app/user/service.py (3)
get_user_by_id(54-63)update_user_profile(65-81)delete_user(83-93)
🪛 Ruff (0.12.2)
backend/app/expenses/service.py
17-17: fastapi.status imported but unused
Remove unused import: fastapi.status
(F401)
48-48: Undefined name errors
(F821)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
53-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
240-240: Undefined name errors
(F821)
244-246: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-250: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
294-294: Undefined name errors
(F821)
296-297: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
414-414: f-string without any placeholders
Remove extraneous f prefix
(F541)
433-433: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/tests/auth/test_auth_service.py
7-7: app.auth.security.create_refresh_token imported but unused
Remove unused import
(F401)
7-7: app.auth.security.get_password_hash imported but unused
Remove unused import
(F401)
7-7: app.auth.security.verify_password imported but unused
Remove unused import
(F401)
21-23: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
584-587: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
639-640: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
746-746: Local variable service is assigned to but never used
Remove assignment to unused variable service
(F841)
backend/app/auth/service.py
131-131: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
132-132: Undefined name logger
(F821)
133-136: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
153-153: Undefined name logger
(F821)
154-157: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
159-159: Undefined name verify_password
(F821)
160-160: Undefined name logger
(F821)
170-170: Undefined name logger
(F821)
171-174: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
194-197: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
219-219: Undefined name logger
(F821)
220-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
239-239: Undefined name logger
(F821)
258-258: Undefined name logger
(F821)
260-263: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
271-271: Undefined name logger
(F821)
275-278: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
284-284: Undefined name logger
(F821)
314-314: Undefined name logger
(F821)
316-319: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
331-331: Undefined name logger
(F821)
332-335: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
347-347: Undefined name logger
(F821)
348-351: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
359-359: Undefined name logger
(F821)
383-383: Undefined name logger
(F821)
384-386: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
398-398: Undefined name logger
(F821)
399-402: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
422-422: Undefined name logger
(F821)
425-427: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
450-450: Undefined name logger
(F821)
453-455: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
495-495: Undefined name logger
(F821)
502-502: Undefined name get_password_hash
(F821)
518-518: Undefined name logger
(F821)
526-526: Undefined name logger
(F821)
528-531: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
566-566: Undefined name logger
(F821)
569-572: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/groups/service.py
103-103: try-except block with duplicate exception Exception
(B025)
backend/tests/expenses/test_expense_service.py
488-488: SyntaxError: Compound statements are not allowed on the same line as simple statements
489-489: SyntaxError: Simple statements must be separated by newlines or semicolons
782-782: SyntaxError: Compound statements are not allowed on the same line as simple statements
783-783: SyntaxError: Expected an indented block after with statement
786-786: SyntaxError: Simple statements must be separated by newlines or semicolons
923-923: SyntaxError: Compound statements are not allowed on the same line as simple statements
926-926: SyntaxError: Simple statements must be separated by newlines or semicolons
1075-1075: SyntaxError: Compound statements are not allowed on the same line as simple statements
1076-1076: SyntaxError: Simple statements must be separated by newlines or semicolons
1150-1150: SyntaxError: Compound statements are not allowed on the same line as simple statements
1151-1151: SyntaxError: Expected an indented block after with statement
1151-1151: SyntaxError: Simple statements must be separated by newlines or semicolons
1174-1174: SyntaxError: Compound statements are not allowed on the same line as simple statements
1175-1175: SyntaxError: Expected an indented block after with statement
1175-1175: SyntaxError: Simple statements must be separated by newlines or semicolons
1275-1275: SyntaxError: Compound statements are not allowed on the same line as simple statements
1276-1276: SyntaxError: Expected an indented block after with statement
1278-1278: SyntaxError: Simple statements must be separated by newlines or semicolons
1364-1364: SyntaxError: Compound statements are not allowed on the same line as simple statements
1365-1365: SyntaxError: Expected an indented block after with statement
1365-1365: SyntaxError: Simple statements must be separated by newlines or semicolons
1505-1505: SyntaxError: Compound statements are not allowed on the same line as simple statements
1506-1506: SyntaxError: Expected an indented block after with statement
1506-1506: SyntaxError: Simple statements must be separated by newlines or semicolons
2073-2073: SyntaxError: Compound statements are not allowed on the same line as simple statements
2074-2074: SyntaxError: Expected an indented block after with statement
2074-2074: SyntaxError: missing closing quote in string literal
backend/tests/user/test_user_service.py
311-311: Redefinition of unused test_delete_user_invalid_object_id from line 299
(F811)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - splitwizer
- GitHub Check: Header rules - splitwizer
- GitHub Check: Pages changed - splitwizer
🔇 Additional comments (16)
backend/app/groups/service.py (7)
6-6: LGTM: Good addition of logging and specific exception handling imports.The imports for
loggerandbson.errorsalign with the PR objectives to improve exception handling and logging across backend services.Also applies to: 8-8
72-86: LGTM: Proper handling of invalid ObjectId with appropriate fallback.The specific handling of
errors.InvalidIdwith warning logging and graceful fallback to default user data is well-implemented and aligns with the PR objectives.
131-133: LGTM: Appropriate error handling for document transformation.The generic exception handling with warning logging is suitable here since this method deals with document structure validation rather than specific ObjectId conversion.
194-200: LGTM: Consistent ObjectId validation pattern.The specific handling of
errors.InvalidIdwith warning logging and graceful return aligns with the established pattern and PR objectives for improved exception handling.
226-232: LGTM: Consistent ObjectId validation pattern.The exception handling follows the same pattern as other methods, providing specific handling for invalid ObjectId with appropriate logging.
256-262: LGTM: Consistent ObjectId validation pattern.The exception handling is consistent with other methods in the class, returning
Falseappropriately for delete operations when ObjectId is invalid.
87-102: Critical issue: Duplicate exception handling blocks.There are two consecutive
except Exception as e:blocks handling the same exception type, which creates unreachable code and is flagged by static analysis.Apply this diff to remove the duplicate exception handler:
- except Exception as e: - logger.error( - f"Error enriching userId {member_user_id}: {e}") - # If user lookup fails, add member with basic info - enriched_members.append( - { - "userId": member_user_id, - "role": member.get("role", "member"), - "joinedAt": member.get("joinedAt"), - "user": { - "name": f"User {member_user_id[-4:]}", - "email": f"{member_user_id}@example.com", - "avatar": None, - }, - } - ) - except Exception as e: - logger.error( - f"Error enriching userId {member_user_id}: {e}") + except Exception as e: + logger.error( + f"Error enriching userId {member_user_id}: {e}") # If user lookup fails, add member with basic info enriched_members.append( {Likely an incorrect or invalid review comment.
backend/tests/user/test_user_service.py (3)
183-193: LGTM: Well-structured test for invalid ObjectId handling.The test correctly verifies that invalid ObjectId inputs result in no database calls and return
None, which aligns with the service implementation changes.
255-266: LGTM: Comprehensive test for invalid ObjectId in update operation.The test properly validates that invalid ObjectId formats are handled gracefully without making database calls and return
Noneas expected.
297-307: LGTM: Good test coverage for delete operation with invalid ObjectId.The test correctly verifies the expected behavior when an invalid ObjectId is provided to the delete operation.
backend/tests/expenses/test_expense_service.py (1)
7-7: LGTM! Import addition supports exception handling improvements.The addition of
HTTPExceptionimport aligns with the PR objective to replace generic exception handling with more specific HTTP exceptions.backend/app/expenses/service.py (1)
47-54: Excellent error handling improvements!The comprehensive replacement of generic exceptions with specific HTTPException instances using appropriate status codes (400 for bad requests, 403 for authorization, 404 for not found, 500 for server errors) aligns perfectly with the PR objectives. The addition of logging at key error points enhances observability.
Also applies to: 60-63, 240-250, 294-297, 303-307, 313-316, 323-326, 379-382, 390-392, 423-425, 456-462, 627-632, 721-727, 774-776, 783-783, 808-808, 827-829, 847-849, 1181-1183
backend/tests/auth/test_auth_service.py (1)
26-753: Excellent test coverage for auth service!This comprehensive test suite thoroughly covers all authentication workflows including success paths, error handling, and edge cases. The tests properly verify HTTP exception status codes and error messages, aligning perfectly with the improved exception handling in the service implementation.
backend/app/auth/service.py (3)
160-160: Good addition of authentication failure logging.The logging of authentication failures will help with monitoring and debugging.
233-240: Good handling of user update failures.The approach to log update failures as warnings without raising exceptions is appropriate since this is non-critical.
354-360: Good approach to token revocation error handling.The decision to log but not raise an exception for token revocation failure is appropriate since the new token was successfully created.
| raise ValueError("Group not found or user not a member") | ||
| except errors.InvalidId: # Incorrect ObjectId format | ||
| logger.warning(f"Invalid group ID format: {group_id}") | ||
| raise HTTPException(status_code=400, detail="Invalid group ID") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper exception chaining in except blocks.
When raising exceptions within except blocks, use from e to preserve the exception chain or from None to explicitly suppress it. This helps with debugging by preserving the original exception context.
For example, change:
- except errors.InvalidId:
+ except errors.InvalidId as e:
logger.warning(f"Invalid group ID format: {group_id}")
- raise HTTPException(status_code=400, detail="Invalid group ID")
+ raise HTTPException(status_code=400, detail="Invalid group ID") from eApply similar changes to all other except blocks that raise exceptions.
Also applies to: 53-54, 244-246, 249-250, 296-297, 433-433
🧰 Tools
🪛 Ruff (0.12.2)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py at lines 50, 53-54, 244-246, 249-250,
296-297, and 433, update all raise statements inside except blocks to use proper
exception chaining by adding 'from e' after the raise statement to preserve the
original exception context or 'from None' if you want to suppress it. This
involves modifying lines like 'raise HTTPException(...)' to 'raise
HTTPException(...) from e' where 'e' is the caught exception variable.
| except InvalidId: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid {field_name}" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper exception chaining in helper function.
When raising exceptions within except blocks, use proper exception chaining.
except InvalidId:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid {field_name}"
- )
+ ) from None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except InvalidId: | |
| raise HTTPException( | |
| status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid {field_name}" | |
| ) | |
| except InvalidId: | |
| raise HTTPException( | |
| status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid {field_name}" | |
| ) from None |
🧰 Tools
🪛 Ruff (0.12.2)
21-23: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/tests/auth/test_auth_service.py around lines 20 to 23, the
HTTPException is raised inside an except block without proper exception
chaining. Modify the raise statement to include "from e" where "e" is the caught
InvalidId exception, to maintain the original traceback and provide better error
context.
Devasy
added
enhancement
Devasy
left a comment
Devasy
left a comment
There was a problem hiding this comment.
Please take the review comments into consideration
There was a problem hiding this comment.
I'm gonna skip this one as It's entirely coded by you, just add some file docstring at the top explaining the tests that this file performs
| @@ -1,11 +1,10 @@ | |||
| import asyncio | |||
| from datetime import datetime, timedelta, timezone | |||
There was a problem hiding this comment.
Removal of this imports may cause downstream failures
There was a problem hiding this comment.
yes, noted, I have resolved them , it was a problem of merging from main and I accidentally commit without running tests or checking, will add requested changes soon and commit
| with pytest.raises( | ||
| ValueError, match="Expense not found or not authorized to edit" | ||
| ): | ||
| '''with pytest.raises(ValueError, match="Expense not found or not authorized to edit"): |
There was a problem hiding this comment.
From this point the file is messed up by the AI agent or maybe during the merge resolution
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
backend/app/auth/service.py (1)
291-298: Consider more specific exception handling.The generic exception catch at the method level may mask specific errors and expose internal details in the error message.
- except HTTPException: - raise - except Exception as e: - logger.exception("Unexpected error during Google authentication") - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=f"Google authentication failed: {str(e)}", - ) + except HTTPException: + raise + except Exception as e: + logger.exception("Unexpected error during Google authentication") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Google authentication failed" + ) from e
♻️ Duplicate comments (30)
backend/tests/expenses/test_expense_service.py (11)
364-380: Clean up commented code and approve HTTPException implementation.The updated test correctly expects HTTPException with status code 403 for unauthorized expense updates, which properly reflects the stricter exception handling mentioned in the PR objectives.
Remove the commented-out code block:
- """with pytest.raises(ValueError, match="Expense not found or not authorized to edit"): - await expense_service.update_expense( - "group_id", - "65f1a2b3c4d5e6f7a8b9c0d1", - update_request, - "unauthorized_user" - )"""
505-513: Clean up commented code and approve 404 exception handling.The HTTPException implementation correctly uses 404 status code for expense not found scenarios, maintaining proper HTTP semantics.
Remove the commented-out code:
- """ with pytest.raises(ValueError, match="Expense not found"): - await expense_service.get_expense_by_id("65f1a2b3c4d5e6f7a8b9c0d0", "65f1a2b3c4d5e6f7a8b9c0d1", "user_a")"""
798-814: Clean up commented code and approve authorization handling.The HTTPException test correctly validates 403 status code for unauthorized expense deletion with an appropriate error message.
Remove the malformed commented code:
- """with pytest.raises(ValueError, match="Expense not found or not authorized to delete"): - await expense_service.delete_expense(group_id, expense_id, user_id) - - mock_db.settlements.delete_many.assert_not_called() - mock_db.expenses.delete_one.assert_not_called()"""
942-955: Clean up commented code and approve manual settlement authorization.The HTTPException implementation correctly handles group access validation for manual settlement creation with status code 403.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_manual_settlement(group_id, settlement_request, user_id) - - mock_db.settlements.insert_one.assert_not_called()"""
1096-1104: Clean up commented code and approve settlements access control.The HTTPException implementation maintains consistent 403 status code usage for group access control across settlement operations.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_group_settlements(group_id, user_id)"""
1171-1181: Clean up commented code and approve settlement retrieval handling.The HTTPException test correctly uses 404 status code for settlement not found scenarios, maintaining proper HTTP semantics.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Settlement not found"): - await expense_service.get_settlement_by_id(group_id, settlement_id_str, user_id)"""
1198-1208: Clean up commented code and approve group access validation.The HTTPException implementation correctly validates group membership for settlement access with appropriate 403 status code.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_settlement_by_id(group_id, settlement_id_str, user_id)"""
1301-1313: Clean up commented code and approve settlement update handling.The HTTPException implementation correctly handles settlement update failures with 404 status code, maintaining consistency with other not found scenarios.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Settlement not found"): - await expense_service.update_settlement_status( - group_id, settlement_id_str, new_status - )"""
1391-1401: Clean up commented code and approve settlement deletion authorization.The HTTPException test maintains consistent authorization validation with proper 403 status code for settlement deletion access control.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.delete_settlement(group_id, settlement_id_str, user_id)"""
1534-1544: Clean up commented code and approve balance retrieval authorization.The HTTPException implementation correctly validates group membership for balance retrieval operations with appropriate 403 status code.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_user_balance_in_group(group_id, target_user_id_str, current_user_id)"""
2102-2110: Clean up commented code and approve analytics authorization handling.The HTTPException implementation correctly handles group analytics access control, maintaining the consistent use of 403 status codes for authorization failures throughout the test suite.
Remove the commented-out code:
- """with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.get_group_analytics(group_id, user_id)"""backend/app/expenses/service.py (3)
241-251: Add exception chaining for better error context.The exception handling is improved, but should use proper exception chaining.
Apply this fix:
- except errors.InvalidId: # Incorrect ObjectId format for group_id or expense_id + except errors.InvalidId as e: # Incorrect ObjectId format for group_id or expense_id logger.warning( f"Invalid ObjectId(s): group_id={group_id}, expense_id={expense_id}" ) raise HTTPException( - status_code=400, detail="Invalid group ID or expense ID" - ) + status_code=400, detail="Invalid group ID or expense ID" + ) from e except Exception as e: logger.error(f"Unexpected error parsing IDs: {e}") raise HTTPException( - status_code=500, detail="Unable to process IDs") + status_code=500, detail="Unable to process IDs") from e
295-298: Add exception chaining for InvalidId.- except errors.InvalidId: + except errors.InvalidId as e: logger.warning(f"Invalid expense ID format: {expense_id}") raise HTTPException( - status_code=400, detail="Invalid expense ID format") + status_code=400, detail="Invalid expense ID format") from e
49-55: Use proper exception chaining when re-raising exceptions.When raising exceptions within except blocks, use
from eto preserve the exception chain for better debugging context.Apply this fix:
- except errors.InvalidId: # Incorrect ObjectId format + except errors.InvalidId as e: # Incorrect ObjectId format logger.warning(f"Invalid group ID format: {group_id}") - raise HTTPException(status_code=400, detail="Invalid group ID") + raise HTTPException(status_code=400, detail="Invalid group ID") from e except Exception as e: logger.error(f"Unexpected error parsing groupId: {e}") raise HTTPException( - status_code=500, detail="Failed to process group ID") + status_code=500, detail="Failed to process group ID") from ebackend/app/auth/service.py (16)
141-146: Add exception chaining and use the exception variable.This exception handler should use proper exception chaining and include the exception details in logging.
- except Exception as e: - logger.exception("Unexpected error while creating user with email") - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error", - ) + except Exception as e: + logger.exception(f"Unexpected error while creating user with email: {str(e)}") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error" + ) from e
162-167: Add exception chaining for database errors.The PyMongoError handling should use exception chaining to preserve the original error context.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error" + ) from e
177-184: Add exception chaining for token generation errors.The refresh token generation error should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Failed to generate refresh token", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to generate refresh token" + ) from e
201-207: Add exception chaining for Firebase authentication errors.The Firebase ID token validation should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid Google ID token", - ) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid Google ID token" + ) from e
224-233: Add exception chaining for database user lookup.The database error handling should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error" + ) from e
263-273: Add exception chaining for user creation errors.The user creation error handling should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Failed to create user", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to create user" + ) from e
276-288: Add exception chaining for refresh token creation.The refresh token creation error handling should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Failed to generate refresh token", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to generate refresh token" + ) from e
315-329: Add exception chaining for token validation database errors.The refresh token validation error should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error" + ) from e
338-345: Add exception chaining for user lookup errors.The user lookup error should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error" + ) from e
352-361: Add exception chaining for new token creation.The new refresh token creation error should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Failed to create refresh token", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to create refresh token" + ) from e
389-396: Add exception chaining for JWT verification errors.The JWT verification error should use exception chaining.
- raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid token" - ) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid token" + ) from e
405-412: Use specific exception type and add chaining.The database operation should catch
PyMongoErrorinstead of genericExceptionand use exception chaining.- try: - user = await db.users.find_one({"_id": user_id}) - except Exception as e: - logger.error("Error while verifying token: %s", str(e)) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error", - ) + try: + user = await db.users.find_one({"_id": user_id}) + except PyMongoError as e: + logger.error("Database error while verifying token: %s", str(e)) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error" + ) from e
429-437: Add exception chaining for user lookup errors.The database user lookup error should use exception chaining.
- raise HTTPException( - status_code=500, detail="Internal server error during user lookup." - ) + raise HTTPException( + status_code=500, detail="Internal server error during user lookup." + ) from e
448-465: Add exception chaining for token storage errors.The reset token storage error should use exception chaining.
- raise HTTPException( - status_code=500, detail="Internal server error during token storage." - ) + raise HTTPException( + status_code=500, detail="Internal server error during token storage." + ) from e
533-541: Add exception chaining for unexpected errors.The generic exception handler should use exception chaining to preserve error context.
- raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error during password reset", - ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error during password reset" + ) from e
562-582: Add exception chaining and remove internal error details from client response.The exception handling should use chaining and avoid exposing internal implementation details.
- except Exception as e: - logger.error( - f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}" - ) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=f"Failed to create refresh token: {str(e)}", - ) + except Exception as e: + logger.error( + f"Failed to create refresh token for user_id: {user_id}. Error: {str(e)}" + ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to create refresh token" + ) from e
🧹 Nitpick comments (6)
backend/tests/expenses/test_expense_routes.py (1)
6-6: Remove unused Firebase auth import.The
firebase_admin.authimport is currently unused in this test file. While the AI summary suggests this was added for future Firebase authentication integration, it's better to add imports when they're actually needed.-from firebase_admin import auth as firebase_authNote: The
conftest.pyalready provides comprehensive Firebase mocking through themock_firebase_adminfixture, so when Firebase auth integration is implemented in these route tests, you can utilize that existing infrastructure.backend/tests/expenses/test_expense_service.py (2)
8-8: Remove unused import.The
bson.errorsimport is flagged as unused by static analysis. If it's not being used in the current test implementations, it should be removed.-from bson import ObjectId, errors +from bson import ObjectId
124-148: Clean up commented code and approve exception handling updates.The new HTTPException-based tests correctly validate status codes (400 for invalid ObjectId, 403 for authorization failures) and error messages. This aligns perfectly with the PR's objective of improving exception handling.
Remove the commented-out code block to eliminate clutter:
- """# Test with invalid ObjectId format - with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_expense( - "invalid_group", expense_request, "user_a" - ) - - # Test with valid ObjectId format but non-existent group - with pytest.raises(ValueError, match="Group not found or user not a member"): - await expense_service.create_expense("65f1a2b3c4d5e6f7a8b9c0d0", expense_request, "user_a")"""backend/tests/auth/test_auth_service.py (2)
69-75: Reconsider the validate_object_id helper function usage.The
validate_object_idhelper is only used once intest_create_refresh_token_record_invalid_user_id, and that test doesn't actually test the AuthService. Consider either:
- Remove this helper and the test since it's not testing service functionality
- Modify the test to actually test the service's handling of invalid ObjectIds
If keeping the helper, add exception chaining:
except InvalidId: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid {field_name}" - ) + ) from NoneAlso applies to: 798-806
637-640: Simplify nested with statements.Combine multiple context managers into a single with statement for cleaner code.
For lines 637-640:
- with patch("app.auth.service.generate_reset_token", return_value="mocktoken"): - with caplog.at_level( - logging.INFO - ): # caplog captures the log messages (previously print statements) + with patch("app.auth.service.generate_reset_token", return_value="mocktoken"), \ + caplog.at_level(logging.INFO): result = await service.request_password_reset("[email protected]")For lines 692-693:
- with patch("app.auth.service.generate_reset_token", return_value="mocktoken"): - with pytest.raises(HTTPException) as exc_info: + with patch("app.auth.service.generate_reset_token", return_value="mocktoken"), \ + pytest.raises(HTTPException) as exc_info: await service.request_password_reset("[email protected]")Also applies to: 692-693
backend/app/auth/service.py (1)
8-8: Remove unused import.The
create_access_tokenimport is not used anywhere in this file.- create_access_token,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/auth/service.py(15 hunks)backend/app/expenses/service.py(17 hunks)backend/app/groups/service.py(7 hunks)backend/tests/auth/test_auth_service.py(1 hunks)backend/tests/expenses/test_expense_routes.py(1 hunks)backend/tests/expenses/test_expense_service.py(14 hunks)backend/tests/user/test_user_service.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/groups/service.py
- backend/tests/user/test_user_service.py
🧰 Additional context used
📓 Path-based instructions (5)
backend/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend code must be implemented using FastAPI with Python 3.9+ and organized under the /backend/ directory.
Files:
backend/app/auth/service.pybackend/app/expenses/service.pybackend/tests/auth/test_auth_service.pybackend/tests/expenses/test_expense_routes.pybackend/tests/expenses/test_expense_service.py
backend/app/{auth,user,groups,expenses}/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/{auth,user,groups,expenses}/**/*.py: Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
When adding a new API endpoint, add the route to the appropriate service router file in the backend.
Files:
backend/app/auth/service.pybackend/app/expenses/service.py
backend/app/auth/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
JWT authentication with refresh token rotation must be implemented in the backend.
Files:
backend/app/auth/service.py
backend/app/expenses/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/expenses/**/*.py: Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Settlements must track debt resolution between users in the expense tracking logic.
Files:
backend/app/expenses/service.py
backend/tests/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend tests must be placed in the /backend/tests/ directory and run using pytest.
Files:
backend/tests/auth/test_auth_service.pybackend/tests/expenses/test_expense_routes.pybackend/tests/expenses/test_expense_service.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
backend/app/auth/service.py (5)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/auth/security.py : To troubleshoot authentication issues, check JWT token handling in app/auth/security.py.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/auth/**/*.py : JWT authentication with refresh token rotation must be implemented in the backend.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/database.py : MongoDB must be used for persistent data storage in the backend, with connection logic in backend/app/database.py.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : When adding a new API endpoint, add the route to the appropriate service router file in the backend.
backend/app/expenses/service.py (4)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Settlements must track debt resolution between users in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/database.py : MongoDB must be used for persistent data storage in the backend, with connection logic in backend/app/database.py.
backend/tests/auth/test_auth_service.py (1)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/auth/security.py : To troubleshoot authentication issues, check JWT token handling in app/auth/security.py.
backend/tests/expenses/test_expense_routes.py (2)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : When adding a new API endpoint, add the route to the appropriate service router file in the backend.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/{auth,user,groups,expenses}/**/*.py : Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
backend/tests/expenses/test_expense_service.py (3)
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Settlements must track debt resolution between users in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
🧬 Code Graph Analysis (2)
backend/tests/auth/test_auth_service.py (4)
backend/app/auth/security.py (3)
create_refresh_token(80-87)get_password_hash(37-47)verify_password(23-34)backend/app/auth/service.py (10)
AuthService(74-584)create_user_with_email(85-146)authenticate_user_with_email(148-185)authenticate_with_google(187-298)get_db(79-83)_create_refresh_token_record(543-584)refresh_access_token(300-372)verify_access_token(374-419)request_password_reset(421-474)confirm_password_reset(476-541)backend/tests/conftest.py (1)
mock_db(65-100)backend/tests/auth/test_auth_routes.py (2)
test_signup_with_email_success(19-49)test_login_with_email_success(120-163)
backend/tests/expenses/test_expense_routes.py (1)
backend/tests/conftest.py (1)
mock_firebase_admin(17-58)
🪛 Ruff (0.12.2)
backend/app/auth/service.py
8-8: app.auth.security.create_access_token imported but unused
Remove unused import: app.auth.security.create_access_token
(F401)
141-141: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
143-146: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-167: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
181-184: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
204-207: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
230-233: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
270-273: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
285-288: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
326-329: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
342-345: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
358-361: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
394-396: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
409-412: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
435-437: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
463-465: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
538-541: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
579-582: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/expenses/service.py
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
54-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
245-247: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
250-251: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
297-298: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
415-415: f-string without any placeholders
Remove extraneous f prefix
(F541)
434-434: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/tests/auth/test_auth_service.py
56-56: firebase_admin imported but unused
Remove unused import: firebase_admin
(F401)
58-58: app.auth.security.create_refresh_token imported but unused
Remove unused import
(F401)
58-58: app.auth.security.get_password_hash imported but unused
Remove unused import
(F401)
58-58: app.auth.security.verify_password imported but unused
Remove unused import
(F401)
64-64: firebase_admin.credentials imported but unused
Remove unused import: firebase_admin.credentials
(F401)
73-75: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
637-640: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
692-693: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
799-799: Local variable service is assigned to but never used
Remove assignment to unused variable service
(F841)
backend/tests/expenses/test_expense_routes.py
6-6: firebase_admin.auth imported but unused
Remove unused import: firebase_admin.auth
(F401)
backend/tests/expenses/test_expense_service.py
8-8: bson.errors imported but unused
Remove unused import: bson.errors
(F401)
🔇 Additional comments (6)
backend/app/expenses/service.py (2)
61-64: Good improvement in authorization error handling.Using HTTPException with 403 status code for authorization failures is the correct approach.
457-463: Excellent security logging for unauthorized access attempts.Adding warning logs for unauthorized delete attempts provides a good audit trail while raising appropriate HTTPException with 403 status.
backend/app/auth/service.py (4)
20-21: LGTM! Essential imports for improved exception handling.The new imports for
JWTErrorandPyMongoErrorare correctly added and used throughout the file for specific exception handling.
170-170: LGTM! Good addition of security logging.Adding logging for authentication failures helps with security monitoring and debugging.
243-250: LGTM! Appropriate handling of non-critical user update failure.Logging a warning for profile update failure without raising an exception is correct since this shouldn't block the authentication flow.
364-370: LGTM! Appropriate handling of token revocation failure.Logging the token revocation error without raising is correct since the new token was successfully issued. The comment clearly explains this decision.
| except Exception: | ||
| logger.error( | ||
| f"Warning: Failed to recalculate settlements: {e}", | ||
| exc_info=True, | ||
| f"Warning: Failed to recalculate settlements", exc_info=True | ||
| ) |
There was a problem hiding this comment.
Fix f-string without placeholders.
The f-string on line 415 has no placeholders.
- except Exception:
+ except Exception:
logger.error(
- f"Warning: Failed to recalculate settlements", exc_info=True
+ "Warning: Failed to recalculate settlements", exc_info=True
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception: | |
| logger.error( | |
| f"Warning: Failed to recalculate settlements: {e}", | |
| exc_info=True, | |
| f"Warning: Failed to recalculate settlements", exc_info=True | |
| ) | |
| except Exception: | |
| logger.error( | |
| "Warning: Failed to recalculate settlements", exc_info=True | |
| ) |
🧰 Tools
🪛 Ruff (0.12.2)
415-415: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 413 to 416, the f-string used in
the logger.error call does not contain any placeholders, making the f-string
unnecessary. Replace the f-string with a regular string literal by removing the
leading 'f' before the string to fix this.
| ) as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback | ||
| logger.exception( | ||
| f"Unhandled error in update_expense for expense {expense_id}: {e}" | ||
| ) | ||
| import traceback | ||
|
|
||
| traceback.print_exc() | ||
| raise Exception(f"Database error during expense update: {str(e)}") |
There was a problem hiding this comment.
Remove redundant traceback printing and add exception chaining.
The logger.exception() already logs the full traceback, making traceback.print_exc() redundant.
except (
Exception
) as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback
logger.exception(
f"Unhandled error in update_expense for expense {expense_id}: {e}"
)
- import traceback
-
- traceback.print_exc()
- raise Exception(f"Database error during expense update: {str(e)}")
+ raise Exception(f"Database error during expense update: {str(e)}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback | |
| logger.exception( | |
| f"Unhandled error in update_expense for expense {expense_id}: {e}" | |
| ) | |
| import traceback | |
| traceback.print_exc() | |
| raise Exception(f"Database error during expense update: {str(e)}") | |
| except ( | |
| Exception | |
| ) as e: # logger.exception() will provide the entire traceback, so its safe to remove traceback | |
| logger.exception( | |
| f"Unhandled error in update_expense for expense {expense_id}: {e}" | |
| ) | |
| raise Exception(f"Database error during expense update: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
444-444: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 437 to 444, remove the redundant
call to traceback.print_exc() since logger.exception() already logs the full
traceback. Additionally, when re-raising the exception, use exception chaining
by adding "from e" to preserve the original exception context.
| import logging | ||
| from datetime import datetime, timedelta, timezone | ||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
| import firebase_admin | ||
| import pytest | ||
| from app.auth.security import create_refresh_token, get_password_hash, verify_password | ||
| from app.auth.service import AuthService | ||
| from bson import ObjectId | ||
| from bson.errors import InvalidId | ||
| from fastapi import HTTPException, status | ||
| from firebase_admin import auth as firebase_auth | ||
| from firebase_admin import credentials |
There was a problem hiding this comment.
Remove unused imports to keep the code clean.
Several imports are not used in the test file.
Remove these unused imports:
-import logging
from datetime import datetime, timedelta, timezone
from unittest.mock import AsyncMock, MagicMock, patch
-import firebase_admin
import pytest
-from app.auth.security import create_refresh_token, get_password_hash, verify_password
from app.auth.service import AuthService
from bson import ObjectId
from bson.errors import InvalidId
from fastapi import HTTPException, status
from firebase_admin import auth as firebase_auth
-from firebase_admin import credentials
from jose import JWTError
from pymongo.errors import DuplicateKeyError, PyMongoError📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import logging | |
| from datetime import datetime, timedelta, timezone | |
| from unittest.mock import AsyncMock, MagicMock, patch | |
| import firebase_admin | |
| import pytest | |
| from app.auth.security import create_refresh_token, get_password_hash, verify_password | |
| from app.auth.service import AuthService | |
| from bson import ObjectId | |
| from bson.errors import InvalidId | |
| from fastapi import HTTPException, status | |
| from firebase_admin import auth as firebase_auth | |
| from firebase_admin import credentials | |
| from datetime import datetime, timedelta, timezone | |
| from unittest.mock import AsyncMock, MagicMock, patch | |
| import pytest | |
| from app.auth.service import AuthService | |
| from bson import ObjectId | |
| from bson.errors import InvalidId | |
| from fastapi import HTTPException, status | |
| from firebase_admin import auth as firebase_auth | |
| from jose import JWTError | |
| from pymongo.errors import DuplicateKeyError, PyMongoError |
🧰 Tools
🪛 Ruff (0.12.2)
56-56: firebase_admin imported but unused
Remove unused import: firebase_admin
(F401)
58-58: app.auth.security.create_refresh_token imported but unused
Remove unused import
(F401)
58-58: app.auth.security.get_password_hash imported but unused
Remove unused import
(F401)
58-58: app.auth.security.verify_password imported but unused
Remove unused import
(F401)
64-64: firebase_admin.credentials imported but unused
Remove unused import: firebase_admin.credentials
(F401)
🤖 Prompt for AI Agents
In backend/tests/auth/test_auth_service.py around lines 52 to 64, there are
several imports that are not used anywhere in the test file. Review the imports
and remove any that are not referenced in the code to keep the file clean and
maintainable. This includes removing unused modules like logging, datetime,
timezone, AsyncMock, MagicMock, patch, firebase_admin, pytest,
create_refresh_token, get_password_hash, verify_password, AuthService, ObjectId,
InvalidId, HTTPException, status, firebase_auth, and credentials if they are not
used.
2 participants
🔧 Improve Exception Handling & Testing for Backend Services
📌 Summary
This PR enhances the backend robustness by improving exception handling and test coverage across multiple service layers.
✅ Changes Made
except Exceptionblocks in:auth/service.pyuser/service.pygroups/service.pyexpenses/service.pyHTTPExceptionhandling with appropriate status codes.auth/service.py(now with 100% coverage).🧪 Test Coverage Summary
98%100%📸 Attached: Test coverage report screenshot for verification.
🔗 Fixes #60
This contribution addresses the GSSoC'25 issue on improving backend exception handling and testing.
Summary by CodeRabbit
Bug Fixes
Tests