Revert "feat: Don't upload chunks that are already on the server (#1651…

…)" This reverts commit 7470bac.
getsentry · Jun 27, 2023 · 13344ab · 13344ab
1 parent bc466fa
commit 13344ab
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 136 deletions.
diff --git a/src/utils/file_upload.rs b/src/utils/file_upload.rs
@@ -274,12 +274,39 @@ fn upload_files_parallel(
     Ok(())
 }
 
-fn poll_assemble(
-    checksum: Digest,
-    chunks: &[Digest],
+fn upload_files_chunked(
     context: &UploadContext,
+    files: &SourceFiles,
     options: &ChunkUploadOptions,
 ) -> Result<()> {
+    let archive = build_artifact_bundle(context, files, None)?;
+
+    let progress_style =
+        ProgressStyle::default_spinner().template("{spinner} Optimizing bundle for upload...");
+
+    let pb = ProgressBar::new_spinner();
+    pb.enable_steady_tick(100);
+    pb.set_style(progress_style);
+
+    let view = ByteView::open(archive.path())?;
+    let (checksum, checksums) = get_sha1_checksums(&view, options.chunk_size)?;
+    let chunks = view
+        .chunks(options.chunk_size as usize)
+        .zip(checksums.iter())
+        .map(|(data, checksum)| Chunk((*checksum, data)))
+        .collect::<Vec<_>>();
+
+    pb.finish_with_duration("Optimizing");
+
+    let progress_style = ProgressStyle::default_bar().template(&format!(
+        "{} Uploading files...\
+       \n{{wide_bar}}  {{bytes}}/{{total_bytes}} ({{eta}})",
+        style(">").dim(),
+    ));
+
+    upload_chunks(&chunks, options, progress_style)?;
+    println!("{} Uploaded files to Sentry", style(">").dim());
+
     let progress_style = ProgressStyle::default_spinner().template("{spinner} Processing files...");
 
     let pb = ProgressBar::new_spinner();
@@ -293,21 +320,21 @@ fn poll_assemble(
     };
 
     let api = Api::current();
-    let use_artifact_bundle =
-        options.supports(ChunkUploadCapability::ArtifactBundles) && context.project.is_some();
     let response = loop {
         // prefer standalone artifact bundle upload over legacy release based upload
-        let response = if use_artifact_bundle {
+        let response = if options.supports(ChunkUploadCapability::ArtifactBundles)
+            && context.project.is_some()
+        {
             api.assemble_artifact_bundle(
                 context.org,
                 vec![context.project.unwrap().to_string()],
                 checksum,
-                chunks,
+                &checksums,
                 context.release,
                 context.dist,
             )?
         } else {
-            api.assemble_release_artifacts(context.org, context.release()?, checksum, chunks)?
+            api.assemble_release_artifacts(context.org, context.release()?, checksum, &checksums)?
         };
 
         // Poll until there is a response, unless the user has specified to skip polling. In
@@ -349,66 +376,6 @@ fn poll_assemble(
     Ok(())
 }
 
-fn upload_files_chunked(
-    context: &UploadContext,
-    files: &SourceFiles,
-    options: &ChunkUploadOptions,
-) -> Result<()> {
-    let archive = build_artifact_bundle(context, files, None)?;
-
-    let progress_style =
-        ProgressStyle::default_spinner().template("{spinner} Optimizing bundle for upload...");
-
-    let pb = ProgressBar::new_spinner();
-    pb.enable_steady_tick(100);
-    pb.set_style(progress_style);
-
-    let view = ByteView::open(archive.path())?;
-    let (checksum, checksums) = get_sha1_checksums(&view, options.chunk_size)?;
-    let mut chunks = view
-        .chunks(options.chunk_size as usize)
-        .zip(checksums.iter())
-        .map(|(data, checksum)| Chunk((*checksum, data)))
-        .collect::<Vec<_>>();
-
-    pb.finish_with_duration("Optimizing");
-
-    let progress_style = ProgressStyle::default_bar().template(&format!(
-        "{} Uploading files...\
-       \n{{wide_bar}}  {{bytes}}/{{total_bytes}} ({{eta}})",
-        style(">").dim(),
-    ));
-
-    // Filter out chunks that are already on the server. This only matters if we're in the
-    // `assemble_artifact_bundle` case; `assemble_release_artifacts` always returns `missing_chunks: []`,
-    // so there's no point querying the endpoint.
-    if options.supports(ChunkUploadCapability::ArtifactBundles) && context.project.is_some() {
-        let api = Api::current();
-        let response = api.assemble_artifact_bundle(
-            context.org,
-            vec![context.project.unwrap().to_string()],
-            checksum,
-            &checksums,
-            context.release,
-            context.dist,
-        )?;
-        chunks.retain(|Chunk((digest, _))| response.missing_chunks.contains(digest));
-    };
-
-    upload_chunks(&chunks, options, progress_style)?;
-
-    if !chunks.is_empty() {
-        println!("{} Uploaded files to Sentry", style(">").dim());
-        poll_assemble(checksum, &checksums, context, options)
-    } else {
-        println!(
-            "{} Nothing to upload, all files are on the server",
-            style(">").dim()
-        );
-        Ok(())
-    }
-}
-
 fn build_debug_id(files: &SourceFiles) -> DebugId {
     let mut sorted_files = Vec::from_iter(files);
     sorted_files.sort_by_key(|x| x.0);

diff --git a/tests/integration/debug_files/bundle_jvm.rs b/tests/integration/debug_files/bundle_jvm.rs
@@ -21,8 +21,7 @@ fn command_bundle_jvm_out_not_found_creates_dir() {
         testcase_cwd_path.join("jvm"),
     )
     .unwrap();
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     register_test("debug_files/debug_files-bundle-jvm-output-not-found.trycmd");
 }
 
@@ -36,23 +35,20 @@ fn command_bundle_jvm_fails_out_is_file() {
     }
     copy_recursively("tests/integration/_fixtures/jvm/", testcase_cwd_path).unwrap();
     write(testcase_cwd_path.join("file.txt"), "some file content").unwrap();
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
 
     register_test("debug_files/debug_files-bundle-jvm-output-is-file.trycmd");
 }
 
 #[test]
 fn command_bundle_jvm_fails_input_not_found() {
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     register_test("debug_files/debug_files-bundle-jvm-input-not-found.trycmd");
 }
 
 #[test]
 fn command_bundle_jvm_fails_input_is_file() {
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     register_test("debug_files/debug_files-bundle-jvm-input-is-file.trycmd");
 }
 
@@ -66,15 +62,13 @@ fn command_bundle_jvm_input_dir_empty() {
     }
     copy_recursively("tests/integration/_fixtures/jvm/", testcase_cwd_path).unwrap();
     create_dir(testcase_cwd_path.join("empty-dir")).unwrap();
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     register_test("debug_files/debug_files-bundle-jvm-input-dir-empty.trycmd");
 }
 
 #[test]
 fn command_bundle_jvm_fails_invalid_uuid() {
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     register_test("debug_files/debug_files-bundle-jvm-invalid-uuid.trycmd");
 }
 
@@ -85,7 +79,6 @@ fn command_bundle_jvm() {
         remove_dir_all(testcase_cwd_path).unwrap();
     }
     copy_recursively("tests/integration/_fixtures/jvm/", testcase_cwd_path).unwrap();
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     register_test("debug_files/debug_files-bundle-jvm.trycmd");
 }
diff --git a/tests/integration/mod.rs b/tests/integration/mod.rs
@@ -121,30 +121,8 @@ pub enum ServerBehavior {
     Modern,
 }
 
-#[derive(Debug)]
-pub struct ChunkOptions {
-    chunk_size: usize,
-    missing_chunks: Vec<String>,
-}
-
-impl Default for ChunkOptions {
-    fn default() -> Self {
-        Self {
-            chunk_size: 8388608,
-            missing_chunks: vec![],
-        }
-    }
-}
-
 // Endpoints need to be bound, as they need to live long enough for test to finish
-pub fn mock_common_upload_endpoints(
-    behavior: ServerBehavior,
-    chunk_options: ChunkOptions,
-) -> Vec<Mock> {
-    let ChunkOptions {
-        chunk_size,
-        missing_chunks,
-    } = chunk_options;
+pub fn mock_common_upload_endpoints(behavior: ServerBehavior) -> Vec<Mock> {
     let (accept, release_request_count, assemble_endpoint) = match behavior {
         ServerBehavior::Legacy => (
             "\"release_files\"",
@@ -160,7 +138,7 @@ pub fn mock_common_upload_endpoints(
     let chunk_upload_response = format!(
         "{{
             \"url\": \"{}/api/0/organizations/wat-org/chunk-upload/\",
-            \"chunkSize\": {chunk_size},
+            \"chunkSize\": 8388608,
             \"chunksPerRequest\": 64,
             \"maxRequestSize\": 33554432,
             \"concurrency\": 8,
@@ -187,12 +165,9 @@ pub fn mock_common_upload_endpoints(
                 .with_response_body("[]"),
         ),
         mock_endpoint(
-            EndpointOptions::new("POST", assemble_endpoint, 200).with_response_body(format!(
-                r#"{{"state":"created","missingChunks":{}}}"#,
-                serde_json::to_string(&missing_chunks).unwrap()
-            )),
-        )
-        .expect_at_least(1),
+            EndpointOptions::new("POST", assemble_endpoint, 200)
+                .with_response_body(r#"{"state":"created","missingChunks":[]}"#),
+        ),
     ]
 }
 

diff --git a/tests/integration/sourcemaps/upload.rs b/tests/integration/sourcemaps/upload.rs
@@ -1,6 +1,6 @@
 use crate::integration::{
-    assert_endpoints, mock_common_upload_endpoints, mock_endpoint, register_test, ChunkOptions,
-    EndpointOptions, ServerBehavior,
+    assert_endpoints, mock_common_upload_endpoints, mock_endpoint, register_test, EndpointOptions,
+    ServerBehavior,
 };
 
 #[test]
@@ -15,7 +15,7 @@ fn command_sourcemaps_upload() {
 
 #[test]
 fn command_sourcemaps_upload_successfully_upload_file() {
-    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     let _files = mock_endpoint(
         EndpointOptions::new(
             "GET",
@@ -31,7 +31,7 @@ fn command_sourcemaps_upload_successfully_upload_file() {
 
 #[test]
 fn command_sourcemaps_upload_skip_already_uploaded() {
-    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     let _files = mock_endpoint(
         EndpointOptions::new(
             "GET",
@@ -56,7 +56,7 @@ fn command_sourcemaps_upload_skip_already_uploaded() {
 
 #[test]
 fn command_sourcemaps_upload_no_dedupe() {
-    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     let _files = mock_endpoint(
         EndpointOptions::new(
             "GET",
@@ -81,22 +81,14 @@ fn command_sourcemaps_upload_no_dedupe() {
 
 #[test]
 fn command_sourcemaps_upload_modern() {
-    let upload_endpoints = mock_common_upload_endpoints(
-        ServerBehavior::Modern,
-        ChunkOptions {
-            missing_chunks: vec!["ec8450a9db19805703a27a2545c18b7b27ba0d7d".to_string()],
-            // Set the chunk size so the bundle will be split into two chunks
-            chunk_size: 512,
-        },
-    );
+    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern);
     register_test("sourcemaps/sourcemaps-upload-modern.trycmd");
     assert_endpoints(&upload_endpoints);
 }
 
 #[test]
 fn command_sourcemaps_upload_empty() {
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy);
     let _files = mock_endpoint(
         EndpointOptions::new(
             "GET",
@@ -110,20 +102,13 @@ fn command_sourcemaps_upload_empty() {
 
 #[test]
 fn command_sourcemaps_upload_some_debugids() {
-    let upload_endpoints = mock_common_upload_endpoints(
-        ServerBehavior::Modern,
-        ChunkOptions {
-            missing_chunks: vec!["5e102ab3da27af9d1095a9c847d4e92a57fe01af".to_string()],
-            chunk_size: 524288,
-        },
-    );
+    let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern);
     register_test("sourcemaps/sourcemaps-upload-some-debugids.trycmd");
     assert_endpoints(&upload_endpoints);
 }
 
 #[test]
 fn command_sourcemaps_upload_no_debugids() {
-    let _upload_endpoints =
-        mock_common_upload_endpoints(ServerBehavior::Modern, Default::default());
+    let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern);
     register_test("sourcemaps/sourcemaps-upload-no-debugids.trycmd");
 }