fix(sourcemaps): Fix double association bug (#2764)

Fixes a bug where the same sourcemap can be associated with mulitple
source files.

### Change description

We solve this problem by keeping track of the sourcemaps we have already
associated with a source file. If we guess a sourcemap which has already
been associated, we do not associate the given source file with any
sourcemap.

Also, we iterate first through all the sources which explicitly
reference the sourcemap – i.e., for which, we don't have to guess the
sourcemap. These are associated first. As they are explicitly linked to
a sourcemap, we intentionally **do not prevent duplicates** here.

On the second iteration, we guess the sourcemap reference for all
remaining sources. **We enforce the no duplicates rule on the second
iteration**. Sources cannot be associated with a sourcemap that was
explicitly referenced by another source on the first iteration. If
multiple sources guess the same sourcemap, and that sourcemap is not
explicitly associated with any source, then none of the sources will be
associated with that sourcemap (as it is unclear how that sourcemap
should be associated).

In all cases, we warn the users that we failed to associate the sources
properly.

### Issues

Fixes #2445
Fixes
[CLI-5](https://linear.app/getsentry/issue/CLI-5/ensure-collect-sourcemap-references-does-not-associate-the-same)

Author: szokeasaurusrex

src/utils/file_upload.rs

@@ -211,7 +211,7 @@ impl<'a> TryFrom<&'a UploadContext<'_>> for LegacyUploadContext<'a> {
     }
 }
 
-#[derive(Eq, PartialEq, Debug, Copy, Clone)]
+#[derive(Eq, PartialEq, Debug, Copy, Clone, Hash)]
 pub enum LogLevel {
     Warning,
     Error,
@@ -226,7 +226,7 @@ impl fmt::Display for LogLevel {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct SourceFile {
     pub url: String,
     pub path: PathBuf,

src/utils/sourcemaps.rs

@@ -25,6 +25,7 @@ use crate::utils::file_search::ReleaseFileMatch;
 use crate::utils::file_upload::{
     initialize_legacy_release_upload, FileUpload, SourceFile, SourceFiles, UploadContext,
 };
+use crate::utils::fs;
 use crate::utils::logging::is_quiet_mode;
 use crate::utils::progress::ProgressBar;
 use crate::utils::sourcemaps::inject::{InjectReportBuilder, ReportItem};
@@ -172,6 +173,7 @@ fn guess_sourcemap_reference(
 /// and original url with which the file was added to the processor.
 /// This enable us to look up the source map file based on the original url.
 /// Which can be used for example for debug id referencing.
+#[derive(Eq, Hash, PartialEq, Debug)]
 pub struct SourceMapReference {
     url: String,
     original_url: Option<String>,
@@ -289,51 +291,44 @@ impl SourceMapProcessor {
     fn collect_sourcemap_references(&mut self) {
         let sourcemaps = self
             .sources
-            .iter()
-            .map(|x| x.1)
+            .values()
             .filter(|x| x.ty == SourceFileType::SourceMap)
             .map(|x| x.url.clone())
-            .collect();
-
-        for source in self.sources.values_mut() {
-            // Skip everything but minified JS files.
-            if source.ty != SourceFileType::MinifiedSource {
-                continue;
-            }
-
-            if self.sourcemap_references.contains_key(&source.url) {
-                continue;
-            }
-
-            let Ok(contents) = std::str::from_utf8(&source.contents) else {
-                continue;
-            };
+            .collect::<HashSet<_>>();
 
-            // If this is a full external URL, the code below is going to attempt
-            // to "normalize" it with the source path, resulting in a bogus path
-            // like "path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map"
-            // that can't be resolved to a source map file.
-            // Instead, we pretend we failed to discover the location, and we fall back to
-            // guessing the source map location based on the source location.
-            let location =
-                discover_sourcemaps_location(contents).filter(|loc| !is_remote_sourcemap(loc));
-            let sourcemap_reference = match location {
-                Some(url) => SourceMapReference::from_url(url.to_owned()),
-                None => match guess_sourcemap_reference(&sourcemaps, &source.url) {
-                    Ok(target) => target,
-                    Err(err) => {
-                        source.warn(format!(
-                            "could not determine a source map reference ({err})"
-                        ));
-                        self.sourcemap_references.insert(source.url.clone(), None);
-                        continue;
-                    }
-                },
-            };
+        let (sources_with_location, sources_without_location) = self
+            .sources
+            .values_mut()
+            .filter(|source| {
+                source.ty == SourceFileType::MinifiedSource
+                    && !self.sourcemap_references.contains_key(&source.url)
+            })
+            .collect_sourcemap_locations();
+
+        // First pass: if location discovered, add to sourcemap_references. Also, keep track of
+        // the sourcemaps we associate, and the source file we associate them with.
+        let explicitly_associated_sourcemaps = sources_with_location
+            .iter()
+            .map(|(source, location)| {
+                let sourcemap_reference = SourceMapReference::from_url(location.clone());
+                let sourcemap_path = full_sourcemap_path(source, location);
+
+                self.sourcemap_references
+                    .insert(source.url.clone(), Some(sourcemap_reference));
+
+                (fs::path_as_url(&sourcemap_path), source.url.clone())
+            })
+            .collect::<HashMap<_, _>>();
+
+        // Second pass: for remaining sourcemaps, try to guess the location
+        let guessed_sourcemap_references = guess_sourcemap_references(
+            sources_without_location,
+            sourcemaps,
+            explicitly_associated_sourcemaps,
+        );
 
-            self.sourcemap_references
-                .insert(source.url.clone(), Some(sourcemap_reference));
-        }
+        self.sourcemap_references
+            .extend(guessed_sourcemap_references);
     }
 
     pub fn dump_log(&self, title: &str) {
@@ -1086,6 +1081,164 @@ impl SourceMapProcessor {
     }
 }
 
+/// For a set of source files without a sourcemap location, guess the sourcemap references.
+///
+/// Parameters:
+///   - `sources_without_location`: The set of source files without a sourcemap location.
+///   - `sourcemaps`: The set of available sourcemaps.
+///   - `explicitly_associated_sourcemaps`: The set of sourcemaps that are explicitly associated
+///     with a source file, and thus, cannot be guessed. If we guess such a sourcemap, we will
+///     warn the user. This is stored in a map, where the key is the sourcemap URL, and the value
+///     is the source file URL that is explicitly associated with the sourcemap.
+///
+/// Returns:
+///   - A map from sourcemap URLs to the sourcemap references, which may be `None` if we couldn't
+///     guess the sourcemap reference.
+fn guess_sourcemap_references(
+    sources_without_location: HashSet<&mut SourceFile>,
+    sourcemaps: HashSet<String>,
+    explicitly_associated_sourcemaps: HashMap<String, String>,
+) -> HashMap<String, Option<SourceMapReference>> {
+    let mut sourcemap_references = HashMap::new();
+
+    sources_without_location
+        .into_iter()
+        .fold(
+            // Collect sources guessed as associated with each sourcemap. This way, we ensure
+            // we only associate the sourcemap with any sources if it is only guessed once.
+            HashMap::new(),
+            |mut sources_associated_with_sm, source| {
+                let sourcemap_reference = guess_sourcemap_reference(&sourcemaps, &source.url)
+                    .inspect_err(|err| {
+                        source.warn(format!(
+                            "could not determine a source map reference ({err})"
+                        ));
+                    })
+                    .ok()
+                    .filter(|sourcemap_reference| {
+                        explicitly_associated_sourcemaps
+                            .get(
+                                sourcemap_reference
+                                    .original_url
+                                    .as_ref()
+                                    .expect("original url set in guess_sourcemap_reference"),
+                            )
+                            .inspect(|url| {
+                                source.warn(format!(
+                                    "based on the file name, we guessed a source map \
+                                    reference ({}), which is already associated with source \
+                                    {url}. Please explicitly set the sourcemap URL with a \
+                                    `//# sourceMappingURL=...` comment in the source file.",
+                                    sourcemap_reference.url
+                                ));
+                            })
+                            .is_none()
+                    });
+
+                if let Some(sourcemap_reference) = sourcemap_reference {
+                    sources_associated_with_sm
+                        .entry(sourcemap_reference)
+                        .or_insert_with(Vec::new)
+                        .push(source);
+                } else {
+                    sourcemap_references.insert(source.url.clone(), None);
+                }
+
+                sources_associated_with_sm
+            },
+        )
+        .into_iter()
+        .for_each(|(sourcemap_reference, mut sources)| {
+            if let [source] = sources.as_slice() {
+                // One source -> we can safely associate the sourcemap with it.
+                sourcemap_references.insert(source.url.clone(), Some(sourcemap_reference));
+            } else {
+                // Multiple sources -> it is unclear which source we should associate
+                // the sourcemap with, so don't associate it with any of them.
+                sources.iter_mut().for_each(|source| {
+                    source.warn(format!(
+                        "Could not associate this source with a source map. We \
+                        guessed the sourcemap reference {} for multiple sources, including \
+                        this one. Please explicitly set the sourcemap URL with a \
+                        `//# sourceMappingURL=...` comment in the source file, to make the \
+                        association clear.",
+                        sourcemap_reference.url
+                    ));
+                    sourcemap_references.insert(source.url.clone(), None);
+                });
+            }
+        });
+
+    sourcemap_references
+}
+
+/// Compute the full path to a sourcemap file, given the sourcemap's source file and the relative
+/// path to the sourcemap from the source file.
+fn full_sourcemap_path(source: &SourceFile, sourcemap_relative_path: &String) -> PathBuf {
+    let full_sourcemap_path = source
+        .path
+        .parent()
+        .expect("source path has a parent")
+        .join(sourcemap_relative_path);
+
+    full_sourcemap_path
+}
+
+/// A tuple of a map and a set, returned by `collect_sourcemap_locations`.
+/// The map contains the sourcefiles for which we found a sourcemap location listed in the file, with
+/// the sourcemap location as the value.
+/// The set contains the sourcefiles for which we did not find a sourcemap location listed in the file.
+type SourcemapLocations<'s> = (
+    HashMap<&'s mut SourceFile, String>,
+    HashSet<&'s mut SourceFile>,
+);
+
+trait SourcesIteratorExt<'s> {
+    fn collect_sourcemap_locations(self) -> SourcemapLocations<'s>;
+}
+
+impl<'s, I> SourcesIteratorExt<'s> for I
+where
+    I: IntoIterator<Item = &'s mut SourceFile>,
+{
+    /// Consume this iterator of sources, collecting the sourcemap locations for them.
+    fn collect_sourcemap_locations(self) -> SourcemapLocations<'s> {
+        self.into_iter()
+            .map(|source| {
+                let location = location_from_contents(&source.contents).map(String::from);
+                (source, location)
+            })
+            .fold(
+                (HashMap::new(), HashSet::new()),
+                |(mut sources_with_location, mut sources_without_location), (source, location)| {
+                    match location {
+                        Some(location) => {
+                            sources_with_location.insert(source, location);
+                        }
+                        None => {
+                            sources_without_location.insert(source);
+                        }
+                    }
+                    (sources_with_location, sources_without_location)
+                },
+            )
+    }
+}
+
+/// Extract the sourcemap location from the contents of a source file.
+fn location_from_contents(contents: &[u8]) -> Option<&str> {
+    str::from_utf8(contents)
+        .map(discover_sourcemaps_location)
+        .ok()
+        .flatten()
+        // If this is a full external URL, the code above is going to attempt
+        // to "normalize" it with the source path, resulting in a bogus path
+        // like "path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map"
+        // that can't be resolved to a source map file.
+        // So, we filter out such locations.
+        .filter(|loc| !is_remote_sourcemap(loc))
+}
+
 fn adjust_regular_sourcemap(
     sourcemap: &mut SourceMap,
     source_file: &mut SourceFile,

tests/integration/_cases/sourcemaps/sourcemaps-inject-double-association.trycmd

@@ -0,0 +1,28 @@
+```
+$ sentry-cli sourcemaps inject ./
+? success
+> Searching ./
+> Found 11 files
+> Analyzing 11 sources
+> Injecting debug ids
+
+Source Map Debug ID Injection Report
+  Modified: The following source files have been modified to have debug ids
+    0fb38a98-fdda-5fd2-8e58-d8370f1596fb - ./app.min.js
+        - warning: based on the file name, we guessed a source map reference (app.min.js.map), which is already associated with source ./references_app.min.js. Please explicitly set the sourcemap URL with a `//# sourceMappingURL=...` comment in the source file.
+    2de47eef-a93e-5d79-8c45-afb649b15f9e - ./mapInSubdirectory.min.js
+    f369af08-3e09-58d4-92a9-7f432f7d51cb - ./otherApp.js
+        - warning: Could not associate this source with a source map. We guessed the sourcemap reference otherApp.map for multiple sources, including this one. Please explicitly set the sourcemap URL with a `//# sourceMappingURL=...` comment in the source file, to make the association clear.
+    4ee42454-c84d-5d2a-b0b9-8cda74c57838 - ./otherApp.min.js
+        - warning: Could not associate this source with a source map. We guessed the sourcemap reference otherApp.map for multiple sources, including this one. Please explicitly set the sourcemap URL with a `//# sourceMappingURL=...` comment in the source file, to make the association clear.
+    44931b59-c632-5017-a624-3bb0ac2a0317 - ./references_app.min.js
+    edefb3dc-9f7a-5540-9db0-270bbd81b8fd - ./subdirectory/app.min.js
+    228cdb31-bb0a-533c-ac4c-c2ab111ab148 - ./subdirectory/mapForFileInParentDirectory.min.js
+        - warning: based on the file name, we guessed a source map reference (mapForFileInParentDirectory.min.js.map), which is already associated with source ./mapInSubdirectory.min.js. Please explicitly set the sourcemap URL with a `//# sourceMappingURL=...` comment in the source file.
+  Modified: The following sourcemap files have been modified to have debug ids
+    44931b59-c632-5017-a624-3bb0ac2a0317 - ./app.min.js.map
+    edefb3dc-9f7a-5540-9db0-270bbd81b8fd - ./subdirectory/app.min.js.map
+    2de47eef-a93e-5d79-8c45-afb649b15f9e - ./subdirectory/mapForFileInParentDirectory.min.js.map
+
+
+```

tests/integration/_expected_outputs/sourcemaps/sourcemaps-inject-double-association/app.min.js

@@ -0,0 +1,6 @@
+
+!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof self?self:{},n=(new e.Error).stack;n&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[n]="2de47eef-a93e-5d79-8c45-afb649b15f9e")}catch(e){}}();
+!function(e,t){"use strict";function n(e){return document.getElementById(e)}function r(e,t){e.addEventListener("click",t)}var o=n("btn");r(o,function(){console.log("mapInSubdirectory.min.js!")})}(window,document);
+
+//# sourceMappingURL=subdirectory/mapForFileInParentDirectory.min.js.map
+//# debugId=2de47eef-a93e-5d79-8c45-afb649b15f9e

tests/integration/_expected_outputs/sourcemaps/sourcemaps-inject-double-association/app.min.js.map

@@ -0,0 +1,6 @@
+
+!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof self?self:{},n=(new e.Error).stack;n&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[n]="44931b59-c632-5017-a624-3bb0ac2a0317")}catch(e){}}();
+!function(e,t){"use strict";function n(e){return document.querySelector(e)}function r(e,t){e.textContent=t}var o=n("#message");r(o,"Hello World!")}(window,document);
+//# sourceMappingURL=app.min.js.map
+
+//# debugId=44931b59-c632-5017-a624-3bb0ac2a0317