Support prefix in storage listing#2378
Conversation
There was a problem hiding this comment.
Pull request overview
Adds --prefix support to Azure Storage listing operations so users can scope container and blob listings by name prefix.
Changes:
- Added
--prefixoption wiring forstorage_blob_get(blob listing) andstorage_blob_container_get(container listing). - Updated
IStorageService/StorageServicelisting APIs to accept aprefixparameter and pass it into Azure SDK list calls. - Added/updated unit tests, live tests, and command documentation to cover and describe prefix filtering.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Storage/src/Services/StorageService.cs | Implements prefix filtering in Azure SDK list calls; also refactors SKU/tier validation sets. |
| tools/Azure.Mcp.Tools.Storage/src/Services/IStorageService.cs | Extends service interface APIs with optional prefix parameter. |
| tools/Azure.Mcp.Tools.Storage/src/Options/StorageOptionDefinitions.cs | Introduces --prefix option definitions for blob/container listing commands. |
| tools/Azure.Mcp.Tools.Storage/src/Options/Blob/BlobGetOptions.cs | Adds Prefix to blob get options payload. |
| tools/Azure.Mcp.Tools.Storage/src/Options/Blob/Container/ContainerGetOptions.cs | Adds Prefix to container get options payload. |
| tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/BlobGetCommand.cs | Registers/binds --prefix and forwards it to the storage service; updates description text. |
| tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/Container/ContainerGetCommand.cs | Registers/binds --prefix and forwards it to the storage service; updates description text. |
| tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/Container/ContainerCreateCommand.cs | Description formatting update. |
| tools/Azure.Mcp.Tools.Storage/src/Models/BlobInfo.cs | Adds explicit namespace declaration. |
| tools/Azure.Mcp.Tools.Storage/src/Models/ContainerInfo.cs | Adds explicit namespace declaration. |
| tools/Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.UnitTests/Blob/BlobGetCommandTests.cs | Adds/updates unit coverage for blob get/list flows (includes prefix parsing cases). |
| tools/Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.UnitTests/Blob/Container/ContainerGetCommandTests.cs | Adds/updates unit coverage for container get/list flows (includes prefix parsing cases). |
| tools/Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.UnitTests/Blob/Container/ContainerCreateCommandTests.cs | Adds unit coverage for container create command. |
| tools/Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.LiveTests/StorageCommandTests.cs | Adds live tests validating prefix-filtered listing for blobs and containers. |
| tools/Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.LiveTests/assets.json | Updates recorded test assets tag. |
| servers/Azure.Mcp.Server/docs/e2eTestPrompts.md | Adds e2e prompts for prefix-filtered listing scenarios. |
| servers/Azure.Mcp.Server/docs/azmcp-commands.md | Updates CLI docs to include --prefix for relevant commands. |
tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/BlobGetCommand.cs
Outdated
Show resolved
Hide resolved
tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/Container/ContainerGetCommand.cs
Outdated
Show resolved
Hide resolved
.../Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.UnitTests/Blob/BlobGetCommandTests.cs
Show resolved
Hide resolved
...s.Storage/tests/Azure.Mcp.Tools.Storage.UnitTests/Blob/Container/ContainerGetCommandTests.cs
Show resolved
Hide resolved
jongio
left a comment
There was a problem hiding this comment.
Prefix support looks clean - nice use of the SDK's native prefix: parameter. The bundled refactoring (HashSet extraction, namespace additions, description reformatting) and new test suites are welcome additions.
One behavioral note and a couple of nits inline.
tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/Container/ContainerGetCommand.cs
Show resolved
Hide resolved
jongio
left a comment
There was a problem hiding this comment.
Core prefix implementation is correct - uses the SDK's native prefix: parameter cleanly. A few things to address:
Constructor injection divergence (BlobGetCommand.cs, ContainerGetCommand.cs):
Every other command in this project resolves IStorageService from the context via context.GetService<IStorageService>() (AccountGetCommand, AccountCreateCommand, BlobUploadCommand, ContainerCreateCommand, TableListCommand). This PR switches BlobGetCommand and ContainerGetCommand to constructor injection while leaving the other five commands unchanged. Both patterns work at runtime since IStorageService is registered as a singleton, but having two patterns in the same project is confusing for contributors. The test setup also diverges - new tests pass the mock via constructor while existing tests register mocks in the service collection. I'd pick one approach and apply it consistently. If constructor injection is the direction, that's a good separate PR that converts all commands at once.
Inconsistent type qualification (StorageService.cs):
GetBlobDetails uses Storage.Models.BlobInfo (partially qualified) in its return type and list construction, but GetContainerDetails in the same file uses ContainerInfo (unqualified). Both types are in Azure.Mcp.Tools.Storage.Models and both are imported via the using directive at the top. The partial qualification is unnecessary and inconsistent - just use BlobInfo directly.
Prefix test gap (BlobGetCommandTests.cs, ContainerGetCommandTests.cs):
The prefix feature is the main deliverable of this PR, but there's no dedicated test that verifies the prefix value actually reaches the service call. The InlineData theory includes --prefix prefix cases but uses Arg.Any<string?>() for the prefix parameter, so it only verifies the command doesn't crash - not that the value is forwarded. Consider adding a test like ExecuteAsync_WithPrefix_PassesPrefixToService that passes --prefix myprefix and uses Arg.Is("myprefix") to verify the service receives it.
Minor:
ContainerGetCommand.cs: double blank line between the license header and the first using; blank line between namespace and class was removed. The rest of the project uses single blank line and keeps the namespace/class separator.- This PR bundles several unrelated changes (HashSet refactor, tenant bug fix, namespace additions, changelog script fix, description reformatting). Consider splitting out the unrelated items.
What does this PR do?
Adds support for
--prefixwhen listing Storage blob containers and blobs to help target listing scope.GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline