From d703df9c64427ab93243375d8a460bbfdf29adfe Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Fri, 14 Aug 2026 15:03:32 +0200 Subject: [PATCH 1/2] Database context - Put the database back in the Query and Invoke script methods The Query and Invoke script methods of Server and Database do not run on a private connection. The execution manager of an SMO database is the connection context of the parent server, which belongs to the caller, so these methods issued a USE and never switched back. Every command using them handed the connection back pointing at a different database, and everything the caller ran afterwards silently executed in the wrong one. All four methods now remember ConnectionContext.CurrentDatabase and put it back in a finally, so a failing query restores it too. The Server pair needs the same treatment of its own, because Server.Query and Server.Invoke call $this.Databases[$Database].ExecuteWithResults() directly and never go through the Database methods. Restoring rather than running on a copied connection is deliberate. A copy works, but it is a different session: it cannot see the temp tables or SET options of the caller, and it opens a connection per call. Restoring keeps the session, and costs one round trip only when the database actually moved. The database the caller was on is restored, not master. A connection sitting in msdb is returned to msdb - restoring to master would have passed every other test and still been wrong. This covers the script methods only. The direct SMO calls of #10555, and SMO's own Create() and Drop(), are untouched and still leak - Invoke-DbaDbUpgrade in #10556 is one of those. (do *) Co-Authored-By: Claude Opus 5 (1M context) --- tests/InModule.TypeExtensions.Tests.ps1 | 122 ++++++++++++++++++++++++ xml/dbatools.Types.ps1xml | 59 +++++++++++- 2 files changed, 176 insertions(+), 5 deletions(-) create mode 100644 tests/InModule.TypeExtensions.Tests.ps1 diff --git a/tests/InModule.TypeExtensions.Tests.ps1 b/tests/InModule.TypeExtensions.Tests.ps1 new file mode 100644 index 000000000000..adb3b44afb8a --- /dev/null +++ b/tests/InModule.TypeExtensions.Tests.ps1 @@ -0,0 +1,122 @@ +#Requires -Module @{ ModuleName="Pester"; ModuleVersion="5.0" } +param( + $ModuleName = "dbatools", + $CommandName = "InModule.TypeExtensions", + $PSDefaultParameterValues = $TestConfig.Defaults +) + +Describe $CommandName -Tag IntegrationTests { + # The Query and Invoke script methods of Server and Database in xml\dbatools.Types.ps1xml run through + # the execution manager of a database, which is the connection context of the parent server and belongs + # to the caller. SMO issues a USE and never switches back, so the methods put the database back. See #10555. + BeforeAll { + # We want to run all commands in the BeforeAll block with EnableException to ensure that the test fails if the setup fails. + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $contextDbName = "dbatoolsci_typeext_$(Get-Random)" + $null = New-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Name $contextDbName + + # We want to run all commands outside of the BeforeAll block without EnableException to be able to test for specific warnings. + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + # We want to run all commands in the AfterAll block with EnableException to ensure that the test fails if the cleanup fails. + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = Remove-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Database $contextDbName -ErrorAction SilentlyContinue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + Context "The database context of the caller survives the script methods (#10555)" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # Only a non-pooled connection can show this. SMO reopens a pooled connection at its default + # database, which hides the leak. + $callerServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -NonPooledConnection + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $callerServer | Disconnect-DbaInstance + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "leaves the database context alone in Database.Query" { + $null = $callerServer.Databases[$contextDbName].Query("SELECT 1") + $callerServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "master" + } + + It "leaves the database context alone in Database.Invoke" { + $null = $callerServer.Databases[$contextDbName].Invoke("SELECT 1") + $callerServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "master" + } + + It "leaves the database context alone in Server.Query with a database" { + $null = $callerServer.Query("SELECT 1", $contextDbName) + $callerServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "master" + } + + It "leaves the database context alone in Server.Invoke with a database" { + $null = $callerServer.Invoke("SELECT 1", $contextDbName) + $callerServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "master" + } + + It "still runs the query in the database that was asked for" { + $callerServer.Databases[$contextDbName].Query("SELECT DB_NAME() AS dbname").dbname | Should -Be $contextDbName + $callerServer.Query("SELECT DB_NAME() AS dbname", $contextDbName).dbname | Should -Be $contextDbName + } + + It "still returns every table when AllTables is used" { + $allTables = $callerServer.Databases[$contextDbName].Query("SELECT 1 AS a; SELECT 2 AS b", $true) + $allTables.Count | Should -Be 2 + } + + It "keeps the session, so temporary objects survive" { + $null = $callerServer.ConnectionContext.ExecuteNonQuery("CREATE TABLE #dbatoolsci_marker (id INT)") + $null = $callerServer.Databases[$contextDbName].Query("SELECT 1") + { $callerServer.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #dbatoolsci_marker") } | Should -Not -Throw + } + + It "puts the database back even when the query fails" { + { $callerServer.Databases[$contextDbName].Query("SELECT * FROM dbatoolsci_does_not_exist") } | Should -Throw + $callerServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "master" + } + } + + Context "The database the caller was on is restored, not master (#10555)" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # A connection that starts out somewhere other than master. Restoring to master would pass the + # tests above and still be wrong here. + $msdbServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -Database msdb -NonPooledConnection + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $msdbServer | Disconnect-DbaInstance + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "leaves the connection in msdb after Database.Query" { + $null = $msdbServer.Databases[$contextDbName].Query("SELECT 1") + $msdbServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "msdb" + } + + It "leaves the connection in msdb after Server.Query with a database" { + $null = $msdbServer.Query("SELECT 1", $contextDbName) + $msdbServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "msdb" + } + } +} diff --git a/xml/dbatools.Types.ps1xml b/xml/dbatools.Types.ps1xml index 7ceb92e5afb6..f9b10670e6fe 100644 --- a/xml/dbatools.Types.ps1xml +++ b/xml/dbatools.Types.ps1xml @@ -12,8 +12,21 @@ param ( $AllTables = $false ) -if ($AllTables) { ($this.ExecuteWithResults($Query)).Tables } -else { ($this.ExecuteWithResults($Query)).Tables[0] } +# ExecuteWithResults does not run on a private connection: the execution manager of a database is the +# connection context of the parent server, which belongs to the caller. It issues a USE and never switches +# back, so the previous database is put back here. See #10555. +$connectionContext = $this.Parent.ConnectionContext +$previousDatabase = $connectionContext.CurrentDatabase + +try { + if ($AllTables) { ($this.ExecuteWithResults($Query)).Tables } + else { ($this.ExecuteWithResults($Query)).Tables[0] } +} finally { + if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + $escapedDatabase = $previousDatabase.Replace("]", "]]") + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } +} @@ -22,7 +35,20 @@ else { ($this.ExecuteWithResults($Query)).Tables[0] } param ( $Command ) -$this.ExecuteNonQuery($Command) + +# See the comment on Query above: this leaves the connection of the caller in this database, so the +# previous database is put back afterwards. See #10555. +$connectionContext = $this.Parent.ConnectionContext +$previousDatabase = $connectionContext.CurrentDatabase + +try { + $this.ExecuteNonQuery($Command) +} finally { + if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + $escapedDatabase = $previousDatabase.Replace("]", "]]") + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } +} @@ -43,7 +69,19 @@ param ( try { if ($Database) { - $dataSet = $this.Databases[$Database].ExecuteWithResults($Query) + # Going through the database object leaves the connection of the caller in that database, so the + # previous database is put back afterwards. This does not go through Database.Query, so it needs the + # same treatment of its own. See #10555. + $connectionContext = $this.ConnectionContext + $previousDatabase = $connectionContext.CurrentDatabase + try { + $dataSet = $this.Databases[$Database].ExecuteWithResults($Query) + } finally { + if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + $escapedDatabase = $previousDatabase.Replace("]", "]]") + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } + } } else { $dataSet = $this.ConnectionContext.ExecuteWithResults($Query) } @@ -74,7 +112,18 @@ param ( try { if ($Database) { - $this.Databases[$Database].ExecuteNonQuery($Command) + # See the comment on Query above: this does not go through Database.Invoke either, so the previous + # database is put back here as well. See #10555. + $connectionContext = $this.ConnectionContext + $previousDatabase = $connectionContext.CurrentDatabase + try { + $this.Databases[$Database].ExecuteNonQuery($Command) + } finally { + if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + $escapedDatabase = $previousDatabase.Replace("]", "]]") + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } + } } else { $this.ConnectionContext.ExecuteNonQuery($Command) } From cfce992a7ec4b7aed620ee2a856eb78c38637f9d Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Fri, 21 Aug 2026 21:49:34 +0200 Subject: [PATCH 2/2] Database context - Compare case sensitively and never let the restore become the outcome Two fixes to the four script methods, both from the review on #10579. The comparison that decides whether the database has to be put back was case insensitive, so on an instance with a case sensitive collation it reported AppDb and appdb as equal and skipped the restore, leaving the caller in the wrong database - the very leak this change is about, on a valid configuration. It is -cne now, which cannot restore needlessly, because both sides are read from the same property and one database always spells itself the same way. The USE in the finally was unguarded, so a statement that made the previous database unreachable - taking it offline, dropping it, renaming it, revoking access - threw although it had succeeded, and a caller reading that as "it did not run" might run it a second time. A failing statement had its own error replaced for the same reason. Restoring is housekeeping and warns now instead of throwing. A Write-Warning inside a script method can be suppressed by the caller through WarningPreference or WarningAction, but it cannot be captured with WarningVariable or 3>&1, so the test sets the preference and asserts on behaviour rather than on the warning text. Tests: the temporary table test proved nothing about the session, because the table never left the caller's connection and a copied context would have passed it as well. It queries the marker through the wrapper and compares @@SPID now, so the candidate that was rejected in the design fails it. Two new contexts. The failing restore returns normally and really does take the database offline. Databases whose names differ only in case are told apart - guarded by a BeforeDiscovery probe of the instance collation, because the scenario cannot be built at all on a case insensitive instance. It skips on the current CI instance and passes against a case sensitive one. Both were verified to fail against the old code. 16 test files of wrapper using commands, 116 tests, no failures. (do Connect-DbaInstance, Invoke-DbaQuery, Get-DbaDatabase) --- tests/InModule.TypeExtensions.Tests.ps1 | 104 +++++++++++++++++++++++- xml/dbatools.Types.ps1xml | 44 ++++++++-- 2 files changed, 137 insertions(+), 11 deletions(-) diff --git a/tests/InModule.TypeExtensions.Tests.ps1 b/tests/InModule.TypeExtensions.Tests.ps1 index adb3b44afb8a..fc47728bad91 100644 --- a/tests/InModule.TypeExtensions.Tests.ps1 +++ b/tests/InModule.TypeExtensions.Tests.ps1 @@ -9,6 +9,16 @@ Describe $CommandName -Tag IntegrationTests { # The Query and Invoke script methods of Server and Database in xml\dbatools.Types.ps1xml run through # the execution manager of a database, which is the connection context of the parent server and belongs # to the caller. SMO issues a USE and never switches back, so the methods put the database back. See #10555. + BeforeDiscovery { + # Two databases whose names differ only in case can only exist on an instance with a case sensitive + # collation, and that is the only place where a case insensitive comparison in the restore can be + # caught. The collation decides it, not the version, so this is asked of the instance rather than + # assumed. On a case insensitive instance the scenario cannot be built at all and the Context skips. + $caseDiscoveryServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle + $instanceIsCaseSensitive = $caseDiscoveryServer.Collation -match "_CS_" + $null = $caseDiscoveryServer | Disconnect-DbaInstance + } + BeforeAll { # We want to run all commands in the BeforeAll block with EnableException to ensure that the test fails if the setup fails. $PSDefaultParameterValues["*-Dba*:EnableException"] = $true @@ -78,10 +88,18 @@ Describe $CommandName -Tag IntegrationTests { $allTables.Count | Should -Be 2 } - It "keeps the session, so temporary objects survive" { + It "runs on the session of the caller and not on a copy of the connection" { + # Checking the temporary table through the caller afterwards proves nothing: it never left the + # caller's session, so a copied connection context would pass that too. The wrapper itself has + # to see the table, and its SPID has to be the caller's. ConnectionContext.Copy() plus + # GetDatabaseConnection() was the other candidate for this fix and fails both assertions. + $callerSpid = $callerServer.ConnectionContext.ExecuteScalar("SELECT @@SPID") $null = $callerServer.ConnectionContext.ExecuteNonQuery("CREATE TABLE #dbatoolsci_marker (id INT)") - $null = $callerServer.Databases[$contextDbName].Query("SELECT 1") - { $callerServer.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #dbatoolsci_marker") } | Should -Not -Throw + + $wrapperResult = $callerServer.Databases[$contextDbName].Query("SELECT @@SPID AS spid, (SELECT COUNT(*) FROM #dbatoolsci_marker) AS marker") + + $wrapperResult.spid | Should -Be $callerSpid + $wrapperResult.marker | Should -Be 0 } It "puts the database back even when the query fails" { @@ -119,4 +137,84 @@ Describe $CommandName -Tag IntegrationTests { $msdbServer.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -Be "msdb" } } + + Context "A failing restore does not become the outcome of the call (#10555)" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $offlineDbName = "dbatoolsci_typeext_offline_$(Get-Random)" + $null = New-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Name $offlineDbName + $offlineServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -Database $offlineDbName -NonPooledConnection + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $offlineServer | Disconnect-DbaInstance + $null = Set-DbaDbState -SqlInstance $TestConfig.InstanceSingle -Database $offlineDbName -Online -Force -ErrorAction SilentlyContinue + $null = Remove-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Database $offlineDbName -ErrorAction SilentlyContinue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "returns normally when the statement made the previous database unreachable" { + # The wrapper moves the connection to master to run this, and the USE that would put it back + # cannot work afterwards, because by then the database it names is offline. Restoring is + # housekeeping and must not turn a statement that succeeded into an exception: a caller reading + # that as "it did not run" might well run it a second time. The wrapper warns instead, which a + # script method can only write to the host, so the preference is set rather than captured. + $WarningPreference = "SilentlyContinue" + $offlineStatement = "ALTER DATABASE [$offlineDbName] SET OFFLINE WITH ROLLBACK IMMEDIATE" + + { $offlineServer.Databases["master"].Invoke($offlineStatement) } | Should -Not -Throw + + (Get-DbaDbState -SqlInstance $TestConfig.InstanceSingle -Database $offlineDbName).Status | Should -Be "OFFLINE" + } + } + + Context "Databases whose names differ only in case are told apart (#10579)" -Skip:(-not $instanceIsCaseSensitive) { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $caseSuffix = Get-Random + $caseDbMixed = "dbatoolsci_CaseCtx$caseSuffix" + $caseDbLower = "dbatoolsci_casectx$caseSuffix" + + # The two names may differ only in case, but their files may not: NTFS is case insensitive, so + # names derived from the database name collide, first the mdf and then the ldf. So the second + # database is created under a name of its own and renamed afterwards, which keeps its files + # apart and needs no explicit file paths. + $caseDbTemporary = "dbatoolsci_casetmp$caseSuffix" + $null = New-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Name $caseDbMixed + $null = New-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Name $caseDbTemporary + $null = Invoke-DbaQuery -SqlInstance $TestConfig.InstanceSingle -Query "ALTER DATABASE [$caseDbTemporary] MODIFY NAME = [$caseDbLower]" + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + foreach ($caseDbName in $caseDbMixed, $caseDbLower, $caseDbTemporary) { + $null = Remove-DbaDatabase -SqlInstance $TestConfig.InstanceSingle -Database $caseDbName -ErrorAction SilentlyContinue + } + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "restores the database of the caller when the two names differ only in case" { + # A case insensitive comparison reports the two as equal and skips the restore, so the caller is + # left in the wrong database - the very leak this change is about, on a valid configuration. + $caseCaller = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -Database $caseDbMixed -NonPooledConnection + try { + $null = $caseCaller.Databases[$caseDbLower].Query("SELECT 1") + + $caseCaller.ConnectionContext.ExecuteScalar("SELECT DB_NAME()") | Should -BeExactly $caseDbMixed + } finally { + $null = $caseCaller | Disconnect-DbaInstance + } + } + } } diff --git a/xml/dbatools.Types.ps1xml b/xml/dbatools.Types.ps1xml index f9b10670e6fe..df880e1bc570 100644 --- a/xml/dbatools.Types.ps1xml +++ b/xml/dbatools.Types.ps1xml @@ -22,9 +22,22 @@ try { if ($AllTables) { ($this.ExecuteWithResults($Query)).Tables } else { ($this.ExecuteWithResults($Query)).Tables[0] } } finally { - if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + # The comparison is case sensitive on purpose. Database names use the collation of the instance, so + # on a case sensitive instance AppDb and appdb are two different databases, and -ne would report + # them as equal and skip the restore. It cannot restore needlessly: both sides are read from the + # same property, so the same database always spells itself the same way. + if ($previousDatabase -and $connectionContext.CurrentDatabase -cne $previousDatabase) { $escapedDatabase = $previousDatabase.Replace("]", "]]") - $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + try { + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } catch { + # Putting the database back is housekeeping and must never become the outcome of the call. + # A query that makes the previous database unreachable - taking it offline, dropping it, + # renaming it, revoking access - would otherwise throw here although it succeeded, and the + # caller would read that as "it did not run" and might do it a second time. A query that + # failed would have its own error replaced by this one, which is just as misleading. + Write-Warning "The database context could not be restored to [$previousDatabase]: $($_.Exception.Message)" + } } } @@ -44,9 +57,14 @@ $previousDatabase = $connectionContext.CurrentDatabase try { $this.ExecuteNonQuery($Command) } finally { - if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + # See the comment on Query above for why this is case sensitive and why a failing restore only warns. + if ($previousDatabase -and $connectionContext.CurrentDatabase -cne $previousDatabase) { $escapedDatabase = $previousDatabase.Replace("]", "]]") - $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + try { + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } catch { + Write-Warning "The database context could not be restored to [$previousDatabase]: $($_.Exception.Message)" + } } } @@ -77,9 +95,14 @@ try { try { $dataSet = $this.Databases[$Database].ExecuteWithResults($Query) } finally { - if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + # See the comment on Database.Query for why this is case sensitive and why a failing restore only warns. + if ($previousDatabase -and $connectionContext.CurrentDatabase -cne $previousDatabase) { $escapedDatabase = $previousDatabase.Replace("]", "]]") - $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + try { + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } catch { + Write-Warning "The database context could not be restored to [$previousDatabase]: $($_.Exception.Message)" + } } } } else { @@ -119,9 +142,14 @@ try { try { $this.Databases[$Database].ExecuteNonQuery($Command) } finally { - if ($previousDatabase -and $connectionContext.CurrentDatabase -ne $previousDatabase) { + # See the comment on Database.Query for why this is case sensitive and why a failing restore only warns. + if ($previousDatabase -and $connectionContext.CurrentDatabase -cne $previousDatabase) { $escapedDatabase = $previousDatabase.Replace("]", "]]") - $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + try { + $null = $connectionContext.ExecuteNonQuery("USE [$escapedDatabase]") + } catch { + Write-Warning "The database context could not be restored to [$previousDatabase]: $($_.Exception.Message)" + } } } } else {