Re: [pbs-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use new http-error crate

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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