Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/api/data_types/code_mappings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub struct BulkCodeMappingsRequest {
pub mappings: Vec<BulkCodeMapping>,
}

#[derive(Debug, Deserialize, Serialize)]
#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct BulkCodeMapping {
pub stack_root: String,
Expand Down
98 changes: 72 additions & 26 deletions src/commands/code_mappings/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,16 @@ use anyhow::{bail, Context as _, Result};
use clap::{Arg, ArgMatches, Command};
use log::debug;

use crate::api::{Api, BulkCodeMapping, BulkCodeMappingResult, BulkCodeMappingsRequest};
use crate::api::{
Api, BulkCodeMapping, BulkCodeMappingResult, BulkCodeMappingsRequest, BulkCodeMappingsResponse,
};
use crate::config::Config;
use crate::utils::formatting::Table;
use crate::utils::vcs;

/// Maximum number of mappings the backend accepts per request.
const BATCH_SIZE: usize = 300;

pub fn make_command(command: Command) -> Command {
command
.about("Upload code mappings for a project from a JSON file. Each mapping pairs a stack trace root (e.g. com/example/module) with the corresponding source path in your repository (e.g. modules/module/src/main/java/com/example/module).")
Expand Down Expand Up @@ -144,56 +149,97 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
};

let mapping_count = mappings.len();
let request = BulkCodeMappingsRequest {
project,
repository: repo_name,
default_branch,
mappings,
};
let batches: Vec<&[BulkCodeMapping]> = mappings.chunks(BATCH_SIZE).collect();
let total_batches = batches.len();
Comment on lines +152 to +153
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: You can avoid allocating a Vec for the chunks pretty easily. Here, you compute the total batches using div_ceil, and in the loop, you iterate the batches directly, computing them lazily on each iteration.

Suggested change
let batches: Vec<&[BulkCodeMapping]> = mappings.chunks(BATCH_SIZE).collect();
let total_batches = batches.len();
let total_batches = mapping_count.div_ceil(BATCH_SIZE);


println!("Uploading {mapping_count} code mapping(s)...");

let api = Api::current();
let response = api
.authenticated()?
.bulk_upload_code_mappings(&org, &request)?;
let authenticated = api.authenticated()?;

let mut merged = MergedResponse::default();

for (i, batch) in batches.iter().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: companion comment

