Repository navigation
Copy commands - Stop writing $null into the output of 17 commands - #10795
Conversation
…ed job Query() returns $null for a batch without a result set, and the call had no $null = in front of it. The script of every copied job went through it, so each job put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaAgentJob) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ery copied category Query() returns $null for a batch without a result set, and the call had no $null = in front of it. All five sites - job, operator and alert categories, each created new or recreated with -Force - put a $null in front of the result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaAgentJobCategory) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… copied step Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every step of a synchronized job put a $null in front of the result of the job. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaAgentJobStep) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… copied operator Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied operator put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaAgentOperator) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pied proxy Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied proxy put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaAgentProxy) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… copied schedule Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied schedule put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaAgentSchedule) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…copied device Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied backup device put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. The copy in the test is limited to the device of the test, so the count does not depend on other devices of the instance. Part of #10794 (do Copy-DbaBackupDevice) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…opied message Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied message - one per language - put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaCustomError) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… copied collection set Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every migrated collection set put a $null in front of its result. The second site, the server reconfiguration, cannot run: -NoServerReconfig is always set to $true just before it. No test: copying collection sets needs a configured Data Collector on both instances, which neither CI nor the lab has. Part of #10794 (do Copy-DbaDataCollector) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pied assembly Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Setting TRUSTWORTHY and dropping an existing assembly with -Force each put a $null in front of the result. The create call passed the database name as the second argument of Database.Query, which is AllTables, not a database. That returned the empty table collection and so wrote nothing, but only by accident; the argument is gone. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaDbAssembly) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ons with -Force and stop writing $null Three defects of the same command: - Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied audit put a $null in front of its result. - An audit that exists on the destination got the status Skipped, but the object was never written, so the caller got nothing for it. - -Force is meant to drop the server audit specifications that use the audit before it replaces the audit, but the loop went over $spec and tested and dropped $auditSpecification, a variable that does not exist. No specification was ever dropped; they stayed on the destination while the audit was replaced underneath them. The specifications are collected first, then disabled and dropped. The command had no integration test. The new one copies an audit, copies it again to get the skipped result and replaces it with -Force while an enabled specification uses it on the destination. Part of #10794 (do Copy-DbaInstanceAudit) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tput of every copied specification Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Every copied specification put a $null in front of its result. The command had no integration test. The new one copies a specification whose audit exists on both instances and counts the output. Part of #10794 (do Copy-DbaInstanceAuditSpecification) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d servers with another network name Query() returns $null for a batch without a result set, and the call had no $null = in front of it. A SQL Server linked server whose name differs from its data source gets its network name set with sp_setnetname after the copy, and that call put a $null in front of the result. The existing test copies a linked server whose name is its data source, so it never got there. A new context copies one whose network name was changed with sp_setnetname, counts the output and checks the network name on the destination. Part of #10794 (do Copy-DbaLinkedServer) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Force, and stop writing $null Three defects of the same command: - Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Sixteen sites - the classifier function, the reconfigurations, every pool and every workload group - put $null between the results. - -ExcludeResourcePool assigned the filtered pools to $pool instead of $pools, so no pool was copied at all. The filter now also leaves out the system pools internal and default, as the default path does. - -Force dropped the workload groups of an existing pool while it enumerated their collection, which failed with "Collection was modified", so replacing a pool that has workload groups always reported Failed. The groups are collected first. The test counts the output against the status values, because their number depends on the edition, and a new case copies with -ExcludeResourcePool and -Force over the pools of the first copy. Part of #10794 (do Copy-DbaResourceGovernor) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pied role Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Creating the role and granting each of its permissions put a $null in front of its result. The test counts the output itself now: $results.Status.Count skips a $null, which is how the leak went unnoticed. Part of #10794 (do Copy-DbaServerRole) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-Force Query() returns $null for a batch without a result set, and the call had no $null = in front of it. Dropping a startup procedure that exists on the destination put a $null in front of its result. The test had no -Force case. The new one replaces the procedure that the first context copied and counts the output. Part of #10794 (do Copy-DbaStartupProcedure) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… of the classic mode Query() returns $null for a batch without a result set, and the call had no $null = in front of it. The classic mode runs every statement of the transfer script through it, so it returned one $null per successful statement, while its documentation says it returns no output. The -Classic test asserted that the output is not empty, which only passed because of those $null values. It now asserts that there is no output. Part of #10794 (do Copy-DbaSystemDbUserObject) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed at 32a9d40504547d07c8e79d248de2f6f374107795. No material defect found.
Reviewed the complete 33-file patch, all 40 changed Query call sites, surrounding migration/status/error paths, tests, description, discussions, and exact-head CI. The suppressed calls perform administrative SQL and retain the intended migration result objects. The assembly-query argument correction, skipped-audit output, forced audit-specification handling, and Resource Governor collection/filter changes were also checked.
A focused PowerShell 7.6.7 check of the actual Server.Query script method confirmed the extra null output and showed why Status.Count missed it. The updated tests count actual output while retaining meaningful state/status assertions. All 20 executed head checks passed and one Windows check was skipped. SQL integration tests were not rerun locally; the Data Collector suppression sites have no configured integration boundary.
|
awesome 🙌🏼 |
Part of #10794: the 17 Copy commands. The instance level commands and the database level commands follow in two more PRs, and the two sites of Invoke-DbaDbDataMasking and Invoke-DbaDbLogShipRecovery are fixed in #10791.
One commit per command with its test, as in the tier PRs.
The defect
The
Query()script methods of the SMO Server and Database objects (xml/dbatools.Types.ps1xml) return(...ExecuteWithResults($Query)).Tables[0], which is$nullfor a batch without a result set: CREATE, DROP, ALTER, EXEC of most procedures. Called as a statement of its own, the call writes that$nullinto the output of the command. SoCopy-DbaAgentOperatorfor two operators returned four objects, two of them$null, and@(Copy-DbaAgentJob ...).Countwas one too high for every job.The fix is
$null =in front of the 40 calls, as most of the code base already does.The tests did not notice because they count with
$results.Status.Countor read$results.Name. Member enumeration skips$null, so both look right while the output has twice as many items. Every touched test counts the output itself now:@($results).Count. The test guidetests/CLAUDE.mdrecommends$results.Status.Count"for accurate counting in dbatools context", which is exactly the pattern that hid this. That is for the follow-up, not this PR.Plain
$null =fixes$nullCommands with more than the
$nullFixed in the same commit, so that no command is left half fixed.
Copy-DbaDbAssembly: two sites (TRUSTWORTHY, -Force drop). The create call passed the database name as the second argument of
Database.Query, which isAllTables, not a database. That returned the empty table collection and so wrote nothing, but only by accident. The argument is gone; the call runs in the right database anyway, because it goes through the destination database object.Copy-DbaInstanceAudit: one site, plus
-Forceis meant to drop the server audit specifications that use the audit before it replaces the audit, but the loop went over$specand tested and dropped$auditSpecification, a variable that does not exist. No specification was ever dropped; they stayed on the destination while the audit was replaced underneath them. The specifications are collected first (a drop inside the enumeration would modify the collection), then disabled and dropped.Copy-DbaLinkedServer: the
sp_setnetnamefor a SQL Server linked server whose name differs from its data source. Its hunk is 80 lines away from the one in #10792, so the two PRs do not conflict.Copy-DbaResourceGovernor: 16 sites, plus
-ExcludeResourcePoolassigned the filtered pools to$poolinstead of$pools, so no pool was copied at all. The filter now also leaves out the system pools internal and default, as the default path does;-Forcedropped the workload groups of an existing pool while it enumerated their collection, which failed with "Collection was modified", so replacing a pool that has workload groups always reported Failed. The groups are collected first. The new-ExcludeResourcePooltest found this one.Copy-DbaSystemDbUserObject: the classic mode ran every statement of its transfer script through
Query(), so it returned one$nullper successful statement, while its documentation says "The Classic mode returns no output". The-Classictest asserted that the output is not empty and passed only because of those$nullvalues; it asserts that there is no output now.Tests
-NoServerReconfigis set to$truejust before it.Lab (set03: SQL Server 2019, 2022 and 2025, Windows PowerShell 5.1): all 16 test files green on this branch. With the command files of development and the tests of this branch, every file fails at exactly the new assertions and nowhere else, for example:
-ExcludeResourcePoolcopied no pool$nullvalues instead of no output-ForceThe
-Forcedefect of Copy-DbaResourceGovernor showed up in between, on the run with only the-ExcludeResourcePoolfix: the pool reported Failed with "Collection was modified".Not in this PR
Query()calls, once the three PRs are merged; until then it would fail on the sites they fix.🤖 Generated with Claude Code