This is an automated email from the ASF dual-hosted git repository. apitrou pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push: new 145c4d35b2 GH-40731: [C++][Parquet] Minor enhancement code of encryption (#40732) 145c4d35b2 is described below commit 145c4d35b273fd1a6cfa7c7adad23e6c02ae9a61 Author: mwish <maplewish...@gmail.com> AuthorDate: Tue Mar 26 17:28:04 2024 +0800 GH-40731: [C++][Parquet] Minor enhancement code of encryption (#40732) ### Rationale for this change * Applying more `std::move` in encryption * Trying to use `NULLPTR` rather than `NULL`. ### What changes are included in this PR? Applying more `std::move` in encryption ### Are these changes tested? No need ### Are there any user-facing changes? No * GitHub Issue: #40731 Authored-by: mwish <maplewish...@gmail.com> Signed-off-by: Antoine Pitrou <anto...@python.org> --- cpp/src/parquet/encryption/crypto_factory.cc | 2 +- cpp/src/parquet/encryption/file_key_unwrapper.cc | 14 +++++++++----- cpp/src/parquet/encryption/file_key_unwrapper.h | 4 +++- cpp/src/parquet/encryption/file_key_wrapper.cc | 7 ++++--- cpp/src/parquet/encryption/key_management_test.cc | 2 +- cpp/src/parquet/encryption/key_toolkit.cc | 2 +- cpp/src/parquet/encryption/key_toolkit.h | 4 ++-- cpp/src/parquet/encryption/kms_client.cc | 2 +- cpp/src/parquet/encryption/kms_client.h | 2 +- 9 files changed, 23 insertions(+), 16 deletions(-) diff --git a/cpp/src/parquet/encryption/crypto_factory.cc b/cpp/src/parquet/encryption/crypto_factory.cc index 2cf15b5680..72506bdc01 100644 --- a/cpp/src/parquet/encryption/crypto_factory.cc +++ b/cpp/src/parquet/encryption/crypto_factory.cc @@ -30,7 +30,7 @@ namespace parquet::encryption { void CryptoFactory::RegisterKmsClientFactory( std::shared_ptr<KmsClientFactory> kms_client_factory) { - key_toolkit_->RegisterKmsClientFactory(kms_client_factory); + key_toolkit_->RegisterKmsClientFactory(std::move(kms_client_factory)); } std::shared_ptr<FileEncryptionProperties> CryptoFactory::GetFileEncryptionProperties( diff --git a/cpp/src/parquet/encryption/file_key_unwrapper.cc b/cpp/src/parquet/encryption/file_key_unwrapper.cc index 9e430de853..02bea127fd 100644 --- a/cpp/src/parquet/encryption/file_key_unwrapper.cc +++ b/cpp/src/parquet/encryption/file_key_unwrapper.cc @@ -30,33 +30,37 @@ FileKeyUnwrapper::FileKeyUnwrapper( const std::string& file_path, const std::shared_ptr<::arrow::fs::FileSystem>& file_system) : FileKeyUnwrapper(std::move(key_toolkit), /*key_toolkit=*/nullptr, - kms_connection_config, cache_lifetime_seconds, file_path, - file_system) {} + kms_connection_config, cache_lifetime_seconds, + /*key_material_store=*/nullptr, file_path, file_system) {} FileKeyUnwrapper::FileKeyUnwrapper( KeyToolkit* key_toolkit, const KmsConnectionConfig& kms_connection_config, double cache_lifetime_seconds, const std::string& file_path, const std::shared_ptr<::arrow::fs::FileSystem>& file_system) : FileKeyUnwrapper(/*key_toolkit_owner=*/nullptr, key_toolkit, kms_connection_config, - cache_lifetime_seconds, file_path, file_system) {} + cache_lifetime_seconds, /*key_material_store=*/nullptr, file_path, + file_system) {} FileKeyUnwrapper::FileKeyUnwrapper( KeyToolkit* key_toolkit, const KmsConnectionConfig& kms_connection_config, double cache_lifetime_seconds, std::shared_ptr<FileKeyMaterialStore> key_material_store) : FileKeyUnwrapper(/*key_toolkit_owner=*/nullptr, key_toolkit, kms_connection_config, - cache_lifetime_seconds, /*file_path=*/"", + cache_lifetime_seconds, std::move(key_material_store), + /*file_path=*/"", /*file_system=*/nullptr) {} FileKeyUnwrapper::FileKeyUnwrapper( std::shared_ptr<KeyToolkit> key_toolkit_owner, KeyToolkit* key_toolkit, const KmsConnectionConfig& kms_connection_config, double cache_lifetime_seconds, + std::shared_ptr<FileKeyMaterialStore> key_material_store, const std::string& file_path, const std::shared_ptr<::arrow::fs::FileSystem>& file_system) : key_toolkit_owner_(std::move(key_toolkit_owner)), key_toolkit_(key_toolkit ? key_toolkit : key_toolkit_owner_.get()), kms_connection_config_(kms_connection_config), cache_entry_lifetime_seconds_(cache_lifetime_seconds), + key_material_store_(std::move(key_material_store)), file_path_(file_path), file_system_(file_system) { kek_per_kek_id_ = key_toolkit_->kek_read_cache_per_token().GetOrCreateInternalCache( @@ -120,7 +124,7 @@ KeyWithMasterId FileKeyUnwrapper::GetDataEncryptionKey(const KeyMaterial& key_ma data_key = internal::DecryptKeyLocally(encoded_wrapped_dek, kek_bytes, aad); } - return KeyWithMasterId(data_key, master_key_id); + return KeyWithMasterId(std::move(data_key), std::move(master_key_id)); } std::shared_ptr<KmsClient> FileKeyUnwrapper::GetKmsClientFromConfigOrKeyMaterial( diff --git a/cpp/src/parquet/encryption/file_key_unwrapper.h b/cpp/src/parquet/encryption/file_key_unwrapper.h index d05c3efd17..c60c0c71ba 100644 --- a/cpp/src/parquet/encryption/file_key_unwrapper.h +++ b/cpp/src/parquet/encryption/file_key_unwrapper.h @@ -73,7 +73,9 @@ class PARQUET_EXPORT FileKeyUnwrapper : public DecryptionKeyRetriever { private: FileKeyUnwrapper(std::shared_ptr<KeyToolkit> key_toolkit_owner, KeyToolkit* key_toolkit, const KmsConnectionConfig& kms_connection_config, - double cache_lifetime_seconds, const std::string& file_path, + double cache_lifetime_seconds, + std::shared_ptr<FileKeyMaterialStore> key_material_store, + const std::string& file_path, const std::shared_ptr<::arrow::fs::FileSystem>& file_system); std::shared_ptr<KmsClient> GetKmsClientFromConfigOrKeyMaterial( diff --git a/cpp/src/parquet/encryption/file_key_wrapper.cc b/cpp/src/parquet/encryption/file_key_wrapper.cc index 704651ebaa..032ae45821 100644 --- a/cpp/src/parquet/encryption/file_key_wrapper.cc +++ b/cpp/src/parquet/encryption/file_key_wrapper.cc @@ -53,7 +53,7 @@ std::string FileKeyWrapper::GetEncryptionKeyMetadata(const std::string& data_key const std::string& master_key_id, bool is_footer_key, std::string key_id_in_file) { - if (kms_client_ == NULL) { + if (kms_client_ == NULLPTR) { throw ParquetException("No KMS client available. See previous errors."); } @@ -103,7 +103,7 @@ std::string FileKeyWrapper::GetEncryptionKeyMetadata(const std::string& data_key key_counter_++; } } - key_material_store_->AddKeyMaterial(key_id_in_file, serialized_key_material); + key_material_store_->AddKeyMaterial(key_id_in_file, std::move(serialized_key_material)); std::string serialized_key_metadata = KeyMetadata::CreateSerializedForExternalMaterial(key_id_in_file); return serialized_key_metadata; @@ -120,7 +120,8 @@ KeyEncryptionKey FileKeyWrapper::CreateKeyEncryptionKey( // Encrypt KEK with Master key std::string encoded_wrapped_kek = kms_client_->WrapKey(kek_bytes, master_key_id); - return KeyEncryptionKey(kek_bytes, kek_id, encoded_wrapped_kek); + return KeyEncryptionKey(std::move(kek_bytes), std::move(kek_id), + std::move(encoded_wrapped_kek)); } } // namespace parquet::encryption diff --git a/cpp/src/parquet/encryption/key_management_test.cc b/cpp/src/parquet/encryption/key_management_test.cc index 2ae8ed96b6..1506a00a14 100644 --- a/cpp/src/parquet/encryption/key_management_test.cc +++ b/cpp/src/parquet/encryption/key_management_test.cc @@ -69,7 +69,7 @@ class TestEncryptionKeyManagement : public ::testing::Test { wrap_locally_ = wrap_locally; std::shared_ptr<KmsClientFactory> kms_client_factory = std::make_shared<TestOnlyInMemoryKmsClientFactory>(wrap_locally, key_list_); - crypto_factory_.RegisterKmsClientFactory(kms_client_factory); + crypto_factory_.RegisterKmsClientFactory(std::move(kms_client_factory)); } std::string GetFileName(bool double_wrapping, bool wrap_locally, diff --git a/cpp/src/parquet/encryption/key_toolkit.cc b/cpp/src/parquet/encryption/key_toolkit.cc index cb488d3fa2..81e102126d 100644 --- a/cpp/src/parquet/encryption/key_toolkit.cc +++ b/cpp/src/parquet/encryption/key_toolkit.cc @@ -31,7 +31,7 @@ namespace parquet::encryption { std::shared_ptr<KmsClient> KeyToolkit::GetKmsClient( const KmsConnectionConfig& kms_connection_config, double cache_entry_lifetime_ms) { - if (kms_client_factory_ == NULL) { + if (kms_client_factory_ == nullptr) { throw ParquetException("No KmsClientFactory is registered."); } auto kms_client_per_kms_instance_cache = diff --git a/cpp/src/parquet/encryption/key_toolkit.h b/cpp/src/parquet/encryption/key_toolkit.h index f63ade4c8c..339692a99a 100644 --- a/cpp/src/parquet/encryption/key_toolkit.h +++ b/cpp/src/parquet/encryption/key_toolkit.h @@ -62,10 +62,10 @@ class PARQUET_EXPORT KeyToolkit { void RemoveCacheEntriesForAllTokens(); void RegisterKmsClientFactory(std::shared_ptr<KmsClientFactory> kms_client_factory) { - if (kms_client_factory_ != NULL) { + if (kms_client_factory_ != NULLPTR) { throw ParquetException("KMS client factory has already been registered."); } - kms_client_factory_ = kms_client_factory; + kms_client_factory_ = std::move(kms_client_factory); } /// Key rotation. In the single wrapping mode, decrypts data keys with old master keys, diff --git a/cpp/src/parquet/encryption/kms_client.cc b/cpp/src/parquet/encryption/kms_client.cc index fee03dd3db..0ce41da177 100644 --- a/cpp/src/parquet/encryption/kms_client.cc +++ b/cpp/src/parquet/encryption/kms_client.cc @@ -34,7 +34,7 @@ void KmsConnectionConfig::SetDefaultIfEmpty() { if (kms_instance_url.empty()) { kms_instance_url = KmsClient::kKmsInstanceUrlDefault; } - if (refreshable_key_access_token == NULL) { + if (refreshable_key_access_token == nullptr) { refreshable_key_access_token = std::make_shared<KeyAccessToken>(); } } diff --git a/cpp/src/parquet/encryption/kms_client.h b/cpp/src/parquet/encryption/kms_client.h index a55fd552ee..ef363d9c2c 100644 --- a/cpp/src/parquet/encryption/kms_client.h +++ b/cpp/src/parquet/encryption/kms_client.h @@ -63,7 +63,7 @@ struct PARQUET_EXPORT KmsConnectionConfig { KmsConnectionConfig(); const std::string& key_access_token() const { - if (refreshable_key_access_token == NULL || + if (refreshable_key_access_token == NULLPTR || refreshable_key_access_token->value().empty()) { throw ParquetException("key access token is not set!"); }