Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to retrieve users by UUID, removes an unused field from settings filters, and cleans up deprecated Pydantic configuration. The PR is marked as "InProgress" in the title, suggesting it's still under development.
Changes:
- Added new service function and endpoint to retrieve users by UUID
- Removed
settings_typefield fromSettingsFiltermodel - Removed deprecated
orm_modeConfig fromCommentResponseschema
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/pin_sphere/users/service.py | Added get_user_by_user_id function to fetch users by UUID, added models import |
| server/pin_sphere/users/filters.py | Removed unused settings_type field and Literal import from SettingsFilter |
| server/pin_sphere/users/endpoint.py | Added GET endpoint for retrieving users by UUID with new imports |
| server/pin_sphere/comments/schemas.py | Removed deprecated orm_mode configuration from CommentResponse |
Comments suppressed due to low confidence (1)
server/pin_sphere/users/filters.py:72
- The removal of the settings_type field may cause issues with the update_settings function in service.py:132, which explicitly excludes "settings_type" when calling model_dump. With settings_type removed from the model, this exclusion is now referencing a non-existent field. While this won't cause an immediate error, the exclusion is now unnecessary and should be removed from service.py to keep the code clean.
general: Optional[GeneralSettingsFilter] = None
notification: Optional[NotificationSettingsFilter] = None
appearance: Optional[AppearanceSettingsFilter] = None
privacy_and_security: Optional[PrivacySecuritySettingsFilter] = None
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def get_user_by_user_id( | ||
| user_id: UUID = Path(), db: AsyncSession = Depends(get_async_session) | ||
| ): | ||
| return await service.get_user_by_user_id(user_id, db) |
There was a problem hiding this comment.
This endpoint lacks error handling when the user is not found. The function can return None (as specified in the service function return type), but there's no check to raise a 404 HTTPException when the user doesn't exist. Other similar endpoints in this file (like delete_account on line 109, read_user on line 184) properly check for None and raise appropriate exceptions. This should follow the same pattern.
| return await service.get_user_by_user_id(user_id, db) | |
| user = await service.get_user_by_user_id(user_id, db) | |
| if not user: | |
| raise HTTPException(status_code=404, detail="User not found") | |
| return user |
|
|
||
|
|
||
| @router.get( | ||
| "/{user_id}", |
There was a problem hiding this comment.
This endpoint path "/{user_id}" conflicts with the existing "/{username}" GET endpoint on line 178. FastAPI will route requests to the first matching pattern, which means this new endpoint will intercept all requests intended for the username endpoint. When a client requests "/users/john", FastAPI will try to parse "john" as a UUID for this endpoint instead of treating it as a username. Consider using a more specific path like "/by-id/{user_id}" or "/user-id/{user_id}" to avoid this ambiguity.
| "/{user_id}", | |
| "/by-id/{user_id}", |
| async def get_user_by_user_id( | ||
| user_id: uuid.UUID, db: AsyncSession | ||
| ) -> models.User | None: | ||
| query = select(models.User).filter(models.User.id == str(user_id)) |
There was a problem hiding this comment.
There's an inconsistency in the model reference. This function uses models.User while other functions in the same file (like get_user, get_user_by_email, get_user_by_username) use the directly imported User from core.models. Since User is already imported on line 11, consider using User directly instead of models.User for consistency with the rest of the codebase.
| async def get_user_by_user_id( | ||
| user_id: uuid.UUID, db: AsyncSession | ||
| ) -> models.User | None: | ||
| query = select(models.User).filter(models.User.id == str(user_id)) |
There was a problem hiding this comment.
The User model's id field is defined as UUID type in the database (see core/database/base_model.py:34), but this query converts the UUID to a string for comparison. This conversion may not be necessary depending on the database driver's UUID handling. Other functions in this file (like get_user, get_user_by_email) don't perform string conversion when filtering. Consider checking if the database driver handles UUID comparisons directly, and if so, use models.User.id == user_id instead for consistency and potential performance improvement.
| query = select(models.User).filter(models.User.id == str(user_id)) | |
| query = select(models.User).filter(models.User.id == user_id) |
| async def get_user_by_user_id( | ||
| user_id: uuid.UUID, db: AsyncSession |
There was a problem hiding this comment.
The parameter order is inconsistent with other service functions in this file. Other functions follow the pattern of putting the database session (db) as the first parameter (see get_user, get_user_by_email, get_user_by_username). This function should use get_user_by_user_id(db: AsyncSession, user_id: uuid.UUID) for consistency with established conventions in this file.
| summary="Delete user account", | ||
| description="Delete a user account by username. Returns the deleted user information.", |
There was a problem hiding this comment.
The summary and description are incorrect for this endpoint. The endpoint is a GET operation that retrieves a user by ID, not a DELETE operation. The summary should be something like "Get user by ID" and the description should indicate that it retrieves user information by their unique user ID.
| summary="Delete user account", | |
| description="Delete a user account by username. Returns the deleted user information.", | |
| summary="Get user by ID", | |
| description="Retrieve user information by their unique user ID.", |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 26880863 | Triggered | PostgreSQL Credentials | 77ce6a4 | server/alembic.ini | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
No description provided.