From 64739778c47ae32424e20d417dc2e2d36ae3c5f9 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Thu, 27 Aug 2026 05:57:13 +0800 Subject: [PATCH] refactor(admin): consolidate json_response, empty_response, and extract_query_params into admin utils (#6694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin surface had accumulated one near-identical response helper per handler file. This folds the byte-equivalent ones into `rustfs/src/admin/utils.rs` so the wire shape of an admin JSON answer is pinned in one place instead of being re-derived twelve times. Folded into `crate::admin::utils`: - `json_response(status, &value)` — 9 local definitions removed: batch_job.rs, kms_backup.rs, oidc.rs, diagnostics.rs (identical signature), object_data_cache.rs and site_replication.rs (hard-coded `StatusCode::OK`, whose call sites now pass `StatusCode::OK` explicitly), ilm_transition.rs (arguments were `(&value, status)` and are swapped at every call site), and kms_key_metadata.rs / kms_key_lifecycle.rs (concrete response types now covered by the generic helper). - `empty_response(status)` — 2 local definitions removed: site_replication.rs (`Body::empty()`) and table_catalog/mod.rs (`Body::default()`); `Body::empty()` is defined as `Body::default()`, so the two were already the same response. - `extract_query_params(uri)` — 4 local definitions removed: kms_keys.rs (was `pub(super)`), replication.rs, batch_job.rs, config_admin.rs. All four bodies were behaviourally identical (`form_urlencoded::parse` over `uri.query()`, last-wins on repeated keys, valueless parameters kept as empty strings); they differed only in blank lines. kms_key_lifecycle.rs, which imported the kms_keys copy, now imports the shared one. Intentionally left alone: - heal.rs `json_response` — different shape: returns a bare `S3Response` (not `S3Result`) and additionally sets `CONTENT_LENGTH`. - kms_rekey.rs `json_response` — same divergent shape as heal.rs: bare `S3Response` over already-serialized `Vec`. - idp_compat.rs `json_response` — encrypts the payload via `encode_compatible_admin_payload`; it is not a duplicate of the plain JSON helper. - scanner.rs `json_response` — takes raw `Vec`, and `ScannerCycleStateResetHandler` genuinely passes a byte literal rather than a serializable value, so the local helper stays. - oidc.rs `extract_query_param` — singular, returns `Option` for one key, hand-rolls its own splitting via the `urlencoding` crate; a different function, not a variant of the map builder. Wire behaviour on the success path is byte-identical everywhere: same status, same `Content-Type: application/json` (every local copy spelled the same value, whether via a per-file `JSON_CONTENT_TYPE`/`CONTENT_TYPE_JSON` constant, `HeaderValue::from_static`, or `"application/json".parse()`), same serialized body bytes, and no other header. The only behavioural change is the message text on the serde-serialization-failure arm, which is now uniformly `failed to serialize response: {e}`; that arm is unreachable for these owned response structs and the acceptance criteria pin only status and content type. No `include_str!` self-grep assertion needed updating: the affected tests in ilm_transition.rs, site_replication.rs, kms_keys.rs, kms_key_metadata.rs, kms_key_lifecycle.rs, object_data_cache.rs, and table_catalog/tests.rs are all bounded by handler `impl Operation` / entry-point markers that sit well after the removed helpers, and none of them assert on a `json_response`, `empty_response`, or `extract_query_params` string. Tests: `rustfs/src/admin/utils.rs` gains `json_response_carries_status_content_type_and_serialized_body`, `json_response_reports_serialization_failure_as_internal_error`, `empty_response_has_no_body_and_no_headers`, `extract_query_params_decodes_percent_escapes`, and `extract_query_params_keeps_valueless_parameters_and_survives_no_query`. The percent-decoding coverage previously in batch_job's `extract_query_params_decodes_job_id` moves there, and batch_job keeps its own end-to-end coverage as `require_job_id_decodes_and_rejects_missing_and_empty`. Reference: rustfs/backlog#1829 T6 --- rustfs/src/admin/handlers/batch_job.rs | 38 +-- rustfs/src/admin/handlers/config_admin.rs | 21 +- rustfs/src/admin/handlers/diagnostics.rs | 13 +- rustfs/src/admin/handlers/ilm_transition.rs | 32 +-- rustfs/src/admin/handlers/kms_backup.rs | 11 +- .../src/admin/handlers/kms_key_lifecycle.rs | 13 +- rustfs/src/admin/handlers/kms_key_metadata.rs | 11 +- rustfs/src/admin/handlers/kms_keys.rs | 17 +- .../src/admin/handlers/object_data_cache.rs | 36 ++- rustfs/src/admin/handlers/oidc.rs | 11 +- rustfs/src/admin/handlers/replication.rs | 16 +- rustfs/src/admin/handlers/site_replication.rs | 225 ++++++++++-------- .../src/admin/handlers/table_catalog/mod.rs | 5 +- rustfs/src/admin/utils.rs | 137 ++++++++++- 14 files changed, 314 insertions(+), 272 deletions(-) diff --git a/rustfs/src/admin/handlers/batch_job.rs b/rustfs/src/admin/handlers/batch_job.rs index ca3f0e07d..1170d3dde 100644 --- a/rustfs/src/admin/handlers/batch_job.rs +++ b/rustfs/src/admin/handlers/batch_job.rs @@ -37,18 +37,16 @@ use crate::admin::auth::authorize_admin_request; use crate::admin::router::{AdminOperation, Operation, S3Router}; -use crate::admin::utils::read_compatible_admin_body; +use crate::admin::utils::{extract_query_params, json_response, read_compatible_admin_body}; use crate::server::ADMIN_PREFIX; -use http::{HeaderMap, HeaderValue, Uri}; +use http::Uri; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::MAX_ADMIN_REQUEST_BODY_SIZE; use rustfs_credentials::Credentials; use rustfs_policy::policy::action::{Action, AdminAction}; -use s3s::header::CONTENT_TYPE; use s3s::{Body, S3Error, S3ErrorCode, S3Request, S3Response, S3Result, s3_error}; use serde::Serialize; -use std::collections::HashMap; use tracing::warn; /// Job types recognised by the MinIO batch-job admin API. @@ -58,27 +56,10 @@ use tracing::warn; /// (`NotImplemented`) from "unknown job type" (`InvalidRequest`). const KNOWN_JOB_TYPES: &[&str] = &["replicate", "keyrotate", "expire"]; -fn extract_query_params(uri: &Uri) -> HashMap { - let mut params = HashMap::new(); - if let Some(query) = uri.query() { - for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { - params.insert(key.into_owned(), value.into_owned()); - } - } - params -} - async fn validate_batch_job_admin_request(req: &S3Request, action: AdminAction) -> S3Result { authorize_admin_request(req, vec![Action::AdminAction(action)]).await } -fn json_response(status: StatusCode, value: &T) -> S3Result> { - let data = serde_json::to_vec(value).map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("{e}")))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - Ok(S3Response::with_headers((status, Body::from(data)), headers)) -} - /// Best-effort detection of the declared job type from a MinIO batch-job /// definition body. /// @@ -269,7 +250,7 @@ fn no_such_job(job_id: &str) -> S3Error { #[cfg(test)] mod tests { - use super::{detect_job_type, extract_query_params, require_job_id}; + use super::{detect_job_type, require_job_id}; use http::Uri; #[test] @@ -302,17 +283,16 @@ mod tests { assert_eq!(detect_job_type(b"transmogrify:\n foo: bar\n"), None); } + /// `jobId` reaches the handler percent-decoded — the shared query parser + /// is covered in `crate::admin::utils`; this pins the endpoint's own use + /// of it, including the rejection of a missing or empty id. #[test] - fn extract_query_params_decodes_job_id() { - let uri: Uri = "/rustfs/admin/v3/status-job?jobId=abc%2F123" + fn require_job_id_decodes_and_rejects_missing_and_empty() { + let encoded: Uri = "/rustfs/admin/v3/status-job?jobId=abc%2F123" .parse() .expect("uri should parse"); - let params = extract_query_params(&uri); - assert_eq!(params.get("jobId"), Some(&"abc/123".to_string())); - } + assert_eq!(require_job_id(&encoded).expect("job id"), "abc/123"); - #[test] - fn require_job_id_rejects_missing_and_empty() { let missing: Uri = "/rustfs/admin/v3/status-job".parse().expect("uri should parse"); assert!(require_job_id(&missing).is_err()); diff --git a/rustfs/src/admin/handlers/config_admin.rs b/rustfs/src/admin/handlers/config_admin.rs index 82096b741..15bcd7763 100644 --- a/rustfs/src/admin/handlers/config_admin.rs +++ b/rustfs/src/admin/handlers/config_admin.rs @@ -30,10 +30,12 @@ use crate::admin::storage_api::config::{ save_admin_server_config_snapshot, }; use crate::admin::storage_api::contract::list::ListOperations as _; -use crate::admin::utils::{encode_compatible_admin_payload, is_compat_admin_request, read_compatible_admin_body}; +use crate::admin::utils::{ + encode_compatible_admin_payload, extract_query_params, is_compat_admin_request, read_compatible_admin_body, +}; use crate::error::ApiError; use crate::server::ADMIN_PREFIX; -use http::{HeaderMap, HeaderValue, Uri}; +use http::{HeaderMap, HeaderValue}; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::audit::{ @@ -81,7 +83,7 @@ use rustfs_policy::policy::action::{Action, AdminAction}; use s3s::header::CONTENT_TYPE; use s3s::{Body, S3Error, S3ErrorCode, S3Request, S3Response, S3Result, s3_error}; use serde::Serialize; -use std::collections::{BTreeSet, HashMap}; +use std::collections::BTreeSet; use std::env; use std::mem::size_of; use time::OffsetDateTime; @@ -664,18 +666,6 @@ pub fn register_config_route(r: &mut S3Router) -> std::io::Resul Ok(()) } -fn extract_query_params(uri: &Uri) -> HashMap { - let mut params = HashMap::new(); - - if let Some(query) = uri.query() { - for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { - params.insert(key.into_owned(), value.into_owned()); - } - } - - params -} - async fn validate_config_admin_request(req: &S3Request) -> S3Result { // Pre-check keeps this endpoint's historical missing-credentials message; // the shared gate reports "get cred failed". @@ -2282,6 +2272,7 @@ impl Operation for SetConfigHandler { #[cfg(test)] mod tests { use super::*; + use http::Uri; use serial_test::serial; use temp_env::with_vars; diff --git a/rustfs/src/admin/handlers/diagnostics.rs b/rustfs/src/admin/handlers/diagnostics.rs index 37095a619..18758b2fc 100644 --- a/rustfs/src/admin/handlers/diagnostics.rs +++ b/rustfs/src/admin/handlers/diagnostics.rs @@ -26,6 +26,7 @@ use crate::admin::auth::authorize_admin_request; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::admin::storage_api::access::spawn_traced; +use crate::admin::utils::json_response; use crate::server::ADMIN_PREFIX; use crate::storage::storage_api::get_global_lock_clients; use bytes::Bytes; @@ -37,7 +38,7 @@ use rustfs_lock::{LockLeaseInfo, LockMode, LockType, ObjectKey, get_global_lock_ use rustfs_policy::policy::action::{Action, AdminAction}; use s3s::header::CONTENT_TYPE; use s3s::stream::{ByteStream, DynByteStream}; -use s3s::{Body, S3Error, S3ErrorCode, S3Request, S3Response, S3Result, StdError, s3_error}; +use s3s::{Body, S3Error, S3Request, S3Response, S3Result, StdError, s3_error}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::pin::Pin; @@ -48,7 +49,6 @@ use tokio::sync::{Semaphore, SemaphorePermit, mpsc}; use tokio_stream::wrappers::ReceiverStream; use tracing::warn; -const CONTENT_TYPE_JSON: &str = "application/json"; const CONTENT_TYPE_NDJSON: &str = "application/x-ndjson"; pub(crate) const CLIENT_DEVNULL_MAX_BYTES: u64 = 1024 * 1024 * 1024; pub(crate) const CLIENT_DEVNULL_MAX_DURATION: Duration = Duration::from_secs(30); @@ -143,14 +143,6 @@ async fn authorize(req: &S3Request, action: AdminAction) -> S3Result<()> { Ok(()) } -fn json_response(status: StatusCode, value: &T) -> S3Result> { - let data = serde_json::to_vec(value) - .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("failed to serialize response: {e}")))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, HeaderValue::from_static(CONTENT_TYPE_JSON)); - Ok(S3Response::with_headers((status, Body::from(data)), headers)) -} - async fn read_body(input: Body) -> S3Result> { let mut input = input; let body = input @@ -1061,6 +1053,7 @@ fn query_values(uri: &Uri, key: &str) -> Vec { mod tests { use super::*; use http::{Extensions, Uri}; + use s3s::S3ErrorCode; fn build_request(method: Method, uri: &'static str) -> S3Request { S3Request { diff --git a/rustfs/src/admin/handlers/ilm_transition.rs b/rustfs/src/admin/handlers/ilm_transition.rs index 798242439..c418af8b3 100644 --- a/rustfs/src/admin/handlers/ilm_transition.rs +++ b/rustfs/src/admin/handlers/ilm_transition.rs @@ -30,8 +30,9 @@ use crate::admin::storage_api::lifecycle::{ request_manual_transition_job_cancel, save_manual_transition_job_record, update_manual_transition_job_record, }; use crate::admin::storage_api::runtime::ECStore; +use crate::admin::utils::json_response; use crate::server::{ADMIN_PREFIX, RemoteAddr}; -use http::{HeaderMap, HeaderValue}; +use http::HeaderMap; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::MAX_ADMIN_REQUEST_BODY_SIZE; @@ -40,7 +41,6 @@ use rustfs_utils::{ MaskedAccessKey, http::{AMZ_REQUEST_ID, REQUEST_ID_HEADER}, }; -use s3s::header::CONTENT_TYPE; use s3s::{Body, S3Error, S3ErrorCode, S3Request, S3Response, S3Result, s3_error}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -51,7 +51,6 @@ use tokio_util::sync::CancellationToken; use tracing::{error, info, warn}; use uuid::Uuid; -const JSON_CONTENT_TYPE: &str = "application/json"; const DEFAULT_MANUAL_TRANSITION_MAX_OBJECTS: u64 = 10_000; const MAX_MANUAL_TRANSITION_OBJECTS: u64 = 100_000; const MAX_MANUAL_TRANSITION_DURATION_SECONDS: u64 = 3600; @@ -629,17 +628,6 @@ fn map_manual_transition_job_load_error(err: StorageError, job_id: Uuid) -> S3Er } } -fn json_response(response: &T, status: StatusCode) -> S3Result> { - let body = serde_json::to_vec(response).map_err(|err| { - S3Error::with_message(S3ErrorCode::InternalError, format!("failed to encode manual transition response: {err}")) - })?; - let content_type = HeaderValue::from_str(JSON_CONTENT_TYPE) - .map_err(|err| S3Error::with_message(S3ErrorCode::InternalError, format!("invalid content type: {err}")))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, content_type); - Ok(S3Response::with_headers((status, Body::from(body)), headers)) -} - async fn update_manual_transition_job_record_if_owned( store: Arc, job_id: Uuid, @@ -921,10 +909,10 @@ impl Operation for ManualTransitionRunHandler { cancel_endpoint: Some(status_endpoint), report: record.report, }; - return json_response(&response, StatusCode::ACCEPTED); + return json_response(StatusCode::ACCEPTED, &response); } StartManualTransitionJobResult::Conflict(response) => { - return json_response(&response, StatusCode::CONFLICT); + return json_response(StatusCode::CONFLICT, &response); } } } @@ -960,7 +948,7 @@ impl Operation for ManualTransitionRunHandler { report, }; - json_response(&response, StatusCode::OK) + json_response(StatusCode::OK, &response) } } @@ -1000,7 +988,7 @@ impl Operation for ManualTransitionJobStatusHandler { release_manual_transition_admission(store, &record); } } - json_response(&manual_transition_job_response(record), StatusCode::OK) + json_response(StatusCode::OK, &manual_transition_job_response(record)) } } @@ -1022,7 +1010,7 @@ impl Operation for ManualTransitionJobCancelHandler { { cancel_token.cancel(); } - json_response(&manual_transition_job_response(record), StatusCode::OK) + json_response(StatusCode::OK, &manual_transition_job_response(record)) } } @@ -1039,7 +1027,7 @@ impl Operation for TransitionReconcileInspectHandler { let status = inspect_transition_transaction_for_operator(store, transaction_id) .await .map_err(map_transition_operator_error)?; - json_response(&status, StatusCode::OK) + json_response(StatusCode::OK, &status) } } @@ -1074,7 +1062,7 @@ impl Operation for TransitionReconcileApplyHandler { "exact_delete_completed_journal_already_finalized" }; log_transition_reconcile_applied(transaction_id, "delete_candidate", outcome, &request_id, &actor, &remote_addr); - json_response(&TransitionCandidateDeleteResponse { outcome, result }, StatusCode::OK) + json_response(StatusCode::OK, &TransitionCandidateDeleteResponse { outcome, result }) } ValidatedTransitionReconcileAction::FinalizeMissing => { finalize_missing_transition_transaction_for_operator(store, transaction_id) @@ -1089,12 +1077,12 @@ impl Operation for TransitionReconcileApplyHandler { &remote_addr, ); json_response( + StatusCode::OK, &TransitionFinalizeMissingResponse { outcome: "journal_finalized", journal_retained: false, transaction_id, }, - StatusCode::OK, ) } } diff --git a/rustfs/src/admin/handlers/kms_backup.rs b/rustfs/src/admin/handlers/kms_backup.rs index f94651874..ba047b77d 100644 --- a/rustfs/src/admin/handlers/kms_backup.rs +++ b/rustfs/src/admin/handlers/kms_backup.rs @@ -44,10 +44,11 @@ use super::kms_audit::{KmsAdminAudit, KmsAdminOperation}; use crate::admin::auth::validate_admin_request; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::admin::runtime_sources::{current_deployment_id, current_kms_runtime_service_manager}; +use crate::admin::utils::json_response; use crate::auth::{check_key_valid, get_session_token}; use crate::server::{ADMIN_PREFIX, RemoteAddr}; use base64_simd::STANDARD as BASE64; -use hyper::{HeaderMap, Method, StatusCode}; +use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::MAX_ADMIN_REQUEST_BODY_SIZE; use rustfs_kms::backup::{ @@ -58,7 +59,6 @@ use rustfs_kms::backup::{ use rustfs_kms::config::{BackendConfig, KmsConfig, LocalConfig, VaultAuthMethod}; use rustfs_kms::{KmsBackend, KmsManager, KmsServiceManager, KmsServiceStatus}; use rustfs_policy::policy::action::{Action, KmsAction}; -use s3s::header::CONTENT_TYPE; use s3s::{Body, S3Request, S3Response, S3Result, s3_error}; use serde::{Deserialize, Serialize}; use std::path::PathBuf; @@ -890,13 +890,6 @@ async fn backup_status() -> KmsBackupStatusResponse { // HTTP plumbing // --------------------------------------------------------------------------- -fn json_response(status: StatusCode, value: &T) -> S3Result> { - let data = serde_json::to_vec(value).map_err(|e| s3_error!(InternalError, "failed to serialize response: {}", e))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, "application/json".parse().expect("static content type should parse")); - Ok(S3Response::with_headers((status, Body::from(data)), headers)) -} - #[derive(Debug, Serialize)] struct ErrorResponse { success: bool, diff --git a/rustfs/src/admin/handlers/kms_key_lifecycle.rs b/rustfs/src/admin/handlers/kms_key_lifecycle.rs index 32fbdf43c..93ad7834e 100644 --- a/rustfs/src/admin/handlers/kms_key_lifecycle.rs +++ b/rustfs/src/admin/handlers/kms_key_lifecycle.rs @@ -15,13 +15,14 @@ //! KMS key lifecycle admin API handlers: enable, disable and rotate. use super::kms_audit::KmsAdminAudit; -use super::kms_keys::{extract_query_params, scoped_key_id}; +use super::kms_keys::scoped_key_id; use crate::admin::auth::validate_admin_request_with_kms_key; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::admin::runtime_sources::current_kms_runtime_service_manager; +use crate::admin::utils::{extract_query_params, json_response}; use crate::auth::{check_key_valid, get_session_token}; use crate::server::{ADMIN_PREFIX, RemoteAddr}; -use hyper::{HeaderMap, Method, StatusCode}; +use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::MAX_ADMIN_REQUEST_BODY_SIZE; use rustfs_kms::{ @@ -29,7 +30,6 @@ use rustfs_kms::{ types::{DescribeKeyRequest, KeyMetadata, OperationContext}, }; use rustfs_policy::policy::action::{Action, KmsAction}; -use s3s::header::CONTENT_TYPE; use s3s::{Body, S3Request, S3Response, S3Result, s3_error}; use serde::{Deserialize, Serialize}; use std::sync::Arc; @@ -233,13 +233,6 @@ async fn execute_lifecycle( } } -fn json_response(status: StatusCode, response: &KmsKeyLifecycleResponse) -> S3Result> { - let data = serde_json::to_vec(response).map_err(|e| s3_error!(InternalError, "failed to serialize response: {}", e))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, "application/json".parse().expect("static content type should parse")); - Ok(S3Response::with_headers((status, Body::from(data)), headers)) -} - fn unavailable_response(message: &str, key_id: String) -> S3Result> { json_response( StatusCode::SERVICE_UNAVAILABLE, diff --git a/rustfs/src/admin/handlers/kms_key_metadata.rs b/rustfs/src/admin/handlers/kms_key_metadata.rs index 8e4bd4a3a..7db8ed396 100644 --- a/rustfs/src/admin/handlers/kms_key_metadata.rs +++ b/rustfs/src/admin/handlers/kms_key_metadata.rs @@ -23,9 +23,10 @@ use super::kms_keys::scoped_key_id; use crate::admin::auth::validate_admin_request_with_kms_key; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::admin::runtime_sources::current_kms_runtime_service_manager; +use crate::admin::utils::json_response; use crate::auth::{check_key_valid, get_session_token}; use crate::server::{ADMIN_PREFIX, RemoteAddr}; -use hyper::{HeaderMap, Method, StatusCode}; +use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::MAX_ADMIN_REQUEST_BODY_SIZE; use rustfs_kms::{ @@ -33,7 +34,6 @@ use rustfs_kms::{ types::{DescribeKeyRequest, KeyMetadata}, }; use rustfs_policy::policy::action::{Action, KmsAction}; -use s3s::header::CONTENT_TYPE; use s3s::{Body, S3Request, S3Response, S3Result, s3_error}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -268,13 +268,6 @@ async fn execute_metadata_update( } } -fn json_response(status: StatusCode, response: &KmsKeyMetadataResponse) -> S3Result> { - let data = serde_json::to_vec(response).map_err(|e| s3_error!(InternalError, "failed to serialize response: {}", e))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, "application/json".parse().expect("static content type should parse")); - Ok(S3Response::with_headers((status, Body::from(data)), headers)) -} - fn unavailable_response(message: &str, key_id: String) -> S3Result> { json_response( StatusCode::SERVICE_UNAVAILABLE, diff --git a/rustfs/src/admin/handlers/kms_keys.rs b/rustfs/src/admin/handlers/kms_keys.rs index 6e0bbdf08..b89c58397 100644 --- a/rustfs/src/admin/handlers/kms_keys.rs +++ b/rustfs/src/admin/handlers/kms_keys.rs @@ -18,6 +18,7 @@ use super::kms_audit::{KmsAdminAudit, KmsAdminOperation}; use crate::admin::auth::{validate_admin_request, validate_admin_request_with_kms_key}; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::admin::runtime_sources::{current_kms_runtime_service_manager, current_or_init_kms_runtime_service_manager}; +use crate::admin::utils::extract_query_params; use crate::auth::{check_key_valid, get_session_token}; use crate::kms_deletion_gate::current_key_impact; use crate::server::{ADMIN_PREFIX, RemoteAddr}; @@ -100,22 +101,6 @@ pub struct GenerateDataKeyApiResponse { pub ciphertext_blob: String, // Base64 encoded } -/// The query parameters of an admin KMS request. -/// -/// Parsed with `form_urlencoded`, as the rest of the admin surface does, so a -/// parameter written without a value (`?status`) arrives as an empty value -/// rather than disappearing: a validated parameter must be able to tell "not -/// asked for" from "asked for, unreadable". -pub(super) fn extract_query_params(uri: &hyper::Uri) -> HashMap { - let mut params = HashMap::new(); - if let Some(query) = uri.query() { - for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { - params.insert(key.into_owned(), value.into_owned()); - } - } - params -} - /// Status values a `status` filter may name, spelled as the response spells /// them. const KEY_STATUS_FILTERS: &[(&str, KeyStatus)] = &[ diff --git a/rustfs/src/admin/handlers/object_data_cache.rs b/rustfs/src/admin/handlers/object_data_cache.rs index 6e45e7cb6..520e683ed 100644 --- a/rustfs/src/admin/handlers/object_data_cache.rs +++ b/rustfs/src/admin/handlers/object_data_cache.rs @@ -24,20 +24,17 @@ use crate::admin::auth::authorize_admin_request; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::admin::runtime_sources::current_object_data_cache; +use crate::admin::utils::json_response; use crate::app::object_data_cache::ObjectDataCacheAdapter; use crate::server::ADMIN_PREFIX; -use http::{HeaderMap, HeaderValue}; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_object_data_cache::{ObjectDataCacheIdentity, ObjectDataCacheInvalidationReason, ObjectDataCacheInvalidationResult}; use rustfs_policy::policy::action::{Action, AdminAction}; -use s3s::header::CONTENT_TYPE; -use s3s::{Body, S3Error, S3ErrorCode, S3Request, S3Response, S3Result, s3_error}; +use s3s::{Body, S3Request, S3Response, S3Result, s3_error}; use serde::Serialize; use std::sync::Arc; -const JSON_CONTENT_TYPE: &str = "application/json"; - #[derive(Debug, Serialize)] struct ObjectDataCacheStatsResponse { mode: &'static str, @@ -85,14 +82,6 @@ async fn authorize(req: &S3Request, action: AdminAction) -> S3Result<()> { Ok(()) } -fn json_response(body: &T) -> S3Result> { - let data = serde_json::to_vec(body) - .map_err(|err| S3Error::with_message(S3ErrorCode::InternalError, format!("failed to encode response: {err}")))?; - let mut headers = HeaderMap::new(); - headers.insert(CONTENT_TYPE, HeaderValue::from_static(JSON_CONTENT_TYPE)); - Ok(S3Response::with_headers((StatusCode::OK, Body::from(data)), headers)) -} - fn query_value(req: &S3Request, key: &str) -> Option { req.uri.query().and_then(|query| { url::form_urlencoded::parse(query.as_bytes()) @@ -146,7 +135,7 @@ impl Operation for ObjectDataCacheStatsHandler { }, }; - json_response(&response) + json_response(StatusCode::OK, &response) } } @@ -181,19 +170,24 @@ impl Operation for ObjectDataCacheFlushHandler { }; let (outcome, removed_keys) = invalidation_outcome(&result); - json_response(&ObjectDataCacheFlushResponse { - scope, - bucket, - object, - outcome, - removed_keys, - }) + json_response( + StatusCode::OK, + &ObjectDataCacheFlushResponse { + scope, + bucket, + object, + outcome, + removed_keys, + }, + ) } } #[cfg(test)] mod tests { use super::*; + use http::HeaderMap; + use s3s::S3ErrorCode; #[test] fn flush_outcome_maps_removed_and_noop() { diff --git a/rustfs/src/admin/handlers/oidc.rs b/rustfs/src/admin/handlers/oidc.rs index 37d12dc88..19fc7f951 100644 --- a/rustfs/src/admin/handlers/oidc.rs +++ b/rustfs/src/admin/handlers/oidc.rs @@ -23,6 +23,7 @@ use crate::admin::service::federated_identity::DefaultFederatedSessionBinding; use crate::admin::storage_api::config::{ read_admin_config_without_migrate, read_admin_server_config_snapshot, save_admin_server_config_snapshot, }; +use crate::admin::utils::json_response; use crate::server::{ADMIN_PREFIX, CONSOLE_PREFIX, MINIO_ADMIN_PREFIX}; use http::StatusCode; use hyper::Method; @@ -881,16 +882,6 @@ async fn parse_json_body(req: &mut S3Request) -> S3Re serde_json::from_slice(&body).map_err(|e| s3_error!(InvalidRequest, "invalid JSON: {}", e)) } -fn json_response(status: StatusCode, payload: &T) -> S3Result> { - let body = serde_json::to_vec(payload) - .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("serialize error: {e}")))?; - - let mut resp = S3Response::new((status, Body::from(body))); - resp.headers - .insert(http::header::CONTENT_TYPE, http::HeaderValue::from_static("application/json")); - Ok(resp) -} - async fn load_server_config_from_store() -> S3Result { let store = oidc_config_store()?; diff --git a/rustfs/src/admin/handlers/replication.rs b/rustfs/src/admin/handlers/replication.rs index a198329f5..3e8868967 100644 --- a/rustfs/src/admin/handlers/replication.rs +++ b/rustfs/src/admin/handlers/replication.rs @@ -34,11 +34,11 @@ use crate::admin::storage_api::contract::bucket::{BucketOperations, BucketOption use crate::admin::storage_api::contract::list::ListOperations as _; use crate::admin::storage_api::error::StorageError; use crate::admin::storage_api::runtime::PeerRestClient; -use crate::admin::utils::read_compatible_admin_body; +use crate::admin::utils::{extract_query_params, read_compatible_admin_body}; use crate::error::ApiError; use crate::server::ADMIN_PREFIX; use crate::storage::storage_api::lock_bucket_targets_metadata; -use http::{HeaderMap, HeaderValue, Uri}; +use http::{HeaderMap, HeaderValue}; use hyper::{Method, StatusCode}; use jiff::Timestamp; use matchit::Params; @@ -116,18 +116,6 @@ fn site_endpoint_for(endpoint: &str, secure: bool) -> String { } } -fn extract_query_params(uri: &Uri) -> HashMap { - let mut params = HashMap::new(); - - if let Some(query) = uri.query() { - for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { - params.insert(key.into_owned(), value.into_owned()); - } - } - - params -} - fn map_bucket_target_error(err: BucketTargetError) -> S3Error { match err { BucketTargetError::BucketRemoteTargetNotFound { .. } diff --git a/rustfs/src/admin/handlers/site_replication.rs b/rustfs/src/admin/handlers/site_replication.rs index 2dfa83a18..3881b22eb 100644 --- a/rustfs/src/admin/handlers/site_replication.rs +++ b/rustfs/src/admin/handlers/site_replication.rs @@ -47,7 +47,7 @@ use crate::admin::storage_api::contract::bucket::{ }; use crate::admin::storage_api::error::{Error as StorageError, is_err_bucket_not_found}; use crate::admin::storage_api::runtime::ECStore; -use crate::admin::utils::{encode_compatible_admin_payload, read_compatible_admin_body}; +use crate::admin::utils::{empty_response, encode_compatible_admin_payload, json_response, read_compatible_admin_body}; use crate::auth::constant_time_eq; use crate::config::get_config_snapshot; use crate::error::ApiError; @@ -61,7 +61,7 @@ use base64_simd::URL_SAFE_NO_PAD; use futures::StreamExt; use hmac::{Hmac, Mac}; use http::header::{CONTENT_TYPE, HOST}; -use http::{HeaderMap, HeaderValue, Uri}; +use http::{HeaderMap, Uri}; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_config::{ @@ -969,14 +969,6 @@ fn reject_site_replicator_on_public_admin(cred: &rustfs_credentials::Credentials Ok(()) } -fn json_response(value: &T) -> S3Result> { - let data = serde_json::to_vec(value) - .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("failed to serialize response: {e}")))?; - let mut headers = HeaderMap::new(); - headers.insert(s3s::header::CONTENT_TYPE, HeaderValue::from_static("application/json")); - Ok(S3Response::with_headers((StatusCode::OK, Body::from(data)), headers)) -} - fn go_gob_site_netperf_response(value: &SiteNetPerfNodeResult) -> S3Response<(StatusCode, Body)> { let data = encode_go_gob_site_netperf_node_result(value); S3Response::new((StatusCode::OK, Body::from(data))) @@ -1058,10 +1050,6 @@ fn write_go_gob_uint(out: &mut Vec, value: u64) { out.extend_from_slice(used); } -fn empty_response(status: StatusCode) -> S3Response<(StatusCode, Body)> { - S3Response::new((status, Body::empty())) -} - async fn read_plain_admin_body(mut input: Body) -> S3Result> { let body = input .store_all_limited(MAX_ADMIN_REQUEST_BODY_SIZE) @@ -4272,7 +4260,7 @@ async fn execute_site_replication_repair_locked( )); } if existing.status == "success" { - return json_response(&site_replication_repair_operation_response(existing)); + return json_response(StatusCode::OK, &site_replication_repair_operation_response(existing)); } if !constant_time_eq(&existing.plan_token, &plan_token) { return Err(S3Error::with_message( @@ -4305,7 +4293,7 @@ async fn execute_site_replication_repair_locked( }) .await?; if operation.status == "success" { - return json_response(&site_replication_repair_operation_response(&operation)); + return json_response(StatusCode::OK, &site_replication_repair_operation_response(&operation)); } let service_account_secret_key = site_replicator_service_account_secret(&state.service_account_access_key).await?; @@ -4367,7 +4355,7 @@ async fn execute_site_replication_repair_locked( summarize_site_replication_repair_operation(&mut operation); persist_site_replication_repair_operation(&operation).await?; - json_response(&site_replication_repair_operation_response(&operation)) + json_response(StatusCode::OK, &site_replication_repair_operation_response(&operation)) } pub async fn site_replication_make_bucket_hook(bucket: &str, lock_enabled: bool) -> S3Result<()> { @@ -10182,13 +10170,16 @@ impl Operation for SiteReplicationAddHandler { // response below (BUG2) rather than swallowed; they do not abort the overall add. initial_sync_errors.extend(backfill_existing_buckets_after_add(&state, &local_peer, None).await); - json_response(&ReplicateAddStatus { - success: true, - status: SITE_REPL_ADD_SUCCESS.to_string(), - initial_sync_error_message: initial_sync_errors.render(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - ..Default::default() - }) + json_response( + StatusCode::OK, + &ReplicateAddStatus { + success: true, + status: SITE_REPL_ADD_SUCCESS.to_string(), + initial_sync_error_message: initial_sync_errors.render(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }, + ) } } @@ -10255,7 +10246,7 @@ impl Operation for SiteReplicationRemoveHandler { site_replication_remove_status(&peer_errors) }; - json_response(&status) + json_response(StatusCode::OK, &status) } } @@ -10286,7 +10277,7 @@ impl Operation for SiteReplicationInfoHandler { validate_site_replication_admin_request(&req, AdminAction::SiteReplicationInfoAction).await?; let state = load_site_replication_state().await?; let local_peer = current_local_peer(&req, &state); - json_response(&site_replication_info_for(&state, &local_peer)) + json_response(StatusCode::OK, &site_replication_info_for(&state, &local_peer)) } } @@ -10300,7 +10291,7 @@ impl Operation for SiteReplicationMetaInfoHandler { let local_peer = current_local_peer(&req, &state); let opts = sr_status_options(&req.uri); let info = filter_sr_info(build_sr_info(&state, &local_peer).await?, &opts); - json_response(&info) + json_response(StatusCode::OK, &info) } } @@ -10313,7 +10304,7 @@ impl Operation for SiteReplicationStatusHandler { let state = load_site_replication_state().await?; let local_peer = current_local_peer(&req, &state); let status = build_status_info(&state, &local_peer, &req.uri).await?; - json_response(&status) + json_response(StatusCode::OK, &status) } } @@ -10568,7 +10559,7 @@ impl Operation for SRPeerJoinHandler { result = "join_superseded", "admin site replication state" ); - return json_response(&superseded_join_response(peer)); + return json_response(StatusCode::OK, &superseded_join_response(peer)); } }; // Fix 1 (receiving side): ensure the joining peer also sets up replication for any @@ -10587,10 +10578,13 @@ impl Operation for SRPeerJoinHandler { "admin site replication state" ); } - json_response(&applied_join_response( - state.peers.get(&local_peer.deployment_id).cloned().unwrap_or(local_peer), - backfill_errors.render(), - )) + json_response( + StatusCode::OK, + &applied_join_response( + state.peers.get(&local_peer.deployment_id).cloned().unwrap_or(local_peer), + backfill_errors.render(), + ), + ) } } @@ -10757,7 +10751,7 @@ impl Operation for SRPeerGetIDPSettingsHandler { async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { validate_site_replication_admin_request(&req, AdminAction::SiteReplicationAddAction).await?; - json_response(&local_idp_settings()) + json_response(StatusCode::OK, &local_idp_settings()) } } @@ -11036,12 +11030,15 @@ impl Operation for SiteReplicationEditHandler { } } - json_response(&ReplicateEditStatus { - success: true, - status: SITE_REPL_EDIT_SUCCESS.to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - ..Default::default() - }) + json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: true, + status: SITE_REPL_EDIT_SUCCESS.to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }, + ) } } @@ -11051,14 +11048,17 @@ pub struct SRPeerEditCapabilitiesHandler {} impl Operation for SRPeerEditCapabilitiesHandler { async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { validate_site_replication_admin_request(&req, AdminAction::SiteReplicationOperationAction).await?; - json_response(&ReplicateEditStatus { - success: query_pairs(&req.uri) - .get("capability") - .is_some_and(|value| peer_edit_capability_supported(value)), - status: SITE_REPL_EDIT_SUCCESS.to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - ..Default::default() - }) + json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: query_pairs(&req.uri) + .get("capability") + .is_some_and(|value| peer_edit_capability_supported(value)), + status: SITE_REPL_EDIT_SUCCESS.to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }, + ) } } @@ -11173,30 +11173,39 @@ impl Operation for SRPeerEditHandler { let service_account_access_key = match outcome { PeerEditOutcome::Applied(service_account_access_key) => service_account_access_key, PeerEditOutcome::Acked => { - return json_response(&ReplicateEditStatus { - success: true, - status: SITE_REPL_EDIT_SUCCESS.to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - ..Default::default() - }); + return json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: true, + status: SITE_REPL_EDIT_SUCCESS.to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }, + ); } PeerEditOutcome::Rejected(err_detail) => { - return json_response(&ReplicateEditStatus { - success: false, - status: SITE_REPL_EDIT_SUCCESS.to_string(), - err_detail: err_detail.to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - }); + return json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: false, + status: SITE_REPL_EDIT_SUCCESS.to_string(), + err_detail: err_detail.to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }, + ); } }; if endpoint_refresh_requested { if service_account_access_key.is_empty() { - return json_response(&ReplicateEditStatus { - success: false, - status: SITE_REPL_EDIT_SUCCESS.to_string(), - err_detail: "site replicator service account is not configured".to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - }); + return json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: false, + status: SITE_REPL_EDIT_SUCCESS.to_string(), + err_detail: "site replicator service account is not configured".to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }, + ); } let service_account_secret_key = site_replicator_service_account_secret(&service_account_access_key).await?; let pending_id = refresh_id.unwrap_or_default(); @@ -11214,19 +11223,25 @@ impl Operation for SRPeerEditHandler { }) .await?; if !committed { - return json_response(&ReplicateEditStatus { - success: false, - status: SITE_REPL_EDIT_SUCCESS.to_string(), - err_detail: "endpoint target refresh state changed during update".to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - }); + return json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: false, + status: SITE_REPL_EDIT_SUCCESS.to_string(), + err_detail: "endpoint target refresh state changed during update".to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }, + ); } - return json_response(&ReplicateEditStatus { - success: true, - status: SITE_REPL_EDIT_SUCCESS.to_string(), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - ..Default::default() - }); + return json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: true, + status: SITE_REPL_EDIT_SUCCESS.to_string(), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }, + ); } Ok(empty_response(StatusCode::OK)) } @@ -11433,7 +11448,7 @@ impl Operation for SiteReplicationResyncOpHandler { .buckets .sort_by(|left, right| left.bucket.cmp(&right.bucket).then(left.target_arn.cmp(&right.target_arn))); let (limit, offset) = parse_site_resync_page(&query, &status)?; - json_response(&site_resync_page(&status, limit, offset)?) + json_response(StatusCode::OK, &site_resync_page(&status, limit, offset)?) } } @@ -11479,17 +11494,20 @@ impl Operation for SiteReplicationRepairHandler { if body.preflight_token.is_some() || body.operation_id.is_some() { return Err(s3_error!(InvalidRequest, "dry-run does not accept preflightToken or operationId")); } - return json_response(&SiteReplicationRepairPreflight { - mode: "dry-run", - status: "planned", - preflight_token, - retry_events: state - .retry_queue - .iter() - .filter(|event| retry_event_replayed_by_bootstrap(event)) - .count(), - sites, - }); + return json_response( + StatusCode::OK, + &SiteReplicationRepairPreflight { + mode: "dry-run", + status: "planned", + preflight_token, + retry_events: state + .retry_queue + .iter() + .filter(|event| retry_event_replayed_by_bootstrap(event)) + .count(), + sites, + }, + ); } let supplied_token = body @@ -11542,7 +11560,7 @@ impl Operation for SiteReplicationRepairStatusHandler { .get(&operation_id) .cloned() .ok_or_else(|| s3_error!(InvalidRequest, "repair operation was not found"))?; - json_response(&site_replication_repair_operation_response(&operation)) + json_response(StatusCode::OK, &site_replication_repair_operation_response(&operation)) } } @@ -11696,17 +11714,20 @@ impl Operation for SRRotateServiceAccountHandler { peer_errors.push("service account rotation is still pending".to_string()); } - json_response(&ReplicateEditStatus { - success: complete && peer_errors.is_empty(), - status: if complete && peer_errors.is_empty() { - "Success" - } else { - "Partial" - } - .to_string(), - err_detail: peer_errors.join("; "), - api_version: Some(SITE_REPL_API_VERSION.to_string()), - }) + json_response( + StatusCode::OK, + &ReplicateEditStatus { + success: complete && peer_errors.is_empty(), + status: if complete && peer_errors.is_empty() { + "Success" + } else { + "Partial" + } + .to_string(), + err_detail: peer_errors.join("; "), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }, + ) } } diff --git a/rustfs/src/admin/handlers/table_catalog/mod.rs b/rustfs/src/admin/handlers/table_catalog/mod.rs index a51e356ea..5a8e94033 100644 --- a/rustfs/src/admin/handlers/table_catalog/mod.rs +++ b/rustfs/src/admin/handlers/table_catalog/mod.rs @@ -17,6 +17,7 @@ use crate::admin::runtime_sources::default_admin_usecase; use crate::admin::storage_api::access::{ReqInfo, authorize_internal_object_request}; use crate::admin::storage_api::bucket::metadata::table_catalog_path_hash; use crate::admin::storage_api::runtime::ECStore; +use crate::admin::utils::empty_response; use crate::admin::{ auth::{AdminResourceScope, validate_admin_action_with_bucket_object_for_iam}, router::{AdminOperation, Operation, S3Router}, @@ -969,10 +970,6 @@ fn build_sensitive_json_response(status: StatusCode, body: &T) -> Ok(response) } -fn empty_response(status: StatusCode) -> S3Response<(StatusCode, Body)> { - S3Response::new((status, Body::default())) -} - fn duration_millis_u64(duration: StdDuration) -> u64 { u64::try_from(duration.as_millis()).unwrap_or(u64::MAX) } diff --git a/rustfs/src/admin/utils.rs b/rustfs/src/admin/utils.rs index b690043c9..205cbbadf 100644 --- a/rustfs/src/admin/utils.rs +++ b/rustfs/src/admin/utils.rs @@ -13,8 +13,12 @@ // limitations under the License. use crate::server::{MINIO_ADMIN_PREFIX, has_path_prefix}; +use http::{HeaderMap, HeaderValue, StatusCode, Uri}; use rustfs_crypto::{decrypt_data, decrypt_stream_io, encrypt_stream_io}; -use s3s::{Body, S3Result, s3_error}; +use s3s::header::CONTENT_TYPE; +use s3s::{Body, S3Error, S3ErrorCode, S3Response, S3Result, s3_error}; +use serde::Serialize; +use std::collections::HashMap; /// Returns `true` if `s` contains any whitespace character. /// @@ -59,6 +63,48 @@ pub(crate) fn encode_compatible_admin_payload(path: &str, secret_key: &str, data } } +/// Serialize `value` as the JSON body of an admin response with `status`. +/// +/// The admin surface answers almost every endpoint this way, so the shape is +/// pinned here rather than re-derived per handler: `Content-Type: +/// application/json`, no other header, and the serialized bytes verbatim as +/// the body. Serialization failure is reported as `InternalError`; the +/// response structs the admin handlers pass here are plain owned data, so that +/// arm is unreachable in practice. +pub(crate) fn json_response(status: StatusCode, value: &T) -> S3Result> { + let data = serde_json::to_vec(value) + .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, format!("failed to serialize response: {e}")))?; + let mut headers = HeaderMap::new(); + headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); + Ok(S3Response::with_headers((status, Body::from(data)), headers)) +} + +/// A bodiless admin response carrying only `status`. +/// +/// Used by the endpoints whose success answer is the status code itself +/// (`204 No Content`, or a `200 OK` acknowledgement with nothing to report). +/// No `Content-Type` is set, because there is no content to type. +pub(crate) fn empty_response(status: StatusCode) -> S3Response<(StatusCode, Body)> { + S3Response::new((status, Body::empty())) +} + +/// Collect a request URI's query string into a parameter map. +/// +/// Parsed with `form_urlencoded`, as the rest of the admin surface does, so a +/// parameter written without a value (`?status`) arrives as an empty value +/// rather than disappearing: a validated parameter must be able to tell "not +/// asked for" from "asked for, unreadable". Percent escapes and `+` are +/// decoded, and a repeated key keeps its last occurrence. +pub(crate) fn extract_query_params(uri: &Uri) -> HashMap { + let mut params = HashMap::new(); + if let Some(query) = uri.query() { + for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { + params.insert(key.into_owned(), value.into_owned()); + } + } + params +} + #[cfg(test)] mod tests { use super::*; @@ -119,4 +165,93 @@ mod tests { assert_eq!(decoded, payload); } + + async fn body_bytes(mut body: Body) -> Vec { + body.store_all_limited(64 * 1024).await.expect("body should read").to_vec() + } + + /// The wire contract every admin endpoint that returns JSON depends on: + /// the requested status, `application/json`, and the serialized bytes + /// verbatim — nothing else. + #[tokio::test] + async fn json_response_carries_status_content_type_and_serialized_body() { + #[derive(Serialize)] + struct Payload { + success: bool, + message: &'static str, + } + + let response = json_response( + StatusCode::ACCEPTED, + &Payload { + success: true, + message: "queued", + }, + ) + .expect("payload should serialize"); + + assert_eq!(response.output.0, StatusCode::ACCEPTED); + assert_eq!( + response.headers.get(CONTENT_TYPE).and_then(|value| value.to_str().ok()), + Some("application/json") + ); + assert_eq!(response.headers.len(), 1); + assert_eq!(body_bytes(response.output.1).await, br#"{"success":true,"message":"queued"}"#.to_vec()); + } + + /// A serialization failure must surface as `InternalError` rather than a + /// panic or a half-written body. + #[test] + fn json_response_reports_serialization_failure_as_internal_error() { + struct Unserializable; + + impl Serialize for Unserializable { + fn serialize(&self, _serializer: S) -> Result { + Err(serde::ser::Error::custom("nope")) + } + } + + let err = json_response(StatusCode::OK, &Unserializable).expect_err("serialization must fail"); + assert_eq!(err.code(), &S3ErrorCode::InternalError); + assert!(err.message().unwrap_or_default().contains("failed to serialize response")); + } + + /// The bodiless answer must stay bodiless and must not claim a content + /// type: callers use it for `204 No Content` and bare acknowledgements. + #[tokio::test] + async fn empty_response_has_no_body_and_no_headers() { + for status in [StatusCode::OK, StatusCode::NO_CONTENT] { + let response = empty_response(status); + assert_eq!(response.output.0, status); + assert!(response.headers.is_empty()); + assert!(body_bytes(response.output.1).await.is_empty()); + } + } + + /// Percent escapes must be decoded, so a job id or key id containing `/` + /// arrives whole rather than as its escape sequence. + #[test] + fn extract_query_params_decodes_percent_escapes() { + let uri: Uri = "/rustfs/admin/v3/status-job?jobId=abc%2F123" + .parse() + .expect("uri should parse"); + let params = extract_query_params(&uri); + assert_eq!(params.get("jobId"), Some(&"abc/123".to_string())); + } + + /// A parameter written without a value must arrive as an empty value, not + /// vanish: a validated parameter has to tell "not asked for" from "asked + /// for, unreadable". A missing query string yields no parameters at all. + #[test] + fn extract_query_params_keeps_valueless_parameters_and_survives_no_query() { + let valueless: Uri = "/rustfs/admin/v3/kms/keys?status".parse().expect("uri should parse"); + let params = extract_query_params(&valueless); + assert_eq!(params.get("status"), Some(&String::new())); + + let plus: Uri = "/rustfs/admin/v3/kms/keys?name=a+b".parse().expect("uri should parse"); + assert_eq!(extract_query_params(&plus).get("name"), Some(&"a b".to_string())); + + let bare: Uri = "/rustfs/admin/v3/kms/keys".parse().expect("uri should parse"); + assert!(extract_query_params(&bare).is_empty()); + } }