Issue 92#95
Conversation
…olderName to RenameToFolderName, modifed UploadBlob method and existing tests to cover the TargetFolder field.
WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (6)
Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/Definitions/Destination.cs (1)
Line range hint
134-141: Update HandleExistingFile documentation to clarify target folder behavior.The
HandleExistingFileproperty's documentation should be updated to clarify how it interacts with the newTargetFolderproperty.Apply this diff to enhance the documentation:
/// <summary> /// How the existing blob will be handled. /// Append: Append the blob with Source.SourceFile. Block and Page blobs will be downloaded as a temp file which will be deleted after local append and upload processes are complete. No downloading needed for Append Blob. /// Overwrite: The original blob will be deleted before uploading the new one. /// Error: Depending on Options.ThrowErrorOnFailure, throw an exception or Result will contain an error message instead of the blob's URL. + /// Note: When TargetFolder is specified, the existence check and operations will be performed on the full path including the target folder. /// </summary> /// <example>HandleExistingFile.Error</example> [DefaultValue(HandleExistingFile.Error)] public HandleExistingFile HandleExistingFile { get; set; }Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/Definitions/Source.cs (1)
44-57: Consider adding migration guidance for breaking changes.Both property renames (
BlobName→RenameToBlobNameandBlobFolderName→RenameToFolderName) are breaking changes that improve clarity but may affect existing users. Consider adding migration guidance in the changelog or documentation to help users update their code.Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/UploadBlob.cs (2)
62-65: Trim leading slashes fromTargetFolderto avoid unintended pathsIf
TargetFoldercontains leading slashes, it may result in incorrect blob paths. Consider trimming both leading and trailing slashes.Apply this diff:
- blobName = $"{destination.TargetFolder.TrimEnd('/', '\\')}/{blobName}"; + blobName = $"{destination.TargetFolder.Trim('/', '\\')}/{blobName}";
84-88: Consistent trimming of slashes forTargetFolderin directory uploadsSimilar to previous, consider trimming both leading and trailing slashes from
TargetFolderto maintain consistent blob paths when uploading directories.Apply this diff:
- blobName = $"{destination.TargetFolder.TrimEnd('/', '\\')}/{blobName}"; + blobName = $"{destination.TargetFolder.Trim('/', '\\')}/{blobName}";Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob.Tests/UnitTests.cs (2)
20-20: Avoid hardcoding storage account name in testsConsider retrieving the storage account name from an environment variable to improve flexibility and security.
Apply this diff:
- private readonly string _storageAccount = "stataskdevelopment"; + private readonly string _storageAccount = Environment.GetEnvironmentVariable("Frends_AzureBlobStorage_StorageAccount");
291-293: Clarify the use of empty string forTargetFolderSetting
TargetFolderto an empty string may cause unintended behavior. Consider usingnullor adding validation to handle this scenario.Apply this diff:
- _destinationCS.TargetFolder = ""; + _destinationCS.TargetFolder = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Frends.AzureBlobStorage.UploadBlob/CHANGELOG.md(1 hunks)Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob.Tests/UnitTests.cs(11 hunks)Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/Definitions/Destination.cs(1 hunks)Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/Definitions/Source.cs(1 hunks)Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/UploadBlob.cs(4 hunks)
🔇 Additional comments (7)
Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/Definitions/Destination.cs (1)
64-69: Verify the impact of this new property on existing blob operations.
The addition of TargetFolder could affect how blobs are stored and accessed. Let's verify the implementation details.
✅ Verification successful
Let me gather more information about how this new property is being used in the implementation.
Based on the results, I can now provide a final response:
The TargetFolder property implementation is properly handled and tested
The implementation shows that:
- The property is correctly used to prepend folder paths to blob names during upload
- Path separators are properly normalized using
TrimEnd('/', '\\') - The feature is well-tested in UnitTests.cs with various scenarios:
- Null/empty target folder
- Target folder with actual value
- Both connection string and OAuth2 authentication methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing blob path handling and potential impacts
# Look for existing blob path construction logic
rg -l 'blob.*path|path.*blob' | while read -r file; do
echo "=== $file ==="
rg -A 5 'blob.*path|path.*blob' "$file"
done
# Check for existing tests related to blob paths
ast-grep --pattern 'class $_ {
$$$
[Test]
$$$
void $_($$$) {
$$$
blob$$$path$$$
$$$
}
$$$
}'
Length of output: 2229
Script:
#!/bin/bash
# Check the implementation to see how TargetFolder is used
rg -A 10 "TargetFolder"
# Look for blob name construction or manipulation
ast-grep --pattern 'BlobClient($$$)'
# Check for tests that might verify the TargetFolder functionality
fd -e cs test -x rg -l "TargetFolder" {}
Length of output: 13992
Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/Definitions/Source.cs (2)
52-57: Fix documentation typo and verify breaking change.
The rename from BlobFolderName to RenameToFolderName improves clarity, but there's a typo in the documentation (double period after "Storage..").
Apply this fix:
- /// Specifies a custom name for the blob folder in Azure Blob Storage.. If left empty, blob folder's name will be the same as source directory (e.g., 'Example' in C:\temp\Example).
+ /// Specifies a custom name for the blob folder in Azure Blob Storage. If left empty, blob folder's name will be the same as source directory (e.g., 'Example' in C:\temp\Example).Let's verify the impact of this breaking change:
✅ Verification successful
Fix documentation typo in property comment.
The property rename from BlobFolderName to RenameToFolderName is properly documented in the changelog as part of version 3.0.0. The breaking change is well-communicated and justified for improved clarity. Only the documentation typo needs to be fixed.
- /// Specifies a custom name for the blob folder in Azure Blob Storage.. If left empty, blob folder's name will be the same as source directory (e.g., 'Example' in C:\temp\Example).
+ /// Specifies a custom name for the blob folder in Azure Blob Storage. If left empty, blob folder's name will be the same as source directory (e.g., 'Example' in C:\temp\Example).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old property name
rg "BlobFolderName" --type cs
# Check if the breaking change is documented in changelog
cat CHANGELOG.md
Length of output: 93
Script:
#!/bin/bash
# Let's check if there's a changelog file with a different name pattern
fd -i changelog
# Search for any references to the old property name in the entire codebase
rg "BlobFolderName" -l
# Let's also check the test files specifically for any usage
fd -e cs -e Tests test
Length of output: 1953
Script:
#!/bin/bash
# Let's check the changelog content for this specific component
cat "Frends.AzureBlobStorage.UploadBlob/CHANGELOG.md"
# Let's also check the actual occurrences of BlobFolderName with context
rg "BlobFolderName" -C 2
Length of output: 2856
44-49: Property rename improves clarity but requires verification of breaking changes.
The rename from BlobName to RenameToBlobName makes the property's purpose more explicit. The updated documentation clearly explains the default behavior.
Let's verify the impact of this breaking change:
✅ Verification successful
The previous search revealed:
- The property rename is consistently updated in the UploadBlob project
- Other projects (ReadBlob, DownloadBlob, DeleteBlob) use their own
BlobNameproperties which are unrelated - No CHANGELOG.md found, so we need to check other documentation files for breaking changes
Let's verify if there's documentation about breaking changes in other locations.
Let's try a different approach to locate and check documentation files. The previous fd command had syntax issues, so switching to find for basic file discovery.
Let's check the changelog for breaking changes documentation and any remaining references to the old property name in the documentation.
Property rename is properly documented as a breaking change
The rename from BlobName to RenameToBlobName is properly documented in the changelog under version 3.0.0 as a breaking change. The code changes are consistent across the UploadBlob project, and the property is not referenced by other projects in the solution. No further action is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old property name
rg "BlobName" --type cs
# Check if the breaking change is documented in changelog
cat CHANGELOG.md
Length of output: 7204
Script:
#!/bin/bash
# Check for changelog or release notes in other common locations
fd -g "*CHANGELOG*" -g "*RELEASE*" -g "*HISTORY*" -i
# Check for documentation files that might contain breaking changes
fd -g "README*" -g "BREAKING*" -g "UPGRADE*" -i
Length of output: 409
Script:
#!/bin/bash
# Search for documentation files from the repository root
find . -type f -iname "README*" -o -iname "CHANGELOG*" -o -iname "RELEASE*" -o -iname "HISTORY*"
# Check content of README if found in the UploadBlob project directory
cat Frends.AzureBlobStorage.UploadBlob/README.md 2>/dev/null
Length of output: 1640
Script:
#!/bin/bash
# Check the changelog content for the UploadBlob project
cat Frends.AzureBlobStorage.UploadBlob/CHANGELOG.md
# Check if there are any references to the old property name in documentation
rg "BlobName" Frends.AzureBlobStorage.UploadBlob/README.md Frends.AzureBlobStorage.UploadBlob/CHANGELOG.md
Length of output: 2204
Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob/UploadBlob.cs (2)
60-61: Blob renaming logic is appropriate
The updated logic correctly handles renaming the blob when RenameToBlobName is specified or when compression is enabled.
78-80: Ensure RenameToFolderName is validated properly
When using source.RenameToFolderName, validate it for any invalid path characters to prevent potential errors during path construction.
Frends.AzureBlobStorage.UploadBlob/CHANGELOG.md (1)
3-7: Changelog updates are clear and well-documented
The addition of version 3.0.0 with detailed descriptions of added and changed features enhances the clarity of the project's history.
Frends.AzureBlobStorage.UploadBlob/Frends.AzureBlobStorage.UploadBlob.Tests/UnitTests.cs (1)
144-150: Verify handling of invalid FileEncoding in tests
The FileEncoding is set to "foo/bar", which is not a standard encoding. Ensure that the test is designed to handle this case and asserts the expected behavior.
| /// <summary> | ||
| /// Specifies the target folder path for the blob in Azure Blob Storage. | ||
| /// This value will be prepended to blob names during upload, effectively creating a virtual directory structure. | ||
| /// </summary> | ||
| /// <example>backups/2024/</example> | ||
| public string TargetFolder { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and improve documentation for the TargetFolder property.
While the property implementation is functionally correct, consider these improvements:
- Add validation to ensure proper folder path format
- Document path format requirements (e.g., handling of slashes)
- Add UI hints for better user experience
Apply this diff to enhance the property:
/// <summary>
/// Specifies the target folder path for the blob in Azure Blob Storage.
/// This value will be prepended to blob names during upload, effectively creating a virtual directory structure.
+ /// The path should:
+ /// - Use forward slashes (/) as directory separators
+ /// - Not start with a forward slash
+ /// - Optionally end with a forward slash (will be normalized during upload)
+ /// - Not contain invalid characters: \ " [ ] < > |
/// </summary>
/// <example>backups/2024/</example>
+ [DisplayFormat(DataFormatString = "Text")]
+ [RegularExpression(@"^[^\\<>\[\]""|]+(\/[^\\<>\[\]""|]+)*\/?$",
+ ErrorMessage = "Invalid folder path format. Use forward slashes and avoid special characters.")]
+ [Description("Virtual directory path where the blob will be stored.")]
public string TargetFolder { 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> | |
| /// Specifies the target folder path for the blob in Azure Blob Storage. | |
| /// This value will be prepended to blob names during upload, effectively creating a virtual directory structure. | |
| /// </summary> | |
| /// <example>backups/2024/</example> | |
| public string TargetFolder { get; set; } | |
| /// <summary> | |
| /// Specifies the target folder path for the blob in Azure Blob Storage. | |
| /// This value will be prepended to blob names during upload, effectively creating a virtual directory structure. | |
| /// The path should: | |
| /// - Use forward slashes (/) as directory separators | |
| /// - Not start with a forward slash | |
| /// - Optionally end with a forward slash (will be normalized during upload) | |
| /// - Not contain invalid characters: \ " [ ] < > | | |
| /// </summary> | |
| /// <example>backups/2024/</example> | |
| [DisplayFormat(DataFormatString = "Text")] | |
| [RegularExpression(@"^[^\\<>\[\]""|]+(\/[^\\<>\[\]""|]+)*\/?$", | |
| ErrorMessage = "Invalid folder path format. Use forward slashes and avoid special characters.")] | |
| [Description("Virtual directory path where the blob will be stored.")] | |
| public string TargetFolder { get; set; } |
| if (destination.ContainerName.Contains('/') || destination.ContainerName.Contains('\\')) | ||
| { | ||
| throw new Exception("The container name cannot contain '/' or '\\'."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more specific exception type for container name validation
Instead of throwing a general Exception, consider throwing an ArgumentException to provide more context about the invalid ContainerName parameter.
Apply this diff to improve exception handling:
- throw new Exception("The container name cannot contain '/' or '\\'.");
+ throw new ArgumentException("The container name cannot contain '/' or '\\'.", nameof(destination.ContainerName));📝 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.
| if (destination.ContainerName.Contains('/') || destination.ContainerName.Contains('\\')) | |
| { | |
| throw new Exception("The container name cannot contain '/' or '\\'."); | |
| } | |
| if (destination.ContainerName.Contains('/') || destination.ContainerName.Contains('\\')) | |
| { | |
| throw new ArgumentException("The container name cannot contain '/' or '\\'.", nameof(destination.ContainerName)); | |
| } |
| string normalizedBlobName = blobName.Replace('/', '\\'); | ||
|
|
||
| var tempFile = Path.Combine(Path.GetTempPath(), normalizedBlobName); | ||
|
|
||
| string directoryPath = Path.GetDirectoryName(tempFile); | ||
|
|
||
| if (!Directory.Exists(directoryPath)) | ||
| { | ||
| Directory.CreateDirectory(directoryPath); | ||
| } |
There was a problem hiding this comment.
Validate blobName to prevent path traversal vulnerabilities
Using blobName directly in file paths without validation can lead to security issues if it contains path traversal characters. Sanitize blobName to ensure only valid file names are used when creating temporary files.
Apply this diff to sanitize blobName:
- string normalizedBlobName = blobName.Replace('/', '\\');
+ string normalizedBlobName = Path.GetFileName(blobName.Replace('/', '\\'));This change uses Path.GetFileName to extract only the file name, preventing directory traversal.
📝 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.
| string normalizedBlobName = blobName.Replace('/', '\\'); | |
| var tempFile = Path.Combine(Path.GetTempPath(), normalizedBlobName); | |
| string directoryPath = Path.GetDirectoryName(tempFile); | |
| if (!Directory.Exists(directoryPath)) | |
| { | |
| Directory.CreateDirectory(directoryPath); | |
| } | |
| string normalizedBlobName = Path.GetFileName(blobName.Replace('/', '\\')); | |
| var tempFile = Path.Combine(Path.GetTempPath(), normalizedBlobName); | |
| string directoryPath = Path.GetDirectoryName(tempFile); | |
| if (!Directory.Exists(directoryPath)) | |
| { | |
| Directory.CreateDirectory(directoryPath); | |
| } |
1 participant
issue 92
Summary by CodeRabbit
New Features
Destination.TargetFolder, allowing users to specify a target folder for blob uploads.Improvements
Source.BlobNametoSource.RenameToBlobNameSource.BlobFolderNametoSource.RenameToFolderNameBug Fixes
ContainerNameto prevent invalid characters.