Export-DbaDacPackage - Add thread-safe database enumeration and SQL Server version check

[potatoqualitee]

Summary

This PR implements a thread-safety fix for Export-DbaDacPackage to resolve critical issues when running in parallel PowerShell runspaces.

Changes

1. Thread-Safe Database Enumeration - Replaced SMO-based Get-DbaDatabase with T-SQL query using Invoke-DbaQuery
2. SQL Server Version Check - Added validation for SQL Server 2008 R2+ (DAC Framework minimum version 10.50)
3. Documentation Updates - Updated parameter help and .NOTES to reflect version requirements

Problem

When using Export-DbaDacPackage in parallel runspaces, the function failed with "Databases not found" errors on 73-94% of servers due to SMO thread-safety issues. The original implementation used Get-DbaDatabase which relies on SMO's $server.Databases collection - SMO objects are NOT thread-safe.

Solution

Replace SMO-based database enumeration with T-SQL query-based enumeration. This eliminates SMO object sharing across runspaces while maintaining identical functionality.

Testing Evidence

• 15 SQL Server instances tested (VMs, Azure SQL Database, Azure SQL Managed Instance)
• 541+ total databases extracted successfully
• 100% success rate in parallel execution (vs 26.7% with SMO approach)
• 0 "Databases not found" errors with the T-SQL approach

Fixes #9980

---

Generated with Claude Code

[claude]

Code Review: Export-DbaDacPackage Thread-Safety Fix

Thank you for this PR! The thread-safety fix addresses a critical issue with parallel execution. I've reviewed the changes against the dbatools style guide and have the following feedback:

✅ Strengths

1. Excellent Problem Identification - The thread-safety issue with SMO in parallel runspaces is well-documented and the T-SQL solution is appropriate per the style guide (SMO vs T-SQL usage section)
2. Strong Testing Evidence - 100% success rate across 541+ databases vs 26.7% baseline demonstrates clear improvement
3. Proper Version Check - SQL Server 2008 R2+ requirement (version 10.50) is correctly implemented with both major and minor version checking
4. Good Documentation - Comments explaining the thread-safety rationale are helpful

⚠️ Issues to Address

1. CRITICAL: SQL Injection Vulnerability (lines 226-228)

The ExcludeDatabase filter concatenates database names directly into SQL without parameterization:

# CURRENT (VULNERABLE):
if ($ExcludeDatabase) {
    $excludedDatabaseList = $ExcludeDatabase | ForEach-Object { "'$_"' }
    $query += "`n  AND name NOT IN ($($excludedDatabaseList -join ","))"
}

Problem: If $ExcludeDatabase contains a malicious value like "test'; DROP DATABASE master; --", it would be concatenated directly into the query.

Solution: Use parameterized queries with Invoke-DbaQuery -SqlParameter:

$query = @"
SELECT name
FROM sys.databases
WHERE database_id > 4
  AND state = 0
"@

$sqlParams = @{}

if ($ExcludeDatabase) {
    $placeholders = @()
    for ($i = 0; $i -lt $ExcludeDatabase.Count; $i++) {
        $placeholders += "@exclude$i"
        $sqlParams["exclude$i"] = $ExcludeDatabase[$i]
    }
    $query += "`n  AND name NOT IN ($($placeholders -join ','))"
}

$query += "`nORDER BY name"

$dbNames = Invoke-DbaQuery -SqlInstance $server -Query $query -SqlParameter $sqlParams -EnableException | Select-Object -ExpandProperty name

2. Style Violation: Splat Required for 3+ Parameters (line 233)

Per CLAUDE.md section "SPLAT USAGE REQUIREMENT":

USE SPLATS ONLY FOR 3+ PARAMETERS

# CURRENT (line 233):
$dbNames = Invoke-DbaQuery -SqlInstance $server -Query $query -EnableException | Select-Object -ExpandProperty name

# SHOULD BE:
$splatQuery = @{
    SqlInstance     = $server
    Query           = $query
    EnableException = $true
}
$dbNames = Invoke-DbaQuery @splatQuery | Select-Object -ExpandProperty name

3. Documentation Gap (line 76)

The .PARAMETER SqlInstance help was updated, but the .NOTES section addition should have proper formatting. Remove the blank line at line 76 for consistency with existing style.

4. Minor: Spacing After Stop-Function (line 241)

# Current (missing space):
Stop-Function -Message "Databases not found on $instance"-Target $instance -Continue

# Should be:
Stop-Function -Message "Databases not found on $instance" -Target $instance -Continue

📝 Recommendations

1. Test the SQL Injection Fix - Add a test case with a database name containing special characters (quotes, semicolons, etc.)
2. Consider Edge Cases - What happens if $ExcludeDatabase contains duplicate values? (Minor, but worth considering)
3. Performance Note - The T-SQL approach is actually more efficient than SMO enumeration, so this is a win-win (thread-safe AND faster)

🎯 Priority

1. MUST FIX: SQL injection vulnerability (Issue Revised Get-SQLDefaultPaths function #1)
2. MUST FIX: Splat usage for 3+ parameters (Issue Restore-HallengrenBackups.ps1 - Test-SQLSA Function - Sysadmin check fails if User is a member of an AD Group with SysAdmin permissions #2)
3. SHOULD FIX: Documentation formatting (Issue BackupRestore without SetSourceReadOnly #3)
4. NICE TO FIX: Spacing in error message (Issue Copy-SqlLinkedServer ignored Linked server  #4)

Summary

The core solution (replacing SMO with T-SQL) is excellent and follows dbatools best practices. The SQL injection vulnerability is the critical issue that needs immediate attention before merge. Once the parameterization is fixed, this will be a solid improvement to the codebase.

Great work on identifying and fixing the thread-safety issue! The testing evidence is particularly impressive.

---

Review performed by Claude Code against dbatools CLAUDE.md style guide

[potatoqualitee]

@claude fix it all

[claude]

Claude finished @potatoqualitee's task —— View job

---

All Fixes Implemented ✅

Successfully implemented all 4 code review fixes:

• Read current state of Export-DbaDacPackage.ps1
• Fix Revised Get-SQLDefaultPaths function #1: SQL injection vulnerability - implement parameterized queries
• Fix Restore-HallengrenBackups.ps1 - Test-SQLSA Function - Sysadmin check fails if User is a member of an AD Group with SysAdmin permissions #2: Add splat for Invoke-DbaQuery (3+ parameters)
• Fix BackupRestore without SetSourceReadOnly #3: Remove blank line at line 76 (documentation formatting)
• Fix Copy-SqlLinkedServer ignored Linked server  #4: Verify spacing in error message (already correct)
• Commit and push changes
• Update comment with completion status

Summary of Changes

1. SQL Injection Fix (Critical)
Replaced unsafe string concatenation with parameterized queries:

# Before (vulnerable):
$excludedDatabaseList = $ExcludeDatabase | ForEach-Object { "'$_'" }
$query += "`n  AND name NOT IN ($($excludedDatabaseList -join ","))"

# After (secure):
$sqlParams = @{}
if ($ExcludeDatabase) {
    $placeholders = @()
    for ($i = 0; $i -lt $ExcludeDatabase.Count; $i++) {
        $placeholders += "@exclude$i"
        $sqlParams["exclude$i"] = $ExcludeDatabase[$i]
    }
    $query += "`n  AND name NOT IN ($($placeholders -join ','))"
}

2. Splat Usage (Style Guide Compliance)
Added proper splat for Invoke-DbaQuery with 3+ parameters:

$splatQuery = @{
    SqlInstance     = $server
    Query           = $query
    EnableException = $true
}
if ($sqlParams.Count -gt 0) {
    $splatQuery.SqlParameter = $sqlParams
}
$dbNames = Invoke-DbaQuery @splatQuery | Select-Object -ExpandProperty name

3. Documentation Formatting
Removed extra blank line in .NOTES section for consistency.

4. Error Message Spacing
Verified correct spacing (was already properly formatted).

Testing Recommendations

• Test with databases containing special characters in names (quotes, semicolons, etc.)
• Test with ExcludeDatabase parameter to verify parameterization works correctly
• Run existing test suite to ensure no regressions

All changes follow the dbatools CLAUDE.md style guide and maintain PowerShell v3 compatibility.

--- |

[potatoqualitee]

Tests pass 👍🏼