Skip to content

Commit

Permalink
Revert AzureOptions constructor. Just make account_name a public member.
Browse files Browse the repository at this point in the history
  • Loading branch information
Tom-Newton committed Dec 22, 2023
1 parent de0a869 commit 8cdcf4b
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 20 deletions.
23 changes: 12 additions & 11 deletions cpp/src/arrow/filesystem/azurefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ using HNSSupport = internal::HierarchicalNamespaceSupport;
// -----------------------------------------------------------------------
// AzureOptions Implementation

// AzureOptions::AzureOptions(const std::string& account_name) = default;
AzureOptions::AzureOptions(const std::string& account_name)
: account_name_(account_name) {}
AzureOptions::AzureOptions() = default;

AzureOptions::~AzureOptions() = default;

Expand All @@ -60,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
blob_storage_scheme == other.blob_storage_scheme &&
dfs_storage_scheme == other.dfs_storage_scheme &&
default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
credential_kind_ == other.credential_kind_;
if (!equals) {
return false;
Expand Down Expand Up @@ -108,8 +106,11 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
if (account_name.empty()) {
return Status::Invalid("AzureOptions doesn't contain a valid account name");
}
storage_shared_key_credential_ =
std::make_shared<Storage::StorageSharedKeyCredential>(account_name_, account_key);
std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
return Status::OK();
}

Expand Down Expand Up @@ -143,36 +144,36 @@ Status AzureOptions::ConfigureWorkloadIdentityCredential() {

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
const {
if (account_name_.empty()) {
if (account_name.empty()) {
return Status::Invalid("AzureOptions doesn't contain a valid account name");
}
switch (credential_kind_) {
case CredentialKind::kAnonymous:
break;
case CredentialKind::kTokenCredential:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
token_credential_);
case CredentialKind::kStorageSharedKeyCredential:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
storage_shared_key_credential_);
}
return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
if (account_name_.empty()) {
if (account_name.empty()) {
return Status::Invalid("AzureOptions doesn't contain a valid account name");
}
switch (credential_kind_) {
case CredentialKind::kAnonymous:
break;
case CredentialKind::kTokenCredential:
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name_), token_credential_);
AccountDfsUrl(account_name), token_credential_);
case CredentialKind::kStorageSharedKeyCredential:
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name_), storage_shared_key_credential_);
AccountDfsUrl(account_name), storage_shared_key_credential_);
}
return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down
6 changes: 4 additions & 2 deletions cpp/src/arrow/filesystem/azurefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
/// \brief account name of the Azure Storage account.
std::string account_name;

/// \brief hostname[:port] of the Azure Blob Storage Service.
///
/// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand Down Expand Up @@ -94,13 +97,12 @@ struct ARROW_EXPORT AzureOptions {
kStorageSharedKeyCredential,
} credential_kind_ = CredentialKind::kAnonymous;

std::string account_name_;
std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;

public:
AzureOptions(const std::string& account_name);
AzureOptions();
~AzureOptions();

Status ConfigureDefaultCredential();
Expand Down
19 changes: 12 additions & 7 deletions cpp/src/arrow/filesystem/azurefs_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -272,20 +272,23 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
};

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
AzureOptions options("dummy-account-name");
AzureOptions options;
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(
options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
AzureOptions options("dummy-account-name");
AzureOptions options;
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
AzureOptions options("dummy-account-name");
AzureOptions options;
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

Expand All @@ -294,13 +297,14 @@ TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
AzureOptions options("dummy-account-name");
AzureOptions options;
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, OptionsCompare) {
AzureOptions options("account-name");
AzureOptions options;
EXPECT_TRUE(options.Equals(options));
}

Expand Down Expand Up @@ -374,7 +378,7 @@ class TestAzureFileSystem : public ::testing::Test {
std::unique_ptr<DataLake::DataLakeServiceClient> datalake_service_client_;

public:
TestAzureFileSystem() : rng_(std::random_device()()), options_("temp") {}
TestAzureFileSystem() : rng_(std::random_device()()) {}

virtual Result<BaseAzureEnv*> GetAzureEnv() const = 0;
virtual HNSSupport CachedHNSSupport(const BaseAzureEnv& env) const = 0;
Expand All @@ -391,7 +395,8 @@ class TestAzureFileSystem : public ::testing::Test {
}

static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
AzureOptions options(env->account_name());
AzureOptions options;
options.account_name = env->account_name();
switch (env->backend()) {
case AzureBackend::kAzurite:
options.blob_storage_authority = "127.0.0.1:10000";
Expand Down

0 comments on commit 8cdcf4b

Please sign in to comment.