From c7bd2f722893a00c6e8896236533e489ebb797cc Mon Sep 17 00:00:00 2001 From: sdairs Date: Fri, 28 Aug 2026 10:15:24 +0100 Subject: [PATCH] Fix: cloud service delete on a missing service now errors instead of exiting 0 `instance_delete` silently swallowed a 404 into `Ok(None)`, which `service_delete` rendered as `null`/exit 0 (or "already absent" in human output) instead of surfacing the NOT_FOUND error, misleading scripts into thinking a delete of an already-deleted service succeeded. `CloudClient::delete_service` now propagates the 404 the same way `get_service` does, so `service delete` on a nonexistent ID fails with `Error: NOT_FOUND: ...` and exit code 1, consistent with `service get`. Fixes #601 Co-Authored-By: Claude Fable 5 --- crates/clickhousectl/src/cloud/services.rs | 28 +++--- .../tests/cli_request_shape_test.rs | 85 +++++++++++-------- 2 files changed, 60 insertions(+), 53 deletions(-) diff --git a/crates/clickhousectl/src/cloud/services.rs b/crates/clickhousectl/src/cloud/services.rs index 74c40b34..48b69bf5 100644 --- a/crates/clickhousectl/src/cloud/services.rs +++ b/crates/clickhousectl/src/cloud/services.rs @@ -1451,7 +1451,7 @@ async fn service_delete( } let response = client - .delete_service_if_exists(&org_id, service_id) + .delete_service(&org_id, service_id) .await .map_err(|error| service_delete_error(error, force, service_id))?; cleanup_service_query_key(client, &org_id, service_id, &query_key_ids).await?; @@ -1460,8 +1460,6 @@ async fn service_delete( } if json { println!("{}", serde_json::to_string_pretty(&response)?); - } else if response.is_none() { - println!("Service {} is already absent", service_id); } else { println!("Service {} deletion initiated", service_id); } @@ -2297,22 +2295,20 @@ impl CloudClient { Self::unwrap_response(response) } - pub async fn delete_service_if_exists( + pub async fn delete_service( &self, org_id: &str, service_id: &str, - ) -> crate::cloud::client::Result> { - match self.api().instance_delete(org_id, service_id).await { - Ok(response) => Ok(Some(DeleteResponse { - status: response.status, - request_id: response.request_id, - })), - Err(clickhouse_cloud_api::Error::Api { status: 404, .. }) => { - self.get_organization(org_id).await?; - Ok(None) - } - Err(error) => Err(self.convert_error_for_organization(error, org_id)), - } + ) -> crate::cloud::client::Result { + let response = self + .api() + .instance_delete(org_id, service_id) + .await + .map_err(|error| self.convert_error_for_organization(error, org_id))?; + Ok(DeleteResponse { + status: response.status, + request_id: response.request_id, + }) } pub async fn change_service_state( diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index cdf7bb5b..7cbc0d90 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -861,7 +861,7 @@ async fn service_delete_without_a_stored_query_key_only_deletes_the_service() { } #[tokio::test] -async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { +async fn forced_service_delete_surfaces_not_found_for_an_absent_service() { let mock = MockServer::start().await; Mock::given(method("GET")) .and(path(format!( @@ -874,27 +874,6 @@ async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { .expect(1) .mount(&mock) .await; - Mock::given(method("DELETE")) - .and(path(format!( - "/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}" - ))) - .respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({ - "status": 404, - "error": "NOT_FOUND", - }))) - .expect(1) - .mount(&mock) - .await; - Mock::given(method("GET")) - .and(path("/v1/organizations/org-1")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "result": {}, - "status": 200, - "requestId": "stub-org-get", - }))) - .expect(1) - .mount(&mock) - .await; Mock::given(method("DELETE")) .and(path(format!( "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" @@ -910,9 +889,16 @@ async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { let dir = tempfile::tempdir().unwrap(); write_service_query_key(dir.path(), Some("org-1"), Some(DELETE_TEST_API_KEY_ID)); let output = invoke_service_delete(&mock, dir.path(), true); - assert_success(&output); - assert_eq!(String::from_utf8_lossy(&output.stdout), "null\n"); + assert_eq!(output.status.code(), Some(1)); + assert_eq!(String::from_utf8_lossy(&output.stdout), ""); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "Error: NOT_FOUND: request scoped to organization org-1\n" + ); + // The delete request never succeeded, so local query-key cleanup and the + // organization-scoped key deletion (which would follow a successful + // delete) must not have been attempted. let requests = mock.received_requests().await.unwrap(); let request_shape = requests .iter() @@ -934,13 +920,17 @@ async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { "DELETE".to_string(), format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") ), - ("GET".to_string(), "/v1/organizations/org-1".to_string()), - ( - "DELETE".to_string(), - format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") - ), ] ); + + let stored: Value = serde_json::from_slice( + &std::fs::read(dir.path().join(".clickhouse/credentials.json")).unwrap(), + ) + .unwrap(); + assert_eq!( + stored["service_query_keys"][DELETE_TEST_SERVICE_ID]["api_key_id"], + DELETE_TEST_API_KEY_ID + ); } #[tokio::test] @@ -1183,12 +1173,6 @@ async fn service_delete_does_not_treat_a_missing_organization_as_an_absent_servi .and(path(format!( "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" ))) - .respond_with(not_found.clone()) - .expect(1) - .mount(&mock) - .await; - Mock::given(method("GET")) - .and(path("/v1/organizations/org-1")) .respond_with(not_found) .expect(1) .mount(&mock) @@ -1197,18 +1181,45 @@ async fn service_delete_does_not_treat_a_missing_organization_as_an_absent_servi let dir = tempfile::tempdir().unwrap(); let output = invoke_service_delete(&mock, dir.path(), false); assert_eq!(output.status.code(), Some(1)); + assert_eq!(String::from_utf8_lossy(&output.stdout), ""); assert_eq!( String::from_utf8_lossy(&output.stderr), "Error: NOT_FOUND: request scoped to organization org-1\n" ); let requests = mock.received_requests().await.unwrap(); - assert_eq!(requests.len(), 2); + assert_eq!(requests.len(), 1); assert_eq!( requests[0].url.path(), format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") ); - assert_eq!(requests[1].url.path(), "/v1/organizations/org-1"); +} + +#[tokio::test] +async fn service_delete_preserves_a_detailed_not_found_error() { + let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path("/v1/organizations/org-1/services/missing-service")) + .respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({ + "status": 404, + "error": "Service missing-service was not found", + "requestId": "stub-missing-service", + }))) + .expect(1) + .mount(&mock) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &["service", "delete", "missing-service", "--org-id", "org-1"], + ); + + assert_eq!(output.status.code(), Some(1)); + assert_eq!(String::from_utf8_lossy(&output.stdout), ""); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "Error: Service missing-service was not found\n" + ); } #[tokio::test]