Repository navigation
Invalid sqlite behavior about unused named parameter #55533
Description
Version
all
Platform
all
Subsystem
sqlite
What steps will reproduce the bug?
IMO, there is an issue with SQLite's behavior, as highlighted by the current test (https://github.com/nodejs/node/blob/main/test/parallel/test-sqlite-named-parameters.js#L17-L32):
test('throws on unknown named parameters', (t) => {
const db = new DatabaseSync(nextDb());
t.after(() => { db.close(); });
const setup = db.exec(
'CREATE TABLE types(key INTEGER PRIMARY KEY, val INTEGER) STRICT;'
);
t.assert.strictEqual(setup, undefined);
t.assert.throws(() => {
const stmt = db.prepare('INSERT INTO types (key, val) VALUES ($k, $v)');
stmt.run({ $k: 1, $unknown: 1 });
}, {
code: 'ERR_INVALID_STATE',
message: /Unknown named parameter '\$unknown'/,
});
});
How often does it reproduce? Is there a required condition?
NA
What is the expected behavior? Why is that the expected behavior?
IMO, as mentioned earlier here, it should be permissible to set more parameters (such as the $unknown parameter of this test) than those actually used in the SQL string.
This approach is utilized by better-sqlite3 and is particularly useful when conditionally constructing your WHERE clause with parameters that may or may not be used in the final query.
The code and the associated error test should be modified to throw an error regarding the $v named parameter instead, similar to the behavior of better-sqlite3.
What do you see instead?
NA
Additional information
No response
Activity
badkeyy commentedon Oct 28, 2024
I agree on this, however I also see the potential for this causing correctness issues if developers do not match their queries and parameters correctly (without the
runthrowing an error).Though if there are no immediate objections I will create a draft for this.
tniessen commentedon Oct 28, 2024
I suspect that this is an intentional design decision to make the API more robust. Similarly, should missing keys implicitly be treated as
NULLvalues?badkeyy commentedon Oct 28, 2024
IMO this would lead to a lot of hard to detect bugs and confusion.
@cjihrig What was your design intention here?
webda2l commentedon Oct 28, 2024
I couldn't find any libraries that throw an error for unused named parameters.. https://chatgpt.com/share/671f9240-9380-8006-af8b-6b74e8639d18
badkeyy commentedon Oct 28, 2024
Not sure if ChatGPT is the best source for this, but after briefly looking into some related docs of different sqIite modules I also wasn't able to find such behavior
cjihrig commentedon Oct 28, 2024
I don't have a strong opinion here. It's an experimental feature and anyone is welcome to open a PR to see how it is received.
webda2l commentedon Oct 28, 2024
FYI about competitors :),
Bun implementation strict mode, is similar to better-sqlite3. It allows unused named parameters (strict or not strict mode), and trigger an error in strict mode if there is in the SQL an unknow param. https://bun.sh/docs/api/sqlite#binding-values
Deno implementation (WASM, https://docs.deno.com/runtime/tutorials/connecting_to_databases/#connect-to-sqlite-with-the-wasm-optimized-module, & FFI, https://docs.deno.com/runtime/tutorials/connecting_to_databases/#connect-to-sqlite-with-the-ffi-module) disallow both unused named parameters :/ but allow both using in the SQL an unknow param (threated as null).
https://codesandbox.io/p/devbox/black-worker-4t37md
https://codesandbox.io/p/devbox/h87vx8
sqlite: add support for unknown named parameters
sqlite: add support for unknown named parameters
sqlite: add support for unknown named parameters
sqlite: add support for unknown named parameters
sqlite: add support for unknown named parameters
sqlite: add support for unknown named parameters