Restore DB parameter checking and add tests

Some parameter situations also weren't being properly handled

chrome/content/zotero/xpcom/db.js

@@ -23,6 +23,8 @@
     ***** END LICENSE BLOCK *****
 */
 
+"use strict";
+
 // Exclusive locking mode (default) prevents access to Zotero database while Zotero is open
 // and speeds up DB access (http://www.sqlite.org/pragma.html#pragma_locking_mode).
 // Normal mode is more convenient for development, but risks database corruption, particularly if
@@ -122,7 +124,7 @@ Zotero.DBConnection.prototype.getAsyncStatement = Zotero.Promise.coroutine(funct
 });
 
 
-Zotero.DBConnection.prototype.parseQueryAndParams = function (sql, params, options) {
+Zotero.DBConnection.prototype.parseQueryAndParams = function (sql, params) {
 	// If single scalar value, wrap in an array
 	if (!Array.isArray(params)) {
 		if (typeof params == 'string' || typeof params == 'number' || params === null) {
@@ -138,31 +140,43 @@ Zotero.DBConnection.prototype.parseQueryAndParams = function (sql, params, optio
 	}
 	
 	// Find placeholders
-	var placeholderRE = /\s*[=,(]\s*\?/g;
-	
 	if (params.length) {
-		if (options && options.checkParams) {
+		let matches = sql.match(/\?\d*/g);
-			let matches = sql.match(placeholderRE);
+		if (!matches) {
-			
+			throw new Error("Parameters provided for query without placeholders "
-			if (!matches) {
+				+ "[QUERY: " + sql + "]");
-				throw new Error("Parameters provided for query without placeholders "
+		}
-					+ "[QUERY: " + sql + "]");
+		else {
+			// Count numbered parameters (?1) properly
+			let num = 0;
+			let numbered = {};
+			for (let i = 0; i < matches.length; i++) {
+				let match = matches[i];
+				if (match == '?') {
+					num++;
+				}
+				else {
+					numbered[match] = true;
+				}
 			}
-			else if (matches.length != params.length) {
+			num += Object.keys(numbered).length;
+			
+			if (params.length != num) {
 				throw new Error("Incorrect number of parameters provided for query "
-					+ "(" + params.length + ", expecting " + matches.length + ") "
+					+ "(" + params.length + ", expecting " + num + ") "
 					+ "[QUERY: " + sql + "]");
 			}
 		}
 		
 		// First, determine the type of query using first word
-		var matches = sql.match(/^[^\s\(]*/);
+		let queryMethod = sql.match(/^[^\s\(]*/)[0].toLowerCase();
-		var queryMethod = matches[0].toLowerCase();
 		
 		// Reset lastIndex, since regexp isn't recompiled dynamically
-		placeholderRE.lastIndex = 0;
+		let placeholderRE = /\s*[=,(]\s*\?/g;
-		var lastNullParamIndex = -1;
 		for (var i=0; i<params.length; i++) {
+			// Find index of this parameter, skipping previous ones
+			matches = placeholderRE.exec(sql);
+			
 			if (typeof params[i] == 'boolean') {
 				throw new Error("Invalid boolean parameter " + i + " '" + params[i] + "' "
 					+ "[QUERY: " + sql + "]");
@@ -193,23 +207,17 @@ Zotero.DBConnection.prototype.parseQueryAndParams = function (sql, params, optio
 			//
 			// Replace NULL bound parameters with hard-coded NULLs
 			//
-			
-			// Find index of this parameter, skipping previous ones
-			do {
-				var matches = placeholderRE.exec(sql);
-				lastNullParamIndex++;
-			}
-			while (lastNullParamIndex < i);
-			lastNullParamIndex = i;
-			
 			if (!matches) {
 				throw new Error("Null parameter provided for a query without placeholders "
 					+ "-- use false or undefined [QUERY: " + sql + "]");
 			}
 			
-			if (matches[0].indexOf('=') == -1) {
+			if (matches[0].trim().indexOf('=') == -1) {
-				// mozStorage supports null bound parameters in value lists (e.g., "(?,?)") natively
+				if (queryMethod == 'select') {
-				continue;
+					throw new Error("NULL cannot be used for parenthesized placeholders "
+						+ "in SELECT queries [QUERY: " + sql + "]");
+				}
+				var repl = matches[0].replace('?', 'NULL');
 			}
 			else if (queryMethod == 'select') {
 				var repl = ' IS NULL';
@@ -226,14 +234,12 @@ Zotero.DBConnection.prototype.parseQueryAndParams = function (sql, params, optio
 			
 			params.splice(i, 1);
 			i--;
-			lastNullParamIndex--;
-			continue;
 		}
 		if (!params.length) {
 			params = [];
 		}
 	}
-	else if (options && options.checkParams && placeholderRE.test(sql)) {
+	else if (/\?/g.test(sql)) {
 		throw new Error("Parameters not provided for query containing placeholders "
 			+ "[QUERY: " + sql + "]");
 	}

test/tests/dbTest.js

@@ -15,6 +15,125 @@ describe("Zotero.DB", function() {
 		yield Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tmpTable);
 	});
 	
+	
+	describe("#queryAsync()", function () {
+		var tmpTable;
+		
+		before(function* () {
+			tmpTable = "tmp_queryAsync";
+			yield Zotero.DB.queryAsync("CREATE TEMPORARY TABLE " + tmpTable + " (a, b)");
+			yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (1, 2)");
+			yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (3, 4)");
+			yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (5, NULL)");
+		})
+		after(function* () {
+			if (tmpTable) {
+				yield Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tmpTable);
+			}
+		})
+		
+		it("should throw an error if no parameters are passed for a query with placeholders", function* () {
+			var e = yield getPromiseError(Zotero.DB.queryAsync("SELECT itemID FROM items WHERE itemID=?"));
+			assert.ok(e);
+			assert.include(e.message, "for query containing placeholders");
+		})
+		
+		it("should throw an error if too few parameters are passed", function* () {
+			var e = yield getPromiseError(Zotero.DB.queryAsync("SELECT itemID FROM items WHERE itemID=? OR itemID=?", [1]));
+			assert.ok(e);
+			assert.include(e.message, "Incorrect number of parameters provided for query");
+		})
+		
+		it("should throw an error if too many parameters are passed", function* () {
+			var e = yield getPromiseError(Zotero.DB.queryAsync("SELECT itemID FROM items WHERE itemID=?", [1, 2]));
+			assert.ok(e);
+			assert.include(e.message, "Incorrect number of parameters provided for query");
+		})
+		
+		it("should throw an error if too many parameters are passed for numbered placeholders", function* () {
+			var e = yield getPromiseError(Zotero.DB.queryAsync("SELECT itemID FROM items WHERE itemID=?1 OR itemID=?1", [1, 2]));
+			assert.ok(e);
+			assert.include(e.message, "Incorrect number of parameters provided for query");
+		})
+		
+		it("should accept a single placeholder given as a value", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b=?", 2);
+			assert.lengthOf(rows, 1);
+			assert.equal(rows[0].a, 1);
+		})
+		
+		it("should accept a single placeholder given as an array", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b=?", [2]);
+			assert.lengthOf(rows, 1);
+			assert.equal(rows[0].a, 1);
+		})
+		
+		it("should accept multiple placeholders", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b=? OR b=?", [2, 4]);
+			assert.lengthOf(rows, 2);
+			assert.equal(rows[0].a, 1);
+			assert.equal(rows[1].a, 3);
+		})
+		
+		it("should accept a single placeholder within parentheses", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b IN (?)", 2);
+			assert.lengthOf(rows, 1);
+			assert.equal(rows[0].a, 1);
+		})
+		
+		it("should accept multiple placeholders within parentheses", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b IN (?, ?)", [2, 4]);
+			assert.lengthOf(rows, 2);
+			assert.equal(rows[0].a, 1);
+			assert.equal(rows[1].a, 3);
+		})
+		
+		it("should replace =? with IS NULL if NULL is passed as a value", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b=?", null);
+			assert.lengthOf(rows, 1);
+			assert.equal(rows[0].a, 5);
+		})
+		
+		it("should replace =? with IS NULL if NULL is passed in an array", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b=?", [null]);
+			assert.lengthOf(rows, 1);
+			assert.equal(rows[0].a, 5);
+		})
+		
+		it("should replace ? with NULL for placeholders within parentheses in INSERT statements", function* () {
+			yield Zotero.DB.queryAsync("CREATE TEMPORARY TABLE tmp_srqwnfpwpinss (a, b)");
+			// Replace ", ?"
+			yield Zotero.DB.queryAsync("INSERT INTO tmp_srqwnfpwpinss (a, b) VALUES (?, ?)", [1, null]);
+			assert.equal(
+				(yield Zotero.DB.valueQueryAsync("SELECT a FROM tmp_srqwnfpwpinss WHERE b IS NULL")),
+				1
+			);
+			// Replace "(?"
+			yield Zotero.DB.queryAsync("DELETE FROM tmp_srqwnfpwpinss");
+			yield Zotero.DB.queryAsync("INSERT INTO tmp_srqwnfpwpinss (a, b) VALUES (?, ?)", [null, 2]);
+			assert.equal(
+				(yield Zotero.DB.valueQueryAsync("SELECT b FROM tmp_srqwnfpwpinss WHERE a IS NULL")),
+				2
+			);
+			yield Zotero.DB.queryAsync("DROP TABLE tmp_srqwnfpwpinss");
+		})
+		
+		it("should throw an error if NULL is passed for placeholder within parentheses in a SELECT statement", function* () {
+			var e = yield getPromiseError(Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b IN (?)", null));
+			assert.ok(e);
+			assert.include(e.message, "NULL cannot be used for parenthesized placeholders in SELECT queries");
+		})
+		
+		it("should handle numbered parameters", function* () {
+			var rows = yield Zotero.DB.queryAsync("SELECT a FROM " + tmpTable + " WHERE b=?1 "
+				+ "UNION SELECT b FROM " + tmpTable + " WHERE b=?1", 2);
+			assert.lengthOf(rows, 2);
+			assert.equal(rows[0].a, 1);
+			assert.equal(rows[1].a, 2);
+		})
+	})
+	
+	
 	describe("#executeTransaction()", function () {
 		it("should serialize concurrent transactions", Zotero.Promise.coroutine(function* () {
 			this.timeout(1000);