imprv: Single rename implementation and test code (#5122)

* Cleaned code

* Typescriptized file

* Improved

* Implemented single rename

* Fixed import

* Uncomment page.test.js

* Uncomment page.test.js

* Removed unnecessary code

* Fixed lint errors

* Removed unnecessary methods

* improved

* Removed isRecursive false process & Added comments for PageRedirect

* Modified comments

packages/app/src/interfaces/page.ts

@@ -5,7 +5,7 @@ import { ITag } from './tag';
 import { HasObjectId } from './has-object-id';
 
 
-export type IPage = {
+export interface IPage {
   path: string,
   status: string,
   revision: Ref<IRevision>,

packages/app/src/server/crowi/index.js

@@ -20,6 +20,7 @@ import AppService from '../service/app';
 import AclService from '../service/acl';
 import SearchService from '../service/search';
 import AttachmentService from '../service/attachment';
+import PageService from '../service/page';
 import PageGrantService from '../service/page-grant';
 import { SlackIntegrationService } from '../service/slack-integration';
 import { UserNotificationService } from '../service/user-notification';
@@ -679,9 +680,8 @@ Crowi.prototype.setupImport = async function() {
 };
 
 Crowi.prototype.setupPageService = async function() {
-  const PageEventService = require('../service/page');
   if (this.pageService == null) {
-    this.pageService = new PageEventService(this);
+    this.pageService = new PageService(this);
   }
   if (this.pageGrantService == null) {
     this.pageGrantService = new PageGrantService(this);

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

@@ -39,7 +39,7 @@ type TargetAndAncestorsResult = {
 export interface PageModel extends Model<PageDocument> {
   [x: string]: any; // for obsolete methods
   createEmptyPagesByPaths(paths: string[], publicOnly?: boolean): Promise<void>
-  getParentIdAndFillAncestors(path: string, parent: (PageDocument & { _id: any }) | null): Promise<string | null>
+  getParentAndFillAncestors(path: string): Promise<PageDocument & { _id: any }>
   findByPathAndViewer(path: string | null, user, userGroups?, useFindOne?: boolean, includeEmpty?: boolean): Promise<PageDocument[]>
   findTargetAndAncestorsByPathOrId(pathOrId: string): Promise<TargetAndAncestorsResult>
   findChildrenByParentPathOrIdAndViewer(parentPathOrId: string, user, userGroups?): Promise<PageDocument[]>
@@ -139,18 +139,77 @@ schema.statics.createEmptyPagesByPaths = async function(paths: string[], publicO
   }
 };
 
-/*
- * Find the parent and update if the parent exists.
- * If not,
- *   - first   run createEmptyPagesByPaths with ancestor's paths to ensure all the ancestors exist
- *   - second  update ancestor pages' parent
- *   - finally return the target's parent page id
+schema.statics.createEmptyPage = async function(
+    path: string, parent: any, // 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');
+  }
+
+  const Page = this;
+  const page = new Page();
+  page.path = path;
+  page.isEmpty = true;
+  page.parent = parent;
+
+  return page.save();
+};
+
+/**
+ * Replace an existing page with an empty page.
+ * It updates the children's parent to the new empty page's _id.
+ * @param exPage a page document to be replaced
+ * @param pageToReplaceWith (optional) a page document to replace with
+ * @returns Promise<void>
  */
-schema.statics.getParentIdAndFillAncestors = async function(path: string, parent: PageDocument | null): Promise<Schema.Types.ObjectId> {
-  const parentPath = nodePath.dirname(path);
+schema.statics.replaceTargetWithPage = async function(exPage, pageToReplaceWith?): Promise<void> {
+  // find parent
+  const parent = await this.findOne({ _id: exPage.parent });
+  if (parent == null) {
+    throw Error('parent to update does not exist. Prepare parent first.');
+  }
+
+  // create empty page at path
+  let newTarget = pageToReplaceWith;
+  if (newTarget) {
+    newTarget = await this.createEmptyPage(exPage.path, parent);
+  }
 
+  // find children by ex-page _id
+  const children = await this.find({ parent: exPage._id });
+
+  // bulkWrite
+  const operationForNewTarget = {
+    updateOne: {
+      filter: { _id: newTarget._id },
+      update: {
+        parent: parent._id,
+      },
+    },
+  };
+  const operationsForChildren = {
+    updateMany: {
+      filter: children.map(d => d._id),
+      update: {
+        parent: newTarget._id,
+      },
+    },
+  };
+
+  await this.bulkWrite([operationForNewTarget, operationsForChildren]);
+};
+
+/**
+ * Find parent or create parent if not exists.
+ * It also updates parent of ancestors
+ * @param path string
+ * @returns Promise<PageDocument>
+ */
+schema.statics.getParentAndFillAncestors = async function(path: string): Promise<PageDocument> {
+  const parentPath = nodePath.dirname(path);
+  const parent = await this.findOne({ path: parentPath }); // find the oldest parent which must always be the true parent
   if (parent != null) {
-    return parent._id;
+    return parent;
   }
 
   /*
@@ -162,16 +221,15 @@ schema.statics.getParentIdAndFillAncestors = async function(path: string, parent
   await this.createEmptyPagesByPaths(ancestorPaths);
 
   // find ancestors
-  const builder = new PageQueryBuilder(this.find({}, { _id: 1, path: 1 }), true);
+  const builder = new PageQueryBuilder(this.find(), true);
   const ancestors = await builder
     .addConditionToListByPathsArray(ancestorPaths)
     .addConditionToSortPagesByDescPath()
     .query
-    .lean()
     .exec();
 
-  const ancestorsMap = new Map(); // Map<path, _id>
-  ancestors.forEach(page => !ancestorsMap.has(page.path) && ancestorsMap.set(page.path, page._id)); // the earlier element should be the true ancestor
+  const ancestorsMap = new Map(); // Map<path, page>
+  ancestors.forEach(page => !ancestorsMap.has(page.path) && ancestorsMap.set(page.path, page)); // the earlier element should be the true ancestor
 
   // bulkWrite to update ancestors
   const nonRootAncestors = ancestors.filter(page => !isTopPage(page.path));
@@ -191,8 +249,9 @@ schema.statics.getParentIdAndFillAncestors = async function(path: string, parent
   });
   await this.bulkWrite(operations);
 
-  const parentId = ancestorsMap.get(parentPath);
-  return parentId;
+  const createdParent = ancestorsMap.get(parentPath);
+
+  return createdParent;
 };
 
 // Utility function to add viewer condition to PageQueryBuilder instance
@@ -508,11 +567,10 @@ export default (crowi: Crowi): any => {
       page = new Page();
     }
 
-    let parentId: string | null = null;
-    const parentPath = nodePath.dirname(path);
-    const parent = await this.findOne({ path: parentPath }); // find the oldest parent which must always be the true parent
+    let parentId: IObjectId | string | null = null;
+    const parent = await Page.getParentAndFillAncestors(path);
     if (!isTopPage(path)) {
-      parentId = await Page.getParentIdAndFillAncestors(path, parent);
+      parentId = parent._id;
     }
 
     page.path = path;

packages/app/src/server/routes/apiv3/page-listing.ts

@@ -98,7 +98,8 @@ export default (crowi: Crowi): Router => {
     const { pageIds } = req.query;
 
     try {
-      const shortBodiesMap = await crowi.pageService.shortBodiesMapByPageIds(pageIds, req.user);
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      const shortBodiesMap = await crowi.pageService!.shortBodiesMapByPageIds(pageIds as string[], req.user);
       return res.apiv3({ shortBodiesMap });
     }
     catch (err) {

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

@@ -178,7 +178,6 @@ module.exports = (crowi) => {
       body('newPagePath').isLength({ min: 1 }).withMessage('newPagePath is required'),
       body('isRenameRedirect').if(value => value != null).isBoolean().withMessage('isRenameRedirect must be boolean'),
       body('isRemainMetadata').if(value => value != null).isBoolean().withMessage('isRemainMetadata must be boolean'),
-      body('isRecursively').if(value => value != null).isBoolean().withMessage('isRecursively must be boolean'),
     ],
 
     duplicatePage: [
@@ -454,7 +453,7 @@ module.exports = (crowi) => {
    *            description: page path is already existed
    */
   router.put('/rename', accessTokenParser, loginRequiredStrictly, csrf, validator.renamePage, apiV3FormValidator, async(req, res) => {
-    const { pageId, isRecursively, revisionId } = req.body;
+    const { pageId, revisionId } = req.body;
 
     let newPagePath = pathUtils.normalizePath(req.body.newPagePath);
 
@@ -464,7 +463,7 @@ module.exports = (crowi) => {
     };
 
     if (!isCreatablePage(newPagePath)) {
-      return res.apiv3Err(new ErrorV3(`Could not use the path '${newPagePath})'`, 'invalid_path'), 409);
+      return res.apiv3Err(new ErrorV3(`Could not use the path '${newPagePath}'`, 'invalid_path'), 409);
     }
 
     // check whether path starts slash
@@ -488,7 +487,7 @@ module.exports = (crowi) => {
       if (!page.isUpdatable(revisionId)) {
         return res.apiv3Err(new ErrorV3('Someone could update this page, so couldn\'t delete.', 'notfound_or_forbidden'), 409);
       }
-      page = await crowi.pageService.renamePage(page, newPagePath, req.user, options, isRecursively);
+      page = await crowi.pageService.renamePage(page, newPagePath, req.user, options);
     }
     catch (err) {
       logger.error(err);

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

@@ -11,6 +11,7 @@ import loggerFactory from '~/utils/logger';
 import { generateGrantCondition, PageModel } from '~/server/models/page';
 import { stringifySnapshot } from '~/models/serializers/in-app-notification-snapshot/page';
 import ActivityDefine from '../util/activityDefine';
+import { IPage } from '~/interfaces/page';
 
 const debug = require('debug')('growi:services:page');
 
@@ -191,21 +192,83 @@ class PageService {
       .cursor({ batchSize: BULK_REINDEX_SIZE });
   }
 
-  async renamePage(page, newPagePath, user, options, isRecursively = false) {
+  // TODO: rewrite recursive rename
+  async renamePage(page, newPagePath, user, options) {
+    // v4 compatible process
+    const isV5Compatible = this.crowi.configManager.getConfig('crowi', 'app:isV5Compatible');
+    if (!isV5Compatible) {
+      return this.renamePageV4(page, newPagePath, user, options);
+    }
 
+    const Page = this.crowi.model('Page');
+    const {
+      path, grant, grantedUsers: grantedUserIds, grantedGroup: grantUserGroupId,
+    } = page;
+    const updateMetadata = options.updateMetadata || false;
+
+    // sanitize path
+    newPagePath = this.crowi.xss.process(newPagePath); // eslint-disable-line no-param-reassign
+
+    /*
+     * UserGroup & Owner validation
+     */
+    if (grant !== Page.GRANT_RESTRICTED) {
+      let isGrantNormalized = false;
+      try {
+        const shouldCheckDescendants = false;
+
+        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(path, grant, grantedUserIds, grantUserGroupId, shouldCheckDescendants);
+      }
+      catch (err) {
+        logger.error(`Failed to validate grant of page at "${newPagePath}" when renaming`, err);
+        throw err;
+      }
+      if (!isGrantNormalized) {
+        throw Error(`This page cannot be renamed to "${newPagePath}" since the selected grant or grantedGroup is not assignable to this page.`);
+      }
+    }
+
+    // update descendants first
+    await this.renameDescendantsWithStream(page, newPagePath, user, options);
+
+    /*
+     * TODO: https://redmine.weseek.co.jp/issues/86577
+     * bulkWrite PageRedirectDocument if createRedirectPage is true
+     */
+
+    /*
+     * update target
+     */
+    const update: Partial<IPage> = {};
+    // find or create parent
+    const newParent = await Page.getParentAndFillAncestors(newPagePath);
+    // update Page
+    update.path = newPagePath;
+    update.parent = newParent._id;
+    if (updateMetadata) {
+      update.lastUpdateUser = user;
+      update.updatedAt = new Date();
+    }
+    const renamedPage = await Page.findByIdAndUpdate(page._id, { $set: update }, { new: true });
+
+    this.pageEvent.emit('rename', page, user);
+
+    return renamedPage;
+  }
+
+  // !!renaming always include descendant pages!!
+  private async renamePageV4(page, newPagePath, user, options) {
     const Page = this.crowi.model('Page');
     const Revision = this.crowi.model('Revision');
     const path = page.path;
-    const createRedirectPage = options.createRedirectPage || false;
     const updateMetadata = options.updateMetadata || false;
 
     // sanitize path
     newPagePath = this.crowi.xss.process(newPagePath); // eslint-disable-line no-param-reassign
 
     // create descendants first
-    if (isRecursively) {
-      await this.renameDescendantsWithStream(page, newPagePath, user, options);
-    }
+    await this.renameDescendantsWithStream(page, newPagePath, user, options);
+
 
     const update: any = {};
     // update Page
@@ -219,10 +282,10 @@ class PageService {
     // update Rivisions
     await Revision.updateRevisionListByPath(path, { path: newPagePath }, {});
 
-    if (createRedirectPage) {
-      const body = `redirect ${newPagePath}`;
-      await Page.create(path, body, user, { redirectTo: newPagePath });
-    }
+    /*
+     * TODO: https://redmine.weseek.co.jp/issues/86577
+     * bulkWrite PageRedirectDocument if createRedirectPage is true
+     */
 
     this.pageEvent.emit('rename', page, user);
 
@@ -231,20 +294,13 @@ class PageService {
 
 
   private async renameDescendants(pages, user, options, oldPagePathPrefix, newPagePathPrefix) {
-    const Page = this.crowi.model('Page');
-
     const pageCollection = mongoose.connection.collection('pages');
-    const revisionCollection = mongoose.connection.collection('revisions');
-    const { updateMetadata, createRedirectPage } = options;
+    const { updateMetadata } = options;
 
     const unorderedBulkOp = pageCollection.initializeUnorderedBulkOp();
-    const createRediectPageBulkOp = pageCollection.initializeUnorderedBulkOp();
-    const revisionUnorderedBulkOp = revisionCollection.initializeUnorderedBulkOp();
-    const createRediectRevisionBulkOp = revisionCollection.initializeUnorderedBulkOp();
 
     pages.forEach((page) => {
       const newPagePath = page.path.replace(oldPagePathPrefix, newPagePathPrefix);
-      const revisionId = new mongoose.Types.ObjectId();
 
       if (updateMetadata) {
         unorderedBulkOp
@@ -254,25 +310,10 @@ class PageService {
       else {
         unorderedBulkOp.find({ _id: page._id }).update({ $set: { path: newPagePath } });
       }
-      if (createRedirectPage) {
-        createRediectPageBulkOp.insert({
-          path: page.path, revision: revisionId, creator: user._id, lastUpdateUser: user._id, status: Page.STATUS_PUBLISHED, redirectTo: newPagePath,
-        });
-        createRediectRevisionBulkOp.insert({
-          _id: revisionId, path: page.path, body: `redirect ${newPagePath}`, author: user._id, format: 'markdown',
-        });
-      }
-      revisionUnorderedBulkOp.find({ path: page.path }).update({ $set: { path: newPagePath } });
     });
 
     try {
       await unorderedBulkOp.execute();
-      await revisionUnorderedBulkOp.execute();
-      // Execute after unorderedBulkOp to prevent duplication
-      if (createRedirectPage) {
-        await createRediectPageBulkOp.execute();
-        await createRediectRevisionBulkOp.execute();
-      }
     }
     catch (err) {
       if (err.code !== 11000) {
@@ -862,7 +903,7 @@ class PageService {
     }
   }
 
-  async shortBodiesMapByPageIds(pageIds = [], user) {
+  async shortBodiesMapByPageIds(pageIds: string[] = [], user) {
     const Page = mongoose.model('Page');
     const MAX_LENGTH = 350;
 

packages/app/src/test/integration/service/page.test.js

@@ -304,9 +304,6 @@ describe('PageService', () => {
       expect(wrongPage).toBeNull();
     });
 
-    /*
-     * TODO: rewrite test when modify rename function
-     */
     test('rename page with different tree with isRecursively [shallower]', async() => {
       // setup
       expect(await Page.findOne({ path: '/level1' })).toBeNull();