From 9dd17a212ed63ef5eb72fc1c04d1d21cb228b537 Mon Sep 17 00:00:00 2001 From: Abe Jellinek <1770299+AbeJellinek@users.noreply.github.com> Date: Mon, 27 Jul 2026 15:49:42 -0400 Subject: [PATCH] Track data object versions locally Add a clientVersion column for items, collections, searches, and libraries, incremented once per library per transaction on every object save or deletion. The local API reports these versions instead of synced versions -- in object JSON, format=versions, since= filtering, and Last-Modified-Version -- since synced versions don't reflect local changes and are 0 for unsynced objects. Group metadata responses keep reporting the synced group version, which has no local counterpart. --------- Co-authored-by: Dan Stillman --- .../content/zotero/xpcom/data/collection.js | 4 ++ .../content/zotero/xpcom/data/collections.js | 1 + .../content/zotero/xpcom/data/dataObject.js | 20 +++++- chrome/content/zotero/xpcom/data/group.js | 3 + chrome/content/zotero/xpcom/data/item.js | 25 +++++--- chrome/content/zotero/xpcom/data/items.js | 1 + chrome/content/zotero/xpcom/data/library.js | 36 ++++++++++- chrome/content/zotero/xpcom/data/search.js | 4 ++ chrome/content/zotero/xpcom/data/searches.js | 1 + chrome/content/zotero/xpcom/data/tags.js | 12 ++-- chrome/content/zotero/xpcom/db.js | 1 + chrome/content/zotero/xpcom/schema.js | 7 ++ .../zotero/xpcom/server/server_localAPI.js | 41 ++++++++++-- resource/schema/userdata.sql | 6 +- test/content/support.js | 2 +- test/tests/dataObjectTest.js | 64 +++++++++++++++++++ test/tests/libraryTest.js | 21 ++++++ test/tests/server_localAPITest.js | 59 ++++++++++++++++- 18 files changed, 281 insertions(+), 27 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index a68f150dc3..ad25464f89 100644 --- a/chrome/content/zotero/xpcom/data/collection.js +++ b/chrome/content/zotero/xpcom/data/collection.js @@ -67,6 +67,10 @@ Zotero.defineProperty(Zotero.Collection.prototype, 'version', { get: function () { return this._get('version'); }, set: function (val) { return this._set('version', val); } }); +Zotero.defineProperty(Zotero.Collection.prototype, 'clientVersion', { + get: function() { return this._get('clientVersion'); }, + set: function(val) { return this._set('clientVersion', val); } +}); Zotero.defineProperty(Zotero.Collection.prototype, 'synced', { get: function () { return this._get('synced'); }, set: function (val) { return this._set('synced', val); } diff --git a/chrome/content/zotero/xpcom/data/collections.js b/chrome/content/zotero/xpcom/data/collections.js index e38e29527d..0a4cecd197 100644 --- a/chrome/content/zotero/xpcom/data/collections.js +++ b/chrome/content/zotero/xpcom/data/collections.js @@ -38,6 +38,7 @@ Zotero.Collections = function () { libraryID: "O.libraryID", key: "O.key", version: "O.version", + clientVersion: "O.clientVersion", synced: "O.synced", deleted: "DC.collectionID IS NOT NULL AS deleted", diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index f3a0c6caa3..5b92ea0bec 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -50,6 +50,7 @@ Zotero.DataObject = function () { this._dateAdded = null; this._dateModified = null; this._version = null; + this._clientVersion = null; this._synced = null; this._identified = false; this._parentID = null; @@ -1296,6 +1297,13 @@ Zotero.DataObject.prototype._finalizeSave = async function (env) { else if (env.skipCache) { Zotero.logError("skipCache is only for new objects"); } + + let libraryClientVersion = await this.library.incrementClientVersion(); + await Zotero.DB.queryAsync( + `UPDATE ${this.ObjectsClass.table} SET clientVersion = ? WHERE ${this.ObjectsClass.idColumn}=?`, + [libraryClientVersion, this.id] + ); + this._clientVersion = libraryClientVersion; }; @@ -1454,6 +1462,8 @@ Zotero.DataObject.prototype._initErase = function (env) { }; Zotero.DataObject.prototype._finalizeErase = async function (env) { + await this.library.incrementClientVersion(); + // Delete versions from sync cache if (this._objectType != 'feedItem') { await Zotero.Sync.Data.Local.deleteCacheObjectVersions( @@ -1482,10 +1492,15 @@ Zotero.DataObject.prototype._finalizeErase = async function (env) { Zotero.DataObject.prototype.toResponseJSON = function (options = {}) { + // Default to showing synced properties, since that's what the API does, and this function + // is generally used to emulate the API + options.syncedStorageProperties ??= true; + options.syncedVersionProperty ??= true; + let uri = Zotero.URI.getObjectURI(this); var json = { key: this.key, - version: this.version, + version: options.syncedVersionProperty ? this.version : this.clientVersion, library: this.library.toResponseJSON({ ...options, includeGroupDetails: false }), links: { self: { @@ -1500,6 +1515,9 @@ Zotero.DataObject.prototype.toResponseJSON = function (options = {}) { meta: {}, data: this.toJSON(options) }; + // Keep the data block's version consistent with the top-level version (toJSON() reports + // the synced version, which differs from clientVersion when syncedVersionProperty is false) + json.data.version = json.version; if (options.version) { json.version = json.data.version = options.version; } diff --git a/chrome/content/zotero/xpcom/data/group.js b/chrome/content/zotero/xpcom/data/group.js index c5d824030a..a3119b0e2a 100644 --- a/chrome/content/zotero/xpcom/data/group.js +++ b/chrome/content/zotero/xpcom/data/group.js @@ -244,6 +244,9 @@ Zotero.Group.prototype.toResponseJSON = function (options = {}) { let uri = Zotero.URI.getGroupURI(this); return { id: this.id, + // Group metadata isn't locally writable, so group responses always report the + // synced group version -- unlike group.clientVersion, which is inherited from + // Zotero.Library and tracks the group library's contents version: this.version, links: { self: { diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index be609f3e09..029e0332c6 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -128,8 +128,8 @@ Zotero.defineProperty(Zotero.Item.prototype, 'itemID', { enumerable: false }); -for (let name of ['libraryID', 'key', 'dateAdded', 'dateModified', 'version', 'synced', - 'createdByUserID', 'lastModifiedByUserID']) { +for (let name of ['libraryID', 'key', 'dateAdded', 'dateModified', 'version', 'clientVersion', + 'synced', 'createdByUserID', 'lastModifiedByUserID']) { let prop = '_' + name; Zotero.defineProperty(Zotero.Item.prototype, name, { get: function () { return this[prop]; }, @@ -6128,12 +6128,6 @@ Zotero.Item.prototype.toJSON = function (options = {}) { Zotero.Item.prototype.toResponseJSON = function (options = {}) { - // Default to showing synced storage properties, since that's what the API does, and this function - // is generally used to emulate the API - if (options.syncedStorageProperties === undefined) { - options.syncedStorageProperties = true; - } - var json = this.constructor._super.prototype.toResponseJSON.call(this, options); // creatorSummary @@ -6164,6 +6158,15 @@ Zotero.Item.prototype.toResponseJSON = function (options = {}) { }; } + // When the caller wants the current storage properties from the file on disk + // (syncedStorageProperties: false), they're only available asynchronously, so add null + // placeholders here to keep their place in the JSON and let toResponseJSONAsync() fill + // them in + if (this.isStoredFileAttachment() && !options.skipStorageProperties && !options.syncedStorageProperties) { + json.data.mtime = null; + json.data.md5 = null; + } + return json; }; @@ -6200,6 +6203,12 @@ Zotero.Item.prototype.toResponseJSONAsync = async function (options = {}) { else if (this.isImportedAttachment()) { json.links.enclosure.length = await getFileSize(this); } + + if (this.isStoredFileAttachment() && !options.skipStorageProperties) { + json.data.mtime = await this.attachmentModificationTime ?? null; + json.data.md5 = await this.attachmentHash ?? null; + } + return json; }; diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index eb6c4035f6..dd35b89a53 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/chrome/content/zotero/xpcom/data/items.js @@ -48,6 +48,7 @@ Zotero.Items = function () { libraryID: "O.libraryID", key: "O.key", version: "O.version", + clientVersion: "O.clientVersion", synced: "O.synced", createdByUserID: "createdByUserID", diff --git a/chrome/content/zotero/xpcom/data/library.js b/chrome/content/zotero/xpcom/data/library.js index cd77f664be..2d4dd2af7b 100644 --- a/chrome/content/zotero/xpcom/data/library.js +++ b/chrome/content/zotero/xpcom/data/library.js @@ -39,6 +39,7 @@ Zotero.Library = function (params = {}) { this._storageDownloadNeeded = false; this._lastReadItemInSession = null; + this._lastClientVersionIncrementTransactionID = null; Zotero.Utilities.Internal.assignProps( this, @@ -48,6 +49,7 @@ Zotero.Library = function (params = {}) { 'editable', 'filesEditable', 'libraryVersion', + 'clientVersion', 'storageVersion', 'lastSync', 'archived' @@ -71,7 +73,7 @@ Zotero.Library = function (params = {}) { // DB columns Zotero.defineProperty(Zotero.Library, '_dbColumns', { value: Object.freeze([ - 'type', 'editable', 'filesEditable', 'version', 'storageVersion', 'lastSync', 'archived', 'isAdmin' + 'type', 'editable', 'filesEditable', 'version', 'clientVersion', 'storageVersion', 'lastSync', 'archived', 'isAdmin' ]) }); @@ -209,7 +211,7 @@ Zotero.defineProperty(Zotero.Library.prototype, 'allowsLinkedFiles', { // Create other accessors (function () { - let accessors = ['editable', 'filesEditable', 'storageVersion', 'archived', 'isAdmin']; + let accessors = ['editable', 'filesEditable', 'clientVersion', 'storageVersion', 'archived', 'isAdmin']; for (let i=0; i dataObject.version > since); + response.data = response.data.filter(dataObject => dataObject.clientVersion > since); } } @@ -225,9 +228,25 @@ class LocalAPIEndpoint { 'Total-Results': totalResults, 'Link': Object.entries(links).map(([rel, url]) => `<${url}>; rel="${rel}"`).join(', ') }; - let lastModifiedVersion = dataIsArray - ? Zotero.Libraries.get(requestData.libraryID).libraryVersion - : response.data.version; + let lastModifiedVersion; + // Unlike other multi-object responses, a group list has no meaningful + // Last-Modified-Version: each group has its own synced metadata version, and the + // user library version that would otherwise be returned says nothing about + // group metadata + if (this instanceof Zotero.Server.LocalAPI.Groups) { + lastModifiedVersion = undefined; + } + else if (dataIsArray) { + lastModifiedVersion = Zotero.Libraries.get(requestData.libraryID).clientVersion; + } + // Group metadata isn't locally writable, so single-group responses report the + // synced group version, matching the version fields in the response body + else if (response.data instanceof Zotero.Group) { + lastModifiedVersion = response.data.version; + } + else { + lastModifiedVersion = response.data.clientVersion; + } if (lastModifiedVersion !== undefined) { headers['Last-Modified-Version'] = lastModifiedVersion; } @@ -352,7 +371,13 @@ class LocalAPIEndpoint { return this.makeResponse(400, 'text/plain', 'Only multi-object requests can output versions'); } contentType = 'application/json'; - body = JSON.stringify(Object.fromEntries(dataObjectOrObjects.map(o => [o.key, o.version])), null, 4); + // Groups are keyed by id and report their synced metadata version, which is + // independent of the local content versions used for other object types + body = JSON.stringify(Object.fromEntries(dataObjectOrObjects.map( + o => (o instanceof Zotero.Group + ? [o.id, o.version] + : [o.key, o.clientVersion]) + )), null, 4); break; case 'json': case null: @@ -900,7 +925,9 @@ async function toResponseJSON(dataObjectOrObjects, searchParams) { let responseJSON = dataObject.toResponseJSONAsync ? await dataObject.toResponseJSONAsync({ apiURL: `http://localhost:${Zotero.Server.port}/api/`, - includeGroupDetails: true + includeGroupDetails: true, + syncedStorageProperties: false, + syncedVersionProperty: false, }) : dataObject; diff --git a/resource/schema/userdata.sql b/resource/schema/userdata.sql index c0d5724a31..86645bc2fa 100644 --- a/resource/schema/userdata.sql +++ b/resource/schema/userdata.sql @@ -1,4 +1,4 @@ --- 128 +-- 129 -- Copyright (c) 2009 Center for History and New Media -- George Mason University, Fairfax, Virginia, USA @@ -164,6 +164,7 @@ CREATE TABLE items ( libraryID INT NOT NULL, key TEXT NOT NULL, version INT NOT NULL DEFAULT 0, + clientVersion INT NOT NULL DEFAULT 0, synced INT NOT NULL DEFAULT 0, UNIQUE (libraryID, key), FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE @@ -301,6 +302,7 @@ CREATE TABLE collections ( libraryID INT NOT NULL, key TEXT NOT NULL, version INT NOT NULL DEFAULT 0, + clientVersion INT NOT NULL DEFAULT 0, synced INT NOT NULL DEFAULT 0, UNIQUE (libraryID, key), FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE, @@ -357,6 +359,7 @@ CREATE TABLE savedSearches ( libraryID INT NOT NULL, key TEXT NOT NULL, version INT NOT NULL DEFAULT 0, + clientVersion INT NOT NULL DEFAULT 0, synced INT NOT NULL DEFAULT 0, UNIQUE (libraryID, key), FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE @@ -400,6 +403,7 @@ CREATE TABLE libraries ( editable INT NOT NULL, filesEditable INT NOT NULL, version INT NOT NULL DEFAULT 0, + clientVersion INT NOT NULL DEFAULT 0, storageVersion INT NOT NULL DEFAULT 0, lastSync INT NOT NULL DEFAULT 0, archived INT NOT NULL DEFAULT 0, diff --git a/test/content/support.js b/test/content/support.js index 547d803dd4..152ae02d52 100644 --- a/test/content/support.js +++ b/test/content/support.js @@ -650,7 +650,7 @@ var modifyDataObject = function (obj, params = {}, saveOptions) { default: obj.name = params.name !== undefined ? params.name : Zotero.Utilities.randomString(); } - return obj.saveTx(saveOptions); + return obj.save({ tx: true, ...saveOptions }); }; /** diff --git a/test/tests/dataObjectTest.js b/test/tests/dataObjectTest.js index 9aa9f21b1a..2837a666b6 100644 --- a/test/tests/dataObjectTest.js +++ b/test/tests/dataObjectTest.js @@ -52,6 +52,70 @@ describe("Zotero.DataObject", function () { }) }) + describe("#clientVersion", function () { + it("should be set to library clientVersion after creating object", async function () { + for (let type of types) { + let obj = await createDataObject(type); + assert.equal(obj.clientVersion, obj.library.clientVersion); + await obj.eraseTx(); + } + }); + + it("should increase after modifying object", async function () { + for (let type of types) { + let obj = await createDataObject(type); + let clientVersion = obj.clientVersion; + + await modifyDataObject(obj); + + assert.isAbove(obj.clientVersion, clientVersion); + assert.equal(obj.clientVersion, obj.library.clientVersion); + + await obj.eraseTx(); + } + }); + + it("should increase once per library per transaction", async function () { + for (let type of types) { + let obj1 = await createDataObject(type); + let obj2 = await createDataObject(type); + + let group = await getGroup(); + let obj3 = await createDataObject(type, { libraryID: group.libraryID }); + + assert.isBelow(obj1.clientVersion, obj2.clientVersion); + assert.equal(obj2.clientVersion, Zotero.Libraries.userLibrary.clientVersion); + + await modifyDataObject(obj1); + assert.isAbove(obj1.clientVersion, obj2.clientVersion); + assert.equal(obj1.clientVersion, Zotero.Libraries.userLibrary.clientVersion); + + let libraryVersionBefore = Zotero.Libraries.userLibrary.clientVersion; + let groupVersionBefore = group.clientVersion; + await Zotero.DB.executeTransaction(async () => { + await modifyDataObject(obj1, undefined, { tx: false }); + await modifyDataObject(obj2, undefined, { tx: false }); + await modifyDataObject(obj3, undefined, { tx: false }); + }); + assert.equal(obj1.clientVersion, Zotero.Libraries.userLibrary.clientVersion); + assert.equal(obj1.clientVersion, obj2.clientVersion); + assert.notEqual(obj1.clientVersion, libraryVersionBefore); + + assert.equal(obj3.clientVersion, group.clientVersion); + assert.notEqual(obj3.clientVersion, groupVersionBefore); + } + }); + + it("should increment library clientVersion when object is deleted", async function () { + for (let type of types) { + let obj = await createDataObject(type); + let libraryVersion = obj.library.clientVersion; + await obj.eraseTx(); + assert.equal(obj.library.clientVersion, libraryVersion + 1); + } + }); + }); + describe("#synced", function () { it("should be set to false after creating object", async function () { for (let type of types) { diff --git a/test/tests/libraryTest.js b/test/tests/libraryTest.js index 872ed00f92..5edf54c7f8 100644 --- a/test/tests/libraryTest.js +++ b/test/tests/libraryTest.js @@ -63,6 +63,27 @@ describe("Zotero.Library", function () { }); }); + describe("#clientVersion", function () { + it("should be settable to increasing values", function () { + let library = new Zotero.Library(); + assert.throws(() => library.clientVersion = -2); + assert.throws(() => library.clientVersion = "a"); + assert.throws(() => library.clientVersion = 1.1); + assert.doesNotThrow(() => library.clientVersion = 0); + assert.doesNotThrow(() => library.clientVersion = 5); + }); + it("should not be possible to decrement", function () { + let library = new Zotero.Library(); + library.clientVersion = 5; + assert.throws(() => library.clientVersion = 0); + }); + it("should not be possible to set to -1", function () { + let library = new Zotero.Library(); + library.clientVersion = 5; + assert.throws(() => library.clientVersion = -1); + }); + }); + describe("#editable", function () { it("should return editable status", function () { let library = Zotero.Libraries.get(Zotero.Libraries.userLibraryID); diff --git a/test/tests/server_localAPITest.js b/test/tests/server_localAPITest.js index 39a16e67ce..b7ccbf3bf9 100644 --- a/test/tests/server_localAPITest.js +++ b/test/tests/server_localAPITest.js @@ -272,9 +272,10 @@ describe("Local API Server", function () { }); describe("=versions", function () { - it("should output a JSON object mapping keys to versions", async function () { + it("should output a JSON object mapping keys to local versions", async function () { let { response } = await apiGet('/users/0/items?format=versions'); - assert.propertyVal(response, collectionItem1.key, collectionItem1.version); + assert.isAbove(collectionItem1.clientVersion, 0); + assert.propertyVal(response, collectionItem1.key, collectionItem1.clientVersion); }); }); }); @@ -325,11 +326,25 @@ describe("Local API Server", function () { describe("?since", function () { it("should filter the results", async function () { - let { response: response1 } = await apiGet('/users/0/items?since=' + (Zotero.Libraries.userLibrary.libraryVersion + 1)); + let version = Zotero.Libraries.userLibrary.clientVersion; + + let { response: response1 } = await apiGet('/users/0/items?since=' + version); assert.isEmpty(response1); let { response: response2 } = await apiGet('/users/0/items?since=0'); assert.lengthOf(response2, allItems.length); + + let tempItem = await createDataObject('item'); + let { response: response3 } = await apiGet('/users/0/items?since=' + version); + assert.lengthOf(response3, 1); + assert.equal(response3[0].key, tempItem.key); + assert.equal(response3[0].version, tempItem.clientVersion); + assert.equal(tempItem.clientVersion, version + 1); + + await tempItem.eraseTx(); + + let { response: response4 } = await apiGet('/users/0/items?since=' + version); + assert.lengthOf(response4, 0); }); }); @@ -364,7 +379,45 @@ describe("Local API Server", function () { }); }); + describe("/users//groups", function () { + it("should omit Last-Modified-Version", async function () { + let group = await getGroup(); + let xhr = await apiGet('/users/0/groups'); + assert.isNull(xhr.getResponseHeader('Last-Modified-Version')); + assert.include(xhr.response.map(g => g.id), group.id); + }); + + it("should ignore ?since", async function () { + let group = await getGroup(); + let { response: all } = await apiGet('/users/0/groups'); + let { response: since } = await apiGet(`/users/0/groups?since=${group.version + 1}`); + assert.sameDeepMembers(since, all); + }); + + it("should map group IDs to metadata versions with ?format=versions", async function () { + let group = await getGroup(); + let { response } = await apiGet('/users/0/groups?format=versions'); + assert.propertyVal(response, String(group.id), group.version); + }); + }); + describe("/groups/", function () { + it("should report the synced group version at the top level, in data, and in Last-Modified-Version", async function () { + let group = await getGroup(); + // Changing the group library's contents doesn't affect the group metadata version + let item = await createDataObject('item', { libraryID: group.libraryID }); + let xhr = await apiGet(`/groups/${group.groupID}`); + let response = xhr.response; + assert.notEqual(group.version, group.clientVersion); + assert.equal(response.version, group.version); + assert.equal(response.data.version, group.version); + assert.equal( + parseInt(xhr.getResponseHeader('Last-Modified-Version')), + group.version + ); + await item.eraseTx(); + }); + it("should return 404 for unknown group", async function () { let { response } = await apiGet( '/groups/99999999999',