Merge pull request #5235 from weseek/feat/delete-revert-update-descendant-count

feat: Updating descendant count for delete, deleteCompletely, revert method

packages/app/src/server/models/page.ts

@@ -145,7 +145,7 @@ schema.statics.createEmptyPagesByPaths = async function(paths: string[], publicO
 };
 
 schema.statics.createEmptyPage = async function(
-    path: string, parent: any, // TODO: improve type including IPage at https://redmine.weseek.co.jp/issues/86506
+    path: string, parent: any, descendantCount: number, // TODO: improve type including IPage at https://redmine.weseek.co.jp/issues/86506
 ): Promise<PageDocument & { _id: any }> {
   if (parent == null) {
     throw Error('parent must not be null');
@@ -156,6 +156,7 @@ schema.statics.createEmptyPage = async function(
   page.path = path;
   page.isEmpty = true;
   page.parent = parent;
+  page.descendantCount = descendantCount;
 
   return page.save();
 };
@@ -174,7 +175,7 @@ schema.statics.replaceTargetWithPage = async function(exPage, pageToReplaceWith?
   }
 
   // create empty page at path
-  const newTarget = pageToReplaceWith == null ? await this.createEmptyPage(exPage.path, parent) : pageToReplaceWith;
+  const newTarget = pageToReplaceWith == null ? await this.createEmptyPage(exPage.path, parent, exPage.descendantCount) : pageToReplaceWith;
 
   // find children by ex-page _id
   const children = await this.find({ parent: exPage._id });
@@ -454,21 +455,12 @@ schema.statics.getAggrConditionForPageWithProvidedPathAndDescendants = function(
  * add/subtract descendantCount of pages with provided paths by increment.
  * increment can be negative number
  */
-schema.statics.incrementDescendantCountOfPaths = async function(paths:string[], increment: number):Promise<void> {
-  const pages = await this.aggregate([{ $match: { path: { $in: paths } } }]);
-  const operations = pages.map((page) => {
-    return {
-      updateOne: {
-        filter: { path: page.path },
-        update: { descendantCount: page.descendantCount + increment },
-      },
-    };
-  });
-  await this.bulkWrite(operations);
+schema.statics.incrementDescendantCountOfPageIds = async function(pageIds: ObjectIdLike[], increment: number): Promise<void> {
+  await this.updateMany({ _id: { $in: pageIds } }, { $inc: { descendantCount: increment } });
 };
 
 // update descendantCount of a page with provided id
-schema.statics.recountDescendantCountOfSelfAndDescendants = async function(id:mongoose.Types.ObjectId):Promise<void> {
+schema.statics.recountDescendantCountOfSelfAndDescendants = async function(id: ObjectIdLike):Promise<void> {
   const res = await this.aggregate(
     [
       {
@@ -478,8 +470,8 @@ schema.statics.recountDescendantCountOfSelfAndDescendants = async function(id:mo
       },
       {
         $project: {
-          path: 1,
           parent: 1,
+          isEmpty: 1,
           descendantCount: 1,
         },
       },
@@ -490,7 +482,9 @@ schema.statics.recountDescendantCountOfSelfAndDescendants = async function(id:mo
             $sum: '$descendantCount',
           },
           sumOfDocsCount: {
-            $sum: 1,
+            $sum: {
+              $cond: { if: { $eq: ['$isEmpty', true] }, then: 0, else: 1 }, // exclude isEmpty true page from sumOfDocsCount
+            },
           },
         },
       },
@@ -508,6 +502,22 @@ schema.statics.recountDescendantCountOfSelfAndDescendants = async function(id:mo
   await this.findByIdAndUpdate(id, query);
 };
 
+schema.statics.findAncestorsUsingParentRecursively = async function(pageId: ObjectIdLike, shouldIncludeTarget: boolean) {
+  const self = this;
+  const target = await this.findById(pageId);
+
+  async function findAncestorsRecursively(target, ancestors = shouldIncludeTarget ? [target] : []) {
+    const parent = await self.findOne({ _id: target.parent });
+    if (parent == null) {
+      return ancestors;
+    }
+
+    return findAncestorsRecursively(parent, [...ancestors, parent]);
+  }
+
+  return findAncestorsRecursively(target);
+};
+
 export type PageCreateOptions = {
   format?: string
   grantUserGroupId?: ObjectIdLike
@@ -612,6 +622,8 @@ export default (crowi: Crowi): any => {
 
     let savedPage = await page.save();
 
+    await crowi.pageService?.updateDescendantCountOfAncestors(page._id, 1, false);
+
     /*
      * After save
      */

packages/app/src/server/routes/page.js

@@ -1209,7 +1209,12 @@ module.exports = function(crowi, app) {
         await crowi.pageService.deleteCompletely(page, req.user, options, isRecursively);
       }
       else {
-        if (!page.isEmpty && !page.isUpdatable(previousRevision)) {
+        const notRecursivelyAndEmpty = page.isEmpty && !isRecursively;
+        if (notRecursivelyAndEmpty) {
+          return res.json(ApiResponse.error(`Page '${pageId}' is not found.`, 'notfound'));
+        }
+
+        if (!page.isUpdatable(previousRevision)) {
           return res.json(ApiResponse.error('Someone could update this page, so couldn\'t delete.', 'outdated'));
         }
 

packages/app/src/server/service/page.ts

@@ -343,9 +343,6 @@ class PageService {
       }
     }
 
-    // update descendants first
-    await this.renameDescendantsWithStream(page, newPagePath, user, options, shouldUseV4Process);
-
     /*
      * update target
      */
@@ -363,6 +360,10 @@ class PageService {
 
     this.pageEvent.emit('rename', page, user);
 
+    // TODO: resume
+    // update descendants first
+    this.renameDescendantsWithStream(page, newPagePath, user, options, shouldUseV4Process);
+
     return renamedPage;
   }
 
@@ -570,7 +571,7 @@ class PageService {
       .pipe(createBatchStream(BULK_REINDEX_SIZE))
       .pipe(writeStream);
 
-    await streamToPromise(readStream);
+    await streamToPromise(writeStream);
   }
 
   private async renameDescendantsWithStreamV4(targetPage, newPagePath, user, options = {}) {
@@ -679,10 +680,6 @@ class PageService {
       newPagePath, page.revision.body, user, options,
     );
 
-    if (isRecursively) {
-      this.duplicateDescendantsWithStream(page, newPagePath, user, shouldUseV4Process);
-    }
-
     // take over tags
     const originTags = await page.findRelatedTagsById();
     let savedTags = [];
@@ -695,6 +692,11 @@ class PageService {
     const result = serializePageSecurely(createdPage);
     result.tags = savedTags;
 
+    // TODO: resume
+    if (isRecursively) {
+      this.duplicateDescendantsWithStream(page, newPagePath, user, shouldUseV4Process);
+    }
+
     return result;
   }
 
@@ -893,7 +895,7 @@ class PageService {
 
     const duplicateDescendants = this.duplicateDescendants.bind(this);
     const shouldNormalizeParent = this.shouldNormalizeParent.bind(this);
-    const normalizeParentRecursively = this.normalizeParentRecursively.bind(this);
+    const normalizeParentAndDescendantCountOfDescendants = this.normalizeParentAndDescendantCountOfDescendants.bind(this);
     const pageEvent = this.pageEvent;
     let count = 0;
     const writeStream = new Writable({
@@ -915,9 +917,7 @@ class PageService {
         const shouldNormalize = shouldNormalizeParent(page);
         if (shouldNormalize) {
           try {
-            const escapedPath = escapeStringRegexp(newPagePath);
-            const regexps = [new RegExp(`^${escapedPath}`, 'i')];
-            await normalizeParentRecursively(null, regexps);
+            await normalizeParentAndDescendantCountOfDescendants(newPagePath);
             logger.info(`Successfully normalized duplicated descendant pages under "${newPagePath}"`);
           }
           catch (err) {
@@ -1004,20 +1004,19 @@ class PageService {
       throw new Error('Page is not deletable.');
     }
 
-    // replace with an empty page
-    const shouldReplace = !isRecursively && await Page.exists({ parent: page._id });
-    if (shouldReplace) {
-      await Page.replaceTargetWithPage(page);
-    }
-
-    if (isRecursively) {
-      this.deleteDescendantsWithStream(page, user, shouldUseV4Process); // use the same process in both version v4 and v5
-    }
-    else {
+    if (!isRecursively) {
       // replace with an empty page
       const shouldReplace = await Page.exists({ parent: page._id });
       if (shouldReplace) {
-        await Page.replaceTargetWithEmptyPage(page);
+        await Page.replaceTargetWithPage(page);
+      }
+
+      // update descendantCount of ancestors'
+      await this.updateDescendantCountOfAncestors(page.parent, -1, true);
+
+      const shouldDeleteLeafEmptyPages = !shouldReplace;
+      if (shouldDeleteLeafEmptyPages) {
+        // TODO https://redmine.weseek.co.jp/issues/87667 : delete leaf empty pages here
       }
     }
 
@@ -1030,7 +1029,7 @@ class PageService {
       await Revision.updateRevisionListByPageId(page._id, { pageId: page._id });
       deletedPage = await Page.findByIdAndUpdate(page._id, {
         $set: {
-          path: newPath, status: Page.STATUS_DELETED, deleteUser: user._id, deletedAt: Date.now(), parent: null, // set parent as null
+          path: newPath, status: Page.STATUS_DELETED, deleteUser: user._id, deletedAt: Date.now(), parent: null, descendantCount: 0, // set parent as null
         },
       }, { new: true });
       await PageTagRelation.updateMany({ relatedPage: page._id }, { $set: { isPageTrashed: true } });
@@ -1041,6 +1040,21 @@ class PageService {
       this.pageEvent.emit('create', deletedPage, user);
     }
 
+    // TODO: resume
+    // no await for deleteDescendantsWithStream and updateDescendantCountOfAncestors
+    if (isRecursively) {
+      (async() => {
+        const deletedDescendantCount = await this.deleteDescendantsWithStream(page, user, shouldUseV4Process); // use the same process in both version v4 and v5
+
+        // update descendantCount of ancestors'
+        if (page.parent != null) {
+          await this.updateDescendantCountOfAncestors(page.parent, (deletedDescendantCount + 1) * -1, true);
+
+          // TODO https://redmine.weseek.co.jp/issues/87667 : delete leaf empty pages here
+        }
+      })();
+    }
+
     return deletedPage;
   }
 
@@ -1108,7 +1122,7 @@ class PageService {
             filter: { _id: page._id },
             update: {
               $set: {
-                path: newPath, status: Page.STATUS_DELETED, deleteUser: user._id, deletedAt: Date.now(), parent: null, // set parent as null
+                path: newPath, status: Page.STATUS_DELETED, deleteUser: user._id, deletedAt: Date.now(), parent: null, descendantCount: 0, // set parent as null
               },
             },
           },
@@ -1150,9 +1164,9 @@ class PageService {
   }
 
   /**
-   * Create delete stream
+   * Create delete stream and return deleted document count
    */
-  private async deleteDescendantsWithStream(targetPage, user, shouldUseV4Process = true) {
+  private async deleteDescendantsWithStream(targetPage, user, shouldUseV4Process = true): Promise<number> {
     let readStream;
     if (shouldUseV4Process) {
       readStream = await this.generateReadStreamToOperateOnlyDescendants(targetPage.path, user);
@@ -1165,9 +1179,13 @@ class PageService {
 
     const deleteDescendants = this.deleteDescendants.bind(this);
     let count = 0;
+    let nDeletedNonEmptyPages = 0; // used for updating descendantCount
+
     const writeStream = new Writable({
       objectMode: true,
       async write(batch, encoding, callback) {
+        nDeletedNonEmptyPages += batch.filter(d => !d.isEmpty).length;
+
         try {
           count += batch.length;
           await deleteDescendants(batch, user);
@@ -1189,6 +1207,10 @@ class PageService {
     readStream
       .pipe(createBatchStream(BULK_REINDEX_SIZE))
       .pipe(writeStream);
+
+    await streamToPromise(readStream);
+
+    return nDeletedNonEmptyPages;
   }
 
   private async deleteCompletelyOperation(pageIds, pagePaths) {
@@ -1257,14 +1279,31 @@ class PageService {
 
     await this.deleteCompletelyOperation(ids, paths);
 
-    if (isRecursively) {
-      this.deleteCompletelyDescendantsWithStream(page, user, options, shouldUseV4Process);
+    if (!isRecursively) {
+      await this.updateDescendantCountOfAncestors(page.parent, -1, true);
+
+      // TODO https://redmine.weseek.co.jp/issues/87667 : delete leaf empty pages here
     }
 
     if (!page.isEmpty && !preventEmitting) {
       this.pageEvent.emit('deleteCompletely', page, user);
     }
 
+    // TODO: resume
+    if (isRecursively) {
+      // no await for deleteCompletelyDescendantsWithStream
+      (async() => {
+        const deletedDescendantCount = await this.deleteCompletelyDescendantsWithStream(page, user, options, shouldUseV4Process);
+
+        // update descendantCount of ancestors'
+        if (page.parent != null) {
+          await this.updateDescendantCountOfAncestors(page.parent, (deletedDescendantCount + 1) * -1, true);
+        }
+
+        // TODO https://redmine.weseek.co.jp/issues/87667 : delete leaf empty pages here
+      })();
+    }
+
     return;
   }
 
@@ -1294,7 +1333,7 @@ class PageService {
   /**
    * Create delete completely stream
    */
-  private async deleteCompletelyDescendantsWithStream(targetPage, user, options = {}, shouldUseV4Process = true) {
+  private async deleteCompletelyDescendantsWithStream(targetPage, user, options = {}, shouldUseV4Process = true): Promise<number> {
     let readStream;
 
     if (shouldUseV4Process) { // pages don't have parents
@@ -1305,11 +1344,15 @@ class PageService {
       readStream = await factory.generateReadable();
     }
 
-    const deleteMultipleCompletely = this.deleteMultipleCompletely.bind(this);
     let count = 0;
+    let nDeletedNonEmptyPages = 0; // used for updating descendantCount
+
+    const deleteMultipleCompletely = this.deleteMultipleCompletely.bind(this);
     const writeStream = new Writable({
       objectMode: true,
       async write(batch, encoding, callback) {
+        nDeletedNonEmptyPages += batch.filter(d => !d.isEmpty).length;
+
         try {
           count += batch.length;
           await deleteMultipleCompletely(batch, user, options);
@@ -1331,6 +1374,10 @@ class PageService {
     readStream
       .pipe(createBatchStream(BULK_REINDEX_SIZE))
       .pipe(writeStream);
+
+    await streamToPromise(readStream);
+
+    return nDeletedNonEmptyPages;
   }
 
   // use the same process in both v4 and v5
@@ -1394,13 +1441,28 @@ class PageService {
     page.lastUpdateUser = user;
     const updatedPage = await Page.findByIdAndUpdate(page._id, {
       $set: {
-        path: newPath, status: Page.STATUS_PUBLISHED, lastUpdateUser: user._id, deleteUser: null, deletedAt: null, parent: parent._id,
+        path: newPath, status: Page.STATUS_PUBLISHED, lastUpdateUser: user._id, deleteUser: null, deletedAt: null, parent: parent._id, descendantCount: 0,
       },
     }, { new: true });
     await PageTagRelation.updateMany({ relatedPage: page._id }, { $set: { isPageTrashed: false } });
 
     if (isRecursively) {
-      this.revertDeletedDescendantsWithStream(page, user, options, shouldUseV4Process);
+      await this.updateDescendantCountOfAncestors(parent._id, 1, true);
+    }
+
+    // TODO: resume
+    if (!isRecursively) {
+      // no await for revertDeletedDescendantsWithStream
+      (async() => {
+        const revertedDescendantCount = await this.revertDeletedDescendantsWithStream(page, user, options, shouldUseV4Process);
+
+        // update descendantCount of ancestors'
+        if (page.parent != null) {
+          await this.updateDescendantCountOfAncestors(page.parent, revertedDescendantCount + 1, true);
+
+          // TODO https://redmine.weseek.co.jp/issues/87667 : delete leaf empty pages here
+        }
+      })();
     }
 
     return updatedPage;
@@ -1436,7 +1498,7 @@ class PageService {
   /**
    * Create revert stream
    */
-  private async revertDeletedDescendantsWithStream(targetPage, user, options = {}, shouldUseV4Process = true) {
+  private async revertDeletedDescendantsWithStream(targetPage, user, options = {}, shouldUseV4Process = true): Promise<number> {
     if (shouldUseV4Process) {
       return this.revertDeletedDescendantsWithStreamV4(targetPage, user, options);
     }
@@ -1444,7 +1506,7 @@ class PageService {
     const readStream = await this.generateReadStreamToOperateOnlyDescendants(targetPage.path, user);
 
     const revertDeletedDescendants = this.revertDeletedDescendants.bind(this);
-    const normalizeParentRecursively = this.normalizeParentRecursively.bind(this);
+    const normalizeParentAndDescendantCountOfDescendants = this.normalizeParentAndDescendantCountOfDescendants.bind(this);
     const shouldNormalizeParent = this.shouldNormalizeParent.bind(this);
     let count = 0;
     const writeStream = new Writable({
@@ -1468,9 +1530,7 @@ class PageService {
         if (shouldNormalize) {
           try {
             const newPath = Page.getRevertDeletedPageName(targetPage.path);
-            const escapedPath = escapeStringRegexp(newPath);
-            const regexps = [new RegExp(`^${escapedPath}`, 'i')];
-            await normalizeParentRecursively(null, regexps);
+            await normalizeParentAndDescendantCountOfDescendants(newPath);
             logger.info(`Successfully normalized reverted descendant pages under "${newPath}"`);
           }
           catch (err) {
@@ -1487,6 +1547,10 @@ class PageService {
     readStream
       .pipe(createBatchStream(BULK_REINDEX_SIZE))
       .pipe(writeStream);
+
+    await streamToPromise(readStream);
+
+    return count;
   }
 
   private async revertDeletedDescendantsWithStreamV4(targetPage, user, options = {}) {
@@ -1518,6 +1582,10 @@ class PageService {
     readStream
       .pipe(createBatchStream(BULK_REINDEX_SIZE))
       .pipe(writeStream);
+
+    await streamToPromise(readStream);
+
+    return count;
   }
 
 
@@ -1858,6 +1926,15 @@ class PageService {
     }
   }
 
+  private async normalizeParentAndDescendantCountOfDescendants(path: string): Promise<void> {
+    const escapedPath = escapeStringRegexp(path);
+    const regexps = [new RegExp(`^${escapedPath}`, 'i')];
+    await this.normalizeParentRecursively(null, regexps);
+
+    // update descendantCount of descendant pages
+    await this.updateDescendantCountOfSelfAndDescendants(path);
+  }
+
   // TODO: use websocket to show progress
   private async normalizeParentRecursively(grant, regexps, publicOnly = false): Promise<void> {
     const BATCH_SIZE = 100;
@@ -2046,7 +2123,7 @@ class PageService {
    * - page that has the same path as the provided path
    * - pages that are descendants of the above page
    */
-  async updateDescendantCountOfSelfAndDescendants(path = '/') {
+  async updateDescendantCountOfSelfAndDescendants(path) {
     const BATCH_SIZE = 200;
     const Page = this.crowi.model('Page');
 
@@ -2056,8 +2133,7 @@ class PageService {
     const recountWriteStream = new Writable({
       objectMode: true,
       async write(pageDocuments, encoding, callback) {
-        for (const document of pageDocuments) {
-          // eslint-disable-next-line no-await-in-loop
+        for await (const document of pageDocuments) {
           await Page.recountDescendantCountOfSelfAndDescendants(document._id);
         }
         callback();
@@ -2073,11 +2149,12 @@ class PageService {
     await streamToPromise(recountWriteStream);
   }
 
-  // update descendantCount of all pages that are ancestors of a provided path by count
-  async updateDescendantCountOfAncestors(path = '/', count = 0) {
+  // update descendantCount of all pages that are ancestors of a provided pageId by count
+  async updateDescendantCountOfAncestors(pageId: ObjectIdLike, inc: number, shouldIncludeTarget: boolean): Promise<void> {
     const Page = this.crowi.model('Page');
-    const ancestors = collectAncestorPaths(path);
-    await Page.incrementDescendantCountOfPaths(ancestors, count);
+    const ancestors = await Page.findAncestorsUsingParentRecursively(pageId, shouldIncludeTarget);
+    const ancestorPageIds = ancestors.map(p => p._id);
+    await Page.incrementDescendantCountOfPageIds(ancestorPageIds, inc);
   }
 
 }