Merge pull request #2676 from weseek/fix/3538-remove-attachment-metadata

Fix/3538 remove attachment metadata

src/server/crowi/index.js

@@ -105,6 +105,7 @@ Crowi.prototype.init = async function() {
     this.setupSlack(),
     this.setupCsrf(),
     this.setUpFileUpload(),
+    this.setupAttachmentService(),
     this.setUpAcl(),
     this.setUpCustomize(),
     this.setUpRestQiitaAPI(),
@@ -143,6 +144,7 @@ Crowi.prototype.initForTest = async function() {
     // this.setupSlack(),
     // this.setupCsrf(),
     // this.setUpFileUpload(),
+    // this.setupAttachmentService(),
     this.setUpAcl(),
     // this.setUpCustomize(),
     // this.setUpRestQiitaAPI(),
@@ -550,6 +552,16 @@ Crowi.prototype.setUpFileUpload = async function() {
   }
 };
 
+/**
+ * setup AttachmentService
+ */
+Crowi.prototype.setupAttachmentService = async function() {
+  const AttachmentService = require('../service/attachment');
+  if (this.attachmentService == null) {
+    this.attachmentService = new AttachmentService(this);
+  }
+};
+
 /**
  * setup RestQiitaAPIService
  */

src/server/models/attachment.js

@@ -42,8 +42,7 @@ module.exports = function(crowi) {
   attachmentSchema.set('toJSON', { virtuals: true });
 
 
-  attachmentSchema.statics.create = async function(pageId, user, fileStream, originalName, fileFormat, fileSize) {
-    const fileUploader = require('../service/file-uploader')(crowi);
+  attachmentSchema.statics.createWithoutSave = function(pageId, user, fileStream, originalName, fileFormat, fileSize) {
     const Attachment = this;
 
     const extname = path.extname(originalName);
@@ -52,7 +51,7 @@ module.exports = function(crowi) {
       fileName = `${fileName}${extname}`;
     }
 
-    let attachment = new Attachment();
+    const attachment = new Attachment();
     attachment.page = pageId;
     attachment.creator = user._id;
     attachment.originalName = originalName;
@@ -61,31 +60,8 @@ module.exports = function(crowi) {
     attachment.fileSize = fileSize;
     attachment.createdAt = Date.now();
 
-    // upload file
-    await fileUploader.uploadFile(fileStream, attachment);
-    // save attachment
-    attachment = await attachment.save();
-
     return attachment;
   };
 
-  attachmentSchema.statics.removeAttachmentsByPageId = async function(pageId) {
-    const attachments = await this.find({ page: pageId });
-
-    const promises = attachments.map(async(attachment) => {
-      return this.removeWithSubstanceById(attachment._id);
-    });
-
-    return Promise.all(promises);
-  };
-
-  attachmentSchema.statics.removeWithSubstanceById = async function(id) {
-    const fileUploader = require('../service/file-uploader')(crowi);
-    // retrieve data from DB to get a completely populated instance
-    const attachment = await this.findById(id);
-    await fileUploader.deleteFile(attachment);
-    return await attachment.remove();
-  };
-
   return mongoose.model('Attachment', attachmentSchema);
 };

src/server/models/page.js

@@ -1130,6 +1130,7 @@ module.exports = function(crowi) {
     }));
   };
 
