From 13cbabd52848739924bbbd2192e96c57bc33db0f Mon Sep 17 00:00:00 2001 From: Subhra264 Date: Mon, 26 Aug 2024 11:57:10 +0530 Subject: [PATCH] fix: use 409 when deleting destination/templates already in use (#4294) Fixes #4291 ## Summary by CodeRabbit - **New Features** - Introduced a new method for handling 409 Conflict responses in the HTTP responses, improving error messaging clarity. - **Bug Fixes** - Updated error handling to return conflict responses instead of forbidden responses when certain delete operations fail due to resource conflicts. - Enhanced clarity in user membership responses by changing the error message when a user is already part of an organization. These changes improve user experience by providing clearer and more accurate feedback on error conditions related to resource management. --- src/common/meta/http.rs | 7 +++++++ src/handler/http/request/alerts/destinations.rs | 4 ++-- src/handler/http/request/alerts/templates.rs | 4 ++-- src/handler/http/request/dashboards/reports.rs | 2 +- src/service/alerts/destinations.rs | 2 +- src/service/alerts/templates.rs | 2 +- src/service/users.rs | 6 +++--- 7 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/common/meta/http.rs b/src/common/meta/http.rs index d647131ae7..944d14adaf 100644 --- a/src/common/meta/http.rs +++ b/src/common/meta/http.rs @@ -102,6 +102,13 @@ impl HttpResponse { .json(Self::error(StatusCode::FORBIDDEN.into(), error.to_string())) } + /// Send a Forbidden response in json format and associate the + /// provided error as `error` field. + pub fn conflict(error: impl ToString) -> ActixHttpResponse { + ActixHttpResponse::Conflict() + .json(Self::error(StatusCode::CONFLICT.into(), error.to_string())) + } + /// Send a NotFound response in json format and associate the /// provided error as `error` field. pub fn not_found(error: impl ToString) -> ActixHttpResponse { diff --git a/src/handler/http/request/alerts/destinations.rs b/src/handler/http/request/alerts/destinations.rs index e1ed0a399b..e3dffd60bb 100644 --- a/src/handler/http/request/alerts/destinations.rs +++ b/src/handler/http/request/alerts/destinations.rs @@ -183,7 +183,7 @@ async fn list_destinations( ), responses( (status = 200, description = "Success", content_type = "application/json", body = HttpResponse), - (status = 403, description = "Forbidden", content_type = "application/json", body = HttpResponse), + (status = 409, description = "Conflict", content_type = "application/json", body = HttpResponse), (status = 404, description = "NotFound", content_type = "application/json", body = HttpResponse), (status = 500, description = "Failure", content_type = "application/json", body = HttpResponse), ) @@ -194,7 +194,7 @@ async fn delete_destination(path: web::Path<(String, String)>) -> Result Ok(MetaHttpResponse::ok("Alert destination deleted")), Err(e) => match e { - (http::StatusCode::FORBIDDEN, e) => Ok(MetaHttpResponse::forbidden(e)), + (http::StatusCode::CONFLICT, e) => Ok(MetaHttpResponse::conflict(e)), (http::StatusCode::NOT_FOUND, e) => Ok(MetaHttpResponse::not_found(e)), (_, e) => Ok(MetaHttpResponse::internal_error(e)), }, diff --git a/src/handler/http/request/alerts/templates.rs b/src/handler/http/request/alerts/templates.rs index ecb27bfa6d..9efe9ab4f8 100644 --- a/src/handler/http/request/alerts/templates.rs +++ b/src/handler/http/request/alerts/templates.rs @@ -174,7 +174,7 @@ async fn list_templates(path: web::Path, _req: HttpRequest) -> Result) -> Result Ok(MetaHttpResponse::ok("Alert template deleted")), Err(e) => match e { - (http::StatusCode::FORBIDDEN, e) => Ok(MetaHttpResponse::forbidden(e)), + (http::StatusCode::CONFLICT, e) => Ok(MetaHttpResponse::conflict(e)), (http::StatusCode::NOT_FOUND, e) => Ok(MetaHttpResponse::not_found(e)), (_, e) => Ok(MetaHttpResponse::internal_error(e)), }, diff --git a/src/handler/http/request/dashboards/reports.rs b/src/handler/http/request/dashboards/reports.rs index ea81f7b4e3..2540b55e23 100644 --- a/src/handler/http/request/dashboards/reports.rs +++ b/src/handler/http/request/dashboards/reports.rs @@ -202,7 +202,7 @@ async fn delete_report(path: web::Path<(String, String)>) -> Result Ok(MetaHttpResponse::ok("Report deleted")), Err(e) => match e { - (http::StatusCode::FORBIDDEN, e) => Ok(MetaHttpResponse::forbidden(e)), + (http::StatusCode::CONFLICT, e) => Ok(MetaHttpResponse::conflict(e)), (http::StatusCode::NOT_FOUND, e) => Ok(MetaHttpResponse::not_found(e)), (_, e) => Ok(MetaHttpResponse::internal_error(e)), }, diff --git a/src/service/alerts/destinations.rs b/src/service/alerts/destinations.rs index 486f08687c..d60e277370 100644 --- a/src/service/alerts/destinations.rs +++ b/src/service/alerts/destinations.rs @@ -173,7 +173,7 @@ pub async fn delete(org_id: &str, name: &str) -> Result<(), (http::StatusCode, a for alert in alerts.iter() { if stream_key.starts_with(org_id) && alert.destinations.contains(&name.to_string()) { return Err(( - http::StatusCode::FORBIDDEN, + http::StatusCode::CONFLICT, anyhow::anyhow!("Alert destination is in use for alert {}", alert.name), )); } diff --git a/src/service/alerts/templates.rs b/src/service/alerts/templates.rs index 61dfa9898d..ec4d3b624e 100644 --- a/src/service/alerts/templates.rs +++ b/src/service/alerts/templates.rs @@ -111,7 +111,7 @@ pub async fn delete(org_id: &str, name: &str) -> Result<(), (http::StatusCode, a for dest in ALERTS_DESTINATIONS.iter() { if dest.key().starts_with(org_id) && dest.value().template.eq(&name) { return Err(( - http::StatusCode::FORBIDDEN, + http::StatusCode::CONFLICT, anyhow::anyhow!( "Alert template is in use for destination {}", &dest.value().name.clone() diff --git a/src/service/users.rs b/src/service/users.rs index 52cdecb46b..d02cbc5195 100644 --- a/src/service/users.rs +++ b/src/service/users.rs @@ -397,9 +397,9 @@ pub async fn add_user_to_org( for org in orgs.iter() { if org.name.eq(org_id) { // External user is already part of this org - return Ok(HttpResponse::Forbidden().json(MetaHttpResponse::error( - http::StatusCode::FORBIDDEN.into(), - "Not Allowed".to_string(), + return Ok(HttpResponse::Conflict().json(MetaHttpResponse::error( + http::StatusCode::CONFLICT.into(), + "User is already part of the org".to_string(), ))); } }