Suggested change
for (i, batch) in batches.iter().enumerate() {
for (i, batch) in mappings.chunks(BATCH_SIZE).enumerate() {

if total_batches > 1 {
println!("Sending batch {}/{total_batches}...", i + 1);
}
let request = BulkCodeMappingsRequest {
project: project.clone(),
repository: repo_name.clone(),
default_branch: default_branch.clone(),
mappings: batch.to_vec(),
};
Comment on lines +166 to +171
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: Please see https://github.com/getsentry/sentry-cli/pull/3209/changes#r2959847064; we can and probably should avoid cloning these strings and the vector.

match authenticated.bulk_upload_code_mappings(&org, &request) {
Ok(response) => merged.add(response),
Err(err) => {
merged
.batch_errors
.push(format!("Batch {}/{total_batches} failed: {err}", i + 1));
}
}
}
Comment on lines +160 to +180
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: I think it is a bit cleaner if you separate the merging the response from actually making the bulk requests. You can do this by implementing FromIterator<Result<BulkCodeMappingsResponse, String>> for MergedResponse (see companion comment) and then just collect-ing into the MergedResponse

Suggested change
let mut merged = MergedResponse::default();
for (i, batch) in batches.iter().enumerate() {
if total_batches > 1 {
println!("Sending batch {}/{total_batches}...", i + 1);
}
let request = BulkCodeMappingsRequest {
project: project.clone(),
repository: repo_name.clone(),
default_branch: default_branch.clone(),
mappings: batch.to_vec(),
};
match authenticated.bulk_upload_code_mappings(&org, &request) {
Ok(response) => merged.add(response),
Err(err) => {
merged
.batch_errors
.push(format!("Batch {}/{total_batches} failed: {err}", i + 1));
}
}
}
let merged: MergedResponse = mappings
.chunks(BATCH_SIZE)
.enumerate()
.map(|(i, batch)| {
if total_batches > 1 {
println!("Sending batch {}/{total_batches}...", i + 1);
}
let request = BulkCodeMappingsRequest {
project: project.clone(),
repository: repo_name.clone(),
default_branch: default_branch.clone(),
mappings: batch.to_vec(),
};
authenticated
.bulk_upload_code_mappings(&org, &request)
.map_err(|err| format!("Batch {}/{total_batches} failed: {err}", i + 1))
})
.collect();


// Display error details (successful mappings are summarized in counts only).
print_error_table(&merged.mappings);

for err in &merged.batch_errors {
println!("{err}");
}

print_results_table(response.mappings);
let total_errors = merged.errors + merged.batch_errors.len() as u64;
println!(
"Created: {}, Updated: {}, Errors: {}",
response.created, response.updated, response.errors
"Created: {}, Updated: {}, Errors: {total_errors}",
merged.created, merged.updated
);

if response.errors > 0 {
bail!(
"{} mapping(s) failed to upload. See errors above.",
response.errors
);
if total_errors > 0 {
bail!("{total_errors} error(s) during upload. See details above.");
}

Ok(())
}

fn print_results_table(mappings: Vec<BulkCodeMappingResult>) {
fn print_error_table(mappings: &[BulkCodeMappingResult]) {
let error_mappings: Vec<_> = mappings.iter().filter(|r| r.status == "error").collect();

if error_mappings.is_empty() {
return;
}

let mut table = Table::new();
table
.title_row()
.add("Stack Root")
.add("Source Root")
.add("Status");
.add("Detail");

for result in mappings {
let status = match result.detail {
Some(detail) if result.status == "error" => format!("error: {detail}"),
_ => result.status,
};
for result in &error_mappings {
Comment on lines +203 to +216
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: We can avoid allocating a vector for the error_mappings by directly iterating over the filter

Suggested change
let error_mappings: Vec<_> = mappings.iter().filter(|r| r.status == "error").collect();
if error_mappings.is_empty() {
return;
}
let mut table = Table::new();
table
.title_row()
.add("Stack Root")
.add("Source Root")
.add("Status");
.add("Detail");
for result in mappings {
let status = match result.detail {
Some(detail) if result.status == "error" => format!("error: {detail}"),
_ => result.status,
};
for result in &error_mappings {
if !mappings.iter().any(|r| r.status == "error") {
return;
}
let mut table = Table::new();
table
.title_row()
.add("Stack Root")
.add("Source Root")
.add("Detail");
for result in mappings.iter().filter(|r| r.status == "error") {

let detail = result.detail.as_deref().unwrap_or("unknown error");
table
.add_row()
.add(&result.stack_root)
.add(&result.source_root)
.add(&status);
.add(detail);
}

table.print();
println!();
}

#[derive(Default)]
struct MergedResponse {
created: u64,
updated: u64,
errors: u64,
mappings: Vec<BulkCodeMappingResult>,
batch_errors: Vec<String>,
}

impl MergedResponse {
fn add(&mut self, response: BulkCodeMappingsResponse) {
self.created += response.created;
self.updated += response.updated;
self.errors += response.errors;
self.mappings.extend(response.mappings);
}
}
Comment on lines +238 to +245
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: By implementing FromIterator, you can just .collect directly to the MergedResponse. See here.

Suggested change
impl MergedResponse {
fn add(&mut self, response: BulkCodeMappingsResponse) {
self.created += response.created;
self.updated += response.updated;
self.errors += response.errors;
self.mappings.extend(response.mappings);
}
}
impl FromIterator<Result<BulkCodeMappingsResponse, String>> for MergedResponse {
fn from_iter<I>(iter: I) -> Self
where
I: IntoIterator<Item = Result<BulkCodeMappingsResponse, String>>,
{
let mut merged = Self::default();
for result in iter {
match result {
Ok(response) => {
merged.created += response.created;
merged.updated += response.updated;
merged.errors += response.errors;
merged.mappings.extend(response.mappings);
}
Err(err) => merged.batch_errors.push(err),
}
}
merged
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
```
$ sentry-cli code-mappings upload tests/integration/_fixtures/code_mappings/mappings.json --org wat-org --project wat-project --repo owner/repo --default-branch main
? failed
Uploading 2 code mapping(s)...
+------------------+---------------------------------------------+-------------------+
| Stack Root | Source Root | Detail |
+------------------+---------------------------------------------+-------------------+
| com/example/maps | modules/maps/src/main/java/com/example/maps | duplicate mapping |
+------------------+---------------------------------------------+-------------------+

Created: 1, Updated: 0, Errors: 1
error: 1 error(s) during upload. See details above.

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.

```
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
```
$ sentry-cli code-mappings upload tests/integration/_fixtures/code_mappings/mappings.json --org wat-org --project wat-project --repo owner/repo --default-branch main
? failed
Uploading 2 code mapping(s)...
+------------------+---------------------------------------------+-------------------+
| Stack Root | Source Root | Detail |
+------------------+---------------------------------------------+-------------------+
| com/example/maps | modules/maps/src/main/java/com/example/maps | duplicate mapping |
+------------------+---------------------------------------------+-------------------+

Created: 1, Updated: 0, Errors: 1
error: 1 error(s) during upload. See details above.

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.

```
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,6 @@
$ sentry-cli code-mappings upload tests/integration/_fixtures/code_mappings/mappings.json --org wat-org --project wat-project --repo owner/repo --default-branch main
? success
Uploading 2 code mapping(s)...
+------------------+---------------------------------------------+---------+
| Stack Root | Source Root | Status |
+------------------+---------------------------------------------+---------+
| com/example/core | modules/core/src/main/java/com/example/core | created |
| com/example/maps | modules/maps/src/main/java/com/example/maps | created |
+------------------+---------------------------------------------+---------+

Created: 2, Updated: 0, Errors: 0

```
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"created": 1,
"updated": 0,
"errors": 1,
"mappings": [
{"stackRoot": "com/example/core", "sourceRoot": "modules/core/src/main/java/com/example/core", "status": "created"},
{"stackRoot": "com/example/maps", "sourceRoot": "modules/maps/src/main/java/com/example/maps", "status": "error", "detail": "duplicate mapping"}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"created": 1,
"updated": 0,
"errors": 1,
"mappings": [
{"stackRoot": "com/example/core", "sourceRoot": "modules/core/src/main/java/com/example/core", "status": "created"},
{"stackRoot": "com/example/maps", "sourceRoot": "modules/maps/src/main/java/com/example/maps", "status": "error", "detail": "duplicate mapping"}
]
}
85 changes: 84 additions & 1 deletion tests/integration/code_mappings/upload.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::integration::{MockEndpointBuilder, TestManager};
use std::sync::atomic::{AtomicU16, Ordering};

use crate::integration::{AssertCommand, MockEndpointBuilder, TestManager};

#[test]
fn command_code_mappings_upload() {
Expand All @@ -10,3 +12,84 @@ fn command_code_mappings_upload() {
.register_trycmd_test("code_mappings/code-mappings-upload.trycmd")
.with_default_token();
}

#[test]
fn command_code_mappings_upload_partial_error() {
TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/code-mappings/bulk/")
.with_response_file("code_mappings/post-bulk-partial-error.json"),
)
.register_trycmd_test("code_mappings/code-mappings-upload-partial-error.trycmd")
.with_default_token();
}

#[test]
fn command_code_mappings_upload_207_partial_error() {
TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/code-mappings/bulk/")
.with_status(207)
.with_response_file("code_mappings/post-bulk-207-partial-error.json"),
)
.register_trycmd_test("code_mappings/code-mappings-upload-207-partial-error.trycmd")
.with_default_token();
}

#[test]
fn command_code_mappings_upload_batches() {
// Generate a fixture with 301 mappings to force 2 batches (300 + 1).
let mut mappings = Vec::with_capacity(301);
for i in 0..301 {
mappings.push(serde_json::json!({
"stackRoot": format!("com/example/m{i}"),
"sourceRoot": format!("modules/m{i}/src/main/java/com/example/m{i}"),
}));
}
let fixture = tempfile::NamedTempFile::new().expect("failed to create temp file");
serde_json::to_writer(&fixture, &mappings).expect("failed to write fixture");

let call_count = AtomicU16::new(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: 8 bits is enough

Suggested change
let call_count = AtomicU16::new(0);
let call_count = AtomicU8::new(0);


TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/code-mappings/bulk/")
.expect(2)
.with_response_fn(move |_request| {
let n = call_count.fetch_add(1, Ordering::Relaxed);
// Return appropriate counts per batch
let (created, mapping_count) = if n == 0 { (300, 300) } else { (1, 1) };
let mut batch_mappings = Vec::new();
for i in 0..mapping_count {
let idx = n as usize * 300 + i;
batch_mappings.push(serde_json::json!({
"stackRoot": format!("com/example/m{idx}"),
"sourceRoot": format!("modules/m{idx}/src/main/java/com/example/m{idx}"),
"status": "created",
}));
}
serde_json::to_vec(&serde_json::json!({
"created": created,
"updated": 0,
"errors": 0,
"mappings": batch_mappings,
}))
.expect("failed to serialize response")
}),
)
.assert_cmd([
"code-mappings",
"upload",
fixture.path().to_str().expect("valid utf-8 path"),
"--org",
"wat-org",
"--project",
"wat-project",
"--repo",
"owner/repo",
"--default-branch",
"main",
])
.with_default_token()
.run_and_assert(AssertCommand::Success);
}
Loading