From 9b247ebba7cfb11e7b8547cff03a1be223414adc Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Sat, 21 Jan 2017 03:38:36 -0500
Subject: [PATCH] Fix error trying to generate report for many items

When adding many search conditions (e.g., when matching many items with the
`key` condition), the query can fail due to either the bound parameter limit or
the expression tree size limit.

To avoid this, add support for an 'inlineFilter' property on search conditions
when using the 'is' or 'isNot' operator. 'inlineFilter' is a function that
returns a quoted value suitable for direct embedding in the SQL statement, or
false if not valid. Multiple consecutive conditions for the same 'inlineFilter'
field are combined into an `IN (x, y, z)` condition.
---
 chrome/content/zotero/xpcom/api.js            |   3 -
 chrome/content/zotero/xpcom/data/search.js    | 112 ++++++++++++------
 .../zotero/xpcom/data/searchConditions.js     |  12 +-
 test/tests/searchTest.js                      |  11 ++
 4 files changed, 99 insertions(+), 39 deletions(-)

diff --git a/chrome/content/zotero/xpcom/api.js b/chrome/content/zotero/xpcom/api.js
index 6fffe78d9..f3aa9b040 100644
--- a/chrome/content/zotero/xpcom/api.js
+++ b/chrome/content/zotero/xpcom/api.js
@@ -96,17 +96,14 @@ Zotero.API = {
 						s.addCondition('key', 'is', params.objectKey);
 					}
 					else if (params.objectID) {
-						Zotero.debug('adding ' + params.objectID);
 						s.addCondition('itemID', 'is', params.objectID);
 					}
 					
 					if (params.itemKey) {
-						s.addCondition('blockStart');
 						for (let i=0; i<params.itemKey.length; i++) {
 							let itemKey = params.itemKey[i];
 							s.addCondition('key', 'is', itemKey);
 						}
-						s.addCondition('blockEnd');
 					}
 					
 					// Display all top-level items
