From d094f3239acc3b5b9693210ad4af1ab1f599cbeb Mon Sep 17 00:00:00 2001 From: Victor Colin Amador Date: Mon, 8 Jun 2026 15:30:40 -0700 Subject: [PATCH 1/3] Fix SQL subscription name-to-GUID resolution (#449, #453) Resolve the subscription through ISubscriptionService in GetSqlServerResourceAsync, RenameDatabaseAsync, ListServersAsync, and CreateServerAsync instead of building a SubscriptionResource directly from the raw subscription value, which failed with a cryptic ARM error when a subscription display name was passed instead of a GUID. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../vcolin7-sql-subscription-resolution.yaml | 4 + .../src/Services/SqlService.cs | 20 +-- .../Services/SqlServiceTests.cs | 117 ++++++++++++++++++ 3 files changed, 131 insertions(+), 10 deletions(-) create mode 100644 servers/Azure.Mcp.Server/changelog-entries/vcolin7-sql-subscription-resolution.yaml create mode 100644 tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs diff --git a/servers/Azure.Mcp.Server/changelog-entries/vcolin7-sql-subscription-resolution.yaml b/servers/Azure.Mcp.Server/changelog-entries/vcolin7-sql-subscription-resolution.yaml new file mode 100644 index 0000000000..367f0bd57e --- /dev/null +++ b/servers/Azure.Mcp.Server/changelog-entries/vcolin7-sql-subscription-resolution.yaml @@ -0,0 +1,4 @@ +changes: + - section: "Bugs Fixed" + description: | + Fixed Azure SQL tools failing with a cryptic ARM error when a subscription display name (instead of a subscription ID) was provided. The `sql server show`, `sql server list`, `sql server create`, and `sql db rename` operations now resolve the subscription through the subscription service. diff --git a/tools/Azure.Mcp.Tools.Sql/src/Services/SqlService.cs b/tools/Azure.Mcp.Tools.Sql/src/Services/SqlService.cs index 0dc73efd80..80216be88f 100644 --- a/tools/Azure.Mcp.Tools.Sql/src/Services/SqlService.cs +++ b/tools/Azure.Mcp.Tools.Sql/src/Services/SqlService.cs @@ -18,6 +18,7 @@ namespace Azure.Mcp.Tools.Sql.Services; public class SqlService(ISubscriptionService subscriptionService, ITenantService tenantService, ILogger logger) : BaseAzureResourceService(subscriptionService, tenantService), ISqlService { + private readonly ISubscriptionService _subscriptionService = subscriptionService; private readonly ILogger _logger = logger; /// @@ -36,9 +37,7 @@ private async Task GetSqlServerResourceAsync( RetryPolicyOptions? retryPolicy, CancellationToken cancellationToken = default) { - var armClient = await CreateArmClientAsync(null, retryPolicy, null, cancellationToken); - var subscriptionResource = armClient.GetSubscriptionResource( - ResourceManager.Resources.SubscriptionResource.CreateResourceIdentifier(subscription)); + var subscriptionResource = await _subscriptionService.GetSubscription(subscription, null, retryPolicy, cancellationToken); var resourceGroupResource = await subscriptionResource.GetResourceGroupAsync(resourceGroup, cancellationToken); return await resourceGroupResource.Value.GetSqlServers().GetAsync(serverName, cancellationToken: cancellationToken); } @@ -317,16 +316,19 @@ public async Task RenameDatabaseAsync( (nameof(subscription), subscription), (nameof(newDatabaseName), newDatabaseName)); + var subscriptionResource = await _subscriptionService.GetSubscription(subscription, null, retryPolicy, cancellationToken); + var subscriptionId = subscriptionResource.Data.SubscriptionId; + var armClient = await CreateArmClientAsync(null, retryPolicy, null, cancellationToken); var currentDatabaseId = SqlDatabaseResource.CreateResourceIdentifier( - subscription, + subscriptionId, resourceGroup, serverName, databaseName); var targetDatabaseId = SqlDatabaseResource.CreateResourceIdentifier( - subscription, + subscriptionId, resourceGroup, serverName, newDatabaseName); @@ -632,9 +634,8 @@ public async Task CreateServerAsync( (nameof(administratorLogin), administratorLogin), (nameof(administratorPassword), administratorPassword)); - // Use ARM client directly for create operations - var armClient = await CreateArmClientAsync(null, retryPolicy, null, cancellationToken); - var subscriptionResource = armClient.GetSubscriptionResource(ResourceManager.Resources.SubscriptionResource.CreateResourceIdentifier(subscription)); + // Resolve the subscription (supports both subscription IDs and names) before navigating to the resource group + var subscriptionResource = await _subscriptionService.GetSubscription(subscription, null, retryPolicy, cancellationToken); var resourceGroupResource = await subscriptionResource.GetResourceGroupAsync(resourceGroup, cancellationToken); var serverData = new SqlServerData(location) @@ -730,8 +731,7 @@ public async Task> ListServersAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - var armClient = await CreateArmClientAsync(null, retryPolicy, null, cancellationToken); - var subscriptionResource = armClient.GetSubscriptionResource(ResourceManager.Resources.SubscriptionResource.CreateResourceIdentifier(subscription)); + var subscriptionResource = await _subscriptionService.GetSubscription(subscription, null, retryPolicy, cancellationToken); ResourceManager.Resources.ResourceGroupResource resourceGroupResource; try diff --git a/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs b/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs new file mode 100644 index 0000000000..3e40614d7c --- /dev/null +++ b/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs @@ -0,0 +1,117 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Azure.Core; +using Azure.Mcp.Core.Services.Azure.Subscription; +using Azure.Mcp.Core.Services.Azure.Tenant; +using Azure.Mcp.Tools.Sql.Services; +using Azure.ResourceManager; +using Microsoft.Extensions.Logging; +using Microsoft.Mcp.Core.Options; +using Microsoft.Mcp.Core.Services.Azure.Authentication; +using NSubstitute; +using NSubstitute.ExceptionExtensions; +using Xunit; + +namespace Azure.Mcp.Tools.Sql.Tests.Services; + +public class SqlServiceTests +{ + private const string SubscriptionName = "my-subscription-name"; + + // Sentinel thrown by the mocked subscription service so tests can prove the + // service resolves the subscription via ISubscriptionService (the #449/#453 fix) + // instead of building a SubscriptionResource directly from the raw value. + private sealed class SubscriptionResolvedException : Exception; + + private readonly ISubscriptionService _subscriptionService; + private readonly ITenantService _tenantService; + private readonly ILogger _logger; + private readonly SqlService _service; + + public SqlServiceTests() + { + _subscriptionService = Substitute.For(); + _tenantService = Substitute.For(); + _logger = Substitute.For>(); + + var cloudConfig = Substitute.For(); + cloudConfig.CloudType.Returns(AzureCloudConfiguration.AzureCloud.AzurePublicCloud); + cloudConfig.AuthorityHost.Returns(new Uri("https://login.microsoftonline.com")); + cloudConfig.ArmEnvironment.Returns(ArmEnvironment.AzurePublicCloud); + _tenantService.CloudConfiguration.Returns(cloudConfig); + + var credential = Substitute.For(); + _tenantService.GetTokenCredentialAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(credential)); + + _subscriptionService.GetSubscription( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .ThrowsAsync(); + + _service = new SqlService(_subscriptionService, _tenantService, _logger); + } + + [Fact] + public async Task GetServerAsync_ResolvesSubscriptionThroughSubscriptionService() + { + await Assert.ThrowsAsync(() => + _service.GetServerAsync("server1", "rg", SubscriptionName, null, TestContext.Current.CancellationToken)); + + await AssertSubscriptionResolvedAsync(); + } + + [Fact] + public async Task ListServersAsync_ResolvesSubscriptionThroughSubscriptionService() + { + await Assert.ThrowsAsync(() => + _service.ListServersAsync("rg", SubscriptionName, null, TestContext.Current.CancellationToken)); + + await AssertSubscriptionResolvedAsync(); + } + + [Fact] + public async Task CreateServerAsync_ResolvesSubscriptionThroughSubscriptionService() + { + await Assert.ThrowsAsync(() => + _service.CreateServerAsync( + "server1", + "rg", + SubscriptionName, + "eastus", + "admin", + "P@ssw0rd!", + null, + null, + null, + TestContext.Current.CancellationToken)); + + await AssertSubscriptionResolvedAsync(); + } + + [Fact] + public async Task RenameDatabaseAsync_ResolvesSubscriptionThroughSubscriptionService() + { + await Assert.ThrowsAsync(() => + _service.RenameDatabaseAsync( + "server1", + "olddb", + "newdb", + "rg", + SubscriptionName, + null, + TestContext.Current.CancellationToken)); + + await AssertSubscriptionResolvedAsync(); + } + + private Task AssertSubscriptionResolvedAsync() => + _subscriptionService.Received(1).GetSubscription( + SubscriptionName, + Arg.Any(), + Arg.Any(), + Arg.Any()); +} From fabf41a9cf44a73d2966a47ec27ba4211ccc2441 Mon Sep 17 00:00:00 2001 From: Victor Colin Amador Date: Mon, 15 Jun 2026 12:04:20 -0600 Subject: [PATCH 2/3] Address PR feedback: use specific exception message instead of sentinel class in SqlServiceTests Replaces the SubscriptionResolvedException sentinel type with an InvalidOperationException carrying a distinctive message, and asserts the message matches. Addresses review feedback on #449/#453. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Services/SqlServiceTests.cs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs b/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs index 3e40614d7c..f46adf0d67 100644 --- a/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs +++ b/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Services/SqlServiceTests.cs @@ -19,10 +19,10 @@ public class SqlServiceTests { private const string SubscriptionName = "my-subscription-name"; - // Sentinel thrown by the mocked subscription service so tests can prove the - // service resolves the subscription via ISubscriptionService (the #449/#453 fix) + // Distinctive message thrown by the mocked subscription service so tests can prove + // the service resolves the subscription via ISubscriptionService (the #449/#453 fix) // instead of building a SubscriptionResource directly from the raw value. - private sealed class SubscriptionResolvedException : Exception; + private const string SubscriptionResolvedMessage = "SqlServiceTests: subscription resolved via ISubscriptionService"; private readonly ISubscriptionService _subscriptionService; private readonly ITenantService _tenantService; @@ -50,7 +50,7 @@ public SqlServiceTests() Arg.Any(), Arg.Any(), Arg.Any()) - .ThrowsAsync(); + .ThrowsAsync(new InvalidOperationException(SubscriptionResolvedMessage)); _service = new SqlService(_subscriptionService, _tenantService, _logger); } @@ -58,25 +58,27 @@ public SqlServiceTests() [Fact] public async Task GetServerAsync_ResolvesSubscriptionThroughSubscriptionService() { - await Assert.ThrowsAsync(() => + var ex = await Assert.ThrowsAsync(() => _service.GetServerAsync("server1", "rg", SubscriptionName, null, TestContext.Current.CancellationToken)); + Assert.Equal(SubscriptionResolvedMessage, ex.Message); await AssertSubscriptionResolvedAsync(); } [Fact] public async Task ListServersAsync_ResolvesSubscriptionThroughSubscriptionService() { - await Assert.ThrowsAsync(() => + var ex = await Assert.ThrowsAsync(() => _service.ListServersAsync("rg", SubscriptionName, null, TestContext.Current.CancellationToken)); + Assert.Equal(SubscriptionResolvedMessage, ex.Message); await AssertSubscriptionResolvedAsync(); } [Fact] public async Task CreateServerAsync_ResolvesSubscriptionThroughSubscriptionService() { - await Assert.ThrowsAsync(() => + var ex = await Assert.ThrowsAsync(() => _service.CreateServerAsync( "server1", "rg", @@ -89,13 +91,14 @@ await Assert.ThrowsAsync(() => null, TestContext.Current.CancellationToken)); + Assert.Equal(SubscriptionResolvedMessage, ex.Message); await AssertSubscriptionResolvedAsync(); } [Fact] public async Task RenameDatabaseAsync_ResolvesSubscriptionThroughSubscriptionService() { - await Assert.ThrowsAsync(() => + var ex = await Assert.ThrowsAsync(() => _service.RenameDatabaseAsync( "server1", "olddb", @@ -105,6 +108,7 @@ await Assert.ThrowsAsync(() => null, TestContext.Current.CancellationToken)); + Assert.Equal(SubscriptionResolvedMessage, ex.Message); await AssertSubscriptionResolvedAsync(); } From 9d867b704af345d8301c21ee1b14330d69693e8c Mon Sep 17 00:00:00 2001 From: Victor Colin Amador Date: Mon, 15 Jun 2026 12:11:15 -0600 Subject: [PATCH 3/3] Re-record SQL tests affected by subscription resolution change The subscription name-to-GUID fix (#449/#453) adds a GET /subscriptions/{id} ARM call via ISubscriptionService before navigating to the SQL server. The prior recordings lacked that interaction, causing playback mismatches for the four tests that route through GetSqlServerResourceAsync. Re-recorded them and updated the assets tag. Re-recorded: ListSqlServerEntraAdmins, ListSqlServerFirewallRules, CreateFirewallRule, DeleteFirewallRule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../tests/Azure.Mcp.Tools.Sql.Tests/assets.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/assets.json b/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/assets.json index 6de3f971c2..864cd0afd8 100644 --- a/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/assets.json +++ b/tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "", "TagPrefix": "Azure.Mcp.Tools.Sql.Tests", - "Tag": "Azure.Mcp.Tools.Sql.Tests_a6712a8ecc" + "Tag": "Azure.Mcp.Tools.Sql.Tests_44375821fe" }