Migrate Sql tools to new tool design#2956
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates Azure SQL tool commands and their tests to the attribute-based option binding design (removing manual RegisterOptions/BindOptions code paths) and aligns command execution with the newer SubscriptionCommand<TOptions, TResult> pattern.
Changes:
- Converted SQL commands to
SubscriptionCommand<..., ...>withISubscriptionResolverand strongly-typedExecuteAsync(CommandContext, TOptions, ...). - Replaced
SqlOptionDefinitions/manual option registration with[Option]/[OptionContainer]-annotated options types (plus sharedSqlOptionDescriptions). - Updated unit tests to use
SubscriptionCommandUnitTestsBase<,>.
Invoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with write access to the repo need to validate the contents of this PR before leaving a comment with the text /azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Server/ServerGetCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Server/ServerDeleteCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Server/ServerCreateCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/FirewallRule/FirewallRuleListCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/FirewallRule/FirewallRuleDeleteCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/FirewallRule/FirewallRuleCreateCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/EntraAdmin/EntraAdminListCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/ElasticPool/ElasticPoolListCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Database/DatabaseUpdateCommandTests.cs | Updates tests for new option-binding/command base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Database/DatabaseRenameCommandTests.cs | Updates tests for new option-binding/command base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Database/DatabaseGetCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Database/DatabaseDeleteCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Database/DatabaseCreateCommandTests.cs | Switches test base to subscription-aware unit test base. |
| tools/Azure.Mcp.Tools.Sql/src/Services/SqlService.cs | Minor SDK type name cleanups (e.g., SqlDatabaseData, SqlSku, SqlFirewallRuleData). |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlSku.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlServerAadAdministratorProperties.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlServerAadAdministratorData.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlFirewallRuleProperties.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlFirewallRuleData.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlDatabaseProperties.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlDatabaseData.cs | Converts to file-scoped namespace and flattens type definition. |
| tools/Azure.Mcp.Tools.Sql/src/Options/SqlOptionDescriptions.cs | Adds shared description constants for SQL options. |
| tools/Azure.Mcp.Tools.Sql/src/Options/SqlOptionDefinitions.cs | Removes legacy System.CommandLine option definitions. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Server/ServerGetOptions.cs | Moves server option to [Option] attribute binding. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Server/ServerDeleteOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Server/ServerCreateOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/FirewallRule/FirewallRuleListOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/FirewallRule/FirewallRuleDeleteOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/FirewallRule/FirewallRuleCreateOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/EntraAdmin/EntraAdminListOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/ElasticPool/ElasticPoolListOptions.cs | Simplifies base options inheritance and adds required server option. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Database/DatabaseUpdateOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Database/DatabaseRenameOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Database/DatabaseGetOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Database/DatabaseDeleteOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/Database/DatabaseCreateOptions.cs | Moves options to [Option] attributes and required properties. |
| tools/Azure.Mcp.Tools.Sql/src/Options/BaseSqlOptions.cs | Replaces old base options inheritance with ISubscriptionOption + attributed options. |
| tools/Azure.Mcp.Tools.Sql/src/Options/BaseElasticPoolOptions.cs | Removes legacy base options type. |
| tools/Azure.Mcp.Tools.Sql/src/Options/BaseDatabaseOptions.cs | Removes legacy base options type. |
| tools/Azure.Mcp.Tools.Sql/src/GlobalUsings.cs | Removes legacy global System.CommandLine using. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Server/ServerGetCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Server/ServerDeleteCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Server/ServerCreateCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/FirewallRule/FirewallRuleListCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/FirewallRule/FirewallRuleDeleteCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/FirewallRule/FirewallRuleCreateCommand.cs | Migrates command validation and execution to typed options binding. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/EntraAdmin/EntraAdminListCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/ElasticPool/ElasticPoolListCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Database/DatabaseUpdateCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Database/DatabaseRenameCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Database/DatabaseGetCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Database/DatabaseDeleteCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Database/DatabaseCreateCommand.cs | Migrates command execution to strongly-typed options + resolver-based subscription flow. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/BaseSqlCommand.cs | Removes legacy base command that performed manual option wiring. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/BaseElasticPoolCommand.cs | Removes legacy base command. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/BaseDatabaseCommand.cs | Removes legacy base command. |
| servers/Azure.Mcp.Server/changelog-entries/1782421569874.yaml | Adds breaking-change changelog entry for SQL tools. |
Comments suppressed due to low confidence (1)
tools/Azure.Mcp.Tools.Sql/src/Commands/Server/ServerCreateCommand.cs:14
- This file has unused
usingdirectives (Azure.Mcp.Tools.Sql.OptionsandMicrosoft.Mcp.Core.Extensions). WithTreatWarningsAsErrors=true,CS8019will fail the build. Remove the unused usings.
using System.Net;
using Azure.Mcp.Core.Commands.Subscription;
using Azure.Mcp.Core.Services.Azure.Subscription;
using Azure.Mcp.Tools.Sql.Models;
using Azure.Mcp.Tools.Sql.Options;
using Azure.Mcp.Tools.Sql.Options.Server;
using Azure.Mcp.Tools.Sql.Services;
using Microsoft.Extensions.Logging;
using Microsoft.Mcp.Core.Commands;
using Microsoft.Mcp.Core.Extensions;
using Microsoft.Mcp.Core.Models.Command;
jongio
left a comment
There was a problem hiding this comment.
Migration correctly follows the two-generic SubscriptionCommand<TOptions, TResult> pattern established by recent PRs (#2950, #2940, #2937). Verified:
- Base class hierarchy removal (
BaseSqlCommand,BaseDatabaseCommand,BaseElasticPoolCommand) is clean; options flattened correctly into leaf classes GetStatusCodeoverride removal is safe;AuthenticatedCommand<TOptions, TResult>already mapsRequestFailedExceptionto its HTTP status- Custom validation in
FirewallRuleCreateCommand.ValidateOptionscorrectly preserves the IP address and dangerous-range checks from the old validator requiredkeyword usage is correct: mandatory user-facing options (Server, Database, ResourceGroup, etc.) are required; optional ones (Subscription, Version, etc.) remain nullable- Test base class migration to
SubscriptionCommandUnitTestsBaseis correct - Result records changed from
internaltopublic sealedto satisfy the two-generic pattern'sTResultconstraint
Re: the --tenant removal flagged by the bot. The author's position is valid since SqlService never passes tenant downstream. Worth noting that other recently-migrated tools (Monitor, AKS, StorageSync) do include Tenant for consistency, and SubscriptionResolver can use it for multi-tenant subscription name resolution. If the team decides to standardize, this would need a follow-up. Not blocking since it's explicitly declared as a breaking change in the changelog.
What does this PR do?
Migrates Sql tools to new design where Register and Bind options are based on
Optionattributes.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