Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sqlite custom function callback errors reporting #56773

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Charlesnorris509
Copy link

@Charlesnorris509 Charlesnorris509 commented Jan 26, 2025

Fixes #56772

Add error reporting for custom function callback errors to the sqlite API.

  • src/node_sqlite.cc

    • Call sqlite3_result_error() in UserDefinedFunction::xFunc when the JS callback throws an error.
    • Update the retval check to handle the error condition and call sqlite3_result_error().
  • test/parallel/test-sqlite-custom-functions.js

    • Add a new test to verify that custom function callback errors are reported to the sqlite API.
    • Write tests to ensure that error conditions in the JS callback result in sqlite3_result_error() being called.

For more details, open the Copilot Workspace session.

Fixes nodejs#56772

Add error reporting for custom function callback errors to the sqlite API.

* **src/node_sqlite.cc**
  - Call `sqlite3_result_error()` in `UserDefinedFunction::xFunc` when the JS callback throws an error.
  - Update the `retval` check to handle the error condition and call `sqlite3_result_error()`.

* **test/parallel/test-sqlite-custom-functions.js**
  - Add a new test to verify that custom function callback errors are reported to the sqlite API.
  - Write tests to ensure that error conditions in the JS callback result in `sqlite3_result_error()` being called.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/nodejs/node/issues/56772?shareId=XXXX-XXXX-XXXX-XXXX).
@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 Jan 26, 2025
@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2025

Thanks for opening this PR. I think the correct fix requires more work though:

  • There are three early returns that need to be addressed. This PR currently only addresses one of them.
  • The bigger change is that we need to create state indicating that there is both a pending JavaScript exception and a SQLite error.
  • Then, before creating the SQLite error object, we need to check if there is a JavaScript exception pending. If there is, we should not create the SQLite exception. I believe in its current state, this PR will surface the SQLite error and swallow the JavaScript exception.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2025

For reference, I think the fix should look more like this - cjihrig@f113c39. Note, I already had this patch mostly written when I saw your PR.

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: custom function callback errors should be reported to sqlite API
3 participants