Skip to content

sqlite: improve error reporting for prepared statements in SQLTagStore - #60227

Open
islandryu wants to merge 3 commits into
nodejs:mainfrom
islandryu:60198
Open

sqlite: improve error reporting for prepared statements in SQLTagStore#60227
islandryu wants to merge 3 commits into
nodejs:mainfrom
islandryu:60198

Conversation

@islandryu

@islandryu islandryu commented Oct 12, 2025

Copy link
Copy Markdown
Member

Fixes: #60198

I added report content for SQLTagStore.
Although the text in errstr differs from that of a conventional prepared statement, it now matches the corresponding SQLite function name.

@nodejs-github-bot

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 Oct 12, 2025

test('failed prepares throw', () => {
assert.throws(() => {
sql.all`SELECT * FROM does_not_exist`; // eslint-disable-line no-unused-expressions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have updated the ESLint configuration.

Comment thread src/node_sqlite.cc Outdated
message: 'Failed to prepare statement',
code: 'ERR_SQLITE_ERROR',
errcode: 1,
errstr: 'no such table: does_not_exist',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wrong way around I think, see other comment.

@codecov

codecov Bot commented Oct 12, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.97297% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (9c641c5) to head (e17312d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 72.97% 3 Missing and 7 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #60227   +/-   ##
=======================================
  Coverage   90.11%   90.11%           
=======================================
  Files         752      752           
  Lines      252200   252236   +36     
  Branches    47433    47447   +14     
=======================================
+ Hits       227267   227309   +42     
+ Misses      16239    16236    -3     
+ Partials     8694     8691    -3     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.72% <72.97%> (-0.09%) ⬇️

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

@trivikr

trivikr commented Aug 12, 2026

Copy link
Copy Markdown
Member

@islandryu Can you please rebase from main?

Fixes: nodejs#60198
Signed-off-by: islandryu <shimaryuhei@gmail.com>
Signed-off-by: islandryu <shimaryuhei@gmail.com>
Signed-off-by: islandryu <shimaryuhei@gmail.com>
@trivikr

trivikr commented Aug 19, 2026

Copy link
Copy Markdown
Member

The lint errors need to be fixed.

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.

sqlite: less-than-useful error reporting when using SQLTagStore queries

4 participants