From e74af0bee6127fd660ecc79136b633d9320476e6 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 10 Sep 2020 11:10:59 +0100 Subject: [PATCH 1/3] [misc] PackWorker: drop workaround for db.close in mongojs@1 REF: 3ed3932ee837920e95661b0308dee012f40a71dd --- services/track-changes/app/js/PackWorker.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/services/track-changes/app/js/PackWorker.js b/services/track-changes/app/js/PackWorker.js index e6a02fef3d..c0784f097f 100644 --- a/services/track-changes/app/js/PackWorker.js +++ b/services/track-changes/app/js/PackWorker.js @@ -78,18 +78,6 @@ logger.log( `checking for updates, limit=${LIMIT}, delay=${DOCUMENT_PACK_DELAY}, timeout=${TIMEOUT}` ) -// work around for https://github.com/mafintosh/mongojs/issues/224 -db.close = function (callback) { - return this._getServer(function (err, server) { - if (err != null) { - return callback(err) - } - server = server.destroy != null ? server : server.topology - server.destroy(true, true) - return callback() - }) -} - const finish = function () { if (shutDownTimer != null) { logger.log('cancelling timeout') From b86acf3ea1429a4c22f71b69ed792cabeb5b4203 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 10 Sep 2020 14:18:14 +0100 Subject: [PATCH 2/3] [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) }) From 6d3fa2ee5b347a2867d9416a64bfc7cc9f72aa26 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 10 Sep 2020 14:54:18 +0100 Subject: [PATCH 3/3] [misc] mongodb: use findOne and find().toArray() --- services/track-changes/app/js/MongoManager.js | 9 +--- services/track-changes/app/js/PackManager.js | 41 ++++++++----------- services/track-changes/app/js/PackWorker.js | 3 +- .../unit/js/MongoManager/MongoManagerTests.js | 4 +- 4 files changed, 22 insertions(+), 35 deletions(-) diff --git a/services/track-changes/app/js/MongoManager.js b/services/track-changes/app/js/MongoManager.js index 6a84fe7bc1..94ae462722 100644 --- a/services/track-changes/app/js/MongoManager.js +++ b/services/track-changes/app/js/MongoManager.js @@ -115,16 +115,11 @@ module.exports = MongoManager = { if (callback == null) { callback = function (error, metadata) {} } - return db.projectHistoryMetaData.find( + return db.projectHistoryMetaData.findOne( { project_id: ObjectId(project_id.toString()) }, - function (error, results) { - if (error != null) { - return callback(error) - } - return callback(null, results[0]) - } + callback ) }, diff --git a/services/track-changes/app/js/PackManager.js b/services/track-changes/app/js/PackManager.js index 5d753aebb2..418d20283e 100644 --- a/services/track-changes/app/js/PackManager.js +++ b/services/track-changes/app/js/PackManager.js @@ -301,7 +301,8 @@ module.exports = PackManager = { // console.log "query:", query return db.docHistory .find(query) - .sort({ v: -1 }, function (err, result) { + .sort({ v: -1 }) + .toArray(function (err, result) { if (err != null) { return callback(err) } @@ -380,20 +381,9 @@ module.exports = PackManager = { fetchPacksIfNeeded(project_id, doc_id, pack_ids, callback) { let id - return db.docHistory.find( - { - _id: { - $in: (() => { - const result = [] - for (id of Array.from(pack_ids)) { - result.push(ObjectId(id)) - } - return result - })() - } - }, - { _id: 1 }, - function (err, loadedPacks) { + return db.docHistory + .find({ _id: { $in: pack_ids.map(ObjectId) } }, { _id: 1 }) + .toArray(function (err, loadedPacks) { if (err != null) { return callback(err) } @@ -428,8 +418,7 @@ module.exports = PackManager = { return callback() } ) - } - ) + }) }, // Retrieve all changes across a project @@ -438,7 +427,8 @@ module.exports = PackManager = { // get all the docHistory Entries return db.docHistory .find({ project_id: ObjectId(project_id) }, { pack: false }) - .sort({ 'meta.end_ts': -1 }, function (err, packs) { + .sort({ 'meta.end_ts': -1 }) + .toArray(function (err, packs) { let pack if (err != null) { return callback(err) @@ -449,9 +439,9 @@ module.exports = PackManager = { allPacks.push(pack) seenIds[pack._id] = true } - return db.docHistoryIndex.find( - { project_id: ObjectId(project_id) }, - function (err, indexes) { + return db.docHistoryIndex + .find({ project_id: ObjectId(project_id) }) + .toArray(function (err, indexes) { if (err != null) { return callback(err) } @@ -470,8 +460,7 @@ module.exports = PackManager = { null, new ProjectIterator(allPacks, before, PackManager.getPackById) ) - } - ) + }) }) }, @@ -617,7 +606,8 @@ module.exports = PackManager = { } return db.docHistory .find(query, { pack: false }) - .sort({ v: 1 }, function (err, packs) { + .sort({ v: 1 }) + .toArray(function (err, packs) { if (err != null) { return callback(err) } @@ -642,7 +632,8 @@ module.exports = PackManager = { } return db.docHistory .find(query, { pack: false }) - .sort({ v: 1 }, function (err, packs) { + .sort({ v: 1 }) + .toArray(function (err, packs) { if (err != null) { return callback(err) } diff --git a/services/track-changes/app/js/PackWorker.js b/services/track-changes/app/js/PackWorker.js index e6a02fef3d..59df90cf29 100644 --- a/services/track-changes/app/js/PackWorker.js +++ b/services/track-changes/app/js/PackWorker.js @@ -191,7 +191,8 @@ if (pending != null) { .sort({ last_checked: 1 }) - .limit(LIMIT, function (err, results) { + .limit(LIMIT) + .toArray(function (err, results) { if (err != null) { logger.log({ err }, 'error checking for updates') finish() diff --git a/services/track-changes/test/unit/js/MongoManager/MongoManagerTests.js b/services/track-changes/test/unit/js/MongoManager/MongoManagerTests.js index dc044cf288..aeed0b0fca 100644 --- a/services/track-changes/test/unit/js/MongoManager/MongoManagerTests.js +++ b/services/track-changes/test/unit/js/MongoManager/MongoManagerTests.js @@ -190,7 +190,7 @@ describe('MongoManager', function () { beforeEach(function () { this.metadata = { mock: 'metadata' } this.db.projectHistoryMetaData = { - find: sinon.stub().callsArgWith(1, null, [this.metadata]) + findOne: sinon.stub().callsArgWith(1, null, this.metadata) } return this.MongoManager.getProjectMetaData( this.project_id, @@ -199,7 +199,7 @@ describe('MongoManager', function () { }) it('should look up the meta data in the db', function () { - return this.db.projectHistoryMetaData.find + return this.db.projectHistoryMetaData.findOne .calledWith({ project_id: ObjectId(this.project_id) }) .should.equal(true) })