FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: improve error reporting for prepared statements in SQLTagStore by islandryu · Pull Request #60227 · nodejs/node · GitHub

/ node Public

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

Open
islandryu wants to merge 3 commits into
nodejs:mainfrom
islandryu:60198
ConversationCommits (3)ChecksFiles changed
Open

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

Conversation

islandryu commented Oct 12, 2025 •
edited
Loading

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

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
louwers reviewed Oct 12, 2025
View reviewed changes
Comment thread test/parallel/test-sqlite-template-tag.js Outdated

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

louwers Oct 12, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Consider configuring eslint with https://eslint.org/docs/latest/rules/no-unused-expressions#allowtaggedtemplates

islandryu Oct 12, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have updated the ESLint configuration.

louwers reviewed Oct 12, 2025
View reviewed changes
Comment thread src/node_sqlite.cc Outdated
louwers reviewed Oct 12, 2025
View reviewed changes
Comment thread test/parallel/test-sqlite-template-tag.js Outdated
message: 'Failed to prepare statement',
code: 'ERR_SQLITE_ERROR',
errcode: 1,
errstr: 'no such table: does_not_exist',

louwers Oct 12, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Wrong way around I think, see other comment.

codecov Bot commented Oct 12, 2025 •
edited
Loading

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 commented Aug 12, 2026

Copy link
Copy Markdown
Member

@islandryu Can you please rebase from main?

sqlite: improve error reporting for prepared statements in SQLTagStore
880d426 
Fixes: nodejs#60198
Signed-off-by: islandryu <shimaryuhei@gmail.com>
swap errstr and errmsg
4f2fe69 
Signed-off-by: islandryu <shimaryuhei@gmail.com>

trivikr commented Aug 19, 2026

Copy link
Copy Markdown
Member

The lint errors need to be fixed.

trivikr commented Aug 23, 2026

Copy link
Copy Markdown
Member

Why is this reverting was was done in #61096?

islandryu commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

If you're referring to src/node_sqlite.cc:4018, that's not a revert but a difference in approach.
The issue has already been resolved in #61096.
#61096 makes the message itself more detailed, whereas this PR puts that into errmessage instead.
Unless there's a specific reason otherwise, I think this PR and issue #60198 can be closed.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
louwers louwers left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

Back | FazBrowse Home | New Git URL