From 25621d3a21ec06ce340592511891b577d4062476 Mon Sep 17 00:00:00 2001 From: TheNEwmanator15 Date: Thu, 24 Sep 2026 21:53:40 +0100 Subject: [PATCH 1/3] Cache field lookups when loading item data Zotero.Item#setField() in loadIn mode repeats the same lookups for every value (field ID, type-specific field, validity for the type, multiline), several of which allocate. Cache them per item type and field in Zotero.ItemFields._getLoadInfo(). The cache is rebuilt by ItemFields.init() and dropped if any method it depends on is replaced, so an overridden method is still consulted. Also look up each item type's fields once per load rather than once per item. setField() is still called for every value with the same arguments, so code that wraps it sees the same calls, and the loaded data is identical. Co-Authored-By: Claude Opus 5.5 --- chrome/content/zotero/xpcom/data/item.js | 25 ++++++++- .../content/zotero/xpcom/data/itemFields.js | 56 ++++++++++++++++++- chrome/content/zotero/xpcom/data/items.js | 9 ++- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index c80df3c129..721b795312 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/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'); diff --git a/chrome/content/zotero/xpcom/data/itemFields.js b/chrome/content/zotero/xpcom/data/itemFields.js index a9e16e1675..a884fe6dbe 100644 --- a/chrome/content/zotero/xpcom/data/itemFields.js +++ b/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] = new Map(); + } + var info = byField.get(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.set(fieldID, info); + } + return info; + }; + + /* * Return the fieldName for a passed fieldID or fieldName */ diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index dd35b89a53..f98b5eadcc 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/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 Date: Fri, 25 Sep 2026 00:11:48 +0100 Subject: [PATCH 2/3] Load creators and primary data with less per-row work - _loadCreators(): read columns by index in onRow as rows arrive instead of materializing every item/creator row as a Proxy, whose property reads each go through getResultByName() - _loadPrimaryData(): compute the column list and ID column once per query rather than once per row - ItemFields._getLoadInfo(): index the cache with arrays instead of a Map Co-Authored-By: Claude Opus 5.5 --- .../content/zotero/xpcom/data/dataObjects.js | 7 +- .../content/zotero/xpcom/data/itemFields.js | 6 +- chrome/content/zotero/xpcom/data/items.js | 75 ++++++++++--------- 3 files changed, 48 insertions(+), 40 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/dataObjects.js b/chrome/content/zotero/xpcom/data/dataObjects.js index 3e5726602d..414d438f05 100644 --- a/chrome/content/zotero/xpcom/data/dataObjects.js +++ b/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 { + 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); From 2b928b8a01452a331d8700ca6f804d26e6b84ace Mon Sep 17 00:00:00 2001 From: TheNEwmanator15 <3686761+Thenewmanator15@users.noreply.github.com> Date: Sat, 26 Sep 2026 01:19:45 +0100 Subject: [PATCH 3/3] Test item data loading and the field lookup cache The _loadItemData() tests pass on the code before this change too: they pin the normalization it must keep (newlines, ISBN hyphenation, base-mapped fields, absent fields) and that setField() is still called for every value. Co-Authored-By: Claude Opus 5.5 --- test/tests/itemFieldsTest.js | 38 +++++++++++++++++++ test/tests/itemsTest.js | 71 ++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/test/tests/itemFieldsTest.js b/test/tests/itemFieldsTest.js index 5ea3e09f09..c2c48ba0c1 100644 --- a/test/tests/itemFieldsTest.js +++ b/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); + }); + }); }) diff --git a/test/tests/itemsTest.js b/test/tests/itemsTest.js index 9842f2bd60..fb442e012c 100644 --- a/test/tests/itemsTest.js +++ b/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); + }); + }); });