Skip to content

sqlite: validate StatementSync.run() integers - #65178

Open
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:sqlite-statement-sync-lastinsertrowid-precision
Open

sqlite: validate StatementSync.run() integers#65178
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:sqlite-statement-sync-lastinsertrowid-precision

Conversation

@trivikr

@trivikr trivikr commented Aug 10, 2026

Copy link
Copy Markdown
Member

Fixes: #65177

Use the standard SQLite integer conversion for changes and lastInsertRowid. Throw ERR_OUT_OF_RANGE when a value cannot be represented safely as a Number, or return it as a BigInt when BigInt reads are enabled.

Assisted-by: codex:gpt-5.6-sol

@nodejs-github-bot

nodejs-github-bot commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Aug 10, 2026
@trivikr
trivikr force-pushed the sqlite-statement-sync-lastinsertrowid-precision branch from fb954ab to 01f2928 Compare August 10, 2026 04:25
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2026
@codecov

codecov Bot commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (c7b962c) to head (75ccff6).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #65178      +/-   ##
==========================================
+ Coverage   90.31%   90.32%   +0.01%     
==========================================
  Files         751      751              
  Lines      250474   250482       +8     
  Branches    47335    47346      +11     
==========================================
+ Hits       226218   226251      +33     
+ Misses      15601    15600       -1     
+ Partials     8655     8631      -24
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.13% <92.85%> (+0.01%) ⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

araujogui
araujogui reviewed Aug 10, 2026
View reviewed changes
Comment thread src/node_sqlite.cc Outdated
araujogui
araujogui approved these changes Aug 11, 2026
View reviewed changes

@araujogui araujogui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@trivikr
trivikr requested a review from Qard August 12, 2026 01:06
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2026
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/75802/

@trivikr trivikr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 15, 2026
trivikr added 2 commits August 15, 2026 12:58
@trivikr
sqlite: validate StatementSync.run() integers
990ea83 
Use the standard SQLite integer conversion for changes and
lastInsertRowid. Throw ERR_OUT_OF_RANGE when a value cannot be
represented safely as a Number, or return it as a BigInt when
BigInt reads are enabled.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: codex:gpt-5.6-sol
@trivikr
fixup! sqlite: validate StatementSync.run() integers
75ccff6
@trivikr
trivikr force-pushed the sqlite-statement-sync-lastinsertrowid-precision branch from e07659a to 75ccff6 Compare August 15, 2026 20:07
@trivikr trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 15, 2026
@nodejs-github-bot

nodejs-github-bot commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/75869/

@trivikr
trivikr requested a review from Renegade334 August 15, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qard Qard Qard approved these changes
@Renegade334 Renegade334 Awaiting requested review from Renegade334
+1 more reviewer
@araujogui araujogui araujogui approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sqlite: StatementSync.run() loses precision for lastInsertRowid instead of throwing ERR_OUT_OF_RANGE

4 participants

@trivikr @nodejs-github-bot @Qard @araujogui