Fix SQL list databases and elastic pools to scope by server#2865
Open
vcolin7 wants to merge 4 commits into
Open
Fix SQL list databases and elastic pools to scope by server#2865vcolin7 wants to merge 4 commits into
vcolin7 wants to merge 4 commits into
Conversation
) ListDatabasesAsync and GetElasticPoolsAsync ignored serverName and returned all resources in the resource group. Scope the Azure Resource Graph query to the requested server via an id-contains additionalFilter, extracted into a testable BuildServerResourceFilter helper, and add unit tests covering the filter and KQL escaping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Align ListDatabasesAsync and GetElasticPoolsAsync with the sibling list methods (GetEntraAdministratorsAsync, ListFirewallRulesAsync, ListServersAsync) by using the server-scoped ARM SDK collections instead of an Azure Resource Graph 'id contains' filter. Server scoping is now structural (the server is part of the request path) rather than a KQL predicate, and the SDK auto-pages the full set so the ResourceQueryResults truncation model is no longer needed. - ISqlService/SqlService: return List<T>; iterate GetSqlDatabases()/GetElasticPools().GetAllAsync(); add SDK-based ConvertToSqlElasticPoolModel(ElasticPoolResource). - Commands: consume List<T>; drop AreResultsTruncated from result records (matches FirewallRuleListResult). - Remove now-orphaned elastic-pool JSON shadow models and their serialization registrations; drop obsolete BuildServerResourceFilter helper and its unit test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#448, #452) The switch from Azure Resource Graph to the typed ARM SDK changed the outgoing REST requests for ListDatabasesAsync and GetElasticPoolsAsync, invalidating the existing SqlCommandTests recordings. Re-recorded the SqlCommandTests session records and updated assets.json to the new tag (Azure.Mcp.Tools.Sql.Tests_50222315f7). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vcolin7
requested review from
a team,
achyuth-ms,
ericshape and
jeremyfrosti
as code owners
vcolin7
requested review from
hallipr,
jongio,
joshfree,
saikoumudi,
srnagar,
tmeschter and
xiangyan99
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Azure SQL db get (list databases) and elastic-pool list returning resources across all servers in a resource group by switching the underlying implementation from Azure Resource Graph (resource-group scoped) to server-scoped ARM SDK collections.
Changes:
- Reworked
SqlService.ListDatabasesAsyncandSqlService.GetElasticPoolsAsyncto enumerate server-scoped ARM collections (GetSqlDatabases()/GetElasticPools()), returningList<T>instead ofResourceQueryResults<T>. - Updated SQL commands and unit tests to consume the new
List<T>return types and removed theAreResultsTruncatedoutput field. - Removed now-unused JSON shadow models and corresponding
SqlJsonContextregistrations; updated SQL test recordings tag (assets.json) and added a changelog entry.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Sql/src/Services/SqlService.cs | Switched database + elastic pool listing to server-scoped ARM SDK enumeration and updated elastic pool conversion to typed SDK models. |
| tools/Azure.Mcp.Tools.Sql/src/Services/ISqlService.cs | Updated method return types to List<SqlDatabase> / List<SqlElasticPool>. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/Database/DatabaseGetCommand.cs | Updated response wrapper to accept List<SqlDatabase> and removed truncation field. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/ElasticPool/ElasticPoolListCommand.cs | Updated response wrapper to accept List<SqlElasticPool> and removed truncation field. |
| tools/Azure.Mcp.Tools.Sql/src/Commands/SqlJsonContext.cs | Removed source-gen registrations for deleted elastic pool JSON shadow models. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlElasticPoolData.cs | Deleted unused ARG JSON shadow model. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlElasticPoolProperties.cs | Deleted unused ARG JSON shadow model. |
| tools/Azure.Mcp.Tools.Sql/src/Services/Models/SqlElasticPoolPerDatabaseSettings.cs | Deleted unused ARG JSON shadow model. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/Database/DatabaseGetCommandTests.cs | Updated mocks to return List<SqlDatabase> instead of ResourceQueryResults. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/ElasticPool/ElasticPoolListCommandTests.cs | Updated mocks to return List<SqlElasticPool> instead of ResourceQueryResults. |
| tools/Azure.Mcp.Tools.Sql/tests/Azure.Mcp.Tools.Sql.Tests/assets.json | Updated recordings tag to Azure.Mcp.Tools.Sql.Tests_50222315f7. |
| servers/Azure.Mcp.Server/changelog-entries/copilot-fix-sql-server-filter.yaml | Added changelog entry documenting the server-scoping fix. |
vcolin7
changed the title
alzimmermsft
approved these changes
Jun 12, 2026
…ols (#448, #452) The ARM SDK refactor of ListDatabasesAsync and GetElasticPoolsAsync routed the subscription through SubscriptionResource.CreateResourceIdentifier, which only accepts a subscription GUID. This regressed the prior Resource Graph behavior where a subscription name also worked, as flagged by Copilot review on PR #2865. Scope the fix narrowly to just these two list methods by resolving the subscription via ISubscriptionService before navigating the ARM hierarchy. The shared GetSqlServerResourceAsync helper and the server/firewall/entra-admin/create/rename paths are intentionally left unchanged to avoid overlapping with PR #2861. The new ResolveSubscriptionIdAsync helper skips the extra ARM request when the value is already a subscription ID, so existing recordings remain valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vcolin7
force-pushed
the
vcolin7/sql-servername-filter
branch
from
aee3d02 to
754015e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
The SQL
db get(list databases) andelastic-pool listtools incorrectly ignored theserverparameter and returned every database / elastic pool in a given resource group, not just those on the requested server.Rather than patch the ARG queries with a server predicate, this PR switches both
ListDatabasesAsyncandGetElasticPoolsAsyncto the typed ARM SDK server-scoped collection pattern already used by the sibling read methods (GetEntraAdministratorsAsync,ListFirewallRulesAsync,ListServersAsync). Server scoping is now part of the request URL path instead of a KQL filter. This keeps the SQL service consistent with the rest of the toolset.Key changes
SqlService.cs: rewrote both methods to resolve the server resource and enumerate itsGetSqlDatabases()/GetElasticPools()collections; removed ARG-only helpers and theSystem.Text.Jsonshadow-model conversion.ISqlService.cs: return types are nowList<SqlDatabase>/List<SqlElasticPool>(droppedResourceQueryResults).DatabaseGetCommand.cs/ElasticPoolListCommand.cs: consumeList<T>and drop theAreResultsTruncatedfield, matchingFirewallRuleListResult.SqlJsonContextregistrations.List<T>mocks and re-recorded the affectedSqlCommandTestsintegration tests (assets.json bumped toAzure.Mcp.Tools.Sql.Tests_50222315f7).GitHub issue number?
Fixes #448
Fixes #452
Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentation — N/A (no description or surface change)README.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1— N/AToolDescriptionEvaluator— N/A (descriptions unchanged)consolidated-tools.json— N/Aazmcp-commands.md— N/A (no command/option changes)./eng/scripts/Update-AzCommandsMetadata.ps1— N/Ae2eTestPrompts.md— N/AInvoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with
writeaccess 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.