From 1740e7efa17a079f8a7b31103c5e245db2f14a60 Mon Sep 17 00:00:00 2001
From: liuyi <forehalo@gmail.com>
Date: Thu, 23 Nov 2023 07:39:02 +0000
Subject: [PATCH] fix(server): check state changes before saving history record
 (#5038)

---
 .../backend/server/src/modules/doc/history.ts | 11 +++
 packages/backend/server/tests/history.spec.ts | 77 +++++++++++++------
 2 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/packages/backend/server/src/modules/doc/history.ts b/packages/backend/server/src/modules/doc/history.ts
index f3cdb8ad5..61ae73e55 100644
--- a/packages/backend/server/src/modules/doc/history.ts
+++ b/packages/backend/server/src/modules/doc/history.ts
@@ -1,3 +1,5 @@
+import { isDeepStrictEqual } from 'node:util';
+
 import { Injectable, Logger } from '@nestjs/common';
 import { OnEvent } from '@nestjs/event-emitter';
 import { Cron, CronExpression } from '@nestjs/schedule';
@@ -41,6 +43,14 @@ export class DocHistoryManager {
     }
 
     if (shouldCreateHistory) {
+      // skip the history recording when no actual update on snapshot happended
+      if (last && isDeepStrictEqual(last.state, snapshot.state)) {
+        this.logger.debug(
+          `State matches, skip creating history record for ${snapshot.id} in workspace ${snapshot.workspaceId}`
+        );
+        return;
+      }
+
       await this.db.snapshotHistory
         .create({
           select: {
@@ -129,6 +139,7 @@ export class DocHistoryManager {
       },
       select: {
         timestamp: true,
+        state: true,
       },
       orderBy: {
         timestamp: 'desc',
diff --git a/packages/backend/server/tests/history.spec.ts b/packages/backend/server/tests/history.spec.ts
index 2ce320797..5555206a3 100644
--- a/packages/backend/server/tests/history.spec.ts
+++ b/packages/backend/server/tests/history.spec.ts
@@ -74,9 +74,9 @@ test('should create doc history if never created before', async t => {
   t.is(history?.timestamp.getTime(), timestamp.getTime());
 });
 
-test('should not create history is timestamp equals to last record', async t => {
+test('should not create history if timestamp equals to last record', async t => {
   const timestamp = new Date();
-  Sinon.stub(manager, 'last').resolves({ timestamp });
+  Sinon.stub(manager, 'last').resolves({ timestamp, state: null });
 
   await manager.onDocUpdated({
     ...snapshot,
@@ -93,10 +93,55 @@ test('should not create history is timestamp equals to last record', async t =>
   t.falsy(history);
 });
 
-test('should create history if time diff is larger than interval config', async t => {
+test('should not create history if state equals to last record', async t => {
+  const timestamp = new Date();
+  Sinon.stub(manager, 'last').resolves({
+    timestamp: new Date(timestamp.getTime() - 1),
+    state: snapshot.state,
+  });
+
+  await manager.onDocUpdated({
+    ...snapshot,
+    updatedAt: timestamp,
+  });
+
+  const history = await db.snapshotHistory.findFirst({
+    where: {
+      workspaceId: '1',
+      id: 'doc1',
+    },
+  });
+
+  t.falsy(history);
+});
+
+test('should not create history if time diff is less than interval config', async t => {
+  const timestamp = new Date();
+  Sinon.stub(manager, 'last').resolves({
+    timestamp: new Date(timestamp.getTime() - 1000),
+    state: Buffer.from([0, 1]),
+  });
+
+  await manager.onDocUpdated({
+    ...snapshot,
+    updatedAt: timestamp,
+  });
+
+  const history = await db.snapshotHistory.findFirst({
+    where: {
+      workspaceId: '1',
+      id: 'doc1',
+    },
+  });
+
+  t.falsy(history);
+});
+
+test('should create history if time diff is larger than interval config and state diff', async t => {
   const timestamp = new Date();
   Sinon.stub(manager, 'last').resolves({
     timestamp: new Date(timestamp.getTime() - 1000 * 60 * 20),
+    state: Buffer.from([0, 1]),
   });
 
   await manager.onDocUpdated({
@@ -114,31 +159,11 @@ test('should create history if time diff is larger than interval config', async
   t.truthy(history);
 });
 
-test('should not create history if time diff is less than interval config', async t => {
-  const timestamp = new Date();
-  Sinon.stub(manager, 'last').resolves({
-    timestamp: new Date(timestamp.getTime() - 1000),
-  });
-
-  await manager.onDocUpdated({
-    ...snapshot,
-    updatedAt: timestamp,
-  });
-
-  const history = await db.snapshotHistory.findFirst({
-    where: {
-      workspaceId: '1',
-      id: 'doc1',
-    },
-  });
-
-  t.falsy(history);
-});
-
 test('should create history with force flag even if time diff in small', async t => {
   const timestamp = new Date();
   Sinon.stub(manager, 'last').resolves({
     timestamp: new Date(timestamp.getTime() - 1),
+    state: Buffer.from([0, 1]),
   });
 
   await manager.onDocUpdated(
@@ -309,4 +334,8 @@ test('should be able to cleanup expired history', async t => {
 
   count = await db.snapshotHistory.count();
   t.is(count, 10);
+
+  const example = await db.snapshotHistory.findFirst();
+  t.truthy(example);
+  t.true(example!.expiredAt > new Date());
 });