Sql Connection Optimization#2433
Conversation
WalkthroughConnect-Sql now constructs a Microsoft.SqlServer.Management.Common.ServerConnection (ApplicationName='SqlServerDsc'), instantiates an SMO.Server with it, and routes Connect/Disconnect via SMO.Server.ConnectionContext. Invoke-SqlDscQuery no longer disconnects server objects. SMO test stubs and unit tests updated to use the Common ServerConnection type; CHANGELOG and CI guidance adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Cmdlet as rgba(52,118,172,0.5) Cmdlet (Connect-Sql)
participant ServerConn as rgba(72,164,82,0.5) ServerConnection
participant SMOServer as rgba(201,72,72,0.5) SMO.Server
participant Engine as rgba(120,120,200,0.5) SQL Engine
Cmdlet->>ServerConn: create ServerConnection (ApplicationName='SqlServerDsc')
Cmdlet->>SMOServer: instantiate SMO.Server with ServerConnection
SMOServer->>Engine: Connect() via SMO.Server.ConnectionContext
Cmdlet->>SMOServer: perform operations via SMO.Server
Note right of Cmdlet: Invoke-SqlDscQuery no longer auto-disconnects SMO.Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (1)
270-281: Guard against$sqlServerObjectbeing$nullor not connected before Disconnect.
An exception before$sqlServerObjectis created (or before a connection opens) will cause thefinallyblock to throw and mask the original error. Add a null/IsOpen check.🛠️ Proposed fix
- $sqlServerObject.ConnectionContext.Disconnect() + if ($null -ne $sqlServerObject -and $sqlServerObject.ConnectionContext.IsOpen) + { + $sqlServerObject.ConnectionContext.Disconnect() + }
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 16-17: Update the two CHANGELOG entries to include issue reference
links and use past-tense, grammatical wording: change the Connect-Sql line to
reference the issue (e.g., #<issue-number>) and rephrase to "Created connection
and server objects as per documentation (see #<issue-number>)", and change the
Invoke-SqlDscQuery line to reference its issue and rephrase to "Removed
disconnect because there was no explicit connect (see #<issue-number>)"; ensure
you replace <issue-number> with the actual issue IDs and keep the function names
Connect-Sql and Invoke-SqlDscQuery intact so reviewers can locate the changes.
🧹 Nitpick comments (1)
source/Public/Invoke-SqlDscQuery.ps1 (1)
274-275: Consider removing the emptyend {}block for clarity.
It adds no behavior and can be removed to simplify the function structure.♻️ Proposed cleanup
- end - {}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-hughes).
a discussion (no related file):
Could you also please remove this line:
|
@dan-hughes seems we got HQRM errors. |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-hughes).
|
@dan-hughes maybe the unit tests does not have the Running tests from 'D:\a\1\s\tests\Unit\SqlServerDsc.Common\Public\Connect-Sql.Tests.ps1'
Describing SqlServerDsc.Common\Connect-SQL
Context When connecting to the default instance using integrated Windows Authentication
##[error] [-] Should return the correct service instance 67ms (63ms|4ms)
##[error] RuntimeException: Unable to find type [Microsoft.SqlServer.Management.Common.ServerConnection].
##[error] at Connect-Sql, D:\a\1\s\output\builtModule\SqlServerDsc\17.5.0\Modules\SqlServerDsc.Common\SqlServerDsc.Common.psm1:141 |
|
Wrong namespace. Updated but the tests need changing as they expect New-Object to be invoked. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1`:
- Around line 273-281: The finally block may call members on null objects and
mask the original exception; update the Connect-Sql.ps1 finally to null-check
$connectTimer and $sqlServerObject before invoking $connectTimer.Stop() and
$sqlServerObject.ConnectionContext.Disconnect() (e.g., only call Stop() if
$connectTimer -ne $null and only call ConnectionContext.Disconnect() if
$sqlServerObject -ne $null and $sqlServerObject.ConnectionContext -ne $null),
preserving existing behavior but avoiding secondary NREs from Connect()
failures.
| $connectTimer.Stop() | ||
| <# | ||
| Connect will ensure we actually can connect, but we need to disconnect | ||
| Connect() will ensure we actually can connect, but we need to disconnect | ||
| from the session so we don't have anything hanging. If we need run a | ||
| method on the returned $sqlServerObject it will automatically open a | ||
| new session and then close, therefore we don't need to keep this | ||
| session open. | ||
| #> | ||
| $sqlConnectionContext.Disconnect() | ||
| $sqlServerObject.ConnectionContext.Disconnect() |
There was a problem hiding this comment.
Guard against null objects in finally
If Server construction or Connect() throws, Line 281 can throw on $null, masking the original failure. Add null guards (and for $connectTimer) before calling members.
🐛 Proposed fix
finally
{
- $connectTimer.Stop()
+ if ($null -ne $connectTimer)
+ {
+ $connectTimer.Stop()
+ }
<#
Connect() will ensure we actually can connect, but we need to disconnect
from the session so we don't have anything hanging. If we need run a
method on the returned $sqlServerObject it will automatically open a
new session and then close, therefore we don't need to keep this
session open.
#>
- $sqlServerObject.ConnectionContext.Disconnect()
+ if ($null -ne $sqlServerObject -and $null -ne $sqlServerObject.ConnectionContext)
+ {
+ $sqlServerObject.ConnectionContext.Disconnect()
+ }
}🤖 Prompt for AI Agents
In `@source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1` around lines 273 -
281, The finally block may call members on null objects and mask the original
exception; update the Connect-Sql.ps1 finally to null-check $connectTimer and
$sqlServerObject before invoking $connectTimer.Stop() and
$sqlServerObject.ConnectionContext.Disconnect() (e.g., only call Stop() if
$connectTimer -ne $null and only call ConnectionContext.Disconnect() if
$sqlServerObject -ne $null and $sqlServerObject.ConnectionContext -ne $null),
preserving existing behavior but avoiding secondary NREs from Connect()
failures.
|
Hmm, which one to use... UPDATE: It looks like |
dan-hughes
force-pushed
the
sql-connection-optimization
branch
from
021b892 to
78dfa00
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2433 +/- ##
=====================================
- Coverage 94% 94% -1%
=====================================
Files 225 225
Lines 10761 10759 -2
=====================================
- Hits 10141 10139 -2
Misses 620 620
🚀 New features to boost your workflow:
|
It looks like it is there, but in the wrong namespace (and missing properties): SqlServerDsc/tests/Unit/Stubs/SMO.cs Lines 1365 to 1383 in e971dc1 There is no such class in the SMO namespace according to docs, so it should be moved. |
|
This is going to be a change one thing, break 6 others. I wonder if both namespaces were valid in a previous SMO version and the was a ServerConnection class for both SMO and RMO. |
|
I don't think so, I think the SMO stubs was added in the wrong place and never used so it wasn't discovered. The old unit tests always (mostly) mocked New-Object. New tests are trying to use the stubs instead. |
|
Is there a list of SMO versions the module supports? I feel like the stubs need regenerating to see what else gets uncovered. Depending on how that goes will decide what to do next... |
|
Suggest to just add the necessary stubs that you need for the unit tests to work. If there are backward compatibility issues we need to address them as bugs come in. That is what I do, build stubs from Microsoft docs. But if you want to build stubs from upstream the safest bet is to build from https://github.com/microsoft/sqlmanagementobjects
The problem is that it:
The user can choose what SMO they should use depending on the workload for the node. SqlServerDsc will use SqlServer if it is available unless overridden with environment variable. I would have like to built our own SMO from the SMO GitHub repo an used that, but there are a lot of if and buts with that (backward compatibility, missing assemblies for older SQL Server versions) plus if we load our assembly we block any other module the user wants to use that require a different SMO version. Dbatools has solved that by "hiding it" in it own "runspace" (don't remember what the technique is called) which we also need to do if we go that route. Dbatools has also gotten permission to use assemblies that are not part of SMO but bundled in SQLPS, but the dbatools permission is for "developer" work, and felt like the permission was not doable for production use: https://github.com/dataplat/dbatools.library/blob/aecbec9ece523f77b2f959e5225352d97f1194e6/var/third-party-licenses/sql-server-data-tools-license-terms.txt#L45C111-L45C127 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/Unit/Stubs/SMO.cs`:
- Around line 2784-2815: ServerConnection.ConnectTimeout is typed as string but
should be int, and the Server class needs a constructor that accepts a
ServerConnection to match usage; change the ConnectTimeout field in the
ServerConnection class from string to int (symbol: ConnectTimeout in class
ServerConnection) and add a new constructor in the Server class with signature
Server(ServerConnection connection) (place it alongside the existing Server()
and Server(string name) constructors) that initializes the Server instance from
the provided ServerConnection object.
|
Hmm. I wonder if adding the output type to Connect-SQL caused this... |
|
I can't see what is going on, it just feels like |
OutputType is only used by IDE's for determining intellisense etc. it not used by the parser... 🤔 |
|
There are other resources faling to, they use PSDscRunAsCredential to login as another user... Try adding this in Connect-Sql so we can see what assemblies are actually available: [System.AppDomain]::CurrentDomain.GetAssemblies() | ? Location -like *SqlServer* | % { Write-Verbose ("GAC:{0}`t`tVersion:{1}`t`tLocation:{2}" -f $_.GlobalAssemblyCache,$_.ImageRuntimeVersion,$_.Location) -Verbose } |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1`:
- Line 96: Replace the forced Write-Verbose assembly dump with a localized,
opt-in debug message: change the pipeline starting at
[System.AppDomain]::CurrentDomain.GetAssemblies() … ForEach-Object {
Write-Verbose ... -Verbose } to use Write-Debug and a localized string (e.g.,
get-string 'SqlServerDsc_AssemblyDump' or 'AssemblyDumpFormat') formatted with
$_.GlobalAssemblyCache,$_.ImageRuntimeVersion,$_.Location; remove the explicit
-Verbose switch so callers control verbosity, and add the new localization
key/value to the module strings file.
♻️ Duplicate comments (1)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (1)
275-283: Null-guardconnectTimerandsqlServerObjectinfinally.Same concern as prior review: if construction/Connect fails, these can be
$nulland mask the original error.
🧹 Nitpick comments (1)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (1)
133-133: Prefer::new()for .NET type construction instead ofNew-Object.PowerShell 5.0 (the project minimum) fully supports the
::new()syntax. This aligns with project guidelines and reduces reliance onNew-Objectfor standard .NET types.♻️ Suggested refactor
-$sqlConnectionContext = New-Object -TypeName Microsoft.SqlServer.Management.Common.ServerConnection +$sqlConnectionContext = [Microsoft.SqlServer.Management.Common.ServerConnection]::new()-$sqlServerObject = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server -ArgumentList $sqlConnectionContext +$sqlServerObject = [Microsoft.SqlServer.Management.Smo.Server]::new($sqlConnectionContext)Also applies to: line 184
| $Encrypt | ||
| ) | ||
|
|
||
| [System.AppDomain]::CurrentDomain.GetAssemblies() | Where-Object { $_.Location -like '*SqlServer*' } | ForEach-Object { Write-Verbose ("GAC:{0}`t`tVersion:{1}`t`tLocation:{2}" -f $_.GlobalAssemblyCache,$_.ImageRuntimeVersion,$_.Location) -Verbose } |
There was a problem hiding this comment.
Localize the assembly dump and avoid forcing verbose output.
This message is hardcoded and uses -Verbose, which bypasses caller preferences. It also reads like internal diagnostics; Write-Debug fits better and still allows opt‑in visibility. Also ensure a localization key is added in the strings file. As per coding guidelines, use localized strings for all diagnostic messages.
🔧 Proposed adjustment
-$assemblies = [System.AppDomain]::CurrentDomain.GetAssemblies() | Where-Object { $_.Location -like '*SqlServer*' } | ForEach-Object { Write-Verbose ("GAC:{0}`t`tVersion:{1}`t`tLocation:{2}" -f $_.GlobalAssemblyCache,$_.ImageRuntimeVersion,$_.Location) -Verbose }
+$assemblies = [System.AppDomain]::CurrentDomain.GetAssemblies() |
+ Where-Object { $_.Location -like '*SqlServer*' }
+
+foreach ($assembly in $assemblies)
+{
+ $message = $script:localizedData.Connect_SqlServerAssemblyLoaded -f @(
+ $assembly.GlobalAssemblyCache,
+ $assembly.ImageRuntimeVersion,
+ $assembly.Location
+ )
+
+ Write-Debug -Message $message
+}🤖 Prompt for AI Agents
In `@source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1` at line 96,
Replace the forced Write-Verbose assembly dump with a localized, opt-in debug
message: change the pipeline starting at
[System.AppDomain]::CurrentDomain.GetAssemblies() … ForEach-Object {
Write-Verbose ... -Verbose } to use Write-Debug and a localized string (e.g.,
get-string 'SqlServerDsc_AssemblyDump' or 'AssemblyDumpFormat') formatted with
$_.GlobalAssemblyCache,$_.ImageRuntimeVersion,$_.Location; remove the explicit
-Verbose switch so callers control verbosity, and add the new localization
key/value to the module strings file.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (1)
133-137: SetConnectTimeoutto an appropriate value separate fromStatementTimeout.Line 136 sets
ConnectTimeoutto$StatementTimeout(default 600 seconds). These serve different purposes:
StatementTimeout: Query execution durationConnectTimeout: Connection establishment durationThe SMO default for
ConnectTimeoutis 30 seconds. Setting it to 600 seconds is excessive for connection establishment and masks real connection/network issues. Use a separate, smaller timeout value (15–30 seconds) or allow it to use the SMO default instead.
♻️ Duplicate comments (2)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (2)
96-97: Localize the assembly diagnostic and useWrite-Debuginstead of forced verbose.This line uses a hardcoded string and forces
-Verboseoutput, bypassing caller preferences. Per coding guidelines, all diagnostic messages must use localized strings from$script:localizedData, and internal diagnostics should useWrite-Debug.🔧 Proposed fix
- [System.AppDomain]::CurrentDomain.GetAssemblies() | Where-Object { $_.Location -like '*SqlServer*' } | ForEach-Object { Write-Verbose ("GAC:{0}`t`tVersion:{1}`t`tLocation:{2}" -f $_.GlobalAssemblyCache, $_.ImageRuntimeVersion, $_.Location) -Verbose } - + $assemblies = [System.AppDomain]::CurrentDomain.GetAssemblies() | + Where-Object -FilterScript { $_.Location -like '*SqlServer*' } + + foreach ($assembly in $assemblies) + { + Write-Debug -Message ( + $script:localizedData.Connect_SqlServerAssemblyLoaded -f @( + $assembly.GlobalAssemblyCache, + $assembly.ImageRuntimeVersion, + $assembly.Location + ) + ) + }Also add the localization key to the strings file:
Connect_SqlServerAssemblyLoaded = GAC: {0}, Version: {1}, Location: {2}. (CS0003)
273-284: Add null guards in thefinallyblock to prevent masking original exceptions.If the
Serverconstructor (Line 184) orConnect()(Line 192) throws,$sqlServerObjectwill be null and Line 283 will throw aNullReferenceException, masking the original failure.🐛 Proposed fix
finally { - $connectTimer.Stop() + if ($null -ne $connectTimer) + { + $connectTimer.Stop() + } + <# Connect() will ensure we actually can connect, but we need to disconnect from the session so we don't have anything hanging. If we need run a method on the returned $sqlServerObject it will automatically open a new session and then close, therefore we don't need to keep this session open. #> - $sqlServerObject.ConnectionContext.Disconnect() + if ($null -ne $sqlServerObject -and $null -ne $sqlServerObject.ConnectionContext) + { + $sqlServerObject.ConnectionContext.Disconnect() + } }
🧹 Nitpick comments (1)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (1)
1-51: Missing INPUTS and OUTPUTS sections in comment-based help.As per coding guidelines, comment-based help must include INPUTS and OUTPUTS sections. The OutputType attribute was added, but the corresponding documentation is missing.
📝 Suggested addition before line 33
+ .INPUTS + None. + + .OUTPUTS + `Microsoft.SqlServer.Management.Smo.Server` + + Returns a SQL Server Management Object (SMO) Server object representing + the connected SQL Server Database Engine instance. + .EXAMPLE
|
@johlju Is there a way to override the logic to workout which tests to run? |
What do you mean? not following you 🙂 |
|
It looks like it doesn't find any assemblies when it fails, could you add a debug write-verbose that outputs all the modules (SQLPS, SqlServer) in Connect-Sql: Write-Verbose -Message (Get-Module -Name *sql* -ListAvailable | Out-String) -VerboseThis is very, doesn't understand why this happens with the changes in the PR. |
|
You can also make the stage |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1`:
- Around line 96-100: Remove the temporary debugging verbose outputs: delete the
Write-Verbose (Get-Module ... | Out-String) call and the assembly enumeration
block that writes verbose lines (the pipeline starting with
[System.AppDomain]::CurrentDomain.GetAssemblies() ... ForEach-Object {
Write-Verbose (...) }) so the function no longer forces verbose output; keep
Import-SqlDscPreferredModule intact, and if persistent diagnostics are required
replace the removed messages with localized Write-Debug calls (or proper
localized logging) tied to the Connect-Sql function's debug logging rather than
unconditional Write-Verbose.
♻️ Duplicate comments (1)
source/Modules/SqlServerDsc.Common/Public/Connect-Sql.ps1 (1)
275-286: Previous feedback not yet addressed: Add null guards infinallyblock.The null guard issue flagged in a previous review remains unresolved. If
Serverconstruction orConnect()throws, calling$connectTimer.Stop()or$sqlServerObject.ConnectionContext.Disconnect()on$nullwill throw aNullReferenceException, masking the original failure.
You were correct, it is the output type, I thought it wasn't parsed, but it is. My bad... Example showing the parse error: PS> function Connect-Sql
>> {
>> [OutputType([Microsoft.SqlServer.Management.Smo.Server])]
>> [CmdletBinding()]
>> param (
>> [Parameter()]
>> [String]
>> $InstanceName
>> )
>>
>> Write-Verbose -Message "Connecting to SQL Server instance '$InstanceName'..."
>> }
PS> connect-sql
InvalidOperation:
Line |
4 | [OutputType([Microsoft.SqlServer.Management.Smo.Server])]
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| Unable to find type [Microsoft.SqlServer.Management.Smo.Server]. |
|
I just realized that we maybe have a flaw in the integration tests. We are installing the SqlServer module as part of the configuration prior to installing the instances. But at that point Pester has already imported the SqlServerDsc module which might mean SqlServerDsc will not be re-imported after SqlServer module have been installed (so SqlServerDsc imports SqlServer so it loads SMO assemblies). A real scenario would be to install SqlServer module before invoking Pester, like a normal user would do. That would probably make OutputType work with the Server type. But there is also the scenario where a user can install SqlServerDsc, make use of its public commands to download server media and install an instance. After that install SqlServer module. Then use the Connect-public command to connect to that newly installed instance. Without having import call inside Connect-Sql that would fail because in this scenario SMO would not be available, and the OutputType would fail in this scenario. Maybe it possible to guard against it the later scenario, and fix the integration tests to handle the former scenario. @coderabbitai can you investigate this issue and the create a GitHub issue to track it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 21042 🏁 Script executed: Length of output: 28748
🏁 Script executed: Length of output: 88 🏁 Script executed: Length of output: 56
Summary of FindingsThe root cause is that PowerShell parses and validates the Two Affected Scenarios:
Key Evidence:
Recommended Solutions:Short-term: Change Long-term: Restructure integration tests to install SqlServer module before importing SqlServerDsc, mirroring real-world usage patterns. All details, including five potential solution options, are documented in issue 🧠 Learnings used |
|
Another option would be to move out Inport-* from inside Connect-Sql and let the user or resources/commands always call Import-* prior to calling Connect-*. That would solve OutputType problem as well 🤔 But for now I suggest just switching to System.Object and we look at it in another PRs. |
|
Changed the OutputType to System.Object. Fixes everything! |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 9 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dan-hughes).
|
Great work! |
3 participants
Pull Request (PR) description
Create Server and ServerConnection objects as per documentation.
Prevent Invoke-SqlDscQuery from closing the connection which is not recommended.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is