* [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate
@ 2023-07-26 12:50 Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 1/5] http-error: add new http-error crate Lukas Wagner
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 12:50 UTC (permalink / raw)
To: pve-devel, pbs-devel
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`)
These patches also update the users of the old `HttpError` to the new version.
`proxmox-notify`, which previously used it's own error type, is now also using
the new crate.
proxmox:
Lukas Wagner (3):
http-error: add new http-error crate
router: rest-server: auth-api: use new http-error crate
notify: use HttpError from proxmox-http-error
Cargo.toml | 2 +
proxmox-auth-api/Cargo.toml | 2 +
proxmox-auth-api/src/api/access.rs | 3 +-
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-rest-server/Cargo.toml | 1 +
proxmox-rest-server/src/h2service.rs | 2 +-
proxmox-rest-server/src/rest.rs | 2 +-
proxmox-router/Cargo.toml | 1 +
proxmox-router/src/error.rs | 44 ---------
proxmox-router/src/lib.rs | 7 +-
18 files changed, 340 insertions(+), 304 deletions(-)
create mode 100644 proxmox-http-error/Cargo.toml
create mode 100644 proxmox-http-error/src/lib.rs
delete mode 100644 proxmox-router/src/error.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(-)
proxmox-backup:
Lukas Wagner (1):
use `HttpError` and macros from `proxmox-http-error` crate
Cargo.toml | 3 +++
src/api2/access/openid.rs | 5 ++---
src/api2/access/tfa.rs | 3 ++-
src/api2/admin/datastore.rs | 5 +++--
src/api2/admin/namespace.rs | 3 ++-
src/api2/backup/mod.rs | 5 +++--
src/api2/config/access/ldap.rs | 3 ++-
src/api2/config/access/openid.rs | 3 ++-
src/api2/config/acme.rs | 5 ++---
src/api2/config/changer.rs | 3 ++-
src/api2/config/datastore.rs | 3 ++-
src/api2/config/drive.rs | 3 ++-
src/api2/config/media_pool.rs | 3 ++-
src/api2/config/prune.rs | 3 ++-
src/api2/config/remote.rs | 3 ++-
src/api2/config/sync.rs | 3 ++-
src/api2/config/tape_backup_job.rs | 3 ++-
src/api2/config/tape_encryption_keys.rs | 3 ++-
src/api2/config/traffic_control.rs | 3 ++-
src/api2/config/verify.rs | 3 ++-
src/api2/helpers.rs | 2 +-
src/api2/reader/mod.rs | 5 +++--
src/auth.rs | 2 +-
src/backup/hierarchy.rs | 2 +-
src/server/pull.rs | 4 ++--
25 files changed, 51 insertions(+), 32 deletions(-)
Summary over all repositories:
45 files changed, 433 insertions(+), 372 deletions(-)
--
murpp v0.4.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH proxmox 1/5] http-error: add new http-error crate
2023-07-26 12:50 [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate Lukas Wagner
@ 2023-07-26 12:50 ` Lukas Wagner
2023-07-26 13:35 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use " Lukas Wagner
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 12:50 UTC (permalink / raw)
To: pve-devel, pbs-devel; +Cc: Lukas Wagner, Wolfgang Bumiller
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..17be2e0
--- /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, features = ["derive"]}
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use new http-error crate
2023-07-26 12:50 [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 1/5] http-error: add new http-error crate Lukas Wagner
@ 2023-07-26 12:50 ` Lukas Wagner
2023-07-26 13:41 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 3/5] notify: use HttpError from proxmox-http-error Lukas Wagner
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 12:50 UTC (permalink / raw)
To: pve-devel, pbs-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-auth-api/Cargo.toml | 2 ++
proxmox-auth-api/src/api/access.rs | 3 +-
proxmox-rest-server/Cargo.toml | 1 +
proxmox-rest-server/src/h2service.rs | 2 +-
proxmox-rest-server/src/rest.rs | 2 +-
proxmox-router/Cargo.toml | 1 +
proxmox-router/src/error.rs | 44 ----------------------------
proxmox-router/src/lib.rs | 7 +----
8 files changed, 9 insertions(+), 53 deletions(-)
delete mode 100644 proxmox-router/src/error.rs
diff --git a/proxmox-auth-api/Cargo.toml b/proxmox-auth-api/Cargo.toml
index bd1884a..45d7cbc 100644
--- a/proxmox-auth-api/Cargo.toml
+++ b/proxmox-auth-api/Cargo.toml
@@ -28,6 +28,7 @@ serde = { workspace = true, optional = true, features = [ "derive" ] }
serde_json = { workspace = true, optional = true }
serde_plain = { workspace = true, optional = true }
+proxmox-http-error = { workspace = true, optional = true }
proxmox-rest-server = { workspace = true, optional = true }
proxmox-router = { workspace = true, optional = true }
proxmox-schema = { workspace = true, optional = true, features = [ "api-macro", "api-types" ] }
@@ -45,6 +46,7 @@ api = [
"dep:http",
"dep:serde_json",
+ "dep:proxmox-http-error",
"dep:proxmox-rest-server",
"dep:proxmox-router",
"dep:proxmox-tfa",
diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index 428d22a..d9e11f4 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -4,7 +4,8 @@ use anyhow::{bail, format_err, Error};
use serde_json::{json, Value};
use proxmox_rest_server::RestEnvironment;
-use proxmox_router::{http_err, Permission, RpcEnvironment};
+use proxmox_router::{Permission, RpcEnvironment};
+use proxmox_http_error::http_err;
use proxmox_schema::{api, api_types::PASSWORD_SCHEMA};
use proxmox_tfa::api::TfaChallenge;
diff --git a/proxmox-rest-server/Cargo.toml b/proxmox-rest-server/Cargo.toml
index 8daf1d3..4bf2bca 100644
--- a/proxmox-rest-server/Cargo.toml
+++ b/proxmox-rest-server/Cargo.toml
@@ -38,6 +38,7 @@ url.workspace = true
proxmox-async.workspace = true
proxmox-compression.workspace = true
proxmox-http = { workspace = true, optional = true }
+proxmox-http-error.workspace = true
proxmox-io.workspace = true
proxmox-lang.workspace = true
proxmox-router.workspace = true
diff --git a/proxmox-rest-server/src/h2service.rs b/proxmox-rest-server/src/h2service.rs
index 84a3098..08636bc 100644
--- a/proxmox-rest-server/src/h2service.rs
+++ b/proxmox-rest-server/src/h2service.rs
@@ -8,7 +8,7 @@ use std::task::{Context, Poll};
use futures::*;
use hyper::{Body, Request, Response, StatusCode};
-use proxmox_router::http_err;
+use proxmox_http_error::http_err;
use proxmox_router::{ApiResponseFuture, HttpError, Router, RpcEnvironment};
use crate::formatter::*;
diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs
index cffcedc..8db570e 100644
--- a/proxmox-rest-server/src/rest.rs
+++ b/proxmox-rest-server/src/rest.rs
@@ -22,11 +22,11 @@ use tokio::time::Instant;
use tower_service::Service;
use url::form_urlencoded;
+use proxmox_http_error::{http_bail, http_err};
use proxmox_router::{
check_api_permission, ApiHandler, ApiMethod, HttpError, Permission, RpcEnvironment,
RpcEnvironmentType, UserInformation,
};
-use proxmox_router::{http_bail, http_err};
use proxmox_schema::{ObjectSchemaType, ParameterSchema};
use proxmox_async::stream::AsyncReaderStream;
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
deleted file mode 100644
index e285cf7..0000000
--- a/proxmox-router/src/error.rs
+++ /dev/null
@@ -1,44 +0,0 @@
-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)
- }
-}
-
-/// 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..1bb5026 100644
--- a/proxmox-router/src/lib.rs
+++ b/proxmox-router/src/lib.rs
@@ -5,11 +5,6 @@ pub mod format;
#[cfg(feature = "cli")]
pub mod cli;
-// this is public so the `http_err!` macro can access `http::StatusCode` through it
-#[doc(hidden)]
-#[cfg(feature = "server")]
-pub mod error;
-
mod permission;
mod router;
mod rpc_environment;
@@ -17,7 +12,7 @@ mod serializable_return;
#[doc(inline)]
#[cfg(feature = "server")]
-pub use error::HttpError;
+pub use proxmox_http_error::HttpError;
pub use permission::*;
pub use router::*;
--
2.39.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH proxmox 3/5] notify: use HttpError from proxmox-http-error
2023-07-26 12:50 [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 1/5] http-error: add new http-error crate Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use " Lukas Wagner
@ 2023-07-26 12:50 ` Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-perl-rs 4/5] notify: use new HttpError type Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-backup 5/5] use `HttpError` and macros from `proxmox-http-error` crate Lukas Wagner
4 siblings, 0 replies; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 12:50 UTC (permalink / raw)
To: pve-devel, pbs-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH proxmox-perl-rs 4/5] notify: use new HttpError type
2023-07-26 12:50 [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate Lukas Wagner
` (2 preceding siblings ...)
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 3/5] notify: use HttpError from proxmox-http-error Lukas Wagner
@ 2023-07-26 12:50 ` Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-backup 5/5] use `HttpError` and macros from `proxmox-http-error` crate Lukas Wagner
4 siblings, 0 replies; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 12:50 UTC (permalink / raw)
To: pve-devel, pbs-devel
Use `proxmox-http-error::HttpError` instead of
`proxmox-notify::api::ApiError`.
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH proxmox-backup 5/5] use `HttpError` and macros from `proxmox-http-error` crate
2023-07-26 12:50 [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate Lukas Wagner
` (3 preceding siblings ...)
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-perl-rs 4/5] notify: use new HttpError type Lukas Wagner
@ 2023-07-26 12:50 ` Lukas Wagner
2023-07-26 13:42 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
4 siblings, 1 reply; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 12:50 UTC (permalink / raw)
To: pve-devel, pbs-devel
The `HttpError` type from `proxmox-router` has been moved into its
own crate.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Cargo.toml | 3 +++
src/api2/access/openid.rs | 5 ++---
src/api2/access/tfa.rs | 3 ++-
src/api2/admin/datastore.rs | 5 +++--
src/api2/admin/namespace.rs | 3 ++-
src/api2/backup/mod.rs | 5 +++--
src/api2/config/access/ldap.rs | 3 ++-
src/api2/config/access/openid.rs | 3 ++-
src/api2/config/acme.rs | 5 ++---
src/api2/config/changer.rs | 3 ++-
src/api2/config/datastore.rs | 3 ++-
src/api2/config/drive.rs | 3 ++-
src/api2/config/media_pool.rs | 3 ++-
src/api2/config/prune.rs | 3 ++-
src/api2/config/remote.rs | 3 ++-
src/api2/config/sync.rs | 3 ++-
src/api2/config/tape_backup_job.rs | 3 ++-
src/api2/config/tape_encryption_keys.rs | 3 ++-
src/api2/config/traffic_control.rs | 3 ++-
src/api2/config/verify.rs | 3 ++-
src/api2/helpers.rs | 2 +-
src/api2/reader/mod.rs | 5 +++--
src/auth.rs | 2 +-
src/backup/hierarchy.rs | 2 +-
src/server/pull.rs | 4 ++--
25 files changed, 51 insertions(+), 32 deletions(-)
diff --git a/Cargo.toml b/Cargo.toml
index 5e0aca5f..a4d6a8d0 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -60,6 +60,7 @@ proxmox-borrow = "1"
proxmox-compression = "0.2"
proxmox-fuse = "0.1.3"
proxmox-http = { version = "0.9.0", features = [ "client", "http-helpers", "websocket" ] } # see below
+proxmox-http-error = "0.1.0"
proxmox-human-byte = "0.1"
proxmox-io = "1.0.1" # tools and client use "tokio" feature
proxmox-lang = "1.1"
@@ -202,6 +203,7 @@ proxmox-async.workspace = true
proxmox-auth-api = { workspace = true, features = [ "api", "pam-authenticator" ] }
proxmox-compression.workspace = true
proxmox-http = { workspace = true, features = [ "client-trait", "proxmox-async", "rate-limited-stream" ] } # pbs-client doesn't use these
+proxmox-http-error.workspace = true
proxmox-human-byte.workspace = true
proxmox-io.workspace = true
proxmox-lang.workspace = true
@@ -246,6 +248,7 @@ proxmox-rrd.workspace = true
#proxmox-compression = { path = "../proxmox/proxmox-compression" }
#proxmox-fuse = { path = "../proxmox-fuse" }
#proxmox-http = { path = "../proxmox/proxmox-http" }
+#proxmox-http-error = { path = "../proxmox/proxmox-http-error" }
#proxmox-io = { path = "../proxmox/proxmox-io" }
#proxmox-lang = { path = "../proxmox/proxmox-lang" }
#proxmox-rest-server = { path = "../proxmox/proxmox-rest-server" }
diff --git a/src/api2/access/openid.rs b/src/api2/access/openid.rs
index 8e39cbc9..6e703bac 100644
--- a/src/api2/access/openid.rs
+++ b/src/api2/access/openid.rs
@@ -4,9 +4,8 @@ use serde_json::{json, Value};
use proxmox_auth_api::api::ApiTicket;
use proxmox_auth_api::ticket::Ticket;
-use proxmox_router::{
- http_err, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap,
-};
+use proxmox_http_error::http_err;
+use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
use proxmox_schema::api;
use proxmox_sortable_macro::sortable;
diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs
index 589535a6..4182aa18 100644
--- a/src/api2/access/tfa.rs
+++ b/src/api2/access/tfa.rs
@@ -2,7 +2,8 @@
use anyhow::Error;
-use proxmox_router::{http_bail, http_err, Permission, Router, RpcEnvironment};
+use proxmox_http_error::{http_bail, http_err};
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::api;
use proxmox_tfa::api::methods;
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a95031e7..7f4b006f 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -17,9 +17,10 @@ use tokio_stream::wrappers::ReceiverStream;
use proxmox_async::blocking::WrappedReaderStream;
use proxmox_async::{io::AsyncChannelWriter, stream::AsyncReaderStream};
use proxmox_compression::zstd::ZstdEncoder;
+use proxmox_http_error::http_err;
use proxmox_router::{
- http_err, list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission,
- Router, RpcEnvironment, RpcEnvironmentType, SubdirMap,
+ list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission, Router,
+ RpcEnvironment, RpcEnvironmentType, SubdirMap,
};
use proxmox_schema::*;
use proxmox_sortable_macro::sortable;
diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index b2b08ecc..ccf24616 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -2,7 +2,8 @@ use anyhow::{bail, Error};
use serde_json::Value;
use pbs_config::CachedUserInfo;
-use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
use proxmox_schema::*;
use pbs_api_types::{
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 652d5baa..1e31e954 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -9,7 +9,8 @@ use hyper::{Body, Request, Response, StatusCode};
use serde::Deserialize;
use serde_json::{json, Value};
-use proxmox_router::{http_err, list_subdirs_api_method};
+use proxmox_http_error::{http_bail, http_err};
+use proxmox_router::list_subdirs_api_method;
use proxmox_router::{
ApiHandler, ApiMethod, ApiResponseFuture, Permission, Router, RpcEnvironment, SubdirMap,
};
@@ -114,7 +115,7 @@ fn upgrade_to_backup_protocol(
}
if !datastore.namespace_path(&backup_ns).exists() {
- proxmox_router::http_bail!(NOT_FOUND, "namespace not found");
+ http_bail!(NOT_FOUND, "namespace not found");
}
// FIXME: include namespace here?
diff --git a/src/api2/config/access/ldap.rs b/src/api2/config/access/ldap.rs
index 911142a0..e02b33db 100644
--- a/src/api2/config/access/ldap.rs
+++ b/src/api2/config/access/ldap.rs
@@ -4,8 +4,9 @@ use anyhow::{format_err, Error};
use hex::FromHex;
use serde_json::Value;
+use proxmox_http_error::http_bail;
use proxmox_ldap::Connection;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/access/openid.rs b/src/api2/config/access/openid.rs
index 4901880e..e4cb199f 100644
--- a/src/api2/config/access/openid.rs
+++ b/src/api2/config/access/openid.rs
@@ -4,7 +4,8 @@ use anyhow::Error;
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/acme.rs b/src/api2/config/acme.rs
index 1954318b..a5a2804b 100644
--- a/src/api2/config/acme.rs
+++ b/src/api2/config/acme.rs
@@ -10,9 +10,8 @@ use lazy_static::lazy_static;
use serde::{Deserialize, Serialize};
use serde_json::{json, Value};
-use proxmox_router::{
- http_bail, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap,
-};
+use proxmox_http_error::http_bail;
+use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
use proxmox_schema::{api, param_bail};
use proxmox_sys::{task_log, task_warn};
diff --git a/src/api2/config/changer.rs b/src/api2/config/changer.rs
index 01908e3b..e059ef7e 100644
--- a/src/api2/config/changer.rs
+++ b/src/api2/config/changer.rs
@@ -3,7 +3,8 @@ use anyhow::Error;
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 5e013c39..41de8540 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -5,7 +5,8 @@ use anyhow::Error;
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment, RpcEnvironmentType};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
use proxmox_schema::{api, param_bail, ApiType};
use proxmox_section_config::SectionConfigData;
use proxmox_sys::{task_warn, WorkerTaskContext};
diff --git a/src/api2/config/drive.rs b/src/api2/config/drive.rs
index 02589aaf..d7b5147c 100644
--- a/src/api2/config/drive.rs
+++ b/src/api2/config/drive.rs
@@ -3,7 +3,8 @@ use anyhow::{format_err, Error};
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/media_pool.rs b/src/api2/config/media_pool.rs
index 4a4cec56..b9839e7b 100644
--- a/src/api2/config/media_pool.rs
+++ b/src/api2/config/media_pool.rs
@@ -1,7 +1,8 @@
use ::serde::{Deserialize, Serialize};
use anyhow::Error;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/prune.rs b/src/api2/config/prune.rs
index 6f391722..5f19eade 100644
--- a/src/api2/config/prune.rs
+++ b/src/api2/config/prune.rs
@@ -3,7 +3,8 @@ use hex::FromHex;
use serde::{Deserialize, Serialize};
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/remote.rs b/src/api2/config/remote.rs
index 76dd3b89..cae45546 100644
--- a/src/api2/config/remote.rs
+++ b/src/api2/config/remote.rs
@@ -8,7 +8,8 @@ use proxmox_router::SubdirMap;
use proxmox_sortable_macro::sortable;
use serde_json::Value;
-use proxmox_router::{http_bail, http_err, ApiMethod, Permission, Router, RpcEnvironment};
+use proxmox_http_error::{http_bail, http_err};
+use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 01e5f2ce..0bcde927 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -3,7 +3,8 @@ use anyhow::{bail, Error};
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
index 386ff530..f923e894 100644
--- a/src/api2/config/tape_backup_job.rs
+++ b/src/api2/config/tape_backup_job.rs
@@ -3,7 +3,8 @@ use anyhow::Error;
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
index 788ed0e7..c36103e6 100644
--- a/src/api2/config/tape_encryption_keys.rs
+++ b/src/api2/config/tape_encryption_keys.rs
@@ -2,7 +2,8 @@ use anyhow::{bail, format_err, Error};
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/traffic_control.rs b/src/api2/config/traffic_control.rs
index 30ea40ec..57124ef1 100644
--- a/src/api2/config/traffic_control.rs
+++ b/src/api2/config/traffic_control.rs
@@ -3,7 +3,8 @@ use anyhow::Error;
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/config/verify.rs b/src/api2/config/verify.rs
index 82dbe43b..d1edad0e 100644
--- a/src/api2/config/verify.rs
+++ b/src/api2/config/verify.rs
@@ -3,7 +3,8 @@ use anyhow::Error;
use hex::FromHex;
use serde_json::Value;
-use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
+use proxmox_http_error::http_bail;
+use proxmox_router::{Permission, Router, RpcEnvironment};
use proxmox_schema::{api, param_bail};
use pbs_api_types::{
diff --git a/src/api2/helpers.rs b/src/api2/helpers.rs
index 3dc1befc..d9c3c648 100644
--- a/src/api2/helpers.rs
+++ b/src/api2/helpers.rs
@@ -4,7 +4,7 @@ use anyhow::Error;
use futures::stream::TryStreamExt;
use hyper::{header, Body, Response, StatusCode};
-use proxmox_router::http_bail;
+use proxmox_http_error::http_bail;
pub async fn create_download_response(path: PathBuf) -> Result<Response<Body>, Error> {
let file = match tokio::fs::File::open(path.clone()).await {
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index b1a5612b..1ef9334a 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -9,9 +9,10 @@ use hyper::{Body, Request, Response, StatusCode};
use serde::Deserialize;
use serde_json::Value;
+use proxmox_http_error::http_err;
use proxmox_router::{
- http_err, list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission,
- Router, RpcEnvironment, SubdirMap,
+ list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission, Router,
+ RpcEnvironment, SubdirMap,
};
use proxmox_schema::{BooleanSchema, ObjectSchema};
use proxmox_sortable_macro::sortable;
diff --git a/src/auth.rs b/src/auth.rs
index 318d1ff2..d8e56df0 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -9,13 +9,13 @@ use std::pin::Pin;
use anyhow::{bail, Error};
use futures::Future;
use once_cell::sync::{Lazy, OnceCell};
-use proxmox_router::http_bail;
use serde_json::json;
use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
use proxmox_auth_api::ticket::{Empty, Ticket};
use proxmox_auth_api::types::Authid;
use proxmox_auth_api::Keyring;
+use proxmox_http_error::http_bail;
use proxmox_ldap::{Config, Connection, ConnectionMode};
use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
index 640a7762..204aee6e 100644
--- a/src/backup/hierarchy.rs
+++ b/src/backup/hierarchy.rs
@@ -62,7 +62,7 @@ pub fn check_ns_privs_full(
let priv_names = privs_to_priv_names(full_access_privs | partial_access_privs).join("|");
let path = format!("/{}", acl_path.join("/"));
- proxmox_router::http_bail!(
+ proxmox_http_error::http_bail!(
FORBIDDEN,
"permission check failed - missing {priv_names} on {path}"
);
diff --git a/src/server/pull.rs b/src/server/pull.rs
index a973a10e..97c306f8 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -386,7 +386,7 @@ async fn pull_snapshot(
Ok(manifest_file) => manifest_file,
Err(err) => {
match err.downcast_ref::<HttpError>() {
- Some(HttpError { code, message }) => match *code {
+ Some(HttpError { code, message, .. }) => match *code {
StatusCode::NOT_FOUND => {
task_log!(
worker,
@@ -805,7 +805,7 @@ async fn query_namespaces(
let mut result = match client.get(&path, Some(data)).await {
Ok(res) => res,
Err(err) => match err.downcast_ref::<HttpError>() {
- Some(HttpError { code, message }) => match *code {
+ Some(HttpError { code, message, .. }) => match *code {
StatusCode::NOT_FOUND => {
if params.remote_ns.is_root() && params.max_depth.is_none() {
task_log!(worker, "Could not query remote for namespaces (404) -> temporarily switching to backwards-compat mode");
--
2.39.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pve-devel] [pbs-devel] [PATCH proxmox 1/5] http-error: add new http-error crate
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 1/5] http-error: add new http-error crate Lukas Wagner
@ 2023-07-26 13:35 ` Wolfgang Bumiller
0 siblings, 0 replies; 10+ messages in thread
From: Wolfgang Bumiller @ 2023-07-26 13:35 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel, pbs-devel
On Wed, Jul 26, 2023 at 02:50:02PM +0200, Lukas Wagner wrote:
> 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..17be2e0
> --- /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, features = ["derive"]}
^ No need for the [derive] here if you don't use it ;-)
> 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
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pve-devel] [pbs-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use new http-error crate
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use " Lukas Wagner
@ 2023-07-26 13:41 ` Wolfgang Bumiller
2023-07-26 13:45 ` Lukas Wagner
0 siblings, 1 reply; 10+ messages in thread
From: Wolfgang Bumiller @ 2023-07-26 13:41 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel, pbs-devel
I'd like `proxmox-router` to keep re-exporting both the `http_bail/err`
macros and the `HttpError` type. This would require much fewer changes
at all the call sites, and we don't need to explicitly depend on the new
crate everywhere. Its point is mostly that perlmod code doesn't need to
pull the router into our perl code base.
On Wed, Jul 26, 2023 at 02:50:03PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> proxmox-auth-api/Cargo.toml | 2 ++
> proxmox-auth-api/src/api/access.rs | 3 +-
> proxmox-rest-server/Cargo.toml | 1 +
> proxmox-rest-server/src/h2service.rs | 2 +-
> proxmox-rest-server/src/rest.rs | 2 +-
> proxmox-router/Cargo.toml | 1 +
> proxmox-router/src/error.rs | 44 ----------------------------
> proxmox-router/src/lib.rs | 7 +----
> 8 files changed, 9 insertions(+), 53 deletions(-)
> delete mode 100644 proxmox-router/src/error.rs
>
> diff --git a/proxmox-auth-api/Cargo.toml b/proxmox-auth-api/Cargo.toml
> index bd1884a..45d7cbc 100644
> --- a/proxmox-auth-api/Cargo.toml
> +++ b/proxmox-auth-api/Cargo.toml
Effectively this crate wouldn't need to be touched at all.
> @@ -28,6 +28,7 @@ serde = { workspace = true, optional = true, features = [ "derive" ] }
> serde_json = { workspace = true, optional = true }
> serde_plain = { workspace = true, optional = true }
>
> +proxmox-http-error = { workspace = true, optional = true }
> proxmox-rest-server = { workspace = true, optional = true }
> proxmox-router = { workspace = true, optional = true }
> proxmox-schema = { workspace = true, optional = true, features = [ "api-macro", "api-types" ] }
> @@ -45,6 +46,7 @@ api = [
> "dep:http",
> "dep:serde_json",
>
> + "dep:proxmox-http-error",
> "dep:proxmox-rest-server",
> "dep:proxmox-router",
> "dep:proxmox-tfa",
> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
> index 428d22a..d9e11f4 100644
> --- a/proxmox-auth-api/src/api/access.rs
> +++ b/proxmox-auth-api/src/api/access.rs
> @@ -4,7 +4,8 @@ use anyhow::{bail, format_err, Error};
> use serde_json::{json, Value};
>
> use proxmox_rest_server::RestEnvironment;
> -use proxmox_router::{http_err, Permission, RpcEnvironment};
> +use proxmox_router::{Permission, RpcEnvironment};
> +use proxmox_http_error::http_err;
> use proxmox_schema::{api, api_types::PASSWORD_SCHEMA};
> use proxmox_tfa::api::TfaChallenge;
>
> diff --git a/proxmox-rest-server/Cargo.toml b/proxmox-rest-server/Cargo.toml
> index 8daf1d3..4bf2bca 100644
> --- a/proxmox-rest-server/Cargo.toml
> +++ b/proxmox-rest-server/Cargo.toml
Nor this one.
> @@ -38,6 +38,7 @@ url.workspace = true
> proxmox-async.workspace = true
> proxmox-compression.workspace = true
> proxmox-http = { workspace = true, optional = true }
> +proxmox-http-error.workspace = true
> proxmox-io.workspace = true
> proxmox-lang.workspace = true
> proxmox-router.workspace = true
> diff --git a/proxmox-rest-server/src/h2service.rs b/proxmox-rest-server/src/h2service.rs
> index 84a3098..08636bc 100644
> --- a/proxmox-rest-server/src/h2service.rs
> +++ b/proxmox-rest-server/src/h2service.rs
> @@ -8,7 +8,7 @@ use std::task::{Context, Poll};
> use futures::*;
> use hyper::{Body, Request, Response, StatusCode};
>
> -use proxmox_router::http_err;
> +use proxmox_http_error::http_err;
> use proxmox_router::{ApiResponseFuture, HttpError, Router, RpcEnvironment};
>
> use crate::formatter::*;
> diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs
> index cffcedc..8db570e 100644
> --- a/proxmox-rest-server/src/rest.rs
> +++ b/proxmox-rest-server/src/rest.rs
> @@ -22,11 +22,11 @@ use tokio::time::Instant;
> use tower_service::Service;
> use url::form_urlencoded;
>
> +use proxmox_http_error::{http_bail, http_err};
> use proxmox_router::{
> check_api_permission, ApiHandler, ApiMethod, HttpError, Permission, RpcEnvironment,
> RpcEnvironmentType, UserInformation,
> };
> -use proxmox_router::{http_bail, http_err};
> use proxmox_schema::{ObjectSchemaType, ParameterSchema};
>
> use proxmox_async::stream::AsyncReaderStream;
<snip>
basically all of the above could be dropped, and only the remaining
parts below together with re-exports are required.
Unless I'm missing something.
> 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
> deleted file mode 100644
> index e285cf7..0000000
> --- a/proxmox-router/src/error.rs
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -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)
> - }
> -}
> -
> -/// 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..1bb5026 100644
> --- a/proxmox-router/src/lib.rs
> +++ b/proxmox-router/src/lib.rs
> @@ -5,11 +5,6 @@ pub mod format;
> #[cfg(feature = "cli")]
> pub mod cli;
>
> -// this is public so the `http_err!` macro can access `http::StatusCode` through it
> -#[doc(hidden)]
> -#[cfg(feature = "server")]
> -pub mod error;
> -
> mod permission;
> mod router;
> mod rpc_environment;
> @@ -17,7 +12,7 @@ mod serializable_return;
>
> #[doc(inline)]
> #[cfg(feature = "server")]
> -pub use error::HttpError;
> +pub use proxmox_http_error::HttpError;
>
> pub use permission::*;
> pub use router::*;
> --
> 2.39.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pve-devel] [pbs-devel] [PATCH proxmox-backup 5/5] use `HttpError` and macros from `proxmox-http-error` crate
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-backup 5/5] use `HttpError` and macros from `proxmox-http-error` crate Lukas Wagner
@ 2023-07-26 13:42 ` Wolfgang Bumiller
0 siblings, 0 replies; 10+ messages in thread
From: Wolfgang Bumiller @ 2023-07-26 13:42 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel, pbs-devel
I think with the re-exports we should be able to drop this patch
entirely.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pve-devel] [pbs-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use new http-error crate
2023-07-26 13:41 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
@ 2023-07-26 13:45 ` Lukas Wagner
0 siblings, 0 replies; 10+ messages in thread
From: Lukas Wagner @ 2023-07-26 13:45 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel, pbs-devel
On 7/26/23 15:41, Wolfgang Bumiller wrote:
> I'd like `proxmox-router` to keep re-exporting both the `http_bail/err`
> macros and the `HttpError` type. This would require much fewer changes
> at all the call sites, and we don't need to explicitly depend on the new
> crate everywhere. Its point is mostly that perlmod code doesn't need to
> pull the router into our perl code base.
Alright, I'll send a v2 with the things re-exported.
Thanks!
--
- Lukas
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-07-26 13:45 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-26 12:50 [pve-devel] [PATCH proxmox{, -perl-rs, -backup} 0/5] move `HttpError` from `proxmox-router` into its own crate Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 1/5] http-error: add new http-error crate Lukas Wagner
2023-07-26 13:35 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use " Lukas Wagner
2023-07-26 13:41 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
2023-07-26 13:45 ` Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox 3/5] notify: use HttpError from proxmox-http-error Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-perl-rs 4/5] notify: use new HttpError type Lukas Wagner
2023-07-26 12:50 ` [pve-devel] [PATCH proxmox-backup 5/5] use `HttpError` and macros from `proxmox-http-error` crate Lukas Wagner
2023-07-26 13:42 ` [pve-devel] [pbs-devel] " Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox