sqlite: add limits property to DatabaseSync #61298
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
mertcanaltin
force-pushed
the
mert/sqlite-limits
branch
from
f2bfe7a to
f57afa7
Compare
mertcanaltin
force-pushed
the
mert/sqlite-limits
branch
from
f57afa7 to
ffd7dd0
Compare
louwers
left a comment
louwers
left a comment
There was a problem hiding this comment.
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. |
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
|
@addaleax thanks for reviews, I tried solve |
|
Would it be possible to interpret 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 |
|
+1 Of course! I used Infinity instead of Null and updated the test documents. |
|
Neat, is @Renegade334 on board with that too? |
|
SGTM! The only alternative thought I'd have would be making |
|
Hi, I think I need a review here. @Renegade334 @louwers |
src/node_sqlite.cc
Outdated
| Isolate* isolate = info.GetIsolate(); | ||
| LocalVector<Value> names(isolate); | ||
|
|
||
| for (size_t i = 0; i < kLimitMapping.size(); ++i) { |
There was a problem hiding this comment.
Prefer a range based for loop. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#res-for-range
There was a problem hiding this comment.
I need "i" here, so I'm using the traditional for method.
louwers
left a comment
louwers
left a comment
There was a problem hiding this comment.
Left some comments. But a collaborator will have to review this as well.
thanks for reviews 🙏 |
Co-authored-by: Bart Louwers <[email protected]>
Co-authored-by: René <[email protected]>
| LocalVector<Value> names(isolate); | ||
|
|
||
| for (size_t i = 0; i < kLimitMapping.size(); ++i) { | ||
| for (const auto& mapping : kLimitMapping) { |
There was a problem hiding this comment.
Consider using a structured binding declaration.
|
|
||
| // Apply initial limits | ||
| const auto& limits = open_config_.initial_limits(); | ||
| for (size_t i = 0; i < limits.size(); ++i) { |
There was a problem hiding this comment.
You're right that here a regular for-loop has to be used. Well, C++23 will allow std::views::enumerate. But we're not there yet
Nit: change i to limit_id to avoid a single letter variable, and make it more clear what it represents.
There was a problem hiding this comment.
thanks for detail, I will edit
Co-authored-by: Bart Louwers <[email protected]>
|
|
||
| Local<Object> limits_obj = limits_v.As<Object>(); | ||
|
|
||
| // Iterate through known limit names and extract values |
There was a problem hiding this comment.
Copying suggested approach from other thread, since the comment got marked outdated:
// Iterate through known limit names and extract values
for (const auto& [js_name, sqlite_limit_id] : kLimitMapping) {
Local<String> key;
if (!String::NewFromUtf8(env->isolate(),
js_name.data(),
NewStringType::kNormal,
js_name.size())
.ToLocal(&key)) {
return;
}
Local<Value> val;
if (!limits_obj->Get(env->context(), key).ToLocal(&val)) {
return;
}
if (!val->IsUndefined()) {
if (!val->IsInt32()) {
std::string msg = "The \"options.limits." +
std::string(js_name) +
"\" argument must be an integer.";
THROW_ERR_INVALID_ARG_TYPE(env->isolate(), msg);
return;
}
int limit_val = val.As<Int32>()->Value();
if (limit_val < 0) {
std::string msg = "The \"options.limits." +
std::string(js_name) +
"\" argument must be non-negative.";
THROW_ERR_OUT_OF_RANGE(env->isolate(), msg);
return;
}
open_config.set_initial_limit(sqlite_limit_id, limit_val);
}
}
| bool allow_bare_named_params_ = true; | ||
| bool allow_unknown_named_params_ = false; | ||
| bool defensive_ = false; | ||
| std::array<std::optional<int>, kNumSqliteLimits> initial_limits_; |
There was a problem hiding this comment.
| std::array<std::optional<int>, kNumSqliteLimits> initial_limits_; | |
| std::array<std::optional<int>, kLimitMapping.size()> initial_limits_; |
Still need to be declared in the header it seems.
There was a problem hiding this comment.
Edit: oh I guess it needs to move to the header completely then?
5 participants
Fixes: #61268
add limits property to databaseSync