Fix some issues with large request bodies

CHANGELOG.md

@@ -50,6 +50,7 @@ Here's the full list of changes:
 - Redraw TUI when terminal is resized
 - Clamp text window scroll state when window is resized or text changes
 - Fix extraneous input events when exiting Vim [#351](https://github.com/LucasPickering/slumber/issues/351)
+- Improve performance and fix crashes when handling large request/response bodies [#356](https://github.com/LucasPickering/slumber/issues/356)
 
 ## [1.8.1] - 2024-08-11
 

crates/slumber_cli/src/commands/request.rs

@@ -153,7 +153,7 @@ impl BuildRequestCommand {
         let collection_file = CollectionFile::load(collection_path).await?;
         let collection = collection_file.collection;
         let config = Config::load()?;
-        let http_engine = HttpEngine::new(&config.ignore_certificate_hosts);
+        let http_engine = HttpEngine::new(&config.http);
 
         // Validate profile ID, so we can provide a good error if it's invalid
         if let Some(profile_id) = &self.profile {

crates/slumber_config/src/lib.rs

@@ -18,7 +18,10 @@ pub use theme::Theme;
 use anyhow::Context;
 use indexmap::IndexMap;
 use serde::{Deserialize, Serialize};
-use slumber_core::util::{parse_yaml, DataDirectory, ResultTraced};
+use slumber_core::{
+    http::HttpEngineConfig,
+    util::{parse_yaml, DataDirectory, ResultTraced},
+};
 use std::{fs::File, path::PathBuf};
 use tracing::info;
 
@@ -34,8 +37,7 @@ pub struct Config {
     /// Command to use for in-app editing. If provided, overrides
     /// `VISUAL`/`EDITOR` environment variables
     pub editor: Option<String>,
-    /// TLS cert errors on these hostnames are ignored. Be careful!
-    pub ignore_certificate_hosts: Vec<String>,
+    pub http: HttpEngineConfig,
     /// Should templates be rendered inline in the UI, or should we show the
     /// raw text?
     pub preview_templates: bool,
@@ -86,7 +88,7 @@ impl Default for Config {
     fn default() -> Self {
         Self {
             editor: None,
-            ignore_certificate_hosts: Default::default(),
+            http: HttpEngineConfig::default(),
             preview_templates: true,
             input_bindings: Default::default(),
             theme: Default::default(),

crates/slumber_core/src/db.rs

@@ -355,7 +355,7 @@ impl CollectionDatabase {
                     ":method": exchange.request.method.as_str(),
                     ":url": exchange.request.url.as_str(),
                     ":request_headers": SqlWrap(&exchange.request.headers),
-                    ":request_body": exchange.request.body.as_deref(),
+                    ":request_body": exchange.request.body(),
 
                     ":status_code": exchange.response.status.as_u16(),
                     ":response_headers": SqlWrap(&exchange.response.headers),

crates/slumber_core/src/db/migrations.rs

@@ -192,7 +192,7 @@ fn migrate_requests_v2_up(transaction: &Transaction) -> HookResult {
             ":method": exchange.request.method.as_str(),
             ":url": exchange.request.url.as_str(),
             ":request_headers": SqlWrap(&exchange.request.headers),
-            ":request_body": exchange.request.body.as_deref(),
+            ":request_body": exchange.request.body(),
 
             ":status_code": exchange.response.status.as_u16(),
             ":response_headers": SqlWrap(&exchange.response.headers),

crates/slumber_core/src/http.rs

@@ -62,6 +62,7 @@ use reqwest::{
     multipart::{Form, Part},
     Client, RequestBuilder, Response, Url,
 };
+use serde::{Deserialize, Serialize};
 use std::{collections::HashSet, sync::Arc};
 use tracing::{info, info_span};
 
@@ -80,16 +81,17 @@ pub struct HttpEngine {
     /// for. If the user didn't specify any (99.9% of cases), don't bother
     /// creating a client because it's expensive.
     danger_client: Option<(Client, HashSet<String>)>,
+    large_body_size: usize,
 }
 
 impl HttpEngine {
     /// Build a new HTTP engine, which can be used for the entire program life
-    pub fn new(ignore_certificate_hosts: &[String]) -> Self {
+    pub fn new(config: &HttpEngineConfig) -> Self {
         let client = Client::builder()
             .user_agent(USER_AGENT)
             .build()
             .expect("Error building reqwest client");
-        let danger_client = if ignore_certificate_hosts.is_empty() {
+        let danger_client = if config.ignore_certificate_hosts.is_empty() {
             None
         } else {
             Some((
@@ -98,12 +100,13 @@ impl HttpEngine {
                     .danger_accept_invalid_certs(true)
                     .build()
                     .expect("Error building reqwest client"),
-                ignore_certificate_hosts.iter().cloned().collect(),
+                config.ignore_certificate_hosts.iter().cloned().collect(),
             ))
         };
         Self {
             client,
             danger_client,
+            large_body_size: config.large_body_size,
         }
     }
 
@@ -166,6 +169,7 @@ impl HttpEngine {
                 seed,
                 template_context.selected_profile.clone(),
                 &request,
+                self.large_body_size,
             )
             .into(),
             client: client.clone(),
@@ -284,7 +288,25 @@ impl HttpEngine {
 
 impl Default for HttpEngine {
     fn default() -> Self {
-        Self::new(&[])
+        Self::new(&HttpEngineConfig::default())
+    }
+}
+
+#[derive(Debug, Serialize, Deserialize)]
+pub struct HttpEngineConfig {
+    /// TLS cert errors on these hostnames are ignored. Be careful!
+    pub ignore_certificate_hosts: Vec<String>,
+    /// Request/response bodies over this size are treated differently, for
+    /// performance reasons
+    pub large_body_size: usize,
+}
+
+impl Default for HttpEngineConfig {
+    fn default() -> Self {
+        Self {
+            ignore_certificate_hosts: Default::default(),
+            large_body_size: 1000 * 1000, // 1MB
+        }
     }
 }
 
@@ -324,7 +346,7 @@ impl RequestTicket {
         let id = self.record.id;
 
         // Capture the rest of this method in a span
-        let _ = info_span!("HTTP request", request_id = %id, ?self).entered();
+        let _ = info_span!("HTTP request", request_id = %id).entered();
 
         // This start time will be accurate because the request doesn't launch
         // until this whole future is awaited

crates/slumber_core/src/http/models.rs

@@ -19,7 +19,7 @@ use derive_more::{Display, From, FromStr};
 use mime::Mime;
 use reqwest::{
     header::{self, HeaderMap},
-    Client, Method, Request, StatusCode, Url,
+    Body, Client, Method, Request, StatusCode, Url,
 };
 use serde::{Deserialize, Serialize};
 use std::{
@@ -247,7 +247,8 @@ pub struct RequestRecord {
     pub url: Url,
     #[serde(with = "cereal::serde_header_map")]
     pub headers: HeaderMap,
-    /// Body content as bytes. This should be decoded as needed
+    /// Body content as bytes. This should be decoded as needed. This will
+    /// **not** be populated for bodies that are above the "large" threshold.
     pub body: Option<Bytes>,
 }
 
@@ -265,6 +266,7 @@ impl RequestRecord {
         seed: RequestSeed,
         profile_id: Option<ProfileId>,
         request: &Request,
+        max_body_size: usize,
     ) -> Self {
         Self {
             id: seed.id,
@@ -274,11 +276,15 @@ impl RequestRecord {
             method: request.method().clone(),
             url: request.url().clone(),
             headers: request.headers().clone(),
-            body: request.body().and_then(|body| {
-                // Stream bodies are just thrown away for now
-                // https://github.com/LucasPickering/slumber/issues/256
-                Some(body.as_bytes()?.to_owned().into())
-            }),
+            body: request
+                .body()
+                // Stream bodies and bodies over a certain size threshold are
+                // thrown away. Storing request bodies in general doesn't
+                // provide a ton of value, so we shouldn't do it at the expense
+                // of performance
+                .and_then(Body::as_bytes)
+                .filter(|body| body.len() <= max_body_size)
+                .map(|body| body.to_owned().into()),
         }
     }
 
@@ -308,6 +314,10 @@ impl RequestRecord {
         Ok(buf)
     }
 
+    pub fn body(&self) -> Option<&[u8]> {
+        self.body.as_deref()
+    }
+
     /// Get the body of the request, decoded as UTF-8. Returns an error if the
     /// body isn't valid UTF-8.
     pub fn body_str(&self) -> anyhow::Result<Option<&str>> {

crates/slumber_core/src/test_util.rs

@@ -2,7 +2,7 @@
 
 use crate::{
     collection::{ChainSource, HasId},
-    http::HttpEngine,
+    http::{HttpEngine, HttpEngineConfig},
     template::{Prompt, Prompter},
     util::{get_repo_root, ResultTraced},
 };
@@ -61,7 +61,10 @@ pub fn temp_dir() -> TempDir {
 #[fixture]
 #[once]
 pub fn http_engine() -> HttpEngine {
-    HttpEngine::new(&["danger".to_owned()])
+    HttpEngine::new(&HttpEngineConfig {
+        ignore_certificate_hosts: vec!["danger".to_owned()],
+        ..Default::default()
+    })
 }
 
 /// Guard for a temporary directory. Create the directory on creation, delete

crates/slumber_tui/src/context.rs

@@ -45,7 +45,7 @@ impl TuiContext {
     fn new(config: Config) -> Self {
         let styles = Styles::new(&config.theme);
         let input_engine = InputEngine::new(config.input_bindings.clone());
-        let http_engine = HttpEngine::new(&config.ignore_certificate_hosts);
+        let http_engine = HttpEngine::new(&config.http);
         Self {
             config,
             styles,

crates/slumber_tui/src/view/common/template_preview.rs

@@ -15,7 +15,6 @@ use slumber_core::{
     template::{Template, TemplateChunk},
 };
 use std::{
-    mem,
     ops::Deref,
     sync::{Arc, Mutex},
 };
@@ -130,8 +129,7 @@ impl Widget for &TemplatePreview {
 /// See ratatui docs: <https://docs.rs/ratatui/latest/ratatui/text/index.html>
 #[derive(Debug, Default)]
 struct TextStitcher {
-    completed_lines: Vec<Line<'static>>,
-    next_line: Vec<Span<'static>>,
+    text: Text<'static>,
 }
 
 impl TextStitcher {
@@ -155,33 +153,40 @@ impl TextStitcher {
 
             stitcher.add_chunk(chunk_text, style);
         }
-        stitcher.into_text()
+        stitcher.text
     }
 
     /// Add one chunk to the text. This will recursively split on any line
     /// breaks in the text until it reaches the end.
-    fn add_chunk(&mut self, mut chunk_text: String, style: Style) {
-        // If we've reached a line ending, push the line and start a new one.
-        // Intentionally ignore \r; it won't cause any harm in the output text
-        match chunk_text.find('\n') {
-            Some(index) => {
-                // Exclude newline. +1 is safe because we know index points to
-                // a char and therefore is before the end of the string
-                let rest = chunk_text.split_off(index + 1);
-                let popped = chunk_text.pop(); // Pop the newline
-                debug_assert_eq!(popped, Some('\n'));
+    fn add_chunk(&mut self, chunk_text: String, style: Style) {
+        let ends_in_newline = chunk_text.ends_with("\n");
 
-                self.add_span(chunk_text, style);
-                self.end_line();
-
-                // Recursion!
-                // If the newline was the last char, this chunk will be empty
-                if !rest.is_empty() {
-                    self.add_chunk(rest, style);
-                }
+        // The first line should extend the final line of the current text,
+        // because there isn't necessarily a line break between chunks
+        let mut lines = chunk_text.lines();
+        if let Some(first_line) = lines.next() {
+            if !first_line.is_empty() {
+                self.text
+                    .push_span(Span::styled(first_line.to_owned(), style));
             }
-            // This chunk has no line breaks, just add it and move on
-            None => self.add_span(chunk_text, style),
+        }
+        self.text.extend(lines.map(|line| {
+            // If the text is empty, push an empty line instead of a line with
+            // a single empty chunk
+            if line.is_empty() {
+                Line::default()
+            } else {
+                // Push a span instead of a whole line, because if this is the
+                // last line, the next chunk may extend it
+                Span::styled(line.to_owned(), style).into()
+            }
+        }));
+
+        // std::lines throws away trailing newlines, but we care about them
+        // because the next chunk needs to go on a new line. We also care about
+        // keeping trailing newlines at the end of HTTP bodies, for correctness
+        if ends_in_newline {
+            self.text.push_line(Line::default());
         }
     }
 
@@ -208,26 +213,6 @@ impl TextStitcher {
             TemplateChunk::Error(_) => "Error".into(),
         }
     }
-
-    fn add_span(&mut self, text: String, style: Style) {
-        if !text.is_empty() {
-            self.next_line.push(Span::styled(text, style));
-        }
-    }
-
-    /// Add the current line to the accumulator, and start a new one
-    fn end_line(&mut self) {
-        if !self.next_line.is_empty() {
-            self.completed_lines
-                .push(mem::take(&mut self.next_line).into());
-        }
-    }
-
-    /// Convert all lines into a text block
-    fn into_text(mut self) -> Text<'static> {
-        self.end_line(); // Make sure to include whatever wasn't finished
-        Text::from(self.completed_lines)
-    }
 }
 
 #[cfg(test)]
@@ -248,20 +233,25 @@ mod tests {
     #[case::line_breaks(
         // Test these cases related to line breaks:
         // - Line break within a raw chunk
+        // - Chunk is just a line break
         // - Line break within a rendered chunk
         // - Line break at chunk boundary
         // - NO line break at chunk boundary
-        "intro\n{{user_id}} 💚💙💜 {{unknown}}\noutro\r\nmore outro",
+        // - Consecutive line breaks
+        "intro\n{{simple}}\n{{emoji}} 💚💙💜 {{unknown}}\n\noutro\r\nmore outro\n",
         vec![
             Line::from("intro"),
+            Line::from(rendered("ww")),
             Line::from(rendered("🧡")),
             Line::from(vec![
                 rendered("💛"),
                 Span::raw(" 💚💙💜 "),
                 error("Error"),
             ]),
-            Line::from("outro\r"), // \r shouldn't create any issues
+            Line::from(""),
+            Line::from("outro"),
             Line::from("more outro"),
+            Line::from(""), // Trailing newline
         ]
     )]
     #[case::binary(
@@ -275,7 +265,10 @@ mod tests {
         #[case] template: Template,
         #[case] expected: Vec<Line<'static>>,
     ) {
-        let profile_data = indexmap! { "user_id".into() => "🧡\n💛".into() };
+        let profile_data = indexmap! {
+            "simple".into() => "ww".into(),
+            "emoji".into() => "🧡\n💛".into()
+        };
         let profile = Profile {
             data: profile_data,
             ..Profile::factory(())