Better Errors (#150)

* bump * error with an api status code * remove redundant import * fix ApiError * fix ApiError by converting StatusCode to u16 * revert * u16 as StatusCode * update tests * improve tests readability * fix: ci tests & add docker-compose local test harness --------- Co-authored-by: Gero Gerke <[email protected]>
influxdb-rs · Apr 4, 2024 · 9d8d453 · 9d8d453
1 parent bfe457c
commit 9d8d453
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 36 deletions.
diff --git a/docker-compose.integrationtest.yaml b/docker-compose.integrationtest.yaml
@@ -0,0 +1,27 @@
+version: '3'
+services:
+  influxdb:
+      image: influxdb:1.8
+      ports:
+        - 8086:8086
+  authed_influxdb:
+    image: influxdb:1.8
+    ports:
+      - 9086:8086
+    environment:
+      INFLUXDB_HTTP_AUTH_ENABLED: true
+      INFLUXDB_ADMIN_USER: admin
+      INFLUXDB_ADMIN_PASSWORD: password
+      INFLUXDB_USER: nopriv_user
+      INFLUXDB_USER_PASSWORD: password
+  influxdbv2:
+    image: influxdb:2.6
+    ports:
+      - 2086:8086
+    environment:
+      DOCKER_INFLUXDB_INIT_MODE: setup
+      DOCKER_INFLUXDB_INIT_USERNAME: admin
+      DOCKER_INFLUXDB_INIT_PASSWORD: password
+      DOCKER_INFLUXDB_INIT_ORG: testing
+      DOCKER_INFLUXDB_INIT_BUCKET: mydb
+      DOCKER_INFLUXDB_INIT_ADMIN_TOKEN: admintoken
diff --git a/influxdb/src/client/mod.rs b/influxdb/src/client/mod.rs
@@ -16,7 +16,6 @@
 //! ```
 
 use futures_util::TryFutureExt;
-use http::StatusCode;
 #[cfg(feature = "reqwest")]
 use reqwest::{Client as HttpClient, RequestBuilder, Response as HttpResponse};
 use std::collections::{BTreeMap, HashMap};
@@ -281,7 +280,7 @@ impl Client {
         })?;
 
         // todo: improve error parsing without serde
