From e2120a8f3fb470a8e9f89b5f90f6ac497c514265 Mon Sep 17 00:00:00 2001
From: kirillgroshkov <kirill.groshkov@naturalcycles.com>
Date: Thu, 25 Jan 2024 21:17:46 +0100
Subject: [PATCH] fix: CommonDao.save skipIfEquals compares post-conversion

---
 src/commondao/common.dao.test.ts | 15 +++++++++---
 src/commondao/common.dao.ts      | 39 +++++++++++---------------------
 2 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/src/commondao/common.dao.test.ts b/src/commondao/common.dao.test.ts
index 8cf4a16..a998d5f 100644
--- a/src/commondao/common.dao.test.ts
+++ b/src/commondao/common.dao.test.ts
@@ -7,6 +7,7 @@ import {
   pTry,
   pExpectedError,
   BaseDBEntity,
+  _deepFreeze,
 } from '@naturalcycles/js-lib'
 import {
   AjvSchema,
@@ -25,6 +26,7 @@ import {
   TestItemBM,
   TestItemDBM,
   testItemBMJsonSchema,
+  createTestItemBM,
 } from '../testing'
 import { CommonDao } from './common.dao'
 import { CommonDaoCfg, CommonDaoLogLevel, CommonDaoSaveBatchOptions } from './common.dao.model'
@@ -289,9 +291,7 @@ test('mutation', async () => {
 
   const saved = await dao.save(obj)
 
-  // Should be a new object, not the same (by reference)
-  // Non-mutation should only be ensured inside `validateAndConvert` method
-  // NO: should return the original object
+  // Should return the original object
   // eslint-disable-next-line jest/prefer-equality-matcher
   expect(obj === saved).toBe(true)
 
@@ -299,6 +299,15 @@ test('mutation', async () => {
   expect((obj as any).created).toBe(MOCK_TS_2018_06_21)
 })
 
+test('validateAndConvert does not mutate and returns new reference', async () => {
+  const bm = createTestItemBM()
+  _deepFreeze(bm)
+
+  const bm2 = dao.validateAndConvert(bm, testItemBMSchema)
+  // eslint-disable-next-line jest/prefer-equality-matcher
+  expect(bm === bm2).toBe(false)
+})
+
 test('should preserve null on load and save', async () => {
   const r = await dao.save({
     id: '123',
diff --git a/src/commondao/common.dao.ts b/src/commondao/common.dao.ts
index 1cd1826..dfc519d 100644
--- a/src/commondao/common.dao.ts
+++ b/src/commondao/common.dao.ts
@@ -732,9 +732,15 @@ export class CommonDao<BM extends BaseDBEntity, DBM extends BaseDBEntity = BM> {
   async save(bm: Unsaved<BM>, opt: CommonDaoSaveOptions<BM, DBM> = {}): Promise<BM> {
     this.requireWriteAccess()
 
-    if (opt.skipIfEquals && _deepJsonEquals(bm, opt.skipIfEquals)) {
-      // Skipping the save operation
-      return bm as BM
+    if (opt.skipIfEquals) {
+      // We compare with convertedBM, to account for cases when some extra property is assigned to bm,
+      // which should be removed post-validation, but it breaks the "equality check"
+      // Post-validation the equality check should work as intended
+      const convertedBM = this.validateAndConvert(bm as Partial<BM>, this.cfg.bmSchema, opt)
+      if (_deepJsonEquals(convertedBM, opt.skipIfEquals)) {
+        // Skipping the save operation
+        return bm as BM
+      }
     }
 
     const idWasGenerated = !bm.id && this.cfg.generateId
@@ -1083,15 +1089,9 @@ export class CommonDao<BM extends BaseDBEntity, DBM extends BaseDBEntity = BM> {
     }
 
     // DBM > BM
-    let bm: Partial<BM>
-    if (this.cfg.hooks!.beforeDBMToBM) {
-      bm = await this.cfg.hooks!.beforeDBMToBM(dbm)
-    } else {
-      bm = dbm as any
-    }
+    const bm = ((await this.cfg.hooks!.beforeDBMToBM?.(dbm)) || dbm) as Partial<BM>
 
     // Validate/convert BM
-
     return this.validateAndConvert(bm, this.cfg.bmSchema, opt)
   }
 
@@ -1108,23 +1108,11 @@ export class CommonDao<BM extends BaseDBEntity, DBM extends BaseDBEntity = BM> {
   async bmToDBM(bm?: BM, opt?: CommonDaoOptions): Promise<DBM | undefined> {
     if (bm === undefined) return
 
-    // should not do it on load, but only on save!
-    // this.assignIdCreatedUpdated(bm, opt)
-
     // bm gets assigned to the new reference
     bm = this.validateAndConvert(bm, this.cfg.bmSchema, opt)
 
     // BM > DBM
-    let dbm: DBM
-    if (this.cfg.hooks!.beforeBMToDBM) {
-      dbm = { ...((await this.cfg.hooks!.beforeBMToDBM(bm!)) as DBM) }
-    } else {
-      dbm = bm as any
-    }
-
-    // Validate/convert DBM
-    // return this.validateAndConvert(dbm, this.cfg.dbmSchema, DBModelType.DBM, opt)
-    return dbm
+    return ((await this.cfg.hooks!.beforeBMToDBM?.(bm!)) || bm) as DBM
   }
 
   async bmsToDBM(bms: BM[], opt: CommonDaoOptions = {}): Promise<DBM[]> {
@@ -1158,10 +1146,9 @@ export class CommonDao<BM extends BaseDBEntity, DBM extends BaseDBEntity = BM> {
   }
 
   /**
-   * Returns *converted value*.
-   * Validates (unless `skipValidation=true` passed).
-   *
+   * Returns *converted value* (NOT the same reference).
    * Does NOT mutate the object.
+   * Validates (unless `skipValidation=true` passed).
    */
   validateAndConvert<T>(
     obj: Partial<T>,