commit 80f888f37426059875b25e45641939d8e45d84c7
parent 60ed6d447e63976de94cb4400ebf46a268089b57
Author: Dan Stillman <dstillman@zotero.org>
Date: Mon, 12 Dec 2016 03:26:35 -0500
Fix replacement of existing item storage directories
- Make Zotero.Attachments.createDirectoryForItem() delete existing
directory instead of moving it to orphaned-files; also now returns a
string path instead of an nsIFile
- Use above function during file sync instead of
_deleteExistingAttachmentFiles(), which was partly broken
- Fix throwing on errors when saving some attachment types
Diffstat:
6 files changed, 133 insertions(+), 185 deletions(-)
diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js
@@ -61,62 +61,61 @@ Zotero.Attachments = new function(){
}
var attachmentItem, itemID, newFile, contentType;
- return Zotero.DB.executeTransaction(function* () {
- // Create a new attachment
- attachmentItem = new Zotero.Item('attachment');
- if (parentItemID) {
- let {libraryID: parentLibraryID, key: parentKey} =
- Zotero.Items.getLibraryAndKeyFromID(parentItemID);
- attachmentItem.libraryID = parentLibraryID;
- }
- else if (libraryID) {
- attachmentItem.libraryID = libraryID;
- }
- attachmentItem.setField('title', newName);
- attachmentItem.parentID = parentItemID;
- attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE;
- if (collections) {
- attachmentItem.setCollections(collections);
- }
- yield attachmentItem.save(saveOptions);
-
- // Create directory for attachment files within storage directory
- destDir = yield this.createDirectoryForItem(attachmentItem);
-
- // Point to copied file
- newFile = destDir.clone();
- newFile.append(newName);
-
- // Copy file to unique filename, which automatically shortens long filenames
- newFile = Zotero.File.copyToUnique(file, newFile);
-
- contentType = yield Zotero.MIME.getMIMETypeFromFile(newFile);
-
- attachmentItem.attachmentContentType = contentType;
- attachmentItem.attachmentPath = newFile.path;
- yield attachmentItem.save(saveOptions);
- }.bind(this))
- .then(function () {
- return _postProcessFile(attachmentItem, newFile, contentType);
- })
- .catch(function (e) {
+ try {
+ yield Zotero.DB.executeTransaction(function* () {
+ // Create a new attachment
+ attachmentItem = new Zotero.Item('attachment');
+ if (parentItemID) {
+ let {libraryID: parentLibraryID, key: parentKey} =
+ Zotero.Items.getLibraryAndKeyFromID(parentItemID);
+ attachmentItem.libraryID = parentLibraryID;
+ }
+ else if (libraryID) {
+ attachmentItem.libraryID = libraryID;
+ }
+ attachmentItem.setField('title', newName);
+ attachmentItem.parentID = parentItemID;
+ attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE;
+ if (collections) {
+ attachmentItem.setCollections(collections);
+ }
+ yield attachmentItem.save(saveOptions);
+
+ // Create directory for attachment files within storage directory
+ destDir = yield this.createDirectoryForItem(attachmentItem);
+
+ // Point to copied file
+ newFile = OS.Path.join(destDir, newName);
+
+ // Copy file to unique filename, which automatically shortens long filenames
+ newFile = Zotero.File.copyToUnique(file, newFile);
+
+ contentType = yield Zotero.MIME.getMIMETypeFromFile(newFile);
+
+ attachmentItem.attachmentContentType = contentType;
+ attachmentItem.attachmentPath = newFile.path;
+ yield attachmentItem.save(saveOptions);
+ }.bind(this));
+ yield _postProcessFile(attachmentItem, newFile, contentType);
+ }
+ catch (e) {
Zotero.logError(e);
- var msg = "Failed importing file " + file.path;
- Zotero.logError(msg);
+ Zotero.logError("Failed importing file " + file.path);
// Clean up
try {
- if (destDir && destDir.exists()) {
- destDir.remove(true);
+ if (destDir && (yield OS.File.exists(destDir))) {
+ yield OS.File.removeDir(destDir);
}
}
catch (e) {
Zotero.logError(e);
}
- }.bind(this))
- .then(function () {
- return attachmentItem;
- });
+
+ throw e;
+ }
+
+ return attachmentItem;
});
@@ -174,39 +173,39 @@ Zotero.Attachments = new function(){
}
var attachmentItem, itemID, destDir, newFile;
- return Zotero.DB.executeTransaction(function* () {
- // Create a new attachment
- attachmentItem = new Zotero.Item('attachment');
- let {libraryID, key: parentKey} = Zotero.Items.getLibraryAndKeyFromID(parentItemID);
- attachmentItem.libraryID = libraryID;
- attachmentItem.setField('title', title);
- attachmentItem.setField('url', url);
- attachmentItem.parentID = parentItemID;
- attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL;
- attachmentItem.attachmentContentType = contentType;
- attachmentItem.attachmentCharset = charset;
-
- // DEBUG: this should probably insert access date too so as to
- // create a proper item, but at the moment this is only called by
- // translate.js, which sets the metadata fields itself
- itemID = yield attachmentItem.save();
-
- var storageDir = Zotero.getStorageDirectory();
- destDir = this.getStorageDirectory(attachmentItem);
- yield _moveOrphanedDirectory(destDir);
- file.parent.copyTo(storageDir, destDir.leafName);
-
- // Point to copied file
- newFile = destDir.clone();
- newFile.append(file.leafName);
-
- attachmentItem.attachmentPath = newFile.path;
- yield attachmentItem.save();
- }.bind(this))
- .then(function () {
- return _postProcessFile(attachmentItem, newFile, contentType, charset);
- })
- .catch(function (e) {
+ try {
+ yield Zotero.DB.executeTransaction(function* () {
+ // Create a new attachment
+ attachmentItem = new Zotero.Item('attachment');
+ let {libraryID, key: parentKey} = Zotero.Items.getLibraryAndKeyFromID(parentItemID);
+ attachmentItem.libraryID = libraryID;
+ attachmentItem.setField('title', title);
+ attachmentItem.setField('url', url);
+ attachmentItem.parentID = parentItemID;
+ attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL;
+ attachmentItem.attachmentContentType = contentType;
+ attachmentItem.attachmentCharset = charset;
+
+ // DEBUG: this should probably insert access date too so as to
+ // create a proper item, but at the moment this is only called by
+ // translate.js, which sets the metadata fields itself
+ itemID = yield attachmentItem.save();
+
+ var storageDir = Zotero.getStorageDirectory();
+ destDir = this.getStorageDirectory(attachmentItem);
+ yield OS.File.removeDir(destDir.path);
+ file.parent.copyTo(storageDir, destDir.leafName);
+
+ // Point to copied file
+ newFile = destDir.clone();
+ newFile.append(file.leafName);
+
+ attachmentItem.attachmentPath = newFile.path;
+ yield attachmentItem.save();
+ }.bind(this));
+ yield _postProcessFile(attachmentItem, newFile, contentType, charset);
+ }
+ catch (e) {
Zotero.logError(e);
// Clean up
@@ -218,10 +217,10 @@ Zotero.Attachments = new function(){
catch (e) {
Zotero.logError(e, 1);
}
- }.bind(this))
- .then(function () {
- return attachmentItem;
- });
+
+ throw e;
+ }
+ return attachmentItem;
});
@@ -787,21 +786,18 @@ Zotero.Attachments = new function(){
/**
* Create directory for attachment files within storage directory
*
- * If a directory exists with the same name, move it to orphaned-files
+ * If a directory exists, delete and recreate
*
* @param {Number} itemID - Item id
- * @return {Promise<nsIFile>}
+ * @return {Promise<String>} - Path of new directory
*/
this.createDirectoryForItem = Zotero.Promise.coroutine(function* (item) {
if (!(item instanceof Zotero.Item)) {
throw new Error("'item' must be a Zotero.Item");
}
- var dir = this.getStorageDirectory(item);
- yield _moveOrphanedDirectory(dir);
- if (!dir.exists()) {
- Zotero.debug("Creating directory " + dir.path);
- dir.create(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0755);
- }
+ var dir = this.getStorageDirectory(item).path;
+ yield OS.File.removeDir(dir, { ignoreAbsent: true });
+ yield Zotero.File.createDirectoryIfMissingAsync(dir);
return dir;
});
@@ -1161,55 +1157,6 @@ Zotero.Attachments = new function(){
/**
- * If directory exists and is non-empty, move it to orphaned-files directory
- *
- * If empty, just remove it
- */
- var _moveOrphanedDirectory = Zotero.Promise.coroutine(function* (dir) {
- if (!dir.exists()) {
- return;
- }
-
- dir = dir.clone();
-
- // If directory is empty or has only hidden files, delete it
- var files = dir.directoryEntries;
- files.QueryInterface(Components.interfaces.nsIDirectoryEnumerator);
- var empty = true;
- while (files.hasMoreElements()) {
- var file = files.getNext();
- file.QueryInterface(Components.interfaces.nsIFile);
- if (file.leafName[0] == '.') {
- continue;
- }
- empty = false;
- break;
- }
- files.close();
- if (empty) {
- dir.remove(true);
- return;
- }
-
- // Create orphaned-files directory if it doesn't exist
- var orphaned = OS.Path.join(Zotero.DataDirectory.dir, 'orphaned-files');
- yield Zotero.File.createDirectoryIfMissingAsync(orphaned);
-
- // Find unique filename for orphaned file
- var orphanTarget = OS.Path.join(orphaned, dir.leafName);
- var newName = null;
- if (yield OS.File.exists(orphanTarget)) {
- let newFile = yield OS.File.openUnique(orphanTarget, { humanReadable: true })
- newName = OS.Path.basename(newFile.path);
- newFile.file.close();
- }
-
- // Move target to orphaned files directory
- dir.moveTo(Zotero.File.pathToFile(orphaned), newName);
- });
-
-
- /**
* Create a new item of type 'attachment' and add to the itemAttachments table
*
* @param {Object} options - 'file', 'url', 'title', 'linkMode', 'contentType', 'charsetID',
diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
@@ -2536,7 +2536,7 @@ Zotero.Item.prototype.relinkAttachmentFile = Zotero.Promise.coroutine(function*
}
// Create storage directory if necessary
else if (!(yield OS.File.exists(storageDir))) {
- Zotero.Attachments.createDirectoryForItem(this);
+ yield Zotero.Attachments.createDirectoryForItem(this);
}
let newFile;
diff --git a/chrome/content/zotero/xpcom/fulltext.js b/chrome/content/zotero/xpcom/fulltext.js
@@ -659,7 +659,15 @@ Zotero.Fulltext = Zotero.FullText = new function(){
// If file is stored outside of Zotero, create a directory for the item
// in the storage directory and save the cache file there
if (linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE) {
- var parentDirPath = (yield Zotero.Attachments.createDirectoryForItem(item)).path;
+ let path = item.getFilePath();
+ if (!path) {
+ Zotero.debug("Invalid path for item " + itemID);
+ return false;
+ }
+ var parentDirPath = OS.Path.dirname(path);
+ if (!(yield OS.File.exists(parentDirPath))) {
+ yield Zotero.Attachments.createDirectoryForItem(item);
+ }
}
else {
var parentDirPath = OS.Path.dirname(filePath);
diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js
@@ -626,12 +626,7 @@ Zotero.Sync.Storage.Local = {
throw new Error("Downloaded file not found");
}
- var parentDirPath = Zotero.Attachments.getStorageDirectory(item).path;
- if (!(yield OS.File.exists(parentDirPath))) {
- yield Zotero.Attachments.createDirectoryForItem(item);
- }
-
- yield this._deleteExistingAttachmentFiles(item);
+ yield Zotero.Attachments.createDirectoryForItem(item);
var path = item.getFilePath();
if (!path) {
@@ -741,16 +736,12 @@ Zotero.Sync.Storage.Local = {
}
var parentDir = Zotero.Attachments.getStorageDirectory(item).path;
- if (!(yield OS.File.exists(parentDir))) {
- yield Zotero.Attachments.createDirectoryForItem(item);
- }
-
try {
- yield this._deleteExistingAttachmentFiles(item);
+ yield Zotero.Attachments.createDirectoryForItem(item);
}
catch (e) {
zipReader.close();
- throw (e);
+ throw e;
}
var returnFile = null;
@@ -906,17 +897,6 @@ Zotero.Sync.Storage.Local = {
}),
- _deleteExistingAttachmentFiles: Zotero.Promise.method(function (item) {
- var parentDir = Zotero.Attachments.getStorageDirectory(item).path;
- // OS.File.DirectoryIterator, used by OS.File.removeDir(), isn't reliable on Travis,
- // returning entry.isDir == false for subdirectories, so use nsIFile instead
- if (Zotero.automatedTest) {
- Zotero.File.pathToFile(parentDir).remove(true);
- }
- return OS.File.removeDir(parentDir);
- }),
-
-
/**
* @return {Promise<Object[]>} - A promise for an array of conflict objects
*/
diff --git a/test/tests/attachmentsTest.js b/test/tests/attachmentsTest.js
@@ -274,5 +274,36 @@ describe("Zotero.Attachments", function() {
assert.isTrue(yield Zotero.Attachments.hasMultipleFiles(item));
assert.equal((yield Zotero.Attachments.getNumFiles(item)), 2);
})
- })
+ });
+
+ describe("#createDirectoryForItem()", function () {
+ it("should create missing directory", function* () {
+ var item = yield importFileAttachment('test.png');
+ var path = OS.Path.dirname(item.getFilePath());
+ yield OS.File.removeDir(path);
+ yield Zotero.Attachments.createDirectoryForItem(item);
+ assert.isTrue(yield OS.File.exists(path));
+ });
+
+ it("should delete all existing files", function* () {
+ var item = yield importFileAttachment('test.html');
+ var path = OS.Path.dirname(item.getFilePath());
+ var files = ['a', 'b', 'c', 'd'];
+ for (let file of files) {
+ yield Zotero.File.putContentsAsync(OS.Path.join(path, file), file);
+ }
+ yield Zotero.Attachments.createDirectoryForItem(item);
+ assert.isTrue(yield Zotero.File.directoryIsEmpty(path));
+ assert.isTrue(yield OS.File.exists(path));
+ });
+
+ it("should handle empty directory", function* () {
+ var item = yield importFileAttachment('test.png');
+ var file = item.getFilePath();
+ var dir = OS.Path.dirname(item.getFilePath());
+ yield OS.File.remove(file);
+ yield Zotero.Attachments.createDirectoryForItem(item);
+ assert.isTrue(yield OS.File.exists(dir));
+ });
+ });
})
diff --git a/test/tests/storageLocalTest.js b/test/tests/storageLocalTest.js
@@ -218,24 +218,6 @@ describe("Zotero.Sync.Storage.Local", function () {
})
})
- describe("#_deleteExistingAttachmentFiles()", function () {
- it("should delete all files", function* () {
- var item = yield importFileAttachment('test.html');
- var path = OS.Path.dirname(item.getFilePath());
- var files = ['a', 'b', 'c', 'd'];
- for (let file of files) {
- yield Zotero.File.putContentsAsync(OS.Path.join(path, file), file);
- }
- yield Zotero.Sync.Storage.Local._deleteExistingAttachmentFiles(item);
- for (let file of files) {
- assert.isFalse(
- (yield OS.File.exists(OS.Path.join(path, file))),
- `File '${file}' doesn't exist`
- );
- }
- })
- })
-
describe("#getConflicts()", function () {
it("should return an array of objects for attachments in conflict", function* () {
var libraryID = Zotero.Libraries.userLibraryID;
