ref(dif): Put hash in ChunkedDifRequest

Putting the hash in the `ChunkedDifRequest` allows us to avoid needing to transform objects into tuples with a `ChunkedDifRequest` and a hash. With this change, we can greatly simplify the API for creating a `ChunkedDifRequest` and an `AssembleDifRequest`

Author: szokeasaurusrex

src/api/data_types/chunking/dif.rs

@@ -14,6 +14,28 @@ pub struct ChunkedDifRequest<'a> {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub debug_id: Option<DebugId>,
     pub chunks: &'a [Digest],
+    #[serde(skip)]
+    hash: Digest,
+}
+
+impl<'a> ChunkedDifRequest<'a> {
+    /// Create a new ChunkedDifRequest with the given name, chunk hashes,
+    /// and total hash for the entire file.
+    pub fn new(name: &'a str, chunks: &'a [Digest], hash: Digest) -> Self {
+        Self {
+            name,
+            chunks,
+            hash,
+            debug_id: None,
+        }
+    }
+
+    /// Set the provided debug_id on the request, or clear it if
+    /// `None` is passed.
+    pub fn with_debug_id(mut self, debug_id: Option<DebugId>) -> Self {
+        self.debug_id = debug_id;
+        self
+    }
 }
 
 #[derive(Debug, Deserialize)]
@@ -25,5 +47,40 @@ pub struct ChunkedDifResponse {
     pub dif: Option<DebugInfoFile>,
 }
 
-pub type AssembleDifsRequest<'a> = HashMap<Digest, ChunkedDifRequest<'a>>;
+#[derive(Debug, Serialize)]
+#[serde(transparent)]
+pub struct AssembleDifsRequest<'a>(HashMap<Digest, ChunkedDifRequest<'a>>);
+
+impl AssembleDifsRequest<'_> {
+    /// Strips the debug_id from all requests in the request. We need
+    /// to strip the debug_ids whenever the server does not support chunked
+    /// uploading of PDBs, to maintain backwards compatibility. The
+    /// calling code is responsible for calling this function when needed.
+    ///
+    /// See: https://github.com/getsentry/sentry-cli/issues/980
+    /// See: https://github.com/getsentry/sentry-cli/issues/1056
+    pub fn strip_debug_ids(&mut self) {
+        for r in self.0.values_mut() {
+            r.debug_id = None;
+        }
+    }
+}
+
+impl<'a, T> FromIterator<T> for AssembleDifsRequest<'a>
+where
+    T: Into<ChunkedDifRequest<'a>>,
+{
+    fn from_iter<I>(iter: I) -> Self
+    where
+        I: IntoIterator<Item = T>,
+    {
+        Self(
+            iter.into_iter()
+                .map(|obj| obj.into())
+                .map(|r| (r.hash, r))
+                .collect(),
+        )
+    }
+}
+
 pub type AssembleDifsResponse = HashMap<Digest, ChunkedDifResponse>;

src/utils/chunks/types.rs

@@ -4,7 +4,9 @@ use std::fmt::{Display, Formatter, Result as FmtResult};
 
 use anyhow::Result;
 use sha1_smol::Digest;
+use symbolic::common::DebugId;
 
+use crate::api::ChunkedDifRequest;
 use crate::utils::chunks::Chunk;
 use crate::utils::fs;
 
@@ -16,6 +18,10 @@ pub type MissingObjectsInfo<'m, T> = (Vec<&'m Chunked<T>>, Vec<Chunk<'m>>);
 pub trait Assemblable {
     /// Returns the name of the object.
     fn name(&self) -> &str;
+
+    /// Returns the debug ID of the object.
+    /// Types which do not have a debug ID should return `None`.
+    fn debug_id(&self) -> Option<DebugId>;
 }
 
 /// Chunked arbitrary data with computed SHA1 checksums.
@@ -94,4 +100,18 @@ where
     fn name(&self) -> &str {
         self.object().name()
     }
+
+    fn debug_id(&self) -> Option<DebugId> {
+        self.object().debug_id()
+    }
+}
+
+impl<'c, T> From<&'c Chunked<T>> for ChunkedDifRequest<'c>
+where
+    T: Assemblable,
+{
+    fn from(value: &'c Chunked<T>) -> Self {
+        Self::new(value.name(), value.chunk_hashes(), value.checksum())
+            .with_debug_id(value.debug_id())
+    }
 }

src/utils/dif_upload.rs

@@ -33,7 +33,7 @@ use zip::result::ZipError;
 use zip::{write::FileOptions, ZipArchive, ZipWriter};
 
 use crate::api::{
-    Api, ChunkUploadCapability, ChunkUploadOptions, ChunkedDifRequest, ChunkedFileState,
+    Api, AssembleDifsRequest, ChunkUploadCapability, ChunkUploadOptions, ChunkedFileState,
 };
 use crate::config::Config;
 use crate::constants::{DEFAULT_MAX_DIF_SIZE, DEFAULT_MAX_WAIT};