diff --git a/chrome/content/zotero/xpcom/data/search.js b/chrome/content/zotero/xpcom/data/search.js
index 5443f078c..2cdde4a22 100644
--- a/chrome/content/zotero/xpcom/data/search.js
+++ b/chrome/content/zotero/xpcom/data/search.js
@@ -925,61 +925,78 @@ Zotero.Search.prototype._buildQuery = Zotero.Promise.coroutine(function* () {
 	
 	var conditions = [];
 	
-	for (var i in this._conditions){
-		var data = Zotero.SearchConditions.get(this._conditions[i]['condition']);
+	let lastCondition;
+	for (let condition of Object.values(this._conditions)) {
+		let name = condition.condition;
+		let conditionData = Zotero.SearchConditions.get(name);
 		
 		// Has a table (or 'savedSearch', which doesn't have a table but isn't special)
-		if (data.table || data.name == 'savedSearch' || data.name == 'tempTable') {
-			conditions.push({
-				name: data['name'],
-				alias: data['name']!=this._conditions[i]['condition']
-					? this._conditions[i]['condition'] : false,
-				table: data['table'],
-				field: data['field'],
-				operator: this._conditions[i]['operator'],
-				value: this._conditions[i]['value'],
-				flags: data['flags'],
-				required: this._conditions[i]['required']
-			});
+		if (conditionData.table || name == 'savedSearch' || name == 'tempTable') {
+			// For conditions with an inline filter using 'is'/'isNot', combine with last condition
+			// if the same
+			if (lastCondition
+					&& name == lastCondition.name
+					&& condition.operator.startsWith('is')
+					&& condition.operator == lastCondition.operator
+					&& conditionData.inlineFilter) {
+				if (!Array.isArray(lastCondition.value)) {
+					lastCondition.value = [lastCondition.value];
+				}
+				lastCondition.value.push(condition.value);
+				continue;
+			}
+			
+			lastCondition = {
+				name,
+				alias: conditionData.name != name ? name : false,
+				table: conditionData.table,
+				field: conditionData.field,
+				operator: condition.operator,
+				value: condition.value,
+				flags: conditionData.flags,
+				required: condition.required,
+				inlineFilter: conditionData.inlineFilter
+			};
+			conditions.push(lastCondition);
 			
 			this._hasPrimaryConditions = true;
 		}
 		
 		// Handle special conditions
 		else {
-			switch (data['name']){
+			switch (conditionData.name) {
 				case 'deleted':
-					var deleted = this._conditions[i].operator == 'true';
+					var deleted = condition.operator == 'true';
 					continue;
 				
 				case 'noChildren':
-					var noChildren = this._conditions[i]['operator']=='true';
+					var noChildren = condition.operator == 'true';
 					continue;
 				
 				case 'includeParentsAndChildren':
-					var includeParentsAndChildren = this._conditions[i]['operator'] == 'true';
+					var includeParentsAndChildren = condition.operator == 'true';
 					continue;
 					
 				case 'includeParents':
-					var includeParents = this._conditions[i]['operator'] == 'true';
+					var includeParents = condition.operator == 'true';
 					continue;
 				
 				case 'includeChildren':
-					var includeChildren = this._conditions[i]['operator'] == 'true';
+					var includeChildren = condition.operator == 'true';
 					continue;
 				
 				case 'unfiled':
-					var unfiled = this._conditions[i]['operator'] == 'true';
+					var unfiled = condition.operator == 'true';
 					continue;
 				
 				// Search subcollections
 				case 'recursive':
-					var recursive = this._conditions[i]['operator']=='true';
+					var recursive = condition.operator == 'true';
 					continue;
 				
 				// Join mode ('any' or 'all')
 				case 'joinMode':
-					var joinMode = this._conditions[i]['operator'].toUpperCase();
+					var joinMode = condition.operator.toUpperCase();
 					continue;
 				
 				case 'fulltextContent':
@@ -995,7 +1012,7 @@ Zotero.Search.prototype._buildQuery = Zotero.Promise.coroutine(function* () {
 					continue;
 			}
 			
-			throw ('Unhandled special condition ' + this._conditions[i]['condition']);
+			throw new Error('Unhandled special condition ' + name);
 		}
 	}
 	
@@ -1462,20 +1479,45 @@ Zotero.Search.prototype._buildQuery = Zotero.Promise.coroutine(function* () {
 								
 							case 'is':
 							case 'isNot': // excluded with NOT IN above
-								// Automatically cast values which might
-								// have been stored as integers
-								if (condition.value && typeof condition.value == 'string'
-										&& condition.value.match(/^[1-9]+[0-9]*$/)) {
-									condSQL += ' LIKE ?';
-								}
-								else if (condition.value === null) {
-									condSQL += ' IS NULL';
-									break;
+								// If inline filter is available, embed value directly to get around
+								// the max bound parameter limit
+								if (condition.inlineFilter) {
+									let src = Array.isArray(condition.value)
+										? condition.value : [condition.value];
+									let values = [];
+									
+									for (let val of src) {
+										val = condition.inlineFilter(val);
+										if (val) {
+											values.push(val);
+										}
+										else {
+											Zotero.logError(`${val} is not a valid `
+												+ `'${condition.field}' value -- skipping`);
+											continue;
+										}
+									}
+									
+									condSQL += values.length > 1
+										? ` IN (${values.join(', ')})`
+										: `=${values[0]}`;
 								}
 								else {
-									condSQL += '=?';
+									// Automatically cast values which might
+									// have been stored as integers
+									if (condition.value && typeof condition.value == 'string'
+											&& condition.value.match(/^[1-9]+[0-9]*$/)) {
+										condSQL += ' LIKE ?';
+									}
+									else if (condition.value === null) {
+										condSQL += ' IS NULL';
+										break;
+									}
+									else {
+										condSQL += '=?';
+									}
+									condSQLParams.push(condition['value']);
 								}
-								condSQLParams.push(condition['value']);
 								break;
 							
 							case 'beginsWith':
diff --git a/chrome/content/zotero/xpcom/data/searchConditions.js b/chrome/content/zotero/xpcom/data/searchConditions.js
index 91ff4ed5b..9eced1fb1 100644
--- a/chrome/content/zotero/xpcom/data/searchConditions.js
+++ b/chrome/content/zotero/xpcom/data/searchConditions.js
@@ -450,7 +450,17 @@ Zotero.SearchConditions = new function(){
 				table: 'items',
 				field: 'key',
 				special: true,
-				noLoad: true
+				noLoad: true,
+				inlineFilter: function (val) {
+					try {
+						val = Zotero.DataObjectUtilities.checkKey(val);
+						if (val) return `'${val}'`;
+					}
+					catch (e) {
+						Zotero.logError(e);
+					}
+					return false;
+				}
 			},
 			
 			{
diff --git a/test/tests/searchTest.js b/test/tests/searchTest.js
index 030210782..42914b60a 100644
--- a/test/tests/searchTest.js
+++ b/test/tests/searchTest.js
@@ -224,6 +224,17 @@ describe("Zotero.Search", function() {
 				});
 			});
 			
+			describe("key", function () {
+				it("should allow more than max bound parameters", function* () {
+					let s = new Zotero.Search();
+					let max = Zotero.DB.MAX_BOUND_PARAMETERS + 100;
+					for (let i = 0; i < max; i++) {
+						s.addCondition('key', 'is', Zotero.DataObjectUtilities.generateKey());
+					}
+					yield s.search();
+				});
+			});
+			
 			describe("savedSearch", function () {
 				it("should return items in the saved search", function* () {
 					var search = yield createDataObject('search');