diff --git a/app/api/auth2fa/specs/__snapshots__/usersUtils.spec.ts.snap b/app/api/auth2fa/specs/__snapshots__/usersUtils.spec.ts.snap index b51989c3f3..837d354f9c 100644 --- 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 index 82903adb2d..8052eca7d9 100644 --- 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('entities', mongoSchema); +const Model = instanceModelWithPermissions('entities', mongoSchema, { + optimisticLock: true, +}); const supportedLanguages = [ 'da', diff --git a/app/api/odm/ModelWithPermissions.ts b/app/api/odm/ModelWithPermissions.ts index b46742f064..a14749d6f7 100644 --- a/app/api/odm/ModelWithPermissions.ts +++ b/app/api/odm/ModelWithPermissions.ts @@ -202,10 +202,11 @@ export class ModelWithPermissions extends OdmModel> { export function instanceModelWithPermissions( collectionName: string, - schema: mongoose.Schema + schema: mongoose.Schema, + options: { optimisticLock: boolean } = { optimisticLock: false } ) { const logHelper = createUpdateLogHelper(collectionName); - const model = new ModelWithPermissions(logHelper, collectionName, schema); + const model = new ModelWithPermissions(logHelper, collectionName, schema, options); models[collectionName] = () => model; return model; } diff --git a/app/api/odm/model.ts b/app/api/odm/model.ts index 19af4f6dac..5cb1cf2690 100644 --- 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 = (UpdateOptions & Omit, 'lean'>) | null; export class OdmModel implements SyncDBDataSource { + private collectionName: string; + db: MultiTenantMongooseModel; logHelper: UpdateLogger; + options: { optimisticLock: boolean }; + + constructor( + logHelper: UpdateLogger, + collectionName: string, + schema: Schema, + options: { optimisticLock: boolean } = { optimisticLock: false } + ) { + this.collectionName = collectionName; + this.db = new MultiTenantMongooseModel(collectionName, schema); + this.logHelper = logHelper; + this.options = options; + } + private documentExists(data: Partial>) { return this.db.findById(data._id, '_id'); } - constructor(logHelper: UpdateLogger, collectionName: string, schema: Schema) { - this.db = new MultiTenantMongooseModel(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>) { + 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>, query?: any) { + async save(data: Partial>, _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>, - { - new: true, - } + query, + { $set: toSaveData as UwaziUpdateQuery>, $inc: { __v: 1 } }, + { new: true } ); + if (saved === null) { throw Error('The document was not updated!'); } + await this.logHelper.upsertLogOne(saved); return saved.toObject>(); } + return this.create(data); + } + + async create(data: Partial>) { const saved = await this.db.create(data); await this.logHelper.upsertLogOne(saved); return saved.toObject>(); @@ -181,9 +238,13 @@ export class OdmModel implements SyncDBDataSource { // export const models: { [index: string]: OdmModel } = {}; export const models: { [index: string]: () => SyncDBDataSource } = {}; -export function instanceModel(collectionName: string, schema: Schema) { +export function instanceModel( + collectionName: string, + schema: Schema, + options: { optimisticLock: boolean } = { optimisticLock: false } +) { const logHelper = createUpdateLogHelper(collectionName); - const model = new OdmModel(logHelper, collectionName, schema); + const model = new OdmModel(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 index 9ee53ac486..5f36b2c449 100644 --- 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(collectionName, schema); + const model = instanceModel(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 index fc2d495c3f..7ef0a46dff 100644 --- 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',