Removing SQL Legacy usage for FirewallRules by jeremyfrosti · Pull Request #29465 · Azure/azure-powershell

jeremyfrosti · 2026-04-27T23:35:30Z

Description

Removing legacy usage of 2014 APIs from SQL

Mandatory Checklist

Please choose the target release of Azure PowerShell. (⚠️Target release is a different concept from API readiness. Please click below links for details.)
- General release
- Public preview
- Private preview
- Engineering build
- No need for a release
Check this box to confirm: I have read the Submitting Changes section of CONTRIBUTING.md and reviewed the following information:

SHOULD update ChangeLog.md file(s) appropriately
- Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
  - A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
- Should not change ChangeLog.md if no new release is required, such as fixing test case only.
SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
SHOULD have proper test coverage for changes in pull request.
SHOULD NOT adjust version of module manually in pull request

azure-client-tools-bot-prd · 2026-04-27T23:35:33Z

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

VeryEarly · 2026-04-27T23:38:57Z

/azp run

azure-pipelines · 2026-04-27T23:39:12Z

Azure Pipelines successfully started running 3 pipeline(s).

Copilot

Pull request overview

This pull request removes legacy (2014-era) SQL Firewall Rule implementation usage by switching the cmdlets’ internal plumbing to the current Microsoft.Azure.Management.Sql SDK surface, and regenerating the Sql management SDK to include FirewallRules operations.

Changes:

Updated Firewall Rule communicator/adapter to use Microsoft.Azure.Management.Sql models/operations instead of LegacySdk.
Added paging-aware listing for server firewall rules using ListByServer + ListByServerNext.
Regenerated Sql.Management.Sdk to include IFirewallRulesOperations and related models/operations, and updated the generation README inputs.

Reviewed changes

Copilot reviewed 4 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/Sql/Sql/FirewallRule/Services/AzureSqlServerFirewallRuleCommunicator.cs	Swaps legacy client/model usage for new SDK operations; adds manual paging for list.
src/Sql/Sql/FirewallRule/Services/AzureSqlServerFirewallRuleAdapter.cs	Updates adapter mapping and upsert payload to new SDK `FirewallRule` shape.
src/Sql/Sql/ChangeLog.md	Adds release note entry for removal of legacy 2014 API usage in firewall rule cmdlets.
src/Sql/Sql.Management.Sdk/README.md	Updates AutoRest input specs/commit and includes FirewallRules spec.
src/Sql/Sql.Management.Sdk/Generated/SqlManagementClient.cs	Adds `FirewallRules` operations property initialization.
src/Sql/Sql.Management.Sdk/Generated/Models/ServerFirewallRuleProperties.cs	New generated model type for firewall rule properties.
src/Sql/Sql.Management.Sdk/Generated/Models/FirewallRuleList.cs	New generated list wrapper model.
src/Sql/Sql.Management.Sdk/Generated/Models/FirewallRule.cs	New generated firewall rule resource model.
src/Sql/Sql.Management.Sdk/Generated/ISqlManagementClient.cs	Exposes `IFirewallRulesOperations` on the client interface.
src/Sql/Sql.Management.Sdk/Generated/IFirewallRulesOperations.cs	New generated operations interface for firewall rules.
src/Sql/Sql.Management.Sdk/Generated/FirewallRulesOperationsExtensions.cs	New generated extension methods for firewall rules operations.
src/Sql/Sql.Management.Sdk/Generated/FirewallRulesOperations.cs	New generated implementation for firewall rules REST operations.

Copilot · 2026-04-27T23:42:38Z

