Skip to content

Commit

Permalink
Don't store large request bodies in the DB
Browse files Browse the repository at this point in the history
  • Loading branch information
LucasPickering committed Aug 31, 2024
1 parent d27557e commit 3991e02
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 28 deletions.
2 changes: 1 addition & 1 deletion crates/slumber_cli/src/commands/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 6 additions & 4 deletions crates/slumber_config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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,
Expand Down Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion crates/slumber_core/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion crates/slumber_core/src/db/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
30 changes: 26 additions & 4 deletions crates/slumber_core/src/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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((
Expand All @@ -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,
}
}

Expand Down Expand Up @@ -166,6 +169,7 @@ impl HttpEngine {
seed,
template_context.selected_profile.clone(),
&request,
self.large_body_size,
)
.into(),
client: client.clone(),
Expand Down Expand Up @@ -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
}
}
}

Expand Down
24 changes: 17 additions & 7 deletions crates/slumber_core/src/http/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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>,
}

Expand All @@ -265,6 +266,7 @@ impl RequestRecord {
seed: RequestSeed,
profile_id: Option<ProfileId>,
request: &Request,
max_body_size: usize,
) -> Self {
Self {
id: seed.id,
Expand All @@ -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()),
}
}

Expand Down Expand Up @@ -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>> {
Expand Down
7 changes: 5 additions & 2 deletions crates/slumber_core/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use crate::{
collection::{ChainSource, HasId},
http::HttpEngine,
http::{HttpEngine, HttpEngineConfig},
template::{Prompt, Prompter},
util::{get_repo_root, ResultTraced},
};
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/slumber_tui/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
40 changes: 33 additions & 7 deletions crates/slumber_tui/src/view/component/request_view.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::{
context::TuiContext,
message::Message,
view::{
common::{
Expand All @@ -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};
Expand Down Expand Up @@ -157,10 +158,35 @@ impl Draw<RequestViewProps> for RequestView {
/// body to prevent a self-reference
fn init_body(request: &RequestRecord) -> Option<Text<'static>> {
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
}
})
}

0 comments on commit 3991e02

Please sign in to comment.