Fspes 140

[MatteoDelOmbra]

Review Checklist

• Task version updated (x.x.0)
• CHANGELOG.md updated
• Solution builds
• Warnings resolved (if possible)
• Typos resolved
• Tests cover new code
• Description how to run tests locally added to README.md (if needed)
• All tests pass locally

---

Summary by CodeRabbit

• Changed
  • Result.Count now reports total rows on success, and on failure it reports an estimated rows-copied value based on the configured notification behavior.
• Bug Fixes
  • Improved failure reporting by carrying forward the actual “rows copied” information when available, and improved null/empty-string handling for typed string fields.
• Documentation
  • Updated XML docs for Result.Count and NotifyAfter to match the new behavior.
• Tests
  • Updated unit test expectations for NotifyAfter scenarios.
• Chores
  • Bumped package version to 3.3.0.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 99f289e9-49b9-46bf-b0af-8f214bc46156

📥 Commits

Reviewing files that changed from the base of the PR and between dbba961 and 244b05e.

📒 Files selected for processing (2)

• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs
• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Options.cs

💤 Files with no reviewable changes (1)

• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Options.cs

---

Walkthrough

BulkInsert now reports the actual total row count in Result.Count on success (using totalRows instead of the event-driven counter) and an estimated rows-copied value on failure (by attaching Data["RowsCopied"] to thrown exceptions and reading it in all catch paths). NotifyAfter calculation was refactored to a switch expression. SetEmptyDataRowsToNull now detects only typed string cells instead of relying on ToString() comparison. XML docs, tests, changelog, and version were updated to match.

Changes

Result.Count Reporting Overhaul

Layer / File(s)	Summary
Result.Count and NotifyAfter contract docs Frends.MicrosoftSQL.BulkInsert/Definitions/Result.cs, Frends.MicrosoftSQL.BulkInsert/Definitions/Options.cs	XML doc for Result.Count updated to describe the approximation-on-failure behavior via Options.NotifyAfter; NotifyAfter doc revised to reflect new above-row-count semantics; minor whitespace cleanup.
ExecuteHandler: totalRows, NotifyAfter switch, and RowsCopied on failure Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs	BulkInsert introduces rowsCopied variable in early setup. ExecuteHandler computes totalRows upfront, switches to a switch expression for NotifyAfter, returns totalRows on success, and on WriteToServerAsync failure attaches Data["RowsCopied"] plus a computed row-range message to the thrown exception.
BulkInsert catch paths threading RowsCopied into Result Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs	Non-transactional catch reads RowsCopied from ex.Data and returns Result(false, rowsCopied) instead of 0; transactional success path preserved; transactional rollback and outer catch similarly extract RowsCopied from exception Data when ThrowErrorOnFailure is false.
SetEmptyDataRowsToNull string cell detection rewrite Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs	Logic updated to check for cells typed as string with Length == 0 and convert only those to DBNull.Value, replacing the prior ToString() == string.Empty approach that also assigned null based on item lookup.
Tests, changelog, and version bump Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs, CHANGELOG.md, Frends.MicrosoftSQL.BulkInsert.csproj	NotifyAfter test assertions updated from result.Count == 0 to result.Count == 3; CHANGELOG.md documents the 3.3.0 behavior change; package version bumped to 3.3.0; test and project file closing braces adjusted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• FrendsPlatform/Frends.MicrosoftSQL#56: Modifies ExecuteHandler's NotifyAfter and row-notification logic and updates Options.NotifyAfter documentation — directly related to the same code paths changed here.
• FrendsPlatform/Frends.MicrosoftSQL#72: Modifies ExecuteHandler and transactional execution paths in BulkInsert.cs, including async/await adjustments that overlap with this PR's error-handling and row-copying refactors.

Suggested reviewers

• ttossavainen
• RikuVirtanen

Poem

🐇 Hop hop, the rows are tallied right,
No more returning zero on a flight!
When errors strike mid-copy spree,
RowsCopied tells you where we'd be.
The changelog blooms, version shines at three —
A bunny loves when counts run free! 🌸

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Fspes 140' is vague and does not convey meaningful information about the changeset, which involves significant updates to row-count reporting behavior, exception handling, and version bumping to 3.3.0.	Replace the title with a descriptive summary of the main change, such as 'Update Result.Count to report actual row count on success and estimated count on failure' or 'Version 3.3.0: Improve row-count tracking in bulk insert operations'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fspes-140

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs (1)

333-336: 🏗️ Heavy lift

Add failure-path assertions for the new Result.Count contract.

These updates validate success results, but the PR’s key change also includes failure-time estimated counts (RowsCopied). Please add tests where ThrowErrorOnFailure = false and WriteToServerAsync fails after partial progress, then assert result.Count reflects the expected approximation.

As per coding guidelines, "Confirm unit tests exist and provide at least 80% coverage."

Also applies to: 372-375

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs`
around lines 333 - 336, The current test only validates the success path for the
result.Count property, but the PR introduces new behavior where result.Count
should reflect estimated row counts even on failure when ThrowErrorOnFailure is
false. Add additional test cases that configure options with ThrowErrorOnFailure
set to false, simulate a failure scenario in WriteToServerAsync after partial
data has been copied, and then assert that result.Count accurately reflects the
expected approximate number of rows that were successfully copied before the
failure occurred.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs`:
- Around line 189-196: The SetEmptyDataRowsToNull method uses
Array.IndexOf(row.ItemArray, column) inside value iteration, which always
returns the index of the first matching empty value in the row, causing
duplicate empty strings to update only the first occurrence. Replace the
value-based foreach loop with index-based iteration using a for loop that
iterates through the column indices directly, so you can access and update each
column by its actual position without relying on Array.IndexOf which may resolve
to the wrong column when values repeat.

In
`@Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Options.cs`:
- Around line 22-23: The XML documentation comment for the NotifyAfter property
contains a duplicated and grammatically incorrect second sentence. Remove the
second documentation line that starts with "Notified value is useful for error
handling" as it repeats the same concept as the first line and contains a typo
("occured" instead of "occurred"). Keep only the first sentence which clearly
explains the purpose of NotifyAfter for error handling.

---

Nitpick comments:
In
`@Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs`:
- Around line 333-336: The current test only validates the success path for the
result.Count property, but the PR introduces new behavior where result.Count
should reflect estimated row counts even on failure when ThrowErrorOnFailure is
false. Add additional test cases that configure options with ThrowErrorOnFailure
set to false, simulate a failure scenario in WriteToServerAsync after partial
data has been copied, and then assert that result.Count accurately reflects the
expected approximate number of rows that were successfully copied before the
failure occurred.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3ff50afc-8775-44d7-b39f-276f5be8e906

📥 Commits

Reviewing files that changed from the base of the PR and between ffb3380 and dbba961.

📒 Files selected for processing (6)

• Frends.MicrosoftSQL.BulkInsert/CHANGELOG.md
• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.Tests/UnitTests.cs
• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs
• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Options.cs
• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Definitions/Result.cs
• Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert.csproj