sqlite: improve error for excess bound parameters - #65164
Conversation
|
Review requested:
|
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65164 +/- ##
==========================================
+ Coverage 90.30% 90.31% +0.01%
==========================================
Files 759 759
Lines 248328 248336 +8
Branches 46853 46858 +5
==========================================
+ Hits 224243 224283 +40
+ Misses 15514 15465 -49
- Partials 8571 8588 +17
π New features to boost your workflow:
|
Signed-off-by: Lazizbek Ergashev <lazerg2@gmail.com>
2fc910b to
f4ad7ab
Compare
pacocartones
left a comment
There was a problem hiding this comment.
Independent verification β logic checks out; one doc/test gap to consider
I verified the fix independently, both against the C++ and against live SQLite semantics (Node 24, node:sqlite). The core change is correct and the error message counts are accurate.
Root cause confirmed: the old loop kept binding anonymous values past the statement's parameter count; sqlite3_bind_* then failed with SQLITE_RANGE (errcode 25), which surfaced as the opaque ERR_SQLITE_ERROR: column index out of range. The explicit capacity check before binding fixes exactly that.
Loop verification (PR head):
anon_idx++afterBindValueis retained, so each arg binds to a distinct slot.- The
while (anon_idx <= param_count)skip over named parameters is correct for every interleaving I checked (named-only, anon-only,$a, ?,?, $a,$a, ?, $b, ?). - The counts are right: at failure,
i - anon_startequals the number of anonymous values bound, which β since the throw only fires once every slot up toparam_countis taken β is the statement's actual anonymous capacity;args.Length() - anon_startis the number of anonymous values received (the named-params object excluded). I hand-checked all four new tests against the code and the expected messages match.
?NNN semantics (relevant to the new doc sentence): SQLite numbers ?NNN by explicit index and silently allows binding to intermediate "phantom" indices, so SELECT ?2 genuinely accepts up to two values (get('x', 'y') β { a: 'y' }) and SELECT ?2, ? numbers the bare ? at index 3 (bind count 3). The new loop handles these correctly β the doc claim "The ?NNN form raises the number accepted to NNN" holds.
One gap (non-blocking): that doc sentence is the only place the ?NNN form is covered, and there is no test for it. The four added tests exercise ?1,?2, no params, mixed named+anonymous, and named-only. Suggest adding a ?NNN case (e.g. db.prepare('SELECT ?2 AS a').run('x', 'y') succeeds; .run('x', 'y', 'z') throws the new ERR_INVALID_STATE with accepts 2, received 3) β it locks in the documented behavior, including the surprising phantom-index binding, and guards the new loop against regressions.
LGTM otherwise.
Fixes: #65163
BindParams()walked the anonymous arguments without checking how many parameters the statement actually has, so an extra argument failed insidesqlite3_bind_*and came back asERR_SQLITE_ERROR: column index out of range. To a JavaScript caller "column" reads as a table column, which points at the schema rather than at the extra argument.Binding now stops when it runs out of parameter slots and throws
ERR_INVALID_STATEwith both counts, the same way an unknown named parameter is already reported from that function.Callers matching on
ERR_SQLITE_ERRORor errcode 25 for this case will see the new error instead.