MicrosoftSQL.BulkInsert - Added column mapping feature #67
Conversation
WalkthroughThe Frends.MicrosoftSQL.BulkInsert project has been updated to version 3.0.0, introducing a new column mapping feature for bulk insert operations. The update allows users to choose between three column mapping strategies: JsonPropertyOrder, JsonPropertyNames, and ManualColumnMapping. Two new parameters, ColumnMapping and ManualColumnMapping, have been added to the Input class to support these mapping options. The changes enhance the flexibility of bulk insert operations by providing more granular control over how data columns are mapped during database insertions. Changes
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/ColumnMapping.cs (1)
3-5: Fix incorrect XML documentation summary.The summary comment incorrectly refers to "SqlTransactionIsolationLevels" instead of describing the column mapping enum.
-/// SqlTransactionIsolationLevels +/// Defines the strategy for mapping columns during bulk insert operations.Frends.MicrosoftSQL.BulkInsert/CHANGELOG.md (1)
3-6: Improve changelog entry wording.The entry has word repetition that can be improved while maintaining clarity.
## [3.0.0] - 2025-01-15 ### Added -Added column mapping feature which allows user to select from JsonPropertyNames, JsonPropertyOrder and ManualColumnMapping options how the column mapping is handled in bulk insert. -Added parameters ColumnMapping and ManualColumnMapping. +- Introduced column mapping feature allowing users to select from JsonPropertyNames, JsonPropertyOrder and ManualColumnMapping options for bulk insert operations. +- Introduced ColumnMapping and ManualColumnMapping parameters.🧰 Tools
🪛 LanguageTool
[duplication] ~4-~4: Possible typo: you repeated a word.
Context: ... Changelog ## [3.0.0] - 2025-01-15 ### Added - Added column mapping feature which allows use...(ENGLISH_WORD_REPEAT_RULE)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/Frends.MicrosoftSQL.BulkInsert.Tests.csproj (1)
20-20: Fix indentation for consistency.The new package reference is correctly added but has inconsistent indentation.
- <PackageReference Include="Microsoft.SqlServer.Types" Version="160.1000.6" /> + <PackageReference Include="Microsoft.SqlServer.Types" Version="160.1000.6" />Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs (1)
447-463: Consider reusing existing test data.The test uses a new JSON string that's very similar to the class-level
_json. Consider reusing the existing test data to maintain consistency and reduce duplication.- var json = @"[ - { - ""Id"": 1, - ""FirstName"": ""Etu"", - ""LastName"": ""Suku"" - }, - { - ""Id"": 2, - ""LastName"": ""Suku"", - ""FirstName"": ""Etu"" - }, - { - ""FirstName"": ""First"", - ""LastName"": ""Last"", - ""Id"": 3 - } - ]"; + // Reuse existing test data + var input = new Input + { + ConnectionString = _connString, + TableName = _tableName, + InputData = _json, // Use class-level test data + ColumnMapping = columnMapping, + ManualColumnMapping = manualMapping + };Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs (1)
3-3: Remove unnecessary using directive.The
using Microsoft.IdentityModel.Abstractions;directive on line 3 is not used in the current code and can be removed to clean up the codebase.Apply this diff to remove the unused directive:
using Frends.MicrosoftSQL.BulkInsert.Definitions; using Microsoft.Data.SqlClient; -using Microsoft.IdentityModel.Abstractions; using Newtonsoft.Json.Linq;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Frends.MicrosoftSQL.BulkInsert/CHANGELOG.md(1 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/Frends.MicrosoftSQL.BulkInsert.Tests.csproj(1 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs(2 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs(4 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/ColumnMapping.cs(1 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Input.cs(1 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.csproj(1 hunks)
🧰 Additional context used
🪛 LanguageTool
Frends.MicrosoftSQL.BulkInsert/CHANGELOG.md
[duplication] ~4-~4: Possible typo: you repeated a word.
Context: ... Changelog ## [3.0.0] - 2025-01-15 ### Added - Added column mapping feature which allows use...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/ColumnMapping.cs (1)
6-22: LGTM! Well-structured enum with clear documentation.The enum provides three distinct mapping strategies with clear, descriptive documentation for each option.
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Input.cs (2)
25-31: LGTM! Well-documented property with appropriate default value.The ColumnMapping property is well-implemented with clear documentation and a sensible default value.
32-44: Consider adding JSON validation for ManualColumnMapping.While the property is well-documented with a clear example, consider adding validation to ensure:
- The provided string is valid JSON
- The JSON structure matches the expected format
- Case sensitivity is properly enforced as mentioned in the ColumnMapping enum documentation
Would you like me to provide an implementation for the JSON validation?
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.csproj (2)
5-5: Version bump to 3.0.0 is appropriate.The major version increment aligns with semantic versioning principles, as the new column mapping feature introduces potential breaking changes in how column mapping is handled.
Line range hint
26-27: Verify package versions for security and compatibility.Let's verify that the package versions are the latest stable releases and check for any known security advisories.
✅ Verification successful
Both package versions are secure and up-to-date
- Newtonsoft.Json 13.0.3 is the latest stable version and includes all security patches
- Microsoft.Data.SqlClient 5.2.1 is the latest stable version and is not affected by recent security advisories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check NuGet packages for latest versions and security advisories # Check latest versions on NuGet curl -s https://api.nuget.org/v3-flatcontainer/microsoft.data.sqlclient/index.json | jq -r '.versions[]' | tail -n 1 curl -s https://api.nuget.org/v3-flatcontainer/newtonsoft.json/index.json | jq -r '.versions[]' | tail -n 1 # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NUGET, package: "Microsoft.Data.SqlClient") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } ' gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NUGET, package: "Newtonsoft.Json") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } 'Length of output: 2770
Script:
#!/bin/bash # Find Microsoft.Data.SqlClient package reference in the project file cat "Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.csproj"Length of output: 1124
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs (3)
50-50: Ensure consistent usage ofinput.TableNameinExecuteHandlercalls.The calls to
ExecuteHandlerhave been updated to passinputinstead oftableName, which is appropriate. Verify thatExecuteHandlercorrectly utilizesinput.TableNameinternally.The usage appears correct, but as a precaution, please confirm that
input.TableNameis properly accessed withinExecuteHandler.Also applies to: 67-67
124-124: Confirm thatinput.TableNameis properly sanitized before assignment.Assigning
sqlBulkCopy.DestinationTableName = input.TableName;directly may pose a risk ifinput.TableNamecontains invalid characters or SQL injection attempts. Ensure thatinput.TableNameis validated and sanitized before assignment.[security_issue]
Consider adding validation to ensure
input.TableNamecomplies with expected SQL table naming conventions.
115-116: 🛠️ Refactor suggestionAdd validation for
ManualColumnMappinginput to handle invalid JSON.Parsing
input.ManualColumnMappingwithout validation may result in exceptions if the input is not valid JSON. To enhance robustness, add error handling to check for null or invalid JSON before parsing.Apply this diff to include validation:
else if (input.ColumnMapping == ColumnMapping.ManualColumnMapping) { + if (string.IsNullOrWhiteSpace(input.ManualColumnMapping)) + throw new ArgumentException("ManualColumnMapping is required when ColumnMapping is set to ManualColumnMapping."); + JObject manualMapping; + try + { + manualMapping = JObject.Parse(input.ManualColumnMapping); + } + catch (JsonReaderException ex) + { + throw new ArgumentException("ManualColumnMapping must be valid JSON.", ex); + } + - foreach (var column in JObject.Parse(input.ManualColumnMapping).Properties()) + foreach (var column in manualMapping.Properties()) sqlBulkCopy.ColumnMappings.Add(new SqlBulkCopyColumnMapping(column.Name, column.Name)); }This ensures that any issues with the
ManualColumnMappinginput are caught early, providing clearer error messages to the user.✅ Verification successful
The review comment's suggestion for adding JSON validation is correct and necessary.
The codebase shows no existing validation for the
ManualColumnMappinginput, making it susceptible to runtime errors when invalid JSON is provided. The suggested validation will improve the robustness of the code by providing clear error messages for both null/empty inputs and invalid JSON format.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the Input class definition ast-grep --pattern 'class Input { $$$ }' # Search for ManualColumnMapping usage rg "ManualColumnMapping" -A 5 -B 5 # Look for similar JSON parsing patterns rg "JObject.Parse" -A 2 -B 2Length of output: 8778
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs
Outdated
Show resolved
Hide resolved
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs
Show resolved
Hide resolved
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs
Show resolved
Hide resolved
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs (3)
463-463: 🛠️ Refactor suggestionAdd validation tests for manual column mapping.
Include test cases for invalid manual mapping scenarios:
- Invalid JSON syntax
- Missing required mappings
- Invalid column names
var manualMapping = @"{ ""Id"":""Id"", ""FirstName"": ""FirstName"", ""LastName"": ""LastName"" }"; + +// Test invalid manual mapping scenarios +var invalidMappings = new[] +{ + @"{ invalid json }", // Invalid JSON syntax + @"{ ""Id"": ""Id"" }", // Missing required mappings + @"{ ""Id"": ""Id"", ""FirstName"": ""NonExistentColumn"" }" // Invalid column name +}; + +foreach (var invalidMapping in invalidMappings) +{ + var input = new Input + { + ConnectionString = _connString, + TableName = _tableName, + InputData = json, + ColumnMapping = ColumnMapping.ManualColumnMapping, + ManualColumnMapping = invalidMapping + }; + + await Assert.ThrowsExceptionAsync<Exception>(() => + MicrosoftSQL.BulkInsert(input, options, default)); +}Likely invalid or redundant comment.
465-479: 🛠️ Refactor suggestionAdd data validation to verify correct column mapping.
The test should verify that the data was mapped to the correct columns.
var result = await MicrosoftSQL.BulkInsert(input, options, default); Assert.IsTrue(result.Success); + + // Verify data was mapped correctly + using var connection = new SqlConnection(_connString); + connection.Open(); + var command = connection.CreateCommand(); + command.CommandText = $"SELECT * FROM {_tableName} ORDER BY Id"; + using var reader = command.ExecuteReader(); + + // Verify first row + Assert.IsTrue(reader.Read(), "No data was inserted"); + Assert.AreEqual(1, reader.GetInt32(reader.GetOrdinal("Id"))); + Assert.AreEqual("Etu", reader.GetString(reader.GetOrdinal("FirstName"))); + Assert.AreEqual("Suku", reader.GetString(reader.GetOrdinal("LastName"))); + + // Verify property order was handled correctly + Assert.IsTrue(reader.Read(), "Second row not found"); + Assert.AreEqual(2, reader.GetInt32(reader.GetOrdinal("Id"))); + Assert.AreEqual("Etu", reader.GetString(reader.GetOrdinal("FirstName"))); + Assert.AreEqual("Suku", reader.GetString(reader.GetOrdinal("LastName")));Likely invalid or redundant comment.
423-443: 🛠️ Refactor suggestionAdd cleanup between test iterations to prevent data interference.
Each iteration should start with a clean state to ensure test isolation.
foreach (var columnMapping in columnMappings) { + // Cleanup and reinitialize before each test + CleanUp(); + Init(); + var input = new Input {Likely invalid or redundant comment.
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs
Show resolved
Hide resolved
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs
Show resolved
Hide resolved
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/ColumnMapping.cs
Outdated
Show resolved
Hide resolved
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Input.cs
Show resolved
Hide resolved
| @@ -1,5 +1,10 @@ | |||
| # Changelog | |||
|
|
|||
| ## [3.0.0] - 2025-01-15 | |||
There was a problem hiding this comment.
Is this actually a breaking change? I had a feeling that it should not be, as the default behavior is going to remain exactly the same. However, if this is a breaking change, then please describe how it is a breaking change.
There was a problem hiding this comment.
The new parameter will break the automatic updates when trying to import the new version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs (2)
3-3: Remove unused import.The import
Microsoft.IdentityModel.Abstractionsappears to be unused in the code.-using Microsoft.IdentityModel.Abstractions;
115-119: Add validation for ManualColumnMapping.The current implementation assumes that column names in ManualColumnMapping match the destination table. Consider adding validation to ensure the mapped columns exist in both source and destination.
else if (input.ColumnMapping == ColumnMapping.ManualColumnMapping) { + if (string.IsNullOrEmpty(input.ManualColumnMapping)) + throw new ArgumentException("ManualColumnMapping is required when ColumnMapping is set to ManualColumnMapping"); + + var mappings = JObject.Parse(input.ManualColumnMapping); + var sourceColumns = dataSet.Tables[0].Columns.Cast<DataColumn>().Select(c => c.ColumnName).ToList(); + foreach (var column in JObject.Parse(input.ManualColumnMapping).Properties()) + { + if (!sourceColumns.Contains(column.Name)) + throw new ArgumentException($"Source column '{column.Name}' specified in ManualColumnMapping does not exist in the input data"); sqlBulkCopy.ColumnMappings.Add(new SqlBulkCopyColumnMapping(column.Name, column.Name)); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Frends.MicrosoftSQL.BulkInsert/CHANGELOG.md(1 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs(4 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/ColumnMapping.cs(1 hunks)Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Input.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/ColumnMapping.cs
- Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Input.cs
- Frends.MicrosoftSQL.BulkInsert/CHANGELOG.md
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: jefim
PR: FrendsPlatform/Frends.MicrosoftSQL#67
File: Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs:108-117
Timestamp: 2025-01-15T18:17:36.090Z
Learning: In Frends.MicrosoftSQL.BulkInsert, when ColumnMapping is set to JsonPropertyOrder, no explicit column mappings are needed as this is the default behavior of SqlBulkCopy, which maps columns based on their order in the input JSON.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs (2)
50-50: LGTM! Clean refactor to support column mapping.The changes correctly pass the entire input object to ExecuteHandler, enabling the new column mapping functionality while maintaining the existing error handling and transaction management.
Also applies to: 67-67
108-119: LGTM! Clear implementation of column mapping options.The implementation correctly handles all three mapping options with a helpful comment explaining the default JsonPropertyOrder behavior.
#66
[3.0.0] - 2025-01-15
Added
Summary by CodeRabbit
Release Notes for Frends.MicrosoftSQL.BulkInsert v3.0.0
New Features
Breaking Changes
Improvements