99 lines
3.8 KiB
Markdown
99 lines
3.8 KiB
Markdown
# Soft Deletion Commit Fix
|
|
|
|
## Issue Description
|
|
|
|
The soft deletion functionality for shots and assets was not properly setting the `deleted_by` and `deleted_at` fields in the database. This was happening because the soft deletion services were using `db.flush()` to send SQL statements to the database but were not calling `db.commit()` to actually commit the transaction.
|
|
|
|
## Root Cause
|
|
|
|
The issue was in the following services and their corresponding router endpoints:
|
|
|
|
1. **ShotSoftDeletionService** (`backend/services/shot_soft_deletion.py`)
|
|
- Method: `soft_delete_shot_cascade()`
|
|
- Used `db.flush()` but no `db.commit()`
|
|
|
|
2. **AssetSoftDeletionService** (`backend/services/asset_soft_deletion.py`)
|
|
- Method: `soft_delete_asset_cascade()`
|
|
- Used `db.flush()` but no `db.commit()`
|
|
|
|
3. **RecoveryService** (`backend/services/recovery_service.py`)
|
|
- Methods: `recover_shot()`, `recover_asset()`, `bulk_recover_shots()`, `bulk_recover_assets()`
|
|
- Used `db.flush()` but no `db.commit()`
|
|
|
|
4. **BatchOperationsService** (`backend/services/batch_operations.py`)
|
|
- Methods: `batch_delete_shots()`, `batch_delete_assets()`, `batch_recover_shots()`, `batch_recover_assets()`
|
|
- Used `db.flush()` but no `db.commit()`
|
|
|
|
## The Difference Between flush() and commit()
|
|
|
|
- `db.flush()`: Sends pending SQL statements to the database within the current transaction, but doesn't commit the transaction
|
|
- `db.commit()`: Actually commits the transaction, making the changes permanent in the database
|
|
|
|
Without `db.commit()`, the changes were only visible within the current transaction and would be lost when the transaction ended.
|
|
|
|
## Files Fixed
|
|
|
|
### Router Endpoints (Added db.commit() calls):
|
|
|
|
1. **backend/routers/shots.py**
|
|
- `delete_shot()` endpoint - Added commit after successful soft deletion
|
|
|
|
2. **backend/routers/assets.py**
|
|
- `delete_asset()` endpoint - Added commit after successful soft deletion
|
|
|
|
3. **backend/routers/admin.py**
|
|
- `recover_shot()` endpoint - Added commit after successful recovery
|
|
- `recover_asset()` endpoint - Added commit after successful recovery
|
|
- `bulk_recover_shots()` endpoint - Added commit after successful bulk recovery
|
|
- `bulk_recover_assets()` endpoint - Added commit after successful bulk recovery
|
|
- `batch_delete_shots()` endpoint - Added commit after successful batch deletion
|
|
- `batch_delete_assets()` endpoint - Added commit after successful batch deletion
|
|
- `batch_recover_shots_enhanced()` endpoint - Added commit after successful batch recovery
|
|
- `batch_recover_assets_enhanced()` endpoint - Added commit after successful batch recovery
|
|
|
|
### Pattern Applied:
|
|
|
|
```python
|
|
# Before (incorrect):
|
|
result = service.soft_delete_shot_cascade(shot_id, db, current_user)
|
|
|
|
if not result.success:
|
|
raise HTTPException(...)
|
|
|
|
return response_data
|
|
|
|
# After (correct):
|
|
result = service.soft_delete_shot_cascade(shot_id, db, current_user)
|
|
|
|
if not result.success:
|
|
db.rollback() # Also added rollback on failure
|
|
raise HTTPException(...)
|
|
|
|
# Commit the transaction
|
|
db.commit()
|
|
|
|
return response_data
|
|
```
|
|
|
|
## Testing
|
|
|
|
Created `backend/test_shot_deletion_fix.py` to verify the fix works correctly. The test confirms that both `deleted_at` and `deleted_by` fields are now properly set when performing soft deletion operations.
|
|
|
|
## Impact
|
|
|
|
This fix ensures that:
|
|
|
|
1. Soft deletion operations properly set `deleted_by` and `deleted_at` fields
|
|
2. Recovery operations properly clear these fields
|
|
3. Batch operations work correctly
|
|
4. Database transactions are properly committed
|
|
5. Failed operations are properly rolled back
|
|
|
|
## Prevention
|
|
|
|
To prevent similar issues in the future:
|
|
|
|
1. Always pair `db.flush()` with `db.commit()` in router endpoints
|
|
2. Add `db.rollback()` calls in error handling
|
|
3. Consider adding database transaction tests to verify commits work correctly
|
|
4. Review all service methods that modify database state to ensure proper transaction handling |