perf(users): drop redundant get_user_by_id refetch in session-user endpoints (#23794)
* perf(users): drop redundant get_user_by_id refetch in session-user endpoints Five /user/* handlers refetched the user row via Users.get_user_by_id(user.id) immediately after receiving an identical UserModel from Depends(get_verified_user). Since get_verified_user already populated the user within the same request microseconds earlier, the refetch is pure overhead. The dead else branches (unreachable — get_verified_user raises 401 on missing user) are removed as a natural consequence. Affected endpoints: - GET /user/settings - GET /user/status - POST /user/status/update - GET /user/info - POST /user/info/update Eliminates one SELECT per request to each of these endpoints with no behavioral change. * fix(users): preserve USER_NOT_FOUND error on status update failure update_user_status_by_id returns None when the target user is missing or the update raises. The previous commit removed the pre-update existence gate (get_user_by_id) and returned the update result directly, which turned not-found/failure cases into 200 OK with a null body instead of the expected 400 USER_NOT_FOUND. Guard the update result explicitly to preserve the original API contract, matching the equivalent pattern already applied in /user/info/update. * docs(users): note lost-update tradeoff on /user/info/update Make the concurrency tradeoff explicit: merging against the auth-time snapshot slightly widens the lost-update window compared to the previous pre-merge refetch, but the refetch only narrowed (did not eliminate) that window. Real safety requires row locking or a version column. --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -276,14 +276,8 @@ async def update_default_user_permissions(request: Request, form_data: UserPermi
|
||||
async def get_user_settings_by_session_user(
|
||||
user=Depends(get_verified_user), db: AsyncSession = Depends(get_async_session)
|
||||
):
|
||||
user = await Users.get_user_by_id(user.id, db=db)
|
||||
if user:
|
||||
return user.settings
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=ERROR_MESSAGES.USER_NOT_FOUND,
|
||||
)
|
||||
# user already fetched by get_verified_user — no need to refetch
|
||||
return user.settings
|
||||
|
||||
|
||||
############################
|
||||
@@ -339,14 +333,8 @@ async def get_user_status_by_session_user(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail=ERROR_MESSAGES.ACTION_PROHIBITED,
|
||||
)
|
||||
user = await Users.get_user_by_id(user.id, db=db)
|
||||
if user:
|
||||
return user
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=ERROR_MESSAGES.USER_NOT_FOUND,
|
||||
)
|
||||
# user already fetched by get_verified_user — no need to refetch
|
||||
return user
|
||||
|
||||
|
||||
############################
|
||||
@@ -366,15 +354,14 @@ async def update_user_status_by_session_user(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail=ERROR_MESSAGES.ACTION_PROHIBITED,
|
||||
)
|
||||
user = await Users.get_user_by_id(user.id, db=db)
|
||||
if user:
|
||||
user = await Users.update_user_status_by_id(user.id, form_data, db=db)
|
||||
return user
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=ERROR_MESSAGES.USER_NOT_FOUND,
|
||||
)
|
||||
# user already fetched by get_verified_user — no need to refetch
|
||||
updated = await Users.update_user_status_by_id(user.id, form_data, db=db)
|
||||
if updated:
|
||||
return updated
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=ERROR_MESSAGES.USER_NOT_FOUND,
|
||||
)
|
||||
|
||||
|
||||
############################
|
||||
@@ -384,14 +371,8 @@ async def update_user_status_by_session_user(
|
||||
|
||||
@router.get('/user/info', response_model=Optional[dict])
|
||||
async def get_user_info_by_session_user(user=Depends(get_verified_user), db: AsyncSession = Depends(get_async_session)):
|
||||
user = await Users.get_user_by_id(user.id, db=db)
|
||||
if user:
|
||||
return user.info
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=ERROR_MESSAGES.USER_NOT_FOUND,
|
||||
)
|
||||
# user already fetched by get_verified_user — no need to refetch
|
||||
return user.info
|
||||
|
||||
|
||||
############################
|
||||
@@ -403,19 +384,13 @@ async def get_user_info_by_session_user(user=Depends(get_verified_user), db: Asy
|
||||
async def update_user_info_by_session_user(
|
||||
form_data: dict, user=Depends(get_verified_user), db: AsyncSession = Depends(get_async_session)
|
||||
):
|
||||
user = await Users.get_user_by_id(user.id, db=db)
|
||||
if user:
|
||||
if user.info is None:
|
||||
user.info = {}
|
||||
|
||||
user = await Users.update_user_by_id(user.id, {'info': {**user.info, **form_data}}, db=db)
|
||||
if user:
|
||||
return user.info
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail=ERROR_MESSAGES.USER_NOT_FOUND,
|
||||
)
|
||||
# Merges against the auth-time snapshot of user.info. The previous pre-merge
|
||||
# refetch only narrowed (did not eliminate) the lost-update window on concurrent
|
||||
# same-user writes; real safety needs row locking or a version column.
|
||||
existing_info = user.info or {}
|
||||
updated = await Users.update_user_by_id(user.id, {'info': {**existing_info, **form_data}}, db=db)
|
||||
if updated:
|
||||
return updated.info
|
||||
else:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
|
||||
Reference in New Issue
Block a user