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

[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]

Neat, is @Renegade334 on board with that too?

[Renegade334]

SGTM!

The only alternative thought I'd have would be making -1 the magic number. It's perhaps less semantically on-the-button than Infinity, but the handling becomes a lot more straightforward.

[mertcanaltin]

Hi, I think I need a review here. @Renegade334 @louwers

[louwers]

Prefer a range based for loop. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#res-for-range

[mertcanaltin]

I need "i" here, so I'm using the traditional for method.

[louwers]

Why is it needed?

[louwers]

      // 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);
        }
      }

[louwers]

Left some comments. But a collaborator will have to review this as well.

[mertcanaltin]

Left some comments. But a collaborator will have to review this as well.

thanks for reviews 🙏

[louwers]

Consider using a structured binding declaration.

[louwers]

This is not what I meant. If another limit gets added the limit number needs to be manually updated, which is error-prone. Instead you can use

Suggested change

	inline const std::array<std::optional<int>, kNumSqliteLimits>&
	inline const std::array<std::optional<int>, kLimitMapping.size()>&

which works becauze the .size() method is constexpr.

[louwers]

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.