-        if s.contains("\"error\"") {
+        if s.contains("\"error\"") || s.contains("\"Error\"") {
             return Err(Error::DatabaseError {
                 error: format!("influxdb error: \"{}\"", s),
             });
@@ -301,13 +300,10 @@ impl Client {
 
 pub(crate) fn check_status(res: &HttpResponse) -> Result<(), Error> {
     let status = res.status();
-    if status == StatusCode::UNAUTHORIZED.as_u16() {
-        Err(Error::AuthorizationError)
-    } else if status == StatusCode::FORBIDDEN.as_u16() {
-        Err(Error::AuthenticationError)
-    } else {
-        Ok(())
+    if !status.is_success() {
+        return Err(Error::ApiError(status.into()));
     }
+    Ok(())
 }
 
 #[cfg(test)]

diff --git a/influxdb/src/error.rs b/influxdb/src/error.rs
@@ -25,13 +25,9 @@ pub enum Error {
     /// Error which has happened inside InfluxDB
     DatabaseError { error: String },
 
-    #[error("authentication error. No or incorrect credentials")]
-    /// Error happens when no or incorrect credentials are used. `HTTP 401 Unauthorized`
-    AuthenticationError,
-
-    #[error("authorization error. User not authorized")]
-    /// Error happens when the supplied user is not authorized. `HTTP 403 Forbidden`
-    AuthorizationError,
+    #[error("API error with a status code: {0}")]
+    /// Error happens when API returns non 2xx status code.
+    ApiError(u16),
 
     #[error("connection error: {error}")]
     /// Error happens when HTTP request fails

diff --git a/influxdb/tests/integration_tests.rs b/influxdb/tests/integration_tests.rs
@@ -120,6 +120,8 @@ async fn test_authed_write_and_read() {
 #[async_std::test]
 #[cfg(not(tarpaulin_include))]
 async fn test_wrong_authed_write_and_read() {
+    use http::StatusCode;
+
     const TEST_NAME: &str = "test_wrong_authed_write_and_read";
 
     run_test(
@@ -140,9 +142,9 @@ async fn test_wrong_authed_write_and_read() {
             let write_result = client.query(write_query).await;
             assert_result_err(&write_result);
             match write_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     write_result.unwrap_err()
                 ),
             }
@@ -151,9 +153,9 @@ async fn test_wrong_authed_write_and_read() {
             let read_result = client.query(read_query).await;
             assert_result_err(&read_result);
             match read_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     read_result.unwrap_err()
                 ),
             }
@@ -164,9 +166,9 @@ async fn test_wrong_authed_write_and_read() {
             let read_result = client.query(read_query).await;
             assert_result_err(&read_result);
             match read_result {
-                Err(Error::AuthenticationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::FORBIDDEN.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthenticationError: {}",
+                    "Should be an ApiError(UNAUTHENTICATED): {}",
                     read_result.unwrap_err()
                 ),
             }
@@ -190,6 +192,8 @@ async fn test_wrong_authed_write_and_read() {
 #[async_std::test]
 #[cfg(not(tarpaulin_include))]
 async fn test_non_authed_write_and_read() {
+    use http::StatusCode;
+
     const TEST_NAME: &str = "test_non_authed_write_and_read";
 
     run_test(
@@ -208,9 +212,9 @@ async fn test_non_authed_write_and_read() {
             let write_result = non_authed_client.query(write_query).await;
             assert_result_err(&write_result);
             match write_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     write_result.unwrap_err()
                 ),
             }
@@ -220,9 +224,9 @@ async fn test_non_authed_write_and_read() {
 
             assert_result_err(&read_result);
             match read_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     read_result.unwrap_err()
                 ),
             }
@@ -280,6 +284,8 @@ async fn test_write_and_read_field() {
 #[cfg(feature = "serde")]
 #[cfg(not(tarpaulin_include))]
 async fn test_json_non_authed_read() {
+    use http::StatusCode;
+
     const TEST_NAME: &str = "test_json_non_authed_read";
 
     run_test(
@@ -297,9 +303,9 @@ async fn test_json_non_authed_read() {
             let read_result = non_authed_client.json_query(read_query).await;
             assert_result_err(&read_result);
             match read_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be a AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     read_result.unwrap_err()
                 ),
             }

diff --git a/influxdb/tests/integration_tests_v2.rs b/influxdb/tests/integration_tests_v2.rs
@@ -33,7 +33,7 @@ async fn test_authed_write_and_read() {
         },
         || async move {
             let client = Client::new("http://127.0.0.1:2086", "mydb").with_token("admintoken");
-            let read_query = ReadQuery::new("DELETE MEASUREMENT weather");
+            let read_query = ReadQuery::new("DROP MEASUREMENT \"weather\"");
             let read_result = client.query(read_query).await;
             assert_result_ok(&read_result);
             assert!(!read_result.unwrap().contains("error"), "Teardown failed");
@@ -48,6 +48,8 @@ async fn test_authed_write_and_read() {
 #[async_std::test]
 #[cfg(not(tarpaulin))]
 async fn test_wrong_authed_write_and_read() {
+    use http::StatusCode;
+
     run_test(
         || async move {
             let client = Client::new("http://127.0.0.1:2086", "mydb").with_token("falsetoken");
@@ -57,9 +59,9 @@ async fn test_wrong_authed_write_and_read() {
             let write_result = client.query(&write_query).await;
             assert_result_err(&write_result);
             match write_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     write_result.unwrap_err()
                 ),
             }
@@ -68,9 +70,9 @@ async fn test_wrong_authed_write_and_read() {
             let read_result = client.query(&read_query).await;
             assert_result_err(&read_result);
             match read_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     read_result.unwrap_err()
                 ),
             }
@@ -86,6 +88,8 @@ async fn test_wrong_authed_write_and_read() {
 #[async_std::test]
 #[cfg(not(tarpaulin))]
 async fn test_non_authed_write_and_read() {
+    use http::StatusCode;
+
     run_test(
         || async move {
             let non_authed_client = Client::new("http://127.0.0.1:2086", "mydb");
@@ -95,9 +99,9 @@ async fn test_non_authed_write_and_read() {
             let write_result = non_authed_client.query(&write_query).await;
             assert_result_err(&write_result);
             match write_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     write_result.unwrap_err()
                 ),
             }
@@ -106,9 +110,9 @@ async fn test_non_authed_write_and_read() {
             let read_result = non_authed_client.query(&read_query).await;
             assert_result_err(&read_result);
             match read_result {
-                Err(Error::AuthorizationError) => {}
+                Err(Error::ApiError(code)) if code == StatusCode::UNAUTHORIZED.as_u16() => {}
                 _ => panic!(
-                    "Should be an AuthorizationError: {}",
+                    "Should be an ApiError(UNAUTHORIZED): {}",
                     read_result.unwrap_err()
                 ),
             }