Extract SqlBulkCopy column names using dynamic SQL

[paulmedynski]

Description

#3590 allowed SqlBulkCopy to find and work with hidden columns (even if those columns weren't accessible server-side.) It contained a check on the all_columns DMV to make sure that we only checked the graph_type column when querying if that column existed; this was to maintain SQL 2016 compatibility.

#3714 highlighted that both queries are compiled and fail at the point of compilation anyway, so this wasn't accomplishing anything. To make this work, we need to use dynamic SQL to run the column queries. This PR does so.

Supersedes #3719. Original PR by @edwardneal. This PR was created as a direct branch of the SqlClient repo so I can run the new SQL 16 and 17 CI pipeline stages manually to prove this fixes the issue.

Issues

Fixes #3714.

Testing

Manual testing of all test cases against a SQL 2016 instance (via the updated CI pipelines).

[Copilot]

Pull request overview

This PR updates SqlBulkCopy’s target-column discovery query to use dynamic SQL so that referencing sys.all_columns.graph_type doesn’t cause compilation failures on SQL Server 2016, addressing regression #3714.

Changes:

• Reworks SqlBulkCopy.CreateInitialQuery() to build and execute the sys.all_columns column-name query via sp_executesql.
• Adjusts manual test SQL statistics expectations to account for the additional statements executed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopyAllFromReader.cs	Updates expected SelectCount / SelectRows due to extra statements from dynamic SQL.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	Uses dynamic SQL for column-name extraction to avoid SQL 2016 compilation errors when graph_type is absent.

[codecov]

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.51%. Comparing base (60d4b92) to head (8074cd5).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
...Client/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	63.63%	4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (60d4b92) and HEAD (8074cd5). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (60d4b92)	HEAD (8074cd5)
CI-SqlClient	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4092      +/-   ##
==========================================
- Coverage   73.22%   66.51%   -6.72%     
==========================================
  Files         280      274       -6     
  Lines       43000    65785   +22785     
==========================================
+ Hits        31486    43754   +12268     
- Misses      11514    22031   +10517     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	66.51% <63.63%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

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

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[priyankatiwari08]

Suggestion: It might be worth adding a brief inline comment here explaining why CatalogName is interpolated directly into the dynamic SQL string rather than parameterized. SQL doesn't allow parameterizing identifiers (table/schema/catalog names), so this is the correct approach — but without context, future reviewers may flag it as a potential SQL injection vector.

[paulmedynski]

Done in 70269ce — added an inline comment block explaining that CatalogName and escapedObjectName are interpolated directly because SQL Server doesn't allow parameterizing identifiers, and noting both values are pre-escaped via SqlServerEscapeHelper.

[priyankatiwari08]

Looks good to me, Just added a suggestion to add comment related to explaining why CatalogName is interpolated directly into the dynamic SQL string rather than parameterized

[paulmedynski]

Addressed both open review comments in 70269ce:

1. SQLServerVersion lazy-init fix (Copilot): Changed s_sQLServerVersion field initializer from string.Empty to null (and renamed to s_sqlServerVersion) so the ??= null-coalescing assignment actually triggers. IsAtLeastSQL2017() / IsAtLeastSQL2019() will now correctly query the server version on first access.

2. CatalogName interpolation comment (@priyankatiwari08): Added an inline comment block explaining that CatalogName and escapedObjectName are interpolated directly because SQL Server doesn't allow parameterizing identifiers, and both values are pre-escaped via SqlServerEscapeHelper.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTestWithTdsServer.cs:97

• ForceEncryptionRegistryPath no longer handles SQL Server 2016 (major 13). On SQL 2016 it will return string.Empty, and Dispose() later passes that value to RemoveForceEncryptionFromRegistryPath, which may end up opening/setting values on an unintended registry key (or throwing). Add an IsSQL2016() branch that returns the MSSQL13 registry path, and/or defensively avoid calling registry cleanup when the path is empty.

                if (DataTestUtility.IsSQL2022())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL16.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2019())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL15.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2017())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL14.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                return string.Empty;
            }

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTest.cs:59

• ForceEncryptionRegistryPath no longer has an SQL Server 2016 (major 13) mapping; it falls back to string.Empty. This means the test will skip certificate/registry cleanup in Dispose() on SQL 2016 and can leave machine-level state behind. Add an IsSQL2016() branch returning the MSSQL13 registry path so cleanup works on SQL 2016 as well.

                if (DataTestUtility.IsSQL2022())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL16.{InstanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2019())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL15.{InstanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                if (DataTestUtility.IsSQL2017())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL14.{InstanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                return string.Empty;
            }

[Copilot]

IsSQL2022/IsSQL2019/IsSQL2017/IsSQL2016 call SQLServerVersion.Trim() without a null guard. After changing the cached version field to be nullable, SQLServerVersion can now be null (e.g., when TCPConnectionString is empty), which will throw NullReferenceException. Make these helpers null-safe (e.g., use SQLServerVersion?.Trim() or parse like the new IsAtLeastSQL* helpers).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTestWithTdsServer.cs:96

• ForceEncryptionRegistryPath no longer returns a path for SQL Server 2016 (major 13). On SQL 2016 this will return string.Empty, but Dispose() later calls RemoveForceEncryptionFromRegistryPath(ForceEncryptionRegistryPath) without checking for empty, which can lead to attempting to open/set values on the HKLM root key (or throwing). Add an IsSQL2016() branch mapping to MSSQL13.* or guard against empty before using the path.

                if (DataTestUtility.IsSQL2017())
                {
                    return $@"SOFTWARE\Microsoft\Microsoft SQL Server\MSSQL14.{s_instanceName}\MSSQLSERVER\SuperSocketNetLib";
                }
                return string.Empty;

[Copilot]

The comment says "@Column_Names will remain non-null" when OBJECT_ID returns NULL, but @Column_Names is declared as NULL and will remain NULL until later COALESCE assigns ''. Update the comment to reflect the actual behavior (i.e., it remains NULL and is then set to '').

Suggested change

	// OBJECT_ID will return NULL and @Column_Names will remain non-null. The subsequent
	// SELECT * query will then continue to fail with "Invalid object name" rather than with
	// an unusual error because the query being executed is NULL.
	// OBJECT_ID will return NULL and @Column_Names will remain NULL. Later, a COALESCE
	// expression sets @Column_Names to '*', so the subsequent SELECT * query will still
	// fail with "Invalid object name" rather than with an unusual error because the query
	// being executed is NULL.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).