Merge 2b928b8a01 into e863643a43

chrome/content/zotero/xpcom/data/dataObjects.js

@@ -555,14 +555,17 @@ Zotero.DataObjects.prototype._loadPrimaryData = async function (libraryID, ids,
 		sql += ' AND O.' + this._ZDO_id + ' IN (' + ids.join(',') + ')';
 	}
 	
+	var columns = Object.keys(this._primaryDataSQLParts);
+	var idColumn = columns.indexOf(this._ZDO_id);
 	await Zotero.DB.queryAsync(
 		sql,
 		params,
 		{
 			noCache: true,
 			onRow: function (row) {
-				var id = row.getResultByName(this._ZDO_id);
-				var columns = Object.keys(this._primaryDataSQLParts);
+				var id = idColumn != -1
+					? row.getResultByIndex(idColumn)
+					: row.getResultByName(this._ZDO_id);
 				var rowObj = {};
 				for (let i=0; i<columns.length; i++) {
 					rowObj[columns[i]] = row.getResultByIndex(i);

chrome/content/zotero/xpcom/data/item.js

@@ -794,7 +794,30 @@ Zotero.Item.prototype.setField = function (field, value, loadIn) {
 	if (!itemTypeID) {
 		throw new Error('Item type must be set before setting field data');
 	}
-	
+
+	// Loading calls this for every stored value, so use cached field lookups.
+	// Same result as the general path below.
+	if (loadIn && typeof field == 'number') {
+		let info = Zotero.ItemFields._getLoadInfo(itemTypeID, field);
+		if (info && !info.isISBN) {
+			if (field == info.titleID && this.isNote()) {
+				this._noteTitle = value ? value : "";
+				return true;
+			}
+			if (value !== false && !info.valid) {
+				Zotero.debug("'" + field + "' is not a valid field for type '"
+					+ Zotero.ItemTypes.getName(itemTypeID) + "'" + " -- ignoring value '" + value + "'", 2);
+				return false;
+			}
+			if (typeof value == 'string' && !info.multiline
+					&& (value.includes('\n') || value.includes('\r'))) {
+				value = value.replace(/[\r\n]+/g, " ");
+			}
+			this._itemData[info.fieldID] = value;
+			return true;
+		}
+	}
+
 	var fieldID = Zotero.ItemFields.getID(field);
 	if (!fieldID) {
 		throw new Error('"' + field + '" is not a valid itemData field');

chrome/content/zotero/xpcom/data/itemFields.js

@@ -39,7 +39,9 @@ Zotero.ItemFields = new function () {
 	var _typeFieldNamesByBase = {};
 	var _baseFieldIDsByTypeAndField = {};
 	var _autocompleteFields = null;
-	
+	var _loadInfo = [];
+	var _loadInfoMethods = {};
+
 	// Privileged methods
 	this.getName = getName;
 	this.getID = getID;
@@ -59,6 +61,7 @@ Zotero.ItemFields = new function () {
 	this.init = async function () {
 		_fields = {};
 		_fieldsFormats = [];
+		_loadInfo = [];
 		
 		var result = await Zotero.DB.queryAsync('SELECT * FROM fieldFormats');
 		
@@ -123,6 +126,57 @@ Zotero.ItemFields = new function () {
 	}
 	
 	
+	/**
+	 * The lookups Zotero.Item#setField() does for every value loaded from the
+	 * database, cached per item type and field
+	 *
+	 * The cache is dropped if any method it depends on is replaced, so an
+	 * overridden method is still consulted.
+	 *
+	 * @param {Integer} itemTypeID
+	 * @param {Integer} fieldID
+	 * @return {Object|false} - false for an unknown field
+	 */
+	this._getLoadInfo = function (itemTypeID, fieldID) {
+		if (_loadInfoMethods.getID !== this.getID
+				|| _loadInfoMethods.getFieldIDFromTypeAndBase !== this.getFieldIDFromTypeAndBase
+				|| _loadInfoMethods.isValidForType !== this.isValidForType
+				|| _loadInfoMethods.isMultiline !== this.isMultiline
+				|| _loadInfoMethods.getItemTypeID !== Zotero.ItemTypes.getID) {
+			_loadInfo = [];
+			_loadInfoMethods = {
+				getID: this.getID,
+				getFieldIDFromTypeAndBase: this.getFieldIDFromTypeAndBase,
+				isValidForType: this.isValidForType,
+				isMultiline: this.isMultiline,
+				getItemTypeID: Zotero.ItemTypes.getID
+			};
+		}
+		var byField = _loadInfo[itemTypeID];
+		if (!byField) {
+			byField = _loadInfo[itemTypeID] = [];
+		}
+		var info = byField[fieldID];
+		if (info === undefined) {
+			if (!this.getID(fieldID)) {
+				info = false;
+			}
+			else {
+				let resolvedID = this.getFieldIDFromTypeAndBase(itemTypeID, fieldID) || fieldID;
+				info = {
+					fieldID: resolvedID,
+					titleID: this.getID('title'),
+					valid: this.isValidForType(resolvedID, itemTypeID),
+					multiline: this.isMultiline(resolvedID),
+					isISBN: resolvedID == this.getID('ISBN')
+				};
+			}
+			byField[fieldID] = info;
+		}
+		return info;
+	};
+
+
 	/*
 	 * Return the fieldName for a passed fieldID or fieldName
 	 */

chrome/content/zotero/xpcom/data/items.js

@@ -298,6 +298,7 @@ Zotero.Items = function () {
 		var sql = "SELECT itemID FROM items WHERE libraryID=?" + idSQL;
 		var params = [libraryID];
 		var allItemIDs = [];
+		var fieldIDsByType = new Map();
 		await Zotero.DB.queryAsync(
 			sql,
 			params,
@@ -306,9 +307,13 @@ Zotero.Items = function () {
 				onRow: function (row) {
 					let itemID = row.getResultByIndex(0);
 					let item = this._objectCache[itemID];
-					
+
 					// Set nonexistent fields in the cache list to false (instead of null)
-					let fieldIDs = Zotero.ItemFields.getItemTypeFields(item.itemTypeID);
+					let fieldIDs = fieldIDsByType.get(item.itemTypeID);
+					if (!fieldIDs) {
+						fieldIDs = Zotero.ItemFields.getItemTypeFields(item.itemTypeID);
+						fieldIDsByType.set(item.itemTypeID, fieldIDs);
+					}
 					for (let j=0; j<fieldIDs.length; j++) {
 						let fieldID = fieldIDs[j];
 						if (!itemFieldsCached[itemID] || !itemFieldsCached[itemID][fieldID]) {
@@ -399,7 +404,6 @@ Zotero.Items = function () {
 			+ 'FROM items LEFT JOIN itemCreators USING (itemID) '
 			+ 'WHERE libraryID=?' + idSQL + " ORDER BY itemID, orderIndex";
 		var params = [libraryID];
-		var rows = await Zotero.DB.queryAsync(sql, params, { noCache: true });
 		
 		// Mark creator indexes above the number of creators as changed,
 		// so that they're cleared if the item is saved
@@ -420,46 +424,52 @@ Zotero.Items = function () {
 		var item;
 		var index = 0;
 		var maxOrderIndex = -1;
-		for (let i = 0; i < rows.length; i++) {
-			let row = rows[i];
-			let itemID = row.itemID;
-			
-			if (itemID != lastItemID) {
-				if (!this._objectCache[itemID]) {
-					throw new Error("Item " + itemID + " not loaded");
-				}
-				item = this._objectCache[itemID];
+		// Read columns by index as rows arrive, rather than building a Proxy row
+		// object for every item/creator pair
+		await Zotero.DB.queryAsync(sql, params, {
+			noCache: true,
+			onRow: (row, cancel) => {
+				let itemID = row.getResultByIndex(0);
+				let creatorID = row.getResultByIndex(1);
 				
-				item._creators = [];
-				item._creatorIDs = [];
-				item._loaded.creators = true;
-				item._clearChanged('creators');
-				
-				if (!row.creatorID) {
-					lastItemID = row.itemID;
-					continue;
+				if (itemID != lastItemID) {
+					if (!this._objectCache[itemID]) {
+						throw new Error("Item " + itemID + " not loaded");
+					}
+					item = this._objectCache[itemID];
+					
+					item._creators = [];
+					item._creatorIDs = [];
+					item._loaded.creators = true;
+					item._clearChanged('creators');
+					
+					if (!creatorID) {
+						lastItemID = itemID;
+						return;
+					}
+					
+					if (index <= maxOrderIndex) {
+						fixIncorrectIndexes(item, index, maxOrderIndex);
+					}
+					
+					index = 0;
+					maxOrderIndex = -1;
 				}
 				
-				if (index <= maxOrderIndex) {
-					fixIncorrectIndexes(item, index, maxOrderIndex);
+				lastItemID = itemID;
+				
+				let orderIndex = row.getResultByIndex(3);
+				if (orderIndex > maxOrderIndex) {
+					maxOrderIndex = orderIndex;
 				}
 				
-				index = 0;
-				maxOrderIndex = -1;
+				let creatorData = Zotero.Creators.get(creatorID);
+				creatorData.creatorTypeID = row.getResultByIndex(2);
+				item._creators[index] = creatorData;
+				item._creatorIDs[index] = creatorID;
+				index++;
 			}
-			
-			lastItemID = row.itemID;
-			
-			if (row.orderIndex > maxOrderIndex) {
-				maxOrderIndex = row.orderIndex;
-			}
-			
-			let creatorData = Zotero.Creators.get(row.creatorID);
-			creatorData.creatorTypeID = row.creatorTypeID;
-			item._creators[index] = creatorData;
-			item._creatorIDs[index] = row.creatorID;
-			index++;
-		}
+		});
 		
 		if (index <= maxOrderIndex) {
 			fixIncorrectIndexes(item, index, maxOrderIndex);

test/tests/itemFieldsTest.js

@@ -64,4 +64,42 @@ describe("Zotero.ItemFields", function () {
 			assert.equal(Zotero.ItemFields.getDirection('book', 'creator-0-lastName', 'ar'), 'rtl');
 		});
 	});
+
+	describe("#_getLoadInfo()", function () {
+		it("should resolve a base field to the item type's field", function () {
+			var info = Zotero.ItemFields._getLoadInfo(
+				Zotero.ItemTypes.getID('thesis'), Zotero.ItemFields.getID('publisher')
+			);
+			assert.equal(info.fieldID, Zotero.ItemFields.getID('university'));
+			assert.isTrue(info.valid);
+		});
+
+		it("should mark a field that isn't valid for the item type", function () {
+			var info = Zotero.ItemFields._getLoadInfo(
+				Zotero.ItemTypes.getID('book'), Zotero.ItemFields.getID('websiteTitle')
+			);
+			assert.isFalse(info.valid);
+		});
+
+		it("should return false for an unknown field", function () {
+			assert.isFalse(Zotero.ItemFields._getLoadInfo(Zotero.ItemTypes.getID('book'), 999999));
+		});
+
+		// A plugin that replaces a lookup method must still be consulted
+		it("should use a replaced lookup method", function () {
+			var bookID = Zotero.ItemTypes.getID('book');
+			var titleID = Zotero.ItemFields.getID('title');
+			assert.isTrue(Zotero.ItemFields._getLoadInfo(bookID, titleID).valid);
+
+			var original = Zotero.ItemFields.isValidForType;
+			Zotero.ItemFields.isValidForType = () => false;
+			try {
+				assert.isFalse(Zotero.ItemFields._getLoadInfo(bookID, titleID).valid);
+			}
+			finally {
+				Zotero.ItemFields.isValidForType = original;
+			}
+			assert.isTrue(Zotero.ItemFields._getLoadInfo(bookID, titleID).valid);
+		});
+	});
 })

test/tests/itemsTest.js

@@ -799,4 +799,75 @@ describe("Zotero.Items", function () {
 			assert.include(ids, att.id);
 		});
 	});
+
+	describe("#_loadItemData()", function () {
+		// Point a stored field at a raw value, bypassing setField(), then reload from the DB
+		async function loadRaw(item, field, raw) {
+			var fieldID = Zotero.ItemFields.getID(field);
+			var valueID = await Zotero.DB.valueQueryAsync("SELECT valueID FROM itemDataValues WHERE value=?", raw);
+			if (!valueID) {
+				valueID = Zotero.ID.get('itemDataValues');
+				await Zotero.DB.queryAsync("INSERT INTO itemDataValues (valueID, value) VALUES (?, ?)", [valueID, raw]);
+			}
+			await Zotero.DB.queryAsync(
+				"REPLACE INTO itemData (itemID, fieldID, valueID) VALUES (?, ?, ?)", [item.id, fieldID, valueID]
+			);
+			await Zotero.Items._loadDataTypeInLibrary('itemData', item.libraryID, [item.id]);
+		}
+
+		it("should strip newlines from single-line fields when loading", async function () {
+			var item = await createDataObject('item', { title: "Title" });
+			await loadRaw(item, 'title', "Line one\nLine two");
+			assert.equal(item.getField('title'), "Line one Line two");
+		});
+
+		it("should keep newlines in multiline fields when loading", async function () {
+			var item = await createDataObject('item');
+			await loadRaw(item, 'abstractNote', "Line one\nLine two");
+			assert.equal(item.getField('abstractNote'), "Line one\nLine two");
+		});
+
+		it("should hyphenate ISBNs when loading", async function () {
+			var item = await createDataObject('item', { itemType: 'book' });
+			await loadRaw(item, 'ISBN', "9780306406157");
+			assert.equal(item.getField('ISBN'), "978-0-306-40615-7");
+		});
+
+		it("should load a base-mapped field into the item type's field", async function () {
+			var item = await createDataObject('item', { itemType: 'thesis' });
+			await loadRaw(item, 'university', "University of Somewhere");
+			assert.equal(item.getField('university'), "University of Somewhere");
+			assert.equal(item.getField('publisher', false, true), "University of Somewhere");
+		});
+
+		it("should set absent fields to empty", async function () {
+			var item = await createDataObject('item', { itemType: 'book' });
+			await Zotero.Items._loadDataTypeInLibrary('itemData', item.libraryID, [item.id]);
+			assert.strictEqual(item.getField('volume'), "");
+		});
+
+		// Plugins wrap Zotero.Item.prototype.setField (e.g., zotero-plugin-toolkit's
+		// FieldHook), so loading must keep calling it for every value
+		it("should call setField for every loaded value", async function () {
+			var item = await createDataObject('item', { itemType: 'book', title: "Called" });
+			var original = Zotero.Item.prototype.setField;
+			var calls = [];
+			Zotero.Item.prototype.setField = function (field, value, loadIn) {
+				if (this.id == item.id) {
+					calls.push([field, value, loadIn]);
+				}
+				return original.apply(this, arguments);
+			};
+			try {
+				await Zotero.Items._loadDataTypeInLibrary('itemData', item.libraryID, [item.id]);
+			}
+			finally {
+				Zotero.Item.prototype.setField = original;
+			}
+			var titleID = Zotero.ItemFields.getID('title');
+			assert.deepInclude(calls, [titleID, "Called", true]);
+			// One call per field of the item type: stored values plus absent ones set empty
+			assert.lengthOf(calls, Zotero.ItemFields.getItemTypeFields(item.itemTypeID).length);
+		});
+	});
 });