Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Azure-SDK-based stream-object #112

Merged
merged 94 commits into from
Sep 19, 2024

Conversation

ptahmose
Copy link
Contributor

@ptahmose ptahmose commented Sep 18, 2024

Description

  • adding an input-stream-object based on Azure-SDK for reading data from Azure-Blob-store
  • adding unittests for new functionality
  • the required C++ version had to be raised to C++14, because that's required by the Azure-SDK-headers

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

locally, with CZIcmd, unittests and by beta-users

Checklist:

  • I followed the Contributing Guidelines.
  • I did a self-review.
  • I commented my code, particularly in hard-to-understand areas.
  • I updated the documentation.
  • I updated the version of libCZI following Versioning of libCZI depending on the type of change
    • Bug fix -> PATCH
    • New feature -> MINOR
    • Breaking change -> MAJOR
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Updated `cmake.yml` to streamline Windows dependency installation by removing redundant commands. Enhanced `azureblobinputstream.cpp` with `DefaultAzureCredential` initialization and improved `cout` formatting. Modified `AzureBlobInputStream::Read` to handle HTTP status codes correctly, ensuring accurate byte count reporting. Cleaned up commented-out code for better readability.
Added [[noreturn]] attribute to static methods in CziParse.cpp and CziParse.h for better compiler optimizations and code clarity. Introduced a new constructor in azureblobinputstream.cpp to handle std::wstring URLs, utilizing Utilities::convertUtf8ToWchar_t. Implemented TokenizeAzureUriString in utilities.cpp to parse std::wstring key-value pairs into a std::map. Updated CMakeLists.txt to include test_azureblobstream.cpp in libCZI_UnitTests. Added tests for TokenizeAzureUriString in test_azureblobstream.cpp. Updated azureblobinputstream.h and utilities.h to declare new functions and include necessary headers.
- Modify `AzureBlobInputStream` constructor to accept URL instead of filename.
- Add `CreateWithDefaultAzureCredential` method for service client creation.
- Implement `DetermineAuthenticationMode` in constructor for auth mode.
- Update `Read` method with size/offset checks and detailed error handling.
- Add `DetermineServiceUrl` method for constructing service URL.
- Introduce `AuthenticationMode` enum for different auth methods.
- Update `azureblobinputstream.h` with new methods and enum declarations.
- Adjust commented-out code and preprocessor directives for debugging/future use.
Refactored `azureblobinputstream.cpp` for better readability and maintainability. Key changes include:
- Used iterator for `containername` and `blobname` checks in `tokenized_file_name` map.
- Moved UTF-8 string conversion closer to usage.
- Removed commented-out code and unnecessary debug print statements.
- Determined `service_url` using `AzureBlobInputStream::DetermineServiceUrl`.
- Streamlined initialization of `serviceClient_` and `blockBlobClient_`.
- Improved clarity and efficiency of `CreateWithDefaultAzureCredential` method.
- Removed `#include <iostream>` from `azureblobinputstream.cpp`.
- Modified `CreateWithDefaultAzureCredential`:
  - Use `make_shared` for `DefaultAzureCredential`.
  - Create `BlobServiceClient` as a local variable.
  - Use local `BlobServiceClient` for `BlobContainerClient` and `BlockBlobClient`.
  - Assign `BlockBlobClient` to `blockBlobClient_` using `unique_ptr`.
- Updated `Read` method to remove commented-out line.
- Simplified `~AzureBlobInputStream` by removing empty block.
- Commented out `serviceClient_` in `azureblobinputstream.h`.
Introduced support for connection string-based authentication in Azure Blob storage. Added static constants for various URI keys to improve code readability and maintainability. Implemented a new constructor and method `CreateWithConnectionString` for handling connection string-based authentication. Updated `DetermineAuthenticationMode` to be static and handle the new auth mode. Renamed member variable `blockBlobClient_` to `block_blob_client_` for consistency. Improved error messages in `DetermineServiceUrl`.
@ptahmose ptahmose added the cla Contributor License Agreement sent to Admin label Sep 18, 2024
@ptahmose ptahmose requested a review from a team September 18, 2024 21:27
Added SPDX identifiers for copyright and LGPL-3.0-or-later license.
Included a separator line for clarity.
Modified `cmake.yml` to prevent termination of the Azurite process after running tests. This ensures the Azurite process remains active for the subsequent code-coverage step.
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 31.88406% with 141 lines in your changes missing coverage. Please review.

Project coverage is 65.51%. Comparing base (ec7376c) to head (c9734d6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Src/libCZI/StreamsLib/azureblobinputstream.cpp 4.76% 140 Missing ⚠️
Src/libCZI/utilities.cpp 98.21% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   66.16%   65.51%   -0.66%     
==========================================
  Files          85       86       +1     
  Lines       10685    10892     +207     
==========================================
+ Hits         7070     7136      +66     
- Misses       3615     3756     +141     
Flag Coverage Δ
windows-latest 65.51% <31.88%> (-0.66%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@DaveyJonesBitPail DaveyJonesBitPail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comments

.github/workflows/cmake.yml Show resolved Hide resolved
.github/workflows/cmake.yml Show resolved Hide resolved
Src/libCZI/Doc/stream_objects.markdown Outdated Show resolved Hide resolved
Src/libCZI/Doc/stream_objects.markdown Outdated Show resolved Hide resolved
Src/libCZI/utilities.cpp Outdated Show resolved Hide resolved
Src/libCZI_UnitTests/test_azureblobstream.cpp Show resolved Hide resolved
Src/libCZI_UnitTests/test_azureblobstream.cpp Outdated Show resolved Hide resolved
Src/libCZI_UnitTests/test_azureblobstream.cpp Show resolved Hide resolved
Src/libCZI/utilities.cpp Show resolved Hide resolved
m-aXimilian
m-aXimilian previously approved these changes Sep 19, 2024
Src/CMakeLists.txt Show resolved Hide resolved
.github/workflows/cmake.yml Show resolved Hide resolved
.github/workflows/cmake.yml Show resolved Hide resolved
.github/workflows/cmake.yml Show resolved Hide resolved
@ptahmose ptahmose merged commit 900cada into ZEISS:main Sep 19, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla Contributor License Agreement sent to Admin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants