Skip to content

fix(catalog/s3tables): stop dropping endpoint_url for empty properties #2623

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

Open
Kurtiscwright wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −4
Open

fix(catalog/s3tables): stop dropping endpoint_url for empty properties #2623

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
21 changes: 17 additions & 4 deletions crates/catalog/s3tables/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,6 @@ pub(crate) async fn create_sdk_config(
) -> SdkConfig {
let mut config = aws_config::defaults(BehaviorVersion::latest());

if properties.is_empty() {
return config.load().await;
}

@CTTY CTTY Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we just move this block to after the endpoint_url check?

@Kurtiscwright Kurtiscwright Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I considered that approach, but I chose the current implementation for two reasons:

  1. It read like a premature optimization to me to have this check. This validation is only ran at Catalog load time so once per session and the number of checks are only 4 at the moment.
  2. Keeping this check leaves this same class of bug for any future property flag implementations.

For both of the above reasons. I felt it was better to remove the check altogether rather than shuffle it further down. I can add it back in if there is strong disagreement with my two reasons listed above.

if let Some(endpoint_url) = endpoint_url {
config = config.endpoint_url(endpoint_url);
}
Expand All @@ -69,3 +65,20 @@ pub(crate) async fn create_sdk_config(

config.load().await
}

#[cfg(test)]
mod tests {
use super::*;

#[tokio::test]
async fn test_config_with_custom_endpoint() {
let properties = HashMap::new();
let endpoint_url = "http://localhost:5001";

let sdk_config = create_sdk_config(&properties, Some(endpoint_url.to_string())).await;

let result = sdk_config.endpoint_url().unwrap();

assert_eq!(result, endpoint_url);
}
}
Loading