[pve-devel] [PATCH v2 proxmox{, -perl-rs} 0/4] move `HttpError` from `proxmox-router` into its own crate

[pve-devel] [PATCH v2 proxmox{, -perl-rs} 0/4] move `HttpError` from `proxmox-router` into its own crate

[Lukas Wagner]

This patch series moves `HttpError` from `proxmox-router` into its own little 
crate. The main benefit of this is that one can now use `HttpError`
independently (e.g. for `proxmox-notify`)

Changes since v1:
  - Re-export HttpError from proxmox-router instead of changing all
    existing users.
  - Drop unused 'derive' features for the serde dependency



proxmox:

Lukas Wagner (3):
  http-error: add new http-error crate
  router: re-export `HttpError` from `proxmox-http-error`
  notify: use HttpError from proxmox-http-error

 Cargo.toml                         |   2 +
 proxmox-http-error/Cargo.toml      |  16 ++++
 proxmox-http-error/src/lib.rs      |  81 ++++++++++++++++++
 proxmox-notify/Cargo.toml          |   1 +
 proxmox-notify/src/api/common.rs   |  33 ++++----
 proxmox-notify/src/api/filter.rs   |  59 ++++++-------
 proxmox-notify/src/api/gotify.rs   |  85 ++++++++++---------
 proxmox-notify/src/api/group.rs    |  82 +++++++++---------
 proxmox-notify/src/api/mod.rs      | 130 ++++++++++-------------------
 proxmox-notify/src/api/sendmail.rs |  93 ++++++++++++---------
 proxmox-router/Cargo.toml          |   1 +
 proxmox-router/src/error.rs        |  42 +---------
 proxmox-router/src/lib.rs          |   2 +-
 13 files changed, 334 insertions(+), 293 deletions(-)
 create mode 100644 proxmox-http-error/Cargo.toml
 create mode 100644 proxmox-http-error/src/lib.rs


proxmox-perl-rs:

Lukas Wagner (1):
  notify: use new HttpError type

 common/src/notify.rs | 77 +++++++++++++++++++++++---------------------
 pve-rs/Cargo.toml    |  1 +
 2 files changed, 42 insertions(+), 36 deletions(-)


Summary over all repositories:
  15 files changed, 376 insertions(+), 329 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH v2 proxmox 1/4] http-error: add new http-error crate

[Lukas Wagner]

Break out proxmox-router's HttpError into it's own crate so that it can
be used without pulling in proxmox-router.

This commit also implements `Serialize` for `HttpError` so that it can
be returned from perlmod bindings, allowing Perl code to access the
status code as well as the message.

Also add some smoke-tests to make sure that the `http_bail` and
`http_err` macros actually produce valid code.

Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 Cargo.toml                    |  2 +
 proxmox-http-error/Cargo.toml | 16 +++++++
 proxmox-http-error/src/lib.rs | 81 +++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 proxmox-http-error/Cargo.toml
 create mode 100644 proxmox-http-error/src/lib.rs

diff --git a/Cargo.toml b/Cargo.toml
index 54be5f6..cb82253 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,6 +7,7 @@ members = [
     "proxmox-borrow",
     "proxmox-compression",
     "proxmox-http",
+    "proxmox-http-error",
     "proxmox-human-byte",
     "proxmox-io",
     "proxmox-lang",
@@ -88,6 +89,7 @@ proxmox-api-macro = { version = "1.0.4", path = "proxmox-api-macro" }
 proxmox-async = { version = "0.4.1", path = "proxmox-async" }
 proxmox-compression = { version = "0.2.0", path = "proxmox-compression" }
 proxmox-http = { version = "0.9.0", path = "proxmox-http" }
+proxmox-http-error = { version = "0.1.0", path = "proxmox-http-error" }
 proxmox-human-byte = { version = "0.1.0", path = "proxmox-human-byte" }
 proxmox-io = { version = "1.0.0", path = "proxmox-io" }
 proxmox-lang = { version = "1.1", path = "proxmox-lang" }
diff --git a/proxmox-http-error/Cargo.toml b/proxmox-http-error/Cargo.toml
new file mode 100644
index 0000000..6f54bfc
--- /dev/null
+++ b/proxmox-http-error/Cargo.toml
@@ -0,0 +1,16 @@
+[package]
+name = "proxmox-http-error"
+version = "0.1.0"
+
+edition.workspace = true
+authors.workspace = true
+license.workspace = true
+repository.workspace = true
+description = "Proxmox HTTP Error"
+
+exclude.workspace = true
+
+[dependencies]
+anyhow.workspace = true
+http.workspace = true
+serde.workspace = true
diff --git a/proxmox-http-error/src/lib.rs b/proxmox-http-error/src/lib.rs
new file mode 100644
index 0000000..de45cb9
--- /dev/null
+++ b/proxmox-http-error/src/lib.rs
@@ -0,0 +1,81 @@
+use serde::{ser::SerializeStruct, Serialize, Serializer};
+use std::fmt;
+
+#[doc(hidden)]
+pub use http::StatusCode;
+
+/// HTTP error including `StatusCode` and message.
+#[derive(Debug)]
+pub struct HttpError {
+    pub code: StatusCode,
+    pub message: String,
+}
+
+impl std::error::Error for HttpError {}
+
+impl HttpError {
+    pub fn new(code: StatusCode, message: String) -> Self {
+        HttpError { code, message }
+    }
+}
+
+impl fmt::Display for HttpError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.message)
+    }
+}
+
+impl Serialize for HttpError {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        let mut state = serializer.serialize_struct("HttpError", 2)?;
+        state.serialize_field("code", &self.code.as_u16())?;
+        state.serialize_field("message", &self.message)?;
+        state.end()
+    }
+}
+
+/// Macro to create a HttpError inside a anyhow::Error
+#[macro_export]
+macro_rules! http_err {
+    ($status:ident, $($fmt:tt)+) => {{
+        ::anyhow::Error::from($crate::HttpError::new(
+            $crate::StatusCode::$status,
+            format!($($fmt)+)
+        ))
+    }};
+}
+
+/// Bail with an error generated with the `http_err!` macro.
+#[macro_export]
+macro_rules! http_bail {
+    ($status:ident, $($fmt:tt)+) => {{
+        return Err($crate::http_err!($status, $($fmt)+));
+    }};
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_http_err() {
+        // Make sure the macro generates valid code.
+        http_err!(IM_A_TEAPOT, "Cannot brew coffee");
+    }
+
+    #[test]
+    fn test_http_bail() {
+        fn t() -> Result<(), anyhow::Error> {
+            // Make sure the macro generates valid code.
+            http_bail!(
+                UNAVAILABLE_FOR_LEGAL_REASONS,
+                "Nothing to see here, move along"
+            );
+        }
+
+        assert!(t().is_err());
+    }
+}
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox 2/4] router: re-export `HttpError` from `proxmox-http-error`

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-router/Cargo.toml   |  1 +
 proxmox-router/src/error.rs | 42 +------------------------------------
 proxmox-router/src/lib.rs   |  2 +-
 3 files changed, 3 insertions(+), 42 deletions(-)

diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
index 34a1985..7d4d32f 100644
--- a/proxmox-router/Cargo.toml
+++ b/proxmox-router/Cargo.toml
@@ -26,6 +26,7 @@ rustyline = { version = "9", optional = true }
 libc = { workspace = true, optional = true }
 
 proxmox-lang.workspace = true
+proxmox-http-error.workspace = true
 proxmox-schema.workspace = true
 proxmox-async.workspace = true
 
diff --git a/proxmox-router/src/error.rs b/proxmox-router/src/error.rs
index e285cf7..15c5417 100644
--- a/proxmox-router/src/error.rs
+++ b/proxmox-router/src/error.rs
@@ -1,44 +1,4 @@
-use std::fmt;
+pub use proxmox_http_error::{http_bail, http_err, HttpError};
 
 #[doc(hidden)]
 pub use http::StatusCode;
-
-/// HTTP error including `StatusCode` and message.
-#[derive(Debug)]
-pub struct HttpError {
-    pub code: StatusCode,
-    pub message: String,
-}
-
-impl std::error::Error for HttpError {}
-
-impl HttpError {
-    pub fn new(code: StatusCode, message: String) -> Self {
-        HttpError { code, message }
-    }
-}
-
-impl fmt::Display for HttpError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}", self.message)
-    }
-}
-
-/// Macro to create a HttpError inside a anyhow::Error
-#[macro_export]
-macro_rules! http_err {
-    ($status:ident, $($fmt:tt)+) => {{
-        ::anyhow::Error::from($crate::HttpError::new(
-            $crate::error::StatusCode::$status,
-            format!($($fmt)+)
-        ))
-    }};
-}
-
-/// Bail with an error generated with the `http_err!` macro.
-#[macro_export]
-macro_rules! http_bail {
-    ($status:ident, $($fmt:tt)+) => {{
-        return Err($crate::http_err!($status, $($fmt)+));
-    }};
-}
diff --git a/proxmox-router/src/lib.rs b/proxmox-router/src/lib.rs
index 7f4f2d9..808836a 100644
--- a/proxmox-router/src/lib.rs
+++ b/proxmox-router/src/lib.rs
@@ -17,7 +17,7 @@ mod serializable_return;
 
 #[doc(inline)]
 #[cfg(feature = "server")]
-pub use error::HttpError;
+pub use error::*;
 
 pub use permission::*;
 pub use router::*;
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox 3/4] notify: use HttpError from proxmox-http-error

[Lukas Wagner]

Also improve API documentation in terms of which HttpError is
returned when.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Assumes that the following two patches have been applied before,
    otherwise there will be a conflict (which is trivial to resolve,
    though)
    
    1: "notify: fix build warnings if not all features are enabled":
        https://lists.proxmox.com/pipermail/pve-devel/2023-July/058409.html
    
    2: "notify: fix tests if not all features are enabled"
        https://lists.proxmox.com/pipermail/pve-devel/2023-July/058410.html

 proxmox-notify/Cargo.toml          |   1 +
 proxmox-notify/src/api/common.rs   |  33 ++++----
 proxmox-notify/src/api/filter.rs   |  59 ++++++-------
 proxmox-notify/src/api/gotify.rs   |  85 ++++++++++---------
 proxmox-notify/src/api/group.rs    |  82 +++++++++---------
 proxmox-notify/src/api/mod.rs      | 130 ++++++++++-------------------
 proxmox-notify/src/api/sendmail.rs |  93 ++++++++++++---------
 7 files changed, 232 insertions(+), 251 deletions(-)

diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 5cceb0b..8b36a52 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -14,6 +14,7 @@ log.workspace = true
 once_cell.workspace = true
 openssl.workspace = true
 proxmox-http = { workspace = true, features = ["client-sync"], optional = true }
+proxmox-http-error.workspace = true
 proxmox-human-byte.workspace = true
 proxmox-schema = { workspace = true, features = ["api-macro", "api-types"]}
 proxmox-section-config = { workspace = true }
diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs
index 48761fb..bc7029f 100644
--- a/proxmox-notify/src/api/common.rs
+++ b/proxmox-notify/src/api/common.rs
@@ -1,15 +1,17 @@
-use crate::api::ApiError;
+use super::http_err;
 use crate::{Bus, Config, Notification};
 
