Skip to content

Commit

Permalink
Fix error trying to generate report for many items
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dstillman committed Jan 21, 2017
1 parent dcd1da7 commit 9b247eb
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 39 deletions.
3 changes: 0 additions & 3 deletions chrome/content/zotero/xpcom/api.js
Expand Up @@ -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
Expand Down
112 changes: 77 additions & 35 deletions chrome/content/zotero/xpcom/data/search.js
Expand Up @@ -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':
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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':
Expand Down
12 changes: 11 additions & 1 deletion chrome/content/zotero/xpcom/data/searchConditions.js
Expand Up @@ -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;
}
},

{
Expand Down
11 changes: 11 additions & 0 deletions test/tests/searchTest.js
Expand Up @@ -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');
Expand Down

0 comments on commit 9b247eb

Please sign in to comment.