ref: Use better artifacts url resolution for sourcemaps explain (#1329)

src/commands/sourcemaps/explain.rs

@@ -132,38 +132,14 @@ fn fetch_release_artifacts(org: &str, project: &str, release: &str) -> Result<Ve
 
 // Try to find an artifact which matches the path part of the url extracted from the stacktrace frame,
 // prefixed with the default `~/`, which is a "glob-like" pattern for matchin any hostname.
-//
-// We only need the `pathname` portion of the url, so if it's absolute, just extract it.
-// If it's relative however, parse any random url (example.com) and join it with our relative url,
-// as Rust cannot handle parsing of relative urls.
-//
-// http://localhost:5000/dist/bundle.min.js => ~/dist/bundle.min.js
-// /dist/bundle.js.map => ~/dist/bundle.js.map
-// okboomer => error (invalid relative path, no extension)
-//
-// It should be more generic than using the defaults, but should be sufficient for our current usecase.
-fn find_matching_artifact(artifacts: &[Artifact], abs_path: &str) -> Result<Artifact> {
-    let abs_path = match Url::parse(abs_path) {
-        Ok(path) => Ok(path),
-        Err(_) => {
-            let base = Url::parse("http://example.com").unwrap();
-            base.join(abs_path)
-                .map_err(|_| format_err!("Cannot parse source map url {}", abs_path))
-        }
-    }?;
-    let mut filename = String::from("~");
-    filename.push_str(abs_path.path());
-
-    let full_match = artifacts.iter().find(|a| a.name == filename);
+fn find_matching_artifact(artifacts: &[Artifact], path: &str) -> Result<Artifact> {
+    let full_match = artifacts.iter().find(|a| a.name == path);
     let partial_match = artifacts
         .iter()
-        .find(|a| a.name.ends_with(filename.split('/').last().unwrap()));
+        .find(|a| a.name.ends_with(path.split('/').last().unwrap()));
 
     if full_match.is_none() {
-        error(format!(
-            "Uploaded artifacts do not include entry: {}",
-            filename
-        ));
+        error(format!("Uploaded artifacts do not include entry: {}", path));
 
         if let Some(pm) = partial_match {
             tip(format!(
@@ -176,7 +152,7 @@ fn find_matching_artifact(artifacts: &[Artifact], abs_path: &str) -> Result<Arti
         return Err(QuietExit(1).into());
     }
 
-    success(format!("Artifact {} found.", filename));
+    success(format!("Artifact {} found.", path));
     Ok(full_match.cloned().unwrap())
 }
 
@@ -355,6 +331,34 @@ fn extract_release(event: &ProcessedEvent) -> Result<String> {
     }
 }
 
+fn resolve_sourcemap_url(abs_path: &str, sourcemap_location: &str) -> Result<String> {
+    let base = Url::parse(abs_path)?;
+    base.join(sourcemap_location)
+        .map(|url| url.to_string())
+        .map_err(|e| e.into())
+}
+
+// Unify url to be prefixed with the default `~/`, which is a "glob-like" pattern for matchin any hostname.
+//
+// We only need the `pathname` portion of the url, so if it's absolute, just extract it.
+// If it's relative however, parse any random url (example.com) and join it with our relative url,
+// as Rust cannot handle parsing of relative urls.
+//
+// It should be more generic than using the defaults, but should be sufficient for our current usecase.
+fn unify_artifact_url(abs_path: &str) -> Result<String> {
+    let abs_path = match Url::parse(abs_path) {
+        Ok(path) => Ok(path),
+        Err(_) => {
+            let base = Url::parse("http://example.com").unwrap();
+            base.join(abs_path)
+                .map_err(|_| format_err!("Cannot parse source map url {}", abs_path))
+        }
+    }?;
+    let mut filename = String::from("~");
+    filename.push_str(abs_path.path());
+    Ok(filename)
+}
+
 pub fn execute(matches: &ArgMatches) -> Result<()> {
     let config = Config::current();
     let (org, project) = config.get_org_and_project(matches)?;
@@ -408,8 +412,9 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         }
     }
 
+    let abs_path = frame.abs_path.as_ref().expect("Incorrect abs_path value");
     let artifacts = fetch_release_artifacts(&org, &project, &release)?;
-    let matched_artifact = find_matching_artifact(&artifacts, frame.abs_path.as_ref().unwrap())?;
+    let matched_artifact = find_matching_artifact(&artifacts, &unify_artifact_url(abs_path)?)?;
 
     verify_dists_matches(&matched_artifact, event.dist.as_deref())?;
 
@@ -420,18 +425,24 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
                 QuietExit(1)
             },
         )?;
-    success(format!("Found source map location: {}", sourcemap_location));
+    success(format!(
+        "Found source map location: {}",
+        &sourcemap_location
+    ));
+
+    let sourcemap_url = unify_artifact_url(&resolve_sourcemap_url(abs_path, &sourcemap_location)?)?;
+    success(format!("Resolved source map url: {}", &sourcemap_url));
 
-    let sourcemap_artifact = find_matching_artifact(&artifacts, &sourcemap_location)?;
+    let sourcemap_artifact = find_matching_artifact(&artifacts, &sourcemap_url)?;
     verify_dists_matches(&sourcemap_artifact, event.dist.as_deref())?;
 
     let sourcemap_file =
         fetch_release_artifact_file(&org, &project, &release, &sourcemap_artifact)?;
 
     print_sourcemap(
         &sourcemap_file,
-        frame.lineno.unwrap() as u32 - 1,
-        frame.colno.unwrap() as u32 - 1,
+        frame.lineno.expect("Event frame is missing line number") as u32 - 1,
+        frame.colno.expect("Event frame is missing column number") as u32 - 1,
     )
     .map_err(|err| {
         error(err);
@@ -441,3 +452,53 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
     success("Source Maps should be working fine. Have you tried turning it off and on again?");
     Ok(())
 }
+
+#[test]
+fn test_resolve_sourcemap_url() {
+    // Tests coming from `tests/sentry/utils/test_urls.py` in `getsentry/sentry`
+    let cases = vec![
+        ("http://example.com/foo", "bar", "http://example.com/bar"),
+        ("http://example.com/foo", "/bar", "http://example.com/bar"),
+        ("https://example.com/foo", "/bar", "https://example.com/bar"),
+        (
+            "http://example.com/foo/baz",
+            "bar",
+            "http://example.com/foo/bar",
+        ),
+        (
+            "http://example.com/foo/baz",
+            "/bar",
+            "http://example.com/bar",
+        ),
+        ("aps://example.com/foo", "/bar", "aps://example.com/bar"),
+        (
+            "apsunknown://example.com/foo",
+            "/bar",
+            "apsunknown://example.com/bar",
+        ),
+        (
+            "apsunknown://example.com/foo",
+            "//aha/uhu",
+            "apsunknown://aha/uhu",
+        ),
+    ];
+
+    for (base, to_join, expected) in cases {
+        assert_eq!(resolve_sourcemap_url(base, to_join).unwrap(), expected);
+    }
+}
+
+#[test]
+fn test_unify_artifact_url() {
+    let cases = vec![
+        (
+            "http://localhost:5000/dist/bundle.min.js",
+            "~/dist/bundle.min.js",
+        ),
+        ("/dist/bundle.js.map", "~/dist/bundle.js.map"),
+    ];
+
+    for (path, expected) in cases {
+        assert_eq!(unify_artifact_url(path).unwrap(), expected);
+    }
+}

tests/integration/_cases/sourcemaps/sourcemaps-explain-detect-from-file-content.trycmd

@@ -9,7 +9,8 @@ $ sentry-cli sourcemaps explain 43a57a55cd5a4207ac520c03e1dee1b4
 ✔ Release artifact distribution matched. Event: [none], Artifact: [none]
 ✔ Successfully fetched ~/dist/bundle.min.js file metadata from the server.
 ✔ Successfully fetched ~/dist/bundle.min.js file from the server.
-✔ Found source map location: dist/bundle.min.js.map
+✔ Found source map location: bundle.min.js.map
+✔ Resolved source map url: ~/dist/bundle.min.js.map
 ✖ Uploaded artifacts do not include entry: ~/dist/bundle.min.js.map
 
 ```

tests/integration/_cases/sourcemaps/sourcemaps-explain-detect-from-sourcemap-header.trycmd

@@ -8,7 +8,8 @@ $ sentry-cli sourcemaps explain 43a57a55cd5a4207ac520c03e1dee1b4
 ✔ Artifact ~/dist/bundle.min.js found.
 ✔ Release artifact distribution matched. Event: [none], Artifact: [none]
 ✔ Successfully fetched ~/dist/bundle.min.js file metadata from the server.
-✔ Found source map location: dist/bundle.min.js.map
+✔ Found source map location: bundle.min.js.map
+✔ Resolved source map url: ~/dist/bundle.min.js.map
 ✖ Uploaded artifacts do not include entry: ~/dist/bundle.min.js.map
 
 ```

tests/integration/_cases/sourcemaps/sourcemaps-explain-detect-from-xsourcemap-header.trycmd

@@ -9,6 +9,7 @@ $ sentry-cli sourcemaps explain 43a57a55cd5a4207ac520c03e1dee1b4
 ✔ Release artifact distribution matched. Event: [none], Artifact: [none]
 ✔ Successfully fetched ~/dist/bundle.min.js file metadata from the server.
 ✔ Found source map location: bundle.min.js.map
-✖ Uploaded artifacts do not include entry: ~/bundle.min.js.map
+✔ Resolved source map url: ~/dist/bundle.min.js.map
+✖ Uploaded artifacts do not include entry: ~/dist/bundle.min.js.map
 
 ```

tests/integration/_cases/sourcemaps/sourcemaps-explain-print-sourcemap.trycmd

@@ -8,7 +8,8 @@ $ sentry-cli sourcemaps explain 43a57a55cd5a4207ac520c03e1dee1b4
 ✔ Artifact ~/dist/bundle.min.js found.
 ✔ Release artifact distribution matched. Event: [none], Artifact: [none]
 ✔ Successfully fetched ~/dist/bundle.min.js file metadata from the server.
-✔ Found source map location: dist/bundle.min.js.map
+✔ Found source map location: bundle.min.js.map
+✔ Resolved source map url: ~/dist/bundle.min.js.map
 ✔ Artifact ~/dist/bundle.min.js.map found.
 ✔ Release artifact distribution matched. Event: [none], Artifact: [none]
 ✔ Successfully fetched ~/dist/bundle.min.js.map file from the server.

tests/integration/_responses/sourcemaps/get-file-metadata-sourcemap-header.json

@@ -3,7 +3,7 @@
   "name": "~/dist/bundle.min.js",
   "dist": null,
   "headers": {
-    "Sourcemap": "dist/bundle.min.js.map"
+    "Sourcemap": "bundle.min.js.map"
   },
   "size": 497,
   "sha1": "2fb719956748ab7ec5ae9bcb47606733f5589b72",