sqlite,test,doc: accept Buffer and URL as paths #56991

geeksilva97 · 2025-02-10T14:56:15Z

This PR adds the capability to the sqlite module to accept Buffer and URL as the database location.

codecov · 2025-02-20T02:47:42Z

Codecov Report

Attention: Patch coverage is 88.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.25%. Comparing base (c864dea) to head (7d1111f).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	88.33%	2 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56991      +/-   ##
==========================================
- Coverage   90.33%   90.25%   -0.08%     
==========================================
  Files         630      630              
  Lines      184537   184681     +144     
  Branches    36077    36138      +61     
==========================================
- Hits       166693   166686       -7     
- Misses      10950    11021      +71     
- Partials     6894     6974      +80

Files with missing lines	Coverage Δ
src/node_sqlite.cc	`78.79% <88.33%> (+0.21%)`	⬆️

... and 29 files with indirect coverage changes

src/node_sqlite.cc

doc/api/sqlite.md

test/parallel/test-sqlite-backup.mjs

doc/api/sqlite.md

src/node_sqlite.cc

test/parallel/test-sqlite-backup.mjs

src/node_sqlite.cc

cjihrig · 2025-02-25T14:45:06Z

I think this needs a rebase to fix the CI issue.

cc: @nodejs/sqlite since this PR was opened before the team existed.

src/node_sqlite.cc

louwers · 2025-02-25T15:37:05Z

test/parallel/test-sqlite-database-sync.js

@@ -256,6 +274,15 @@ suite('DatabaseSync.prototype.exec()', () => {
    });
  });

