Skip to content

Commit d9aa99a

Browse files
raulcdraulcd
committed
Add s3 fs module test for FromUriAndOptionsCredentials and refactor S3Options::FromUri and S3Options::FromUriAndOptions
1 parent c3e281e commit d9aa99a

2 files changed

Lines changed: 83 additions & 64 deletions

File tree

‎cpp/src/arrow/filesystem/s3fs.cc‎

Lines changed: 65 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,48 @@ S3Options S3Options::FromAssumeRoleWithWebIdentity() {
327327
}
328328

329329
Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
330-
S3Options options;
330+
return FromUriAndOptions(uri, FileSystemFactoryOptions{}, out_path);
331+
}
332+
333+
Result<S3Options> S3Options::FromUri(const std::string& uri_string,
334+
std::string* out_path) {
335+
Uri uri;
336+
RETURN_NOT_OK(uri.Parse(uri_string));
337+
return FromUri(uri, out_path);
338+
}
339+
340+
namespace {
341+
342+
Result<std::string> GetStringOption(const std::string& key, const std::any& value) {
343+
// TODO: Validate this is necessary with tests.
344+
if (const auto* s = std::any_cast<std::string>(&value)) {
345+
return *s;
346+
}
347+
return Status::Invalid("S3 filesystem option '", key, "' must be a std::string");
348+
}
349+
} // namespace
350+
351+
Result<S3Options> S3Options::FromUriAndOptions(const ::arrow::util::Uri& uri,
352+
const FileSystemFactoryOptions& options,
353+
std::string* out_path) {
354+
std::optional<std::string> access_key, secret_key, session_token;
355+
for (const auto& [key, value] : options) {
356+
if (key == "access_key") {
357+
ARROW_ASSIGN_OR_RAISE(access_key, GetStringOption(key, value));
358+
} else if (key == "secret_key") {
359+
ARROW_ASSIGN_OR_RAISE(secret_key, GetStringOption(key, value));
360+
} else if (key == "session_token") {
361+
ARROW_ASSIGN_OR_RAISE(session_token, GetStringOption(key, value));
362+
} else {
363+
return Status::Invalid("Unexpected option for S3 filesystem: '", key, "'");
364+
}
365+
}
366+
367+
if (access_key.has_value() != secret_key.has_value()) {
368+
return Status::Invalid(
369+
"Both 'access_key' and 'secret_key' must be provided together");
370+
}
371+
S3Options s3_options;
331372

332373
const auto bucket = uri.host();
333374
auto path = uri.path();
@@ -355,105 +396,65 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
355396
options_map.emplace(kv.first, kv.second);
356397
}
357398

