mirrors/zotero
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions

Sort multiple levels of items when generating API JSON

Browse source 
Added Zotero.DataObjects.sortByParent() to sort child items immediately
after their parent items. Zotero.DataObjects.sortByLevel(), which is
used for collections, sorts each level together, but that's less
appropriate for items where, e.g., an embedded-image attachment should
immediately follow the note that depends on it.
This commit is contained in:
Dan Stillman 2020-09-08 04:02:25 -04:00
parent f662b58331
commit a94323fc15
8 changed files with 362 additions and 158 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

75
chrome/content/zotero/xpcom/data/collections.js
View file

@ -156,81 +156,6 @@ Zotero.Collections = function() {
}
/**
* Sort an array of collectionIDs from top-level to deepest
*
* Order within each level is undefined.
*
* This is used to sort higher-level collections first in upload JSON, since otherwise the API
* would reject lower-level collections for having missing parents.
*/
this.sortByLevel = function (ids) {
let levels = {};
// Get objects from ids
let objs = {};
ids.forEach(id => objs[id] = Zotero.Collections.get(id));
// Get top-level collections
let top = ids.filter(id => !objs[id].parentID);
levels["0"] = top.slice();
ids = Zotero.Utilities.arrayDiff(ids, top);
// For each collection in list, walk up its parent tree. If a parent is present in the
// list of ids, add it to the appropriate level bucket and remove it.
while (ids.length) {
let tree = [ids[0]];
let keep = [ids[0]];
let id = ids.shift();
let seen = new Set([id]);
while (true) {
let c = Zotero.Collections.get(id);
let parentID = c.parentID;
if (!parentID) {
break;
}
// Avoid an infinite loop if collections are incorrectly nested within each other
if (seen.has(parentID)) {
throw new Zotero.Error(
"Incorrectly nested collections",
Zotero.Error.ERROR_INVALID_COLLECTION_NESTING,
{
collectionID: id
}
);
}
seen.add(parentID);
tree.push(parentID);
// If parent is in list, remove it
let pos = ids.indexOf(parentID);
if (pos != -1) {
keep.push(parentID);
ids.splice(pos, 1);
}
id = parentID;
}
let level = tree.length - 1;
for (let i = 0; i < tree.length; i++) {
let currentLevel = level - i;
for (let j = 0; j < keep.length; j++) {
if (tree[i] != keep[j]) continue;
if (!levels[currentLevel]) {
levels[currentLevel] = [];
}
levels[currentLevel].push(keep[j]);
}
}
}
var orderedIDs = [];
for (let level in levels) {
orderedIDs = orderedIDs.concat(levels[level]);
}
return orderedIDs;
};
this._loadChildCollections = Zotero.Promise.coroutine(function* (libraryID, ids, idSQL) {
var sql = "SELECT C1.collectionID, C2.collectionID AS childCollectionID "
+ "FROM collections C1 LEFT JOIN collections C2 ON (C1.collectionID=C2.parentCollectionID) "

144
chrome/content/zotero/xpcom/data/dataObjects.js
View file

@ -711,6 +711,150 @@ Zotero.DataObjects.prototype._loadRelations = Zotero.Promise.coroutine(function*
});
/**
* Sort an array of collections or items from top-level to deepest, grouped by level
*
* All top-level objects are returned, followed by all second-level objects, followed by
* third-level, etc. The order within each level is undefined.
*
* This is used to sort higher-level objects first in upload JSON, since otherwise the API would
* reject lower-level objects for having missing parents.
*
* @param {Zotero.DataObject[]} objects - An array of objects
* @return {Zotero.DataObject[]} - A sorted array of objects
*/
Zotero.DataObjects.prototype.sortByLevel = function (objects) {
// Convert to ids
var ids = objects.map(o => o.id);
var levels = {};
// Get top-level objects
var top = objects.filter(o => !o.parentID).map(o => o.id);
levels["0"] = top.slice();
ids = Zotero.Utilities.arrayDiff(ids, top);
// For each object in list, walk up its parent tree. If a parent is present in the
// list of ids, add it to the appropriate level bucket and remove it.
while (ids.length) {
let tree = [ids[0]];
let keep = [ids[0]];
let id = ids.shift();
let seen = new Set([id]);
while (true) {
let o = Zotero[this._ZDO_Objects].get(id);
let parentID = o.parentID;
if (!parentID) {
break;
}
// Avoid an infinite loop if objects are incorrectly nested within each other
if (seen.has(parentID)) {
throw new Zotero.Error(
`Incorrectly nested ${this._ZDO_objects}`,
Zotero.Error.ERROR_INVALID_OBJECT_NESTING,
{
[this._ZDO_id]: id
}
);
}
seen.add(parentID);
tree.push(parentID);
// If parent is in list, remove it
let pos = ids.indexOf(parentID);
if (pos != -1) {
keep.push(parentID);
ids.splice(pos, 1);
}
id = parentID;
}
let level = tree.length - 1;
for (let i = 0; i < tree.length; i++) {
let currentLevel = level - i;
for (let j = 0; j < keep.length; j++) {
if (tree[i] != keep[j]) continue;
if (!levels[currentLevel]) {
levels[currentLevel] = [];
}
levels[currentLevel].push(keep[j]);
}
}
}
var ordered = [];
for (let level in levels) {
ordered = ordered.concat(levels[level]);
}
// Convert back to objects
return ordered.map(id => Zotero[this._ZDO_Objects].get(id));
};
/**
* Sort an array of collections or items from top-level to deepest, grouped by parent
*
* Child objects are included before any sibling objects. The order within each level is undefined.
*
* This is used to sort higher-level objects first in upload JSON, since otherwise the API would
* reject lower-level objects for having missing parents.
*
* @param {Zotero.DataObject[]} ids - An array of data objects
* @return {Zotero.DataObject[]} - A sorted array of data objects
*/
Zotero.DataObjects.prototype.sortByParent = function (objects) {
// Convert to ids
var ids = objects.map(o => o.id);
var ordered = [];
// For each object in list, walk up its parent tree. If a parent is present in the list of
// objects, keep track of it and remove it from the list. When we get to a top-level object, add
// all the objects we've kept to the ordered list.
while (ids.length) {
let id = ids.shift();
let keep = [id];
let seen = new Set([id]);
while (true) {
let o = Zotero[this._ZDO_Objects].get(id);
let parentID = o.parentID;
if (!parentID) {
// We've reached a top-level object, so add any kept ids to the list
ordered.push(...keep);
break;
}
// Avoid an infinite loop if objects are incorrectly nested within each other
if (seen.has(parentID)) {
throw new Zotero.Error(
`Incorrectly nested ${this._ZDO_objects}`,
Zotero.Error.ERROR_INVALID_OBJECT_NESTING,
{
[this._ZDO_id]: id
}
);
}
seen.add(parentID);
// If parent is in list of ids, keep it and remove it from list
let pos = ids.indexOf(parentID);
if (pos != -1) {
keep.unshift(parentID);
ids.splice(pos, 1);
}
// Otherwise, check if parent has already been added to the ordered list, in which case
// we can slot in all kept ids after it
else {
pos = ordered.indexOf(parentID);
if (pos != -1) {
ordered.splice(pos + 1, 0, ...keep);
break;
}
}
id = parentID;
}
}
// Convert back to objects
return ordered.map(id => Zotero[this._ZDO_Objects].get(id));
}
/**
* Flatten API JSON relations object into an array of unique predicate-object pairs
*

2
chrome/content/zotero/xpcom/error.js
View file

@ -51,7 +51,7 @@ Zotero.Error.ERROR_ZFS_UPLOAD_QUEUE_LIMIT = 6;
Zotero.Error.ERROR_ZFS_FILE_EDITING_DENIED = 7;
Zotero.Error.ERROR_INVALID_ITEM_TYPE = 8;
Zotero.Error.ERROR_USER_NOT_AVAILABLE = 9;
Zotero.Error.ERROR_INVALID_COLLECTION_NESTING = 10;
Zotero.Error.ERROR_INVALID_OBJECT_NESTING = 10;
//Zotero.Error.ERROR_SYNC_EMPTY_RESPONSE_FROM_SERVER = 6;
//Zotero.Error.ERROR_SYNC_INVALID_RESPONSE_FROM_SERVER = 7;

7
chrome/content/zotero/xpcom/sync/syncLocal.js
View file

@ -556,12 +556,12 @@ Zotero.Sync.Data.Local = {
// Sort descendent collections last
if (objectType == 'collection') {
try {
ids = Zotero.Collections.sortByLevel(ids);
ids = Zotero.Collections.sortByLevel(ids.map(id => Zotero.Collections.get(id))).map(o => o.id);
}
catch (e) {
Zotero.logError(e);
// If collections were incorrectly nested, fix and try again
if (e instanceof Zotero.Error && e.error == Zotero.Error.ERROR_INVALID_COLLECTION_NESTING) {
if (e instanceof Zotero.Error && e.error == Zotero.Error.ERROR_INVALID_OBJECT_NESTING) {
let c = Zotero.Collections.get(e.collectionID);
Zotero.debug(`Removing parent collection ${c.parentKey} from collection ${c.key}`);
c.parentID = null;
@ -573,6 +573,9 @@ Zotero.Sync.Data.Local = {
}
}
}
else if (objectType == 'item') {
ids = Zotero.Items.sortByParent(ids.map(id => Zotero.Items.get(id))).map(o => o.id);
}
return ids;
}),

36
test/content/support.js
View file

@ -907,7 +907,8 @@ function importFileAttachment(filename, options = {}) {
filename.split('/').forEach((part) => file.append(part));
let importOptions = {
file,
parentItemID: options.parentID
parentItemID: options.parentID,
title: options.title
};
Object.assign(importOptions, options);
return Zotero.Attachments.importFromFile(importOptions);
@ -924,7 +925,20 @@ function importHTMLAttachment() {
}
async function createAnnotation(type, parentItem) {
async function importPDFAttachment(parentItem, options = {}) {
var attachment = await importFileAttachment(
'test.pdf',
{
contentType: 'application/pdf',
parentID: parentItem ? parentItem.id : null,
title: options.title
}
);
return attachment;
}
async function createAnnotation(type, parentItem, options = {}) {
var annotation = new Zotero.Item('annotation');
annotation.parentID = parentItem.id;
annotation.annotationType = type;
@ -941,11 +955,29 @@ async function createAnnotation(type, parentItem) {
[314.4, 412.8, 556.2, 609.6]
]
};
if (options.tags) {
annotation.setTags(options.tags);
}
await annotation.saveTx();
return annotation;
}
async function createEmbeddedImage(parentItem, options = {}) {
var attachment = await Zotero.Attachments.importEmbeddedImage({
blob: await File.createFromFileName(
OS.Path.join(getTestDataDirectory().path, 'test.png')
),
parentItemID: parentItem.id
});
if (options.tags) {
attachment.setTags(options.tags);
await attachment.saveTx();
}
return attachment;
}
/**
* Sets the fake XHR server to response to a given response
*

44
test/tests/collectionsTest.js
View file

@ -83,48 +83,4 @@ describe("Zotero.Collections", function () {
assert.notInstanceOf(collection, Zotero.Feed);
});
});
describe("#sortByLevel()", function () {
it("should return collections sorted from top-level to deepest", function* () {
// - A
// - B
// - C
// - D
// - E
// - F
// - G
// - H
// - I
// Leave out B and G
// Order should be {A, E}, {D, F}, {C, I}, {H} (internal order is undefined)
var check = function (arr) {
assert.sameMembers(arr.slice(0, 2), [c1.id, c5.id]);
assert.sameMembers(arr.slice(2, 4), [c4.id, c6.id]);
assert.sameMembers(arr.slice(4, 6), [c3.id, c9.id]);
assert.equal(arr[6], c8.id);
};
var c1 = yield createDataObject('collection', { "name": "A" });
var c2 = yield createDataObject('collection', { "name": "B", parentID: c1.id });
var c3 = yield createDataObject('collection', { "name": "C", parentID: c2.id });
var c4 = yield createDataObject('collection', { "name": "D", parentID: c1.id });
var c5 = yield createDataObject('collection', { "name": "E" });
var c6 = yield createDataObject('collection', { "name": "F", parentID: c5.id });
var c7 = yield createDataObject('collection', { "name": "G", parentID: c6.id });
var c8 = yield createDataObject('collection', { "name": "H", parentID: c7.id });
var c9 = yield createDataObject('collection', { "name": "I", parentID: c6.id });
var arr = Zotero.Collections.sortByLevel([c1, c3, c4, c5, c6, c8, c9].map(c => c.id));
//Zotero.debug(arr.map(id => Zotero.Collections.get(id).name));
check(arr);
// Check reverse order
arr = Zotero.Collections.sortByLevel([c1, c3, c4, c5, c6, c8, c9].reverse().map(c => c.id));
//Zotero.debug(arr.map(id => Zotero.Collections.get(id).name));
check(arr);
});
});
})

122
test/tests/dataObjectsTest.js
View file

@ -71,6 +71,128 @@ describe("Zotero.DataObjects", function () {
})
})
describe("#sortByLevel()", function () {
it("should return collections sorted from top-level to deepest", async function () {
// - A
// - B
// - C
// - D
// - E
// - F
// - G
// - H
// - I
//
// Leave out B and G
//
// Order should be {A, E}, {D, F}, {C, I}, {H} (internal order is undefined)
var check = function (arr) {
assert.sameMembers(arr.slice(0, 2), [c1, c5]);
assert.sameMembers(arr.slice(2, 4), [c4, c6]);
assert.sameMembers(arr.slice(4, 6), [c3, c9]);
assert.equal(arr[6], c8);
};
var c1 = await createDataObject('collection', { "name": "A" });
var c2 = await createDataObject('collection', { "name": "B", parentID: c1.id });
var c3 = await createDataObject('collection', { "name": "C", parentID: c2.id });
var c4 = await createDataObject('collection', { "name": "D", parentID: c1.id });
var c5 = await createDataObject('collection', { "name": "E" });
var c6 = await createDataObject('collection', { "name": "F", parentID: c5.id });
var c7 = await createDataObject('collection', { "name": "G", parentID: c6.id });
var c8 = await createDataObject('collection', { "name": "H", parentID: c7.id });
var c9 = await createDataObject('collection', { "name": "I", parentID: c6.id });
var arr = Zotero.Collections.sortByLevel([c1, c3, c4, c5, c6, c8, c9]);
//Zotero.debug(arr.map(id => Zotero.Collections.get(id).name));
check(arr);
// Check reverse order
arr = Zotero.Collections.sortByLevel([c1, c3, c4, c5, c6, c8, c9].reverse());
//Zotero.debug(arr.map(id => Zotero.Collections.get(id).name));
check(arr);
});
});
describe("#sortByParent", function () {
it("should return items sorted hierarchically", async function () {
// - A
// - B
// - C
// - D
// - E
// - F
// - G
// - H
// - I
//
// Leave out B and G
//
// Order should be top-down, with child items included immediately after their parents.
// The order of items at the same level is undefined.
function check(arr) {
var str = arr.map(o => title(o)).join('');
var possibilities = [
'ACDEFHI',
'ACDEFIH',
'ADCEFHI',
'ADCEFIH',
'EFHIACD',
'EFHIADC',
'EFIHACD',
'EFIHADC',
];
assert.oneOf(str, possibilities);
}
function title(o) {
return o.getDisplayTitle() || o.getTags()[0].tag;
}
var a = await createDataObject('item', { title: "A" });
var b = await createDataObject('item', { note: "B", itemType: 'note', parentID: a.id });
var c = await createEmbeddedImage(b, { tags: [{ tag: 'C' }] });
var d = await importPDFAttachment(a, { title: 'D' });
var e = await createDataObject('item', { title: "E" });
var f = await importPDFAttachment(e, { title: 'F' });
var g = await createAnnotation('image', f, { tags: [{ tag: 'G' }] });
var h = await createEmbeddedImage(g, { tags: [{ tag: 'H' }] });
var i = await createAnnotation('highlight', f, { tags: [{ tag: 'I' }] });
var arr = Zotero.Items.sortByParent([a, c, d, e, f, h, i]);
Zotero.debug(arr.map(o => title(o)));
check(arr);
// Reverse order
arr = Zotero.Items.sortByParent([a, c, d, e, f, h, i].reverse());
Zotero.debug(arr.map(o => title(o)));
check(arr);
// Top-level first
arr = Zotero.Items.sortByParent([a, e, c, d, f, h, i]);
Zotero.debug(arr.map(o => title(o)));
check(arr);
// Child first
arr = Zotero.Items.sortByParent([c, h, d, i, f, a, e]);
Zotero.debug(arr.map(o => title(o)));
check(arr);
// Random
arr = Zotero.Items.sortByParent([i, e, d, h, c, a, f]);
Zotero.debug(arr.map(o => title(o)));
check(arr);
});
});
describe("#_setIdentifier", function () {
it("should not allow an id change", function* () {
var item = yield createDataObject('item');

90
test/tests/syncEngineTest.js
View file

@ -746,35 +746,58 @@ describe("Zotero.Sync.Data.Engine", function () {
})
it("should upload child item after parent item", function* () {
({ engine, client, caller } = yield setup());
it("should upload child items after parent items", async function () {
({ engine, client, caller } = await setup());
var library = Zotero.Libraries.userLibrary;
var lastLibraryVersion = 5;
library.libraryVersion = lastLibraryVersion;
yield library.saveTx();
await library.saveTx();
// Create top-level note, book, and child note
var item1 = new Zotero.Item('note');
item1.setNote('A');
yield item1.saveTx();
var item2 = yield createDataObject('item');
var item3 = new Zotero.Item('note');
item3.parentItemID = item2.id;
item3.setNote('B');
yield item3.saveTx();
// Create top-level note, embedded-image attachment, book, and child note
var note1 = await createDataObject('item', { itemType: 'note', note: 'A' });
var attachment = await Zotero.Attachments.importEmbeddedImage({
blob: await File.createFromFileName(
OS.Path.join(getTestDataDirectory().path, 'test.png')
),
parentItemID: note1.id
});
var item = await createDataObject('item');
var note2 = await createDataObject('item', { itemType: 'note', parentID: item.id, note: 'B' });
// Move note under parent
item1.parentItemID = item2.id;
yield item1.saveTx();
note1.parentItemID = item.id;
await note1.saveTx();
var handled = false;
server.respond(function (req) {
if (req.method == "POST" && req.url == baseURL + "users/1/items") {
let json = JSON.parse(req.requestBody);
assert.lengthOf(json, 3);
assert.equal(json[0].key, item2.key);
assert.equal(json[1].key, item1.key);
assert.equal(json[2].key, item3.key);
assert.lengthOf(json, 4);
assert.equal(json[0].key, item.key);
assert.oneOf(
[json[1].key, json[2].key, json[3].key].join(''),
[
[note1.key, attachment.key, note2.key].join(''),
[note2.key, note1.key, attachment.key].join(''),
]
);
let successful;
if (json[1].key == note1.key) {
successful = {
"0": item.toResponseJSON({ version: lastLibraryVersion }),
"1": note1.toResponseJSON({ version: lastLibraryVersion }),
"2": attachment.toResponseJSON({ version: lastLibraryVersion }),
"3": note2.toResponseJSON({ version: lastLibraryVersion }),
};
}
else {
successful = {
"0": item.toResponseJSON({ version: lastLibraryVersion }),
"1": note2.toResponseJSON({ version: lastLibraryVersion }),
"2": note1.toResponseJSON({ version: lastLibraryVersion }),
"3": attachment.toResponseJSON({ version: lastLibraryVersion }),
};
}
handled = true;
req.respond(
200,
@ -783,11 +806,7 @@ describe("Zotero.Sync.Data.Engine", function () {
"Last-Modified-Version": ++lastLibraryVersion
},
JSON.stringify({
successful: {
"0": item2.toResponseJSON({ version: lastLibraryVersion }),
"1": item1.toResponseJSON({ version: lastLibraryVersion }),
"2": item3.toResponseJSON({ version: lastLibraryVersion })
},
successful,
unchanged: {},
failed: {}
})
@ -796,7 +815,7 @@ describe("Zotero.Sync.Data.Engine", function () {
}
});
yield engine.start();
await engine.start();
assert.isTrue(handled);
});
@ -4633,29 +4652,32 @@ describe("Zotero.Sync.Data.Engine", function () {
);
let version = data.expectedVersion + 1;
let json = JSON.parse(req.requestBody);
let o1 = json.find(o => o.key == objectJSON[type][1].key);
assert.notProperty(o1, 'version');
let o2 = json.find(o => o.key == objectJSON[type][2].key);
assert.notProperty(o2, 'version');
let o3 = json.find(o => o.key == objectJSON[type][3].key);
assert.notProperty(o3, 'version');
let response = {
successful: {
"0": Object.assign(objectJSON[type][1], { version }),
"1": Object.assign(objectJSON[type][2], { version }),
"2": Object.assign(objectJSON[type][3], { version })
},
unchanged: {},
failed: {}
};
if (type == 'item') {
let o = json.find(o => o.key == objectJSON.item[4].key);
assert.notProperty(o, 'version');
// Attachment items should include storage properties
assert.propertyVal(o, 'mtime', objects.item[4].attachmentSyncedModificationTime);
assert.propertyVal(o, 'md5', objects.item[4].attachmentSyncedHash);
response.successful["3"] = Object.assign(objectJSON[type][4], { version })
}
let response = {
successful: {},
unchanged: {},
failed: {}
};
// Return objects in the order provided
json.map(x => x.key).forEach((key, index) => {
response.successful[index] = Object.assign(
objectJSON[type].find(x => x.key == key),
{ version }
);
});
req.respond(
200,
{