From ff8e0379713a0ac93679976ecd9bd02c36b31ba0 Mon Sep 17 00:00:00 2001 From: Nga Tran Date: Mon, 4 Oct 2021 20:37:53 -0400 Subject: [PATCH 1/2] feat: handle delete response --- .../protos/influxdata/iox/management/v1/service.proto | 1 - influxdb_iox_client/src/client/management.rs | 1 - predicate/src/delete_predicate.rs | 1 - src/influxdb_ioxd/rpc/error.rs | 6 ++++++ src/influxdb_ioxd/rpc/management.rs | 4 ++-- tests/end_to_end_cases/management_api.rs | 6 ++++-- 6 files changed, 12 insertions(+), 7 deletions(-) diff --git a/generated_types/protos/influxdata/iox/management/v1/service.proto b/generated_types/protos/influxdata/iox/management/v1/service.proto index 5793bcd6e0..84fb5bed3e 100644 --- a/generated_types/protos/influxdata/iox/management/v1/service.proto +++ b/generated_types/protos/influxdata/iox/management/v1/service.proto @@ -494,5 +494,4 @@ message DeleteRequest { } message DeleteResponse { - // NGA todo: define an appropriate response } diff --git a/influxdb_iox_client/src/client/management.rs b/influxdb_iox_client/src/client/management.rs index b47e9e2c37..67cc361c5a 100644 --- a/influxdb_iox_client/src/client/management.rs +++ b/influxdb_iox_client/src/client/management.rs @@ -1057,7 +1057,6 @@ impl Client { _ => DeleteError::ServerError(status), })?; - // NGA todo: return a handle to the delete? Ok(()) } diff --git a/predicate/src/delete_predicate.rs b/predicate/src/delete_predicate.rs index 62224f4097..e8da83dd39 100644 --- a/predicate/src/delete_predicate.rs +++ b/predicate/src/delete_predicate.rs @@ -705,7 +705,6 @@ mod tests { assert_eq!(expected, result); } - // NGA todo: check content of error messages #[test] fn test_parse_delete_negative() { // invalid key diff --git a/src/influxdb_ioxd/rpc/error.rs b/src/influxdb_ioxd/rpc/error.rs index 442c2885fb..4fb78f0e6b 100644 --- a/src/influxdb_ioxd/rpc/error.rs +++ b/src/influxdb_ioxd/rpc/error.rs @@ -177,6 +177,12 @@ pub fn default_db_error_handler(error: server::db::Error) -> tonic::Status { ), } .into(), + Error::DeleteFromTable { source, table_name } => PreconditionViolation { + category: "database".to_string(), + subject: "influxdata.com/iox".to_string(), + description: format!("Cannot delete data from table: {} : {}", table_name, source), + } + .into(), Error::CatalogError { source } => default_catalog_error_handler(source), error => { error!(?error, "Unexpected error"); diff --git a/src/influxdb_ioxd/rpc/management.rs b/src/influxdb_ioxd/rpc/management.rs index 6a5fa707dd..a60b791286 100644 --- a/src/influxdb_ioxd/rpc/management.rs +++ b/src/influxdb_ioxd/rpc/management.rs @@ -623,6 +623,7 @@ where .db(&db_name) .map_err(default_server_error_handler)?; + // Parse input strings and build delete predicate let del_predicate_result = ParseDeletePredicate::build_delete_predicate( start_time.clone(), stop_time.clone(), @@ -637,14 +638,13 @@ where })) } Ok(del_predicate) => { - //execute delete + // execute delete db.delete(&table_name, Arc::new(del_predicate)) .await .map_err(default_db_error_handler)?; } } - // NGA todo: return a delete handle with the response? Ok(Response::new(DeleteResponse {})) } } diff --git a/tests/end_to_end_cases/management_api.rs b/tests/end_to_end_cases/management_api.rs index 2f31a2f62d..eaeb6ff923 100644 --- a/tests/end_to_end_cases/management_api.rs +++ b/tests/end_to_end_cases/management_api.rs @@ -1587,8 +1587,10 @@ async fn test_delete() { let pred = "region = west"; let del = management_client .delete(db_name.clone(), table, start, stop, pred) - .await; - assert!(del.is_err()); + .await + .unwrap_err() + .to_string(); + assert!(del.contains("Cannot delete data from table")); // Verify both existing tables still have the same data // query to verify data deleted From 80c4ba959dff4533e8f32bde6853fb41121accea Mon Sep 17 00:00:00 2001 From: Nga Tran Date: Mon, 4 Oct 2021 21:19:10 -0400 Subject: [PATCH 2/2] test: add negative tests for delete hhtp endpoints --- src/influxdb_ioxd/http.rs | 42 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/influxdb_ioxd/http.rs b/src/influxdb_ioxd/http.rs index d56fdc463e..c0d532e60d 100644 --- a/src/influxdb_ioxd/http.rs +++ b/src/influxdb_ioxd/http.rs @@ -282,7 +282,7 @@ impl ApplicationError { Self::ParsingLineProtocol { .. } => self.bad_request(), Self::ParsingDelete { .. } => self.bad_request(), Self::BuildingDeletePredicate { .. } => self.bad_request(), - Self::ExecutingDelete { .. } => self.internal_error(), + Self::ExecutingDelete { .. } => self.bad_request(), Self::ReadingBodyAsGzip { .. } => self.bad_request(), Self::ClientHangup { .. } => self.bad_request(), Self::RouteNotFound { .. } => self.not_found(), @@ -1150,6 +1150,44 @@ mod tests { "+----------------+--------------+-------+-----------------+----------------------+", ]; assert_batches_eq!(expected, &batches); + + // ------------------- + // negative tests + // Not able to parse _measurement="not_a_table" (it must be _measurement=\"not_a_table\" to work) + let delete_line = r#"{"start":"2021-04-01T14:00:00Z","stop":"2021-04-02T14:00:00Z", "predicate":"_measurement="not_a_table" and location=Boston"}"#; + let response = client + .post(&format!( + "{}/api/v2/delete?bucket={}&org={}", + server_url, bucket_name, org_name + )) + .body(delete_line) + .send() + .await; + check_response( + "delete", + response, + StatusCode::BAD_REQUEST, + Some("Unable to parse delete string"), + ) + .await; + + // delete from non-existing table + let delete_line = r#"{"start":"2021-04-01T14:00:00Z","stop":"2021-04-02T14:00:00Z", "predicate":"_measurement=not_a_table and location=Boston"}"#; + let response = client + .post(&format!( + "{}/api/v2/delete?bucket={}&org={}", + server_url, bucket_name, org_name + )) + .body(delete_line) + .send() + .await; + check_response( + "delete", + response, + StatusCode::BAD_REQUEST, + Some("Cannot delete data from non-existing table"), + ) + .await; } #[tokio::test] @@ -1607,7 +1645,7 @@ mod tests { assert_eq!(status, expected_status); if let Some(expected_body) = expected_body { - assert_eq!(body, expected_body); + assert!(body.contains(expected_body)); } } else { panic!("Unexpected error response: {:?}", response);