sqlite: add limits property to DatabaseSync

[mertcanaltin]

Fixes: #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 78.44311% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.72%. Comparing base (e7d6728) to head (be5ce48).
⚠️ Report is 4 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	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61298      +/-   ##
==========================================
- Coverage   89.74%   89.72%   -0.02%     
==========================================
  Files         674      674              
  Lines      204395   204562     +167     
  Branches    39274    39307      +33     
==========================================
+ Hits       183427   183541     +114     
- Misses      13275    13303      +28     
- Partials     7693     7718      +25     

Files with missing lines	Coverage Δ
src/node_sqlite.h	79.31% <71.42%> (-1.09%)	⬇️
src/node_sqlite.cc	80.67% <78.75%> (-0.13%)	⬇️

... and 22 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?

[mertcanaltin]

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

[louwers]

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]

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

[louwers]

Can you reinstate the range for loops again?

[mertcanaltin]

Can you reinstate the range for loops again?

I sended

[louwers]

Woops. Looks like we forgot about SQLITE_LIMIT_WORKER_THREADS: https://www.sqlite.org/c3ref/c_limit_attached.html

Should it be added even though it can be set with a pragma?

Probably not, there are several other limits. https://www.sqlite.org/pragma.html

[mertcanaltin]

I solved conflict 🎉