Copilot commented on code in PR #50128:
URL: https://github.com/apache/arrow/pull/50128#discussion_r3382121578
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -419,6 +419,52 @@ Result<S3Options> S3Options::FromUri(const std::string&
uri_string,
return FromUri(uri, out_path);
}
+namespace {
+
+Result<std::string> GetStringOption(const std::string& key, const std::any&
value) {
+ // TODO: Validate this is necessary with tests.
+ if (const auto* s = std::any_cast<std::string>(&value)) {
+ return *s;
+ }
+ return Status::Invalid("S3 filesystem option '", key, "' must be a
std::string");
+}
+} // namespace
+
+Result<S3Options> S3Options::FromUriAndOptions(const ::arrow::util::Uri& uri,
+ const FileSystemFactoryOptions&
options,
+ std::string* out_path) {
+ std::optional<std::string> access_key, secret_key, session_token;
+ for (const auto& [key, value] : options) {
+ if (key == "access_key") {
+ ARROW_ASSIGN_OR_RAISE(access_key, GetStringOption(key, value));
+ } else if (key == "secret_key") {
+ ARROW_ASSIGN_OR_RAISE(secret_key, GetStringOption(key, value));
+ } else if (key == "session_token") {
+ ARROW_ASSIGN_OR_RAISE(session_token, GetStringOption(key, value));
+ } else {
+ return Status::Invalid("Unexpected option for S3 filesystem: '", key,
"'");
+ }
+ }
+
+ if (access_key.has_value() != secret_key.has_value()) {
+ return Status::Invalid(
+ "Both 'access_key' and 'secret_key' must be provided together");
+ }
+ ARROW_ASSIGN_OR_RAISE(auto s3_options, FromUri(uri, out_path));
+ if (access_key.has_value()) {
+ s3_options.ConfigureAccessKey(*access_key, *secret_key,
session_token.value_or(""));
+ }
+ return s3_options;
Review Comment:
`session_token` provided via `FileSystemFactoryOptions` is currently ignored
unless `access_key` is also provided in options. This makes `session_token` a
no-op when credentials come from the URI (e.g. `s3://AK:SK@...`) and can also
silently drop the option when using default credentials. Consider applying
`session_token` when the parsed URI results in explicit credentials, or
returning Invalid when it can't be applied, to avoid surprising behavior.
##########
cpp/src/arrow/filesystem/s3fs_module_test.cc:
##########
@@ -82,4 +83,11 @@ TEST(S3Test, FromUri) {
"&allow_bucket_creation=0&allow_bucket_deletion=0");
}
+TEST(S3Test, FromUriRejectsUnknownOptions) {
+ FileSystemFactoryOptions options{{"some_option", 1}};
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, ::testing::HasSubstr("Unexpected option"),
+ FileSystemFromUriAndOptions("s3://bucket/key", options));
+}
Review Comment:
New `FileSystemFromUriAndOptions` behavior for S3 is only tested for the
negative case (unknown option). To prevent regressions, add a positive-path
test that verifies valid keys (e.g. `access_key`/`secret_key`, and optionally
`session_token`) are applied and influence the created filesystem (for example
by asserting `fs->MakeUri()` contains the provided credentials or by performing
a simple operation against the Minio test server).
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -3603,11 +3649,18 @@ Result<std::string> ResolveS3BucketRegion(const
std::string& bucket) {
auto kS3FileSystemModule = ARROW_REGISTER_FILESYSTEM(
"s3",
- [](const arrow::util::Uri& uri, const io::IOContext& io_context,
+ [](const arrow::util::Uri& uri, const FileSystemFactoryOptions& options,
+ const io::IOContext& io_context,
std::string* out_path) -> Result<std::shared_ptr<fs::FileSystem>> {
+ /*if (!options.empty()) {
+ return Status::NotImplemented(
+ "S3 filesystem factory options are not supported yet, got: ",
options.size(),
+ " option(s)");
+ }*/
Review Comment:
Leftover commented-out code block rejecting options should be removed now
that the factory consumes `options` via `S3Options::FromUriAndOptions`. Keeping
this block makes the factory logic harder to read and can confuse future
maintenance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
