Skip to content

Invalid sqlite behavior about unused named parameter #55533

Description

@webda2l

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

  1. added
    sqliteIssues and PRs related to the SQLite subsystem.
    on Oct 25, 2024
  2. badkeyy commented on Oct 28, 2024

    @badkeyy
    Contributor

    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 run throwing an error).
    Though if there are no immediate objections I will create a draft for this.

  3. tniessen commented on Oct 28, 2024

    @tniessen
    Member

    I suspect that this is an intentional design decision to make the API more robust. Similarly, should missing keys implicitly be treated as NULL values?

  4. added
    discussIssues opened for discussion and feedback.
    on Oct 28, 2024
  5. badkeyy commented on Oct 28, 2024

    @badkeyy
    Contributor

    Similarly, should missing keys implicitly be treated as NULL values?

    IMO this would lead to a lot of hard to detect bugs and confusion.

    @cjihrig What was your design intention here?

  6. webda2l commented on Oct 28, 2024

    @webda2l
    Author

    I couldn't find any libraries that throw an error for unused named parameters.. https://chatgpt.com/share/671f9240-9380-8006-af8b-6b74e8639d18

  7. badkeyy commented on Oct 28, 2024

    @badkeyy
    Contributor

    I couldn't find any libraries that throw an error for unused named parameters.. https://chatgpt.com/share/671f9240-9380-8006-af8b-6b74e8639d18

    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

  8. cjihrig commented on Oct 28, 2024

    @cjihrig
    Contributor

    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.

  9. webda2l commented on Oct 28, 2024

    @webda2l
    Author

    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).

    image
    https://codesandbox.io/p/devbox/black-worker-4t37md
    image
    https://codesandbox.io/p/devbox/h87vx8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    discussIssues opened for discussion and feedback.sqliteIssues and PRs related to the SQLite subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions