fix: Delete endpoint implementation and error handling

codevalley · codevalley · commit d660d5ad37dc · 2025-03-27T17:51:55.000+05:30
BREAKING CHANGES:
- Added DeletePayloadUseCaseImpl configuration in main.rs
- Fixed missing dependency injection for delete endpoint

Features:
- Improved error handling in Redis repository
- Added detailed logging throughout the application
- Added proper validation for delete operations

Technical Improvements:
- Added EXISTS check before DELETE in Redis
- Added proper type annotations for Redis commands
- Improved error messages and logging context
- Updated plan-progress.md to reflect latest changes

Testing:
- Verified delete endpoint functionality
- Added proper error handling for non-existent payloads
- Improved error response format in API tester
diff --git a/.gitignore b/.gitignore
@@ -30,3 +30,4 @@ Cargo.lock
 dev.db
 *.local
 /tmp/
+.qodo
diff --git a/api-tester.html b/api-tester.html
@@ -53,6 +53,12 @@
         label {
             font-weight: bold;
         }
+        .success {
+            color: #4CAF50;
+        }
+        .error {
+            color: #f44336;
+        }
     </style>
 </head>
 <body>
@@ -138,9 +144,9 @@ <h3>Response:</h3>
                 document.getElementById('createResponse').textContent = JSON.stringify(data, null, 2);
                 
                 // Auto-fill the payload ID for get and delete
-                if (data.id) {
-                    document.getElementById('payloadId').value = data.id;
-                    document.getElementById('deletePayloadId').value = data.id;
+                if (data.hash_id) {
+                    document.getElementById('payloadId').value = data.hash_id;
+                    document.getElementById('deletePayloadId').value = data.hash_id;
                 }
             } catch (error) {
                 document.getElementById('createResponse').textContent = `Error: ${error.message}`;
@@ -176,11 +182,15 @@ <h3>Response:</h3>
                 const response = await fetch(`${API_BASE_URL}/v1/payloads/${payloadId}`, {
                     method: 'DELETE'
                 });
-                
-                const data = await response.json();
-                document.getElementById('deleteResponse').textContent = JSON.stringify(data, null, 2);
+
+                if (response.status === 204) {
+                    document.getElementById('deleteResponse').innerHTML = '<span class="success">Payload deleted successfully</span>';
+                } else {
+                    const text = await response.text();
+                    document.getElementById('deleteResponse').innerHTML = `<span class="error">Error (${response.status}):</span><br><pre>${text}</pre>`;
+                }
             } catch (error) {
-                document.getElementById('deleteResponse').textContent = `Error: ${error.message}`;
+                document.getElementById('deleteResponse').innerHTML = `<span class="error">Error: ${error.message}</span>`;
             }
         });
     </script>
diff --git a/plan-progress.md b/plan-progress.md
@@ -40,7 +40,7 @@
 - [x] Use cases
   - [x] CreatePayload with docs
   - [x] GetPayload with docs
-  - [x] DeletePayload with docs
+  - [x] DeletePayload with docs and proper error handling
 - [x] Rate limiting
   - [x] Redis-based rate limiter implementation
   - [x] Rate limit middleware
@@ -49,16 +49,18 @@
 - [x] Logging setup
   - [x] tracing configuration
   - [x] Request logging middleware
-  - [x] Error logging
+  - [x] Error logging with detailed context
 - [x] Redis repository implementation
   - [x] Connection pool
   - [x] CRUD operations
   - [x] Expiry handling
+  - [x] Improved error handling and logging
 - [x] HTTP API endpoints
   - [x] API versioning structure (v1)
   - [x] Payload size validation
   - [x] Routes and handlers
   - [x] Error handling middleware
+  - [x] Proper status codes (204, 404, etc.)
 
 ### Phase 4: Testing and Documentation 
 - [x] Unit tests
@@ -68,6 +70,7 @@
 - [x] Integration tests
   - [x] API endpoint tests
   - [x] Rate limiting tests
+  - [x] Delete endpoint tests
 - [ ] API documentation
   - [ ] OpenAPI/Swagger specs
   - [ ] Example requests/responses
@@ -90,4 +93,9 @@
   - [ ] Logging configuration
 
 ## Current Status
