Browse Source

Fix migration references and enhance normalization logic for revisions affected by migration scripts

Yuki Takei 3 months ago
parent
commit
74fcde77bf

+ 4 - 1
apps/app/src/migrations/20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js → apps/app/src/migrations/20211227060705-revision-path-to-page-id-schema-migration--fixed-8998.js

@@ -13,11 +13,14 @@ import {
 import loggerFactory from '~/utils/logger';
 
 const logger = loggerFactory(
-  'growi:migrate:revision-path-to-page-id-schema-migration--fixed-7549',
+  'growi:migrate:revision-path-to-page-id-schema-migration--fixed-8998',
 );
 
 const LIMIT = 300;
 
+/**
+ * @see https://dev.growi.org/69301054963f68dfcf2b7111
+ */
 module.exports = {
   // path => pageId
   async up(db, client) {

+ 1 - 1
apps/app/src/server/routes/apiv3/page/index.ts

@@ -1041,7 +1041,7 @@ module.exports = (crowi: Crowi) => {
         return res.apiv3Err(err, 500);
       }
 
-      // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js'
+      // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js' provided by v6.1.0 - v7.0.15
       try {
         await normalizeLatestRevisionIfBroken(pageId);
       } catch (err) {

+ 1 - 1
apps/app/src/server/routes/apiv3/page/update-page.ts

@@ -238,7 +238,7 @@ export const updatePageHandlersFactory: UpdatePageHandlersFactory = (crowi) => {
       }
 
       if (currentPage != null) {
-        // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js'
+        // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js' provided by v6.1.0 - v7.0.15
         try {
           await normalizeLatestRevisionIfBroken(pageId);
         } catch (err) {

+ 7 - 26
apps/app/src/server/routes/apiv3/revisions.js

@@ -2,11 +2,13 @@ import { SCOPE } from '@growi/core/dist/interfaces';
 import { ErrorV3 } from '@growi/core/dist/models';
 import { serializeUserSecurely } from '@growi/core/dist/models/serializers';
 import express from 'express';
-import { connection } from 'mongoose';
 
 import { accessTokenParser } from '~/server/middlewares/access-token-parser';
 import { Revision } from '~/server/models/revision';
-import { normalizeLatestRevisionIfBroken } from '~/server/service/revision/normalize-latest-revision-if-broken';
+import {
+  getAppliedAtForRevisionFilter,
+  normalizeLatestRevisionIfBroken,
+} from '~/server/service/revision/normalize-latest-revision-if-broken';
 import loggerFactory from '~/utils/logger';
 
 import { apiV3FormValidator } from '../../middlewares/apiv3-form-validator';
@@ -17,9 +19,6 @@ const { query, param } = require('express-validator');
 
 const router = express.Router();
 
-const MIGRATION_FILE_NAME =
-  '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549';
-
 /**
  * @swagger
  * components:
@@ -85,24 +84,6 @@ module.exports = (crowi) => {
     ],
   };
 
-  let cachedAppliedAt = null;
-
-  const getAppliedAtOfTheMigrationFile = async () => {
-    if (cachedAppliedAt != null) {
-      return cachedAppliedAt;
-    }
-
-    const migrationCollection = connection.collection('migrations');
-    const migration = await migrationCollection.findOne({
-      fileName: { $regex: `^${MIGRATION_FILE_NAME}` },
-    });
-    const appliedAt = migration.appliedAt;
-
-    cachedAppliedAt = appliedAt;
-
-    return appliedAt;
-  };
-
   /**
    * @swagger
    *
@@ -176,7 +157,7 @@ module.exports = (crowi) => {
         );
       }
 
-      // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js'
+      // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js' provided by v6.1.0 - v7.0.15
       try {
         await normalizeLatestRevisionIfBroken(pageId);
       } catch (err) {
@@ -186,7 +167,7 @@ module.exports = (crowi) => {
       try {
         const page = await Page.findOne({ _id: pageId });
 
-        const appliedAt = await getAppliedAtOfTheMigrationFile();
+        const appliedAt = await getAppliedAtForRevisionFilter();
 
         const queryOpts = {
           offset,
@@ -202,7 +183,7 @@ module.exports = (crowi) => {
 
         const queryCondition = {
           pageId: page._id,
-          createdAt: { $gt: appliedAt },
+          ...(appliedAt != null && { createdAt: { $gt: appliedAt } }),
         };
 
         // https://redmine.weseek.co.jp/issues/151652

+ 115 - 2
apps/app/src/server/service/revision/normalize-latest-revision-if-broken.integ.ts

@@ -1,16 +1,37 @@
 import { getIdStringForRef } from '@growi/core';
 import type { HydratedDocument } from 'mongoose';
-import mongoose, { Types } from 'mongoose';
+import mongoose, { connection, Types } from 'mongoose';
 
 import type { PageDocument, PageModel } from '~/server/models/page';
 import PageModelFactory from '~/server/models/page';
 import { Revision } from '~/server/models/revision';
 
-import { normalizeLatestRevisionIfBroken } from './normalize-latest-revision-if-broken';
+import {
+  __resetCacheForTesting,
+  getAppliedAtForRevisionFilter,
+  normalizeLatestRevisionIfBroken,
+} from './normalize-latest-revision-if-broken';
+
+const OLD_MIGRATION_FILE_NAME =
+  '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549';
+const NEW_MIGRATION_FILE_NAME =
+  '20211227060705-revision-path-to-page-id-schema-migration--fixed-8998';
 
 describe('normalizeLatestRevisionIfBroken', () => {
   beforeAll(async () => {
     await PageModelFactory(null);
+    // Insert migration record to simulate affected instance
+    const migrationCollection = connection.collection('migrations');
+    await migrationCollection.insertOne({
+      fileName: OLD_MIGRATION_FILE_NAME,
+      appliedAt: new Date('2024-01-01'),
+    });
+  });
+
+  afterAll(async () => {
+    // Clean up migration record
+    const migrationCollection = connection.collection('migrations');
+    await migrationCollection.deleteOne({ fileName: OLD_MIGRATION_FILE_NAME });
   });
 
   test('should update the latest revision', async () => {
@@ -131,3 +152,95 @@ describe('normalizeLatestRevisionIfBroken', () => {
     });
   });
 });
+
+describe('getAppliedAtForRevisionFilter', () => {
+  const migrationCollection = () => connection.collection('migrations');
+
+  beforeEach(() => {
+    __resetCacheForTesting();
+  });
+
+  afterEach(async () => {
+    // Clean up all migration records
+    await migrationCollection().deleteMany({
+      fileName: { $in: [OLD_MIGRATION_FILE_NAME, NEW_MIGRATION_FILE_NAME] },
+    });
+  });
+
+  test('should return null when only new migration exists (fresh installation)', async () => {
+    // Arrange
+    await migrationCollection().insertOne({
+      fileName: NEW_MIGRATION_FILE_NAME,
+      appliedAt: new Date('2024-06-01'),
+    });
+
+    // Act
+    const result = await getAppliedAtForRevisionFilter();
+
+    // Assert
+    expect(result).toBeNull();
+  });
+
+  test('should return appliedAt when old migration exists (affected instance)', async () => {
+    // Arrange
+    const appliedAt = new Date('2024-01-01');
+    await migrationCollection().insertOne({
+      fileName: OLD_MIGRATION_FILE_NAME,
+      appliedAt,
+    });
+
+    // Act
+    const result = await getAppliedAtForRevisionFilter();
+
+    // Assert
+    expect(result).toEqual(appliedAt);
+  });
+
+  test('should return appliedAt when both migrations exist (upgraded instance)', async () => {
+    // Arrange
+    const oldAppliedAt = new Date('2024-01-01');
+    await migrationCollection().insertOne({
+      fileName: OLD_MIGRATION_FILE_NAME,
+      appliedAt: oldAppliedAt,
+    });
+    await migrationCollection().insertOne({
+      fileName: NEW_MIGRATION_FILE_NAME,
+      appliedAt: new Date('2024-06-01'),
+    });
+
+    // Act
+    const result = await getAppliedAtForRevisionFilter();
+
+    // Assert
+    expect(result).toEqual(oldAppliedAt);
+  });
+
+  test('should return null when neither migration exists', async () => {
+    // Arrange - no migrations inserted
+
+    // Act
+    const result = await getAppliedAtForRevisionFilter();
+
+    // Assert
+    expect(result).toBeNull();
+  });
+
+  test('should cache the result', async () => {
+    // Arrange
+    const appliedAt = new Date('2024-01-01');
+    await migrationCollection().insertOne({
+      fileName: OLD_MIGRATION_FILE_NAME,
+      appliedAt,
+    });
+
+    // Act - call twice
+    const result1 = await getAppliedAtForRevisionFilter();
+    // Remove the migration record
+    await migrationCollection().deleteOne({ fileName: OLD_MIGRATION_FILE_NAME });
+    const result2 = await getAppliedAtForRevisionFilter();
+
+    // Assert - both should return the same cached value
+    expect(result1).toEqual(appliedAt);
+    expect(result2).toEqual(appliedAt);
+  });
+});

+ 87 - 1
apps/app/src/server/service/revision/normalize-latest-revision-if-broken.ts

@@ -9,14 +9,100 @@ const logger = loggerFactory(
   'growi:service:revision:normalize-latest-revision',
 );
 
+const OLD_MIGRATION_FILE_NAME =
+  '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549';
+const NEW_MIGRATION_FILE_NAME =
+  '20211227060705-revision-path-to-page-id-schema-migration--fixed-8998';
+
+let cachedAppliedAt: Date | null | undefined;
+let cachedIsAffected: boolean | undefined;
+
+/**
+ * Reset the cache for testing purposes.
+ * @internal This function is only for testing.
+ */
+export const __resetCacheForTesting = (): void => {
+  cachedAppliedAt = undefined;
+  cachedIsAffected = undefined;
+};
+
 /**
- * Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js'
+ * Check if this instance went through the problematic migration (v6.1.0 - v7.0.15).
+ *
+ * Condition logic:
+ * - If old migration (fixed-7549) does NOT exist AND new migration (fixed-8998) exists
+ *   → Return false (fresh installation, not affected)
+ * - If old migration (fixed-7549) exists
+ *   → Return true (went through problematic version, affected)
+ * - If neither migration exists
+ *   → Log warning and return false (not affected)
+ *
+ * @returns true if affected by the problematic migration, false otherwise
+ *
+ * @see https://dev.growi.org/69301054963f68dfcf2b7111
+ */
+const isAffectedByProblematicMigration = async (): Promise<boolean> => {
+  if (cachedIsAffected !== undefined) {
+    return cachedIsAffected;
+  }
+
+  const migrationCollection = mongoose.connection.collection('migrations');
+
+  const oldMigration = await migrationCollection.findOne({
+    fileName: { $regex: `^${OLD_MIGRATION_FILE_NAME}` },
+  });
+  const newMigration = await migrationCollection.findOne({
+    fileName: { $regex: `^${NEW_MIGRATION_FILE_NAME}` },
+  });
+
+  // Case: fresh installation (new migration only) → not affected
+  if (oldMigration == null && newMigration != null) {
+    cachedIsAffected = false;
+    cachedAppliedAt = null;
+    return false;
+  }
+
+  // Case: went through problematic version (old migration exists) → affected
+  if (oldMigration != null) {
+    cachedIsAffected = true;
+    cachedAppliedAt = oldMigration.appliedAt;
+    return true;
+  }
+
+  // Case: neither migration exists (unexpected, but handle gracefully) → not affected
+  logger.warn(
+    'Neither old nor new migration file found in migrations collection. This may indicate an incomplete migration state.',
+  );
+  cachedIsAffected = false;
+  cachedAppliedAt = null;
+  return false;
+};
+
+/**
+ * Get the appliedAt date for filtering revisions created before the problematic migration.
+ *
+ * @returns appliedAt date to filter revisions, or null if no filter is needed
+ *
+ * @see https://dev.growi.org/69301054963f68dfcf2b7111
+ */
+export const getAppliedAtForRevisionFilter = async (): Promise<Date | null> => {
+  await isAffectedByProblematicMigration();
+  return cachedAppliedAt ?? null;
+};
+
+/**
+ * Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js' provided by v6.1.0 - v7.0.15
  *
  * @ref https://github.com/growilabs/growi/pull/8998
  */
 export const normalizeLatestRevisionIfBroken = async (
   pageId: string | Types.ObjectId,
 ): Promise<void> => {
+  // Skip if not affected by the problematic migration
+  if (!(await isAffectedByProblematicMigration())) {
+    return;
+  }
+
   if (await Revision.exists({ pageId: { $eq: pageId } })) {
     return;
   }

+ 1 - 1
apps/app/src/server/service/yjs/sync-ydoc.ts

@@ -27,7 +27,7 @@ export const syncYDoc = async (
 ): Promise<void> => {
   const pageId = doc.name;
 
-  // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js'
+  // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js' provided by v6.1.0 - v7.0.15
   await normalizeLatestRevisionIfBroken(pageId);
 
   const revision = await Revision.findOne(

+ 1 - 1
apps/app/src/server/service/yjs/yjs.ts

@@ -143,7 +143,7 @@ class YjsService implements IYjsService {
       );
     };
 
-    // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js'
+    // Normalize the latest revision which was borken by the migration script '20211227060705-revision-path-to-page-id-schema-migration--fixed-7549.js' provided by v6.1.0 - v7.0.15
     await normalizeLatestRevisionIfBroken(pageId);
 
     // get the latest revision createdAt