sqlite: report zero changes for read-only statements#62128
sqlite: report zero changes for read-only statements#62128zerone0x wants to merge 1 commit intonodejs:mainfrom
Conversation
Read-only statements (e.g. SELECT) should always report 0 for both changes and lastInsertRowid. Previously, running a SELECT after an INSERT would leak the change count from the INSERT because sqlite3_changes64() returns the count from the most recent write statement on the connection. Use sqlite3_stmt_readonly() to detect read-only statements and return 0 instead of querying the connection-level counters. Fixes: nodejs#59764 Co-Authored-By: Claude <noreply@anthropic.com>
|
Review requested:
|
There was a problem hiding this comment.
I won't block in case I am a lone voice here, but I am -0.99 on this.
The kicker here is that "statements that impact sqlite3_changes" and "statements for which sqlite3_stmt_readonly is false" are not synonymous. There are a massive group of statements that are not INSERT/UPDATE/DELETE statements that the API will not mark as read-only, so this will result in a scenario where some statements that don't impact sqlite3_changes will return a zero rowcount, and others will return the rowcount of the last relevant query, which I would argue is worse.
I stand by my original comment at #59764 (comment): it shouldn't be a mystery to the user as to what sort of query they're executing, and that will directly determine whether the rowcount has any meaning. So long as this behaviour is documented, I don't see any great advantage to trying to hack around it.
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62128 +/- ##
==========================================
- Coverage 89.66% 89.65% -0.01%
==========================================
Files 676 676
Lines 206348 206351 +3
Branches 39527 39523 -4
==========================================
- Hits 185019 185012 -7
- Misses 13464 13468 +4
- Partials 7865 7871 +6
π New features to boost your workflow:
|
Read-only statements (e.g. SELECT) should always report 0 for both
changesandlastInsertRowid. Previously, running a SELECT afteran INSERT would leak the change count from the INSERT because
sqlite3_changes64()returns the count from the most recent writestatement on the connection.
Use
sqlite3_stmt_readonly()to detect read-only statements andreturn 0 instead of querying the connection-level counters.
Fixes: #59764