mo/mitlist
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refactor logging in item API endpoints to use a local variable for user email; enhance clarity and maintainability of log messages. Update transaction management in item CRUD operations to ensure proper commit handling and version conflict checks.

Browse Source
This commit is contained in:
mohamad 2025-05-08 23:27:51 +02:00
parent d99aef9d11
commit 0dbee3bb4b
2 changed files with 44 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
be/app/api/v1/endpoints/items.py
View File

@ -56,7 +56,8 @@ async def create_list_item(
current_user: UserModel = Depends(get_current_user), current_user: UserModel = Depends(get_current_user),
): ):
"""Adds a new item to a specific list. User must have access to the list.""" """Adds a new item to a specific list. User must have access to the list."""
logger.info(f"User {current_user.email} adding item to list {list_id}: {item_in.name}") user_email = current_user.email # Access email attribute before async operations
logger.info(f"User {user_email} adding item to list {list_id}: {item_in.name}")
# Verify user has access to the target list # Verify user has access to the target list
try: try:
await crud_list.check_list_permission(db=db, list_id=list_id, user_id=current_user.id) await crud_list.check_list_permission(db=db, list_id=list_id, user_id=current_user.id)
@ -67,7 +68,7 @@ async def create_list_item(
created_item = await crud_item.create_item( created_item = await crud_item.create_item(
db=db, item_in=item_in, list_id=list_id, user_id=current_user.id db=db, item_in=item_in, list_id=list_id, user_id=current_user.id
) )
logger.info(f"Item '{created_item.name}' (ID: {created_item.id}) added to list {list_id} by user {current_user.email}.") logger.info(f"Item '{created_item.name}' (ID: {created_item.id}) added to list {list_id} by user {user_email}.")
return created_item return created_item
@ -84,7 +85,8 @@ async def read_list_items(
# Add sorting/filtering params later if needed: sort_by: str = 'created_at', order: str = 'asc' # Add sorting/filtering params later if needed: sort_by: str = 'created_at', order: str = 'asc'
): ):
"""Retrieves all items for a specific list if the user has access.""" """Retrieves all items for a specific list if the user has access."""
logger.info(f"User {current_user.email} listing items for list {list_id}") user_email = current_user.email # Access email attribute before async operations
logger.info(f"User {user_email} listing items for list {list_id}")
# Verify user has access to the list # Verify user has access to the list
try: try:
await crud_list.check_list_permission(db=db, list_id=list_id, user_id=current_user.id) await crud_list.check_list_permission(db=db, list_id=list_id, user_id=current_user.id)
@ -119,20 +121,21 @@ async def update_item(
If the version does not match, a 409 Conflict is returned. If the version does not match, a 409 Conflict is returned.
Sets/unsets `completed_by_id` based on `is_complete` flag. Sets/unsets `completed_by_id` based on `is_complete` flag.
""" """
logger.info(f"User {current_user.email} attempting to update item ID: {item_id} with version {item_in.version}") user_email = current_user.email # Access email attribute before async operations
logger.info(f"User {user_email} attempting to update item ID: {item_id} with version {item_in.version}")
# Permission check is handled by get_item_and_verify_access dependency # Permission check is handled by get_item_and_verify_access dependency
try: try:
updated_item = await crud_item.update_item( updated_item = await crud_item.update_item(
db=db, item_db=item_db, item_in=item_in, user_id=current_user.id db=db, item_db=item_db, item_in=item_in, user_id=current_user.id
) )
logger.info(f"Item {item_id} updated successfully by user {current_user.email} to version {updated_item.version}.") logger.info(f"Item {item_id} updated successfully by user {user_email} to version {updated_item.version}.")
return updated_item return updated_item
except ConflictError as e: except ConflictError as e:
logger.warning(f"Conflict updating item {item_id} for user {current_user.email}: {str(e)}") logger.warning(f"Conflict updating item {item_id} for user {user_email}: {str(e)}")
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e))
except Exception as e: except Exception as e:
logger.error(f"Error updating item {item_id} for user {current_user.email}: {str(e)}") logger.error(f"Error updating item {item_id} for user {user_email}: {str(e)}")
raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="An unexpected error occurred while updating the item.") raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="An unexpected error occurred while updating the item.")
@ -157,12 +160,13 @@ async def delete_item(
If `expected_version` is provided and does not match the item's current version, If `expected_version` is provided and does not match the item's current version,
a 409 Conflict is returned. a 409 Conflict is returned.
""" """
logger.info(f"User {current_user.email} attempting to delete item ID: {item_id}, expected version: {expected_version}") user_email = current_user.email # Access email attribute before async operations
logger.info(f"User {user_email} attempting to delete item ID: {item_id}, expected version: {expected_version}")
# Permission check is handled by get_item_and_verify_access dependency # Permission check is handled by get_item_and_verify_access dependency
if expected_version is not None and item_db.version != expected_version: if expected_version is not None and item_db.version != expected_version:
logger.warning( logger.warning(
f"Conflict deleting item {item_id} for user {current_user.email}. " f"Conflict deleting item {item_id} for user {user_email}. "
f"Expected version {expected_version}, actual version {item_db.version}." f"Expected version {expected_version}, actual version {item_db.version}."
) )
raise HTTPException( raise HTTPException(
@ -171,5 +175,5 @@ async def delete_item(
) )
await crud_item.delete_item(db=db, item_db=item_db) await crud_item.delete_item(db=db, item_db=item_db)
logger.info(f"Item {item_id} (version {item_db.version}) deleted successfully by user {current_user.email}.") logger.info(f"Item {item_id} (version {item_db.version}) deleted successfully by user {user_email}.")
return Response(status_code=status.HTTP_204_NO_CONTENT) return Response(status_code=status.HTTP_204_NO_CONTENT)

54
be/app/crud/item.py
View File

@ -73,32 +73,37 @@ async def get_item_by_id(db: AsyncSession, item_id: int) -> Optional[ItemModel]:
async def update_item(db: AsyncSession, item_db: ItemModel, item_in: ItemUpdate, user_id: int) -> ItemModel: async def update_item(db: AsyncSession, item_db: ItemModel, item_in: ItemUpdate, user_id: int) -> ItemModel:
"""Updates an existing item record, checking for version conflicts.""" """Updates an existing item record, checking for version conflicts."""
try: try:
async with db.begin(): # Check version conflict
if item_db.version != item_in.version: if item_db.version != item_in.version:
raise ConflictError( raise ConflictError(
f"Item '{item_db.name}' (ID: {item_db.id}) has been modified. " f"Item '{item_db.name}' (ID: {item_db.id}) has been modified. "
f"Your version is {item_in.version}, current version is {item_db.version}. Please refresh." f"Your version is {item_in.version}, current version is {item_db.version}. Please refresh."
) )
update_data = item_in.model_dump(exclude_unset=True, exclude={'version'}) # Exclude version update_data = item_in.model_dump(exclude_unset=True, exclude={'version'}) # Exclude version
# Special handling for is_complete # Special handling for is_complete
if 'is_complete' in update_data: if 'is_complete' in update_data:
if update_data['is_complete'] is True: if update_data['is_complete'] is True:
if item_db.completed_by_id is None: # Only set if not already completed by someone if item_db.completed_by_id is None: # Only set if not already completed by someone
update_data['completed_by_id'] = user_id update_data['completed_by_id'] = user_id
else: else:
update_data['completed_by_id'] = None # Clear if marked incomplete update_data['completed_by_id'] = None # Clear if marked incomplete
for key, value in update_data.items(): # Apply updates
setattr(item_db, key, value) for key, value in update_data.items():
setattr(item_db, key, value)
item_db.version += 1 # Increment version item_db.version += 1 # Increment version
db.add(item_db) db.add(item_db)
await db.flush() await db.flush()
await db.refresh(item_db) await db.refresh(item_db)
return item_db
# Commit the transaction if not part of a larger transaction
await db.commit()
return item_db
except IntegrityError as e: except IntegrityError as e:
await db.rollback() await db.rollback()
raise DatabaseIntegrityError(f"Failed to update item due to integrity constraint: {str(e)}") raise DatabaseIntegrityError(f"Failed to update item due to integrity constraint: {str(e)}")
@ -115,10 +120,9 @@ async def update_item(db: AsyncSession, item_db: ItemModel, item_in: ItemUpdate,
async def delete_item(db: AsyncSession, item_db: ItemModel) -> None: async def delete_item(db: AsyncSession, item_db: ItemModel) -> None:
"""Deletes an item record. Version check should be done by the caller (API endpoint).""" """Deletes an item record. Version check should be done by the caller (API endpoint)."""
try: try:
async with db.begin(): await db.delete(item_db)
await db.delete(item_db) await db.commit()
# await db.commit() # Not needed with async with db.begin() return None
return None
except OperationalError as e: except OperationalError as e:
await db.rollback() await db.rollback()
raise DatabaseConnectionError(f"Database connection error while deleting item: {str(e)}") raise DatabaseConnectionError(f"Database connection error while deleting item: {str(e)}")