From 7153e890735920463e130a82c2d62b8748567d69 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 2 Jan 2025 15:23:17 +0100 Subject: [PATCH] feat(dif): Fail `debug-files upload` when file is too big Previously, we only logged an easy-to-miss warning when we had to skip uploading a debug file because it was too big. Now, we instead fail the entire upload loudly, to ensure users do not miss this important information. Also, add tests to verify the new behavior. Resolves #2313 --- src/utils/dif_upload/error.rs | 43 ++++++++++++++++--- src/utils/dif_upload/mod.rs | 30 ++++++------- .../get-chunk-upload-small-max-size.json | 11 +++++ tests/integration/debug_files/upload.rs | 16 +++++++ tests/integration/test_utils/test_manager.rs | 3 ++ 5 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 tests/integration/_responses/debug_files/get-chunk-upload-small-max-size.json diff --git a/src/utils/dif_upload/error.rs b/src/utils/dif_upload/error.rs index 9b5c693b42..797c1cd9eb 100644 --- a/src/utils/dif_upload/error.rs +++ b/src/utils/dif_upload/error.rs @@ -1,5 +1,6 @@ //! Error types for the dif_upload module. +use anyhow::Result; use indicatif::HumanBytes; use thiserror::Error; @@ -21,13 +22,45 @@ pub enum ValidationError { } /// Handles a DIF validation error by logging it to console -/// at the appropriate log level. -pub fn handle(dif_name: &str, error: &ValidationError) { - let message = format!("Skipping {}: {}", dif_name, error); +/// 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!("{message}"), - ValidationError::TooLarge { .. } => log::warn!("{message}"), + | 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/mod.rs b/src/utils/dif_upload/mod.rs index abcaf462c4..7a1055ba5a 100644 --- a/src/utils/dif_upload/mod.rs +++ b/src/utils/dif_upload/mod.rs @@ -658,14 +658,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()) @@ -748,7 +748,7 @@ fn collect_auxdif<'a>( name = name ); } - return None; + return Ok(None); } }; let dif_result = match kind { @@ -764,17 +764,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 let Err(e) = options.validate_dif(&dif) { - error::handle(&name, &e); - return None; + error::handle(&name, &e)?; + return Ok(None); } - Some(dif) + Ok(Some(dif)) } /// Processes and [`DifSource`] which is expected to be an object file. @@ -784,7 +784,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, @@ -799,7 +799,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); @@ -807,7 +807,7 @@ fn collect_object_dif<'a>( Ok(archive) => archive, Err(e) => { warn!("Skipping invalid debug file {}: {}", name, e); - return collected; + return Ok(collected); } }; @@ -840,7 +840,7 @@ fn collect_object_dif<'a>( if let Object::Pe(pe) = &object { if let Ok(Some(ppdb_dif)) = extract_embedded_ppdb(pe, name.as_str()) { if let Err(e) = options.validate_dif(&ppdb_dif) { - error::handle(&ppdb_dif.name, &e); + error::handle(&ppdb_dif.name, &e)?; } else { collected.push(ppdb_dif); } @@ -884,14 +884,14 @@ fn collect_object_dif<'a>( // Skip this file if we don't want to process it. if let Err(e) = options.validate_dif(&dif) { - error::handle(&name, &e); + error::handle(&name, &e)?; continue; } collected.push(dif); } - collected + Ok(collected) } /// Resolves BCSymbolMaps and replaces hidden symbols in a `DifMatch` using 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(), }; }