From b86acf3ea1429a4c22f71b69ed792cabeb5b4203 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 10 Sep 2020 14:18:14 +0100 Subject: [PATCH] [misc] mongodb: change findAndModify queries to updateOne There is no need to fetch mongo data when the call-site does not look at it. Also handle mongo errors in `setTTLOnArchivedPack`. --- services/track-changes/app/js/PackManager.js | 67 ++++++++----------- .../unit/js/PackManager/PackManagerTests.js | 19 +++--- 2 files changed, 37 insertions(+), 49 deletions(-) diff --git a/services/track-changes/app/js/PackManager.js b/services/track-changes/app/js/PackManager.js index 5d753aebb2..14df91d074 100644 --- a/services/track-changes/app/js/PackManager.js +++ b/services/track-changes/app/js/PackManager.js @@ -273,10 +273,7 @@ module.exports = PackManager = { 'appending updates to existing pack' ) Metrics.inc(`append-pack-${temporary ? 'temporary' : 'permanent'}`) - return db.docHistory.findAndModify( - { query, update, new: true, fields: { meta: 1, v_end: 1 } }, - callback - ) + return db.docHistory.updateOne(query, update, callback) }, // Retrieve all changes for a document @@ -497,11 +494,9 @@ module.exports = PackManager = { increaseTTL(pack, callback) { if (pack.expiresAt < new Date(Date.now() + 6 * DAYS)) { // update cache expiry since we are using this pack - return db.docHistory.findAndModify( - { - query: { _id: pack._id }, - update: { $set: { expiresAt: new Date(Date.now() + 7 * DAYS) } } - }, + return db.docHistory.updateOne( + { _id: pack._id }, + { $set: { expiresAt: new Date(Date.now() + 7 * DAYS) } }, (err) => callback(err, pack) ) } else { @@ -727,15 +722,15 @@ module.exports = PackManager = { }, _insertPacksIntoIndex(project_id, doc_id, newPacks, callback) { - return db.docHistoryIndex.findAndModify( + return db.docHistoryIndex.updateOne( + { _id: ObjectId(doc_id.toString()) }, + { + $setOnInsert: { project_id: ObjectId(project_id.toString()) }, + $push: { + packs: { $each: newPacks, $sort: { v: 1 } } + } + }, { - query: { _id: ObjectId(doc_id.toString()) }, - update: { - $setOnInsert: { project_id: ObjectId(project_id.toString()) }, - $push: { - packs: { $each: newPacks, $sort: { v: 1 } } - } - }, upsert: true }, callback @@ -993,11 +988,9 @@ module.exports = PackManager = { _markPackAsFinalised(project_id, doc_id, pack_id, callback) { logger.log({ project_id, doc_id, pack_id }, 'marking pack as finalised') - return db.docHistory.findAndModify( - { - query: { _id: pack_id }, - update: { $set: { finalised: true } } - }, + return db.docHistory.updateOne( + { _id: pack_id }, + { $set: { finalised: true } }, callback ) }, @@ -1018,11 +1011,9 @@ module.exports = PackManager = { markPackAsChecked(project_id, doc_id, pack_id, callback) { logger.log({ project_id, doc_id, pack_id }, 'marking pack as checked') - return db.docHistory.findAndModify( - { - query: { _id: pack_id }, - update: { $currentDate: { last_checked: true } } - }, + return db.docHistory.updateOne( + { _id: pack_id }, + { $currentDate: { last_checked: true } }, callback ) }, @@ -1115,15 +1106,12 @@ module.exports = PackManager = { { project_id, doc_id, pack_id }, 'clearing as archive in progress' ) - return db.docHistoryIndex.findAndModify( + return db.docHistoryIndex.updateOne( { - query: { - _id: ObjectId(doc_id.toString()), - packs: { $elemMatch: { _id: pack_id, inS3: false } } - }, - fields: { 'packs.$': 1 }, - update: { $unset: { 'packs.$.inS3': true } } + _id: ObjectId(doc_id.toString()), + packs: { $elemMatch: { _id: pack_id, inS3: false } } }, + { $unset: { 'packs.$.inS3': true } }, callback ) }, @@ -1153,12 +1141,13 @@ module.exports = PackManager = { }, setTTLOnArchivedPack(project_id, doc_id, pack_id, callback) { - return db.docHistory.findAndModify( - { - query: { _id: pack_id }, - update: { $set: { expiresAt: new Date(Date.now() + 1 * DAYS) } } - }, + return db.docHistory.updateOne( + { _id: pack_id }, + { $set: { expiresAt: new Date(Date.now() + 1 * DAYS) } }, function (err) { + if (err) { + return callback(err) + } logger.log({ project_id, doc_id, pack_id }, 'set expiry on pack') return callback() } diff --git a/services/track-changes/test/unit/js/PackManager/PackManagerTests.js b/services/track-changes/test/unit/js/PackManager/PackManagerTests.js index 6a5708fd9f..a34ccbfa07 100644 --- a/services/track-changes/test/unit/js/PackManager/PackManagerTests.js +++ b/services/track-changes/test/unit/js/PackManager/PackManagerTests.js @@ -68,6 +68,7 @@ describe('PackManager', function () { return (this.db.docHistory = { save: sinon.stub().callsArg(1), insert: sinon.stub().callsArg(1), + updateOne: sinon.stub().yields(), findAndModify: sinon.stub().callsArg(1) }) }) @@ -443,23 +444,21 @@ describe('PackManager', function () { return describe('for a small update that will expire', function () { it('should append the update in mongo', function () { - return this.db.docHistory.findAndModify - .calledWithMatch({ - query: { _id: this.lastUpdate._id }, - update: { + return this.db.docHistory.updateOne + .calledWithMatch( + { _id: this.lastUpdate._id }, + { $push: { pack: { $each: this.newUpdates } }, $set: { v_end: this.newUpdates[this.newUpdates.length - 1].v } } - }) + ) .should.equal(true) }) it('should set an expiry time in the future', function () { - return this.db.docHistory.findAndModify - .calledWithMatch({ - update: { - $set: { expiresAt: new Date(Date.now() + 7 * 24 * 3600 * 1000) } - } + return this.db.docHistory.updateOne + .calledWithMatch(sinon.match.any, { + $set: { expiresAt: new Date(Date.now() + 7 * 24 * 3600 * 1000) } }) .should.equal(true) })