From c24f6096d6b8bc933d84e97a6b0294c9ae00a64c Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 27 Jun 2023 10:27:37 +0200 Subject: [PATCH 1/3] feat: Don't upload chunks that are already on the server (Properly this time) --- CHANGELOG.md | 6 -- src/api.rs | 5 + src/utils/file_upload.rs | 111 +++++++++++++------- tests/integration/debug_files/bundle_jvm.rs | 21 ++-- tests/integration/mod.rs | 41 +++++++- tests/integration/sourcemaps/upload.rs | 47 +++++++-- 6 files changed, 168 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4919814b67..ef646340d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,6 @@ "You know what they say. Fool me once, strike one, but fool me twice... strike three." — Michael Scott -## Unreleased - -### Various fixes and improvements - -- Revert "feat: Don't upload chunks that are already on the server (#1651)" - ## 2.19.2 ### Various fixes and improvements diff --git a/src/api.rs b/src/api.rs index 6305864e84..ca403e3565 100644 --- a/src/api.rs +++ b/src/api.rs @@ -2610,6 +2610,10 @@ pub enum ChunkUploadCapability { /// Chunked upload of standalone artifact bundles ArtifactBundles, + /// Like `ArtifactBundles`, but with deduplicated chunk + /// upload. + ArtifactBundlesV2, + /// Upload of PDBs and debug id overrides Pdbs, @@ -2638,6 +2642,7 @@ impl<'de> Deserialize<'de> for ChunkUploadCapability { "debug_files" => ChunkUploadCapability::DebugFiles, "release_files" => ChunkUploadCapability::ReleaseFiles, "artifact_bundles" => ChunkUploadCapability::ArtifactBundles, + "artifact_bundles_v2" => ChunkUploadCapability::ArtifactBundlesV2, "pdbs" => ChunkUploadCapability::Pdbs, "portablepdbs" => ChunkUploadCapability::PortablePdbs, "sources" => ChunkUploadCapability::Sources, diff --git a/src/utils/file_upload.rs b/src/utils/file_upload.rs index 12e3c3ad3c..02971e3b24 100644 --- a/src/utils/file_upload.rs +++ b/src/utils/file_upload.rs @@ -41,6 +41,7 @@ pub fn initialize_legacy_release_upload(context: &UploadContext) -> Result<()> { if context.project.is_some() && context.chunk_upload_options.map_or(false, |x| { x.supports(ChunkUploadCapability::ArtifactBundles) + || x.supports(ChunkUploadCapability::ArtifactBundlesV2) }) { return Ok(()); @@ -274,39 +275,12 @@ fn upload_files_parallel( Ok(()) } -fn upload_files_chunked( +fn poll_assemble( + checksum: Digest, + chunks: &[Digest], 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::>(); - - 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(); @@ -320,21 +294,22 @@ fn upload_files_chunked( }; let api = Api::current(); + let use_artifact_bundle = (options.supports(ChunkUploadCapability::ArtifactBundles) + || options.supports(ChunkUploadCapability::ArtifactBundlesV2)) + && context.project.is_some(); let response = loop { // prefer standalone artifact bundle upload over legacy release based upload - let response = if options.supports(ChunkUploadCapability::ArtifactBundles) - && context.project.is_some() - { + let response = if use_artifact_bundle { api.assemble_artifact_bundle( context.org, vec![context.project.unwrap().to_string()], checksum, - &checksums, + chunks, context.release, context.dist, )? } else { - api.assemble_release_artifacts(context.org, context.release()?, checksum, &checksums)? + api.assemble_release_artifacts(context.org, context.release()?, checksum, chunks)? }; // Poll until there is a response, unless the user has specified to skip polling. In @@ -376,6 +351,65 @@ fn upload_files_chunked( 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::>(); + + 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 the server supports + // `ArtifactBundlesV2`, otherwise the `missing_chunks` field is meaningless. + if options.supports(ChunkUploadCapability::ArtifactBundlesV2) && 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); @@ -524,7 +558,12 @@ fn print_upload_context_details(context: &UploadContext) { ); let upload_type = match context.chunk_upload_options { None => "single file", - Some(opts) if opts.supports(ChunkUploadCapability::ArtifactBundles) => "artifact bundle", + Some(opts) + if opts.supports(ChunkUploadCapability::ArtifactBundles) + || opts.supports(ChunkUploadCapability::ArtifactBundlesV2) => + { + "artifact bundle" + } _ => "release bundle", }; println!( diff --git a/tests/integration/debug_files/bundle_jvm.rs b/tests/integration/debug_files/bundle_jvm.rs index c8a1291894..5c18ad4329 100644 --- a/tests/integration/debug_files/bundle_jvm.rs +++ b/tests/integration/debug_files/bundle_jvm.rs @@ -21,7 +21,8 @@ fn command_bundle_jvm_out_not_found_creates_dir() { testcase_cwd_path.join("jvm"), ) .unwrap(); - let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); register_test("debug_files/debug_files-bundle-jvm-output-not-found.trycmd"); } @@ -35,20 +36,23 @@ 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); register_test("debug_files/debug_files-bundle-jvm-input-is-file.trycmd"); } @@ -62,13 +66,15 @@ 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); register_test("debug_files/debug_files-bundle-jvm-invalid-uuid.trycmd"); } @@ -79,6 +85,7 @@ 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); register_test("debug_files/debug_files-bundle-jvm.trycmd"); } diff --git a/tests/integration/mod.rs b/tests/integration/mod.rs index 48a27feebf..c6afeaca21 100644 --- a/tests/integration/mod.rs +++ b/tests/integration/mod.rs @@ -119,10 +119,33 @@ pub fn copy_recursively(source: impl AsRef, destination: impl AsRef) pub enum ServerBehavior { Legacy, Modern, + ModernV2, +} + +#[derive(Debug)] +pub struct ChunkOptions { + chunk_size: usize, + missing_chunks: Vec, +} + +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) -> Vec { +pub fn mock_common_upload_endpoints( + behavior: ServerBehavior, + chunk_options: ChunkOptions, +) -> Vec { + let ChunkOptions { + chunk_size, + missing_chunks, + } = chunk_options; let (accept, release_request_count, assemble_endpoint) = match behavior { ServerBehavior::Legacy => ( "\"release_files\"", @@ -134,11 +157,16 @@ pub fn mock_common_upload_endpoints(behavior: ServerBehavior) -> Vec { 0, "/api/0/organizations/wat-org/artifactbundle/assemble/", ), + ServerBehavior::ModernV2 => ( + "\"release_files\", \"artifact_bundles_v2\"", + 0, + "/api/0/organizations/wat-org/artifactbundle/assemble/", + ), }; let chunk_upload_response = format!( "{{ \"url\": \"{}/api/0/organizations/wat-org/chunk-upload/\", - \"chunkSize\": 8388608, + \"chunkSize\": {chunk_size}, \"chunksPerRequest\": 64, \"maxRequestSize\": 33554432, \"concurrency\": 8, @@ -165,9 +193,12 @@ pub fn mock_common_upload_endpoints(behavior: ServerBehavior) -> Vec { .with_response_body("[]"), ), mock_endpoint( - EndpointOptions::new("POST", assemble_endpoint, 200) - .with_response_body(r#"{"state":"created","missingChunks":[]}"#), - ), + 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), ] } diff --git a/tests/integration/sourcemaps/upload.rs b/tests/integration/sourcemaps/upload.rs index 66c3ff35ac..94b5964837 100644 --- 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, EndpointOptions, - ServerBehavior, + assert_endpoints, mock_common_upload_endpoints, mock_endpoint, register_test, ChunkOptions, + 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); + let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); 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); + let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); 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); + let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); let _files = mock_endpoint( EndpointOptions::new( "GET", @@ -81,14 +81,29 @@ fn command_sourcemaps_upload_no_dedupe() { #[test] fn command_sourcemaps_upload_modern() { - let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern); + let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern, Default::default()); + register_test("sourcemaps/sourcemaps-upload-modern.trycmd"); + assert_endpoints(&upload_endpoints); +} + +#[test] +fn command_sourcemaps_upload_modern_v2() { + let upload_endpoints = mock_common_upload_endpoints( + ServerBehavior::ModernV2, + ChunkOptions { + missing_chunks: vec!["ec8450a9db19805703a27a2545c18b7b27ba0d7d".to_string()], + // Set the chunk size so the bundle will be split into two chunks + chunk_size: 512, + }, + ); 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Legacy, Default::default()); let _files = mock_endpoint( EndpointOptions::new( "GET", @@ -102,13 +117,27 @@ fn command_sourcemaps_upload_empty() { #[test] fn command_sourcemaps_upload_some_debugids() { - let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern); + let upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Modern, Default::default()); + register_test("sourcemaps/sourcemaps-upload-some-debugids.trycmd"); + assert_endpoints(&upload_endpoints); +} + +#[test] +fn command_sourcemaps_upload_some_debugids_v2() { + let upload_endpoints = mock_common_upload_endpoints( + ServerBehavior::ModernV2, + ChunkOptions { + missing_chunks: vec!["5e102ab3da27af9d1095a9c847d4e92a57fe01af".to_string()], + chunk_size: 524288, + }, + ); 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); + let _upload_endpoints = + mock_common_upload_endpoints(ServerBehavior::Modern, Default::default()); register_test("sourcemaps/sourcemaps-upload-no-debugids.trycmd"); } From 346e8d906a5270a8a4c60ab4dff37dcbc33e0d62 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 27 Jun 2023 11:03:58 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef646340d6..bfb9de3f96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ "You know what they say. Fool me once, strike one, but fool me twice... strike three." — Michael Scott +## Unreleased + +### Various fixes and improvements + +- feat: Don't upload chunks that are already on the server (fixed version) (#1654) + ## 2.19.2 ### Various fixes and improvements From ce0cd2d435073ad7788b27b39159c3379604ce65 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 27 Jun 2023 11:19:16 +0200 Subject: [PATCH 3/3] fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bfb9de3f96..0fa8c0637c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Various fixes and improvements -- feat: Don't upload chunks that are already on the server (fixed version) (#1654) +- feat: Don't upload chunks that are already on the server (fixed version) (#1660) ## 2.19.2