+use proxmox_http_error::HttpError;
+
 /// Send a notification to a given target.
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns an `ApiError` in case of an error.
-pub fn send(config: &Config, channel: &str, notification: &Notification) -> Result<(), ApiError> {
+/// Returns an `anyhow::Error` in case of an error.
+pub fn send(config: &Config, channel: &str, notification: &Notification) -> Result<(), HttpError> {
     let bus = Bus::from_config(config).map_err(|err| {
-        ApiError::internal_server_error(
-            "Could not instantiate notification bus",
-            Some(Box::new(err)),
+        http_err!(
+            INTERNAL_SERVER_ERROR,
+            "Could not instantiate notification bus: {err}"
         )
     })?;
 
@@ -21,23 +23,20 @@ pub fn send(config: &Config, channel: &str, notification: &Notification) -> Resu
 /// Test target (group or single endpoint) identified by its `name`.
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns an `ApiError` if sending via the endpoint failed.
-pub fn test_target(config: &Config, endpoint: &str) -> Result<(), ApiError> {
+/// Returns an `anyhow::Error` if sending via the endpoint failed.
+pub fn test_target(config: &Config, endpoint: &str) -> Result<(), HttpError> {
     let bus = Bus::from_config(config).map_err(|err| {
-        ApiError::internal_server_error(
-            "Could not instantiate notification bus",
-            Some(Box::new(err)),
+        http_err!(
+            INTERNAL_SERVER_ERROR,
+            "Could not instantiate notification bus: {err}"
         )
     })?;
 
     bus.test_target(endpoint).map_err(|err| match err {
         crate::Error::TargetDoesNotExist(endpoint) => {
-            ApiError::not_found(format!("endpoint '{endpoint}' does not exist"), None)
+            http_err!(NOT_FOUND, "endpoint '{endpoint}' does not exist")
         }
-        _ => ApiError::internal_server_error(
-            format!("Could not test target: {err}"),
-            Some(Box::new(err)),
-        ),
+        _ => http_err!(INTERNAL_SERVER_ERROR, "Could not test target: {err}"),
     })?;
 
     Ok(())
@@ -49,7 +48,7 @@ pub fn test_target(config: &Config, endpoint: &str) -> Result<(), ApiError> {
 /// the result for 'grp1' would be [grp1, a, b, c, filter1, filter2].
 /// The result will always contain the entity that was passed as a parameter.
 /// If the entity does not exist, the result will only contain the entity.
-pub fn get_referenced_entities(config: &Config, entity: &str) -> Result<Vec<String>, ApiError> {
+pub fn get_referenced_entities(config: &Config, entity: &str) -> Result<Vec<String>, HttpError> {
     let entities = super::get_referenced_entities(config, entity);
     Ok(Vec::from_iter(entities.into_iter()))
 }
diff --git a/proxmox-notify/src/api/filter.rs b/proxmox-notify/src/api/filter.rs
index b5b6284..be0c25b 100644
--- a/proxmox-notify/src/api/filter.rs
+++ b/proxmox-notify/src/api/filter.rs
@@ -1,63 +1,69 @@
-use crate::api::ApiError;
+use crate::api::http_err;
 use crate::filter::{DeleteableFilterProperty, FilterConfig, FilterConfigUpdater, FILTER_TYPENAME};
 use crate::Config;
+use proxmox_http_error::HttpError;
 
 /// Get a list of all filters
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns a list of all filters or an `ApiError` if the config is erroneous.
-pub fn get_filters(config: &Config) -> Result<Vec<FilterConfig>, ApiError> {
+/// Returns a list of all filters or a `HttpError` if the config is
+/// (`500 Internal server error`).
+pub fn get_filters(config: &Config) -> Result<Vec<FilterConfig>, HttpError> {
     config
         .config
         .convert_to_typed_array(FILTER_TYPENAME)
-        .map_err(|e| ApiError::internal_server_error("Could not fetch filters", Some(e.into())))
+        .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "Could not fetch filters: {e}"))
 }
 
 /// Get filter with given `name`
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns the endpoint or an `ApiError` if the filter was not found.
-pub fn get_filter(config: &Config, name: &str) -> Result<FilterConfig, ApiError> {
+/// Returns the endpoint or a `HttpError` if the filter was not found (`404 Not found`).
+pub fn get_filter(config: &Config, name: &str) -> Result<FilterConfig, HttpError> {
     config
         .config
         .lookup(FILTER_TYPENAME, name)
-        .map_err(|_| ApiError::not_found(format!("filter '{name}' not found"), None))
+        .map_err(|_| http_err!(NOT_FOUND, "filter '{name}' not found"))
 }
 
 /// Add new notification filter.
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if a filter with the same name already exists or
-/// if the filter could not be saved.
-pub fn add_filter(config: &mut Config, filter_config: &FilterConfig) -> Result<(), ApiError> {
+/// Returns a `HttpError` if:
+///   - an entity with the same name already exists (`400 Bad request`)
+///   - the configuration could not be saved (`500 Internal server error`)
+pub fn add_filter(config: &mut Config, filter_config: &FilterConfig) -> Result<(), HttpError> {
     super::ensure_unique(config, &filter_config.name)?;
 
     config
         .config
         .set_data(&filter_config.name, FILTER_TYPENAME, filter_config)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save filter '{}'", filter_config.name),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save filter '{}': {e}",
+                filter_config.name
             )
         })?;
 
     Ok(())
 }
 
-/// Update existing filter
+/// Update existing notification filter
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the config could not be saved.
+/// Returns a `HttpError` if:
+///   - the configuration could not be saved (`500 Internal server error`)
+///   - an invalid digest was passed (`400 Bad request`)
 pub fn update_filter(
     config: &mut Config,
     name: &str,
     filter_updater: &FilterConfigUpdater,
     delete: Option<&[DeleteableFilterProperty]>,
     digest: Option<&[u8]>,
-) -> Result<(), ApiError> {
+) -> Result<(), HttpError> {
     super::verify_digest(config, digest)?;
 
     let mut filter = get_filter(config, name)?;
@@ -92,12 +98,7 @@ pub fn update_filter(
     config
         .config
         .set_data(name, FILTER_TYPENAME, &filter)
-        .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save filter '{name}'"),
-                Some(e.into()),
-            )
-        })?;
+        .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "could not save filter '{name}': {e}"))?;
 
     Ok(())
 }
@@ -106,8 +107,10 @@ pub fn update_filter(
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the filter does not exist.
-pub fn delete_filter(config: &mut Config, name: &str) -> Result<(), ApiError> {
+/// Returns a `HttpError` if:
+///   - the entity does not exist (`404 Not found`)
+///   - the filter is still referenced by another entity (`400 Bad request`)
+pub fn delete_filter(config: &mut Config, name: &str) -> Result<(), HttpError> {
     // Check if the filter exists
     let _ = get_filter(config, name)?;
     super::ensure_unused(config, name)?;
@@ -142,14 +145,14 @@ filter: filter2
     }
 
     #[test]
-    fn test_update_not_existing_returns_error() -> Result<(), ApiError> {
+    fn test_update_not_existing_returns_error() -> Result<(), HttpError> {
         let mut config = empty_config();
         assert!(update_filter(&mut config, "test", &Default::default(), None, None).is_err());
         Ok(())
     }
 
     #[test]
-    fn test_update_invalid_digest_returns_error() -> Result<(), ApiError> {
+    fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> {
         let mut config = config_with_two_filters();
         assert!(update_filter(
             &mut config,
@@ -164,7 +167,7 @@ filter: filter2
     }
 
     #[test]
-    fn test_filter_update() -> Result<(), ApiError> {
+    fn test_filter_update() -> Result<(), HttpError> {
         let mut config = config_with_two_filters();
 
         let digest = config.digest;
@@ -215,7 +218,7 @@ filter: filter2
     }
 
     #[test]
-    fn test_filter_delete() -> Result<(), ApiError> {
+    fn test_filter_delete() -> Result<(), HttpError> {
         let mut config = config_with_two_filters();
 
         delete_filter(&mut config, "filter1")?;
diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
index 521dd16..0ec48fd 100644
--- a/proxmox-notify/src/api/gotify.rs
+++ b/proxmox-notify/src/api/gotify.rs
@@ -1,4 +1,6 @@
-use crate::api::ApiError;
+use proxmox_http_error::HttpError;
+
+use crate::api::http_err;
 use crate::endpoints::gotify::{
     DeleteableGotifyProperty, GotifyConfig, GotifyConfigUpdater, GotifyPrivateConfig,
     GotifyPrivateConfigUpdater, GOTIFY_TYPENAME,
@@ -8,36 +10,41 @@ use crate::Config;
 /// Get a list of all gotify endpoints.
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns a list of all gotify endpoints or an `ApiError` if the config is erroneous.
-pub fn get_endpoints(config: &Config) -> Result<Vec<GotifyConfig>, ApiError> {
+/// Returns a list of all gotify endpoints or a `HttpError` if the config is
+/// erroneous (`500 Internal server error`).
+pub fn get_endpoints(config: &Config) -> Result<Vec<GotifyConfig>, HttpError> {
     config
         .config
         .convert_to_typed_array(GOTIFY_TYPENAME)
-        .map_err(|e| ApiError::internal_server_error("Could not fetch endpoints", Some(e.into())))
+        .map_err(|e| http_err!(NOT_FOUND, "Could not fetch endpoints: {e}"))
 }
 
 /// Get gotify endpoint with given `name`
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns the endpoint or an `ApiError` if the endpoint was not found.
-pub fn get_endpoint(config: &Config, name: &str) -> Result<GotifyConfig, ApiError> {
+/// Returns the endpoint or a `HttpError` if the endpoint was not found (`404 Not found`).
+pub fn get_endpoint(config: &Config, name: &str) -> Result<GotifyConfig, HttpError> {
     config
         .config
         .lookup(GOTIFY_TYPENAME, name)
-        .map_err(|_| ApiError::not_found(format!("endpoint '{name}' not found"), None))
+        .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found"))
 }
 
 /// Add a new gotify endpoint.
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if an endpoint with the same name already exists,
-/// or if the endpoint could not be saved.
+/// Returns a `HttpError` if:
+///   - an entity with the same name already exists (`400 Bad request`)
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - the configuration could not be saved (`500 Internal server error`)
+///
+/// Panics if the names of the private config and the public config do not match.
 pub fn add_endpoint(
     config: &mut Config,
     endpoint_config: &GotifyConfig,
     private_endpoint_config: &GotifyPrivateConfig,
-) -> Result<(), ApiError> {
+) -> Result<(), HttpError> {
     if endpoint_config.name != private_endpoint_config.name {
         // Programming error by the user of the crate, thus we panic
         panic!("name for endpoint config and private config must be identical");
@@ -56,20 +63,22 @@ pub fn add_endpoint(
         .config
         .set_data(&endpoint_config.name, GOTIFY_TYPENAME, endpoint_config)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save endpoint '{}'", endpoint_config.name),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save endpoint '{}': {e}",
+                endpoint_config.name
             )
-        })?;
-
-    Ok(())
+        })
 }
 
 /// Update existing gotify endpoint
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the config could not be saved.
+/// Returns a `HttpError` if:
+///   - an entity with the same name already exists (`400 Bad request`)
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - the configuration could not be saved (`500 Internal server error`)
 pub fn update_endpoint(
     config: &mut Config,
     name: &str,
@@ -77,7 +86,7 @@ pub fn update_endpoint(
     private_endpoint_config_updater: &GotifyPrivateConfigUpdater,
     delete: Option<&[DeleteableGotifyProperty]>,
     digest: Option<&[u8]>,
-) -> Result<(), ApiError> {
+) -> Result<(), HttpError> {
     super::verify_digest(config, digest)?;
 
     let mut endpoint = get_endpoint(config, name)?;
@@ -120,21 +129,21 @@ pub fn update_endpoint(
         .config
         .set_data(name, GOTIFY_TYPENAME, &endpoint)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save endpoint '{name}'"),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save endpoint '{name}': {e}"
             )
-        })?;
-
-    Ok(())
+        })
 }
 
 /// Delete existing gotify endpoint
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the endpoint does not exist.
-pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), ApiError> {
+/// Returns a `HttpError` if:
+///   - the entity does not exist (`404 Not found`)
+///   - the endpoint is still referenced by another entity (`400 Bad request`)
+pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError> {
     // Check if the endpoint exists
     let _ = get_endpoint(config, name)?;
     super::ensure_unused(config, name)?;
@@ -148,22 +157,20 @@ pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), Api
 fn set_private_config_entry(
     config: &mut Config,
     private_config: &GotifyPrivateConfig,
-) -> Result<(), ApiError> {
+) -> Result<(), HttpError> {
     config
         .private_config
         .set_data(&private_config.name, GOTIFY_TYPENAME, private_config)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!(
-                    "could not save private config for endpoint '{}'",
-                    private_config.name
-                ),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save private config for endpoint '{}': {e}",
+                private_config.name
             )
         })
 }
 
-fn remove_private_config_entry(config: &mut Config, name: &str) -> Result<(), ApiError> {
+fn remove_private_config_entry(config: &mut Config, name: &str) -> Result<(), HttpError> {
     config.private_config.sections.remove(name);
     Ok(())
 }
@@ -173,7 +180,7 @@ mod tests {
     use super::*;
     use crate::api::test_helpers::empty_config;
 
-    pub fn add_default_gotify_endpoint(config: &mut Config) -> Result<(), ApiError> {
+    pub fn add_default_gotify_endpoint(config: &mut Config) -> Result<(), HttpError> {
         add_endpoint(
             config,
             &GotifyConfig {
@@ -193,7 +200,7 @@ mod tests {
     }
 
     #[test]
-    fn test_update_not_existing_returns_error() -> Result<(), ApiError> {
+    fn test_update_not_existing_returns_error() -> Result<(), HttpError> {
         let mut config = empty_config();
 
         assert!(update_endpoint(
@@ -210,7 +217,7 @@ mod tests {
     }
 
     #[test]
-    fn test_update_invalid_digest_returns_error() -> Result<(), ApiError> {
+    fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_gotify_endpoint(&mut config)?;
 
@@ -228,7 +235,7 @@ mod tests {
     }
 
     #[test]
-    fn test_gotify_update() -> Result<(), ApiError> {
+    fn test_gotify_update() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_gotify_endpoint(&mut config)?;
 
@@ -279,7 +286,7 @@ mod tests {
     }
 
     #[test]
-    fn test_gotify_endpoint_delete() -> Result<(), ApiError> {
+    fn test_gotify_endpoint_delete() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_gotify_endpoint(&mut config)?;
 
diff --git a/proxmox-notify/src/api/group.rs b/proxmox-notify/src/api/group.rs
index d6b8f38..6fc71ea 100644
--- a/proxmox-notify/src/api/group.rs
+++ b/proxmox-notify/src/api/group.rs
@@ -1,43 +1,47 @@
-use crate::api::ApiError;
+use proxmox_http_error::HttpError;
+
+use crate::api::{http_bail, http_err};
 use crate::group::{DeleteableGroupProperty, GroupConfig, GroupConfigUpdater, GROUP_TYPENAME};
 use crate::Config;
 
 /// Get all notification groups
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns a list of all groups or an `ApiError` if the config is erroneous.
-pub fn get_groups(config: &Config) -> Result<Vec<GroupConfig>, ApiError> {
+/// Returns a list of all groups or a `HttpError` if the config is
+/// erroneous (`500 Internal server error`).
+pub fn get_groups(config: &Config) -> Result<Vec<GroupConfig>, HttpError> {
     config
         .config
         .convert_to_typed_array(GROUP_TYPENAME)
-        .map_err(|e| ApiError::internal_server_error("Could not fetch groups", Some(e.into())))
+        .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "Could not fetch groups: {e}"))
 }
 
 /// Get group with given `name`
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns the endpoint or an `ApiError` if the group was not found.
-pub fn get_group(config: &Config, name: &str) -> Result<GroupConfig, ApiError> {
+/// Returns the endpoint or an `HttpError` if the group was not found (`404 Not found`).
+pub fn get_group(config: &Config, name: &str) -> Result<GroupConfig, HttpError> {
     config
         .config
         .lookup(GROUP_TYPENAME, name)
-        .map_err(|_| ApiError::not_found(format!("group '{name}' not found"), None))
+        .map_err(|_| http_err!(NOT_FOUND, "group '{name}' not found"))
 }
 
 /// Add a new group.
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if a group with the same name already exists, or
-/// if the group could not be saved
-pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(), ApiError> {
+/// Returns a `HttpError` if:
+///   - an entity with the same name already exists (`400 Bad request`)
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - no endpoints were passed (`400 Bad request`)
+///   - referenced endpoints do not exist (`404 Not found`)
+///   - the configuration could not be saved (`500 Internal server error`)
+pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(), HttpError> {
     super::ensure_unique(config, &group_config.name)?;
 
     if group_config.endpoint.is_empty() {
-        return Err(ApiError::bad_request(
-            "group must contain at least one endpoint",
-            None,
-        ));
+        http_bail!(BAD_REQUEST, "group must contain at least one endpoint",);
     }
 
     if let Some(filter) = &group_config.filter {
@@ -51,27 +55,31 @@ pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(),
         .config
         .set_data(&group_config.name, GROUP_TYPENAME, group_config)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save group '{}'", group_config.name),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save group '{}': {e}",
+                group_config.name
             )
-        })?;
-
-    Ok(())
+        })
 }
 
 /// Update existing group
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the config could not be saved.
+/// Returns a `HttpError` if:
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - an invalid digest was passed (`400 Bad request`)
+///   - no endpoints were passed (`400 Bad request`)
+///   - referenced endpoints do not exist (`404 Not found`)
+///   - the configuration could not be saved (`500 Internal server error`)
 pub fn update_group(
     config: &mut Config,
     name: &str,
     updater: &GroupConfigUpdater,
     delete: Option<&[DeleteableGroupProperty]>,
     digest: Option<&[u8]>,
-) -> Result<(), ApiError> {
+) -> Result<(), HttpError> {
     super::verify_digest(config, digest)?;
 
     let mut group = get_group(config, name)?;
@@ -88,10 +96,7 @@ pub fn update_group(
     if let Some(endpoints) = &updater.endpoint {
         super::ensure_endpoints_exist(config, endpoints)?;
         if endpoints.is_empty() {
-            return Err(ApiError::bad_request(
-                "group must contain at least one endpoint",
-                None,
-            ));
+            http_bail!(BAD_REQUEST, "group must contain at least one endpoint",);
         }
         group.endpoint = endpoints.iter().map(Into::into).collect()
     }
@@ -109,22 +114,15 @@ pub fn update_group(
     config
         .config
         .set_data(name, GROUP_TYPENAME, &group)
-        .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save group '{name}'"),
-                Some(e.into()),
-            )
-        })?;
-
-    Ok(())
+        .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "could not save group '{name}': {e}"))
 }
 
 /// Delete existing group
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the group does not exist.
-pub fn delete_group(config: &mut Config, name: &str) -> Result<(), ApiError> {
+/// Returns a `HttpError` if the group does not exist (`404 Not found`).
+pub fn delete_group(config: &mut Config, name: &str) -> Result<(), HttpError> {
     // Check if the group exists
     let _ = get_group(config, name)?;
 
@@ -141,7 +139,7 @@ mod tests {
     use crate::api::sendmail::tests::add_sendmail_endpoint_for_test;
     use crate::api::test_helpers::*;
 
-    fn add_default_group(config: &mut Config) -> Result<(), ApiError> {
+    fn add_default_group(config: &mut Config) -> Result<(), HttpError> {
         add_sendmail_endpoint_for_test(config, "test")?;
 
         add_group(
@@ -173,14 +171,14 @@ mod tests {
     }
 
     #[test]
-    fn test_add_group() -> Result<(), ApiError> {
+    fn test_add_group() -> Result<(), HttpError> {
         let mut config = empty_config();
         assert!(add_default_group(&mut config).is_ok());
         Ok(())
     }
 
     #[test]
-    fn test_update_group_fails_if_endpoint_does_not_exist() -> Result<(), ApiError> {
+    fn test_update_group_fails_if_endpoint_does_not_exist() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_group(&mut config)?;
 
@@ -199,7 +197,7 @@ mod tests {
     }
 
     #[test]
-    fn test_update_group_fails_if_digest_invalid() -> Result<(), ApiError> {
+    fn test_update_group_fails_if_digest_invalid() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_group(&mut config)?;
 
@@ -215,7 +213,7 @@ mod tests {
     }
 
     #[test]
-    fn test_update_group() -> Result<(), ApiError> {
+    fn test_update_group() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_group(&mut config)?;
 
@@ -249,7 +247,7 @@ mod tests {
     }
 
     #[test]
-    fn test_group_delete() -> Result<(), ApiError> {
+    fn test_group_delete() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_default_group(&mut config)?;
 
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index 9372443..cb0e085 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -1,9 +1,6 @@
-use std::collections::HashSet;
-use std::error::Error as StdError;
-use std::fmt::Display;
-
 use crate::Config;
-use serde::Serialize;
+use proxmox_http_error::HttpError;
+use std::collections::HashSet;
 
 pub mod common;
 pub mod filter;
@@ -13,81 +10,44 @@ pub mod group;
 #[cfg(feature = "sendmail")]
 pub mod sendmail;
 
-#[derive(Debug, Serialize)]
-pub struct ApiError {
-    /// HTTP Error code
-    code: u16,
-    /// Error message
-    message: String,
-    #[serde(skip_serializing)]
-    /// The underlying cause of the error
-    source: Option<Box<dyn StdError + Send + Sync + 'static>>,
+// We have our own, local versions of http_err and http_bail, because
+// we don't want to wrap the error in anyhow::Error. If we were to do that,
+// we would need to downcast in the perlmod bindings, since we need
+// to return `HttpError` from there.
+#[macro_export]
+macro_rules! http_err {
+    ($status:ident, $($fmt:tt)+) => {{
+        proxmox_http_error::HttpError::new(
+            proxmox_http_error::StatusCode::$status,
+            format!($($fmt)+)
+        )
+    }};
 }
 
-impl ApiError {
-    fn new<S: AsRef<str>>(
-        message: S,
-        code: u16,
-        source: Option<Box<dyn StdError + Send + Sync + 'static>>,
-    ) -> Self {
-        Self {
-            message: message.as_ref().into(),
-            code,
-            source,
-        }
-    }
-
-    pub fn bad_request<S: AsRef<str>>(
-        message: S,
-        source: Option<Box<dyn StdError + Send + Sync + 'static>>,
-    ) -> Self {
-        Self::new(message, 400, source)
-    }
-
-    pub fn not_found<S: AsRef<str>>(
-        message: S,
-        source: Option<Box<dyn StdError + Send + Sync + 'static>>,
-    ) -> Self {
-        Self::new(message, 404, source)
-    }
-
-    pub fn internal_server_error<S: AsRef<str>>(
-        message: S,
-        source: Option<Box<dyn StdError + Send + Sync + 'static>>,
-    ) -> Self {
-        Self::new(message, 500, source)
-    }
+#[macro_export]
+macro_rules! http_bail {
+    ($status:ident, $($fmt:tt)+) => {{
+        return Err($crate::api::http_err!($status, $($fmt)+));
+    }};
 }
 
-impl Display for ApiError {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str(&format!("{} {}", self.code, self.message))
-    }
-}
+pub use http_bail;
+pub use http_err;
 
-impl StdError for ApiError {
-    fn source(&self) -> Option<&(dyn StdError + 'static)> {
-        match &self.source {
-            None => None,
-            Some(source) => Some(&**source),
-        }
-    }
-}
-
-fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), ApiError> {
+fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), HttpError> {
     if let Some(digest) = digest {
         if config.digest != *digest {
-            return Err(ApiError::bad_request(
-                "detected modified configuration - file changed by other user? Try again.",
-                None,
-            ));
+            http_bail!(
+                BAD_REQUEST,
+                "detected modified configuration - file changed by other user? Try again."
+            );
         }
     }
 
     Ok(())
 }
 
-fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Result<(), ApiError> {
+fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Result<(), HttpError> {
     #[allow(unused_mut)]
     let mut exists = false;
 
@@ -101,16 +61,16 @@ fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Resul
     }
 
     if !exists {
-        Err(ApiError::not_found(
-            format!("endpoint '{name}' does not exist"),
-            None,
-        ))
+        http_bail!(NOT_FOUND, "endpoint '{name}' does not exist")
     } else {
         Ok(())
     }
 }
 
-fn ensure_endpoints_exist<T: AsRef<str>>(config: &Config, endpoints: &[T]) -> Result<(), ApiError> {
+fn ensure_endpoints_exist<T: AsRef<str>>(
+    config: &Config,
+    endpoints: &[T],
+) -> Result<(), HttpError> {
     for endpoint in endpoints {
         ensure_endpoint_exists(config, endpoint.as_ref())?;
     }
@@ -118,18 +78,18 @@ fn ensure_endpoints_exist<T: AsRef<str>>(config: &Config, endpoints: &[T]) -> Re
     Ok(())
 }
 
-fn ensure_unique(config: &Config, entity: &str) -> Result<(), ApiError> {
+fn ensure_unique(config: &Config, entity: &str) -> Result<(), HttpError> {
     if config.config.sections.contains_key(entity) {
-        return Err(ApiError::bad_request(
-            format!("Cannot create '{entity}', an entity with the same name already exists"),
-            None,
-        ));
+        http_bail!(
+            BAD_REQUEST,
+            "Cannot create '{entity}', an entity with the same name already exists"
+        );
     }
 
     Ok(())
 }
 
-fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, ApiError> {
+fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, HttpError> {
     let mut referrers = HashSet::new();
 
     for group in group::get_groups(config)? {
@@ -165,16 +125,16 @@ fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, ApiEr
     Ok(referrers)
 }
 
-fn ensure_unused(config: &Config, entity: &str) -> Result<(), ApiError> {
+fn ensure_unused(config: &Config, entity: &str) -> Result<(), HttpError> {
     let referrers = get_referrers(config, entity)?;
 
     if !referrers.is_empty() {
         let used_by = referrers.into_iter().collect::<Vec<_>>().join(", ");
 
-        return Err(ApiError::bad_request(
-            format!("cannot delete '{entity}', referenced by: {used_by}"),
-            None,
-        ));
+        http_bail!(
+            BAD_REQUEST,
+            "cannot delete '{entity}', referenced by: {used_by}"
+        );
     }
 
     Ok(())
@@ -240,7 +200,7 @@ mod tests {
     use crate::filter::FilterConfig;
     use crate::group::GroupConfig;
 
-    fn prepare_config() -> Result<Config, ApiError> {
+    fn prepare_config() -> Result<Config, HttpError> {
         let mut config = super::test_helpers::empty_config();
 
         filter::add_filter(
@@ -316,7 +276,7 @@ mod tests {
     }
 
     #[test]
-    fn test_get_referrers_for_entity() -> Result<(), ApiError> {
+    fn test_get_referrers_for_entity() -> Result<(), HttpError> {
         let config = prepare_config().unwrap();
 
         assert_eq!(
diff --git a/proxmox-notify/src/api/sendmail.rs b/proxmox-notify/src/api/sendmail.rs
index 59a57b4..ac8737c 100644
--- a/proxmox-notify/src/api/sendmail.rs
+++ b/proxmox-notify/src/api/sendmail.rs
@@ -1,4 +1,6 @@
-use crate::api::ApiError;
+use proxmox_http_error::HttpError;
+
+use crate::api::{http_bail, http_err};
 use crate::endpoints::sendmail::{
     DeleteableSendmailProperty, SendmailConfig, SendmailConfigUpdater, SENDMAIL_TYPENAME,
 };
@@ -7,32 +9,36 @@ use crate::Config;
 /// Get a list of all sendmail endpoints.
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns a list of all sendmail endpoints or an `ApiError` if the config is erroneous.
-pub fn get_endpoints(config: &Config) -> Result<Vec<SendmailConfig>, ApiError> {
+/// Returns a list of all sendmail endpoints or a `HttpError` if the config is
+/// erroneous (`500 Internal server error`).
+pub fn get_endpoints(config: &Config) -> Result<Vec<SendmailConfig>, HttpError> {
     config
         .config
         .convert_to_typed_array(SENDMAIL_TYPENAME)
-        .map_err(|e| ApiError::internal_server_error("Could not fetch endpoints", Some(e.into())))
+        .map_err(|e| http_err!(NOT_FOUND, "Could not fetch endpoints: {e}"))
 }
 
 /// Get sendmail endpoint with given `name`.
 ///
 /// The caller is responsible for any needed permission checks.
-/// Returns the endpoint or an `ApiError` if the endpoint was not found.
-pub fn get_endpoint(config: &Config, name: &str) -> Result<SendmailConfig, ApiError> {
+/// Returns the endpoint or a `HttpError` if the endpoint was not found (`404 Not found`).
+pub fn get_endpoint(config: &Config, name: &str) -> Result<SendmailConfig, HttpError> {
     config
         .config
         .lookup(SENDMAIL_TYPENAME, name)
-        .map_err(|_| ApiError::not_found(format!("endpoint '{name}' not found"), None))
+        .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found"))
 }
 
 /// Add a new sendmail endpoint.
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if an endpoint with the same name already exists,
-/// or if the endpoint could not be saved.
-pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<(), ApiError> {
+/// Returns a `HttpError` if:
+///   - an entity with the same name already exists (`400 Bad request`)
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - the configuration could not be saved (`500 Internal server error`)
+///   - mailto *and* mailto_user are both set to `None`
+pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<(), HttpError> {
     super::ensure_unique(config, &endpoint.name)?;
 
     if let Some(filter) = &endpoint.filter {
@@ -41,37 +47,39 @@ pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<()
     }
 
     if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
-        return Err(ApiError::bad_request(
-            "must at least provide one recipient, either in mailto or in mailto-user",
-            None,
-        ));
+        http_bail!(
+            BAD_REQUEST,
+            "must at least provide one recipient, either in mailto or in mailto-user"
+        );
     }
 
     config
         .config
         .set_data(&endpoint.name, SENDMAIL_TYPENAME, endpoint)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save endpoint '{}'", endpoint.name),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save endpoint '{}': {e}",
+                endpoint.name
             )
-        })?;
-
-    Ok(())
+        })
 }
 
 /// Update existing sendmail endpoint
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the config could not be saved.
+/// Returns a `HttpError` if:
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - the configuration could not be saved (`500 Internal server error`)
+///   - mailto *and* mailto_user are both set to `None`
 pub fn update_endpoint(
     config: &mut Config,
     name: &str,
     updater: &SendmailConfigUpdater,
     delete: Option<&[DeleteableSendmailProperty]>,
     digest: Option<&[u8]>,
-) -> Result<(), ApiError> {
+) -> Result<(), HttpError> {
     super::verify_digest(config, digest)?;
 
     let mut endpoint = get_endpoint(config, name)?;
@@ -115,31 +123,33 @@ pub fn update_endpoint(
     }
 
     if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
-        return Err(ApiError::bad_request(
-            "must at least provide one recipient, either in mailto or in mailto-user",
-            None,
-        ));
+        http_bail!(
+            BAD_REQUEST,
+            "must at least provide one recipient, either in mailto or in mailto-user"
+        );
     }
 
     config
         .config
         .set_data(name, SENDMAIL_TYPENAME, &endpoint)
         .map_err(|e| {
-            ApiError::internal_server_error(
-                format!("could not save endpoint '{name}'"),
-                Some(e.into()),
+            http_err!(
+                INTERNAL_SERVER_ERROR,
+                "could not save endpoint '{}': {e}",
+                endpoint.name
             )
-        })?;
-
-    Ok(())
+        })
 }
 
 /// Delete existing sendmail endpoint
 ///
 /// The caller is responsible for any needed permission checks.
 /// The caller also responsible for locking the configuration files.
-/// Returns an `ApiError` if the endpoint does not exist.
-pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), ApiError> {
+/// Returns a `HttpError` if:
+///   - an entity with the same name already exists (`400 Bad request`)
+///   - a referenced filter does not exist (`400 Bad request`)
+///   - the configuration could not be saved (`500 Internal server error`)
+pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError> {
     // Check if the endpoint exists
     let _ = get_endpoint(config, name)?;
     super::ensure_unused(config, name)?;
@@ -154,7 +164,10 @@ pub mod tests {
     use super::*;
     use crate::api::test_helpers::*;
 
-    pub fn add_sendmail_endpoint_for_test(config: &mut Config, name: &str) -> Result<(), ApiError> {
+    pub fn add_sendmail_endpoint_for_test(
+        config: &mut Config,
+        name: &str,
+    ) -> Result<(), HttpError> {
         add_endpoint(
             config,
             &SendmailConfig {
@@ -173,7 +186,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_sendmail_create() -> Result<(), ApiError> {
+    fn test_sendmail_create() -> Result<(), HttpError> {
         let mut config = empty_config();
 
         assert_eq!(get_endpoints(&config)?.len(), 0);
@@ -186,7 +199,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_update_not_existing_returns_error() -> Result<(), ApiError> {
+    fn test_update_not_existing_returns_error() -> Result<(), HttpError> {
         let mut config = empty_config();
 
         assert!(update_endpoint(&mut config, "test", &Default::default(), None, None,).is_err());
@@ -195,7 +208,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_update_invalid_digest_returns_error() -> Result<(), ApiError> {
+    fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_sendmail_endpoint_for_test(&mut config, "sendmail-endpoint")?;
 
@@ -219,7 +232,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_sendmail_update() -> Result<(), ApiError> {
+    fn test_sendmail_update() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_sendmail_endpoint_for_test(&mut config, "sendmail-endpoint")?;
 
@@ -275,7 +288,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_sendmail_delete() -> Result<(), ApiError> {
+    fn test_sendmail_delete() -> Result<(), HttpError> {
         let mut config = empty_config();
         add_sendmail_endpoint_for_test(&mut config, "sendmail-endpoint")?;
 
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox-perl-rs 4/4] notify: use new HttpError type

[Lukas Wagner]

Use `proxmox-http-error::HttpError` instead of
`proxmox-notify::api::ApiError`.

Also factoring out the digest decoding into a small helper.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 common/src/notify.rs | 77 +++++++++++++++++++++++---------------------
 pve-rs/Cargo.toml    |  1 +
 2 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/common/src/notify.rs b/common/src/notify.rs
index c8ca533..9f44225 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -5,6 +5,7 @@ mod export {
     use serde_json::Value as JSONValue;
     use std::sync::Mutex;
 
+    use proxmox_http_error::HttpError;
     use proxmox_notify::endpoints::gotify::{
         DeleteableGotifyProperty, GotifyConfig, GotifyConfigUpdater, GotifyPrivateConfig,
         GotifyPrivateConfigUpdater,
@@ -16,7 +17,7 @@ mod export {
         DeleteableFilterProperty, FilterConfig, FilterConfigUpdater, FilterModeOperator,
     };
     use proxmox_notify::group::{DeleteableGroupProperty, GroupConfig, GroupConfigUpdater};
-    use proxmox_notify::{api, api::ApiError, Config, Notification, Severity};
+    use proxmox_notify::{api, Config, Notification, Severity};
 
     pub struct NotificationConfig {
         config: Mutex<Config>,
@@ -91,7 +92,7 @@ mod export {
         title: String,
         body: String,
         properties: Option<JSONValue>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let config = this.config.lock().unwrap();
 
         let notification = Notification {
@@ -108,13 +109,15 @@ mod export {
     fn test_target(
         #[try_from_ref] this: &NotificationConfig,
         target: &str,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let config = this.config.lock().unwrap();
         api::common::test_target(&config, target)
     }
 
     #[export(serialize_error)]
-    fn get_groups(#[try_from_ref] this: &NotificationConfig) -> Result<Vec<GroupConfig>, ApiError> {
+    fn get_groups(
+        #[try_from_ref] this: &NotificationConfig,
+    ) -> Result<Vec<GroupConfig>, HttpError> {
         let config = this.config.lock().unwrap();
         api::group::get_groups(&config)
     }
@@ -123,7 +126,7 @@ mod export {
     fn get_group(
         #[try_from_ref] this: &NotificationConfig,
         id: &str,
-    ) -> Result<GroupConfig, ApiError> {
+    ) -> Result<GroupConfig, HttpError> {
         let config = this.config.lock().unwrap();
         api::group::get_group(&config, id)
     }
@@ -135,7 +138,7 @@ mod export {
         endpoints: Vec<String>,
         comment: Option<String>,
         filter: Option<String>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::group::add_group(
             &mut config,
@@ -157,11 +160,9 @@ mod export {
         filter: Option<String>,
         delete: Option<Vec<DeleteableGroupProperty>>,
         digest: Option<&str>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
-        let digest = digest.map(hex::decode).transpose().map_err(|e| {
-            ApiError::internal_server_error(format!("invalid digest: {e}"), Some(Box::new(e)))
-        })?;
+        let digest = decode_digest(digest)?;
 
         api::group::update_group(
             &mut config,
@@ -177,7 +178,10 @@ mod export {
     }
 
     #[export(serialize_error)]
-    fn delete_group(#[try_from_ref] this: &NotificationConfig, name: &str) -> Result<(), ApiError> {
+    fn delete_group(
+        #[try_from_ref] this: &NotificationConfig,
+        name: &str,
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::group::delete_group(&mut config, name)
     }
@@ -185,7 +189,7 @@ mod export {
     #[export(serialize_error)]
     fn get_sendmail_endpoints(
         #[try_from_ref] this: &NotificationConfig,
-    ) -> Result<Vec<SendmailConfig>, ApiError> {
+    ) -> Result<Vec<SendmailConfig>, HttpError> {
         let config = this.config.lock().unwrap();
         api::sendmail::get_endpoints(&config)
     }
@@ -194,7 +198,7 @@ mod export {
     fn get_sendmail_endpoint(
         #[try_from_ref] this: &NotificationConfig,
         id: &str,
-    ) -> Result<SendmailConfig, ApiError> {
+    ) -> Result<SendmailConfig, HttpError> {
         let config = this.config.lock().unwrap();
         api::sendmail::get_endpoint(&config, id)
     }
@@ -210,7 +214,7 @@ mod export {
         author: Option<String>,
         comment: Option<String>,
         filter: Option<String>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
 
         api::sendmail::add_endpoint(
@@ -240,11 +244,9 @@ mod export {
         filter: Option<String>,
         delete: Option<Vec<DeleteableSendmailProperty>>,
         digest: Option<&str>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
-        let digest = digest.map(hex::decode).transpose().map_err(|e| {
-            ApiError::internal_server_error(format!("invalid digest: {e}"), Some(Box::new(e)))
-        })?;
+        let digest = decode_digest(digest)?;
 
         api::sendmail::update_endpoint(
             &mut config,
@@ -266,7 +268,7 @@ mod export {
     fn delete_sendmail_endpoint(
         #[try_from_ref] this: &NotificationConfig,
         name: &str,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::sendmail::delete_endpoint(&mut config, name)
     }
@@ -274,7 +276,7 @@ mod export {
     #[export(serialize_error)]
     fn get_gotify_endpoints(
         #[try_from_ref] this: &NotificationConfig,
-    ) -> Result<Vec<GotifyConfig>, ApiError> {
+    ) -> Result<Vec<GotifyConfig>, HttpError> {
         let config = this.config.lock().unwrap();
         api::gotify::get_endpoints(&config)
     }
@@ -283,7 +285,7 @@ mod export {
     fn get_gotify_endpoint(
         #[try_from_ref] this: &NotificationConfig,
         id: &str,
-    ) -> Result<GotifyConfig, ApiError> {
+    ) -> Result<GotifyConfig, HttpError> {
         let config = this.config.lock().unwrap();
         api::gotify::get_endpoint(&config, id)
     }
@@ -296,7 +298,7 @@ mod export {
         token: String,
         comment: Option<String>,
         filter: Option<String>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::gotify::add_endpoint(
             &mut config,
@@ -321,11 +323,9 @@ mod export {
         filter: Option<String>,
         delete: Option<Vec<DeleteableGotifyProperty>>,
         digest: Option<&str>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
-        let digest = digest.map(hex::decode).transpose().map_err(|e| {
-            ApiError::internal_server_error(format!("invalid digest: {e}"), Some(Box::new(e)))
-        })?;
+        let digest = decode_digest(digest)?;
 
         api::gotify::update_endpoint(
             &mut config,
@@ -345,7 +345,7 @@ mod export {
     fn delete_gotify_endpoint(
         #[try_from_ref] this: &NotificationConfig,
         name: &str,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::gotify::delete_gotify_endpoint(&mut config, name)
     }
@@ -353,7 +353,7 @@ mod export {
     #[export(serialize_error)]
     fn get_filters(
         #[try_from_ref] this: &NotificationConfig,
-    ) -> Result<Vec<FilterConfig>, ApiError> {
+    ) -> Result<Vec<FilterConfig>, HttpError> {
         let config = this.config.lock().unwrap();
         api::filter::get_filters(&config)
     }
@@ -362,7 +362,7 @@ mod export {
     fn get_filter(
         #[try_from_ref] this: &NotificationConfig,
         id: &str,
-    ) -> Result<FilterConfig, ApiError> {
+    ) -> Result<FilterConfig, HttpError> {
         let config = this.config.lock().unwrap();
         api::filter::get_filter(&config, id)
     }
@@ -376,7 +376,7 @@ mod export {
         mode: Option<FilterModeOperator>,
         invert_match: Option<bool>,
         comment: Option<String>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::filter::add_filter(
             &mut config,
@@ -401,11 +401,9 @@ mod export {
         comment: Option<String>,
         delete: Option<Vec<DeleteableFilterProperty>>,
         digest: Option<&str>,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
-        let digest = digest.map(hex::decode).transpose().map_err(|e| {
-            ApiError::internal_server_error(format!("invalid digest: {e}"), Some(Box::new(e)))
-        })?;
+        let digest = decode_digest(digest)?;
 
         api::filter::update_filter(
             &mut config,
@@ -425,7 +423,7 @@ mod export {
     fn delete_filter(
         #[try_from_ref] this: &NotificationConfig,
         name: &str,
-    ) -> Result<(), ApiError> {
+    ) -> Result<(), HttpError> {
         let mut config = this.config.lock().unwrap();
         api::filter::delete_filter(&mut config, name)
     }
@@ -434,8 +432,15 @@ mod export {
     fn get_referenced_entities(
         #[try_from_ref] this: &NotificationConfig,
         name: &str,
-    ) -> Result<Vec<String>, ApiError> {
+    ) -> Result<Vec<String>, HttpError> {
         let config = this.config.lock().unwrap();
         api::common::get_referenced_entities(&config, name)
     }
+
+    fn decode_digest(digest: Option<&str>) -> Result<Option<Vec<u8>>, HttpError> {
+        digest
+            .map(hex::decode)
+            .transpose()
+            .map_err(|e| api::http_err!(BAD_REQUEST, "invalid digest: {e}"))
+    }
 }
diff --git a/pve-rs/Cargo.toml b/pve-rs/Cargo.toml
index 2b375c0..222fdf5 100644
--- a/pve-rs/Cargo.toml
+++ b/pve-rs/Cargo.toml
@@ -35,6 +35,7 @@ perlmod = { version = "0.13", features = [ "exporter" ] }
 
 proxmox-apt = "0.10"
 proxmox-http = { version = "0.9", features = ["client-sync", "client-trait"] }
+proxmox-http-error = "0.1.0"
 proxmox-notify = "0.1"
 proxmox-openid = "0.10"
 proxmox-resource-scheduling = "0.3.0"
-- 
2.39.2

[pve-devel] applied-series: [PATCH v2 proxmox{, -perl-rs} 0/4] move `HttpError` from `proxmox-router` into its own crate

[Wolfgang Bumiller]

applied series & bumped packages