fix(build): Allow clearing string arguments to build upload (#2946)

### Description
Don't serialize empty-string values for `--vcs-provider`, `--head-repo-name`, `--head-ref`, `--base-ref`, and `--base-repo-name` into HTTP requests to the assemble build artifact endpoint. This allows users to set these values to empty strings to override any default values.

### Issues
- Resolves #2941
- Resolves [CLI-224](https://linear.app/getsentry/issue/CLI-224/skip-serializing-certain-fields-for-build-upload)

Author: szokeasaurusrex

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

src/api/data_types/chunking/build.rs

@@ -31,16 +31,16 @@ pub struct VcsInfo<'a> {
     pub head_sha: Option<Digest>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub base_sha: Option<Digest>,
-    #[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>,
 }

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(

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);
+}