improve

Author: owjs3901

crates/vespera/Cargo.toml

@@ -95,5 +95,11 @@ trybuild = "1"
 name = "validation"
 harness = false
 
+# multipart `4xx`/`422` error-envelope serialization before/after A/B bench
+# (same borrowing-`Serialize` win as `validation`, on `TypedMultipartError`).
+[[bench]]
+name = "multipart_error"
+harness = false
+
 [lints]
 workspace = true

crates/vespera/benches/multipart_error.rs

@@ -0,0 +1,129 @@
+//! Before/after A/B benchmark for the multipart `4xx`/`422` error-envelope
+//! serialization (the cold but attacker-reachable malformed-input path).
+//!
+//! Both arms serialize the **same** [`TypedMultipartError`] to the **same**
+//! bytes (`{"errors":[{"message":...,"path":...}]}`):
+//!
+//! - `before`: the original implementation — materialize the public message
+//!   with `error.to_string()` (one heap `String` per error) and serialize an
+//!   owned-`&str` envelope.
+//! - `after`: the shipped implementation — a borrowing `Serialize` chain that
+//!   streams the error's own `Display` straight into `serde_json` via
+//!   `collect_str` (zero per-error `String` allocation), mirroring the
+//!   `Validated<T>` 422 serializer in `multipart.rs`.
+//!
+//! The delta is the per-error `String` allocation the change removes. Both
+//! arms assert byte-identical output so the bench can never silently drift
+//! from the real envelope contract.
+
+use std::borrow::Cow;
+
+use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main};
+use serde::{Serialize, Serializer, ser::SerializeStruct};
+use vespera::multipart::TypedMultipartError;
+
+/// A realistic client-caused multipart error (the common 422 case): a scalar
+/// field whose value failed to parse. Its `Display` carries the field name,
+/// the wanted type, and the parse error — representative envelope work.
+fn fixture() -> TypedMultipartError {
+    TypedMultipartError::WrongFieldType {
+        field_name: "age".to_owned(),
+        wanted: Cow::Borrowed("u8"),
+        source: "number too large to fit in target type".to_owned(),
+    }
+}
+
+/// The offending field name doubles as the envelope `path`.
+const PATH: &str = "age";
+
+// ── AFTER: shipped borrowing Serialize chain (mirror of multipart.rs) ─
+
+fn serialize_after(err: &TypedMultipartError) -> Vec<u8> {
+    struct Message<'a>(&'a TypedMultipartError);
+    impl Serialize for Message<'_> {
+        fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
+            // Client-caused variant → stream its `Display` with no `String`.
+            s.collect_str(self.0)
+        }
+    }
+    struct OneError<'a> {
+        err: &'a TypedMultipartError,
+        path: &'a str,
+    }
+    impl Serialize for OneError<'_> {
+        fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
+            let mut st = s.serialize_struct("MultipartOneError", 2)?;
+            st.serialize_field("message", &Message(self.err))?;
+            st.serialize_field("path", self.path)?;
+            st.end()
+        }
+    }
+    struct Envelope<'a> {
+        err: &'a TypedMultipartError,
+        path: &'a str,
+    }
+    impl Serialize for Envelope<'_> {
+        fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
+            let mut st = s.serialize_struct("MultipartErrorEnvelope", 1)?;
+            st.serialize_field(
+                "errors",
+                &[OneError {
+                    err: self.err,
+                    path: self.path,
+                }],
+            )?;
+            st.end()
+        }
+    }
+    serde_json::to_vec(&Envelope { err, path: PATH }).expect("infallible")
+}
+
+// ── BEFORE: original owned-`String` message implementation ───────────
+
+fn serialize_before(err: &TypedMultipartError) -> Vec<u8> {
+    #[derive(Serialize)]
+    struct OneError<'a> {
+        message: &'a str,
+        path: &'a str,
+    }
+    #[derive(Serialize)]
+    struct Envelope<'a> {
+        errors: [OneError<'a>; 1],
+    }
+    let message = err.to_string();
+    serde_json::to_vec(&Envelope {
+        errors: [OneError {
+            message: &message,
+            path: PATH,
+        }],
+    })
+    .expect("infallible")
+}
+
+fn bench_multipart_error_envelope(c: &mut Criterion) {
+    let err = fixture();
+
+    // Guard: the two implementations MUST produce identical bytes, so the
+    // A/B compares the same observable work — never a shortcut.
+    assert_eq!(
+        serialize_before(&err),
+        serialize_after(&err),
+        "before/after multipart error-envelope bytes diverged"
+    );
+
+    let mut group = c.benchmark_group("multipart_error_envelope");
+    group.bench_with_input(
+        BenchmarkId::new("owned_string_before", "WrongFieldType"),
+        &err,
+        |b, e| b.iter(|| serialize_before(std::hint::black_box(e))),
+    );
+    group.bench_with_input(
+        BenchmarkId::new("borrowing_serialize_after", "WrongFieldType"),
+        &err,
+        |b, e| b.iter(|| serialize_after(std::hint::black_box(e))),
+    );
+    group.finish();
+}
+
+criterion_group!(benches, bench_multipart_error_envelope);
+criterion_main!(benches);

