chore: Make bucket name required in storage config (#2336)

.gitignore

@@ -16,6 +16,7 @@ benchmarks/artifacts/
 /deps
 
 .vscode/
+.helix/
 
 # Spark jar for iceberg data generation
 *.jar

crates/glaredb/src/commands.rs

@@ -252,7 +252,7 @@ impl RunCommand for MetastoreArgs {
             (Some(bucket), Some(service_account_path), None) => {
                 let service_account_key = std::fs::read_to_string(service_account_path)?;
                 StorageConfig::Gcs {
-                    bucket: Some(bucket),
+                    bucket,
                     service_account_key,
                 }
             }

crates/object_store_util/src/conf.rs

@@ -18,16 +18,16 @@ pub enum StorageConfig {
         secret_access_key: String,
         region: Option<String>,
         endpoint: Option<String>,
-        bucket: Option<String>,
+        bucket: String,
     },
     Gcs {
         service_account_key: String,
-        bucket: Option<String>,
+        bucket: String,
     },
     Azure {
         account_name: String,
         access_key: String,
-        container_name: Option<String>,
+        container_name: String,
     },
     Local {
         path: PathBuf,
@@ -49,6 +49,7 @@ impl StorageConfig {
                 let mut builder = AmazonS3Builder::new()
                     .with_access_key_id(access_key_id)
                     .with_secret_access_key(secret_access_key)
+                    .with_bucket_name(bucket)
                     .with_region(region.clone().unwrap_or_default());
 
                 if let Some(endpoint) = endpoint {
@@ -66,22 +67,15 @@ impl StorageConfig {
                     }
                 }
 
-                if let Some(bucket) = bucket {
-                    builder = builder.with_bucket_name(bucket);
-                }
-
                 Arc::new(builder.build()?)
             }
             StorageConfig::Gcs {
                 service_account_key,
                 bucket,
             } => {
-                let mut builder =
-                    GoogleCloudStorageBuilder::new().with_service_account_key(service_account_key);
-
-                if let Some(bucket) = bucket {
-                    builder = builder.with_bucket_name(bucket);
-                }
+                let builder = GoogleCloudStorageBuilder::new()
+                    .with_service_account_key(service_account_key)
+                    .with_bucket_name(bucket);
 
                 Arc::new(builder.build()?)
             }
@@ -90,13 +84,10 @@ impl StorageConfig {
                 access_key,
                 container_name,
             } => {
-                let mut builder = MicrosoftAzureBuilder::new()
+                let builder = MicrosoftAzureBuilder::new()
                     .with_account(account_name)
-                    .with_access_key(access_key);
-
-                if let Some(container_name) = container_name {
-                    builder = builder.with_container_name(container_name);
-                }
+                    .with_access_key(access_key)
+                    .with_container_name(container_name);
 
                 Arc::new(builder.build()?)
             }

crates/sqlexec/src/engine.rs

@@ -114,7 +114,8 @@ impl EngineStorageConfig {
                         let bucket = opts
                             .get("bucket")
                             .cloned()
-                            .or(url.host_str().map(|h| h.to_string()));
+                            .or(url.host_str().map(|h| h.to_string()))
+                            .ok_or(ExecError::MissingBucketName)?;
 
                         EngineStorageConfig {
                             location: url.clone(),
@@ -156,6 +157,8 @@ impl EngineStorageConfig {
                             }
                         }
 
+                        let bucket = bucket.ok_or(ExecError::MissingBucketName)?;
+
                         EngineStorageConfig {
                             location: url.clone(),
                             conf: StorageConfig::S3 {
@@ -186,7 +189,8 @@ impl EngineStorageConfig {
                         let container_name = opts
                             .get("container_name")
                             .cloned()
-                            .or(url.host_str().map(|h| h.to_string()));
+                            .or(url.host_str().map(|h| h.to_string()))
+                            .ok_or(ExecError::MissingBucketName)?;
 
                         EngineStorageConfig {
                             location: url.clone(),
@@ -227,21 +231,15 @@ impl EngineStorageConfig {
                         .map_err(DatasourceCommonError::from)?,
                     conf: StorageConfig::Gcs {
                         service_account_key,
-                        bucket: Some(bucket),
+                        bucket,
                     },
                 }
             }
-            // Expected gcs config opts for the session.
-            (StorageConfig::Gcs { bucket: None, .. }, None) => {
-                return Err(ExecError::InvalidStorageConfig(
-                    "Missing bucket on session configuration",
-                ))
-            }
             (_, Some(_)) => EngineStorageConfig {
                 location: Url::parse("memory://").map_err(DatasourceCommonError::from)?,
                 conf: StorageConfig::Memory,
             },
-            _ => self.clone(),
+            (_, None) => self.clone(),
         })
     }
 
@@ -567,14 +565,14 @@ mod tests {
                 secret_access_key,
                 region: None,
                 endpoint: None,
-                bucket: Some("some-bucket".to_string()),
+                bucket: "some-bucket".to_string(),
             }
         );
 
         let merged_conf = conf.with_session_config(&SessionStorageConfig {
             gcs_bucket: Some("my-other-bucket".to_string()),
         })?;
-        assert_eq!(merged_conf.conf, StorageConfig::Memory,);
+        assert_eq!(merged_conf.conf, StorageConfig::Memory);
         Ok(())
     }
 }

crates/sqlexec/src/errors.rs

@@ -124,6 +124,9 @@ pub enum ExecError {
     #[error("Invalid URL for remote execution: {0}")]
     InvalidRemoteExecUrl(String),
 
+    #[error("Missing bucket name: required for storage configuration")]
+    MissingBucketName,
+
     #[error(transparent)]
     DatasourceDebug(#[from] datasources::debug::errors::DebugError),