Skip to content

fix: prevent RowsAffected double-counting when triggers fire#363

Open
dlevy-msft-sql wants to merge 4 commits into
microsoft:mainfrom
dlevy-msft-sql:fix/rows-affected-trigger-double-count
Open

fix: prevent RowsAffected double-counting when triggers fire#363
dlevy-msft-sql wants to merge 4 commits into
microsoft:mainfrom
dlevy-msft-sql:fix/rows-affected-trigger-double-count

Conversation

@dlevy-msft-sql
Copy link
Copy Markdown

Problem

When a DML statement (UPDATE/INSERT/DELETE) fires an AFTER trigger that does not use SET NOCOUNT ON, SQL Server sends two TDS tokens with row counts:

  1. DONEINPROC (0xFD) - the trigger's DML row count
  2. DONE (0xFF) - the outer statement's actual row count

The driver's iterateResponse() was accumulating (+=) both counts into rowCount, causing RowsAffected() to return the sum (e.g., 2 instead of 1 for a single-row update that fires a trigger inserting 1 audit row).

This matches the behavior reported in #204 where RowsAffected() returns incorrect values.

Root Cause

In token.go, iterateResponse() line 1200:

case doneStruct:
    if token.Status&doneCount != 0 {
        t.rowCount += int64(token.RowCount)  // BUG: accumulates trigger counts
    }

Fix

Change doneStruct handling from accumulation (+=) to assignment (=):

case doneStruct:
    if token.Status&doneCount != 0 {
        t.rowCount = int64(token.RowCount)  // Use authoritative outer count
    }

The final DONE/DONEPROC token carries the authoritative row count for the outermost statement. DONEINPROC tokens from sp_executesql still work correctly because the subsequent DONEPROC has status=0 (no doneCount flag set), so it doesn't overwrite.

Verified Scenarios

Scenario Before After
Basic UPDATE (no trigger) 1 ✅ 1 ✅
UPDATE with trigger (no NOCOUNT) 2 ❌ 1 ✅
UPDATE with trigger (SET NOCOUNT ON) 1 ✅ 1 ✅
UPDATE all rows with trigger 3 ✅ 3 ✅
Parameterized INSERT via sp_executesql 1 ✅ 1 ✅

Tests

  • Added TestRowsAffectedWithTrigger integration test covering all 4 scenarios
  • Existing TestAffectedRows continues to pass
  • All unit tests pass (./msdsn, ./internal/..., ./integratedauth, ./azuread)

Fixes #204

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.60%. Comparing base (65e137f) to head (c240049).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #363       +/-   ##
===========================================
+ Coverage   80.62%   96.60%   +15.98%     
===========================================
  Files          35       92       +57     
  Lines        6910    74360    +67450     
===========================================
+ Hits         5571    71837    +66266     
- Misses       1068     2188     +1120     
- Partials      271      335       +64     
Flag Coverage Δ
unittests 96.53% <100.00%> (?)

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

Files with missing lines Coverage Δ
token.go 70.09% <100.00%> (+0.56%) ⬆️

... and 58 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dlevy-msft-sql dlevy-msft-sql added this to the v1.11.0 milestone Apr 17, 2026
@dlevy-msft-sql dlevy-msft-sql force-pushed the fix/rows-affected-trigger-double-count branch 4 times, most recently from e841b35 to 8ae789e Compare April 24, 2026 23:03
@dlevy-msft-sql dlevy-msft-sql requested a review from Copilot April 26, 2026 16:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect RowsAffected() values when DML statements fire AFTER triggers that emit their own rowcount tokens, by ensuring the final DONE token’s count is treated as authoritative.

Changes:

  • Update tokenProcessor.iterateResponse() to assign (not accumulate) DONE/DONEPROC row counts.
  • Add unit tests validating rowcount assignment behavior in trigger-like and multi-statement token sequences.
  • Add integration tests covering RowsAffected() with triggers (NOCOUNT on/off) and multi-statement batch behavior.

Reviewed changes

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

File Description
token.go Changes DONE rowcount handling to avoid double-counting when triggers emit additional rowcount tokens.
token_test.go Adds unit tests asserting the new rowcount assignment semantics.
rowsaffected_test.go Adds integration coverage for trigger scenarios and multi-statement batch RowsAffected() behavior.

Comment thread rowsaffected_test.go
Comment thread token.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread rowsaffected_test.go
Comment thread rowsaffected_test.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread token.go Outdated
Change doneInProcStruct row count handling from += to = in
iterateResponse, mirroring the doneStruct fix. For the RPC/sp_executesql
path, both trigger and outer statement counts arrive as DONEINPROC
tokens; accumulation would inflate RowsAffected when triggers fire
without SET NOCOUNT ON.

Add TestRowCountDoneInProcOnlyRPCPath covering the DONEINPROC-only
sequence where DONEPROC lacks the doneCount flag.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread rowsaffected_test.go
Move multi-row update test to before the NOCOUNT trigger switch so it
exercises the original double-counting bug path where both trigger and
outer statement DONEINPROC counts would accumulate.
@dlevy-msft-sql dlevy-msft-sql requested a review from Copilot April 27, 2026 03:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect RowsAffected() result after updated

3 participants