+  test('throws if the URL does not have the file: scheme', (t) => {


You should also check the happy path.

Can you try URL("file://path/to/db.db?mode=ro") and verify that writes error out?

Passing a file:// or file: as URL as a string or Buffer should also be supported.

doc/api/sqlite.md

src/node_sqlite.cc

nodejs-github-bot · 2025-02-26T14:40:54Z

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

test/parallel/test-sqlite-backup.mjs:53

[nitpick] Consider clarifying the error message terminology: while a Buffer is used in tests, the message refers to Uint8Array. Explicitly mentioning Buffer (or clarifying that Buffer is acceptable since it is a Uint8Array) could reduce potential confusion.

message: 'The "destination" argument must be a string, Uint8Array, or URL without null bytes.'

test/parallel/test-sqlite-database-sync.js:31

[nitpick] For consistency with tests using Buffer objects, consider updating the error message to explicitly mention Buffer, or note that Buffer instances are valid as they are a subtype of Uint8Array.

message: /The "path" argument must be a string, Uint8Array, or URL without null bytes/

cjihrig · 2025-02-26T17:42:27Z

You can ignore the review by Copilot. I was curious to see if it would generate anything helpful.

There are actual failures on Windows in the CI though:

duration_ms: 385.002
exitcode: 1
severity: fail
stack: |-
  (node:6684) ExperimentalWarning: SQLite is an experimental feature and might change at any time
  (Use `node --trace-warnings ...` to show where the warning was created)
  Can't clean tmpdir: c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.736
  Files blocking: [ 'database-2.db', 'database-3.db' ]

  c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:80
      throw e;
      ^

  Error: EPERM, Permission denied: \\?\c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.736 '\\?\c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.736'
      at Object.rmSync (node:fs:1246:18)
      at rmSync (c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:31:8)
      at onexit (c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:65:5)
      at process.<anonymous> (c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:54:14)
      at process.emit (node:events:519:35) {
    errno: 1,
    code: 'EPERM',
    path: '\\\\?\\c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.736',
    syscall: 'rm'
  }

  Node.js v24.0.0-pre

geeksilva97 · 2025-02-26T18:40:42Z

You can ignore the review by Copilot. I was curious to see if it would generate anything helpful.

There are actual failures on Windows in the CI though:

duration_ms: 385.002
exitcode: 1
severity: fail
stack: |-
  (node:6684) ExperimentalWarning: SQLite is an experimental feature and might change at any time
  (Use `node --trace-warnings ...` to show where the warning was created)
  Can't clean tmpdir: c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.736
  Files blocking: [ 'database-2.db', 'database-3.db' ]

  c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:80
      throw e;
      ^

  Error: EPERM, Permission denied: \\?\c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.736 '\\?\c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.736'
      at Object.rmSync (node:fs:1246:18)
      at rmSync (c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:31:8)
      at onexit (c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:65:5)
      at process.<anonymous> (c:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:54:14)
      at process.emit (node:events:519:35) {
    errno: 1,
    code: 'EPERM',
    path: '\\\\?\\c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.736',
    syscall: 'rm'
  }

  Node.js v24.0.0-pre

Thanks, Colin. I had forgotten to close the database connection 😅

geeksilva97 · 2025-02-26T18:42:54Z

src/node_sqlite.cc

+        if (file_url->type != ada::scheme::FILE) {
+          THROW_ERR_INVALID_URL_SCHEME(env->isolate());
+          return std::nullopt;
+        }


I removed the FileURLToPath because it was stripping the query params out. We should keep them since SQLite supports them.

Guaranteeing that a valid URL is given we can return the URL href it is.

test/parallel/test-sqlite.js

nodejs-github-bot · 2025-02-27T13:47:45Z

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

nodejs-github-bot · 2025-02-27T17:43:15Z

Landed in c7cf677

test/parallel/test-sqlite.js

doc/api/sqlite.md

PR-URL: #56991 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 Feb 10, 2025

geeksilva97 force-pushed the sqlite-accept-buffer branch from 446d48b to 18fe76b Compare February 10, 2025 17:20

geeksilva97 force-pushed the sqlite-accept-buffer branch 3 times, most recently from 2a608e1 to 5b5a8cf Compare February 20, 2025 01:37

geeksilva97 marked this pull request as ready for review February 20, 2025 01:38

geeksilva97 force-pushed the sqlite-accept-buffer branch 4 times, most recently from 06d30bf to 4561203 Compare February 21, 2025 12:49

geeksilva97 commented Feb 21, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

geeksilva97 changed the title ~~[WIP] sqlite,test,doc: accept Buffer and URL as paths~~ sqlite,test,doc: accept Buffer and URL as paths Feb 21, 2025

geeksilva97 commented Feb 21, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

geeksilva97 commented Feb 21, 2025

View reviewed changes

doc/api/sqlite.md Outdated Show resolved Hide resolved

cjihrig reviewed Feb 21, 2025

View reviewed changes

geeksilva97 force-pushed the sqlite-accept-buffer branch from 303de33 to 7019435 Compare February 21, 2025 18:43

geeksilva97 commented Feb 21, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

cjihrig reviewed Feb 22, 2025

View reviewed changes

geeksilva97 commented Feb 24, 2025

View reviewed changes

src/node_sqlite.cc Show resolved Hide resolved

geeksilva97 force-pushed the sqlite-accept-buffer branch from 07a5b31 to 114df4d Compare February 24, 2025 19:43

louwers reviewed Feb 25, 2025

View reviewed changes

src/node_sqlite.cc Show resolved Hide resolved

louwers reviewed Feb 25, 2025

View reviewed changes

doc/api/sqlite.md Outdated Show resolved Hide resolved

louwers reviewed Feb 25, 2025

View reviewed changes

src/node_sqlite.cc Show resolved Hide resolved

geeksilva97 added 2 commits February 26, 2025 05:53

sqlite,test,doc: allow Buffer and URL as database location

53a0251

test: remove unneeded comments

2533cf1

cjihrig requested a review from Copilot February 26, 2025 17:39

Copilot AI reviewed Feb 26, 2025

View reviewed changes

geeksilva97 added 2 commits February 26, 2025 15:30

sqlite: ensure query params are used

79c1ca1

sqlite,doc: make path argument consistent in all interfaces

8508790

geeksilva97 force-pushed the sqlite-accept-buffer branch from 6d48eee to 8508790 Compare February 26, 2025 18:39

geeksilva97 commented Feb 26, 2025

View reviewed changes

test/parallel/test-sqlite.js Outdated Show resolved Hide resolved

geeksilva97 force-pushed the sqlite-accept-buffer branch 3 times, most recently from 22e4331 to e145acb Compare February 27, 2025 01:07

sqlite,test: test URI passed as string or Buffer

7d1111f

geeksilva97 force-pushed the sqlite-accept-buffer branch from e145acb to 7d1111f Compare February 27, 2025 12:42

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025

cjihrig approved these changes Feb 27, 2025

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2025

nodejs-github-bot merged commit c7cf677 into nodejs:main Feb 27, 2025
66 checks passed

louwers reviewed Feb 27, 2025

View reviewed changes

test/parallel/test-sqlite.js Show resolved Hide resolved

louwers reviewed Feb 27, 2025

View reviewed changes

doc/api/sqlite.md Show resolved Hide resolved

aduh95 pushed a commit that referenced this pull request Mar 9, 2025

sqlite,test,doc: allow Buffer and URL as database location

e80bbb7

PR-URL: #56991 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

github-actions bot mentioned this pull request Mar 12, 2025

2025-03-13, Version 23.10.0 (Current) #57424

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqlite,test,doc: accept Buffer and URL as paths #56991

sqlite,test,doc: accept Buffer and URL as paths #56991

geeksilva97 commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 20, 2025 •

edited

Loading

cjihrig commented Feb 25, 2025

louwers Feb 25, 2025 •

edited

Loading

geeksilva97 Feb 26, 2025

nodejs-github-bot commented Feb 26, 2025

cjihrig commented Feb 26, 2025

geeksilva97 commented Feb 26, 2025 •

edited

Loading

geeksilva97 Feb 26, 2025 •

edited

Loading

nodejs-github-bot commented Feb 27, 2025

nodejs-github-bot commented Feb 27, 2025

sqlite,test,doc: accept Buffer and URL as paths #56991

sqlite,test,doc: accept Buffer and URL as paths #56991

Conversation

geeksilva97 commented Feb 10, 2025 • edited Loading

codecov bot commented Feb 20, 2025 • edited Loading

Codecov Report

cjihrig commented Feb 25, 2025

louwers Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

geeksilva97 Feb 26, 2025

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 26, 2025

Choose a reason for hiding this comment

cjihrig commented Feb 26, 2025

geeksilva97 commented Feb 26, 2025 • edited Loading

geeksilva97 Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 27, 2025

nodejs-github-bot commented Feb 27, 2025

geeksilva97 commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 20, 2025 •

edited

Loading

louwers Feb 25, 2025 •

edited

Loading

geeksilva97 commented Feb 26, 2025 •

edited

Loading

geeksilva97 Feb 26, 2025 •

edited

Loading