Fix 3714 | Extract SqlBulkCopy column names using dynamic SQL by edwardneal · Pull Request #3719 · dotnet/SqlClient

edwardneal · 2025-10-24T21:42:15Z

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.

It'll conflict with #3677, I'd appreciate it if we could review this first.

Issues

Fixes #3714.

Testing

Manual testing of all test cases against a SQL 2016 instance.

This enables the query to compile correctly when the graph_type column is missing

apoorvdeshmukh · 2025-10-27T18:34:50Z

/azp run

Copilot

Pull Request Overview

This PR modifies SqlBulkCopy to use dynamic SQL for column name extraction, enabling compatibility with SQL Server 2016. The change addresses compilation failures that occurred when querying the graph_type column on older SQL Server versions that don't support this column in the sys.all_columns DMV.

Key Changes:

Converted static SQL queries to dynamic SQL using sp_executesql to defer compilation
Changed catalog name escaping from identifier-only to literal string escaping
Updated test expectations to reflect additional SELECT operations from dynamic SQL execution

Reviewed Changes

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

File	Description
SqlBulkCopy.cs	Implements dynamic SQL execution for column queries and updates catalog name escaping to support parameterized queries
CopyAllFromReader.cs	Updates test assertions to account for increased SELECT count and rows from dynamic SQL execution

