diff --git a/CHANGELOG.md b/CHANGELOG.md index 94a673fa6e..4401870598 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,27 @@ "You know what they say. Fool me once, strike one, but fool me twice... strike three." — Michael Scott +## 2.41.1 + +### Various fixes & improvements + +- build: Replace `dotenv` with `dotenvy` (#2351) by @szokeasaurusrex + - This fixes a problem where multiline env variables were not supported in `.env` files + +## 2.41.0 + +### Various fixes & improvements + +- build: Bump `symbolic` to `12.13.3` (#2346) by @szokeasaurusrex +- ref(api): Replace custom deserializer with derive (#2337) by @szokeasaurusrex +- ref(sourcemaps): Reduce sourcemap upload memory usage (#2343) by @szokeasaurusrex +- build: Update `memmap2` (#2340) by @szokeasaurusrex +- ref: Fix new clippy lints (#2341) by @szokeasaurusrex +- feat(dif): Fail `debug-files upload` when file is too big (#2331) by @szokeasaurusrex +- ref(dif): Handle "too big" error with warning (#2330) by @szokeasaurusrex +- ref(dif): Create type for DIF validation errors (#2329) by @szokeasaurusrex +- ref(api): Remove unnecessary `collect` (#2333) by @szokeasaurusrex + ## 2.40.0 ### New features diff --git a/Cargo.lock b/Cargo.lock index 3a36beaf5c..284ef7d110 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -769,10 +769,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10" [[package]] -name = "dotenv" -version = "0.15.0" +name = "dotenvy" +version = "0.15.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77c90badedccf4105eca100756a0b1289e191f6fcbdadd3cee1d2f614f97da8f" +checksum = "1aaf95b3e5c8f23aa320147307562d361db0ae0d51242340f558153b4eb2439b" [[package]] name = "dunce" @@ -1635,9 +1635,9 @@ checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" [[package]] name = "memmap2" -version = "0.9.4" +version = "0.9.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fe751422e4a8caa417e13c3ea66452215d7d63e19e604f4980461212f3ae1322" +checksum = "fd3f7eed9d3848f8b98834af67102b720745c4ec028fcd0aa0239277e7de374f" dependencies = [ "libc", ] @@ -2561,7 +2561,7 @@ dependencies = [ [[package]] name = "sentry-cli" -version = "2.40.0" +version = "2.41.1" dependencies = [ "anyhow", "anylog", @@ -2578,7 +2578,7 @@ dependencies = [ "curl", "data-encoding", "dirs", - "dotenv", + "dotenvy", "elementtree", "flate2", "git2", @@ -2900,9 +2900,9 @@ checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" [[package]] name = "symbolic" -version = "12.12.3" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1ef4ac5c889f15a5adf61b10b6bc68b19172938243a697c9730e754e97ff0da" +checksum = "691b8bd52ccf372a4cce381e62663530e0b96fb5f10e83477a8ec7d5cf421979" dependencies = [ "symbolic-common", "symbolic-debuginfo", @@ -2912,9 +2912,9 @@ dependencies = [ [[package]] name = "symbolic-common" -version = "12.12.3" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e5ba5365997a4e375660bed52f5b42766475d5bc8ceb1bb13fea09c469ea0f49" +checksum = "13a4dfe4bbeef59c1f32fc7524ae7c95b9e1de5e79a43ce1604e181081d71b0c" dependencies = [ "debugid", "memmap2", @@ -2925,9 +2925,9 @@ dependencies = [ [[package]] name = "symbolic-debuginfo" -version = "12.12.3" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "40ac2369b402eaa9c4ce0409094d803e7ee6b253c8bfe76d6057e13a4028d625" +checksum = "474423ce25f811471ec4bd826511510df8a8895f6babd98f64524eda4a3978f4" dependencies = [ "debugid", "elementtree", @@ -2957,9 +2957,9 @@ dependencies = [ [[package]] name = "symbolic-il2cpp" -version = "12.12.3" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0048a96ed9341aa445084c5449b83f0c0e93710ba876662417bd92d7be780070" +checksum = "c69a83048f5c49290be14a65c49eae124e51c175bfb81c75b520827e8bfc7526" dependencies = [ "indexmap", "serde_json", @@ -2969,9 +2969,9 @@ dependencies = [ [[package]] name = "symbolic-ppdb" -version = "12.12.3" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "26c140c509e924792a140bc215cbd6851aef9036e06031f57370dbefb0ac4c51" +checksum = "b4af715d3f52faa38f3acb92c33b4de6546c45565d9a5214338e22e040d6141e" dependencies = [ "flate2", "indexmap", @@ -2985,9 +2985,9 @@ dependencies = [ [[package]] name = "symbolic-symcache" -version = "12.12.3" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b63018a17d8c6c18ef70072572a41d79abfe4016a6222260cb93a865801c83d8" +checksum = "68bf7bed4c5afea4766625b420a523a071a5142c61c0c7706a098a7b54823345" dependencies = [ "indexmap", "symbolic-common", diff --git a/Cargo.toml b/Cargo.toml index d0877aed98..38e2277c23 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ authors = ["Armin Ronacher "] build = "build.rs" name = "sentry-cli" -version = "2.40.0" +version = "2.41.1" edition = "2021" rust-version = "1.80" @@ -28,7 +28,7 @@ clap_complete = "4.4.3" console = "0.15.5" curl = { version = "0.4.46", features = ["static-curl", "static-ssl"] } dirs = "4.0.0" -dotenv = "0.15.0" +dotenvy = "0.15.7" elementtree = "1.2.3" flate2 = { version = "1.0.25", default-features = false, features = [ "rust_backend", @@ -66,7 +66,7 @@ serde = { version = "1.0.152", features = ["derive"] } serde_json = "1.0.93" sha1_smol = { version = "1.0.0", features = ["serde"] } sourcemap = { version = "9.1.2", features = ["ram_bundle"] } -symbolic = { version = "12.12.3", features = ["debuginfo-serde", "il2cpp"] } +symbolic = { version = "12.13.3", features = ["debuginfo-serde", "il2cpp"] } thiserror = "1.0.38" url = "2.3.1" username = "0.2.0" diff --git a/npm-binary-distributions/darwin/package.json b/npm-binary-distributions/darwin/package.json index 49037f8eb6..6cd67734cf 100644 --- a/npm-binary-distributions/darwin/package.json +++ b/npm-binary-distributions/darwin/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-darwin", - "version": "2.40.0", + "version": "2.41.1", "description": "The darwin distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/npm-binary-distributions/linux-arm/package.json b/npm-binary-distributions/linux-arm/package.json index 43eb7401b3..8386e2e93d 100644 --- a/npm-binary-distributions/linux-arm/package.json +++ b/npm-binary-distributions/linux-arm/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-linux-arm", - "version": "2.40.0", + "version": "2.41.1", "description": "The linux arm distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/npm-binary-distributions/linux-arm64/package.json b/npm-binary-distributions/linux-arm64/package.json index 13b05dfceb..8f9611f1c7 100644 --- a/npm-binary-distributions/linux-arm64/package.json +++ b/npm-binary-distributions/linux-arm64/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-linux-arm64", - "version": "2.40.0", + "version": "2.41.1", "description": "The linux arm64 distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/npm-binary-distributions/linux-i686/package.json b/npm-binary-distributions/linux-i686/package.json index 19647792c2..a6c89f658c 100644 --- a/npm-binary-distributions/linux-i686/package.json +++ b/npm-binary-distributions/linux-i686/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-linux-i686", - "version": "2.40.0", + "version": "2.41.1", "description": "The linux x86 and ia32 distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/npm-binary-distributions/linux-x64/package.json b/npm-binary-distributions/linux-x64/package.json index 9ac7821dc8..3909fc72c3 100644 --- a/npm-binary-distributions/linux-x64/package.json +++ b/npm-binary-distributions/linux-x64/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-linux-x64", - "version": "2.40.0", + "version": "2.41.1", "description": "The linux x64 distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/npm-binary-distributions/win32-i686/package.json b/npm-binary-distributions/win32-i686/package.json index 73558e9485..87234a8f93 100644 --- a/npm-binary-distributions/win32-i686/package.json +++ b/npm-binary-distributions/win32-i686/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-win32-i686", - "version": "2.40.0", + "version": "2.41.1", "description": "The windows x86 and ia32 distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/npm-binary-distributions/win32-x64/package.json b/npm-binary-distributions/win32-x64/package.json index 9b1895f6f7..3465d8f69a 100644 --- a/npm-binary-distributions/win32-x64/package.json +++ b/npm-binary-distributions/win32-x64/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli-win32-x64", - "version": "2.40.0", + "version": "2.41.1", "description": "The windows x64 distribution of the Sentry CLI binary.", "repository": "https://github.com/getsentry/sentry-cli", "license": "BSD-3-Clause", diff --git a/package.json b/package.json index e4714439d1..48a5038fea 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@sentry/cli", - "version": "2.40.0", + "version": "2.41.1", "description": "A command line utility to work with Sentry. https://docs.sentry.io/hosted/learn/cli/", "repository": "git://github.com/getsentry/sentry-cli.git", "homepage": "https://docs.sentry.io/hosted/learn/cli/", @@ -30,13 +30,13 @@ "prettier": "2.8.8" }, "optionalDependencies": { - "@sentry/cli-darwin": "2.40.0", - "@sentry/cli-linux-arm": "2.40.0", - "@sentry/cli-linux-arm64": "2.40.0", - "@sentry/cli-linux-i686": "2.40.0", - "@sentry/cli-linux-x64": "2.40.0", - "@sentry/cli-win32-i686": "2.40.0", - "@sentry/cli-win32-x64": "2.40.0" + "@sentry/cli-darwin": "2.41.1", + "@sentry/cli-linux-arm": "2.41.1", + "@sentry/cli-linux-arm64": "2.41.1", + "@sentry/cli-linux-i686": "2.41.1", + "@sentry/cli-linux-x64": "2.41.1", + "@sentry/cli-win32-i686": "2.41.1", + "@sentry/cli-win32-x64": "2.41.1" }, "scripts": { "postinstall": "node ./scripts/install.js", diff --git a/src/api/data_types/chunking/compression.rs b/src/api/data_types/chunking/compression.rs index 7bb1a1eadb..6c0660c802 100644 --- a/src/api/data_types/chunking/compression.rs +++ b/src/api/data_types/chunking/compression.rs @@ -1,16 +1,18 @@ use std::fmt; -use serde::{Deserialize, Deserializer}; +use serde::Deserialize; -#[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Default)] +#[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Default, Deserialize)] +#[serde(rename_all = "lowercase")] pub enum ChunkCompression { - /// No compression should be applied - #[default] - Uncompressed = 0, /// GZIP compression (including header) Gzip = 10, /// Brotli compression Brotli = 20, + /// No compression should be applied + #[default] + #[serde(other)] + Uncompressed = 0, } impl ChunkCompression { @@ -32,17 +34,3 @@ impl fmt::Display for ChunkCompression { } } } - -impl<'de> Deserialize<'de> for ChunkCompression { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - Ok(match String::deserialize(deserializer)?.as_str() { - "gzip" => ChunkCompression::Gzip, - "brotli" => ChunkCompression::Brotli, - // We do not know this compression, so we assume no compression - _ => ChunkCompression::Uncompressed, - }) - } -} diff --git a/src/api/mod.rs b/src/api/mod.rs index 931fa848cf..ab1cd5ad10 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -379,11 +379,10 @@ impl Api { // Curl stores a raw pointer to the stringified checksum internally. We first // transform all checksums to string and keep them in scope until the request // has completed. The original iterator is not needed anymore after this. - let stringified_chunks: Vec<_> = chunks + let stringified_chunks = chunks .into_iter() .map(T::as_ref) - .map(|&(checksum, data)| (checksum.to_string(), data)) - .collect(); + .map(|&(checksum, data)| (checksum.to_string(), data)); let mut form = curl::easy::Form::new(); for (ref checksum, data) in stringified_chunks { diff --git a/src/commands/debug_files/bundle_jvm.rs b/src/commands/debug_files/bundle_jvm.rs index edad1b7722..7c0c20d058 100644 --- a/src/commands/debug_files/bundle_jvm.rs +++ b/src/commands/debug_files/bundle_jvm.rs @@ -12,6 +12,7 @@ use std::collections::BTreeMap; use std::fs; use std::path::PathBuf; use std::str::FromStr; +use std::sync::Arc; use symbolic::debuginfo::sourcebundle::SourceFileType; pub fn make_command(command: Command) -> Command { @@ -97,7 +98,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { SourceFile { url, path: source.path.clone(), - contents: source.contents.clone(), + contents: Arc::new(source.contents.clone()), ty: SourceFileType::Source, headers: BTreeMap::new(), messages: vec![], diff --git a/src/commands/debug_files/bundle_sources.rs b/src/commands/debug_files/bundle_sources.rs index e80c41d2c9..dac41fd980 100644 --- a/src/commands/debug_files/bundle_sources.rs +++ b/src/commands/debug_files/bundle_sources.rs @@ -32,7 +32,7 @@ pub fn make_command(command: Command) -> Command { } fn is_dsym(path: &Path) -> bool { - path.extension().map_or(false, |e| e == "dSYM") + path.extension().is_some_and(|e| e == "dSYM") } fn get_sane_parent(path: &Path) -> &Path { diff --git a/src/commands/files/upload.rs b/src/commands/files/upload.rs index 5972fe63fb..c43729bb60 100644 --- a/src/commands/files/upload.rs +++ b/src/commands/files/upload.rs @@ -3,6 +3,7 @@ use std::ffi::OsStr; use std::fs; use std::io::Read; use std::path::Path; +use std::sync::Arc; use std::time::Duration; use anyhow::{bail, format_err, Result}; @@ -205,7 +206,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { SourceFile { url, path: source.path.clone(), - contents: source.contents.clone(), + contents: Arc::new(source.contents.clone()), ty: SourceFileType::Source, headers: headers.clone(), messages: vec![], diff --git a/src/utils/dif_upload/error.rs b/src/utils/dif_upload/error.rs new file mode 100644 index 0000000000..797c1cd9eb --- /dev/null +++ b/src/utils/dif_upload/error.rs @@ -0,0 +1,66 @@ +//! Error types for the dif_upload module. + +use anyhow::Result; +use indicatif::HumanBytes; +use thiserror::Error; + +/// Represents an error that makes a DIF invalid. +#[derive(Debug, Error)] +pub enum ValidationError { + #[error("Invalid format")] + InvalidFormat, + #[error("Invalid features")] + InvalidFeatures, + #[error("Invalid debug ID")] + InvalidDebugId, + #[error( + "Debug file's size ({}) exceeds the maximum allowed size ({})", + HumanBytes(*size as u64), + HumanBytes(*max_size) + )] + TooLarge { size: usize, max_size: u64 }, +} + +/// Handles a DIF validation error by logging it to console +/// at the appropriate log level. Or, if the error should stop +/// the upload, it will return an error, that can be propagated +/// to the caller. +pub fn handle(dif_name: &str, error: &ValidationError) -> Result<()> { + let message = format!("{}: {}", dif_name, error); + match error { + ValidationError::InvalidFormat + | ValidationError::InvalidFeatures + | ValidationError::InvalidDebugId => log::debug!("Skipping {message}"), + ValidationError::TooLarge { .. } => { + anyhow::bail!("Upload failed due to error in debug file {message}") + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + use rstest::rstest; + + #[rstest] + #[case(ValidationError::InvalidFormat)] + #[case(ValidationError::InvalidFeatures)] + #[case(ValidationError::InvalidDebugId)] + fn test_handle_should_not_error(#[case] error: ValidationError) { + let result = handle("test", &error); + assert!(result.is_ok()); + } + + #[test] + fn test_handle_should_error() { + let error = ValidationError::TooLarge { + size: 1000, + max_size: 100, + }; + let result = handle("test", &error); + assert!(result.is_err()); + } +} diff --git a/src/utils/dif_upload.rs b/src/utils/dif_upload/mod.rs similarity index 96% rename from src/utils/dif_upload.rs rename to src/utils/dif_upload/mod.rs index f905ae696e..fc5e6ee06f 100644 --- a/src/utils/dif_upload.rs +++ b/src/utils/dif_upload/mod.rs @@ -1,6 +1,8 @@ //! Searches, processes and uploads debug information files (DIFs). See //! `DifUpload` for more information. +mod error; + use std::borrow::Cow; use std::collections::{BTreeMap, BTreeSet}; use std::convert::TryInto; @@ -18,7 +20,6 @@ use std::time::Duration; use anyhow::{bail, format_err, Error, Result}; use console::style; -use indicatif::HumanBytes; use log::{debug, info, warn}; use sha1_smol::Digest; use symbolic::common::{Arch, AsSelf, ByteView, DebugId, SelfCell, Uuid}; @@ -32,6 +33,7 @@ use which::which; use zip::result::ZipError; use zip::{write::FileOptions, ZipArchive, ZipWriter}; +use self::error::ValidationError; use crate::api::{Api, ChunkServerOptions, ChunkUploadCapability}; use crate::config::Config; use crate::constants::{DEFAULT_MAX_DIF_SIZE, DEFAULT_MAX_WAIT}; @@ -589,10 +591,7 @@ fn find_uuid_plists( // └─ DWARF // └─ App let plist_name = format!("{:X}.plist", uuid.as_hyphenated()); - let plist = match source.get_relative(format!("../{}", &plist_name)) { - Some(plist) => plist, - None => return None, - }; + let plist = source.get_relative(format!("../{}", &plist_name))?; let mut plists = BTreeMap::new(); plists.insert(plist_name, plist); @@ -656,14 +655,14 @@ fn search_difs(options: &DifUpload) -> Result>> { if Archive::peek(&buffer) != FileFormat::Unknown { let mut difs = - collect_object_dif(source, name, buffer, options, &mut age_overrides); + collect_object_dif(source, name, buffer, options, &mut age_overrides)?; collected.append(difs.as_mut()); } else if BcSymbolMap::test(&buffer) { - if let Some(dif) = collect_auxdif(name, buffer, options, AuxDifKind::BcSymbolMap) { + if let Some(dif) = collect_auxdif(name, buffer, options, AuxDifKind::BcSymbolMap)? { collected.push(dif); } } else if buffer.starts_with(b"( buffer: ByteView<'static>, options: &DifUpload, kind: AuxDifKind, -) -> Option> { +) -> Result>> { let file_stem = Path::new(&name) .file_stem() .map(|stem| stem.to_string_lossy()) @@ -746,7 +745,7 @@ fn collect_auxdif<'a>( name = name ); } - return None; + return Ok(None); } }; let dif_result = match kind { @@ -762,16 +761,17 @@ fn collect_auxdif<'a>( name = name, err = err ); - return None; + return Ok(None); } }; // Skip this file if we don't want to process it. - if !options.validate_dif(&dif) { - return None; + if let Err(e) = options.validate_dif(&dif) { + error::handle(&name, &e)?; + return Ok(None); } - Some(dif) + Ok(Some(dif)) } /// Processes and [`DifSource`] which is expected to be an object file. @@ -781,7 +781,7 @@ fn collect_object_dif<'a>( buffer: ByteView<'static>, options: &DifUpload, age_overrides: &mut BTreeMap, -) -> Vec> { +) -> Result>> { let mut collected = Vec::with_capacity(2); // Try to parse a potential object file. If this is not possible, @@ -796,7 +796,7 @@ fn collect_object_dif<'a>( format == FileFormat::Pe && options.valid_format(DifFormat::Object(FileFormat::Pdb)); if !should_override_age && !options.valid_format(DifFormat::Object(format)) { - return collected; + return Ok(collected); } debug!("trying to parse dif {}", name); @@ -804,7 +804,7 @@ fn collect_object_dif<'a>( Ok(archive) => archive, Err(e) => { warn!("Skipping invalid debug file {}: {}", name, e); - return collected; + return Ok(collected); } }; @@ -836,7 +836,9 @@ fn collect_object_dif<'a>( // If this is a PE file with an embedded Portable PDB, we extract and process the PPDB separately. if let Object::Pe(pe) = &object { if let Ok(Some(ppdb_dif)) = extract_embedded_ppdb(pe, name.as_str()) { - if options.validate_dif(&ppdb_dif) { + if let Err(e) = options.validate_dif(&ppdb_dif) { + error::handle(&ppdb_dif.name, &e)?; + } else { collected.push(ppdb_dif); } } @@ -878,14 +880,15 @@ fn collect_object_dif<'a>( }; // Skip this file if we don't want to process it. - if !options.validate_dif(&dif) { + if let Err(e) = options.validate_dif(&dif) { + error::handle(&name, &e)?; continue; } collected.push(dif); } - collected + Ok(collected) } /// Resolves BCSymbolMaps and replaces hidden symbols in a `DifMatch` using @@ -1691,7 +1694,7 @@ impl<'a> DifUpload<'a> { /// Determines if this file extension matches the search criteria. fn valid_extension(&self, ext: Option<&OsStr>) -> bool { - self.extensions.is_empty() || ext.map_or(false, |e| self.extensions.contains(e)) + self.extensions.is_empty() || ext.is_some_and(|e| self.extensions.contains(e)) } /// Determines if this [`DifFormat`] matches the search criteria. @@ -1723,21 +1726,13 @@ impl<'a> DifUpload<'a> { } /// Checks if a file is too large and logs skip message if so. - fn valid_size(&self, name: &str, size: usize) -> bool { + fn valid_size(&self, size: usize) -> bool { let file_size: Result = size.try_into(); - let too_large = match file_size { - Ok(file_size) => file_size > self.max_file_size, - Err(_) => true, - }; - if too_large { - warn!( - "Skipping debug file since it exceeds {}: {} ({})", - HumanBytes(self.max_file_size), - name, - HumanBytes(file_size.unwrap_or(u64::MAX)), - ); + + match file_size { + Ok(file_size) => file_size <= self.max_file_size, + Err(_) => false, // Definitely too big } - !too_large } /// Validates DIF on whether it should be processed. @@ -1745,33 +1740,28 @@ impl<'a> DifUpload<'a> { /// This takes all the filters configured in the [`DifUpload`] into account and returns /// whether a file should be skipped or not. It also takes care of logging such a skip /// if required. - fn validate_dif(&self, dif: &DifMatch) -> bool { - // Skip if we didn't want this kind of DIF. + fn validate_dif(&self, dif: &DifMatch) -> Result<(), ValidationError> { if !self.valid_format(dif.format()) { - debug!("skipping {} because of format", dif.name); - return false; + return Err(ValidationError::InvalidFormat); } - // Skip if this DIF does not have features we want. if !self.valid_features(dif) { - debug!("skipping {} because of features", dif.name); - return false; + return Err(ValidationError::InvalidFeatures); } - // Skip if this DIF has no DebugId or we are only looking for certain IDs. let id = dif.debug_id.unwrap_or_default(); if id.is_nil() || !self.valid_id(id) { - debug!("skipping {} because of debugid", dif.name); - return false; + return Err(ValidationError::InvalidDebugId); } - // Skip if file exceeds the maximum allowed file size. - if !self.valid_size(&dif.name, dif.data().len()) { - debug!("skipping {} because of size", dif.name); - return false; + if !self.valid_size(dif.data().len()) { + return Err(ValidationError::TooLarge { + size: dif.data().len(), + max_size: self.max_file_size, + }); } - true + Ok(()) } fn into_chunk_options(self, server_options: ChunkServerOptions) -> ChunkOptions<'a> { diff --git a/src/utils/file_search.rs b/src/utils/file_search.rs index 9d3d39acec..32deddda3d 100644 --- a/src/utils/file_search.rs +++ b/src/utils/file_search.rs @@ -136,7 +136,7 @@ impl ReleaseFileSearch { for result in builder.build() { let file = result?; - if file.file_type().map_or(false, |t| t.is_dir()) { + if file.file_type().is_some_and(|t| t.is_dir()) { continue; } pb.set_message(&format!("{}", file.path().display())); diff --git a/src/utils/file_upload.rs b/src/utils/file_upload.rs index 45014b7781..1e59e8b2a3 100644 --- a/src/utils/file_upload.rs +++ b/src/utils/file_upload.rs @@ -42,7 +42,7 @@ pub fn initialize_legacy_release_upload(context: &UploadContext) -> Result<()> { // if a project is provided which is technically unnecessary for the // legacy upload though it will unlikely to be what users want. if context.project.is_some() - && context.chunk_upload_options.map_or(false, |x| { + && context.chunk_upload_options.is_some_and(|x| { x.supports(ChunkUploadCapability::ArtifactBundles) || x.supports(ChunkUploadCapability::ArtifactBundlesV2) }) @@ -119,7 +119,7 @@ impl fmt::Display for LogLevel { pub struct SourceFile { pub url: String, pub path: PathBuf, - pub contents: Vec, + pub contents: Arc>, pub ty: SourceFileType, /// A map of headers attached to the source file. /// @@ -134,7 +134,7 @@ pub struct SourceFile { impl SourceFile { /// Calculates and returns the SHA1 checksum of the file. pub fn checksum(&self) -> Result { - get_sha1_checksum(&*self.contents) + get_sha1_checksum(&**self.contents) } /// Returns the value of the "debug-id" header. @@ -670,7 +670,9 @@ mod tests { let file = SourceFile { url: format!("~/{name}"), path: format!("tests/integration/_fixtures/{name}").into(), - contents: std::fs::read(format!("tests/integration/_fixtures/{name}")).unwrap(), + contents: std::fs::read(format!("tests/integration/_fixtures/{name}")) + .unwrap() + .into(), ty: SourceFileType::SourceMap, headers: Default::default(), messages: Default::default(), diff --git a/src/utils/sourcemaps.rs b/src/utils/sourcemaps.rs index 411c190887..1700057aba 100644 --- a/src/utils/sourcemaps.rs +++ b/src/utils/sourcemaps.rs @@ -6,6 +6,7 @@ use std::mem; use std::path::{Path, PathBuf}; use std::str; use std::str::FromStr; +use std::sync::Arc; use anyhow::{anyhow, bail, Context, Error, Result}; use console::style; @@ -326,7 +327,7 @@ impl SourceMapProcessor { let mut source_file = SourceFile { url: url.clone(), path: file.path, - contents: file.contents, + contents: file.contents.into(), ty, headers: BTreeMap::new(), messages: vec![], @@ -569,7 +570,7 @@ impl SourceMapProcessor { SourceFile { url: sourcemap_source.url.clone(), path: sourcemap_source.path.clone(), - contents: index_sourcemap_content, + contents: index_sourcemap_content.into(), ty: SourceFileType::SourceMap, headers: sourcemap_source.headers.clone(), messages: sourcemap_source.messages.clone(), @@ -590,7 +591,7 @@ impl SourceMapProcessor { SourceFile { url: source_url.clone(), path: PathBuf::from(name.clone()), - contents: sourceview.source().as_bytes().to_vec(), + contents: sourceview.source().as_bytes().to_vec().into(), ty: SourceFileType::MinifiedSource, headers: BTreeMap::new(), messages: vec![], @@ -608,7 +609,7 @@ impl SourceMapProcessor { SourceFile { url: sourcemap_url.clone(), path: PathBuf::from(sourcemap_name), - contents: sourcemap_content, + contents: sourcemap_content.into(), ty: SourceFileType::SourceMap, headers: BTreeMap::new(), messages: vec![], @@ -682,7 +683,7 @@ impl SourceMapProcessor { .flatten_and_rewrite(&options)? .to_writer(&mut new_source)?, }; - source.contents = new_source; + source.contents = new_source.into(); pb.inc(1); } pb.finish_with_duration("Rewriting"); @@ -936,7 +937,7 @@ impl SourceMapProcessor { // If we don't have a sourcemap, it's not safe to inject the code snippet at the beginning, // because that would throw off all the mappings. Instead, inject the snippet at the very end. // This isn't ideal, but it's the best we can do in this case. - inject::fixup_js_file_end(&mut source_file.contents, debug_id) + inject::fixup_js_file_end(Arc::make_mut(&mut source_file.contents), debug_id) .context(format!("Failed to process {}", source_file.path.display()))?; debug_id } @@ -958,10 +959,9 @@ impl SourceMapProcessor { .unwrap_or_else(|| inject::debug_id_from_bytes_hashed(&decoded)); let source_file = self.sources.get_mut(source_url).unwrap(); - let adjustment_map = - inject::fixup_js_file(&mut source_file.contents, debug_id).context( - format!("Failed to process {}", source_file.path.display()), - )?; + let source_file_contents = Arc::make_mut(&mut source_file.contents); + let adjustment_map = inject::fixup_js_file(source_file_contents, debug_id) + .context(format!("Failed to process {}", source_file.path.display()))?; sourcemap.adjust_mappings(&adjustment_map); sourcemap.set_debug_id(Some(debug_id)); @@ -972,10 +972,7 @@ impl SourceMapProcessor { let encoded = data_encoding::BASE64.encode(&decoded); let new_sourcemap_url = format!("{DATA_PREAMBLE}{encoded}"); - inject::replace_sourcemap_url( - &mut source_file.contents, - &new_sourcemap_url, - )?; + inject::replace_sourcemap_url(source_file_contents, &new_sourcemap_url)?; *sourcemap_url = Some(SourceMapReference::from_url(new_sourcemap_url)); debug_id @@ -1022,19 +1019,20 @@ impl SourceMapProcessor { }; let source_file = self.sources.get_mut(source_url).unwrap(); - let adjustment_map = - inject::fixup_js_file(&mut source_file.contents, debug_id) - .context(format!( - "Failed to process {}", - source_file.path.display() - ))?; + let adjustment_map = inject::fixup_js_file( + Arc::make_mut(&mut source_file.contents), + debug_id, + ) + .context(format!("Failed to process {}", source_file.path.display()))?; sourcemap.adjust_mappings(&adjustment_map); sourcemap.set_debug_id(Some(debug_id)); let sourcemap_file = self.sources.get_mut(&sourcemap_url).unwrap(); - sourcemap_file.contents.clear(); - sourcemap.to_writer(&mut sourcemap_file.contents)?; + let sourcemap_file_contents = + Arc::make_mut(&mut sourcemap_file.contents); + sourcemap_file_contents.clear(); + sourcemap.to_writer(sourcemap_file_contents)?; sourcemap_file.set_debug_id(debug_id.to_string()); @@ -1069,11 +1067,11 @@ impl SourceMapProcessor { // If we don't have a sourcemap, it's not safe to inject the code snippet at the beginning, // because that would throw off all the mappings. Instead, inject the snippet at the very end. // This isn't ideal, but it's the best we can do in this case. - inject::fixup_js_file_end(&mut source_file.contents, debug_id) - .context(format!( - "Failed to process {}", - source_file.path.display() - ))?; + inject::fixup_js_file_end( + Arc::make_mut(&mut source_file.contents), + debug_id, + ) + .context(format!("Failed to process {}", source_file.path.display()))?; debug_id } diff --git a/src/utils/sourcemaps/inject.rs b/src/utils/sourcemaps/inject.rs index 24cb27276e..571591dece 100644 --- a/src/utils/sourcemaps/inject.rs +++ b/src/utils/sourcemaps/inject.rs @@ -284,7 +284,7 @@ pub fn find_matching_paths(candidate_paths: &[String], expected_path: &str) -> V while candidate_segments .peek() .zip(expected_segments.peek()) - .map_or(false, |(x, y)| x == y) + .is_some_and(|(x, y)| x == y) { candidate_segments.next(); expected_segments.next(); diff --git a/src/utils/system.rs b/src/utils/system.rs index bd42138e79..d76e5b501e 100644 --- a/src/utils/system.rs +++ b/src/utils/system.rs @@ -5,7 +5,7 @@ use std::process; use anyhow::{Error, Result}; use console::style; -use dotenv::Result as DotenvResult; +use dotenvy::Result as DotenvResult; use lazy_static::lazy_static; use regex::{Captures, Regex}; @@ -139,8 +139,8 @@ pub fn load_dotenv() -> DotenvResult<()> { } match env::var("SENTRY_DOTENV_PATH") { - Ok(path) => dotenv::from_path(path), - Err(_) => dotenv::dotenv().map(|_| ()), + Ok(path) => dotenvy::from_path(path), + Err(_) => dotenvy::dotenv().map(|_| ()), } .map_or_else( |error| { diff --git a/tests/integration/_responses/debug_files/get-chunk-upload-small-max-size.json b/tests/integration/_responses/debug_files/get-chunk-upload-small-max-size.json new file mode 100644 index 0000000000..28d8e4f5bf --- /dev/null +++ b/tests/integration/_responses/debug_files/get-chunk-upload-small-max-size.json @@ -0,0 +1,11 @@ +{ + "url": "organizations/wat-org/chunk-upload/", + "chunkSize": 8388608, + "chunksPerRequest": 64, + "maxFileSize": 2048, + "maxRequestSize": 33554432, + "concurrency": 8, + "hashAlgorithm": "sha1", + "compression": ["gzip"], + "accept": ["debug_files", "release_files", "pdbs", "portablepdbs", "sources", "bcsymbolmaps"] +} diff --git a/tests/integration/debug_files/upload.rs b/tests/integration/debug_files/upload.rs index 080a3b9d4d..5f060294f7 100644 --- a/tests/integration/debug_files/upload.rs +++ b/tests/integration/debug_files/upload.rs @@ -731,3 +731,19 @@ fn chunk_upload_multiple_files_small_chunks_only_some() { "Uploaded chunks differ from the expected chunks" ); } + +#[test] +fn test_dif_too_big() { + TestManager::new() + .mock_endpoint( + MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/") + .with_response_file("debug_files/get-chunk-upload-small-max-size.json"), + ) + .assert_cmd(vec![ + "debug-files", + "upload", + "tests/integration/_fixtures/SrcGenSampleApp.pdb", + ]) + .with_default_token() + .run_and_assert(AssertCommand::Failure); +} diff --git a/tests/integration/test_utils/test_manager.rs b/tests/integration/test_utils/test_manager.rs index be0e338167..6aec668f0d 100644 --- a/tests/integration/test_utils/test_manager.rs +++ b/tests/integration/test_utils/test_manager.rs @@ -185,6 +185,8 @@ pub struct AssertCmdTestManager { pub enum AssertCommand { /// Assert that the command succeeds (i.e. returns a `0` exit code). Success, + /// Assert that the command fails (i.e. returns a non-zero exit code). + Failure, } impl AssertCmdTestManager { @@ -219,6 +221,7 @@ impl AssertCmdTestManager { match assert { AssertCommand::Success => command_result.success(), + AssertCommand::Failure => command_result.failure(), }; }