+  // TODO: transplant to service/page.js because page deletion affects various models data
   pageSchema.statics.revertDeletedPage = async function(page, user, options = {}) {
     const newPath = this.getRevertDeletedPageName(page.path);
 
@@ -1173,31 +1174,16 @@ module.exports = function(crowi) {
   /**
    * This is danger.
    */
+  // TODO: transplant to service/page.js because page deletion affects various models data
   pageSchema.statics.completelyDeletePage = async function(pageData, user, options = {}) {
     validateCrowi();
 
-    // Delete Bookmarks, Attachments, Revisions, Pages and emit delete
-    const Bookmark = crowi.model('Bookmark');
-    const Attachment = crowi.model('Attachment');
-    const Comment = crowi.model('Comment');
-    const PageTagRelation = crowi.model('PageTagRelation');
-    const ShareLink = crowi.model('ShareLink');
-    const Revision = crowi.model('Revision');
-    const pageId = pageData._id;
+    const { _id, path } = pageData;
     const socketClientId = options.socketClientId || null;
 
-    debug('Completely delete', pageData.path);
-
-    await Promise.all([
-      Bookmark.removeBookmarksByPageId(pageId),
-      Attachment.removeAttachmentsByPageId(pageId),
-      Comment.removeCommentsByPageId(pageId),
-      PageTagRelation.remove({ relatedPage: pageId }),
-      ShareLink.remove({ relatedPage: pageId }),
-      Revision.removeRevisionsByPath(pageData.path),
-      this.findByIdAndRemove(pageId),
-      this.removeRedirectOriginPageByPath(pageData.path),
-    ]);
+    logger.debug('Deleting completely', path);
+
+    await crowi.pageService.deleteCompletely(_id, path);
 
     if (socketClientId != null) {
       pageEvent.emit('delete', pageData, user, socketClientId); // update as renamed page
@@ -1208,6 +1194,7 @@ module.exports = function(crowi) {
   /**
    * Delete Bookmarks, Attachments, Revisions, Pages and emit delete
    */
+  // TODO: transplant to service/page.js because page deletion affects various models data
   pageSchema.statics.completelyDeletePageRecursively = async function(targetPage, user, options = {}) {
     const findOpts = { includeTrashed: true };
 
@@ -1304,6 +1291,7 @@ module.exports = function(crowi) {
     return targetPage;
   };
 
+  // TODO: transplant to service/page.js because page deletion affects various models data
   pageSchema.statics.handlePrivatePagesForDeletedGroup = async function(deletedGroup, action, transferToUserGroupId) {
     const Page = mongoose.model('Page');
 

src/server/models/user.js

@@ -207,21 +207,23 @@ module.exports = function(crowi) {
     return userData;
   };
 
+  // TODO: create UserService and transplant this method because image uploading depends on AttachmentService
   userSchema.methods.updateImage = async function(attachment) {
     this.imageAttachment = attachment;
     await this.updateImageUrlCached();
     return this.save();
   };
 
+  // TODO: create UserService and transplant this method because image deletion depends on AttachmentService
   userSchema.methods.deleteImage = async function() {
     validateCrowi();
-    const Attachment = crowi.model('Attachment');
 
     // the 'image' field became DEPRECATED in v3.3.8
     this.image = undefined;
 
     if (this.imageAttachment != null) {
-      Attachment.removeWithSubstanceById(this.imageAttachment._id);
+      const { attachmentService } = crowi;
+      attachmentService.removeAttachment(this.imageAttachment._id);
     }
 
     this.imageAttachment = undefined;

src/server/routes/attachment.js

@@ -3,8 +3,6 @@
 
 const logger = require('@alias/logger')('growi:routes:attachment');
 
-const fs = require('fs');
-
 const ApiResponse = require('../util/apiResponse');
 
 /**
@@ -131,7 +129,7 @@ module.exports = function(crowi, app) {
   const Attachment = crowi.model('Attachment');
   const User = crowi.model('User');
   const Page = crowi.model('Page');
-  const fileUploader = require('../service/file-uploader')(crowi, app);
+  const { fileUploadService, attachmentService } = crowi;
 
 
   /**
@@ -193,13 +191,13 @@ module.exports = function(crowi, app) {
       return res.sendStatus(304);
     }
 
-    if (fileUploader.canRespond()) {
-      return fileUploader.respond(res, attachment);
+    if (fileUploadService.canRespond()) {
+      return fileUploadService.respond(res, attachment);
     }
 
     let fileStream;
     try {
-      fileStream = await fileUploader.findDeliveryFile(attachment);
+      fileStream = await fileUploadService.findDeliveryFile(attachment);
     }
     catch (e) {
       logger.error(e);
@@ -234,32 +232,17 @@ module.exports = function(crowi, app) {
     }
   }
 
-  async function createAttachment(file, user, pageId = null) {
-    // check limit
-    const res = await fileUploader.checkLimit(file.size);
-    if (!res.isUploadable) {
-      throw new Error(res.errorMessage);
-    }
+  async function removeAttachment(attachmentId) {
+    const { fileUploadService } = crowi;
 
-    const fileStream = fs.createReadStream(file.path, {
-      flags: 'r', encoding: null, fd: null, mode: '0666', autoClose: true,
-    });
+    // retrieve data from DB to get a completely populated instance
+    const attachment = await Attachment.findById(attachmentId);
 
-    // create an Attachment document and upload file
-    let attachment;
-    try {
-      attachment = await Attachment.create(pageId, user, fileStream, file.originalname, file.mimetype, file.size);
-    }
-    catch (err) {
-      // delete temporary file
-      fs.unlink(file.path, (err) => { if (err) { logger.error('Error while deleting tmp file.') } });
-      throw err;
-    }
+    await fileUploadService.deleteFile(attachment);
 
-    return attachment;
+    return attachment.remove();
   }
 
-
   const actions = {};
   const api = {};
 
@@ -409,7 +392,7 @@ module.exports = function(crowi, app) {
    */
   api.limit = async function(req, res) {
     const fileSize = Number(req.query.fileSize);
-    return res.json(ApiResponse.success(await fileUploader.checkLimit(fileSize)));
+    return res.json(ApiResponse.success(await fileUploadService.checkLimit(fileSize)));
   };
 
   /**
@@ -522,7 +505,7 @@ module.exports = function(crowi, app) {
 
     let attachment;
     try {
-      attachment = await createAttachment(file, req.user, pageId);
+      attachment = await attachmentService.createAttachment(file, req.user, pageId);
     }
     catch (err) {
       logger.error(err);
@@ -618,7 +601,7 @@ module.exports = function(crowi, app) {
     let attachment;
     try {
       req.user.deleteImage();
-      attachment = await createAttachment(file, req.user);
+      attachment = await attachmentService.createAttachment(file, req.user);
       await req.user.updateImage(attachment);
     }
     catch (err) {
@@ -687,7 +670,7 @@ module.exports = function(crowi, app) {
     }
 
     try {
-      await Attachment.removeWithSubstanceById(id);
+      await removeAttachment(attachment);
     }
     catch (err) {
       logger.error(err);

src/server/service/attachment.js

@@ -0,0 +1,58 @@
+const logger = require('@alias/logger')('growi:service:AttachmentService'); // eslint-disable-line no-unused-vars
+
+const fs = require('fs');
+
+
+/**
+ * the service class for Attachment and file-uploader
+ */
+class AttachmentService {
+
+  constructor(crowi) {
+    this.crowi = crowi;
+  }
+
+  async createAttachment(file, user, pageId = null) {
+    const { fileUploadService } = this.crowi;
+
+    // check limit
+    const res = await fileUploadService.checkLimit(file.size);
+    if (!res.isUploadable) {
+      throw new Error(res.errorMessage);
+    }
+
+    const Attachment = this.crowi.model('Attachment');
+
+    const fileStream = fs.createReadStream(file.path, {
+      flags: 'r', encoding: null, fd: null, mode: '0666', autoClose: true,
+    });
+
+    // create an Attachment document and upload file
+    let attachment;
+    try {
+      attachment = Attachment.createWithoutSave(pageId, user, fileStream, file.originalname, file.mimetype, file.size);
+      await fileUploadService.uploadFile(fileStream, attachment);
+      await attachment.save();
+    }
+    catch (err) {
+      // delete temporary file
+      fs.unlink(file.path, (err) => { if (err) { logger.error('Error while deleting tmp file.') } });
+      throw err;
+    }
+
+    return attachment;
+  }
+
+  async removeAttachment(attachmentId) {
+    const Attachment = this.crowi.model('Attachment');
+    const { fileUploadService } = this.crowi;
+
+    const attachment = await Attachment.findById(attachmentId);
+    await fileUploadService.deleteFile(attachment);
+
+    return attachment.remove();
+  }
+
+}
+
+module.exports = AttachmentService;

src/server/service/file-uploader/aws.js

@@ -49,6 +49,23 @@ module.exports = function(crowi) {
     return filePath;
   }
 
+  async function isFileExists(s3, params) {
+    // check file exists
+    try {
+      await s3.headObject(params).promise();
+    }
+    catch (err) {
+      if (err != null && err.code === 'NotFound') {
+        return false;
+      }
+
+      // error except for 'NotFound
+      throw err;
+    }
+
+    return true;
+  }
+
   lib.isValidUploadSettings = function() {
     return this.configManager.getConfig('crowi', 'aws:accessKeyId') != null
       && this.configManager.getConfig('crowi', 'aws:secretAccessKey') != null
@@ -74,7 +91,12 @@ module.exports = function(crowi) {
       Key: filePath,
     };
 
-    // TODO: ensure not to throw error even when the file does not exist
+    // check file exists
+    const isExists = await isFileExists(s3, params);
+    if (!isExists) {
+      logger.warn(`Any object that relate to the Attachment (${filePath}) does not exist in AWS S3`);
+      return;
+    }
 
     return s3.deleteObject(params).promise();
   };
@@ -108,12 +130,19 @@ module.exports = function(crowi) {
     const awsConfig = getAwsConfig();
     const filePath = getFilePathOnStorage(attachment);
 
+    const params = {
+      Bucket: awsConfig.bucket,
+      Key: filePath,
+    };
+
+    // check file exists
+    const isExists = await isFileExists(s3, params);
+    if (!isExists) {
+      throw new Error(`Any object that relate to the Attachment (${filePath}) does not exist in AWS S3`);
+    }
+
     let stream;
     try {
-      const params = {
-        Bucket: awsConfig.bucket,
-        Key: filePath,
-      };
       stream = s3.getObject(params).createReadStream();
     }
     catch (err) {

src/server/service/file-uploader/gcs.js

@@ -38,6 +38,16 @@ module.exports = function(crowi) {
     return filePath;
   }
 
+  /**
+   * check file existence
+   * @param {File} file https://googleapis.dev/nodejs/storage/latest/File.html
+   */
+  async function isFileExists(file) {
+    // check file exists
+    const res = await file.exists();
+    return res[0];
+  }
+
   lib.isValidUploadSettings = function() {
     return this.configManager.getConfig('crowi', 'gcs:apiKeyJsonPath') != null
       && this.configManager.getConfig('crowi', 'gcs:bucket') != null;
@@ -51,10 +61,16 @@ module.exports = function(crowi) {
   lib.deleteFileByFilePath = async function(filePath) {
     const gcs = getGcsInstance(this.getIsUploadable());
     const myBucket = gcs.bucket(getGcsBucket());
+    const file = myBucket.file(filePath);
 
-    // TODO: ensure not to throw error even when the file does not exist
+    // check file exists
+    const isExists = await isFileExists(file);
+    if (!isExists) {
+      logger.warn(`Any object that relate to the Attachment (${filePath}) does not exist in GCS`);
+      return;
+    }
 
-    return myBucket.file(filePath).delete();
+    return file.delete();
   };
 
   lib.uploadFile = function(fileStream, attachment) {
@@ -80,10 +96,17 @@ module.exports = function(crowi) {
     const gcs = getGcsInstance(this.getIsUploadable());
     const myBucket = gcs.bucket(getGcsBucket());
     const filePath = getFilePathOnStorage(attachment);
+    const file = myBucket.file(filePath);
+
+    // check file exists
+    const isExists = await isFileExists(file);
+    if (!isExists) {
+      throw new Error(`Any object that relate to the Attachment (${filePath}) does not exist in GCS`);
+    }
 
     let stream;
     try {
-      stream = myBucket.file(filePath).createReadStream();
+      stream = file.createReadStream();
     }
     catch (err) {
       logger.error(err);

src/server/service/file-uploader/local.js

@@ -42,6 +42,7 @@ module.exports = function(crowi) {
     }
     catch (err) {
       logger.warn(`Any AttachmentFile which path is '${filePath}' does not exist in local fs`);
+      return;
     }
 
     return fs.unlinkSync(filePath);

src/server/service/page.js

@@ -24,6 +24,41 @@ class PageService {
     return returnObj;
   }
 
+  async deleteCompletely(pageId, pagePath) {
+    // Delete Bookmarks, Attachments, Revisions, Pages and emit delete
+    const Bookmark = this.crowi.model('Bookmark');
+    const Comment = this.crowi.model('Comment');
+    const Page = this.crowi.model('Page');
+    const PageTagRelation = this.crowi.model('PageTagRelation');
+    const ShareLink = this.crowi.model('ShareLink');
+    const Revision = this.crowi.model('Revision');
+
+    return Promise.all([
+      Bookmark.removeBookmarksByPageId(pageId),
+      Comment.removeCommentsByPageId(pageId),
+      PageTagRelation.remove({ relatedPage: pageId }),
+      ShareLink.remove({ relatedPage: pageId }),
+      Revision.removeRevisionsByPath(pagePath),
+      Page.findByIdAndRemove(pageId),
+      Page.removeRedirectOriginPageByPath(pagePath),
+      this.removeAllAttachments(pageId),
+    ]);
+  }
+
+  async removeAllAttachments(pageId) {
+    const Attachment = this.crowi.model('Attachment');
+    const { attachmentService } = this.crowi;
+
+    const attachments = await Attachment.find({ page: pageId });
+
+    const promises = attachments.map(async(attachment) => {
+      return attachmentService.removeAttachment(attachment._id);
+    });
+
+    return Promise.all(promises);
+  }
+
+
 }
 
 module.exports = PageService;

src/server/util/swigFunctions.js

@@ -12,7 +12,6 @@ module.exports = function(crowi, req, locals) {
     passportService,
     appService,
     aclService,
-    fileUploadService,
     customizeService,
   } = crowi;
   debug('initializing swigFunctions');
@@ -69,7 +68,6 @@ module.exports = function(crowi, req, locals) {
    */
   locals.appService = appService;
   locals.aclService = aclService;
-  locals.fileUploadService = fileUploadService;
   locals.customizeService = customizeService;
   locals.passportService = passportService;
   locals.pathUtils = pathUtils;