+
+            var pagedResponse = GetCurrentSqlClient().FirewallRules.ListByServer(resourceGroupName, serverName);
+            resultsList.AddRange(pagedResponse);
+
+            while (!string.IsNullOrEmpty(pagedResponse.NextPageLink))
+            {
+                pagedResponse = GetCurrentSqlClient().FirewallRules.ListByServerNext(pagedResponse.NextPageLink);


List() repeatedly calls GetCurrentSqlClient() (including inside the pagination loop), which creates a new SqlManagementClient each time. This adds avoidable overhead and could cause inconsistent per-invocation settings/headers. Consider creating the client once at the start of List() and reusing it for the initial call and subsequent ListByServerNext() calls.

Suggested change

var pagedResponse = GetCurrentSqlClient().FirewallRules.ListByServer(resourceGroupName, serverName);

resultsList.AddRange(pagedResponse);

while (!string.IsNullOrEmpty(pagedResponse.NextPageLink))

{

pagedResponse = GetCurrentSqlClient().FirewallRules.ListByServerNext(pagedResponse.NextPageLink);

var sqlClient = GetCurrentSqlClient();

var pagedResponse = sqlClient.FirewallRules.ListByServer(resourceGroupName, serverName);

resultsList.AddRange(pagedResponse);

while (!string.IsNullOrEmpty(pagedResponse.NextPageLink))

{

pagedResponse = sqlClient.FirewallRules.ListByServerNext(pagedResponse.NextPageLink);

Copilot · 2026-04-27T23:42:38Z

+        private SqlManagementClient GetCurrentSqlClient(string subscriptionId = null)
        {
            // Get the SQL management client for the current subscription
-            if (SqlClient == null)
+            // Note: client is not cached in static field because that causes ObjectDisposedException in functional tests.
+            var sqlClient = AzureSession.Instance.ClientFactory.CreateArmClient<Management.Sql.SqlManagementClient>(Context, AzureEnvironment.Endpoint.ResourceManager);
+            if (subscriptionId != null)
            {
-                SqlClient = AzureSession.Instance.ClientFactory.CreateClient<SqlManagementClient>(Context, AzureEnvironment.Endpoint.ResourceManager);
+                sqlClient.SubscriptionId = subscriptionId;
            }
-            return SqlClient;
+            return sqlClient;


GetCurrentSqlClient no longer uses the static SqlClient cache, but the class still keeps SqlClient/Subscription static state and resets SqlClient in the constructor. This state is now unused and can mislead future changes; either remove the static fields/subscription-switch logic or reintroduce caching in a safe way.

Copilot · 2026-04-27T23:42:38Z

+        private SqlManagementClient GetCurrentSqlClient(string subscriptionId = null)
        {
            // Get the SQL management client for the current subscription
-            if (SqlClient == null)
+            // Note: client is not cached in static field because that causes ObjectDisposedException in functional tests.
+            var sqlClient = AzureSession.Instance.ClientFactory.CreateArmClient<Management.Sql.SqlManagementClient>(Context, AzureEnvironment.Endpoint.ResourceManager);
+            if (subscriptionId != null)
            {
-                SqlClient = AzureSession.Instance.ClientFactory.CreateClient<SqlManagementClient>(Context, AzureEnvironment.Endpoint.ResourceManager);
+                sqlClient.SubscriptionId = subscriptionId;
            }


GetCurrentSqlClient takes an optional subscriptionId parameter, but the method is private and this class only calls it without arguments. If the parameter isn’t needed, remove it to reduce confusion; otherwise add a call site that passes the intended subscription id (or document why the override is required).

Copilot · 2026-04-27T23:42:39Z

        /// Convert a Management.Sql.LegacySdk.Models.FirewallRule to AzureSqlServerFirewallRuleModel
        /// </summary>
        /// <param name="resourceGroup">The resource group the server is in</param>
        /// <param name="serverName">The name of the server</param>
        /// <param name="resp">The management client server response to convert</param>
        /// <returns>The converted server model</returns>
-        private static AzureSqlServerFirewallRuleModel CreateFirewallRuleModelFromResponse(string resourceGroup, string serverName, Management.Sql.LegacySdk.Models.FirewallRule resp)
+        private static AzureSqlServerFirewallRuleModel CreateFirewallRuleModelFromResponse(string resourceGroup, string serverName, Management.Sql.Models.FirewallRule resp)
        {


The XML doc comment still says it converts a Management.Sql.LegacySdk.Models.FirewallRule, but the method now accepts Management.Sql.Models.FirewallRule. Please update the comment to match the new SDK type to avoid misleading documentation.

Copilot · 2026-04-27T23:42:39Z

+        public IList<Management.Sql.Models.FirewallRule> List(string resourceGroupName, string serverName)
        {
-            return GetCurrentSqlClient().FirewallRules.List(resourceGroupName, serverName).FirewallRules;
+            List<Management.Sql.Models.FirewallRule> resultsList = new List<Management.Sql.Models.FirewallRule>();
+
+            var pagedResponse = GetCurrentSqlClient().FirewallRules.ListByServer(resourceGroupName, serverName);
+            resultsList.AddRange(pagedResponse);
+
+            while (!string.IsNullOrEmpty(pagedResponse.NextPageLink))
+            {
+                pagedResponse = GetCurrentSqlClient().FirewallRules.ListByServerNext(pagedResponse.NextPageLink);
+                resultsList.AddRange(pagedResponse);
+            }


The new paging logic in List() is currently untested in this module’s test suite. Since Get-AzSqlServerFirewallRule depends on List(), consider adding/expanding scenario coverage for firewall rule CRUD (including list pagination via NextPageLink) similar to the existing IPv6 firewall rule scenario tests.

VeryEarly · 2026-04-28T04:41:38Z

/azp run

azure-pipelines · 2026-04-28T04:41:55Z

Azure Pipelines successfully started running 3 pipeline(s).

…tests

Copilot

Pull request overview

Copilot reviewed 12 out of 55 changed files in this pull request and generated 1 comment.

Copilot · 2026-04-29T21:50:37Z

 using Hyak.Common;
+using Microsoft.Azure.Management.Sql.Models;
 using System.Collections.Generic;
 using System.Linq;


using Hyak.Common; is now unused after switching the exception type to ErrorResponseException. With warnings treated as errors in this repo, the unused using can break the build; please remove it (or use CloudException again if intended).

VeryEarly · 2026-04-29T22:08:42Z

/azp run

azure-pipelines · 2026-04-29T22:09:01Z

Azure Pipelines successfully started running 3 pipeline(s).

VeryEarly · 2026-04-30T01:09:04Z

/azp run

azure-pipelines · 2026-04-30T01:09:22Z

Azure Pipelines successfully started running 3 pipeline(s).

VeryEarly · 2026-05-06T16:38:40Z

/azp run

azure-pipelines · 2026-05-06T16:39:01Z

Azure Pipelines successfully started running 3 pipeline(s).

…o fail

VeryEarly · 2026-05-06T20:38:46Z

/azp run

azure-pipelines · 2026-05-06T20:39:07Z

Azure Pipelines successfully started running 3 pipeline(s).

jeremyfrosti added 2 commits April 27, 2026 16:33

Removing SQL Legacy usage for FirewallRules

76f795a

Updating changelog

098d3d9

Copilot AI review requested due to automatic review settings April 27, 2026 23:35

Copilot started reviewing on behalf of jeremyfrosti April 27, 2026 23:37 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

VeryEarly self-assigned this Apr 28, 2026

Fixing the version in recorded tests for firewall rules

76b866e

Small fix to error catching in FirewallRules and rerecording failing …

2bd1d61

…tests

Copilot AI review requested due to automatic review settings April 29, 2026 21:39

Copilot started reviewing on behalf of jeremyfrosti April 29, 2026 21:41 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

Adding generated changes

a543068

Fixing build issue from autogenerated name change

16ceb7e

Copilot AI review requested due to automatic review settings May 6, 2026 16:23

Copilot started reviewing on behalf of jeremyfrosti May 6, 2026 16:26 View session

Adding missed file that was causing local builds to pass but remote t…

909312c

…o fail

VeryEarly approved these changes May 6, 2026

View reviewed changes

VeryEarly merged commit 2c0d8bd into Azure:main May 6, 2026
12 checks passed

Conversation

jeremyfrosti commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Mandatory Checklist

Uh oh!

azure-client-tools-bot-prd Bot commented Apr 27, 2026

Uh oh!

VeryEarly commented Apr 27, 2026

Uh oh!

azure-pipelines Bot commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

VeryEarly commented Apr 28, 2026

Uh oh!

azure-pipelines Bot commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

VeryEarly commented Apr 29, 2026

Uh oh!

azure-pipelines Bot commented Apr 29, 2026

Uh oh!

VeryEarly commented Apr 30, 2026

Uh oh!

azure-pipelines Bot commented Apr 30, 2026

Uh oh!

VeryEarly commented May 6, 2026

Uh oh!

azure-pipelines Bot commented May 6, 2026

Uh oh!

VeryEarly commented May 6, 2026

Uh oh!

azure-pipelines Bot commented May 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeremyfrosti commented Apr 27, 2026 •

edited

Loading