crates/vespera/src/multipart.rs

@@ -165,20 +165,48 @@ impl TypedMultipartError {
         }
     }
 
-    /// Public-facing message for the JSON error envelope.
+    /// Serialize the canonical `4xx`/`422` JSON error envelope
+    /// (`{"errors":[{"message":...,"path":...}]}`) for this error — byte-
+    /// identical to `Validated<T>`'s envelope so JNI hoisting and clients
+    /// treat both uniformly.
     ///
-    /// `Other` wraps internal I/O / blocking-task failures whose source
-    /// string can leak implementation details (temp-file paths, OS error
-    /// text); it is the only `500` variant, so it returns a stable, generic
-    /// message. Every other variant returns its `Display` (already safe —
-    /// it describes a client-supplied field problem). The full `Display`
-    /// (including `Other`'s `source`) stays available for server-side
-    /// logging via the `std::error::Error` impl.
-    fn response_message(&self) -> Cow<'_, str> {
-        if matches!(self, Self::Other { .. }) {
-            Cow::Borrowed("internal error while processing multipart request")
+    /// The message streams through [`MultipartMessage`]: `Other` (the only
+    /// `500`, whose source can leak temp-file paths / OS text) yields a stable
+    /// generic string; every other (client-caused) variant streams its own
+    /// `Display` with NO intermediate `String`. `path` is the offending field
+    /// name when known, else empty. Infallible in practice; the fallback keeps
+    /// this request-time path panic-free instead of unwinding in a handler.
+    fn error_body(&self) -> Vec<u8> {
+        serde_json::to_vec(&MultipartErrorEnvelope {
+            errors: [MultipartOneError {
+                message: MultipartMessage(self),
+                path: self.field_name().unwrap_or(""),
+            }],
+        })
+        .unwrap_or_else(|_| br#"{"errors":[{"message":"serialization error","path":""}]}"#.to_vec())
+    }
+}
+
+/// Stable, source-free public message for the only `500` variant (`Other`),
+/// whose wrapped `source` can leak temp-file paths / OS error text. Every
+/// other variant is client-caused and safe to expose verbatim.
+const MULTIPART_INTERNAL_ERROR_MSG: &str = "internal error while processing multipart request";
+
+/// Streams a multipart error's public message straight into the serializer
+/// with NO intermediate `String`: `Other` becomes [`MULTIPART_INTERNAL_ERROR_MSG`];
+/// every other (client-caused) variant streams its own `Display` via
+/// `collect_str`. Byte-identical to the previous `to_string()`-then-serialize
+/// path (serde escapes a `collect_str` stream exactly like an equal `&str`)
+/// but allocation-free on the common client-error path — mirroring
+/// `Validated<T>`'s 422 serializer.
+struct MultipartMessage<'a>(&'a TypedMultipartError);
+
+impl serde::Serialize for MultipartMessage<'_> {
+    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        if matches!(self.0, TypedMultipartError::Other { .. }) {
+            serializer.serialize_str(MULTIPART_INTERNAL_ERROR_MSG)
         } else {
-            Cow::Owned(self.to_string())
+            serializer.collect_str(self.0)
         }
     }
 }
@@ -191,7 +219,7 @@ impl TypedMultipartError {
 /// map/array/object intermediate).
 #[derive(serde::Serialize)]
 struct MultipartOneError<'a> {
-    message: &'a str,
+    message: MultipartMessage<'a>,
     path: &'a str,
 }
 
@@ -221,24 +249,9 @@ impl IntoResponse for TypedMultipartError {
             Self::FieldTooLarge { .. } => StatusCode::PAYLOAD_TOO_LARGE,
             Self::Other { .. } => StatusCode::INTERNAL_SERVER_ERROR,
         };
-        // Serialize the canonical 422 envelope (see module-scope
-        // `MultipartErrorEnvelope` / `MultipartOneError`); `path` is the
-        // offending field name when known, else empty.
-        let path = self.field_name().unwrap_or("");
-        let message = self.response_message();
-        let body = serde_json::to_vec(&MultipartErrorEnvelope {
-            errors: [MultipartOneError {
-                message: &message,
-                path,
-            }],
-        })
-        // Serializing a struct of two `&str` is infallible in practice; the
-        // fallback keeps this request-time error path panic-free (matching
-        // `Validated<T>`'s 422 envelope) by emitting a minimal valid envelope
-        // instead of unwinding inside a handler.
-        .unwrap_or_else(|_| {
-            br#"{"errors":[{"message":"serialization error","path":""}]}"#.to_vec()
-        });
+        // Serialize the canonical 422 envelope (see `error_body` /
+        // module-scope `MultipartErrorEnvelope`).
+        let body = self.error_body();
         (
             status,
             [(

crates/vespera/src/multipart/tests.rs

@@ -65,27 +65,34 @@ fn test_error_display_duplicate_field() {
 }
 
 #[test]
-fn other_error_response_message_hides_internal_source() {
+fn other_error_body_hides_internal_source() {
     // The internal source (e.g. a temp-file path / OS error) must NOT
-    // leak into the public 500 response message.
+    // leak into the public 500 response body — assert on the ACTUAL
+    // serialized envelope (the production path), not an intermediate.
     let err = TypedMultipartError::Other {
         source: "/tmp/vespera-upload-7f3a.part: No such file or directory".to_string(),
     };
+    let body = String::from_utf8(err.error_body()).expect("envelope is UTF-8");
     assert_eq!(
-        err.response_message(),
-        "internal error while processing multipart request"
+        body,
+        r#"{"errors":[{"message":"internal error while processing multipart request","path":""}]}"#
     );
     assert!(
-        !err.response_message().contains("/tmp/"),
-        "internal source path leaked into response message"
+        !body.contains("/tmp/"),
+        "internal source path leaked into response body"
     );
     // Display still exposes the source for server-side logging.
     assert!(err.to_string().contains("/tmp/"));
-    // Non-Other variants keep their (client-safe) Display message.
+    // Non-Other variants stream their (client-safe) Display message verbatim,
+    // byte-identical to the prior `to_string()` path.
     let missing = TypedMultipartError::MissingField {
         field_name: "avatar".to_string(),
     };
-    assert_eq!(missing.response_message(), "Missing field: `avatar`");
+    let missing_body = String::from_utf8(missing.error_body()).expect("envelope is UTF-8");
+    assert_eq!(
+        missing_body,
+        r#"{"errors":[{"message":"Missing field: `avatar`","path":"avatar"}]}"#
+    );
 }
 
 #[test]

crates/vespera_core/src/openapi.rs

@@ -151,21 +151,48 @@ fn merge_component_map<V>(
     self_map: &mut Option<BTreeMap<String, V>>,
     other_map: Option<BTreeMap<String, V>>,
 ) {
-    let Some(other_map) = other_map else { return };
+    let Some(other_map) = non_empty_component_map(other_map) else {
+        return;
+    };
     let target = self_map.get_or_insert_with(BTreeMap::new);
     for (name, value) in other_map {
         target.entry(name).or_insert(value);
     }
 }
 
+fn non_empty_component_map<V>(map: Option<BTreeMap<String, V>>) -> Option<BTreeMap<String, V>> {
+    map.filter(|entries| !entries.is_empty())
+}
+
 fn has_any_component_map(components: &Components) -> bool {
-    components.schemas.is_some()
-        || components.responses.is_some()
-        || components.parameters.is_some()
-        || components.examples.is_some()
-        || components.request_bodies.is_some()
-        || components.headers.is_some()
-        || components.security_schemes.is_some()
+    components
+        .schemas
+        .as_ref()
+        .is_some_and(|entries| !entries.is_empty())
+        || components
+            .responses
+            .as_ref()
+            .is_some_and(|entries| !entries.is_empty())
+        || components
+            .parameters
+            .as_ref()
+            .is_some_and(|entries| !entries.is_empty())
+        || components
+            .examples
+            .as_ref()
+            .is_some_and(|entries| !entries.is_empty())
+        || components
+            .request_bodies
+            .as_ref()
+            .is_some_and(|entries| !entries.is_empty())
+        || components
+            .headers
+            .as_ref()
+            .is_some_and(|entries| !entries.is_empty())
+        || components
+            .security_schemes
+            .as_ref()
+            .is_some_and(|entries| !entries.is_empty())
 }
 
 /// Merge `other`'s per-method operations into `into` with **self-wins**