Extract SqlBulkCopy column names using dynamic SQL #3719
Conversation
This enables the query to compile correctly when the graph_type column is missing
|
/azp run |
There was a problem hiding this comment.
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_executesqlto 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 |
| else if (!string.IsNullOrEmpty(CatalogName)) | ||
| { | ||
| CatalogName = SqlServerEscapeHelper.EscapeIdentifier(CatalogName); | ||
| CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(SqlServerEscapeHelper.EscapeIdentifier(CatalogName)); |
There was a problem hiding this comment.
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.
| CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(SqlServerEscapeHelper.EscapeIdentifier(CatalogName)); | |
| CatalogName = SqlServerEscapeHelper.EscapeStringAsLiteral(CatalogName); |
There was a problem hiding this comment.
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.
| 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;'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Show resolved
Hide resolved
| 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;'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 successfully started running 2 pipeline(s). |
2 participants
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_columnsDMV to make sure that we only checked thegraph_typecolumn 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.