-
Notifications
You must be signed in to change notification settings - Fork 641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support blob hierarchy operations with a custom delimiter #10192
base: dev
Are you sure you want to change the base?
Conversation
The `AzureStorage` class now supports a delimiter for blob hierarchy operations, allowing it to handle directory-like structures more effectively by skipping directory prefixes. The `AzureStorageFactory` class has been updated to support the new delimiter parameter, ensuring that instances of `AzureStorage` can be created with this new functionality. Minor formatting improvements were made to the code comments.
@clairernovotny |
@@ -38,6 +40,7 @@ public AzureStorage( | |||
logger) | |||
{ | |||
_path = path; | |||
_delimiter = delimiter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no delimiter is passed, the blob listing will be flat, and you won't get hierarchical groupings like folders in the results. Now with delimiter
, it simulates hierarchy. Should its behavior change have a boolean flag to enable or disable the behavior?
|
||
public AzureStorage( | ||
BlobServiceClient account, | ||
string containerName, | ||
string path, | ||
string delimiter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the only delimiter allowed for storage accounts is /
, I'd say, there is no point to parameterize it. Having a constant should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By any chance it would be \
for non-Windows machines? Not sure about actual implementation of Azure storage machines, under the hood it's it's just another VM from my understanding.
@@ -171,8 +174,14 @@ public override IEnumerable<StorageListItem> List(bool getMetadata) | |||
blobTraits |= BlobTraits.Metadata; | |||
} | |||
|
|||
foreach (BlobHierarchyItem blob in _directory.GetBlobsByHierarchy(traits: blobTraits, prefix: _path)) | |||
foreach (BlobHierarchyItem blob in _directory.GetBlobsByHierarchy(traits: blobTraits, delimiter: _delimiter, prefix: _path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm running some tests and without the delimiter specified, GetBlobsByHierarchy
returns a list of blobs having certain prefix, complete, regardless of directory nesting. On storage accounts with hierarchical namespace enabled, it also includes directories themselves.
When delimiter is passed, it only returns immediate children under prefix AND if prefix ends in /
. If prefix does not end with /
, it returns a single item. In either case it does not return all blobs with a prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest introducing a separate method for listing only immediate children, so there is no ambiguity/confusion as to what it returns.
@clairernovotny |
PR Summary
Introduces a
delimiter
parameter to theAzureStorage
class and its methods for blob hierarchy operations.AzureStorage.cs
: Added_delimiter
field, updated constructor,List
, andListAsync
methods to usedelimiter
.AzureStorageFactory.cs
: Added_delimiter
field, updated constructor, andCreate
method to passdelimiter
.Part of https://github.com/NuGet/Engineering/issues/5645