Skip to content

Don't treat parameters with an explicit index as named #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 3, 2025

Conversation

simolus3
Copy link
Collaborator

@simolus3 simolus3 commented Apr 2, 2025

Unlike most of the other libraries we use, better-sqlite3 supports binding parameters by name, extracting values from an object. The way the current implementation works is that named parameters in fact have to be given by name and can't be given as an array. For instance, the query SELECT :name can't be executed with execute(['Simon']), one has to use execute({name: 'Simon'}).

Unfortunately, this requirement is also applied to variables that aren't named at all! The implementation just used to check for sqlite3_bind_parameter_name returning NULL, but that's not the case for parameters with an explicit index (e.g. SELECT ?1).
As a consequence, better-sqlite3 behaves quite weirdly here and wouldn't allow that statement to be executed with execute(['Simon']), one would have to use {[1]: 'Simon'} I suppose. This behavior is different from our other SDKs, which is obviously quite bad.

This PR fixes the internal check to treat ?NNN variables as non-named. This should mostly keep backwards compatibility while also ensuring that we can use these parameters with better-sqlite3. There's still a problem if users other named variables (SELECT :name) because our Node SDK will pass an array to better-sqlite3 which won't work.

One could argue that this approach of binding named parameters is fundamentally flawed to begin with (because in SQLite, named parameters also have an associated index). So passing an array to bind named parameters should be allowed, but it breaks better-sqlite3. I didn't want to do that change right now because I'd then have to also fix a lot of tests. But it might be worth ripping out all special checks for named parameters entirely to restore

@simolus3 simolus3 requested a review from stevensJourney April 2, 2025 08:59
@simolus3 simolus3 merged commit 35a97d1 into master Apr 3, 2025
4 checks passed
@simolus3 simolus3 deleted the fix-bind-with-explicit-index branch April 3, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants