From 3991e021ba9609425d494c47ac870f85f9932b9f Mon Sep 17 00:00:00 2001 From: Lucas Pickering Date: Sat, 31 Aug 2024 15:50:58 -0400 Subject: [PATCH] Don't store large request bodies in the DB --- crates/slumber_cli/src/commands/request.rs | 2 +- crates/slumber_config/src/lib.rs | 10 +++-- crates/slumber_core/src/db.rs | 2 +- crates/slumber_core/src/db/migrations.rs | 2 +- crates/slumber_core/src/http.rs | 30 ++++++++++++-- crates/slumber_core/src/http/models.rs | 24 +++++++---- crates/slumber_core/src/test_util.rs | 7 +++- crates/slumber_tui/src/context.rs | 2 +- .../src/view/component/request_view.rs | 40 +++++++++++++++---- 9 files changed, 91 insertions(+), 28 deletions(-) diff --git a/crates/slumber_cli/src/commands/request.rs b/crates/slumber_cli/src/commands/request.rs index 9018c3f1..ab855b70 100644 --- a/crates/slumber_cli/src/commands/request.rs +++ b/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 { diff --git a/crates/slumber_config/src/lib.rs b/crates/slumber_config/src/lib.rs index b8625eab..ed4c90f1 100644 --- a/crates/slumber_config/src/lib.rs +++ b/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, - /// TLS cert errors on these hostnames are ignored. Be careful! - pub ignore_certificate_hosts: Vec, + 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(), diff --git a/crates/slumber_core/src/db.rs b/crates/slumber_core/src/db.rs index 45e0d95a..4a84647f 100644 --- a/crates/slumber_core/src/db.rs +++ b/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), diff --git a/crates/slumber_core/src/db/migrations.rs b/crates/slumber_core/src/db/migrations.rs index ba5bb178..c5624133 100644 --- a/crates/slumber_core/src/db/migrations.rs +++ b/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), diff --git a/crates/slumber_core/src/http.rs b/crates/slumber_core/src/http.rs index 66112aff..5998ffaf 100644 --- a/crates/slumber_core/src/http.rs +++ b/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)>, + 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, + /// 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 + } } } diff --git a/crates/slumber_core/src/http/models.rs b/crates/slumber_core/src/http/models.rs index 36a06eae..9e80767a 100644 --- a/crates/slumber_core/src/http/models.rs +++ b/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, } @@ -265,6 +266,7 @@ impl RequestRecord { seed: RequestSeed, profile_id: Option, 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> { diff --git a/crates/slumber_core/src/test_util.rs b/crates/slumber_core/src/test_util.rs index e4c46d80..220154de 100644 --- a/crates/slumber_core/src/test_util.rs +++ b/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 diff --git a/crates/slumber_tui/src/context.rs b/crates/slumber_tui/src/context.rs index 49eb9f24..1f4f8cb7 100644 --- a/crates/slumber_tui/src/context.rs +++ b/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, diff --git a/crates/slumber_tui/src/view/component/request_view.rs b/crates/slumber_tui/src/view/component/request_view.rs index 0cdf5a75..a0b9e11a 100644 --- a/crates/slumber_tui/src/view/component/request_view.rs +++ b/crates/slumber_tui/src/view/component/request_view.rs @@ -1,4 +1,5 @@ use crate::{ + context::TuiContext, message::Message, view::{ common::{ @@ -18,7 +19,7 @@ use ratatui::{layout::Layout, prelude::Constraint, text::Text, Frame}; use slumber_config::Action; use slumber_core::{ http::{content_type::ContentType, RequestId, RequestRecord}, - util::MaybeStr, + util::{format_byte_size, MaybeStr}, }; use std::sync::Arc; use strum::{EnumCount, EnumIter}; @@ -157,10 +158,35 @@ impl Draw for RequestView { /// body to prevent a self-reference fn init_body(request: &RequestRecord) -> Option> { let content_type = ContentType::from_headers(&request.headers).ok(); - request.body.as_ref().map(|body| { - highlight::highlight_if( - content_type, - format!("{:#}", MaybeStr(body)).into(), - ) - }) + request + .body() + .map(|body| { + highlight::highlight_if( + content_type, + format!("{:#}", MaybeStr(body)).into(), + ) + }) + .or_else(|| { + // No body available: check if it's because the recipe has no body, + // or if we threw it away. This will have some false + // positives/negatives if the recipe had a body added/removed, but + // it's good enough + let collection = ViewContext::collection(); + let recipe = + collection.recipes.get(&request.recipe_id)?.recipe()?; + if recipe.body.is_some() { + let config = &TuiContext::get().config; + + Some( + format!( + "Body not available. Streamed bodies, or bodies over \ + {}, are not persisted", + format_byte_size(config.http.large_body_size) + ) + .into(), + ) + } else { + None + } + }) }