358-
const auto username = uri.username();
359-
if (!username.empty()) {
360-
options.ConfigureAccessKey(username, uri.password());
399+
if (access_key.has_value()) {
400+
s3_options.ConfigureAccessKey(*access_key, *secret_key, session_token.value_or(""));
361401
} else {
362-
options.ConfigureDefaultCredentials();
402+
const auto username = uri.username();
403+
if (!username.empty()) {
404+
s3_options.ConfigureAccessKey(username, uri.password());
405+
} else {
406+
s3_options.ConfigureDefaultCredentials();
407+
}
363408
}
409+
364410
// Prefer AWS service-specific endpoint url
365411
auto s3_endpoint_env = arrow::internal::GetEnvVar(kAwsEndpointUrlS3EnvVar);
366412
if (s3_endpoint_env.ok()) {
367-
options.endpoint_override = *s3_endpoint_env;
413+
s3_options.endpoint_override = *s3_endpoint_env;
368414
} else {
369415
auto endpoint_env = arrow::internal::GetEnvVar(kAwsEndpointUrlEnvVar);
370416
if (endpoint_env.ok()) {
371-
options.endpoint_override = *endpoint_env;
417+
s3_options.endpoint_override = *endpoint_env;
372418
}
373419
}
374420

375421
bool region_set = false;
376422
for (const auto& kv : options_map) {
377423
if (kv.first == "region") {
378-
options.region = kv.second;
424+
s3_options.region = kv.second;
379425
region_set = true;
380426
} else if (kv.first == "scheme") {
381-
options.scheme = kv.second;
427+
s3_options.scheme = kv.second;
382428
} else if (kv.first == "endpoint_override") {
383-
options.endpoint_override = kv.second;
429+
s3_options.endpoint_override = kv.second;
384430
} else if (kv.first == "allow_delayed_open") {
385-
ARROW_ASSIGN_OR_RAISE(options.allow_delayed_open,
431+
ARROW_ASSIGN_OR_RAISE(s3_options.allow_delayed_open,
386432
::arrow::internal::ParseBoolean(kv.second));
387433
} else if (kv.first == "allow_bucket_creation") {
388-
ARROW_ASSIGN_OR_RAISE(options.allow_bucket_creation,
434+
ARROW_ASSIGN_OR_RAISE(s3_options.allow_bucket_creation,
389435
::arrow::internal::ParseBoolean(kv.second));
390436
} else if (kv.first == "allow_bucket_deletion") {
391-
ARROW_ASSIGN_OR_RAISE(options.allow_bucket_deletion,
437+
ARROW_ASSIGN_OR_RAISE(s3_options.allow_bucket_deletion,
392438
::arrow::internal::ParseBoolean(kv.second));
393439
} else if (kv.first == "tls_ca_file_path") {
394-
options.tls_ca_file_path = kv.second;
440+
s3_options.tls_ca_file_path = kv.second;
395441
} else if (kv.first == "tls_ca_dir_path") {
396-
options.tls_ca_dir_path = kv.second;
442+
s3_options.tls_ca_dir_path = kv.second;
397443
} else if (kv.first == "tls_verify_certificates") {
398-
ARROW_ASSIGN_OR_RAISE(options.tls_verify_certificates,
444+
ARROW_ASSIGN_OR_RAISE(s3_options.tls_verify_certificates,
399445
::arrow::internal::ParseBoolean(kv.second));
400446
} else if (kv.first == "smart_defaults") {
401-
options.smart_defaults = kv.second;
447+
s3_options.smart_defaults = kv.second;
402448
} else {
403449
return Status::Invalid("Unexpected query parameter in S3 URI: '", kv.first, "'");
404450
}
405451
}
406452

407-
if (!region_set && !bucket.empty() && options.endpoint_override.empty()) {
453+
if (!region_set && !bucket.empty() && s3_options.endpoint_override.empty()) {
408454
// XXX Should we use a dedicated resolver with the given credentials?
409-
ARROW_ASSIGN_OR_RAISE(options.region, ResolveS3BucketRegion(bucket));
455+
ARROW_ASSIGN_OR_RAISE(s3_options.region, ResolveS3BucketRegion(bucket));
410456
}
411457

412-
return options;
413-
}
414-
415-
Result<S3Options> S3Options::FromUri(const std::string& uri_string,
416-
std::string* out_path) {
417-
Uri uri;
418-
RETURN_NOT_OK(uri.Parse(uri_string));
419-
return FromUri(uri, out_path);
420-
}
421-
422-
namespace {
423-
424-
Result<std::string> GetStringOption(const std::string& key, const std::any& value) {
425-
// TODO: Validate this is necessary with tests.
426-
if (const auto* s = std::any_cast<std::string>(&value)) {
427-
return *s;
428-
}
429-
return Status::Invalid("S3 filesystem option '", key, "' must be a std::string");
430-
}
431-
} // namespace
432-
433-
Result<S3Options> S3Options::FromUriAndOptions(const ::arrow::util::Uri& uri,
434-
const FileSystemFactoryOptions& options,
435-
std::string* out_path) {
436-
std::optional<std::string> access_key, secret_key, session_token;
437-
for (const auto& [key, value] : options) {
438-
if (key == "access_key") {
439-
ARROW_ASSIGN_OR_RAISE(access_key, GetStringOption(key, value));
440-
} else if (key == "secret_key") {
441-
ARROW_ASSIGN_OR_RAISE(secret_key, GetStringOption(key, value));
442-
} else if (key == "session_token") {
443-
ARROW_ASSIGN_OR_RAISE(session_token, GetStringOption(key, value));
444-
} else {
445-
return Status::Invalid("Unexpected option for S3 filesystem: '", key, "'");
446-
}
447-
}
448-
449-
if (access_key.has_value() != secret_key.has_value()) {
450-
return Status::Invalid(
451-
"Both 'access_key' and 'secret_key' must be provided together");
452-
}
453-
ARROW_ASSIGN_OR_RAISE(auto s3_options, FromUri(uri, out_path));
454-
if (access_key.has_value()) {
455-
s3_options.ConfigureAccessKey(*access_key, *secret_key, session_token.value_or(""));
456-
}
457458
return s3_options;
458459
}
459460

‎cpp/src/arrow/filesystem/s3fs_module_test.cc‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,24 @@ TEST(S3Test, FromUri) {
8383
"&allow_bucket_creation=0&allow_bucket_deletion=0");
8484
}
8585

86+
TEST(S3Test, FromUriAndOptionsCredentials) {
87+
ASSERT_OK_AND_ASSIGN(auto minio, GetMinioEnv()->GetOneServer());
88+
std::string path;
89+
FileSystemFactoryOptions options{
90+
{"access_key", std::string(minio->access_key())},
91+
{"secret_key", std::string(minio->secret_key())},
92+
};
93+
// Credentials supplied via options, NOT in the URI.
94+
ASSERT_OK_AND_ASSIGN(
95+
auto fs,
96+
FileSystemFromUriAndOptions("s3://bucket/somedir/subdir/subfile", options, &path));
97+
// They crossed the module boundary and were applied -> reflected in MakeUri.
98+
EXPECT_EQ(fs->MakeUri("/" + path),
99+
"s3://minio:miniopass@bucket/somedir/subdir/subfile"
100+
"?region=us-east-1&scheme=https&endpoint_override="
101+
"&allow_bucket_creation=0&allow_bucket_deletion=0");
102+
}
103+
86104
TEST(S3Test, FromUriRejectsUnknownOptions) {
87105
FileSystemFactoryOptions options{{"some_option", 1}};
88106
EXPECT_RAISES_WITH_MESSAGE_THAT(

0 commit comments

Comments
 (0)