Copilot · 2025-10-27T18:35:06Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

            else if (!string.IsNullOrEmpty(CatalogName))
            {
-                CatalogName = SqlServerEscapeHelper.EscapeIdentifier(CatalogName);
+                CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(SqlServerEscapeHelper.EscapeIdentifier(CatalogName));


Double-escaping the catalog name (first as identifier, then as literal) may not provide the intended protection. If CatalogName comes from user input, consider validating it against a whitelist of allowed database names instead of relying solely on escaping. The identifier escaping is unnecessary when the value will be embedded as a string literal in dynamic SQL.

Suggested change

CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(SqlServerEscapeHelper.EscapeIdentifier(CatalogName));

CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(CatalogName);

Escaping the identifier twice is necessary because CatalogName is an object name which will be part of a dynamic SQL query. The inner escape handles [] in the property, the outer escape handles prevents ' generating an error. In both cases, we ensure that malicious input cannot be used to inject SQL statements into the metadata query. Removing one of these escapes would be unsafe.

Copilot · 2025-10-27T18:35:06Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

 IF EXISTS (SELECT TOP 1 * FROM sys.all_columns WHERE [object_id] = OBJECT_ID('sys.all_columns') AND [name] = 'graph_type')
 BEGIN
-    SELECT @Column_Names = COALESCE(@Column_Names + ', ', '') + QUOTENAME([name]) FROM {CatalogName}.[sys].[all_columns] WHERE [object_id] = OBJECT_ID('{escapedObjectName}') AND COALESCE([graph_type], 0) NOT IN (1, 3, 4, 6, 7) ORDER BY [column_id] ASC;
+    SET @Column_Name_Query = N'SELECT @Column_Names = COALESCE(@Column_Names + '', '', '''') + QUOTENAME([name]) FROM {CatalogName}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) NOT IN (1, 3, 4, 6, 7) ORDER BY [column_id] ASC;';


The column query logic is duplicated between the two branches. Consider extracting the common query structure and conditionally adding the graph_type filter to reduce duplication and make future maintenance easier.

I'm happy to do this if requested - I just didn't want to raise any red flags by using T-SQL to build a SQL statement dynamically.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

Copilot · 2025-10-27T18:35:07Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

 IF EXISTS (SELECT TOP 1 * FROM sys.all_columns WHERE [object_id] = OBJECT_ID('sys.all_columns') AND [name] = 'graph_type')
 BEGIN
-    SELECT @Column_Names = COALESCE(@Column_Names + ', ', '') + QUOTENAME([name]) FROM {CatalogName}.[sys].[all_columns] WHERE [object_id] = OBJECT_ID('{escapedObjectName}') AND COALESCE([graph_type], 0) NOT IN (1, 3, 4, 6, 7) ORDER BY [column_id] ASC;
+    SET @Column_Name_Query = N'SELECT @Column_Names = COALESCE(@Column_Names + '', '', '''') + QUOTENAME([name]) FROM {CatalogName}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) NOT IN (1, 3, 4, 6, 7) ORDER BY [column_id] ASC;';


The magic numbers (1, 3, 4, 6, 7) in the graph_type filter lack explanation. Add a comment documenting what these values represent (e.g., specific graph column types being excluded) to improve code maintainability.

These are documented here but I agree that it's not intuitive to find; I'll add a comment when responding to the wider review.

azure-pipelines · 2025-10-27T18:35:12Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski · 2025-12-09T17:52:18Z

@edwardneal - If you can merge main, I will kick off a new CI run.

edwardneal · 2025-12-09T18:55:32Z

Thanks @paulmedynski, I've just merged.

paulmedynski · 2025-12-09T18:56:16Z

/azp run

azure-pipelines · 2025-12-09T18:56:40Z

Azure Pipelines successfully started running 2 pipeline(s).

github-actions · 2026-01-09T02:38:56Z

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

edwardneal · 2026-01-09T05:19:39Z

This PR is not stale.

github-actions · 2026-02-09T03:00:19Z

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

edwardneal · 2026-02-09T17:47:39Z

This PR is not stale.

cheenamalhotra · 2026-02-10T16:30:53Z

/azp run

azure-pipelines · 2026-02-10T16:31:26Z

Azure Pipelines successfully started running 2 pipeline(s).

Build column name query using dynamic SQL

a7d6101

This enables the query to compile correctly when the graph_type column is missing

edwardneal requested a review from a team as a code owner October 24, 2025 21:42

apoorvdeshmukh requested a review from Copilot October 27, 2025 18:33

Copilot AI reviewed Oct 27, 2025

View reviewed changes

edwardneal changed the title ~~Extract SqlBulkCopy column names using dynamic SQL~~ Fix 3714 | Extract SqlBulkCopy column names using dynamic SQL Nov 16, 2025

Merge branch 'main' into fix/3714-sqlbulkcopy-sql2016

01d4e23

edwardneal mentioned this pull request Dec 22, 2025

Feature | Support SQL Graph column aliases in SqlBulkCopy #3677

Open

edwardneal mentioned this pull request Jan 7, 2026

SQL Bulk Copy Fails on SQL Server 2016 / 130 #3714

Open

github-actions bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Jan 9, 2026

github-actions bot removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Jan 10, 2026

edwardneal mentioned this pull request Jan 27, 2026

Feature | Prepare for OpenTelemetry spans #3914

Open

github-actions bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 9, 2026

github-actions bot removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 10, 2026

cheenamalhotra added this to the 7.0.0-preview4 milestone Feb 10, 2026

samsharma2700 self-assigned this Feb 10, 2026

	CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(SqlServerEscapeHelper.EscapeIdentifier(CatalogName));
	CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(CatalogName);

Conversation

edwardneal commented Oct 24, 2025

Description

Issues

Testing

Uh oh!

apoorvdeshmukh commented Oct 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

edwardneal Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

edwardneal Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

edwardneal Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

azure-pipelines bot commented Oct 27, 2025

Uh oh!

paulmedynski commented Dec 9, 2025

Uh oh!

edwardneal commented Dec 9, 2025

Uh oh!

paulmedynski commented Dec 9, 2025

Uh oh!

azure-pipelines bot commented Dec 9, 2025

Uh oh!

github-actions bot commented Jan 9, 2026

Uh oh!

edwardneal commented Jan 9, 2026

Uh oh!

github-actions bot commented Feb 9, 2026

Uh oh!

edwardneal commented Feb 9, 2026

Uh oh!

cheenamalhotra commented Feb 10, 2026

Uh oh!

azure-pipelines bot commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

edwardneal Oct 27, 2025 •

edited

Loading