diff --git a/CHANGELOG.md b/CHANGELOG.md index 1afdc592ff..8b91b2f62f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Added validation for the `sentry-cli build upload` command's `--head-sha` and `--base-sha` arguments ([#2945](https://github.com/getsentry/sentry-cli/pull/2945)). The CLI now validates that these are valid SHA1 sums. Passing an empty string is also allowed; this prevents the default values from being used, causing the values to instead be unset. +### Fixes + +- Fixed a bug where providing empty-string values for the `sentry-cli build upload` command's `--vcs-provider`, `--head-repo-name`, `--head-ref`, `--base-ref`, and `--base-repo-name` arguments resulted in 400 errors ([#2946](https://github.com/getsentry/sentry-cli/pull/2946)). Now, setting these to empty strings instead explicitly clears the default value we would set otherwise, as expected. + ## 2.58.1 ### Deprecations diff --git a/src/api/data_types/chunking/build.rs b/src/api/data_types/chunking/build.rs index aad4521737..072f2136ab 100644 --- a/src/api/data_types/chunking/build.rs +++ b/src/api/data_types/chunking/build.rs @@ -31,16 +31,16 @@ pub struct VcsInfo<'a> { pub head_sha: Option, #[serde(skip_serializing_if = "Option::is_none")] pub base_sha: Option, - #[serde(skip_serializing_if = "Option::is_none", rename = "provider")] - pub vcs_provider: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - pub head_repo_name: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - pub base_repo_name: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - pub head_ref: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - pub base_ref: Option<&'a str>, + #[serde(skip_serializing_if = "str::is_empty", rename = "provider")] + pub vcs_provider: &'a str, + #[serde(skip_serializing_if = "str::is_empty")] + pub head_repo_name: &'a str, + #[serde(skip_serializing_if = "str::is_empty")] + pub base_repo_name: &'a str, + #[serde(skip_serializing_if = "str::is_empty")] + pub head_ref: &'a str, + #[serde(skip_serializing_if = "str::is_empty")] + pub base_ref: &'a str, #[serde(skip_serializing_if = "Option::is_none")] pub pr_number: Option<&'a u32>, } diff --git a/src/commands/build/upload.rs b/src/commands/build/upload.rs index c1131bc434..9b605a9a86 100644 --- a/src/commands/build/upload.rs +++ b/src/commands/build/upload.rs @@ -137,7 +137,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { .as_ref() .map(|url| get_provider_from_remote(url)) .map(Cow::Owned) - }); + }) + .unwrap_or_default(); let head_repo_name = matches .get_one("head_repo_name") @@ -148,7 +149,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { .as_ref() .map(|url| get_repo_from_remote_preserve_case(url)) .map(Cow::Owned) - }); + }) + .unwrap_or_default(); let head_ref = matches .get_one("head_ref") @@ -175,7 +177,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { } }) .map(Cow::Owned) - }); + }) + .unwrap_or_default(); let base_ref = matches .get_one("base_ref") @@ -199,7 +202,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { } }) .map(Cow::Owned) - }); + }) + .unwrap_or_default(); let base_repo_name = matches .get_one("base_repo_name") @@ -223,7 +227,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { } }) .map(Cow::Owned) - }); + }) + .unwrap_or_default(); ( vcs_provider, @@ -266,7 +271,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { base_sha.expect("base_sha is Some at this point") ); base_sha = None; - base_ref = None; + base_ref = "".into(); } let pr_number = matches .get_one("pr_number") @@ -331,11 +336,11 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { let vcs_info = VcsInfo { head_sha, base_sha, - vcs_provider: vcs_provider.as_deref(), - head_repo_name: head_repo_name.as_deref(), - base_repo_name: base_repo_name.as_deref(), - head_ref: head_ref.as_deref(), - base_ref: base_ref.as_deref(), + vcs_provider: &vcs_provider, + head_repo_name: &head_repo_name, + base_repo_name: &base_repo_name, + head_ref: &head_ref, + base_ref: &base_ref, pr_number: pr_number.as_ref(), }; match upload_file( diff --git a/tests/integration/build/upload.rs b/tests/integration/build/upload.rs index 8dad5a29c6..dd4159393c 100644 --- a/tests/integration/build/upload.rs +++ b/tests/integration/build/upload.rs @@ -278,3 +278,57 @@ fn command_build_upload_empty_shas() { .register_trycmd_test("build/build-upload-empty-shas.trycmd") .with_default_token(); } + +/// Verify that all string-based arguments to `build upload` can be set to an empty string, +/// and that setting to empty string results in the corresponding fields being omitted from +/// the request body. +#[test] +fn command_build_upload_empty_refs() { + TestManager::new() + .mock_endpoint( + MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/") + .with_response_file("build/get-chunk-upload.json"), + ) + .mock_endpoint( + MockEndpointBuilder::new( + "POST", + "/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/", + ) + .with_response_fn(move |req| { + let body = req.body().expect("body should be readable"); + let json: serde_json::Value = + serde_json::from_slice(body).expect("body should be valid JSON"); + + assert!(json.get("provider").is_none()); + assert!(json.get("head_repo_name").is_none()); + assert!(json.get("base_repo_name").is_none()); + assert!(json.get("head_ref").is_none()); + assert!(json.get("base_ref").is_none()); + + serde_json::json!({ + "state": "created", + "missingChunks": [], + "artifactUrl": "http://sentry.io/wat-org/preprod/wat-project/42" + }) + .to_string() + .into() + }), + ) + .assert_cmd([ + "build", + "upload", + "tests/integration/_fixtures/build/apk.apk", + "--vcs-provider", + "", + "--head-repo-name", + "", + "--base-repo-name", + "", + "--head-ref", + "", + "--base-ref", + "", + ]) + .with_default_token() + .run_and_assert(AssertCommand::Success); +}