Skip to content

sqlite: add limits property to DatabaseSync#61298

Merged
nodejs-github-bot merged 20 commits intonodejs:mainfrom
mertcanaltin:mert/sqlite-limits
Feb 27, 2026
Merged

sqlite: add limits property to DatabaseSync#61298
nodejs-github-bot merged 20 commits intonodejs:mainfrom
mertcanaltin:mert/sqlite-limits

Conversation

@mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Jan 6, 2026

Fixes: #61268

add limits property to databaseSync

@nodejs-github-bot
Copy link
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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 6, 2026
@mertcanaltin mertcanaltin force-pushed the mert/sqlite-limits branch 2 times, most recently from f57afa7 to ffd7dd0 Compare January 6, 2026 18:55
@mertcanaltin mertcanaltin requested a review from louwers January 6, 2026 20:46
Copy link
Contributor

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Should we add SQLITE_MAX_LENGTH etc. as constants? https://sqlite.org/limits.html

@mertcanaltin
Copy link
Member Author

Should we add SQLITE_MAX_LENGTH etc. as constants? https://sqlite.org/limits.html

SQLITE_MAX are compile-time preprocessor macros, so they can't be exported directly at runtime. however, the default values returned by db.limits are exactly these SQLITE_MAX values (assuming SQLite was compiled with default settings).

so db.limits.length returns 1000000000 which equals SQLITE_MAX_LENGTH.

if you want explicit constants, i could add SQLITE_LIMIT (the enum IDs used with sqlite3_limit()) to the constants object.
let me know what you think would be most useful.

@mertcanaltin
Copy link
Member Author

@addaleax thanks for reviews, I tried solve

@Renegade334
Copy link
Member

I think my only main thought is regarding the intended behaviour when requested limits are out of range. sqlite3_limit() will silently truncate values larger than the compile-time limit; at the moment, we just pass such values through to the API and imply success, even though the result is not what the user intended.

Presumably we can add the relevant compile-time value to each LimitInfo and then use this to validate input? (I don't see a problem with using SQLITE_MAX_* – these are the values that represent the underlying behaviour, even if compiled with a shared library or with custom defines.)

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jan 6, 2026

I think my only main thought is regarding the intended behaviour when requested limits are out of range. sqlite3_limit() will silently truncate values larger than the compile-time limit; at the moment, we just pass such values through to the API and imply success, even though the result is not what the user intended.

Presumably we can add the relevant compile-time value to each LimitInfo and then use this to validate input? (I don't see a problem with using SQLITE_MAX_* – these are the values that represent the underlying behaviour, even if compiled with a shared library or with custom defines.)

thanks for comment, I tried use SQLITE_MAX_*
346d53c
7164b3d
fd2a144

@HACKER007-SHIP-IT HACKER007-SHIP-IT mentioned this pull request Jan 7, 2026
1 task
@louwers
Copy link
Contributor

louwers commented Jan 7, 2026

I think my only main thought is regarding the intended behaviour when requested limits are out of range. sqlite3_limit() will silently truncate values larger than the compile-time limit; at the moment, we just pass such values through to the API and imply success, even though the result is not what the user intended.

I think this is actually a lot more ergonomic. Then if you want to reset a limit you can just do:

db.limits.length = Infinity;

Otherwise you'd need to look up the maximum limit. If you make a mistake, or the maximum limit changes, you get an exception...

These limits are for limiting a database. If the user tries to set a limit that is too high, it may not be what "the result the user intended", but the intended result was impossible anyway...

@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 78.57143% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (abddfc9) to head (e47480b).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 78.75% 15 Missing and 19 partials ⚠️
src/node_sqlite.h 75.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #61298    +/-   ##
========================================
  Coverage   89.72%   89.73%            
========================================
  Files         676      676            
  Lines      205988   206237   +249     
  Branches    39490    39549    +59     
========================================
+ Hits       184830   185064   +234     
- Misses      13295    13320    +25     
+ Partials     7863     7853    -10     
Files with missing lines Coverage Δ
src/node_sqlite.h 80.64% <75.00%> (-0.84%) ⬇️
src/node_sqlite.cc 80.76% <78.75%> (-0.14%) ⬇️

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

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jan 7, 2026

I think my only main thought is regarding the intended behaviour when requested limits are out of range. sqlite3_limit() will silently truncate values larger than the compile-time limit; at the moment, we just pass such values through to the API and imply success, even though the result is not what the user intended.

I think this is actually a lot more ergonomic. Then if you want to reset a limit you can just do:

db.limits.length = Infinity;

Otherwise you'd need to look up the maximum limit. If you make a mistake, or the maximum limit changes, you get an exception...

These limits are for limiting a database. If the user tries to set a limit that is too high, it may not be "the result the user intended", but the intended result was impossible anyway...

thanks for the suggestion ❤️ Yes, this is the current behavior values are passed directly to sqlite3_limit() which handles truncation silently. So db.limits.length = Infinity works as a convenient way to reset to the maximum.

@Renegade334
Copy link
Member

I think this is actually a lot more ergonomic. Then if you want to reset a limit you can just do:

db.limits.length = Infinity;

Otherwise you'd need to look up the maximum limit.

While I'm not fundamentally opposed to this pattern, note that Infinity is not an Int32, and therefore fails validation here 😉

Could we use null as the "reset" value, like we do with custom functions, authorizers etc? That way, we can just do a quick IsNull() and pass INT_MAX to SQLite in this case?

@mertcanaltin
Copy link
Member Author

@Renegade334 Thank you, I tried to apply what you said.

@louwers
Copy link
Contributor

louwers commented Jan 9, 2026

Would it be possible to interpret Infinity as a special value?

Having a setter that accepts different types from the corresponding getter feels wrong to me.

Also, I would except that 'resetting' a limit means it goes back to the value that was initially provided to the constructor. Not that it takes on the highest value (which is the same as the default value). Setting it to Infinity leaves little room for alternate interpretations.

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jan 11, 2026

+1 Of course! I used Infinity instead of Null and updated the test documents.

Copy link
Member

@araujogui araujogui left a comment

Choose a reason for hiding this comment

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

LGTM

@Renegade334 Renegade334 added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 27, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2026
@nodejs-github-bot
Copy link
Collaborator

@Renegade334 Renegade334 added semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem. labels Feb 27, 2026
@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 27, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2026
@nodejs-github-bot nodejs-github-bot merged commit 35d3bc8 into nodejs:main Feb 27, 2026
82 of 83 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 35d3bc8

aduh95 pushed a commit that referenced this pull request Feb 28, 2026
PR-URL: #61298
Fixes: #61268
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sqlite: allow setting limits on inputs

7 participants