AzureBlobStorage.WriteBlob - Initial implementation #100
Conversation
WalkthroughA comprehensive implementation for an Azure Blob Storage WriteBlob task has been developed, introducing a new .NET 6.0 project with robust functionality for writing blobs. The project includes GitHub Actions workflows for continuous integration and testing, unit tests, and detailed configuration for connecting to Azure Blob Storage using various authentication methods. The implementation supports multiple connection strategies, content sources, and provides flexible error handling and blob management options. Changes
Sequence DiagramsequenceDiagram
participant User
participant WriteBlob
participant AzureBlobStorage
participant BlobContainer
User->>WriteBlob: Invoke WriteBlob method
WriteBlob->>AzureBlobStorage: Validate connection parameters
AzureBlobStorage->>BlobContainer: Check/Create container
WriteBlob->>AzureBlobStorage: Prepare blob content
AzureBlobStorage->>BlobContainer: Upload blob
BlobContainer-->>AzureBlobStorage: Upload result
AzureBlobStorage-->>WriteBlob: Return operation result
WriteBlob-->>User: Return task result
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (34)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Tag.cs (1)
8-9: Consider adding validation attributes for required fields.
Currently, there's no explicit validation to ensureTagproperties are populated. IfNameis intended to be mandatory for Azure tagging, for instance, you may annotate it with[Required]or other custom validators.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Options.cs (2)
8-9: Class name is sufficiently descriptive.
“Options” clearly communicates that this class contains optional parameters for the blob writing operation. If more parameters are added, consider grouping logically related options under separate classes to keep them organized.
11-17: Property documentation is clear, but could elaborate on what is considered an ‘ignorable error.’
The docstring is concise and to the point. However, you may want to detail or provide a reference to clarify which errors are deemed “ignorable” so maintainers know how this property affects different failure modes.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Result.cs (3)
10-12: Correct typos in XML documentation.
The words "seens" and "occured" are misspelled. They should be "seen" and "occurred" respectively.-/// Operation is seens as completed if an ignorable error has occured +/// Operation is seen as completed if an ignorable error has occurred
18-19: Fix typo in error message reference.
The documentation contains "message.age." It looks like a stray substring. Please remove the extra “.age”.-/// ... the URL will be replaced with the corresponding error message.age. +/// ... the URL will be replaced with the corresponding error message.
24-29: Consider using a record for immutability.
For simple data containers, a C# record can provide cleaner, more expressive, and immutable behavior with less boilerplate.-internal class Result +internal record ResultFrends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Source.cs (5)
10-11: Consider a more descriptive class name.The name "Source" is somewhat generic. While it's valid, a more descriptive name (e.g.,
BlobContentSource) could help future readers understand the purpose of this class at a glance.
19-25: Validate mutual exclusivity whenSourceTypeis notString.Currently, there’s no clear indication of what happens if
ContentStringis set whileSourceTypeisBytes, or vice versa. Adding validation or clarifying documentation can help consumers avoid confusion.
26-33: Note potential memory usage with largeContentBytes.Using byte arrays for heavy data can be memory-intensive. Consider streaming or chunk-based writes when dealing with large files.
34-41: Confirm BOM usage under different OS environments.UTF-8 BOM can cause unexpected behavior on certain platforms or with particular decoders. Ensure you want the BOM included by default, or clarify it in documentation to avoid confusion.
49-55: Handle invalid or missingFileEncodingString.When
Encodingis set toFileEncoding.Other, consider a fallback ifFileEncodingStringis blank or unsupported. A graceful approach might default to UTF-8 without BOM or log a warning to notify users.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.csproj (2)
4-4: Consider multi-targeting for broader compatibility.While .NET 6.0 is an LTS release, consider also targeting .NET 8.0 (latest LTS) to ensure forward compatibility and take advantage of performance improvements.
- <TargetFrameworks>net6.0</TargetFrameworks> + <TargetFrameworks>net6.0;net8.0</TargetFrameworks>
10-13: Enhance package metadata for better discoverability.Consider adding more specific tags and a more detailed description to improve package discoverability and provide better context to users.
- <PackageTags>Frends</PackageTags> + <PackageTags>Frends;Azure;Blob;Storage;Cloud;File</PackageTags> - <Description>Frends Task to write content to Azure Blob Storage.</Description> + <Description>Frends Task to write content to Azure Blob Storage. Supports various authentication methods and content sources with configurable blob properties.</Description>Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs (4)
34-76: Consider aligningConfigureAwait(false)usage for consistency.
InsideWriteBlob(lines 66 and 64), you await calls withoutConfigureAwait(false), whereasHandleWrite(line 92) does includeConfigureAwait(false). For consistency and to avoid potential synchronization context issues in library code, consider addingConfigureAwait(false)to all awaits in this method.- var info = await HandleWrite(blobClient, source, destination, cancellationToken); + var info = await HandleWrite(blobClient, source, destination, cancellationToken).ConfigureAwait(false);
34-76: Re-throw the original exception or wrap it as an inner exception.
Lines 74-75 and 69-70 catch and handle exceptions. In line 75, you return a non-descriptiveResult(false, null)when an exception is thrown, losing details that might help debug the failure. Additionally, creating a newExceptionin line 60 (for theNotSupportedException) or lines 69-70 can lose stack trace context. Consider re-throwing or wrapping it with a custom exception that retains the original stack trace as anInnerException.
78-94: Potential data consistency concern with partial writes.
If the task gets canceled while uploading large data, the partial blob content may remain in the container. Depending on business requirements, consider implementing a strategy to roll back or clean up partial blobs if the upload fails or is canceled.
136-145: Validate container name transformations.
You convert container names to lowercase at lines 47, 57, and again in the container creation method. Consider a single, consistent approach to normalizing container names (e.g., only inValidateContainerNameor once inWriteBlob) to avoid potential mismatch or confusion.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Enums/Enums.cs (4)
1-2: Consider adding an overview comment at the file level.
Although each enum is well documented, an overarching file-level summary could enhance discoverability and context.
3-12: Align the XML summary for SourceType with actual values.
The comment references "single file or entire directory," but the enum deals with string or byte array sources. Clarifying the usage context will reduce confusion.
56-75: Consider adding a future ConnectionMethod for Managed Identity.
Managed Identity is often preferred for Azure resources, reducing secret management overhead.
77-92: Add individual field-level summaries for FileEncoding.
Although there's a top-level description for the enum, explaining each option (especiallyOther) will improve clarity.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Destination.cs (5)
28-35: Add URI validation.
Consider validating theUriproperty format at runtime to catch configuration issues early.
36-42: Standardize naming for ApplicationID.
Renaming toClientIdorAppIdmay align better with Azure best practices and reduce confusion.
43-49: Consistent naming for TenantID.
RenamingTenantIDtoTenantIdwould ensure consistent casing across properties.
67-74: Validate container name constraints.
Azure container names must comply with strict naming rules. Adding a preemptive validation step can help avoid runtime failures.
84-88: Enforce Azure tag limits.
Tag names and values have specific length and count restrictions. Validate or sanitize them to prevent potential errors.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs (5)
16-19: Ensure consistent test credentials handling.
Relying on environment variables may lead to inconsistent results in CI if not properly set. Document or unify credential handling.
66-70: Validate teardown process.
A delay in container deletion could break subsequent test runs if they assume a fresh state.
120-150: Encoding tests only check textual equivalence.
If the system truly requires distinct encodings, consider verifying byte patterns or using an encoding-aware comparison.
184-190: Test depends on library-specific message string.
Matching exact error text can cause brittle tests if library exceptions change. Consider partial matching or custom exception logic.
202-223: Add more varied tag scenarios.
Currently, tests only set one tag. Edge cases or multiple tags would better validate tagging functionality..github/workflows/WriteBlob_release.yml (1)
1-5: Review manual release trigger approach.
workflow_dispatchis suitable for on-demand releases, but you could also combine it with scheduled or version-tag-based triggers..github/workflows/WriteBlob_build_and_test_on_main.yml (1)
1-10: LGTM! Workflow triggers are well configured.The workflow triggers are appropriately set up for main branch changes and manual dispatch, with correct path filtering.
Fix trailing spaces on line 5.
Remove trailing spaces after
branches:for consistency with YAML style guidelines.- branches: + branches:🧰 Tools
🪛 yamllint (1.35.1)
[error] 5-5: trailing spaces
(trailing-spaces)
.github/workflows/WriteBlob_build_and_test_on_push.yml (1)
21-28: LGTM! Secrets are properly configured with test-specific additions.Appropriate use of GitHub secrets including the additional test feed API key.
Add newline at end of file.
Add a newline character at the end of the file to follow YAML best practices.
env_var_value_4: ${{ secrets.Frends_AZUREBLOBSTORAGE_TENANTID }} - env_var_value_5: ${{ secrets.Frends_AZUREBLOBSTORAGE_SASTOKEN }} + env_var_value_5: ${{ secrets.Frends_AZUREBLOBSTORAGE_SASTOKEN }} +🧰 Tools
🪛 yamllint (1.35.1)
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/WriteBlob_build_and_test_on_main.yml(1 hunks).github/workflows/WriteBlob_build_and_test_on_push.yml(1 hunks).github/workflows/WriteBlob_release.yml(1 hunks)Frends.AzureBlobStorage.WriteBlob/CHANGELOG.md(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/Frends.AzureBlobStorage.WriteBlob.Tests.csproj(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.sln(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Destination.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Options.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Result.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Source.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Tag.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Enums/Enums.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.csproj(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/FrendsTaskMetadata.json(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- Frends.AzureBlobStorage.WriteBlob/CHANGELOG.md
- Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.sln
- Frends.AzureBlobStorage.WriteBlob/README.md
- Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/FrendsTaskMetadata.json
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/WriteBlob_build_and_test_on_main.yml
[error] 5-5: trailing spaces
(trailing-spaces)
.github/workflows/WriteBlob_build_and_test_on_push.yml
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / Build on windows-latest
🔇 Additional comments (29)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Tag.cs (2)
14-15: Confirm that theDataFormatString = "Text"is intended.
DisplayFormatis typically used to format display output for UI or serialization. If you want to strictly allow or disallow certain characters forName, consider a custom validation or a[RegularExpression]attribute.
17-22: Validate the value length or content to conform with Azure Blob Storage tag constraints.
Some Azure APIs limit tag value lengths and character sets. Consider adding domain-specific checks or a[RegularExpression]attribute to prevent invalid tag values.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Options.cs (2)
1-4: Namespace definition and using statements look good.
Theusing System.ComponentModel;reference is appropriate for the[DefaultValue]attribute, and the namespace is both logical and intuitive.
5-7: Documentation header is concise and helpful.
Providing a succinct class summary clarifies the intent of optional parameters for this feature, which is beneficial for maintainers and users alike.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Result.cs (1)
5-7: Good use of XML documentation.
The comments provide a clear introduction to the purpose of this class and its result object.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Source.cs (4)
1-3: Explicit references look good.The chosen namespaces appear relevant and keep the code straightforward.
5-9: Namespace and class summary are well-structured.Your documentation is clear, and the namespace name follows best practices. No additional changes are needed here.
12-18: Verify if the[DefaultValue(SourceType.Bytes)]attribute is being respected at runtime.In some scenarios,
[DefaultValue]might not enforce the property’s initial value unless a supporting framework or custom logic applies it. If you rely on this default, consider adding a constructor to ensureSourceType = SourceType.Bytesis always set.
42-48: UIHint attributes look fine.Linking
EnableBOMto theFileEncoding.UTF8enum matches the intended usage and keeps the intent clear.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.csproj (1)
19-21: LGTM!The FrendsTaskMetadata.json configuration is correctly set up for packaging and deployment.
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/Frends.AzureBlobStorage.WriteBlob.Tests.csproj (3)
1-6: LGTM! Project configuration follows best practices.The project configuration is well-structured, using appropriate SDK and targeting .NET 6.0 LTS.
20-22: LGTM! Project reference is correctly configured.The project reference to the main WriteBlob project is properly set up.
9-18: Verify test framework usage before updating packages.Let's analyze the existing test patterns to ensure a smooth transition to the latest package versions.
✅ Verification successful
Safe to update test framework packages
The codebase uses straightforward test patterns without framework-specific features. Tests are properly isolated by component, and the implementations are simple enough that they won't be affected by version updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test framework usage patterns to identify potential breaking changes # Search for NUnit and MSTest usage patterns echo "Checking NUnit usage patterns..." rg -l "using NUnit" -g "*.cs" rg -l "\[Test\]" -g "*.cs" rg -l "\[TestCase" -g "*.cs" echo "Checking MSTest usage patterns..." rg -l "using Microsoft.VisualStudio.TestTools" -g "*.cs" rg -l "\[TestMethod\]" -g "*.cs" rg -l "\[DataTestMethod\]" -g "*.cs"Length of output: 2360
Script:
#!/bin/bash # Check for test base classes and framework-specific features echo "Checking test class implementations..." rg -A 5 "class.*Test" -g "*.cs" echo "Checking for test utilities and base classes..." rg -l "class.*TestBase" -g "*.cs" rg -l "abstract class.*Test" -g "*.cs"Length of output: 10137
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs (1)
108-119: Ensure all potential code pages are available.
You rely onCodePagesEncodingProvider.Instancefor "WINDOWS1252" or "Other." When running on some platforms (e.g., .NET 6 container images), code pages might not always be loaded by default. Validate that this provider is registered in startup or that you have included the right dependencies to ensure the encoding is supported.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Enums/Enums.cs (2)
14-33: AzureBlobType enums look solid.
The XML documentation is comprehensive and matches well with common Azure Blob scenarios.
35-54: Verify handling for 'Append' with non-Append blobs.
WhenHandleExistingFile.Appendis chosen for Block or Page blobs, ensure the code properly downloads, appends locally, and reuploads without errors.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Destination.cs (4)
12-18: LGTM for ConnectionMethod default.
Using[DefaultValue(ConnectionMethod.ConnectionString)]is clear and avoids ambiguity.
50-56: Confirm secret handling for ClientSecret.
Verify that sensitive values are neither logged nor stored in plaintext. Consider Key Vault or similar secure storage.
90-97: Ensure container creation privileges.
WhenCreateContainerIfItDoesNotExistis true, confirm the user's role-based access allows container creation.
99-108: Clear documentation for HandleExistingFile.
The examples and XML comments are well structured, aiding maintainability.Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs (2)
33-64: Potential naming collisions in parallel tests.
Multiple containers generated by date/time strings might still collide if tests run concurrently. Consider adding thread-safe uniqueness.
72-85: Assert content equality after Connection String writes.
Currently, content verification only occurs after the OAuth test. Adding a check earlier strengthens test coverage..github/workflows/WriteBlob_release.yml (1)
6-12: Double-check secret scope.
Verify thatTASKS_FEED_API_KEYis stored as a secure repository or organization secret and is available in this repo’s context..github/workflows/WriteBlob_build_and_test_on_main.yml (3)
11-15: LGTM! Job configuration follows best practices.Good use of reusable workflow and correct working directory specification.
16-20: LGTM! Environment variables are well structured.Comprehensive coverage of Azure authentication methods with consistent naming convention.
21-27: LGTM! Secrets are properly configured.Appropriate use of GitHub secrets for sensitive information with consistent mapping to environment variables.
.github/workflows/WriteBlob_build_and_test_on_push.yml (3)
1-10: LGTM! Workflow triggers are well configured.The workflow triggers are appropriately set up for feature branch changes and manual dispatch, with correct path filtering.
11-15: LGTM! Job configuration follows best practices.Good use of test-specific reusable workflow and correct working directory specification.
16-20: LGTM! Environment variables are consistent with main workflow.Maintains same environment variable structure as the main workflow.
...Storage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.csproj
Outdated
Show resolved
Hide resolved
...eBlob/Frends.AzureBlobStorage.WriteBlob.Tests/Frends.AzureBlobStorage.WriteBlob.Tests.csproj
Show resolved
Hide resolved
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs
Outdated
Show resolved
Hide resolved
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs (3)
20-33: Enhance XML documentation for better clarity.While the documentation is good, consider adding:
- More details about the task's purpose and common use cases
- Complete parameter descriptions
- Expanded return type documentation explaining the Success and Info fields
/// <summary> -/// Azure Blob Storage Task. +/// Task for writing content to Azure Blob Storage blobs. Supports string content, byte arrays, +/// and multiple authentication methods including Connection String, SAS Token, and OAuth2. /// </summary> public class AzureBlobStorage { /// <summary> /// Frends Task to write content to Azure Blob Storage. /// [Documentation](https://tasks.frends.com/tasks/frends-tasks/Frends.AzureBlobStorage.WriteBlob) /// </summary> - /// <param name="source">Source parameters.</param> - /// <param name="destination">Destination parameters.</param> - /// <param name="options">Optional parameters.</param> + /// <param name="source">Source parameters defining the content to be written (string or byte array).</param> + /// <param name="destination">Destination parameters including storage account details and blob configuration.</param> + /// <param name="options">Optional parameters for controlling error handling and other behaviors.</param> /// <param name="cancellationToken">Token generated by Frends to stop this Task.</param> - /// <returns>Object { bool Success, BlobContentInfo Info }</returns> + /// <returns>Object { bool Success: Indicates if the operation completed successfully, + /// BlobContentInfo Info: Contains metadata about the uploaded blob when successful, null on failure }</returns>
43-61: Modernize switch statement using pattern matching.Consider using C# pattern matching for a more concise and maintainable switch expression.
- switch (destination.ConnectionMethod) - { - case ConnectionMethod.ConnectionString: - blobServiceClient = new BlobServiceClient(destination.ConnectionString); - blobClient = new BlobClient(destination.ConnectionString, destination.ContainerName.ToLower(), destination.BlobName); - break; - case ConnectionMethod.SASToken: - var blobContainerClient = new BlobContainerClient(new Uri($"{destination.Uri}/{destination.ContainerName}?"), new AzureSasCredential(destination.SASToken)); - blobClient = blobContainerClient.GetBlobClient(destination.BlobName); - break; - case ConnectionMethod.OAuth2: - var serviceURI = new Uri($"{destination.Uri}"); - var credentials = new ClientSecretCredential(destination.TenantID, destination.ApplicationID, destination.ClientSecret, new ClientSecretCredentialOptions()); - blobServiceClient = new BlobServiceClient(serviceURI, credentials); - var uri = new Uri($"{destination.Uri}/{destination.ContainerName.ToLower()}/{destination.BlobName}"); - blobClient = new BlobClient(uri, credentials); - break; - default: throw new NotSupportedException(); - } + blobClient = destination.ConnectionMethod switch + { + ConnectionMethod.ConnectionString => new BlobClient( + destination.ConnectionString, + destination.ContainerName.ToLower(), + destination.BlobName), + + ConnectionMethod.SASToken => new BlobContainerClient( + new Uri($"{destination.Uri}/{destination.ContainerName}?"), + new AzureSasCredential(destination.SASToken)) + .GetBlobClient(destination.BlobName), + + ConnectionMethod.OAuth2 => { + var credentials = new ClientSecretCredential( + destination.TenantID, + destination.ApplicationID, + destination.ClientSecret, + new ClientSecretCredentialOptions()); + blobServiceClient = new BlobServiceClient(new Uri(destination.Uri), credentials); + return new BlobClient( + new Uri($"{destination.Uri}/{destination.ContainerName.ToLower()}/{destination.BlobName}"), + credentials); + }, + + _ => throw new NotSupportedException($"Connection method {destination.ConnectionMethod} is not supported") + };
136-144: Enhance container name validation.The container name validation could be more robust by checking for consecutive hyphens and ensuring the container name doesn't start or end with a hyphen.
private static void ValidateContainerName(string container) { if (container.Length < 3) throw new Exception("Container name is too short. Name needs to be between 3 and 63 characters."); if (container.Length > 63) throw new Exception("Container name is too long. Name needs to be between 3 and 63 characters."); - if (!Regex.IsMatch(container, @"^[a-z0-9]+(-[a-z0-9]+)*$")) + if (!Regex.IsMatch(container, @"^[a-z0-9][a-z0-9-]*[a-z0-9]$")) throw new Exception("Container name includes invalid characters. See more information: https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#container-names"); + if (container.Contains("--")) + throw new Exception("Container name cannot contain consecutive hyphens."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.csproj(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.csproj
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build / Build on windows-latest
- GitHub Check: build / Code Quality Check
🔇 Additional comments (2)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs (2)
78-93: Add validation for maximum blob size.The
HandleWritemethod should validate that the content size doesn't exceed Azure's maximum blob size limits.private static async Task<BlobContentInfo> HandleWrite(BlobClient blobClient, Source source, Destination destination, CancellationToken cancellationToken) { + const long MaxBlockBlobSize = 190L * 1024 * 1024 * 1024; // 190GB var overwrite = destination.HandleExistingFile == HandleExistingFile.Overwrite; var encoding = GetEncoding(source.Encoding, source.FileEncodingString, source.EnableBOM) ?? throw new Exception("Encoding was invalid."); byte[] bytes = source.SourceType == SourceType.String ? encoding.GetBytes(source.ContentString) : source.ContentBytes; + + if (bytes.Length > MaxBlockBlobSize) + throw new ArgumentException($"Content size ({bytes.Length} bytes) exceeds maximum allowed size for block blobs ({MaxBlockBlobSize} bytes)"); + var tags = destination.Tags != null ? destination.Tags.ToDictionary(tag => tag.Name, tag => tag.Value) : new Dictionary<string, string>();
54-56: Review OAuth2 credential handling security.The
ClientSecretCredentialis created with default options. Consider configuring additional security options such as enabling HTTPS-only communication and setting appropriate token refresh parameters.- var credentials = new ClientSecretCredential(destination.TenantID, destination.ApplicationID, destination.ClientSecret, new ClientSecretCredentialOptions()); + var credentialOptions = new ClientSecretCredentialOptions + { + Transport = new Azure.Core.Pipeline.HttpClientTransport(new HttpClient { DefaultRequestVersion = new Version(2, 0) }), + AuthorityHost = AzureAuthorityHosts.AzurePublicCloud, + // Ensure HTTPS-only communication + DisableInstanceDiscovery = false + }; + var credentials = new ClientSecretCredential(destination.TenantID, destination.ApplicationID, destination.ClientSecret, credentialOptions);
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Result.cs (4)
5-8: Enhance class documentation for better clarity.The current documentation is minimal. Consider adding more details about:
- When this class is used
- What constitutes a successful operation
- Common scenarios where this result is returned
- Example usage
/// <summary> -/// Task's result. +/// Represents the result of an Azure Blob Storage write operation. +/// This class encapsulates the outcome of the operation, including success status, +/// blob information, and the URI of the uploaded blob. +/// Used as the return type for WriteBlob operations. /// </summary>
10-15: Fix documentation typos and improve clarity.The documentation contains typos and could be clearer about the success conditions.
/// <summary> -/// Operation complete. -/// Operation is seens as completed if an ignorable error has occured and Options.ThrowErrorOnFailure is set to false. +/// Indicates whether the operation completed successfully. +/// The operation is considered successful if either: +/// - The blob was written without errors +/// - An ignorable error occurred and Options.ThrowErrorOnFailure is set to false /// </summary>
24-27: Enhance Uri property documentation.The current documentation is minimal and doesn't specify the format or potential values.
/// <summary> -/// URI of uploaded file. +/// The complete URI of the uploaded blob in the format: +/// https://{account}.blob.core.windows.net/{container}/{blobname} +/// Returns null if the operation was not successful. /// </summary> +/// <example>https://mystorageaccount.blob.core.windows.net/mycontainer/example.txt</example>
29-34: Add parameter validation in the constructor.The constructor should validate its parameters to ensure the object is always in a valid state.
internal Result(bool success, BlobContentInfo info, string uri) { + if (success && info == null) + throw new ArgumentNullException(nameof(info), "BlobContentInfo cannot be null when success is true"); + if (success && string.IsNullOrEmpty(uri)) + throw new ArgumentException("URI cannot be empty when success is true", nameof(uri)); + Success = success; Info = info; Uri = uri; }Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs (5)
24-24: Consider making the URI configurable via environment variables.The hard-coded URI
"https://stataskdevelopment.blob.core.windows.net"might cause issues when running tests in different environments.- private readonly string _uri = "https://stataskdevelopment.blob.core.windows.net"; + private readonly string _uri = Environment.GetEnvironmentVariable("Frends_AzureBlobStorage_URI") ?? "https://stataskdevelopment.blob.core.windows.net";
36-36: Consider using a more reliable container naming strategy.The current container naming strategy using milliseconds and ticks could potentially create naming conflicts in parallel test runs. Consider using a UUID instead.
- _containerName = $"test-container{DateTime.Now.ToString("mmssffffff", CultureInfo.InvariantCulture)}"; + _containerName = $"test-container-{Guid.NewGuid():N}";
124-131: Consider using TestCaseSource for encoding tests.The encoding test could be simplified using NUnit's TestCaseSource attribute to reduce code duplication and improve maintainability.
+ private static IEnumerable<FileEncoding> EncodingTestCases() + { + yield return FileEncoding.UTF8; + yield return FileEncoding.Default; + yield return FileEncoding.ASCII; + yield return FileEncoding.WINDOWS1252; + yield return FileEncoding.Other; + } - [Test] + [TestCaseSource(nameof(EncodingTestCases))] - public async Task WriteBlob_TestEncoding() + public async Task WriteBlob_TestEncoding(FileEncoding encoding) { - var encodings = new List<FileEncoding>() - { - FileEncoding.UTF8, - FileEncoding.Default, - FileEncoding.ASCII, - FileEncoding.WINDOWS1252, - FileEncoding.Other - }; _source.FileEncodingString = "windows-1251"; - foreach (var encoding in encodings) - { - _source.Encoding = encoding; + _source.Encoding = encoding;Also applies to: 135-150
187-187: Consider using a constant for the invalid connection string.The invalid connection string could be moved to a constant to improve maintainability and reusability.
+ private const string InvalidConnectionString = "DefaultEndpointsProtocol=https;AccountName=invalid;AccountKey=InvalidAccountKey;EndpointSuffix=core.windows.net"; [Test] public void WriteBlob_InvalidConnectionString_ShouldThrowException() { - _destination.ConnectionString = "DefaultEndpointsProtocol=https;AccountName=invalid;AccountKey=InvalidAccountKey;EndpointSuffix=core.windows.net"; + _destination.ConnectionString = InvalidConnectionString;
239-239: Consider adding retry policy for blob operations.Azure Storage operations might fail transiently. Consider implementing a retry policy for better reliability.
You can use the built-in retry policy configuration:
- await container.CreateIfNotExistsAsync(PublicAccessType.None, null, null); + var options = new BlobClientOptions + { + Retry = { MaxRetries = 3, Delay = TimeSpan.FromSeconds(1) } + }; + var blobServiceClient = new BlobServiceClient(_connectionString, options); + var container = blobServiceClient.GetBlobContainerClient(containerName); + await container.CreateIfNotExistsAsync(PublicAccessType.None);Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs (3)
45-63: Consider extracting connection logic to separate methods.The switch statement handling different connection methods could be refactored into separate methods for better maintainability and testability.
- switch (destination.ConnectionMethod) - { - case ConnectionMethod.ConnectionString: - blobServiceClient = new BlobServiceClient(destination.ConnectionString); - blobClient = new BlobClient(destination.ConnectionString, destination.ContainerName.ToLower(), blobName); - break; - case ConnectionMethod.SASToken: - var blobContainerClient = new BlobContainerClient(new Uri($"{destination.Uri}/{destination.ContainerName}?"), new AzureSasCredential(destination.SASToken)); - blobClient = blobContainerClient.GetBlobClient(blobName); - break; - case ConnectionMethod.OAuth2: - var serviceURI = new Uri($"{destination.Uri}"); - var credentials = new ClientSecretCredential(destination.TenantID, destination.ApplicationID, destination.ClientSecret, new ClientSecretCredentialOptions()); - blobServiceClient = new BlobServiceClient(serviceURI, credentials); - var uri = new Uri($"{destination.Uri}/{destination.ContainerName.ToLower()}/{blobName}"); - blobClient = new BlobClient(uri, credentials); - break; - default: throw new NotSupportedException(); - } + (blobServiceClient, blobClient) = destination.ConnectionMethod switch + { + ConnectionMethod.ConnectionString => CreateConnectionStringClient(destination, blobName), + ConnectionMethod.SASToken => CreateSASTokenClient(destination, blobName), + ConnectionMethod.OAuth2 => CreateOAuth2Client(destination, blobName), + _ => throw new NotSupportedException($"Connection method {destination.ConnectionMethod} is not supported") + };Add these private methods:
private static (BlobServiceClient, BlobClient) CreateConnectionStringClient(Destination destination, string blobName) { var serviceClient = new BlobServiceClient(destination.ConnectionString); var client = new BlobClient(destination.ConnectionString, destination.ContainerName.ToLower(), blobName); return (serviceClient, client); } private static (BlobServiceClient, BlobClient) CreateSASTokenClient(Destination destination, string blobName) { var containerClient = new BlobContainerClient( new Uri($"{destination.Uri}/{destination.ContainerName}?"), new AzureSasCredential(destination.SASToken)); return (null, containerClient.GetBlobClient(blobName)); } private static (BlobServiceClient, BlobClient) CreateOAuth2Client(Destination destination, string blobName) { var serviceUri = new Uri(destination.Uri); var credentials = new ClientSecretCredential( destination.TenantID, destination.ApplicationID, destination.ClientSecret, new ClientSecretCredentialOptions()); var serviceClient = new BlobServiceClient(serviceUri, credentials); var uri = new Uri($"{destination.Uri}/{destination.ContainerName.ToLower()}/{blobName}"); return (serviceClient, new BlobClient(uri, credentials)); }
162-172: Use Stream.CopyTo for better performance.The custom
CopyToimplementation can be replaced with the built-inStream.CopyTomethod which is optimized for performance.- private static void CopyTo(Stream src, Stream dest) - { - byte[] bytes = new byte[4096]; - - int cnt; - - while ((cnt = src.Read(bytes, 0, bytes.Length)) != 0) - { - dest.Write(bytes, 0, cnt); - } - } + private static void CopyTo(Stream src, Stream dest) + { + src.CopyTo(dest); + }
150-160: Optimize compression buffer size.The compression method could benefit from using a larger buffer size and providing compression level control.
private static byte[] Compress(byte[] original) { using var msi = new MemoryStream(original); using var mso = new MemoryStream(); - using (var gs = new GZipStream(mso, CompressionMode.Compress)) + using (var gs = new GZipStream(mso, CompressionLevel.Optimal)) { - CopyTo(msi, gs); + msi.CopyTo(gs, 81920); // Use 80KB buffer for optimal performance } return mso.ToArray(); }Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Destination.cs (8)
7-9: Enhance class documentation.The current documentation is too brief. Consider adding more details about the class's purpose, its role in Azure Blob Storage operations, and important considerations for users.
/// <summary> -/// Destination parameters. +/// Configuration parameters for Azure Blob Storage destination, including connection details, +/// authentication methods, container/blob specifications, and handling options. +/// This class supports multiple authentication methods: Connection String, OAuth2, and SAS Token. /// </summary>
22-23: Use a more secure example for ConnectionString.The current example contains a pattern that resembles a real account key. Consider using a more obvious placeholder value.
-/// <example>DefaultEndpointsProtocol=https;AccountName=accountname;AccountKey=Pdlrxyz==;EndpointSuffix=core.windows.net</example> +/// <example>DefaultEndpointsProtocol=https;AccountName=your_account_name;AccountKey=your_account_key;EndpointSuffix=core.windows.net</example>
53-54: Use a more secure example for ClientSecret.The current example ("Password!") might encourage weak secrets. Consider using a more secure example.
-/// <example>Password!</example> +/// <example>Use a strong secret value generated from Azure Portal</example>
62-63: Fix XML encoding in SASToken example.The XML-encoded ampersands (
&) in the example might cause confusion. Consider using regular ampersands in the example since it's within XML comments.-/// <example>sv=2021-04-10&se=2022-04-10T10%3A431Z&sr=c&sp=l&sig=ZJg983RovE%2BZXI</example> +/// <example>sv=2021-04-10&se=2022-04-10T10%3A431Z&sr=c&sp=l&sig=ZJg983RovE%2BZXI</example>
67-74: Add validation for container name.The property mentions that all letters will be converted to lowercase, but it doesn't validate other container naming rules (3-63 characters, only lowercase letters, numbers, and hyphens).
/// <example>examplecontainer</example> [DisplayFormat(DataFormatString = "Text")] +[RegularExpression("^[a-z0-9](?!.*--)[a-z0-9-]{1,61}[a-z0-9]$", + ErrorMessage = "Container name must be 3-63 characters, start/end with letter/number, contain only lowercase letters, numbers, and hyphens.")] public string ContainerName { get; set; }
80-81: Update BlobName example to use forward slashes.The example uses Windows backslash path format. Since this is for cloud storage, using forward slashes would be more appropriate and consistent across platforms.
-/// <example>BlobName.txt; C:\folder\blobName.txt</example> +/// <example>BlobName.txt; folder/blobName.txt</example>
84-88: Add validation for tag names and values.Azure Blob Storage has specific requirements for tag names and values. Consider adding validation to ensure they meet these requirements.
Add validation attributes to the
Tagclass (not shown in this file):public class Tag { [RegularExpression("^[a-zA-Z0-9+-./_:@]+$", ErrorMessage = "Tag name can only contain alphanumeric and these special characters: + - . / _ : @")] [StringLength(128)] public string Name { get; set; } [StringLength(256)] public string Value { get; set; } }
109-113: Enhance Compress property documentation.The documentation should specify which compression method is used and any implications on the blob's content type or metadata.
/// <summary> -/// Should the string be compressed before sending? +/// Specifies whether to compress the content using GZIP compression before uploading. +/// When enabled, the blob's content will be compressed and appropriate content encoding metadata will be set. /// </summary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Destination.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/Definitions/Result.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build / Code Quality Check
- GitHub Check: build / Build on windows-latest
🔇 Additional comments (4)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs (1)
257-262: 🛠️ Refactor suggestionOptimize blob content verification.
The current implementation creates a new BlobClient instance when one is already available. Also, the StreamReader should be properly disposed of using a using statement.
- var blobClient = new BlobClient(_connectionString, _destination.ContainerName, _destination.BlobName); - var blobDownload = await blobClient.DownloadAsync(); - - using var reader = new StreamReader(blobDownload.Value.Content); - var content = await reader.ReadToEndAsync(); - return content == expected; + var blobDownload = await blob.DownloadAsync(); + using (var reader = new StreamReader(blobDownload.Value.Content)) + { + var content = await reader.ReadToEndAsync(); + return content == expected; + }Likely invalid or redundant comment.
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs (3)
71-77: Improve error handling with logging.The catch block silently swallows exceptions when
ThrowErrorOnFailureis false. Consider logging the error for debugging purposes.
24-78: Well-structured implementation with comprehensive error handling.The implementation provides a robust solution for writing blobs to Azure Storage with support for multiple authentication methods, compression, and proper parameter validation.
28-28: Verify the documentation URL.The documentation URL appears to be a placeholder. Ensure it points to the actual documentation when published.
| /// <summary> | ||
| /// This object contains the source file path and the URL of the blob. | ||
| /// If an ignorable error occurs, such as when a blob already exists and Options.ThrowErrorOnFailure is set to false, the URL will be replaced with the corresponding error message.age. | ||
| /// </summary> | ||
| /// <example>{ { c:\temp\examplefile.txt, https://storage.blob.core.windows.net/container/examplefile.txt }, { c:\temp\examplefile2.txt, Blob examplefile2 already exists. } }</example> | ||
| public BlobContentInfo Info { get; private set; } |
There was a problem hiding this comment.
Review property type and fix documentation inconsistencies.
There are several issues with this property:
- The documentation describes functionality that doesn't match the
BlobContentInfotype. It mentions containing "source file path and URL" butBlobContentInfodoesn't typically store this information. - The example shows key-value pairs, but the property is of type
BlobContentInfo. - The documentation contains a typo ("message.age").
Consider creating a custom type to properly represent this information:
public class BlobWriteInfo
{
public string SourcePath { get; set; }
public string BlobUrl { get; set; }
public BlobContentInfo ContentInfo { get; set; }
}Then update the property:
-public BlobContentInfo Info { get; private set; }
+public BlobWriteInfo Info { get; private set; }| private static async Task<BlobContentInfo> HandleWrite(BlobClient blobClient, Source source, Destination destination, CancellationToken cancellationToken) | ||
| { | ||
| var overwrite = destination.HandleExistingFile == HandleExistingFile.Overwrite; | ||
| var encoding = GetEncoding(source.Encoding, source.FileEncodingString, source.EnableBOM) ?? throw new Exception("Encoding was invalid."); | ||
| byte[] bytes = source.SourceType == SourceType.String ? encoding.GetBytes(source.ContentString) : source.ContentBytes; | ||
| if (destination.Compress) | ||
| bytes = Compress(bytes); | ||
| var tags = destination.Tags != null ? destination.Tags.ToDictionary(tag => tag.Name, tag => tag.Value) : new Dictionary<string, string>(); | ||
|
|
||
| var uploadOptions = new BlobUploadOptions | ||
| { | ||
| Conditions = overwrite ? null : new BlobRequestConditions { IfNoneMatch = new ETag("*") }, | ||
| HttpHeaders = new BlobHttpHeaders { ContentType = MimeUtility.GetMimeMapping(destination.Compress ? "gz" : destination.BlobName), ContentEncoding = encoding.WebName }, | ||
| Tags = tags.Count > 0 ? tags : null | ||
| }; | ||
|
|
||
| return await blobClient.UploadAsync(BinaryData.FromBytes(bytes), uploadOptions, cancellationToken).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content length validation in HandleWrite.
Consider adding validation for content length to prevent uploading empty blobs or blobs that exceed Azure's size limits.
private static async Task<BlobContentInfo> HandleWrite(BlobClient blobClient, Source source, Destination destination, CancellationToken cancellationToken)
{
var overwrite = destination.HandleExistingFile == HandleExistingFile.Overwrite;
var encoding = GetEncoding(source.Encoding, source.FileEncodingString, source.EnableBOM) ?? throw new Exception("Encoding was invalid.");
byte[] bytes = source.SourceType == SourceType.String ? encoding.GetBytes(source.ContentString) : source.ContentBytes;
+
+ if (bytes == null || bytes.Length == 0)
+ throw new ArgumentException("Content cannot be empty");
+
+ // Azure block blob size limit is 190.7 TiB (209,715,200 MB)
+ const long maxBlobSize = 209715200L * 1024 * 1024;
+ if (bytes.Length > maxBlobSize)
+ throw new ArgumentException($"Content size {bytes.Length} bytes exceeds Azure's maximum blob size of {maxBlobSize} bytes");
+
if (destination.Compress)
bytes = Compress(bytes);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static async Task<BlobContentInfo> HandleWrite(BlobClient blobClient, Source source, Destination destination, CancellationToken cancellationToken) | |
| { | |
| var overwrite = destination.HandleExistingFile == HandleExistingFile.Overwrite; | |
| var encoding = GetEncoding(source.Encoding, source.FileEncodingString, source.EnableBOM) ?? throw new Exception("Encoding was invalid."); | |
| byte[] bytes = source.SourceType == SourceType.String ? encoding.GetBytes(source.ContentString) : source.ContentBytes; | |
| if (destination.Compress) | |
| bytes = Compress(bytes); | |
| var tags = destination.Tags != null ? destination.Tags.ToDictionary(tag => tag.Name, tag => tag.Value) : new Dictionary<string, string>(); | |
| var uploadOptions = new BlobUploadOptions | |
| { | |
| Conditions = overwrite ? null : new BlobRequestConditions { IfNoneMatch = new ETag("*") }, | |
| HttpHeaders = new BlobHttpHeaders { ContentType = MimeUtility.GetMimeMapping(destination.Compress ? "gz" : destination.BlobName), ContentEncoding = encoding.WebName }, | |
| Tags = tags.Count > 0 ? tags : null | |
| }; | |
| return await blobClient.UploadAsync(BinaryData.FromBytes(bytes), uploadOptions, cancellationToken).ConfigureAwait(false); | |
| } | |
| private static async Task<BlobContentInfo> HandleWrite(BlobClient blobClient, Source source, Destination destination, CancellationToken cancellationToken) | |
| { | |
| var overwrite = destination.HandleExistingFile == HandleExistingFile.Overwrite; | |
| var encoding = GetEncoding(source.Encoding, source.FileEncodingString, source.EnableBOM) ?? throw new Exception("Encoding was invalid."); | |
| byte[] bytes = source.SourceType == SourceType.String ? encoding.GetBytes(source.ContentString) : source.ContentBytes; | |
| if (bytes == null || bytes.Length == 0) | |
| throw new ArgumentException("Content cannot be empty"); | |
| // Azure block blob size limit is 190.7 TiB (209,715,200 MB) | |
| const long maxBlobSize = 209715200L * 1024 * 1024; | |
| if (bytes.Length > maxBlobSize) | |
| throw new ArgumentException($"Content size {bytes.Length} bytes exceeds Azure's maximum blob size of {maxBlobSize} bytes"); | |
| if (destination.Compress) | |
| bytes = Compress(bytes); | |
| var tags = destination.Tags != null ? destination.Tags.ToDictionary(tag => tag.Name, tag => tag.Value) : new Dictionary<string, string>(); | |
| var uploadOptions = new BlobUploadOptions | |
| { | |
| Conditions = overwrite ? null : new BlobRequestConditions { IfNoneMatch = new ETag("*") }, | |
| HttpHeaders = new BlobHttpHeaders { ContentType = MimeUtility.GetMimeMapping(destination.Compress ? "gz" : destination.BlobName), ContentEncoding = encoding.WebName }, | |
| Tags = tags.Count > 0 ? tags : null | |
| }; | |
| return await blobClient.UploadAsync(BinaryData.FromBytes(bytes), uploadOptions, cancellationToken).ConfigureAwait(false); | |
| } |
| /// <summary> | ||
| /// Determines if the container should be created if it does not exist. | ||
| /// See https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata for naming rules. | ||
| /// </summary> | ||
| /// <example>false</example> | ||
| [UIHint(nameof(ConnectionMethod), "", ConnectionMethod.ConnectionString)] | ||
| [DefaultValue(false)] | ||
| public bool CreateContainerIfItDoesNotExist { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove ConnectionMethod restriction from CreateContainerIfItDoesNotExist.
The CreateContainerIfItDoesNotExist property is restricted to ConnectionString method, but container creation should be possible with other authentication methods that have sufficient permissions.
-[UIHint(nameof(ConnectionMethod), "", ConnectionMethod.ConnectionString)]
[DefaultValue(false)]
public bool CreateContainerIfItDoesNotExist { get; set; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Determines if the container should be created if it does not exist. | |
| /// See https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata for naming rules. | |
| /// </summary> | |
| /// <example>false</example> | |
| [UIHint(nameof(ConnectionMethod), "", ConnectionMethod.ConnectionString)] | |
| [DefaultValue(false)] | |
| public bool CreateContainerIfItDoesNotExist { get; set; } | |
| /// <summary> | |
| /// Determines if the container should be created if it does not exist. | |
| /// See https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata for naming rules. | |
| /// </summary> | |
| /// <example>false</example> | |
| [DefaultValue(false)] | |
| public bool CreateContainerIfItDoesNotExist { get; set; } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs (4)
24-24: Consider making the URI configurable via environment variables.The hard-coded URI
"https://stataskdevelopment.blob.core.windows.net"might not work across different environments. Consider moving it to an environment variable for better flexibility and maintainability.- private readonly string _uri = "https://stataskdevelopment.blob.core.windows.net"; + private readonly string _uri = Environment.GetEnvironmentVariable("Frends_AzureBlobStorage_Uri");
36-36: Consider using a more unique container naming strategy.The current container naming strategy using timestamp (
mmssffffff) could potentially cause collisions in parallel test execution scenarios. Consider incorporating a GUID for guaranteed uniqueness.- _containerName = $"test-container{DateTime.Now.ToString("mmssffffff", CultureInfo.InvariantCulture)}"; + _containerName = $"test-container-{Guid.NewGuid()}";
249-263: Optimize blob verification helper method.The
BlobExistsmethod creates multiple blob clients unnecessarily. The blob client created in line 253 could be reused instead of creating a new one in line 257.private async Task<bool> BlobExists(string containerName, string blobName, string expected) { var blobServiceClient = new BlobServiceClient(_connectionString); var container = blobServiceClient.GetBlobContainerClient(containerName); var blob = container.GetBlobClient(blobName); if (!blob.Exists()) return false; - var blobClient = new BlobClient(_connectionString, _destination.ContainerName, _destination.BlobName); - var blobDownload = await blobClient.DownloadAsync(); + var blobDownload = await blob.DownloadAsync(); using var reader = new StreamReader(blobDownload.Value.Content); var content = await reader.ReadToEndAsync(); return content == expected; }
16-18: Consider adding test parallelization configuration.The test fixture doesn't specify parallelization settings. Consider adding
[NonParallelizable]attribute if these tests shouldn't run in parallel, or ensure all resource names (containers, blobs) are unique if parallel execution is intended.[TestFixture] +[NonParallelizable] // Add this if parallel execution is not safe public class UnitTests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob.Tests/UnitTests.cs(1 hunks)Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Frends.AzureBlobStorage.WriteBlob/Frends.AzureBlobStorage.WriteBlob/WriteBlob.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build / Code Quality Check
- GitHub Check: build / Build on windows-latest
| [Test] | ||
| public async Task WriteBlob_Compress() | ||
| { | ||
| _destination.Compress = true; | ||
| var result = await AzureBlobStorage.WriteBlob(_source, _destination, _options, default); | ||
| Assert.IsTrue(result.Success); | ||
| Assert.AreEqual($"https://stataskdevelopment.blob.core.windows.net/{_destination.ContainerName}/{_destination.BlobName}", result.Uri); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance compression test assertions.
The compression test only verifies success and URI but doesn't validate that the content was actually compressed. Consider adding assertions to verify:
- The compressed size is smaller than the original
- The content can be decompressed correctly
[Test]
public async Task WriteBlob_Compress()
{
_destination.Compress = true;
var result = await AzureBlobStorage.WriteBlob(_source, _destination, _options, default);
Assert.IsTrue(result.Success);
Assert.AreEqual($"https://stataskdevelopment.blob.core.windows.net/{_destination.ContainerName}/{_destination.BlobName}", result.Uri);
+
+ // Verify compression
+ var blobClient = new BlobClient(_connectionString, _destination.ContainerName, _destination.BlobName);
+ var properties = await blobClient.GetPropertiesAsync();
+ Assert.That(properties.Value.ContentLength, Is.LessThan(Encoding.UTF8.GetByteCount(_testContent)));
+
+ // Verify content after decompression
+ Assert.IsTrue(await BlobExists(_destination.ContainerName, _destination.BlobName, _testContent));
}Committable suggestion skipped: line range outside the PR's diff.
#97
[1.0.0] - 2025-01-24
Added
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Testing