@@ -309,38 +309,9 @@ impl Assemblable for DifMatch<'_> {
     fn name(&self) -> &str {
         self.file_name()
     }
-}
-
-/// A tuple which can be collected into a mapping of checksums to
-/// `ChunkedDifRequest`s. The collected mapping can be sent in a
-/// request to the assemble endpoint.
-type AssembleRequest<'a> = (Digest, ChunkedDifRequest<'a>);
-
-trait IntoAssembleRequest {
-    /// Creates an `AssembleRequest` tuple for this object.
-    fn assemble_request(&self, with_debug_id: bool) -> AssembleRequest<'_>;
-}
 
-impl IntoAssembleRequest for Chunked<DifMatch<'_>> {
-    // Some(...) for debug_id can only be done if the ChunkedUploadCapability::Pdbs is
-    // present, which is kind of a protocol bug.  Not supplying it means more recent
-    // sentry-cli versions keep working with ancient versions of sentry by not
-    // triggering this protocol bug in most common situations.
-    // See: https://github.com/getsentry/sentry-cli/issues/980
-    // See: https://github.com/getsentry/sentry-cli/issues/1056
-    fn assemble_request(&self, with_debug_id: bool) -> AssembleRequest<'_> {
-        (
-            self.checksum(),
-            ChunkedDifRequest {
-                name: self.object().file_name(),
-                debug_id: if with_debug_id {
-                    self.object().debug_id
-                } else {
-                    None
-                },
-                chunks: self.chunk_hashes(),
-            },
-        )
+    fn debug_id(&self) -> Option<DebugId> {
+        self.debug_id
     }
 }
 
@@ -1260,14 +1231,15 @@ fn try_assemble<'m, T>(
     options: &DifUpload,
 ) -> Result<MissingObjectsInfo<'m, T>>
 where
-    T: AsRef<[u8]>,
-    Chunked<T>: IntoAssembleRequest,
+    T: AsRef<[u8]> + Assemblable,
 {
     let api = Api::current();
-    let request = objects
-        .iter()
-        .map(|d| d.assemble_request(options.pdbs_allowed))
-        .collect();
+    let mut request: AssembleDifsRequest<'_> = objects.iter().collect();
+
+    if !options.pdbs_allowed {
+        request.strip_debug_ids();
+    }
+
     let response = api
         .authenticated()?
         .assemble_difs(&options.org, &options.project, &request)?;
@@ -1405,7 +1377,6 @@ fn poll_assemble<T>(
 ) -> Result<(Vec<DebugInfoFile>, bool)>
 where
     T: Display + Assemblable,
-    Chunked<T>: IntoAssembleRequest,
 {
     let progress_style = ProgressStyle::default_bar().template(
         "{prefix:.dim} Processing files...\
@@ -1419,10 +1390,11 @@ where
 
     let assemble_start = Instant::now();
 
-    let request = chunked_objects
-        .iter()
-        .map(|d| d.assemble_request(options.pdbs_allowed))
-        .collect();
+    let mut request: AssembleDifsRequest<'_> = chunked_objects.iter().copied().collect();
+    if !options.pdbs_allowed {
+        request.strip_debug_ids();
+    }
+
     let response = loop {
         let response =
             api.authenticated()?

src/utils/proguard_upload.rs

@@ -58,18 +58,10 @@ impl ChunkedMapping {
 
 impl<'a> From<&'a ChunkedMapping> for ChunkedDifRequest<'a> {
     fn from(value: &'a ChunkedMapping) -> Self {
-        ChunkedDifRequest {
-            name: &value.file_name,
-            debug_id: None,
-            chunks: &value.chunk_hashes,
-        }
+        ChunkedDifRequest::new(&value.file_name, &value.chunk_hashes, value.hash)
     }
 }
 
-fn to_assemble(chunked: &ChunkedMapping) -> (Digest, ChunkedDifRequest<'_>) {
-    (chunked.hash, chunked.into())
-}
-
 /// Uploads a set of Proguard mappings to Sentry.
 /// 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.
@@ -99,7 +91,7 @@ pub fn chunk_upload(
 
     println!("Waiting for server to assemble uploaded mappings...");
 
-    let assemble_request = chunked_mappings.iter().map(to_assemble).collect();
+    let assemble_request = chunked_mappings.iter().collect();
     let start = Instant::now();
     while Instant::now().duration_since(start) < ASSEMBLE_POLL_TIMEOUT {
         let all_assembled = Api::current()