-🚀 All core functionality is implemented and tested! Create, Get, and Delete payload endpoints are fully functional with proper error handling and rate limiting. Test coverage is comprehensive across all layers. Next: API documentation and deployment configuration.
+🚀 All core functionality is implemented and tested! Create, Get, and Delete payload endpoints are fully functional with proper error handling and rate limiting. Recent improvements include:
+- Fixed delete endpoint implementation with proper error handling
+- Added detailed logging throughout the application
+- Improved Redis repository error handling and validation
+- Fixed configuration issues in main.rs
+Next: API documentation and deployment configuration.
diff --git a/src/api/v1/payload.rs b/src/api/v1/payload.rs
@@ -189,16 +189,25 @@ pub async fn delete_payload(
             HttpResponse::NoContent().finish()
         }
         Err(e) => {
-            error!(error = %e, "Failed to delete payload");
+            error!(error = %e, error_type = ?std::any::type_name_of_val(&e), "Failed to delete payload");
             match e {
-                UseCaseError::RepositoryError(_) => {
-                    HttpResponse::NotFound().json(serde_json::json!({
-                        "error": "Payload not found"
-                    }))
+                UseCaseError::RepositoryError(e) => {
+                    error!(repo_error = %e, "Repository error details");
+                    // Check if the error message indicates the payload was not found
+                    if e.to_string().contains("not found") {
+                        HttpResponse::NotFound().json(serde_json::json!({
+                            "error": "Payload not found"
+                        }))
+                    } else {
+                        HttpResponse::InternalServerError().json(serde_json::json!({
+                            "error": format!("Failed to delete payload: {}", e)
+                        }))
+                    }
                 }
                 _ => {
+                    error!(other_error = %e, "Unexpected error type");
                     HttpResponse::InternalServerError().json(serde_json::json!({
-                        "error": "An unexpected error occurred"
+                        "error": format!("An unexpected error occurred: {}", e)
                     }))
                 }
             }
diff --git a/src/application/use_cases.rs b/src/application/use_cases.rs
@@ -188,8 +188,23 @@ impl DeletePayloadUseCaseImpl {
 impl DeletePayloadUseCase for DeletePayloadUseCaseImpl {
     async fn delete(&self, id: &str) -> Result<(), UseCaseError> {
         let hash_id = HashId::from_string(id.to_string());
-        self.repository.delete(&hash_id).await
-            .map_err(|e| UseCaseError::RepositoryError(e))
+        
+        // First check if the payload exists
+        match self.repository.get(&hash_id).await {
+            Ok(Some(_)) => {
+                // Payload exists, proceed with deletion
+                self.repository.delete(&hash_id).await
+                    .map_err(|e| UseCaseError::RepositoryError(e))
+            }
+            Ok(None) => {
+                // Payload not found
+                Err(UseCaseError::RepositoryError(anyhow::anyhow!("Payload not found")))
+            }
+            Err(e) => {
+                // Repository error
+                Err(UseCaseError::RepositoryError(e))
+            }
+        }
     }
 }
 
diff --git a/src/infrastructure/redis/mod.rs b/src/infrastructure/redis/mod.rs
@@ -8,6 +8,7 @@ use redis;
 use serde_json;
 use thiserror::Error;
 use async_trait::async_trait;
+use tracing::{debug, error, info};
 
 use crate::{
     application::repository::Repository,
@@ -124,28 +125,60 @@ impl Repository for RedisRepository {
 
         let json: Option<String> = redis::cmd("GET")
             .arg(&key)
-            .query_async(&mut conn)
-            .await?;
+            .query_async::<_, Option<String>>(&mut conn)
+            .await
+            .map_err(|e| anyhow::anyhow!("Failed to get payload: {}", e))?;
 
         match json {
             Some(json) => {
-                let payload: Payload = serde_json::from_str(&json)?;
+                let payload: Payload = serde_json::from_str(&json)
+                    .map_err(|e| anyhow::anyhow!("Failed to deserialize payload: {}", e))?;
                 Ok(Some(payload))
             }
             None => Ok(None),
         }
     }
 
     async fn delete(&self, hash_id: &HashId) -> Result<(), anyhow::Error> {
-        let mut conn = self.get_conn().await?;
+        let mut conn = match self.get_conn().await {
+            Ok(conn) => conn,
+            Err(e) => {
+                error!(error = %e, "Failed to get Redis connection");
+                return Err(anyhow::anyhow!("Failed to get Redis connection: {}", e));
+            }
+        };
         let key = Self::payload_key(hash_id);
         
-        let _: () = redis::cmd("DEL")
+        // Check if key exists first
+        let exists: bool = match redis::cmd("EXISTS")
             .arg(&key)
-            .query_async(&mut conn)
-            .await?;
+            .query_async::<_, bool>(&mut conn)
+            .await {
+                Ok(exists) => exists,
+                Err(e) => {
+                    error!(error = %e, key = %key, "Failed to check if key exists in Redis");
+                    return Err(anyhow::anyhow!("Failed to check if payload exists: {}", e));
+                }
+            };
 
-        Ok(())
+        if !exists {
+            debug!(key = %key, "Key not found in Redis");
+            return Err(anyhow::anyhow!("Payload not found"));
+        }
+        
+        match redis::cmd("DEL")
+            .arg(&key)
+            .query_async::<_, ()>(&mut conn)
+            .await {
+                Ok(_) => {
+                    info!(key = %key, "Successfully deleted key from Redis");
+                    Ok(())
+                }
+                Err(e) => {
+                    error!(error = %e, key = %key, "Failed to delete key from Redis");
+                    Err(anyhow::anyhow!("Failed to delete payload: {}", e))
+                }
+            }
     }
 }
 
diff --git a/src/main.rs b/src/main.rs
@@ -10,6 +10,7 @@ use jump::{
         use_cases::{
             CreatePayloadUseCaseImpl,
             GetPayloadUseCaseImpl,
+            DeletePayloadUseCaseImpl,
         },
     },
     infrastructure::{
@@ -52,6 +53,7 @@ async fn main() -> std::io::Result<()> {
     // Create use cases
     let create_payload_use_case = Arc::new(CreatePayloadUseCaseImpl::new(repository.clone()));
     let get_payload_use_case = Arc::new(GetPayloadUseCaseImpl::new(repository.clone()));
+    let delete_payload_use_case = Arc::new(DeletePayloadUseCaseImpl::new(repository.clone()));
     
     // Configure rate limiter
     let rate_limit_config = RateLimitConfig {
@@ -81,6 +83,7 @@ async fn main() -> std::io::Result<()> {
             // Add application state
             .app_data(web::Data::new(create_payload_use_case.clone()))
             .app_data(web::Data::new(get_payload_use_case.clone()))
+            .app_data(web::Data::new(delete_payload_use_case.clone()))
             // Add API routes
             .configure(api::configure())
     })
diff --git a/test-api.ps1 b/test-api.ps1

-Original file line number
+Diff line change
 dev.db
 *.local
 /tmp/
 +.qodo