Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlite: reset statement immediately in run() #57350

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 6, 2025

This commit updates StatementSync.prototype.run() to reset the prepared statement immediately after calling sqlite3_step() to return the correct change metadata.

Fixes: #57344

This matches the better-sqlite3 implementation.

This commit updates StatementSync.prototype.run() to reset the
prepared statement immediately after calling sqlite3_step() to
return the correct change metadata.

Fixes: nodejs#57344
@nodejs-github-bot
Copy link
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 Mar 6, 2025
@geeksilva97 geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 6, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 6, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Mar 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.18%. Comparing base (395439b) to head (f69726f).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57350      +/-   ##
==========================================
- Coverage   90.19%   90.18%   -0.02%     
==========================================
  Files         630      630              
  Lines      185187   185185       -2     
  Branches    36243    36245       +2     
==========================================
- Hits       167030   167002      -28     
+ Misses      11131    11130       -1     
- Partials     7026     7053      +27     
Files with missing lines Coverage Δ
src/node_sqlite.cc 78.52% <100.00%> (+0.03%) ⬆️

... and 19 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.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 6, 2025

@cjihrig cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Mar 6, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/57350
✔  Done loading data for nodejs/node/pull/57350
----------------------------------- PR info ------------------------------------
Title      sqlite: reset statement immediately in run() (#57350)
Author     Colin Ihrig <[email protected]> (@cjihrig)
Branch     cjihrig:bug -> nodejs:main
Labels     c++, author ready, sqlite
Commits    1
 - sqlite: reset statement immediately in run()
Committers 1
 - cjihrig <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/57350
Fixes: https://github.com/nodejs/node/issues/57344
Reviewed-By: Edy Silva <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57350
Fixes: https://github.com/nodejs/node/issues/57344
Reviewed-By: Edy Silva <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 06 Mar 2025 15:17:50 GMT
   ✔  Approvals: 1
   ✔  - Edy Silva (@geeksilva97): https://github.com/nodejs/node/pull/57350#pullrequestreview-2664814350
   ✘  This PR needs to wait 119 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-03-06T19:40:56Z: https://ci.nodejs.org/job/node-test-pull-request/65618/
- Querying data for job/node-test-pull-request/65618/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/13738933344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node:sqlite returning statement affects changes field in run()
5 participants