From 82df0a2e8331270675404d6ca7c9869f43ee4305 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 25 Sep 2024 09:55:53 -0400 Subject: [PATCH] Respond to PR comments --- include/acquire.zarr.h | 8 +- src/logger/logger.cpp | 4 + src/streaming/CMakeLists.txt | 7 +- src/streaming/acquire.zarr.cpp | 60 +++++++++++--- src/streaming/zarr.stream.cpp | 136 +++++++++++++++++--------------- src/streaming/zarr.stream.hh | 48 +++++------ tests/unit-tests/CMakeLists.txt | 2 +- 7 files changed, 159 insertions(+), 106 deletions(-) diff --git a/include/acquire.zarr.h b/include/acquire.zarr.h index 54a7288a..d74a587f 100644 --- a/include/acquire.zarr.h +++ b/include/acquire.zarr.h @@ -39,9 +39,9 @@ extern "C" /** * @brief Get the version of the Zarr API. - * @return The version of the Zarr API. + * @return Semver formatted version of the Zarr API. */ - uint32_t Zarr_get_api_version(); + const char* Zarr_get_api_version(); /** * @brief Set the log level for the Zarr API. @@ -58,10 +58,10 @@ extern "C" /** * @brief Get the message for the given status code. - * @param status The status code. + * @param code The status code. * @return A human-readable status message. */ - const char* Zarr_get_status_message(ZarrStatusCode status); + const char* Zarr_get_status_message(ZarrStatusCode code); /** * @brief Allocate memory for the dimension array in the Zarr stream settings struct. diff --git a/src/logger/logger.cpp b/src/logger/logger.cpp index 77e92435..c7af5582 100644 --- a/src/logger/logger.cpp +++ b/src/logger/logger.cpp @@ -10,6 +10,10 @@ std::mutex Logger::log_mutex_{}; void Logger::set_log_level(LogLevel level) { + if (level < LogLevel_Debug || level > LogLevel_Error) { + throw std::invalid_argument("Invalid log level"); + } + current_level_ = level; } diff --git a/src/streaming/CMakeLists.txt b/src/streaming/CMakeLists.txt index 4085b738..480e3df3 100644 --- a/src/streaming/CMakeLists.txt +++ b/src/streaming/CMakeLists.txt @@ -16,11 +16,15 @@ target_include_directories(${tgt} ) target_link_libraries(${tgt} PRIVATE - acquire-zarr-logger + acquire-logger blosc_static miniocpp::miniocpp ) +target_compile_definitions(${tgt} PRIVATE + "ACQUIRE_ZARR_API_VERSION=\"0.0.1\"" +) + set_target_properties(${tgt} PROPERTIES MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>" ) @@ -33,5 +37,4 @@ install(TARGETS ${tgt} # Install public header files install(DIRECTORY ${CMAKE_SOURCE_DIR}/include/ DESTINATION include - FILES_MATCHING PATTERN "*.h" ) \ No newline at end of file diff --git a/src/streaming/acquire.zarr.cpp b/src/streaming/acquire.zarr.cpp index 061ae71e..2804bf0c 100644 --- a/src/streaming/acquire.zarr.cpp +++ b/src/streaming/acquire.zarr.cpp @@ -4,32 +4,65 @@ #include // uint32_t -#define ACQUIRE_ZARR_API_VERSION 0 - extern "C" { - uint32_t Zarr_get_api_version() + const char* Zarr_get_api_version() { return ACQUIRE_ZARR_API_VERSION; } - ZarrStatusCode Zarr_set_log_level(ZarrLogLevel level) + ZarrStatusCode Zarr_set_log_level(ZarrLogLevel level_) { - EXPECT_VALID_ARGUMENT( - level < ZarrLogLevelCount, "Invalid log level: %d", level); + LogLevel level; + switch (level_) { + case ZarrLogLevel_Debug: + level = LogLevel_Debug; + break; + case ZarrLogLevel_Info: + level = LogLevel_Info; + break; + case ZarrLogLevel_Warning: + level = LogLevel_Warning; + break; + case ZarrLogLevel_Error: + level = LogLevel_Error; + break; + default: + return ZarrStatusCode_InvalidArgument; + } - Logger::set_log_level(level); + try { + Logger::set_log_level(level); + } catch (const std::exception& e) { + LOG_ERROR("Error setting log level: %s", e.what()); + return ZarrStatusCode_InternalError; + } return ZarrStatusCode_Success; } ZarrLogLevel Zarr_get_log_level() { - return Logger::get_log_level(); + ZarrLogLevel level; + switch (Logger::get_log_level()) { + case LogLevel_Debug: + level = ZarrLogLevel_Debug; + break; + case LogLevel_Info: + level = ZarrLogLevel_Info; + break; + case LogLevel_Warning: + level = ZarrLogLevel_Warning; + break; + default: + level = ZarrLogLevel_Error; + break; + } + return level; } - const char* Zarr_get_status_message(ZarrStatusCode error) + const char* Zarr_get_status_message(ZarrStatusCode code) { - switch (error) { + switch (code) { case ZarrStatusCode_Success: return "Success"; case ZarrStatusCode_InvalidArgument: @@ -83,10 +116,15 @@ extern "C" void ZarrStreamSettings_destroy_dimension_array( struct ZarrStreamSettings_s* settings) { - if (nullptr != settings && nullptr != settings->dimensions) { + if (settings == nullptr) { + return; + } + + if (settings->dimensions != nullptr) { delete[] settings->dimensions; settings->dimensions = nullptr; } + settings->dimension_count = 0; } ZarrStream_s* ZarrStream_create(struct ZarrStreamSettings_s* settings) diff --git a/src/streaming/zarr.stream.cpp b/src/streaming/zarr.stream.cpp index e1eb3b74..51c3fdee 100644 --- a/src/streaming/zarr.stream.cpp +++ b/src/streaming/zarr.stream.cpp @@ -25,7 +25,7 @@ is_compressed_acquisition(const struct ZarrStreamSettings_s* settings) std::string trim(const char* s) { - if (!s || !*s) { + if (s == nullptr || *s == '\0') { return {}; } @@ -48,33 +48,38 @@ trim(const char* s) return trimmed; } +bool +is_empty_string(const char* s, std::string_view error_msg) +{ + auto trimmed = trim(s); + if (trimmed.empty()) { + LOG_ERROR(error_msg); + return true; + } + return false; +} + [[nodiscard]] bool validate_s3_settings(const ZarrS3Settings* settings) { - std::string trimmed = trim(settings->endpoint); - if (trimmed.empty()) { - LOG_ERROR("S3 endpoint is empty"); + if (is_empty_string(settings->endpoint, "S3 endpoint is empty")) { return false; } - - trimmed = trim(settings->bucket_name); - if (trimmed.length() < 3 || trimmed.length() > 63) { - LOG_ERROR("Invalid length for S3 bucket name: %zu. Must be between 3 " - "and 63 characters", - trimmed.length()); + if (is_empty_string(settings->access_key_id, "S3 access key ID is empty")) { return false; } - - trimmed = trim(settings->access_key_id); - if (trimmed.empty()) { - LOG_ERROR("S3 access key ID is empty"); + if (is_empty_string(settings->secret_access_key, + "S3 secret access key is empty")) { return false; } - trimmed = trim(settings->secret_access_key); - if (trimmed.empty()) { - LOG_ERROR("S3 secret access key is empty"); + std::string trimmed = trim(settings->bucket_name); + if (trimmed.length() < 3 || trimmed.length() > 63) { + LOG_ERROR("Invalid length for S3 bucket name: ", + trimmed.length(), + ". Must be between 3 " + "and 63 characters"); return false; } @@ -93,8 +98,9 @@ validate_filesystem_store_path(std::string_view data_root) // parent path must exist and be a directory if (!fs::exists(parent_path) || !fs::is_directory(parent_path)) { - LOG_ERROR("Parent path '%s' does not exist or is not a directory", - parent_path.c_str()); + LOG_ERROR("Parent path '", + parent_path, + "' does not exist or is not a directory"); return false; } @@ -105,7 +111,7 @@ validate_filesystem_store_path(std::string_view data_root) fs::perms::others_write)) != fs::perms::none; if (!is_writable) { - LOG_ERROR("Parent path '%s' is not writable", parent_path.c_str()); + LOG_ERROR("Parent path '", parent_path, "' is not writable"); return false; } @@ -117,12 +123,12 @@ bool validate_compression_settings(const ZarrCompressionSettings* settings) { if (settings->compressor >= ZarrCompressorCount) { - LOG_ERROR("Invalid compressor: %d", settings->compressor); + LOG_ERROR("Invalid compressor: ", settings->compressor); return false; } if (settings->codec >= ZarrCompressionCodecCount) { - LOG_ERROR("Invalid compression codec: %d", settings->codec); + LOG_ERROR("Invalid compression codec: ", settings->codec); return false; } @@ -134,20 +140,24 @@ validate_compression_settings(const ZarrCompressionSettings* settings) } if (settings->level > 9) { - LOG_ERROR("Invalid compression level: %d. Must be between 0 and 9", - settings->level); + LOG_ERROR("Invalid compression level: ", + settings->level, + ". Must be between 0 and 9"); return false; } if (settings->shuffle != BLOSC_NOSHUFFLE && settings->shuffle != BLOSC_SHUFFLE && settings->shuffle != BLOSC_BITSHUFFLE) { - LOG_ERROR("Invalid shuffle: %d. Must be %d (no shuffle), %d (byte " - "shuffle), or %d (bit shuffle)", + LOG_ERROR("Invalid shuffle: ", settings->shuffle, + ". Must be ", BLOSC_NOSHUFFLE, + " (no shuffle), ", BLOSC_SHUFFLE, - BLOSC_BITSHUFFLE); + " (byte shuffle), or ", + BLOSC_BITSHUFFLE, + " (bit shuffle)"); return false; } @@ -158,7 +168,7 @@ validate_compression_settings(const ZarrCompressionSettings* settings) bool validate_custom_metadata(const char* metadata) { - if (nullptr == metadata || !*metadata) { + if (metadata == nullptr || !*metadata) { return true; // custom metadata is optional } @@ -170,7 +180,7 @@ validate_custom_metadata(const char* metadata) ); if (val.is_discarded()) { - LOG_ERROR("Invalid JSON: %s", metadata); + LOG_ERROR("Invalid JSON: ", metadata); return false; } @@ -183,14 +193,12 @@ validate_dimension(const ZarrDimensionProperties* dimension, ZarrVersion version, bool is_append) { - std::string trimmed = trim(dimension->name); - if (trimmed.empty()) { - LOG_ERROR("Invalid name. Must not be empty"); + if (is_empty_string(dimension->name, "Dimension name is empty")) { return false; } if (dimension->type >= ZarrDimensionTypeCount) { - LOG_ERROR("Invalid dimension type: %d", dimension->type); + LOG_ERROR("Invalid dimension type: ", dimension->type); return false; } @@ -200,7 +208,7 @@ validate_dimension(const ZarrDimensionProperties* dimension, } if (dimension->chunk_size_px == 0) { - LOG_ERROR("Invalid chunk size: %zu", dimension->chunk_size_px); + LOG_ERROR("Invalid chunk size: ", dimension->chunk_size_px); return false; } @@ -223,11 +231,11 @@ validate_settings(const struct ZarrStreamSettings_s* settings) auto version = settings->version; if (version < ZarrVersion_2 || version >= ZarrVersionCount) { - LOG_ERROR("Invalid Zarr version: %d", version); + LOG_ERROR("Invalid Zarr version: ", version); return false; } - if (nullptr == settings->store_path) { + if (settings->store_path == nullptr) { LOG_ERROR("Null pointer: store_path"); return false; } @@ -239,16 +247,15 @@ validate_settings(const struct ZarrStreamSettings_s* settings) return false; } - if (is_s3_acquisition(settings) && - !validate_s3_settings(settings->s3_settings)) { - return false; - } else if (!is_s3_acquisition(settings) && - !validate_filesystem_store_path(store_path)) { + if ((is_s3_acquisition(settings) && + !validate_s3_settings(settings->s3_settings)) || + (!is_s3_acquisition(settings) && + !validate_filesystem_store_path(store_path))) { return false; } if (settings->data_type >= ZarrDataTypeCount) { - LOG_ERROR("Invalid data type: %d", settings->data_type); + LOG_ERROR("Invalid data type: ", settings->data_type); return false; } @@ -261,7 +268,7 @@ validate_settings(const struct ZarrStreamSettings_s* settings) return false; } - if (nullptr == settings->dimensions) { + if (settings->dimensions == nullptr) { LOG_ERROR("Null pointer: dimensions"); return false; } @@ -269,8 +276,7 @@ validate_settings(const struct ZarrStreamSettings_s* settings) // we must have at least 3 dimensions const size_t ndims = settings->dimension_count; if (ndims < 3) { - LOG_ERROR("Invalid number of dimensions: %zu. Must be at least 3", - ndims); + LOG_ERROR("Invalid number of dimensions: ", ndims, ". Must be at least 3"); return false; } @@ -309,25 +315,25 @@ ZarrStream::ZarrStream_s(struct ZarrStreamSettings_s* settings) commit_settings_(settings); // create the data store - EXPECT(create_store_(), "%s", error_.c_str()); + EXPECT(create_store_(), error_); // allocate writers - EXPECT(create_writers_(), "%s", error_.c_str()); + EXPECT(create_writers_(), error_); // allocate multiscale frame placeholders create_scaled_frames_(); // allocate metadata sinks - EXPECT(create_metadata_sinks_(), "%s", error_.c_str()); + EXPECT(create_metadata_sinks_(), error_); // write base metadata - EXPECT(write_base_metadata_(), "%s", error_.c_str()); + EXPECT(write_base_metadata_(), error_); // write group metadata - EXPECT(write_group_metadata_(), "%s", error_.c_str()); + EXPECT(write_group_metadata_(), error_); // write external metadata - EXPECT(write_external_metadata_(), "%s", error_.c_str()); + EXPECT(write_external_metadata_(), error_); } ZarrStream_s::~ZarrStream_s() @@ -336,7 +342,7 @@ ZarrStream_s::~ZarrStream_s() // must precede close of chunk file write_group_metadata_(); } catch (const std::exception& e) { - LOG_ERROR("Error finalizing Zarr stream: %s", e.what()); + LOG_ERROR("Error finalizing Zarr stream: ", e.what()); } } @@ -350,15 +356,13 @@ ZarrStream::append(const void* data, size_t nbytes) bool ZarrStream_s::is_s3_acquisition_() const { - return s3_endpoint_.has_value() && s3_bucket_name_.has_value() && - s3_access_key_id_.has_value() && s3_secret_access_key_.has_value(); + return s3_settings_.has_value(); } bool ZarrStream_s::is_compressed_acquisition_() const { - return compressor_.has_value() && compression_codec_.has_value() && - compression_level_.has_value() && compression_shuffle_.has_value(); + return compression_settings_.has_value(); } void @@ -369,17 +373,21 @@ ZarrStream_s::commit_settings_(const struct ZarrStreamSettings_s* settings) custom_metadata_ = trim(settings->custom_metadata); if (is_s3_acquisition(settings)) { - s3_endpoint_ = trim(settings->s3_settings->endpoint); - s3_bucket_name_ = trim(settings->s3_settings->bucket_name); - s3_access_key_id_ = trim(settings->s3_settings->access_key_id); - s3_secret_access_key_ = trim(settings->s3_settings->secret_access_key); + s3_settings_ = { + .endpoint = trim(settings->s3_settings->endpoint), + .bucket_name = trim(settings->s3_settings->bucket_name), + .access_key_id = trim(settings->s3_settings->access_key_id), + .secret_access_key = trim(settings->s3_settings->secret_access_key), + }; } if (is_compressed_acquisition(settings)) { - compressor_ = settings->compression_settings->compressor; - compression_codec_ = settings->compression_settings->codec; - compression_level_ = settings->compression_settings->level; - compression_shuffle_ = settings->compression_settings->shuffle; + compression_settings_ = { + .compressor = settings->compression_settings->compressor, + .codec = settings->compression_settings->codec, + .level = settings->compression_settings->level, + .shuffle = settings->compression_settings->shuffle, + }; } dtype_ = settings->data_type; diff --git a/src/streaming/zarr.stream.hh b/src/streaming/zarr.stream.hh index 15947c1e..2f31a796 100644 --- a/src/streaming/zarr.stream.hh +++ b/src/streaming/zarr.stream.hh @@ -1,5 +1,7 @@ #pragma once +#include "zarr.types.h" + #include #include // size_t @@ -8,7 +10,6 @@ struct ZarrDimension_s { - public: ZarrDimension_s(const char* name, ZarrDimensionType type, uint32_t array_size_px, @@ -22,13 +23,12 @@ struct ZarrDimension_s { } - std::string name; /* Name of the dimension */ - ZarrDimensionType type; /* Type of dimension */ + std::string name; + ZarrDimensionType type; - uint32_t array_size_px; /* Size of the array along this dimension */ - uint32_t chunk_size_px; /* Size of a chunk along this dimension */ - uint32_t shard_size_chunks; /* Number of chunks in a shard along this - * dimension */ + uint32_t array_size_px; + uint32_t chunk_size_px; + uint32_t shard_size_chunks; }; struct ZarrStream_s @@ -46,32 +46,32 @@ struct ZarrStream_s size_t append(const void* data, size_t nbytes); private: + struct S3Settings { + std::string endpoint; + std::string bucket_name; + std::string access_key_id; + std::string secret_access_key; + }; + struct CompressionSettings { + ZarrCompressor compressor; + ZarrCompressionCodec codec; + uint8_t level; + uint8_t shuffle; + }; + std::string error_; // error message. If nonempty, an error occurred. ZarrVersion version_; - std::string store_path_; - - std::optional s3_endpoint_; - std::optional s3_bucket_name_; - std::optional s3_access_key_id_; - std::optional s3_secret_access_key_; - + std::optional s3_settings_; + std::optional compression_settings_; std::string custom_metadata_; - ZarrDataType dtype_; - - std::optional compressor_; - std::optional compression_codec_; - std::optional compression_level_; - std::optional compression_shuffle_; - std::vector dimensions_; - bool multiscale_; - [[nodiscard]] bool is_s3_acquisition_() const; - [[nodiscard]] bool is_compressed_acquisition_() const; + bool is_s3_acquisition_() const; + bool is_compressed_acquisition_() const; /** * @brief Copy settings to the stream. diff --git a/tests/unit-tests/CMakeLists.txt b/tests/unit-tests/CMakeLists.txt index 40f6a8d3..e24ad4bb 100644 --- a/tests/unit-tests/CMakeLists.txt +++ b/tests/unit-tests/CMakeLists.txt @@ -17,7 +17,7 @@ foreach (name ${tests}) ${CMAKE_SOURCE_DIR}/src/streaming ) target_link_libraries(${tgt} PRIVATE - acquire-zarr-logger + acquire-logger acquire-zarr miniocpp::miniocpp )