Skip to content

sqlite: validate backup() source is a DatabaseSync - #66797

Open
araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:sqlite-backup-type-check
Open

araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:sqlite-backup-type-check

Conversation

@araujogui

Copy link
Copy Markdown
Member

backup() cast any BaseObject to Database*, so passing a StatementSync or Session caused a type confusion. It now checks HasInstance() against the DatabaseSync template.

Copilot AI balanced review requested due to automatic review settings October 16, 2026 18:30
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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 Oct 16, 2026
backup() only checked that the source argument was a BaseObject before
casting it to Database, so passing a Statement or Session caused a
type confusion. Check against the DatabaseSync constructor template
instead.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
@araujogui
araujogui force-pushed the sqlite-backup-type-check branch from 3b9b0f3 to 66f69bb Compare October 16, 2026 18:33
@codecov

codecov Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.50%. Comparing base (782a3ac) to head (9402d56).
⚠️ Report is 61 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66797      +/-   ##
==========================================
+ Coverage   90.47%   90.50%   +0.02%     
==========================================
  Files         791      797       +6     
  Lines      277225   277804     +579     
  Branches    53313    53364      +51     
==========================================
+ Hits       250829   251414     +585     
+ Misses      16801    16792       -9     
- Partials     9595     9598       +3     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.95% <100.00%> (-0.37%) ⬇️

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

Cache the template so a shared IsolateData can load the binding twice.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
@araujogui
araujogui force-pushed the sqlite-backup-type-check branch from e488536 to 9402d56 Compare October 18, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

4 participants