ref(proguard): Replace MappingRef with ProguardMapping

The new `ProguardMapping` type directly references the `ByteView` of the Proguard file, rather than simply storing the path to the file. This change will enable us to replace `ChunkedMapping` with `Chunked<ProguardMapping>`.

ref #2196

Author: szokeasaurusrex

src/commands/upload_proguard.rs

@@ -1,10 +1,6 @@
 use std::env;
-use std::fs;
 use std::io;
-use std::path::Path;
-use std::path::PathBuf;
 
-use ::proguard::ProguardMapping;
 use anyhow::{bail, Error, Result};
 use clap::ArgAction;
 use clap::{Arg, ArgMatches, Command};
@@ -20,24 +16,12 @@ use crate::utils::android::dump_proguard_uuids_as_properties;
 use crate::utils::args::ArgExt;
 use crate::utils::fs::TempFile;
 use crate::utils::proguard;
+use crate::utils::proguard::ProguardMapping;
 use crate::utils::system::QuietExit;
 use crate::utils::ui::{copy_with_progress, make_byte_progress_bar};
 
 const CHUNK_UPLOAD_ENV_VAR: &str = "SENTRY_EXPERIMENTAL_PROGUARD_CHUNK_UPLOAD";
 
-#[derive(Debug)]
-pub struct MappingRef {
-    pub path: PathBuf,
-    pub size: u64,
-    pub uuid: Uuid,
-}
-
-impl AsRef<Path> for MappingRef {
-    fn as_ref(&self) -> &Path {
-        &self.path
-    }
-}
-
 pub fn make_command(command: Command) -> Command {
     command
         .about("Upload ProGuard mapping files to a project.")
@@ -168,21 +152,10 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
     // them all up.
     for path in &paths {
         match ByteView::open(path) {
-            Ok(byteview) => {
-                let mapping = ProguardMapping::new(&byteview);
-                if !mapping.has_line_info() {
-                    eprintln!(
-                        "warning: proguard mapping '{path}' was ignored because it \
-                         does not contain any line information."
-                    );
-                } else {
-                    mappings.push(MappingRef {
-                        path: PathBuf::from(path),
-                        size: byteview.len() as u64,
-                        uuid: forced_uuid.copied().unwrap_or_else(|| mapping.uuid()),
-                    });
-                }
-            }
+            Ok(byteview) => match ProguardMapping::try_from(byteview) {
+                Ok(mapping) => mappings.push(mapping),
+                Err(e) => eprintln!("warning: ignoring proguard mapping '{path}': {e}"),
+            },
             Err(ref err) if err.kind() == io::ErrorKind::NotFound => {
                 eprintln!(
                     "warning: proguard mapping '{path}' does not exist. This \
@@ -198,6 +171,14 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         }
     }
 
+    if let Some(&uuid) = forced_uuid {
+        // There should only be one mapping if we are forcing a UUID.
+        // This is checked earlier.
+        for mapping in &mut mappings {
+            mapping.force_uuid(uuid);
+        }
+    }
+
     let api = Api::current();
     let config = Config::current();
 
@@ -243,19 +224,19 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         {
             let mut zip = zip::ZipWriter::new(tf.open()?);
             for mapping in &mappings {
-                let pb = make_byte_progress_bar(mapping.size);
+                let pb = make_byte_progress_bar(mapping.len() as u64);
                 zip.start_file(
-                    format!("proguard/{}.txt", mapping.uuid),
+                    format!("proguard/{}.txt", mapping.uuid()),
                     zip::write::FileOptions::default(),
                 )?;
-                copy_with_progress(&pb, &mut fs::File::open(&mapping.path)?, &mut zip)?;
+                copy_with_progress(&pb, &mut mapping.as_ref(), &mut zip)?;
                 pb.finish_and_clear();
             }
         }
 
         // write UUIDs into the mapping file.
         if let Some(p) = matches.get_one::<String>("write_properties") {
-            let uuids: Vec<_> = mappings.iter().map(|x| x.uuid).collect();
+            let uuids: Vec<_> = mappings.iter().map(|x| x.uuid()).collect();
             dump_proguard_uuids_as_properties(p, &uuids)?;
         }
 
@@ -305,7 +286,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         }
 
         for mapping in &mappings {
-            let uuid = forced_uuid.unwrap_or(&mapping.uuid);
+            let uuid = forced_uuid.copied().unwrap_or(mapping.uuid());
             authenticated_api.associate_proguard_mappings(
                 &org,
                 &project,

src/utils/proguard/mapping.rs

@@ -0,0 +1,64 @@
+use symbolic::common::ByteView;
+use thiserror::Error;
+use uuid::Uuid;
+
+#[derive(Debug, Error)]
+pub enum ProguardMappingError {
+    #[error("Proguard mapping does not contain line information")]
+    MissingLineInfo,
+}
+
+pub struct ProguardMapping<'a> {
+    bytes: ByteView<'a>,
+    uuid: Uuid,
+}
+
+impl<'a> ProguardMapping<'a> {
+    /// Get the length of the mapping in bytes.
+    pub fn len(&self) -> usize {
+        self.bytes.len()
+    }
+
+    /// Get the UUID of the mapping.
+    pub fn uuid(&self) -> Uuid {
+        self.uuid
+    }
+
+    /// Force the UUID of the mapping to a specific value, rather
+    /// than the UUID which is derived from the proguard crate.
+    pub fn force_uuid(&mut self, uuid: Uuid) {
+        self.uuid = uuid;
+    }
+
+    /// Create a new `ProguardMapping` from a `ByteView`.
+    /// Not public because we want to ensure that the `ByteView` contains line
+    /// information, and this method does not check for that. To create a
+    /// `ProguardMapping` externally, use the `TryFrom<ByteView>` implementation.
+    fn new(bytes: ByteView<'a>, uuid: Uuid) -> Self {
+        Self { bytes, uuid }
+    }
+}
+
+impl<'a> TryFrom<ByteView<'a>> for ProguardMapping<'a> {
+    type Error = ProguardMappingError;
+
+    /// Try to create a `ProguardMapping` from a `ByteView`.
+    /// The method returns an error if the mapping does not contain
+    /// line information.
+    fn try_from(value: ByteView<'a>) -> Result<Self, Self::Error> {
+        let mapping = ::proguard::ProguardMapping::new(&value);
+
+        if !mapping.has_line_info() {
+            return Err(ProguardMappingError::MissingLineInfo);
+        }
+
+        let uuid = mapping.uuid();
+        Ok(ProguardMapping::new(value, uuid))
+    }
+}
+
+impl AsRef<[u8]> for ProguardMapping<'_> {
+    fn as_ref(&self) -> &[u8] {
+        self.bytes.as_ref()
+    }
+}

src/utils/proguard/mod.rs

@@ -1,3 +1,5 @@
+mod mapping;
 mod upload;
 
+pub use self::mapping::ProguardMapping;
 pub use self::upload::chunk_upload;

src/utils/proguard/upload.rs

@@ -4,18 +4,17 @@
 //! Proguard mappings, while we work on a more permanent solution, which will
 //! work for all different types of debug files.
 
+use std::thread;
 use std::time::{Duration, Instant};
-use std::{fs, thread};
 
 use anyhow::Result;
 use indicatif::ProgressStyle;
 use sha1_smol::Digest;
 
 use crate::api::{Api, ChunkUploadOptions, ChunkedDifRequest, ChunkedFileState};
-use crate::commands::upload_proguard::MappingRef;
-use crate::utils::chunks;
-use crate::utils::chunks::Chunk;
+use crate::utils::chunks::{self, Chunk};
 use crate::utils::fs::get_sha1_checksums;
+use crate::utils::proguard::ProguardMapping;
 
 /// How often to poll the server for the status of the assembled mappings.
 const ASSEMBLE_POLL_INTERVAL: Duration = Duration::from_secs(1);
@@ -34,9 +33,9 @@ struct ChunkedMapping {
 }
 
 impl ChunkedMapping {
-    fn try_from_mapping(mapping: &MappingRef, chunk_size: u64) -> Result<Self> {
-        let raw_data = fs::read(mapping)?;
-        let file_name = format!("/proguard/{}.txt", mapping.uuid);
+    fn try_from_mapping(mapping: &ProguardMapping, chunk_size: u64) -> Result<Self> {
+        let raw_data = mapping.as_ref().to_vec();
+        let file_name = format!("/proguard/{}.txt", mapping.uuid());
 
         let (hash, chunk_hashes) = get_sha1_checksums(&raw_data, chunk_size as usize)?;
         Ok(Self {
@@ -66,14 +65,14 @@ impl<'a> From<&'a ChunkedMapping> for ChunkedDifRequest<'a> {
 /// Blocks until the mappings have been assembled (up to ASSEMBLE_POLL_TIMEOUT).
 /// Returns an error if the mappings fail to assemble, or if the timeout is reached.
 pub fn chunk_upload(
-    paths: &[MappingRef],
+    mappings: &[ProguardMapping<'_>],
     chunk_upload_options: &ChunkUploadOptions,
     org: &str,
     project: &str,
 ) -> Result<()> {
-    let chunked_mappings: Vec<ChunkedMapping> = paths
+    let chunked_mappings: Vec<ChunkedMapping> = mappings
         .iter()
-        .map(|path| ChunkedMapping::try_from_mapping(path, chunk_upload_options.chunk_size))
+        .map(|mapping| ChunkedMapping::try_from_mapping(mapping, chunk_upload_options.chunk_size))
         .collect::<Result<_>>()?;
 
     let progress_style = ProgressStyle::default_bar().template(

tests/integration/_cases/upload_proguard/upload_proguard-no-upload.trycmd

@@ -1,7 +1,7 @@
 ```
 $ sentry-cli upload-proguard tests/integration/_fixtures/proguard.txt --no-upload
 ? success
-warning: proguard mapping 'tests/integration/_fixtures/proguard.txt' was ignored because it does not contain any line information.
+warning: ignoring proguard mapping 'tests/integration/_fixtures/proguard.txt': Proguard mapping does not contain line information
 > compressing mappings
 > skipping upload.
 

tests/integration/_cases/upload_proguard/upload_proguard.trycmd

@@ -1,7 +1,7 @@
 ```
 $ sentry-cli upload-proguard tests/integration/_fixtures/proguard.txt
 ? success
-warning: proguard mapping 'tests/integration/_fixtures/proguard.txt' was ignored because it does not contain any line information.
+warning: ignoring proguard mapping 'tests/integration/_fixtures/proguard.txt': Proguard mapping does not contain line information
 > compressing mappings
 > uploading mappings
 > Uploaded a total of 0 new mapping files