sqlite: add limits property to DatabaseSync

[mertcanaltin]

for: #61268

add limits property to databaseSync

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[louwers]

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

[mertcanaltin]

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]

@addaleax thanks for reviews, I tried solve

[Renegade334]

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]

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

[louwers]

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]

Codecov Report

❌ Patch coverage is 79.26829% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.50%. Comparing base (6218d14) to head (5a16bdc).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	79.74%	14 Missing and 18 partials ⚠️
src/node_sqlite.h	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61298      +/-   ##
==========================================
+ Coverage   88.01%   88.50%   +0.49%     
==========================================
  Files         704      704              
  Lines      208759   208903     +144     
  Branches    40194    40296     +102     
==========================================
+ Hits       183741   184896    +1155     
+ Misses      16958    15995     -963     
+ Partials     8060     8012      -48     

Files with missing lines	Coverage Δ
src/node_sqlite.h	78.94% <66.66%> (-1.45%)	⬇️
src/node_sqlite.cc	80.14% <79.74%> (-0.03%)	⬇️

... and 109 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]

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]

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?