Removes usage of IServiceProvider from Azure.Mcp.Tools.Cosmos, Device Registry and Cosmos#2478
Removes usage of IServiceProvider from Azure.Mcp.Tools.Cosmos, Device Registry and Cosmos#2478conniey merged 4 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors several MCP tool commands to stop resolving their tool services via CommandContext/IServiceProvider at execution time, instead using constructor injection for the relevant service interfaces.
Changes:
- Updated Cosmos and Device Registry commands to accept their service dependencies via constructor injection and call them directly.
- Updated multiple Compute commands to accept
IComputeServicevia constructor injection and (mostly) use the injected instance. - Updated a Device Registry unit test to construct the command with the injected service.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.DeviceRegistry/tests/Azure.Mcp.Tools.DeviceRegistry.UnitTests/Namespace/NamespaceListCommandTests.cs | Updates test construction to pass IDeviceRegistryService into the command constructor. |
| tools/Azure.Mcp.Tools.DeviceRegistry/src/Commands/Namespace/NamespaceListCommand.cs | Switches from context.GetService<IDeviceRegistryService>() to constructor-injected IDeviceRegistryService. |
| tools/Azure.Mcp.Tools.Cosmos/src/Commands/ItemQueryCommand.cs | Switches from context.GetService<ICosmosService>() to constructor-injected ICosmosService. |
| tools/Azure.Mcp.Tools.Cosmos/src/Commands/CosmosListCommand.cs | Switches from context.GetService<ICosmosService>() to constructor-injected ICosmosService. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vmss/VmssUpdateCommand.cs | Introduces constructor injection of IComputeService and uses it for updates. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vmss/VmssDeleteCommand.cs | Introduces constructor injection of IComputeService and uses it for deletes. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vmss/VmssCreateCommand.cs | Introduces constructor injection of IComputeService and uses it for creates. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vm/VmUpdateCommand.cs | Introduces constructor injection of IComputeService and uses it for updates. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vm/VmDeleteCommand.cs | Introduces constructor injection of IComputeService and uses it for deletes. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vm/VmCreateCommand.cs | Introduces constructor injection of IComputeService (but currently still resolves via context.GetService in ExecuteAsync). |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Disk/DiskUpdateCommand.cs | Introduces constructor injection of IComputeService and uses it for disk updates. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Disk/DiskGetCommand.cs | Introduces constructor injection of IComputeService and uses it for disk retrieval/listing. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Disk/DiskDeleteCommand.cs | Introduces constructor injection of IComputeService and uses it for disk deletion. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Disk/DiskCreateCommand.cs | Introduces constructor injection of IComputeService and uses it for disk creation. |
jongio
left a comment
There was a problem hiding this comment.
The direction is right - constructor injection instead of service locator. This aligns with how VmGetCommand and VmssGetCommand were already refactored.
The main issue: constructor signatures changed in 12 source files but only 1 test file (NamespaceListCommandTests) was updated. The remaining 9 test files still construct commands with just _logger, which won't compile against the new 2-param constructors. See inline comments for specifics.
A couple other things:
- Title says "Cosmos, Device Registry and Cosmos" (Cosmos listed twice). The majority of changes are actually in Compute (10/14 files), which isn't mentioned. Consider updating to: "Remove IServiceProvider usage from Compute, Cosmos, and DeviceRegistry tools."
- PR has merge conflicts against main that need resolving.
jongio
left a comment
There was a problem hiding this comment.
Two of three items from my previous review are addressed - tests compile via the CommandUnitTestsBase migration, and the Cosmos disposal null check looks good.
Two things still need attention:
-
VmCreateCommandstill callscontext.GetService<IComputeService>()and its constructor only takesILogger. This is the same service locator pattern the PR is removing from all other Compute commands. It should get the same treatment - addIComputeServiceto the constructor and use the injected field. -
Null guards on the service parameter aren't consistent. Seven commands use
?? throw new ArgumentNullException(nameof(computeService))but five don't. See inline comments.
What does this PR do?
Removes usage of IServiceProvider from Azure.Mcp.Tools.Cosmos, Device Registry and Cosmos
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