Miles WardandClaude Opus 4.8
674a69f93b
Final cleanup before hardware testing. No behaviour changes; 248 tests green, clippy clean. #1 AppError newtype (http-api/src/error.rs) with one IntoResponse mapping (NotFound→404, Invalid→400, _→500) + From<core::Error>/From<io::Error>. Converted the clearly-safe handlers (sso_put, unattended_upload, branding_clear) to `?`; intentionally left handlers with bespoke status semantics (Invalid→404 on category, 409 on duplicate share / open upload) explicit so no asserted status changes. #2 figment-based Config::load (defaults → TOML → env). Keeps the historical flat OPENPXE_* names (Unraid/entrypoint compatible) AND adds the nested OPENPXE_SECTION__FIELD form; now covers every field (apply_env had silently skipped unattended_dir + bind addrs). 6 Jail tests prove backward-compat. Removed the hand-rolled apply_env. #3 thiserror 1→2; dropped unused mime/mime_guess/once_cell deps. #4 Re-evaluated: Duration::from_hours/from_mins are stable on the pinned 1.95 toolchain and clippy prefers them — kept the readable form (the "unstable" premise didn't hold; MSRV is intentionally 1.95). #5 insta snapshot of the rendered iPXE menu (version-filtered) + wiremock coverage of the SAML metadata-URL fetch (200 + non-2xx). #6 api_status → typed StatusResponse struct (was a 25-key json! blob) with a full_flow guard test asserting every UI key + the started_at string shape. Deferred the /api/docs typed conversion (lowest value, highest churn, zero functional benefit). #7 pct_encode/xml_escape de-duplicated into openpxe_core::encoding (were copied across app.rs + the SAML modules). No new crates. #8 UploadSessions registry → parking_lot::RwLock (sync, never held across .await); per-session lock stays tokio::Mutex. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
93 lines
3.3 KiB
Rust
93 lines
3.3 KiB
Rust
//! Uniform HTTP error mapping for the API layer (v0.5.4).
|
|
//!
|
|
//! Before this, ~40 handlers in `app.rs` hand-wrote
|
|
//! `match … { Err(e) => (StatusCode::…, format!("{e}")).into_response() }`,
|
|
//! and the `openpxe_core::Error` → status mapping drifted between them
|
|
//! (e.g. `Invalid` → 400 in most places, 404 in one). [`AppError`] wraps
|
|
//! `openpxe_core::Error` so a handler can return `Result<T, AppError>` and
|
|
//! `?` its way out, getting one consistent status + body. The body stays
|
|
//! plain-text (matching the previous `(StatusCode, String)` responses) so
|
|
//! existing clients and tests see no shape change; 5xx detail is logged
|
|
//! and returned verbatim exactly as before.
|
|
//!
|
|
//! Handlers with *intentional* domain-specific statuses (e.g. a duplicate
|
|
//! share → 409, a still-open chunked upload → 409) keep their explicit
|
|
//! returns — `AppError` is for the common case, not a straitjacket.
|
|
|
|
use axum::http::StatusCode;
|
|
use axum::response::{IntoResponse, Response};
|
|
use openpxe_core::Error as CoreError;
|
|
|
|
/// Newtype over [`openpxe_core::Error`] with a uniform [`IntoResponse`].
|
|
#[derive(Debug)]
|
|
pub struct AppError(pub CoreError);
|
|
|
|
impl From<CoreError> for AppError {
|
|
fn from(e: CoreError) -> Self {
|
|
AppError(e)
|
|
}
|
|
}
|
|
|
|
impl From<std::io::Error> for AppError {
|
|
fn from(e: std::io::Error) -> Self {
|
|
AppError(CoreError::Io(e))
|
|
}
|
|
}
|
|
|
|
impl AppError {
|
|
/// The HTTP status this error maps to. Public so handlers (and tests)
|
|
/// can reason about the mapping in one place.
|
|
#[must_use]
|
|
pub fn status(&self) -> StatusCode {
|
|
match self.0 {
|
|
CoreError::NotFound(_) => StatusCode::NOT_FOUND,
|
|
CoreError::Invalid(_) => StatusCode::BAD_REQUEST,
|
|
CoreError::Config(_) | CoreError::Io(_) | CoreError::Other(_) => {
|
|
StatusCode::INTERNAL_SERVER_ERROR
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
impl IntoResponse for AppError {
|
|
fn into_response(self) -> Response {
|
|
let status = self.status();
|
|
// Match the prior hand-written responses: the 4xx arms returned the
|
|
// bare inner message (not the `Display` prefix), so a UI showing
|
|
// `await r.text()` reads "metadata too long", not "invalid input:
|
|
// metadata too long". 5xx keeps the full `Display` string.
|
|
let body = match &self.0 {
|
|
CoreError::Invalid(m) | CoreError::NotFound(m) => m.clone(),
|
|
other => other.to_string(),
|
|
};
|
|
if status.is_server_error() {
|
|
// Log the full detail server-side; the body still carries it
|
|
// (unchanged from the prior `format!("{e}")` behaviour), but the
|
|
// log line is what an operator greps for.
|
|
tracing::error!(target: "openpxe::http", error = %self.0, "request failed");
|
|
}
|
|
(status, body).into_response()
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
#[test]
|
|
fn status_mapping_is_consistent() {
|
|
assert_eq!(
|
|
AppError(CoreError::NotFound("x".into())).status(),
|
|
StatusCode::NOT_FOUND
|
|
);
|
|
assert_eq!(
|
|
AppError(CoreError::Invalid("x".into())).status(),
|
|
StatusCode::BAD_REQUEST
|
|
);
|
|
assert_eq!(
|
|
AppError(CoreError::Config("x".into())).status(),
|
|
StatusCode::INTERNAL_SERVER_ERROR
|
|
);
|
|
}
|
|
}
|