From 708f4575e91c758eec6e18a1758f7135475cdcd7 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Mon, 4 Mar 2024 11:24:15 +0100 Subject: [PATCH] fix(sourcemaps): Remove distinction between `Source` and `MinifiedSource` for `sourcemaps inject` (#1958) Until now, the sentry-cli sourcemaps inject command had only been injecting debug IDs into files that we had identified as being minified. However, the intended behavior for this command is for the command to inject debug IDs into all JS source files at the user-specified path; we should not distinguish between minified and non-minified files at all. This PR implements the intended behavior. Users who currently call the sourcemap inject command with a path containing files that should not be injected with debug IDs (such as a path containing both transpiled and source JS files) should update the path passed to only include transpiled JS files, or should use other command line arguments (e.g. --ignore) to exclude the source files from having debug IDs injected. This PR is intended to provide a quick fix to implement the behavior. In the future, we plan to completely refactor and simplify the logic powering the sourcemaps inject command. Fixes GH-1955 --- src/utils/sourcemaps.rs | 48 +++++++------------ ...urce-maps-release_and_dist_from_env.trycmd | 3 +- .../sourcemaps-inject-nomappings.trycmd | 1 + .../sourcemaps-inject-not-compiled.trycmd | 5 ++ .../sourcemaps/sourcemaps-inject.trycmd | 1 + .../sourcemaps-upload-some-debugids.trycmd | 3 +- .../inject-not-compiled/not-compiled.js | 5 ++ tests/integration/sourcemaps/inject.rs | 20 ++++++++ 8 files changed, 54 insertions(+), 32 deletions(-) create mode 100644 tests/integration/_cases/sourcemaps/sourcemaps-inject-not-compiled.trycmd create mode 100644 tests/integration/_fixtures/inject-not-compiled/not-compiled.js diff --git a/src/utils/sourcemaps.rs b/src/utils/sourcemaps.rs index 3adcfdbdcc..ccd1237e1b 100644 --- a/src/utils/sourcemaps.rs +++ b/src/utils/sourcemaps.rs @@ -21,7 +21,6 @@ use symbolic::debuginfo::sourcebundle::SourceFileType; use url::Url; use crate::api::Api; -use crate::utils::enc::decode_unknown_string; use crate::utils::file_search::ReleaseFileMatch; use crate::utils::file_upload::{ initialize_legacy_release_upload, FileUpload, SourceFile, SourceFiles, UploadContext, @@ -37,20 +36,6 @@ pub mod inject; /// Data URLs are used to embed sourcemaps directly in javascript source files. const DATA_PREAMBLE: &str = "data:application/json;base64,"; -fn is_likely_minified_js(code: &[u8]) -> bool { - // if we have a debug id or source maps location reference, this is a minified file - if let Ok(code) = std::str::from_utf8(code) { - if discover_debug_id(code).is_some() || discover_sourcemaps_location(code).is_some() { - return true; - } - } - if let Ok(code_str) = decode_unknown_string(code) { - might_be_minified::analyze_str(&code_str).is_likely_minified() - } else { - false - } -} - fn join_url(base_url: &str, url: &str) -> Result { if base_url.starts_with("~/") { match Url::parse(&format!("http://{base_url}"))?.join(url) { @@ -300,20 +285,6 @@ impl SourceMapProcessor { && sourcemap::ram_bundle::is_ram_bundle_slice(&file.contents) { (SourceFileType::IndexedRamBundle, None) - } else if file - .path - .file_name() - .and_then(OsStr::to_str) - .map(|x| x.contains(".min.")) - .unwrap_or(false) - || is_likely_minified_js(&file.contents) - { - ( - SourceFileType::MinifiedSource, - std::str::from_utf8(&file.contents) - .ok() - .and_then(discover_debug_id), - ) } else if is_hermes_bytecode(&file.contents) { // This is actually a big hack: // For the react-native Hermes case, we skip uploading the bytecode bundle, @@ -323,7 +294,24 @@ impl SourceMapProcessor { file.contents.clear(); (SourceFileType::MinifiedSource, None) } else { - (SourceFileType::Source, None) + // Here, we use MinifiedSource for historical reasons. We used to guess whether + // a JS file was a minified file or a source file, and we would treat these files + // differently when uploading or injecting them. However, the desired behavior is + // and has always been to treat all JS files the same, since users should be + // responsible for providing the file paths for only files they would like to have + // uploaded or injected. The minified file guessing furthermore was not reliable, + // since minification is not a necessary step in the JS build process. + // + // We use MinifiedSource here rather than Source because we want to treat all JS + // files the way we used to treat minified files only. To use Source, we would need + // to analyze all possible code paths that check this value, and update those as + // well. To keep the change minimal, we use MinifiedSource here. + ( + SourceFileType::MinifiedSource, + std::str::from_utf8(&file.contents) + .ok() + .and_then(discover_debug_id), + ) }; let mut source_file = SourceFile { diff --git a/tests/integration/_cases/react_native/xcode-upload-source-maps-release_and_dist_from_env.trycmd b/tests/integration/_cases/react_native/xcode-upload-source-maps-release_and_dist_from_env.trycmd index 427d13c143..6c1613a129 100644 --- a/tests/integration/_cases/react_native/xcode-upload-source-maps-release_and_dist_from_env.trycmd +++ b/tests/integration/_cases/react_native/xcode-upload-source-maps-release_and_dist_from_env.trycmd @@ -22,7 +22,8 @@ Processing react-native sourcemaps for Sentry upload. Source Map Upload Report Scripts - ~/react-native-xcode-bundle.js.map (no sourcemap ref) ~/react-native-xcode-bundle.js (sourcemap at react-native-xcode-bundle.map) + ~/react-native-xcode-bundle.js.map (no sourcemap ref) + - warning: could not determine a source map reference (Could not auto-detect referenced sourcemap for ~/react-native-xcode-bundle.js.map) ``` diff --git a/tests/integration/_cases/sourcemaps/sourcemaps-inject-nomappings.trycmd b/tests/integration/_cases/sourcemaps/sourcemaps-inject-nomappings.trycmd index a8f622c2a6..32e09d3d6c 100644 --- a/tests/integration/_cases/sourcemaps/sourcemaps-inject-nomappings.trycmd +++ b/tests/integration/_cases/sourcemaps/sourcemaps-inject-nomappings.trycmd @@ -12,6 +12,7 @@ Source Map Debug ID Injection Report Modified: The following source files have been modified to have debug ids [..]-[..]-[..]-[..]-[..] - ./server/app/page.min.js [..]-[..]-[..]-[..]-[..] - ./server/chunks/1.min.js + [..]-[..]-[..]-[..]-[..] - ./server/chunks/flight-server-css-manifest.js [..]-[..]-[..]-[..]-[..] - ./server/flight-manifest.js [..]-[..]-[..]-[..]-[..] - ./server/pages/_document.min.js [..]-[..]-[..]-[..]-[..] - ./server/pages/api/hello.min.js diff --git a/tests/integration/_cases/sourcemaps/sourcemaps-inject-not-compiled.trycmd b/tests/integration/_cases/sourcemaps/sourcemaps-inject-not-compiled.trycmd new file mode 100644 index 0000000000..b320add556 --- /dev/null +++ b/tests/integration/_cases/sourcemaps/sourcemaps-inject-not-compiled.trycmd @@ -0,0 +1,5 @@ +``` +$ sentry-cli sourcemaps inject . +? success +... +``` diff --git a/tests/integration/_cases/sourcemaps/sourcemaps-inject.trycmd b/tests/integration/_cases/sourcemaps/sourcemaps-inject.trycmd index ee8bb6d681..70540f9df4 100644 --- a/tests/integration/_cases/sourcemaps/sourcemaps-inject.trycmd +++ b/tests/integration/_cases/sourcemaps/sourcemaps-inject.trycmd @@ -12,6 +12,7 @@ Source Map Debug ID Injection Report Modified: The following source files have been modified to have debug ids [..]-[..]-[..]-[..]-[..] - ./server/app/page.js [..]-[..]-[..]-[..]-[..] - ./server/chunks/1.js + [..]-[..]-[..]-[..]-[..] - ./server/chunks/flight-server-css-manifest.js [..]-[..]-[..]-[..]-[..] - ./server/dummy_embedded.js [..]-[..]-[..]-[..]-[..] - ./server/dummy_hosted.js [..]-[..]-[..]-[..]-[..] - ./server/flight-manifest.js diff --git a/tests/integration/_cases/sourcemaps/sourcemaps-upload-some-debugids.trycmd b/tests/integration/_cases/sourcemaps/sourcemaps-upload-some-debugids.trycmd index a00fea27df..f6ac61c1f1 100644 --- a/tests/integration/_cases/sourcemaps/sourcemaps-upload-some-debugids.trycmd +++ b/tests/integration/_cases/sourcemaps/sourcemaps-upload-some-debugids.trycmd @@ -25,9 +25,10 @@ $ sentry-cli sourcemaps upload tests/integration/_fixtures/upload_some_debugids Source Map Upload Report Scripts - ~/server/chunks/flight-server-css-manifest.js (no sourcemap ref) ~/server/app/page.js (sourcemap at page.js.map) ~/server/chunks/1.js (sourcemap at 1.js.map) + ~/server/chunks/flight-server-css-manifest.js (no sourcemap ref) + - warning: could not determine a source map reference (Could not auto-detect referenced sourcemap for ~/server/chunks/flight-server-css-manifest.js) ~/server/dummy_embedded.js (embedded sourcemap) ~/server/edge-runtime-webpack.js (sourcemap at edge-runtime-webpack.js.map, debug id 2297b93d-928d-421e-8910-127c786382dd) ~/server/flight-manifest.js (no sourcemap ref) diff --git a/tests/integration/_fixtures/inject-not-compiled/not-compiled.js b/tests/integration/_fixtures/inject-not-compiled/not-compiled.js new file mode 100644 index 0000000000..ed4f8b7915 --- /dev/null +++ b/tests/integration/_fixtures/inject-not-compiled/not-compiled.js @@ -0,0 +1,5 @@ +function helloWorld() { + return 'Hello, World!'; +} + +helloWorld(); diff --git a/tests/integration/sourcemaps/inject.rs b/tests/integration/sourcemaps/inject.rs index 1d572f7ee6..1edffca098 100644 --- a/tests/integration/sourcemaps/inject.rs +++ b/tests/integration/sourcemaps/inject.rs @@ -145,3 +145,23 @@ fn command_sourcemaps_inject_bundlers() { assert_eq!(actual_map, expected_map, "CJS, bundler: {bundler}"); } } + +#[test] +fn command_sourcemaps_inject_not_compiled() { + let testcase_cwd_path = + "tests/integration/_cases/sourcemaps/sourcemaps-inject-not-compiled.in/"; + if std::path::Path::new(testcase_cwd_path).exists() { + remove_dir_all(testcase_cwd_path).unwrap(); + } + + copy_recursively( + "tests/integration/_fixtures/inject-not-compiled", + testcase_cwd_path, + ) + .unwrap(); + + register_test("sourcemaps/sourcemaps-inject-not-compiled.trycmd"); + + let file_contents = fs::read_to_string(format!("{testcase_cwd_path}not-compiled.js")).unwrap(); + assert!(file_contents.contains("//# debugId=")); +}