Merge pull request #5302 from weseek/feat/update-descendant-count-by-normalize-parent

feat: Update descendant count by normalize parent

packages/app/src/server/models/obsolete-page.js

@@ -189,6 +189,39 @@ export class PageQueryBuilder {
     return this;
   }
 
+  async addConditionForParentNormalization(user) {
+    // determine UserGroup condition
+    let userGroups = null;
+    if (user != null) {
+      const UserGroupRelation = mongoose.model('UserGroupRelation');
+      userGroups = await UserGroupRelation.findAllUserGroupIdsRelatedToUser(user);
+    }
+
+    const grantConditions = [
+      { grant: null },
+      { grant: GRANT_PUBLIC },
+    ];
+
+    if (user != null) {
+      grantConditions.push(
+        { grant: GRANT_OWNER, grantedUsers: user._id },
+      );
+    }
+
+    if (userGroups != null && userGroups.length > 0) {
+      grantConditions.push(
+        { grant: GRANT_USER_GROUP, grantedGroup: { $in: userGroups } },
+      );
+    }
+
+    this.query = this.query
+      .and({
+        $or: grantConditions,
+      });
+
+    return this;
+  }
+
   addConditionToFilteringByViewer(user, userGroups, showAnyoneKnowsLink = false, showPagesRestrictedByOwner = false, showPagesRestrictedByGroup = false) {
     const grantConditions = [
       { grant: null },

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

@@ -167,7 +167,7 @@ schema.statics.createEmptyPage = async function(
  * @param exPage a page document to be replaced
  * @returns Promise<void>
  */
-schema.statics.replaceTargetWithPage = async function(exPage, pageToReplaceWith?): Promise<void> {
+schema.statics.replaceTargetWithPage = async function(exPage, pageToReplaceWith?, deleteExPageIfEmpty = false): Promise<void> {
   // find parent
   const parent = await this.findOne({ _id: exPage.parent });
   if (parent == null) {
@@ -201,6 +201,12 @@ schema.statics.replaceTargetWithPage = async function(exPage, pageToReplaceWith?
   };
 
   await this.bulkWrite([operationForNewTarget, operationsForChildren]);
+
+  const isExPageEmpty = exPage.isEmpty;
+  if (deleteExPageIfEmpty && isExPageEmpty) {
+    await this.deleteOne({ _id: exPage._id });
+    logger.warn('Deleted empty page since it was replaced with another page.');
+  }
 };
 
 /**
@@ -219,7 +225,7 @@ schema.statics.getParentAndFillAncestors = async function(path: string): Promise
   /*
    * Fill parents if parent is null
    */
-  const ancestorPaths = collectAncestorPaths(path); // paths of parents need to be created
+  const ancestorPaths = collectAncestorPaths(path, [path]); // paths of parents need to be created
 
   // just create ancestors with empty pages
   await this.createEmptyPagesByPaths(ancestorPaths);
@@ -577,6 +583,15 @@ schema.statics.findByPageIdsToEdit = async function(ids, user, shouldIncludeEmpt
   return pages;
 };
 
+schema.statics.normalizeDescendantCountById = async function(pageId) {
+  const children = await this.find({ parent: pageId });
+
+  const sumChildrenDescendantCount = children.map(d => d.descendantCount).reduce((c1, c2) => c1 + c2);
+  const sumChildPages = children.filter(p => !p.isEmpty).length;
+
+  return this.updateOne({ _id: pageId }, { $set: { descendantCount: sumChildrenDescendantCount + sumChildPages } }, { new: true });
+};
+
 export type PageCreateOptions = {
   format?: string
   grantUserGroupId?: ObjectIdLike

packages/app/src/server/routes/apiv3/pages.js

@@ -801,7 +801,7 @@ module.exports = (crowi) => {
     }
     else {
       try {
-        await crowi.pageService.normalizeParentByPageIds(pageIds);
+        await crowi.pageService.normalizeParentByPageIds(pageIds, req.user);
       }
       catch (err) {
         return res.apiv3Err(new ErrorV3(`Failed to migrate pages: ${err.message}`), 500);
@@ -814,7 +814,7 @@ module.exports = (crowi) => {
   router.get('/v5-migration-status', accessTokenParser, loginRequired, async(req, res) => {
     try {
       const isV5Compatible = crowi.configManager.getConfig('crowi', 'app:isV5Compatible');
-      const migratablePagesCount = req.user != null ? await crowi.pageService.v5MigratablePrivatePagesCount(req.user) : null;
+      const migratablePagesCount = req.user != null ? await crowi.pageService.countPagesCanNormalizeParentByUser(req.user) : null; // null check since not using loginRequiredStrictly
       return res.apiv3({ isV5Compatible, migratablePagesCount });
     }
     catch (err) {

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

@@ -233,7 +233,7 @@ class PageGrantService {
       .addConditionToSortPagesByDescPath()
       .query
       .exec();
-    const testAncestor = ancestors[0];
+    const testAncestor = ancestors[0]; // TODO: consider when duplicate testAncestors exist
     if (testAncestor == null) {
       throw Error('testAncestor must exist');
     }
@@ -320,7 +320,9 @@ class PageGrantService {
 
   /**
    * About the rule of validation, see: https://dev.growi.org/61b2cdabaa330ce7d8152844
-   * Only v5 schema pages will be used to compare.
+   * Only v5 schema pages will be used to compare by default (Set includeNotMigratedPages to true to include v4 schema pages as well).
+   * When comparing, it will use path regex to collect pages instead of using parent attribute of the Page model. This is reasonable since
+   * using the path attribute is safer than using the parent attribute in this case. 2022.02.13 -- Taichi Masuyama
    * @returns Promise<boolean>
    */
   async isGrantNormalized(
@@ -344,7 +346,13 @@ class PageGrantService {
     return this.processValidation(comparableTarget, comparableAncestor, comparableDescendants);
   }
 
-  async separateNormalizedAndNonNormalizedPages(pageIds: ObjectIdLike[]): Promise<[(PageDocument & { _id: any })[], (PageDocument & { _id: any })[]]> {
+  /**
+   * Separate normalizable pages and NOT normalizable pages by PageService.prototype.isGrantNormalized method.
+   * normalizable pages = Pages which are able to run normalizeParentRecursively method (grant & userGroup rule is correct)
+   * @param pageIds pageIds to be tested
+   * @returns a tuple with the first element of normalizable pages and the second element of NOT normalizable pages
+   */
+  async separateNormalizableAndNotNormalizablePages(pageIds: ObjectIdLike[]): Promise<[(PageDocument & { _id: any })[], (PageDocument & { _id: any })[]]> {
     if (pageIds.length > LIMIT_FOR_MULTIPLE_PAGE_OP) {
       throw Error(`The maximum number of pageIds allowed is ${LIMIT_FOR_MULTIPLE_PAGE_OP}.`);
     }
@@ -354,8 +362,8 @@ class PageGrantService {
     const shouldCheckDescendants = true;
     const shouldIncludeNotMigratedPages = true;
 
-    const normalizedPages: (PageDocument & { _id: any })[] = [];
-    const nonNormalizedPages: (PageDocument & { _id: any })[] = []; // can be used to tell user which page failed to migrate
+    const normalizable: (PageDocument & { _id: any })[] = [];
+    const nonNormalizable: (PageDocument & { _id: any })[] = []; // can be used to tell user which page failed to migrate
 
     const builder = new PageQueryBuilder(Page.find());
     builder.addConditionToListByPageIdsArray(pageIds);
@@ -369,14 +377,14 @@ class PageGrantService {
 
       const isNormalized = await this.isGrantNormalized(path, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants, shouldIncludeNotMigratedPages);
       if (isNormalized) {
-        normalizedPages.push(page);
+        normalizable.push(page);
       }
       else {
-        nonNormalizedPages.push(page);
+        nonNormalizable.push(page);
       }
     }
 
-    return [normalizedPages, nonNormalizedPages];
+    return [normalizable, nonNormalizable];
   }
 
 }

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

@@ -9,7 +9,7 @@ import { serializePageSecurely } from '../models/serializers/page-serializer';
 import { createBatchStream } from '~/server/util/batch-stream';
 import loggerFactory from '~/utils/logger';
 import {
-  CreateMethod, generateGrantCondition, PageCreateOptions, PageModel,
+  CreateMethod, generateGrantCondition, PageCreateOptions, PageDocument, PageModel,
 } from '~/server/models/page';
 import { stringifySnapshot } from '~/models/serializers/in-app-notification-snapshot/page';
 import ActivityDefine from '../util/activityDefine';
@@ -1065,7 +1065,7 @@ class PageService {
       // replace with an empty page
       const shouldReplace = await Page.exists({ parent: page._id });
       if (shouldReplace) {
-        await Page.replaceTargetWithPage(page);
+        await Page.replaceTargetWithPage(page, null, true);
       }
 
       // update descendantCount of ancestors'
@@ -1077,10 +1077,7 @@ class PageService {
 
     let deletedPage;
     // update Revisions
-    if (page.isEmpty) {
-      await Page.remove({ _id: page._id });
-    }
-    else {
+    if (!page.isEmpty) {
       await Revision.updateRevisionListByPageId(page._id, { pageId: page._id });
       deletedPage = await Page.findByIdAndUpdate(page._id, {
         $set: {
@@ -1808,10 +1805,18 @@ class PageService {
     await inAppNotificationService.emitSocketIo(targetUsers);
   }
 
-  async normalizeParentByPageIds(pageIds: ObjectIdLike[]): Promise<void> {
+  async normalizeParentByPageIds(pageIds: ObjectIdLike[], user): Promise<void> {
     for await (const pageId of pageIds) {
       try {
-        await this.normalizeParentByPageId(pageId);
+        const normalizedPage = await this.normalizeParentByPageId(pageId, user);
+
+        if (normalizedPage == null) {
+          logger.error(`Failed to update descendantCount of page of id: "${pageId}"`);
+        }
+        else {
+          // update descendantCount of ancestors'
+          await this.updateDescendantCountOfAncestors(pageId, normalizedPage.descendantCount, false);
+        }
       }
       catch (err) {
         // socket.emit('normalizeParentByPageIds', { error: err.message }); TODO: use socket to tell user
@@ -1819,17 +1824,23 @@ class PageService {
     }
   }
 
-  private async normalizeParentByPageId(pageId: ObjectIdLike) {
+  private async normalizeParentByPageId(pageId: ObjectIdLike, user) {
     const Page = mongoose.model('Page') as unknown as PageModel;
-    const target = await Page.findById(pageId);
+    const target = await Page.findByIdAndViewerToEdit(pageId, user);
     if (target == null) {
-      throw Error('target does not exist');
+      throw Error('target does not exist or forbidden');
     }
 
     const {
       path, grant, grantedUsers: grantedUserIds, grantedGroup: grantedGroupId,
     } = target;
 
+    // check if any page exists at target path already
+    const existingPage = await Page.findOne({ path });
+    if (existingPage != null && !existingPage.isEmpty) {
+      throw Error('Page already exists. Please rename the page to continue.');
+    }
+
     /*
      * UserGroup & Owner validation
      */
@@ -1852,62 +1863,73 @@ class PageService {
       throw Error('Restricted pages can not be migrated');
     }
 
-    // getParentAndFillAncestors
-    const parent = await Page.getParentAndFillAncestors(target.path);
+    let updatedPage;
+
+    // replace if empty page exists
+    if (existingPage != null && existingPage.isEmpty) {
+      await Page.replaceTargetWithPage(existingPage, target, true);
+      updatedPage = await Page.findById(pageId);
+    }
+    else {
+      // getParentAndFillAncestors
+      const parent = await Page.getParentAndFillAncestors(target.path);
+      updatedPage = await Page.findOneAndUpdate({ _id: pageId }, { parent: parent._id }, { new: true });
+    }
 
-    return Page.updateOne({ _id: pageId }, { parent: parent._id });
+    return updatedPage;
   }
 
+  // TODO: this should be resumable
   async normalizeParentRecursivelyByPageIds(pageIds, user) {
+    const Page = mongoose.model('Page') as unknown as PageModel;
+
     if (pageIds == null || pageIds.length === 0) {
       logger.error('pageIds is null or 0 length.');
       return;
     }
 
-    let normalizedIds;
-    let notNormalizedPaths;
+    if (pageIds.length > LIMIT_FOR_MULTIPLE_PAGE_OP) {
+      throw Error(`The maximum number of pageIds allowed is ${LIMIT_FOR_MULTIPLE_PAGE_OP}.`);
+    }
+
+    let normalizablePages;
+    let nonNormalizablePages;
     try {
-      [normalizedIds, notNormalizedPaths] = await this.crowi.pageGrantService.separateNormalizedAndNonNormalizedPages(pageIds);
+      [normalizablePages, nonNormalizablePages] = await this.crowi.pageGrantService.separateNormalizableAndNotNormalizablePages(pageIds);
     }
     catch (err) {
       throw err;
     }
 
-    if (normalizedIds.length === 0) {
+    if (normalizablePages.length === 0) {
       // socket.emit('normalizeParentRecursivelyByPageIds', { error: err.message }); TODO: use socket to tell user
       return;
     }
 
-    if (notNormalizedPaths.length !== 0) {
-      // TODO: iterate notNormalizedPaths and send socket error to client so that the user can know which path failed to migrate
+    if (nonNormalizablePages.length !== 0) {
+      // TODO: iterate nonNormalizablePages and send socket error to client so that the user can know which path failed to migrate
       // socket.emit('normalizeParentRecursivelyByPageIds', { error: err.message }); TODO: use socket to tell user
     }
 
-    /*
-     * generate regexps
-     */
-    const Page = mongoose.model('Page') as unknown as PageModel;
-
-    let pages;
-    try {
-      pages = await Page.findByPageIdsToEdit(pageIds, user, false);
-    }
-    catch (err) {
-      logger.error('Failed to find pages by ids', err);
-      throw err;
-    }
-
-    // prepare no duplicated area paths
-    let paths = pages.map(p => p.path);
-    paths = omitDuplicateAreaPathFromPaths(paths);
-
-    const regexps = paths.map(path => new RegExp(`^${escapeStringRegexp(path)}`));
+    const pagesToNormalize = omitDuplicateAreaPageFromPages(normalizablePages);
+    const pageIdsToNormalize = pagesToNormalize.map(p => p._id);
+    const pathsToNormalize = Array.from(new Set(pagesToNormalize.map(p => p.path)));
 
     // TODO: insertMany PageOperationBlock
 
-    // migrate recursively
+    // for updating descendantCount
+    const pageIdToExDescendantCountMap = new Map<ObjectIdLike, number>();
+
+    // MAIN PROCESS migrate recursively
+    const regexps = pathsToNormalize.map(p => new RegExp(`^${escapeStringRegexp(p)}`, 'i'));
     try {
       await this.normalizeParentRecursively(null, regexps);
+
+      // find pages to save descendantCount of normalized pages (only pages in pageIds parameter of this method)
+      const pagesBeforeUpdatingDescendantCount = await Page.findByIdsAndViewer(pageIdsToNormalize, user);
+      pagesBeforeUpdatingDescendantCount.forEach((p) => {
+        pageIdToExDescendantCountMap.set(p._id.toString(), p.descendantCount);
+      });
     }
     catch (err) {
       logger.error('V5 initial miration failed.', err);
@@ -1915,6 +1937,28 @@ class PageService {
 
       throw err;
     }
+
+    // POST MAIN PROCESS update descendantCount
+    try {
+      // update descendantCount of self and descendant pages first
+      for await (const path of pathsToNormalize) {
+        await this.updateDescendantCountOfSelfAndDescendants(path);
+      }
+
+      // find pages again to get updated descendantCount
+      // then calculate inc
+      const pagesAfterUpdatingDescendantCount = await Page.findByIdsAndViewer(pageIdsToNormalize, user);
+      for await (const page of pagesAfterUpdatingDescendantCount) {
+        const exDescendantCount = pageIdToExDescendantCountMap.get(page._id.toString()) || 0;
+        const newDescendantCount = page.descendantCount;
+        const inc = newDescendantCount - exDescendantCount;
+        await this.updateDescendantCountOfAncestors(page._id, inc, false);
+      }
+    }
+    catch (err) {
+      logger.error('Failed to update descendantCount after normalizing parent:', err);
+      throw Error(`Failed to update descendantCount after normalizing parent: ${err}`);
+    }
   }
 
   async _isPagePathIndexUnique() {
@@ -2194,12 +2238,23 @@ class PageService {
     }
   }
 
-  async v5MigratablePrivatePagesCount(user) {
+  async countPagesCanNormalizeParentByUser(user): Promise<number> {
     if (user == null) {
       throw Error('user is required');
     }
-    const Page = this.crowi.model('Page');
-    return Page.count({ parent: null, creator: user, grant: { $ne: Page.GRANT_PUBLIC } });
+
+    const Page = mongoose.model('Page') as unknown as PageModel;
+    const { PageQueryBuilder } = Page;
+
+    const builder = new PageQueryBuilder(Page.count(), false);
+    builder.addConditionAsNotMigrated();
+    builder.addConditionAsNonRootPage();
+    builder.addConditionToExcludeTrashed();
+    await builder.addConditionForParentNormalization(user);
+
+    const nMigratablePages = await builder.query.exec();
+
+    return nMigratablePages;
   }
 
   /**
@@ -2207,7 +2262,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: string): Promise<void> {
     const BATCH_SIZE = 200;
     const Page = this.crowi.model('Page');
 

packages/core/src/utils/page-path-utils.ts

@@ -169,8 +169,9 @@ export const collectAncestorPaths = (path: string, ancestorPaths: string[] = [])
  * @returns omitted paths
  */
 export const omitDuplicateAreaPathFromPaths = (paths: string[]): string[] => {
-  return paths.filter((path) => {
-    const isDuplicate = paths.filter(p => (new RegExp(`^${p}\\/.+`, 'i')).test(path)).length > 0;
+  const uniquePaths = Array.from(new Set(paths));
+  return uniquePaths.filter((path) => {
+    const isDuplicate = uniquePaths.filter(p => (new RegExp(`^${p}\\/.+`, 'i')).test(path)).length > 0;
 
     return !isDuplicate;
   });
@@ -178,12 +179,13 @@ export const omitDuplicateAreaPathFromPaths = (paths: string[]): string[] => {
 
 /**
  * return pages with path without duplicate area of regexp /^${path}\/.+/i
+ * if the pages' path are the same, it will NOT omit any of them since the other attributes will not be the same
  * @param paths paths to be tested
  * @returns omitted paths
  */
 export const omitDuplicateAreaPageFromPages = (pages: any[]): any[] => {
   return pages.filter((page) => {
-    const isDuplicate = pages.filter(p => (new RegExp(`^${p.path}\\/.+`, 'i')).test(page.path)).length > 0;
+    const isDuplicate = pages.some(p => (new RegExp(`^${p.path}\\/.+`, 'i')).test(page.path));
 
     return !isDuplicate;
   });