Skip to content
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

Fix binding of explicitly numbered parameters #1032

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mvduin
Copy link

@mvduin mvduin commented Jul 19, 2023

Parameters of the form ?1, ?2, etc aren't named parameters, you're explicitly specifying the parameter index, allowing indexed (i.e. unnamed) parameters to be explicitly ordered, reused, or skipped. Somewhat oddly however sqlite still reports their name* from sqlite3_bind_parameter_name, causing better-sqlite3 to force it to be passed by name which is not what's expected or desired.

(* or rather their first name, e.g. SELECT ?01, ?1, ?001 has a single parameter whose reported name is ?01 though I don't think sqlite documents which of the three names is returned in this case.)

This patch-set fixes this and adds some tests for explicitly numbered parameters (there were none whatsoever).

The reason I care about explicitly numbered parameters is because I'm using tagged templates for queries, and using explicit numbering allows this to be much more robust against e.g. parts of the query being comments out. Here's a highly simplified minimalistic demo of this concept:

class Database extends require('better-sqlite3') {
	#compiled = new WeakMap;
	compile( tmpl ) {
		let stmt = this.#compiled.get( tmpl );
		if( stmt )
			return stmt;
		let sql = '';
		tmpl.forEach( (chunk, i) => {
			if (i) sql += ` ?${i} `;
			sql += chunk;
		});
		console.log(`COMPILING: \x1b[2m${sql.trim()}\x1b[m`);
		stmt = this.prepare( sql );
		this.#compiled.set( tmpl, stmt );
		return stmt;
	}
	get( tmpl, ...args ) {  return this.compile( tmpl ).get( ...args );  }
}

let db = new Database(':memory:');
function demo( a, b ) {
	return db.get`SELECT ${a} as a, ${b} as b`;
}
console.log( demo( 1, 2 ) );
console.log( demo( 'foo', 'bar' ) );

function demo2( a, b, c ) {
	return db.get`
		SELECT ${a} as a,
		--	${b} as b,  -- FIXME can we get rid of this?
			${c} as c`;
}
console.log( demo2( 1, 2, 3 ) );
console.log( demo2( 'foo', 'bar', 'baz' ) );

output:

COMPILING: SELECT  ?1  as a,  ?2  as b
{ a: 1, b: 2 }
{ a: 'foo', b: 'bar' }
COMPILING: SELECT  ?1  as a,
                --       ?2  as b,  -- FIXME can we get rid of this?
                         ?3  as c
{ a: 1, c: 3 }
{ a: 'foo', c: 'baz' }

@mvduin mvduin requested a review from JoshuaWise as a code owner July 19, 2023 11:23
@s5bug
Copy link

s5bug commented Feb 21, 2024

What's preventing this from being merged? I'm working on improving parameter handling in a library I use that depends on better-sqlite3, and this bug has unfortunately become a blocker.

s5bug added a commit to s5bug/porcupine that referenced this pull request Mar 23, 2024
creates a binding dictionary explicitly on JS, zipping the arguments with their index and binding str(idx+1) to the argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants