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 63.88889% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (9c641c5) to head (954b996).
⚠️ Report is 78 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 63.88% 7 Missing and 6 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #60227    +/-   ##
========================================
  Coverage   90.11%   90.12%            
========================================
  Files         752      751     -1     
  Lines      252200   252597   +397     
  Branches    47433    47527    +94     
========================================
+ Hits       227267   227648   +381     
- Misses      16239    16240     +1     
- Partials     8694     8709    +15     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.92% <63.88%> (+0.11%) ⬆️

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

trivikr commented Aug 19, 2026

Copy link
Copy Markdown
Member

The lint errors need to be fixed.

@trivikr

trivikr commented Aug 23, 2026

Copy link
Copy Markdown
Member

Why is this reverting was was done in #61096?

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