Conversation
…, Error/Result refactor, ErrorHandler Breaking changes (2.x → 3.0): - Replace Source + Destination tabs with Input + Options - FileExistsAction.Error → Throw; FileEncoding.WINDOWS1252 → Windows1252; new UTF8WithBOM value - Result gains Success bool, FilePath (was FullPath), Error; loses FileName and Directory - Add Options.ThrowErrorOnFailure (default true) and ErrorMessageOnFailure - Add Helpers/ErrorHandler for standardised error handling - Bump version to 3.0.0; update CHANGELOG and unit tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR refactors the DownloadBlob task's public API from ChangesMajor API Refactoring (v2.0.0 → v3.0.0)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/ErrorHandlerTest.cs (3)
17-20: 💤 Low valueRedundant null assertion.
Assert.ThrowsAsync<Exception>already fails the test if no exception is thrown, soAssert.That(ex, Is.Not.Null)adds no coverage. Consider asserting on the exception'sMessage/InnerExceptioninstead to verify the error contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/ErrorHandlerTest.cs` around lines 17 - 20, Remove the redundant null check after Assert.ThrowsAsync in ErrorHandlerTest.cs: the Assert.ThrowsAsync<Exception>(() => AzureBlobStorage.DownloadBlob(DefaultInput(), DefaultOptions(), DefaultConnection(), CancellationToken.None)) already guarantees an exception, so remove Assert.That(ex, Is.Not.Null) and replace it with a meaningful assertion on the thrown exception (for example assert on ex.Message or ex.InnerException) to validate the error contract coming from AzureBlobStorage.DownloadBlob.
14-48: ⚡ Quick winTests depend on implicit task failure rather than exercising
ErrorHandlerdirectly.Despite the fixture name
ErrorHandlerTest, every test invokes the publicAzureBlobStorage.DownloadBlobwith emptyInput/Connectionand relies on whatever exception that throws first. This couples the tests to the internal validation order ofDownloadBlob— a future change (e.g., earlier argument validation, different exception type) could pass for the wrong reason or break unrelated tests. SinceErrorHandlerisinternal, exposing it viaInternalsVisibleToand unit-testingErrorHandler.Handledirectly would give precise, fast, and stable coverage of the new error contract, keeping the public-API tests inUnitTests.cs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/ErrorHandlerTest.cs` around lines 14 - 48, The tests in ErrorHandlerTest.cs should call ErrorHandler.Handle directly instead of invoking AzureBlobStorage.DownloadBlob; make ErrorHandler internal visible to the test assembly (add InternalsVisibleTo for the test project in the production assembly using AssemblyInfo or the project file) and update the tests to construct appropriate inputs/Exceptions and assert the returned/raised results from ErrorHandler.Handle (e.g., verify behavior when ThrowErrorOnFailure is true/false and ErrorMessageOnFailure is used) rather than depending on implicit failures from DownloadBlob.
22-29: ⚡ Quick winStrengthen failure-path assertions.
Asserting only
result.Success == falseleaves the newResult/Errorcontract largely unverified. Per coding guidelines for result classes, the failure path should also confirmFilePath,Error.Message, andError.AdditionalInfoare populated asErrorHandler.Handlepromises.♻️ Suggested additional assertions
var result = await AzureBlobStorage.DownloadBlob(DefaultInput(), options, DefaultConnection(), CancellationToken.None); Assert.That(result.Success, Is.False); + Assert.That(result.FilePath, Is.Null); + Assert.That(result.Error, Is.Not.Null); + Assert.That(result.Error.Message, Is.Not.Null.And.Not.Empty); + Assert.That(result.Error.AdditionalInfo, Is.InstanceOf<Exception>());Also consider adding a complementary test for the non-throw + custom message path, since
Should_Use_Custom_ErrorMessageOnFailureonly covers the throwing branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/ErrorHandlerTest.cs` around lines 22 - 29, The test Should_Return_Failed_Result_When_ThrowErrorOnFailure_Is_False only asserts result.Success is false; update it to also verify the failure contract by asserting Result.FilePath is non-null/empty, Result.Error.Message contains the expected message, and Result.Error.AdditionalInfo is populated as produced by ErrorHandler.Handle after calling AzureBlobStorage.DownloadBlob with options.ThrowErrorOnFailure = false; additionally add a complementary test (similar to Should_Use_Custom_ErrorMessageOnFailure) that exercises the non-throw branch with a custom error message and asserts Error.Message equals the custom message and AdditionalInfo is set.Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Helpers/ErrorHandler.cs (2)
10-15: ⚡ Quick winAvoid throwing bare
System.Exception; prefer rethrowing or a specific type.Wrapping every failure in
new Exception(...)discards the original exception type (e.g.,RequestFailedException,IOException), making it impossible for callers to catch by specific type. When no custom message is supplied, the wrapper adds nothing — the original exception should just propagate so the stack trace and type are preserved.♻️ Suggested refactor
if (throwOnFailure) { - if (string.IsNullOrEmpty(errorMessageOnFailure)) - throw new Exception(exception.Message, exception); - throw new Exception(errorMessageOnFailure, exception); + if (string.IsNullOrEmpty(errorMessageOnFailure)) + ExceptionDispatchInfo.Capture(exception).Throw(); + throw new Exception(errorMessageOnFailure, exception); }Add
using System.Runtime.ExceptionServices;if applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Helpers/ErrorHandler.cs` around lines 10 - 15, The current code wraps every failure in new Exception(...) losing the original exception type; change this so that when errorMessageOnFailure is null/empty you rethrow the original exception preserving type and stack (use ExceptionDispatchInfo.Capture(exception).Throw(); with using System.Runtime.ExceptionServices), and when a custom message is provided throw a more specific wrapper (e.g., InvalidOperationException or a domain-specific Exception) with the original exception as InnerException (replace new Exception(errorMessageOnFailure, exception) with new InvalidOperationException(errorMessageOnFailure, exception)); keep references to throwOnFailure, errorMessageOnFailure and exception variables in the ErrorHandler logic.
17-19: 💤 Low valueInconsistent message formatting between throw and non-throw paths.
When
throwOnFailure=truewith a custom message, the thrown exception'sMessageis justerrorMessageOnFailureand the original message is preserved only viaInnerException. WhenthrowOnFailure=false, the returnedError.Messagebecomes"{errorMessageOnFailure}: {exception.Message}". Consumers comparing strings or logs across the two modes will see different formats for the same underlying failure. Consider unifying — either include the original message in both, or in neither.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Helpers/ErrorHandler.cs` around lines 17 - 19, The formatting diverges between the throw and non-throw paths: build a single formatted string (use the existing errorMessage variable built from errorMessageOnFailure and exception.Message) and use that same string both for the returned Error.Message and as the message for the thrown exception when throwOnFailure is true (keep the original exception as InnerException). Update the throw path to use errorMessage instead of errorMessageOnFailure so thrown Exception.Message and Error.Message are identical; reference symbols: errorMessage, errorMessageOnFailure, exception, throwOnFailure, and the thrown Exception/returned Error in ErrorHandler.cs.Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/DownloadBlob.cs (1)
40-47: ⚡ Quick winReplace repeated
Split('.')calls withPathAPI methods; blob names with virtual paths will produce incorrect rename paths.Two separate problems here:
Virtual path in blob name (e.g.,
"subfolder/image.png"):blobFileName.Split('.')[…]leaves the directory prefix infileName, so the rename counter producessubfolder/image(1).png. When combined withinput.TargetDirectory, this resolves to a path whose intermediate directory doesn't exist, causingDownloadToAsyncto fail.Path.GetFileName(blobFileName)strips the prefix correctly.Multiple dots are handled correctly by the current code, but splitting the same string 5 times is fragile and noisy.
Path.GetExtension/Path.GetFileNameWithoutExtensionhandle all edge cases natively.♻️ Proposed refactor
- var blobFileName = string.IsNullOrWhiteSpace(input.TargetFileName) - ? input.BlobName - : input.TargetFileName; + var rawName = string.IsNullOrWhiteSpace(input.TargetFileName) + ? Path.GetFileName(input.BlobName) // strip virtual-directory prefix + : input.TargetFileName; + var blobFileName = rawName; var fullDestinationPath = Path.Combine(input.TargetDirectory, blobFileName); - var fileName = blobFileName.Split('.')[0]; - var fileExtension = ""; - - if (blobFileName.Split('.').Length > 1) - { - fileName = string.Join(".", blobFileName.Split('.').Take(blobFileName.Split('.').Length - 1).ToArray()); - fileExtension = "." + blobFileName.Split('.').Last(); - } + var fileName = Path.GetFileNameWithoutExtension(blobFileName); + var fileExtension = Path.GetExtension(blobFileName); // includes leading dot, or "" if none🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/DownloadBlob.cs` around lines 40 - 47, Replace the repeated blobFileName.Split('.') logic with System.IO.Path helpers: use Path.GetFileName(blobFileName) to strip any virtual folder prefix, then use Path.GetExtension(...) and Path.GetFileNameWithoutExtension(...) to derive fileExtension and fileName (respectively); update any rename/collision logic that currently uses fileName/fileExtension so it constructs filenames from these Path-derived values and ensures directories are created before calling DownloadToAsync. This change touches the blobFileName handling and the variables fileName and fileExtension (and the code that combines them when renaming/saving).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/UnitTests.cs`:
- Around line 220-227: The UploadTestFiles helper currently calls
BlobClient.UploadAsync(_testFilePath, default) which passes overwrite=false and
will fail if a previous test left the blob; update the UploadTestFiles method
(reference: UploadTestFiles and the blockBlob/BlobClient usage) to call
UploadAsync with overwrite: true (and keep the cancellation token/default for
the last parameter) so the upload succeeds even when the blob already exists.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Input.cs`:
- Around line 47-48: The property ActionOnExistingFile is only decorated with
[DefaultValue] (UI metadata) but lacks a runtime initializer, so set its default
explicitly by adding an initializer: change the property declaration for
ActionOnExistingFile to include " = FileExistsAction.Throw;" so programmatic
callers get the correct default (mirror how ThrowErrorOnFailure is initialized
in Options.cs).
- Around line 32-33: Add the [NotEmptyString] validation attribute to the
TargetDirectory property (same as ContainerName and BlobName) so empty string
input is rejected; update the TargetDirectory declaration to include
[NotEmptyString] and ensure the same validation namespace/import used by
ContainerName/BlobName is present so the attribute resolves correctly.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Options.cs`:
- Around line 16-17: The Encoding property on the Options class lacks a runtime
initializer so new Options() gets the enum's zero value instead of
FileEncoding.UTF8WithBOM; update the Encoding auto-property to include an
explicit initializer (= FileEncoding.UTF8WithBOM) similar to how
ThrowErrorOnFailure and ErrorMessageOnFailure use their default-value
initializers so both UI and programmatic callers see the intended default;
locate the Options class and the Encoding property and add the initializer
referencing the FileEncoding enum.
- Around line 25-26: The OtherEncoding property is left null which causes
GetEncoding(null) to produce a misleading error; initialize OtherEncoding to an
empty string to match the ErrorMessageOnFailure pattern and avoid null-flow when
Encoding == FileEncoding.Other—update the OtherEncoding auto-property in
Options.cs to have a default value of string.Empty (keeping GetEncoding in
DownloadBlob.cs and the Encoding/FileEncoding check unchanged).
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/DownloadBlob.cs`:
- Around line 117-119: The current loop using StreamReader.ReadLine() and
StreamWriter.WriteLine() (variables sr and sw) appends a spurious trailing
newline to the final line; replace the ReadLine/WriteLine loop with a verbatim
copy by calling sr.ReadToEnd() and writing that to sw (i.e., use
sw.Write(sr.ReadToEnd())) so the file content is copied byte-for-byte without
adding or removing line terminators.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/migration.json`:
- Around line 86-102: The migration currently maps Source.Encoding directly to
Options.Encoding and copies Source.FileEncodingString to Options.OtherEncoding
but ignores Source.EnableBOM, causing UTF8+EnableBOM to lose BOM; update the
migration.json to add a conditional mapping step that checks Source.Encoding ==
"UTF8" and Source.EnableBOM == true and sets Options.Encoding to "UTF8WithBOM"
(falling back to the existing Map rule for other cases), or if conditional steps
are not supported by your migration system, update the upgrade docs to
explicitly state that Source.EnableBOM must be manually re-applied to
Options.Encoding (UTF8WithBOM) after migration; reference the existing keys
Source.Encoding, Source.EnableBOM, Options.Encoding and the Map/Copy mapping
rules when making the change.
---
Nitpick comments:
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/ErrorHandlerTest.cs`:
- Around line 17-20: Remove the redundant null check after Assert.ThrowsAsync in
ErrorHandlerTest.cs: the Assert.ThrowsAsync<Exception>(() =>
AzureBlobStorage.DownloadBlob(DefaultInput(), DefaultOptions(),
DefaultConnection(), CancellationToken.None)) already guarantees an exception,
so remove Assert.That(ex, Is.Not.Null) and replace it with a meaningful
assertion on the thrown exception (for example assert on ex.Message or
ex.InnerException) to validate the error contract coming from
AzureBlobStorage.DownloadBlob.
- Around line 14-48: The tests in ErrorHandlerTest.cs should call
ErrorHandler.Handle directly instead of invoking AzureBlobStorage.DownloadBlob;
make ErrorHandler internal visible to the test assembly (add InternalsVisibleTo
for the test project in the production assembly using AssemblyInfo or the
project file) and update the tests to construct appropriate inputs/Exceptions
and assert the returned/raised results from ErrorHandler.Handle (e.g., verify
behavior when ThrowErrorOnFailure is true/false and ErrorMessageOnFailure is
used) rather than depending on implicit failures from DownloadBlob.
- Around line 22-29: The test
Should_Return_Failed_Result_When_ThrowErrorOnFailure_Is_False only asserts
result.Success is false; update it to also verify the failure contract by
asserting Result.FilePath is non-null/empty, Result.Error.Message contains the
expected message, and Result.Error.AdditionalInfo is populated as produced by
ErrorHandler.Handle after calling AzureBlobStorage.DownloadBlob with
options.ThrowErrorOnFailure = false; additionally add a complementary test
(similar to Should_Use_Custom_ErrorMessageOnFailure) that exercises the
non-throw branch with a custom error message and asserts Error.Message equals
the custom message and AdditionalInfo is set.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/DownloadBlob.cs`:
- Around line 40-47: Replace the repeated blobFileName.Split('.') logic with
System.IO.Path helpers: use Path.GetFileName(blobFileName) to strip any virtual
folder prefix, then use Path.GetExtension(...) and
Path.GetFileNameWithoutExtension(...) to derive fileExtension and fileName
(respectively); update any rename/collision logic that currently uses
fileName/fileExtension so it constructs filenames from these Path-derived values
and ensures directories are created before calling DownloadToAsync. This change
touches the blobFileName handling and the variables fileName and fileExtension
(and the code that combines them when renaming/saving).
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Helpers/ErrorHandler.cs`:
- Around line 10-15: The current code wraps every failure in new Exception(...)
losing the original exception type; change this so that when
errorMessageOnFailure is null/empty you rethrow the original exception
preserving type and stack (use ExceptionDispatchInfo.Capture(exception).Throw();
with using System.Runtime.ExceptionServices), and when a custom message is
provided throw a more specific wrapper (e.g., InvalidOperationException or a
domain-specific Exception) with the original exception as InnerException
(replace new Exception(errorMessageOnFailure, exception) with new
InvalidOperationException(errorMessageOnFailure, exception)); keep references to
throwOnFailure, errorMessageOnFailure and exception variables in the
ErrorHandler logic.
- Around line 17-19: The formatting diverges between the throw and non-throw
paths: build a single formatted string (use the existing errorMessage variable
built from errorMessageOnFailure and exception.Message) and use that same string
both for the returned Error.Message and as the message for the thrown exception
when throwOnFailure is true (keep the original exception as InnerException).
Update the throw path to use errorMessage instead of errorMessageOnFailure so
thrown Exception.Message and Error.Message are identical; reference symbols:
errorMessage, errorMessageOnFailure, exception, throwOnFailure, and the thrown
Exception/returned Error in ErrorHandler.cs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3ef65b0-a2df-44d2-90c9-126b05785f22
📒 Files selected for processing (14)
Frends.AzureBlobStorage.DownloadBlob/CHANGELOG.mdFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/ErrorHandlerTest.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/UnitTests.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Destination.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Enums.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Error.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Input.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Options.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Result.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Source.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/DownloadBlob.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.csprojFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Helpers/ErrorHandler.csFrends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/migration.json
💤 Files with no reviewable changes (2)
- Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Destination.cs
- Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Source.cs
| private async Task UploadTestFiles(string containerName) | ||
| { | ||
| var blobServiceClient = new BlobServiceClient(TestHelper.ConnectionString); | ||
| var container = blobServiceClient.GetBlobContainerClient(containerName); | ||
| await container.CreateIfNotExistsAsync(PublicAccessType.None, null, null); | ||
| var blockBlob = container.GetBlobClient(_testBlob); | ||
| await blockBlob.UploadAsync(_testFilePath, default); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the test file to understand the structure and context
find . -type f -name "UnitTests.cs" | grep -i "azureblobstorage"Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 836
🏁 Script executed:
# Once we find the file, let's read it to see the SetUp/TearDown and test structure
# First get the file path
fd -type f -name "UnitTests.cs" | grep -i blobstorageRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 253
🌐 Web query:
BlobClient.UploadAsync method signature Azure SDK .NET
💡 Result:
The BlobClient.UploadAsync method in the Azure.Storage.Blobs.NET SDK has multiple overloads. The official Microsoft documentation lists the following primary async overloads [1][2]: public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobContentInfo>> UploadAsync( string path, bool overwrite = false, System.Threading.CancellationToken cancellationToken = default); [1] public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobContentInfo>> UploadAsync( System.IO.Stream content, bool overwrite = false, System.Threading.CancellationToken cancellationToken = default); [1] public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobContentInfo>> UploadAsync( BinaryData content, bool overwrite = false, System.Threading.CancellationToken cancellationToken = default); [1] Additional overloads include those accepting BlobUploadOptions: public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobContentInfo>> UploadAsync( string path, Azure.Storage.Blobs.Models.BlobUploadOptions options, System.Threading.CancellationToken cancellationToken = default); [1] public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobContentInfo>> UploadAsync( System.IO.Stream content, Azure.Storage.Blobs.Models.BlobUploadOptions options, System.Threading.CancellationToken cancellationToken = default); [1] public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobContentInfo>> UploadAsync( BinaryData content, Azure.Storage.Blobs.Models.BlobUploadOptions options, System.Threading.CancellationToken cancellationToken = default); [1] These create a new block blob or overwrite an existing one if specified, returning BlobContentInfo [1]. Usage examples include uploading from file path, stream, or BinaryData [3].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/azure.storage.blobs.blobclient.uploadasync?view=azure-dotnet
- 2: https://learn.microsoft.com/en-us/dotnet/api/azure.storage.blobs.blobclient?view=azure-dotnet
- 3: https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blob-upload
🏁 Script executed:
cat -n ./Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/UnitTests.cs | head -100Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 4218
🏁 Script executed:
# Get the file size first to understand its extent
wc -l ./Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/UnitTests.csRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 181
🏁 Script executed:
# Look for SetUp, TearDown, and UploadTestFiles context
grep -n -A 10 "SetUp\|TearDown\|UploadTestFiles" ./Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/UnitTests.cs | head -150Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 1722
Pass overwrite: true to ensure blob upload succeeds even if a previous test left the blob in the container.
BlobClient.UploadAsync(string path, bool overwrite = false, ...) defaults the second positional parameter to overwrite: false. When default is passed as the second argument, it resolves to false, which causes a RequestFailedException if the blob already exists. The TearDown uses DeleteBlobsInContainer() to empty the const container (_container), but if cleanup fails or is incomplete, the next SetUp call will fail when attempting to re-upload the blob.
Proposed fix
- await blockBlob.UploadAsync(_testFilePath, default);
+ await blockBlob.UploadAsync(_testFilePath, overwrite: true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob.Tests/UnitTests.cs`
around lines 220 - 227, The UploadTestFiles helper currently calls
BlobClient.UploadAsync(_testFilePath, default) which passes overwrite=false and
will fail if a previous test left the blob; update the UploadTestFiles method
(reference: UploadTestFiles and the blockBlob/BlobClient usage) to call
UploadAsync with overwrite: true (and keep the cancellation token/default for
the last parameter) so the upload succeeds even when the blob already exists.
There was a problem hiding this comment.
@MichalFrends1 lets add that, no harm and give us more stable environment
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| [DisplayFormat(DataFormatString = "Text")] | ||
| public string TargetDirectory { get; set; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for ValidationHandler and see if TargetDirectory is validated there
ast-grep --pattern 'class ValidationHandler {
$$$
}'
rg -n "TargetDirectory" --type csRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 1134
🏁 Script executed:
cat -n Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Input.csRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 2187
🏁 Script executed:
rg -n "ValidationHandler" --type cs -A 20Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 50394
🏁 Script executed:
rg -n "\[NotEmptyString\]" --type cs -B 1Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 3278
Add [NotEmptyString] validation attribute to TargetDirectory.
ContainerName and BlobName are both guarded with [NotEmptyString], but TargetDirectory is not. An empty string silently resolves Path.Combine("", blobFileName) to a relative path in the working directory, which is almost never the intended behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Input.cs`
around lines 32 - 33, Add the [NotEmptyString] validation attribute to the
TargetDirectory property (same as ContainerName and BlobName) so empty string
input is rejected; update the TargetDirectory declaration to include
[NotEmptyString] and ensure the same validation namespace/import used by
ContainerName/BlobName is present so the attribute resolves correctly.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| [DefaultValue(FileExistsAction.Throw)] | ||
| public FileExistsAction ActionOnExistingFile { get; set; } |
There was a problem hiding this comment.
ActionOnExistingFile is missing an explicit property initializer.
[DefaultValue] is UI/tooling metadata only — it does not set the runtime value. ThrowErrorOnFailure in Options.cs correctly pairs its [DefaultValue(true)] with = true. The same pattern should be applied here; if FileExistsAction.Throw is not ordinal 0, programmatic callers get a silently wrong default.
🛠 Proposed fix
- public FileExistsAction ActionOnExistingFile { get; set; }
+ public FileExistsAction ActionOnExistingFile { get; set; } = FileExistsAction.Throw;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Input.cs`
around lines 47 - 48, The property ActionOnExistingFile is only decorated with
[DefaultValue] (UI metadata) but lacks a runtime initializer, so set its default
explicitly by adding an initializer: change the property declaration for
ActionOnExistingFile to include " = FileExistsAction.Throw;" so programmatic
callers get the correct default (mirror how ThrowErrorOnFailure is initialized
in Options.cs).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| [DefaultValue(FileEncoding.UTF8WithBOM)] | ||
| public FileEncoding Encoding { get; set; } |
There was a problem hiding this comment.
Encoding is missing an explicit property initializer — programmatic callers silently get the wrong default.
[DefaultValue] is consumed by the Frends UI tooling only. ThrowErrorOnFailure and ErrorMessageOnFailure both pair their [DefaultValue] with a = initializer precisely to cover runtime callers. Encoding skips this: new Options() will resolve to whatever enum ordinal 0 is, which may not be FileEncoding.UTF8WithBOM.
🛠 Proposed fix
- public FileEncoding Encoding { get; set; }
+ public FileEncoding Encoding { get; set; } = FileEncoding.UTF8WithBOM;📝 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.
| [DefaultValue(FileEncoding.UTF8WithBOM)] | |
| public FileEncoding Encoding { get; set; } | |
| [DefaultValue(FileEncoding.UTF8WithBOM)] | |
| public FileEncoding Encoding { get; set; } = FileEncoding.UTF8WithBOM; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Options.cs`
around lines 16 - 17, The Encoding property on the Options class lacks a runtime
initializer so new Options() gets the enum's zero value instead of
FileEncoding.UTF8WithBOM; update the Encoding auto-property to include an
explicit initializer (= FileEncoding.UTF8WithBOM) similar to how
ThrowErrorOnFailure and ErrorMessageOnFailure use their default-value
initializers so both UI and programmatic callers see the intended default;
locate the Options class and the Encoding property and add the initializer
referencing the FileEncoding enum.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| [DisplayFormat(DataFormatString = "Text")] | ||
| public string OtherEncoding { get; set; } |
There was a problem hiding this comment.
OtherEncoding is uninitialized — inconsistent with the ErrorMessageOnFailure pattern and exposes a null-flow risk.
ErrorMessageOnFailure is explicitly initialized to string.Empty; OtherEncoding is left at null. When Encoding == FileEncoding.Other and OtherEncoding is never set, GetEncoding(null) in DownloadBlob.cs receives null, which results in a somewhat misleading error message "Invalid encoding string '': …" instead of a clear null validation message.
🛠 Proposed fix
- public string OtherEncoding { get; set; }
+ public string OtherEncoding { get; set; } = string.Empty;📝 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.
| [DisplayFormat(DataFormatString = "Text")] | |
| public string OtherEncoding { get; set; } | |
| [DisplayFormat(DataFormatString = "Text")] | |
| public string OtherEncoding { get; set; } = string.Empty; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/Definitions/Options.cs`
around lines 25 - 26, The OtherEncoding property is left null which causes
GetEncoding(null) to produce a misleading error; initialize OtherEncoding to an
empty string to match the ErrorMessageOnFailure pattern and avoid null-flow when
Encoding == FileEncoding.Other—update the OtherEncoding auto-property in
Options.cs to have a default value of string.Empty (keeping GetEncoding in
DownloadBlob.cs and the Encoding/FileEncoding check unchanged).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| { | ||
| "Type": "Map", | ||
| "Source": "Source.Encoding", | ||
| "Target": "Options.Encoding", | ||
| "Maps": { | ||
| "UTF8": "UTF8", | ||
| "Default": "Default", | ||
| "ASCII": "ASCII", | ||
| "WINDOWS1252": "Windows1252", | ||
| "Other": "Other" | ||
| } | ||
| }, | ||
| { | ||
| "Type": "Copy", | ||
| "Source": "Source.FileEncodingString", | ||
| "Target": "Options.OtherEncoding" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect existing migration.json patterns in sibling tasks for conditional/composite mapping idioms
fd -t f migration.json | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 15165
🏁 Script executed:
# Find CHANGELOG file
fd -t f CHANGELOG | head -5Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 344
🏁 Script executed:
# Search for EnableBOM references in the codebase
rg -i "enablebom" --type-listRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 472
🏁 Script executed:
# Find the DownloadBlob task definition to check Encoding enum and EnableBOM field
fd -t f -i "downloadblob" | grep -E "\.(cs|json)$" | head -10Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 172
🏁 Script executed:
# Check DownloadBlob CHANGELOG for EnableBOM and UTF8WithBOM references
cat Frends.AzureBlobStorage.DownloadBlob/CHANGELOG.mdRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 3930
🏁 Script executed:
# Search for EnableBOM anywhere in the codebase
rg -i "enablebom"Repository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 2460
🏁 Script executed:
# Check the DownloadBlob.cs task definition for Encoding enum and EnableBOM field
cat Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/DownloadBlob.csRepository: FrendsPlatform/Frends.AzureBlobStorage
Length of output: 6735
Migration silently loses EnableBOM=true behavior.
The CHANGELOG (3.0.0 upgrade instruction step 5) documents converting Source.Encoding=UTF8 + EnableBOM=true to Options.Encoding=UTF8WithBOM, but the migration.json only maps Source.Encoding directly and never examines the removed Source.EnableBOM field. Existing tasks with UTF-8 + BOM will migrate to UTF-8 without BOM, which contradicts the documented upgrade path.
The migration system does not appear to support conditional mapping across the codebase. Either implement a conditional migration step that maps to UTF8WithBOM when Source.EnableBOM is true and Source.Encoding is UTF8, or explicitly document this limitation in the upgrade instructions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.AzureBlobStorage.DownloadBlob/Frends.AzureBlobStorage.DownloadBlob/migration.json`
around lines 86 - 102, The migration currently maps Source.Encoding directly to
Options.Encoding and copies Source.FileEncodingString to Options.OtherEncoding
but ignores Source.EnableBOM, causing UTF8+EnableBOM to lose BOM; update the
migration.json to add a conditional mapping step that checks Source.Encoding ==
"UTF8" and Source.EnableBOM == true and sets Options.Encoding to "UTF8WithBOM"
(falling back to the existing Map rule for other cases), or if conditional steps
are not supported by your migration system, update the upgrade docs to
explicitly state that Source.EnableBOM must be manually re-applied to
Options.Encoding (UTF8WithBOM) after migration; reference the existing keys
Source.Encoding, Source.EnableBOM, Options.Encoding and the Map/Copy mapping
rules when making the change.
There was a problem hiding this comment.
@MichalFrends1 well, it's kinda true that's case we dont support with current migration logic, maybe ask Jefim if that's ok to just leave it or maybe add some note or idk
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| - Added `Result.Success` (bool) and `Result.Error` properties. | ||
| - Added `Options.ThrowErrorOnFailure` (default `true`) and `Options.ErrorMessageOnFailure` for non-throwing error handling. | ||
|
|
||
| ### Upgrade instructions (2.x → 3.0) |
There was a problem hiding this comment.
remove those instrucions - we dont do that in changelog
| - [Breaking] `Options.Encoding` replaces `Source.Encoding`; added `UTF8WithBOM` value; `WINDOWS1252` renamed to `Windows1252`. | ||
| - [Breaking] `Options.OtherEncoding` replaces `Source.FileEncodingString`; separate `EnableBOM` property removed (use `UTF8WithBOM`). | ||
| - [Breaking] `Result.FilePath` replaces `Result.FullPath`; `Result.FileName` and `Result.Directory` removed. | ||
| - Added `Result.Success` (bool) and `Result.Error` properties. |
There was a problem hiding this comment.
add second "Added" header and move there what was added
3 participants
Summary
Harmonises
Frends.AzureBlobStorage.DownloadBlobto align with Frends Task Development Guidelines (FSPW-395).Breaking changes (major version bump: → 3.0.0)
SourceandDestinationtabs removed; fields consolidated into newInputtabFileExistsAction.Errorrenamed toFileExistsAction.ThrowFileEncoding.WINDOWS1252renamed toFileEncoding.Windows1252Result.FileNameandResult.Directoryremoved;Result.FullPathrenamed toResult.FilePathSourceto newOptionstab;EnableBOMremoved in favour ofFileEncoding.UTF8WithBOMNew features
Optionstab withEncoding(defaultUTF8WithBOM),OtherEncoding,ThrowErrorOnFailure(defaulttrue),ErrorMessageOnFailureResult.Success(bool) addedResult.ErrorwithMessageandAdditionalInfoaddedErrorHandlerpattern implementedFiles changed
Definitions/Input.cs— new, replaces Source + DestinationDefinitions/Options.cs— newDefinitions/Error.cs— newHelpers/ErrorHandler.cs— newDefinitions/Enums.cs— enum renames + UTF8WithBOM addedDefinitions/Result.cs— restructuredDownloadBlob.cs— signature + implementation updatedDefinitions/Source.cs— deletedDefinitions/Destination.cs— deletedCHANGELOG.md— updated*.csproj— version bumped to 3.0.0UnitTests.cs— migrated + new tests addedTest plan
dotnet build)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
Tests