Skip to content

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

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
araujogui:sqlite-backup-type-check
Oct 25, 2026
Merged

sqlite: validate backup() source is a Database#66797
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
araujogui:sqlite-backup-type-check

Conversation

@araujogui

@araujogui araujogui commented Oct 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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

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

nodejs-github-bot commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

Copilot AI reviewed Oct 16, 2026

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
@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 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.59%. Comparing base (62b4717) to head (49c986c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #66797   +/-   ##
=======================================
  Coverage   90.58%   90.59%           
=======================================
  Files         797      797           
  Lines      278388   278399   +11     
  Branches    53538    53524   -14     
=======================================
+ Hits       252183   252202   +19     
- Misses      16573    16576    +3     
+ Partials     9632     9621   -11
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.18% <100.00%> (+0.12%) ⬆️

... and 28 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 force-pushed the sqlite-backup-type-check branch from e488536 to 9402d56 Compare October 18, 2026 15:36
trivikr
trivikr requested changes Oct 25, 2026
View reviewed changes

@trivikr trivikr 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.

Please rebase to change DatabaseSync -> Database in the first commit too.

Comment thread test/parallel/test-sqlite-backup.mjs Outdated
@araujogui
araujogui force-pushed the sqlite-backup-type-check branch from 9402d56 to 33ddc3e Compare October 25, 2026 16:19
@araujogui araujogui changed the title sqlite: validate backup() source is a DatabaseSync sqlite: validate backup() source is a Database Oct 25, 2026
trivikr
trivikr requested changes Oct 25, 2026
View reviewed changes

@trivikr trivikr left a comment •
edited
Loading

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.

The first commit message still says "sqlite: validate backup() source is a DatabaseSync"

It can be updated if you plan to land it manually. If we're using commit-queue, it need to be changed to "sqlite: validate backup() source is a Database"

@araujogui
sqlite: validate backup() source is a Database
2eee740 
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 Database constructor template
instead.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
@araujogui
sqlite: reuse Database template across environments
49c986c 
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 33ddc3e to 49c986c Compare October 25, 2026 17:17
@trivikr trivikr added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 25, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@trivikr trivikr added commit-queue-squash PRs the Commit Queue should land as one squashed commit. resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. labels Oct 25, 2026
@github-actions github-actions Bot removed the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Oct 25, 2026
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

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

@trivikr trivikr added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 25, 2026
@nodejs-github-bot
nodejs-github-bot merged commit e20da72 into nodejs:main Oct 25, 2026
78 checks passed
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Landed in e20da72

@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@cjihrig cjihrig cjihrig approved these changes
@trivikr trivikr trivikr approved these changes
@geeksilva97 geeksilva97 Awaiting requested review from geeksilva97

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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.

5 participants

@araujogui @nodejs-github-bot @cjihrig @trivikr