Optimistic lock first iteration (#7445)

* Some debug logs to know more about potential implications of optimistic lock on v1 models * include __v as one of the entity base properties * Option to activate optimistic lock per v1 model only entities model has this activated for now
huridocs · Nov 18, 2024 · 719f8c9 · 719f8c9
1 parent f3f6bbe
commit 719f8c9
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 19 deletions.
diff --git a/app/api/auth2fa/specs/__snapshots__/usersUtils.spec.ts.snap b/app/api/auth2fa/specs/__snapshots__/usersUtils.spec.ts.snap
@@ -2,6 +2,7 @@
 
 exports[`auth2fa userUtils reset2fa should set "using2fa" to false, and delete secret for sent user 1`] = `
 Object {
+  "__v": 2,
   "email": "another@email.com",
   "role": "admin",
   "username": "otheruser",

diff --git a/app/api/entities/entitiesModel.ts b/app/api/entities/entitiesModel.ts
@@ -37,7 +37,9 @@ const mongoSchema = new mongoose.Schema(
 mongoSchema.index({ title: 'text' }, { language_override: 'mongoLanguage' });
 mongoSchema.index({ template: 1, language: 1, published: 1 });
 
-const Model = instanceModelWithPermissions<EntitySchema>('entities', mongoSchema);
+const Model = instanceModelWithPermissions<EntitySchema>('entities', mongoSchema, {
+  optimisticLock: true,
+});
 
 const supportedLanguages = [
   'da',

diff --git a/app/api/odm/ModelWithPermissions.ts b/app/api/odm/ModelWithPermissions.ts
@@ -202,10 +202,11 @@ export class ModelWithPermissions<T> extends OdmModel<WithPermissions<T>> {
 
 export function instanceModelWithPermissions<T = any>(
   collectionName: string,
-  schema: mongoose.Schema
+  schema: mongoose.Schema,
+  options: { optimisticLock: boolean } = { optimisticLock: false }
 ) {
   const logHelper = createUpdateLogHelper<T>(collectionName);
-  const model = new ModelWithPermissions<T>(logHelper, collectionName, schema);
+  const model = new ModelWithPermissions<T>(logHelper, collectionName, schema, options);
   models[collectionName] = () => model;
   return model;
 }
diff --git a/app/api/odm/model.ts b/app/api/odm/model.ts
@@ -1,4 +1,5 @@
 import { SyncDBDataSource } from 'api/common.v2/database/SyncDBDataSource';
+import { legacyLogger } from 'api/log';
 import { ObjectId, UpdateOptions } from 'mongodb';
 import mongoose, {
   FilterQuery,
@@ -8,6 +9,7 @@ import mongoose, {
   UpdateQuery,
 } from 'mongoose';
 import { ObjectIdSchema } from 'shared/types/commonTypes';
+import { inspect } from 'util';
 import { MultiTenantMongooseModel } from './MultiTenantMongooseModel';
 import { UpdateLogger, createUpdateLogHelper } from './logHelper';
 import { ModelBulkWriteStream } from './modelBulkWriteStream';
@@ -31,34 +33,89 @@ export type UwaziQueryOptions = QueryOptions;
 export type UwaziUpdateOptions<T> = (UpdateOptions & Omit<MongooseQueryOptions<T>, 'lean'>) | null;
 
 export class OdmModel<T> implements SyncDBDataSource<T, T> {
+  private collectionName: string;
+
   db: MultiTenantMongooseModel<T>;
 
   logHelper: UpdateLogger<T>;
 
+  options: { optimisticLock: boolean };
+
+  constructor(
+    logHelper: UpdateLogger<T>,
+    collectionName: string,
+    schema: Schema,
+    options: { optimisticLock: boolean } = { optimisticLock: false }
+  ) {
+    this.collectionName = collectionName;
+    this.db = new MultiTenantMongooseModel<T>(collectionName, schema);
+    this.logHelper = logHelper;
+    this.options = options;
+  }
+
   private documentExists(data: Partial<DataType<T>>) {
     return this.db.findById(data._id, '_id');
   }
 
-  constructor(logHelper: UpdateLogger<T>, collectionName: string, schema: Schema) {
-    this.db = new MultiTenantMongooseModel<T>(collectionName, schema);
-    this.logHelper = logHelper;
+  private async documentExistsByQuery(query: any = undefined) {
+    const existsByQuery = await this.db.findOne(query, '_id');
+    if (query && !existsByQuery) {
+      throw Error('The document was not updated!');
+    }
+    return true;
+  }
+
+  private async checkVersion(query: any, version: number, data: Partial<DataType<T>>) {
+    if (!this.options.optimisticLock) {
+      return;
+    }
+    if (version === undefined) {
+      legacyLogger.debug(
+        inspect(
+          new Error(
+            `[Optimistic lock] __v not sent for ${this.collectionName} collection with _id ${data._id}`
+          )
+        )
+      );
+      return;
+    }
+    const docMatches = await this.db.findOne({ ...query, __v: version }, '_id');
+    if (!docMatches) {
+      legacyLogger.debug(
+        inspect(
+          new Error(
+            `[Optimistic lock] version conflict '${version}' for ${this.collectionName} collection with _id ${data._id}`
+          )
+        )
+      );
+    }
   }
 
-  async save(data: Partial<DataType<T>>, query?: any) {
+  async save(data: Partial<DataType<T>>, _query?: any) {
     if (await this.documentExists(data)) {
+      // @ts-ignore
+      const { __v: version, ...toSaveData } = data;
+      const query =
+        _query && (await this.documentExistsByQuery(_query)) ? _query : { _id: data._id };
+
+      await this.checkVersion(query, version, data);
       const saved = await this.db.findOneAndUpdate(
-        query || { _id: data._id },
-        data as UwaziUpdateQuery<DataType<T>>,
-        {
-          new: true,
-        }
+        query,
+        { $set: toSaveData as UwaziUpdateQuery<DataType<T>>, $inc: { __v: 1 } },
+        { new: true }
       );
+
       if (saved === null) {
         throw Error('The document was not updated!');
       }
+
       await this.logHelper.upsertLogOne(saved);
       return saved.toObject<WithId<T>>();
     }
+    return this.create(data);
+  }
+
+  async create(data: Partial<DataType<T>>) {
     const saved = await this.db.create(data);
     await this.logHelper.upsertLogOne(saved);
     return saved.toObject<WithId<T>>();
@@ -181,9 +238,13 @@ export class OdmModel<T> implements SyncDBDataSource<T, T> {
 // export const models: { [index: string]: OdmModel<any> } = {};
 export const models: { [index: string]: () => SyncDBDataSource<any, any> } = {};
 
-export function instanceModel<T = any>(collectionName: string, schema: Schema) {
+export function instanceModel<T = any>(
+  collectionName: string,
+  schema: Schema,
+  options: { optimisticLock: boolean } = { optimisticLock: false }
+) {
   const logHelper = createUpdateLogHelper<T>(collectionName);
-  const model = new OdmModel<T>(logHelper, collectionName, schema);
+  const model = new OdmModel<T>(logHelper, collectionName, schema, options);
   models[collectionName] = () => model;
   return model;
 }
diff --git a/app/api/odm/specs/model.spec.ts b/app/api/odm/specs/model.spec.ts
@@ -1,17 +1,19 @@
+import { legacyLogger } from 'api/log';
 import { UpdateLogHelper } from 'api/odm/logHelper';
+import { tenants } from 'api/tenants';
 import { model as updatelogsModel } from 'api/updatelogs';
 import { UpdateLog } from 'api/updatelogs/updatelogsModel';
 import { testingTenants } from 'api/utils/testingTenants';
 import testingDB from 'api/utils/testing_db';
 import mongoose, { Schema } from 'mongoose';
 import { ensure } from 'shared/tsUtils';
-import { tenants } from 'api/tenants';
-import { instanceModel, OdmModel, models, WithId } from '../model';
+import { instanceModel, models, OdmModel, WithId } from '../model';
 
 const testSchema = new Schema({
   name: String,
   value: String,
 });
+
 interface TestDoc {
   name: string;
   value?: string;
@@ -21,7 +23,7 @@ describe('ODM Model', () => {
   const originalDatenow = Date.now;
 
   beforeEach(async () => {
-    await testingDB.clearAllAndLoad({});
+    await testingDB.setupFixturesAndContext({});
   });
 
   afterAll(async () => {
@@ -30,7 +32,7 @@ describe('ODM Model', () => {
   });
 
   const instanceTestingModel = (collectionName: string, schema: Schema) => {
-    const model = instanceModel<TestDoc>(collectionName, schema);
+    const model = instanceModel<TestDoc>(collectionName, schema, { optimisticLock: true });
     tenants.add(
       testingTenants.createTenant({
         name: testingDB.dbName,
@@ -65,12 +67,42 @@ describe('ODM Model', () => {
       expect(createdDocument).toBeDefined();
       expect(createdDocument.name).toEqual('document 1');
 
-      await extendedModel.save({ _id: id, value: 'abc' });
+      await extendedModel.save(Object.assign(createdDocument, { value: 'abc' }));
       const [updatedDoc] = await extendedModel.get({ _id: savedDoc._id });
       expect(updatedDoc).toBeDefined();
       expect(updatedDoc.name).toEqual('document 1');
       expect(updatedDoc.value).toEqual('abc');
     });
+
+    describe('when updating (_id is provided and it already exists)', () => {
+      it('should log a conflict error when trying to update a document with a different version', async () => {
+        const debugSpy = jest.spyOn(legacyLogger, 'debug');
+        const extendedModel = instanceTestingModel('tempSchema', testSchema);
+        const id = testingDB.id();
+
+        const savedDoc = await extendedModel.save({
+          _id: id,
+          name: 'document 1',
+        });
+        await extendedModel.save(savedDoc);
+        await extendedModel.save(savedDoc);
+        expect(debugSpy.mock.calls[0][0]).toContain('version conflict');
+      });
+
+      it('should log when "__v" is not sent as part of the data', async () => {
+        const debugSpy = jest.spyOn(legacyLogger, 'debug');
+        debugSpy.mockClear();
+        const extendedModel = instanceTestingModel('tempSchema', testSchema);
+        const id = testingDB.id();
+
+        await extendedModel.save({
+          _id: id,
+          name: 'document 1',
+        });
+        await extendedModel.save({ _id: id, name: 'document 1' });
+        expect(debugSpy.mock.calls[0][0]).toContain('__v not sent');
+      });
+    });
   });
 
   describe('Logging functionality', () => {

diff --git a/app/react/Entities/utils/filterBaseProperties.js b/app/react/Entities/utils/filterBaseProperties.js
@@ -2,6 +2,7 @@ export default {
   filterBaseProperties: data => {
     const properties = [
       '_id',
+      '__v',
       'language',
       'metadata',
       'suggestedMetadata',