Skip to content

Fix/s3 table creds #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
codingcyclist merged 1 commit into from
Apr 26, 2026
Merged

Fix/s3 table creds #268

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions crates/tower-cmd/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,9 @@ async fn get_catalogs(
let mut vals = HashMap::new();

for catalog in res.catalogs {
// we will decrypt each property and inject it into the vals map.
// Decrypt each property and inject it into the vals map. The API returns
// the correct environment_variable name for each property (including S3
// Tables remapping, static constants, and derived URI).
for property in catalog.properties {
let decrypted_value =
crypto::decrypt(private_key.clone(), property.encrypted_value.to_string())
Expand All @@ -625,7 +627,11 @@ async fn get_catalogs(
"Failed to decrypt catalog property",
))
})?;
let name = create_pyiceberg_catalog_property_name(&catalog.name, &property.name);
let name = property
.environment_variable
.unwrap_or_else(|| {
create_pyiceberg_catalog_property_name(&catalog.name, &property.name)
});
Comment on lines +630 to +634
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Defensively handle empty environment_variable values.

At Line 630, Some("") will bypass fallback and produce an invalid env var key. Treat empty/whitespace values as missing and then fallback to create_pyiceberg_catalog_property_name.

Suggested patch 
-            let name = property
-                .environment_variable
-                .unwrap_or_else(|| {
-                    create_pyiceberg_catalog_property_name(&catalog.name, &property.name)
-                });
+            let name = property
+                .environment_variable
+                .as_deref()
+                .filter(|value| !value.trim().is_empty())
+                .map(str::to_string)
+                .unwrap_or_else(|| {
+                    create_pyiceberg_catalog_property_name(&catalog.name, &property.name)
+                });
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/tower-cmd/src/run.rs` around lines 630 - 634, The code currently uses
property.environment_variable.unwrap_or_else(...) which treats Some("") as
valid; change the logic that sets name so empty or all-whitespace
environment_variable values are treated as missing by checking/trimming
property.environment_variable before deciding to use it, and only fall back to
create_pyiceberg_catalog_property_name(&catalog.name, &property.name) when the
env var is None or blank; update the assignment that defines name (referencing
property.environment_variable, create_pyiceberg_catalog_property_name,
catalog.name, property.name) to perform that defensive check.

vals.insert(name, decrypted_value);
}
}
Expand Down Expand Up @@ -743,10 +749,12 @@ async fn monitor_cli_status(

fn create_pyiceberg_catalog_property_name(catalog_name: &str, property_name: &str) -> String {
let catalog_name = catalog_name
.replace('-', "_")
.replace('.', "_")
.replace(':', "_")
.to_uppercase();
let property_name = property_name
.replace('-', "_")
.replace('.', "_")
.replace(':', "_")
.to_uppercase();
Expand Down
Loading