From 8a8a830ad31198c17fc14b31a890b3cc64404907 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 28 Aug 2020 13:13:19 +0100 Subject: [PATCH] [misc] simplify mongodb collection access using a shared db construct Resolve the getCollection Promises once and store the result in a shared `db` object which can get imported by all the call-sites. The http server is starting only after a Promise of `waitForDb()` resolves. This covers the app code and the acceptance tests. --- services/docstore/app/js/HealthChecker.js | 15 +- services/docstore/app/js/MongoManager.js | 145 ++++++++---------- services/docstore/app/js/mongodb.js | 15 +- .../test/acceptance/js/ArchiveDocsTests.js | 10 +- .../test/acceptance/js/DeletingDocsTests.js | 10 +- .../test/acceptance/js/helpers/DocstoreApp.js | 10 +- .../test/unit/js/MongoManagerTests.js | 23 +-- 7 files changed, 89 insertions(+), 139 deletions(-) diff --git a/services/docstore/app/js/HealthChecker.js b/services/docstore/app/js/HealthChecker.js index a32452a1b2..567774af3e 100644 --- a/services/docstore/app/js/HealthChecker.js +++ b/services/docstore/app/js/HealthChecker.js @@ -10,7 +10,7 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -const { getCollection, ObjectId } = require('./mongodb') +const { db, ObjectId } = require('./mongodb') const request = require('request') const async = require('async') const _ = require('underscore') @@ -19,9 +19,6 @@ const settings = require('settings-sharelatex') const { port } = settings.internal.docstore const logger = require('logger-sharelatex') -const docsCollectionPromise = getCollection('docs') -const docOpsCollectionPromise = getCollection('docOps') - module.exports = { check(callback) { const doc_id = ObjectId() @@ -63,14 +60,8 @@ module.exports = { } }) }, - (cb) => - docsCollectionPromise.then((docs) => - docs.deleteOne({ _id: doc_id, project_id }, cb) - ), - (cb) => - docOpsCollectionPromise.then((docOps) => - docOps.deleteOne({ doc_id }, cb) - ) + (cb) => db.docs.deleteOne({ _id: doc_id, project_id }, cb), + (cb) => db.docOps.deleteOne({ doc_id }, cb) ] return async.series(jobs, callback) } diff --git a/services/docstore/app/js/MongoManager.js b/services/docstore/app/js/MongoManager.js index 1f503ab061..76d9a88f14 100644 --- a/services/docstore/app/js/MongoManager.js +++ b/services/docstore/app/js/MongoManager.js @@ -11,28 +11,23 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ let MongoManager -const { getCollection, ObjectId } = require('./mongodb') +const { db, ObjectId } = require('./mongodb') const logger = require('logger-sharelatex') const metrics = require('@overleaf/metrics') const { promisify } = require('util') -const docsCollectionPromise = getCollection('docs') -const docOpsCollectionPromise = getCollection('docOps') - module.exports = MongoManager = { findDoc(project_id, doc_id, filter, callback) { if (callback == null) { callback = function (error, doc) {} } - docsCollectionPromise.then((docs) => - docs.findOne( - { - _id: ObjectId(doc_id.toString()), - project_id: ObjectId(project_id.toString()) - }, - filter, - callback - ) + db.docs.findOne( + { + _id: ObjectId(doc_id.toString()), + project_id: ObjectId(project_id.toString()) + }, + filter, + callback ) }, @@ -44,9 +39,7 @@ module.exports = MongoManager = { if (!options.include_deleted) { query.deleted = { $ne: true } } - docsCollectionPromise.then((docs) => - docs.find(query, filter).toArray(callback) - ) + db.docs.find(query, filter).toArray(callback) }, getArchivedProjectDocs(project_id, callback) { @@ -54,7 +47,7 @@ module.exports = MongoManager = { project_id: ObjectId(project_id.toString()), inS3: true } - docsCollectionPromise.then((docs) => docs.find(query).toArray(callback)) + db.docs.find(query).toArray(callback) }, upsertIntoDocCollection(project_id, doc_id, updates, callback) { @@ -68,28 +61,24 @@ module.exports = MongoManager = { } } update.$set.project_id = ObjectId(project_id) - docsCollectionPromise.then((docs) => - docs.updateOne( - { _id: ObjectId(doc_id) }, - update, - { upsert: true }, - callback - ) + db.docs.updateOne( + { _id: ObjectId(doc_id) }, + update, + { upsert: true }, + callback ) }, markDocAsDeleted(project_id, doc_id, callback) { - docsCollectionPromise.then((docs) => - docs.updateOne( - { - _id: ObjectId(doc_id), - project_id: ObjectId(project_id) - }, - { - $set: { deleted: true } - }, - callback - ) + db.docs.updateOne( + { + _id: ObjectId(doc_id), + project_id: ObjectId(project_id) + }, + { + $set: { deleted: true } + }, + callback ) }, @@ -105,30 +94,26 @@ module.exports = MongoManager = { _id: doc_id, rev } - docsCollectionPromise.then((docs) => - docs.updateOne(query, update, callback) - ) + db.docs.updateOne(query, update, callback) }, getDocVersion(doc_id, callback) { if (callback == null) { callback = function (error, version) {} } - docOpsCollectionPromise.then((docOps) => - docOps.findOne( - { - doc_id: ObjectId(doc_id) - }, - { - version: 1 - }, - function (error, doc) { - if (error != null) { - return callback(error) - } - callback(null, (doc && doc.version) || 0) + db.docOps.findOne( + { + doc_id: ObjectId(doc_id) + }, + { + version: 1 + }, + function (error, doc) { + if (error != null) { + return callback(error) } - ) + callback(null, (doc && doc.version) || 0) + } ) }, @@ -136,42 +121,36 @@ module.exports = MongoManager = { if (callback == null) { callback = function (error) {} } - docOpsCollectionPromise.then((docOps) => - docOps.updateOne( - { - doc_id: ObjectId(doc_id) - }, - { - $set: { version } - }, - { - upsert: true - }, - callback - ) + db.docOps.updateOne( + { + doc_id: ObjectId(doc_id) + }, + { + $set: { version } + }, + { + upsert: true + }, + callback ) }, destroyDoc(doc_id, callback) { - docsCollectionPromise.then((docs) => - docs.deleteOne( - { - _id: ObjectId(doc_id) - }, - function (err) { - if (err != null) { - return callback(err) - } - docOpsCollectionPromise.then((docOps) => - docOps.deleteOne( - { - doc_id: ObjectId(doc_id) - }, - callback - ) - ) + db.docs.deleteOne( + { + _id: ObjectId(doc_id) + }, + function (err) { + if (err != null) { + return callback(err) } - ) + db.docOps.deleteOne( + { + doc_id: ObjectId(doc_id) + }, + callback + ) + } ) } } diff --git a/services/docstore/app/js/mongodb.js b/services/docstore/app/js/mongodb.js index 5b0168bda8..1838011999 100644 --- a/services/docstore/app/js/mongodb.js +++ b/services/docstore/app/js/mongodb.js @@ -2,18 +2,21 @@ const Settings = require('settings-sharelatex') const { MongoClient, ObjectId } = require('mongodb') const clientPromise = MongoClient.connect(Settings.mongo.url) -const dbPromise = clientPromise.then((client) => client.db()) - -async function getCollection(name) { - return (await dbPromise).collection(name) -} async function waitForDb() { await clientPromise } +const db = {} +waitForDb().then(async function () { + const internalDb = (await clientPromise).db() + + db.docs = internalDb.collection('docs') + db.docOps = internalDb.collection('docOps') +}) + module.exports = { + db, ObjectId, - getCollection, waitForDb } diff --git a/services/docstore/test/acceptance/js/ArchiveDocsTests.js b/services/docstore/test/acceptance/js/ArchiveDocsTests.js index 026fb8dd4d..4a6901f4c7 100644 --- a/services/docstore/test/acceptance/js/ArchiveDocsTests.js +++ b/services/docstore/test/acceptance/js/ArchiveDocsTests.js @@ -17,7 +17,7 @@ const Settings = require('settings-sharelatex') const chai = require('chai') const { expect } = chai const should = chai.should() -const { getCollection, ObjectId } = require('../../../app/js/mongodb') +const { db, ObjectId } = require('../../../app/js/mongodb') const async = require('async') const DocstoreApp = require('./helpers/DocstoreApp') const DocstoreClient = require('./helpers/DocstoreClient') @@ -32,14 +32,6 @@ function uploadContent(path, json, callback) { .catch(callback) } -let db -before(async function () { - db = { - docs: await getCollection('docs'), - docOps: await getCollection('docOps') - } -}) - describe('Archiving', function () { before(function (done) { return DocstoreApp.ensureRunning(done) diff --git a/services/docstore/test/acceptance/js/DeletingDocsTests.js b/services/docstore/test/acceptance/js/DeletingDocsTests.js index 4517b97e36..24c78903ee 100644 --- a/services/docstore/test/acceptance/js/DeletingDocsTests.js +++ b/services/docstore/test/acceptance/js/DeletingDocsTests.js @@ -13,21 +13,13 @@ */ const chai = require('chai') chai.should() -const { getCollection, ObjectId } = require('../../../app/js/mongodb') +const { db, ObjectId } = require('../../../app/js/mongodb') const { expect } = chai const DocstoreApp = require('./helpers/DocstoreApp') const Errors = require('../../../app/js/Errors') const DocstoreClient = require('./helpers/DocstoreClient') -let db -before(async function () { - db = { - docs: await getCollection('docs'), - docOps: await getCollection('docOps') - } -}) - describe('Deleting a doc', function () { beforeEach(function (done) { this.project_id = ObjectId() diff --git a/services/docstore/test/acceptance/js/helpers/DocstoreApp.js b/services/docstore/test/acceptance/js/helpers/DocstoreApp.js index 67dc1760e4..351a744b13 100644 --- a/services/docstore/test/acceptance/js/helpers/DocstoreApp.js +++ b/services/docstore/test/acceptance/js/helpers/DocstoreApp.js @@ -12,6 +12,7 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const app = require('../../../../app') +const { waitForDb } = require('../../../../app/js/mongodb') require('logger-sharelatex').logger.level('error') const settings = require('settings-sharelatex') @@ -27,9 +28,10 @@ module.exports = { return callback() } else if (this.initing) { return this.callbacks.push(callback) - } else { - this.initing = true - this.callbacks.push(callback) + } + this.initing = true + this.callbacks.push(callback) + waitForDb().then(() => { return app.listen( settings.internal.docstore.port, 'localhost', @@ -47,6 +49,6 @@ module.exports = { })() } ) - } + }) } } diff --git a/services/docstore/test/unit/js/MongoManagerTests.js b/services/docstore/test/unit/js/MongoManagerTests.js index c33da4f355..ed0d2376dd 100644 --- a/services/docstore/test/unit/js/MongoManagerTests.js +++ b/services/docstore/test/unit/js/MongoManagerTests.js @@ -21,13 +21,9 @@ const { assert } = require('chai') describe('MongoManager', function () { beforeEach(function () { - this.db = {} this.MongoManager = SandboxedModule.require(modulePath, { requires: { './mongodb': { - getCollection: sinon.stub().callsFake((name) => { - return Promise.resolve((this.db[name] = {})) - }), db: (this.db = { docs: {}, docOps: {} }), ObjectId }, @@ -40,12 +36,12 @@ describe('MongoManager', function () { }) this.project_id = ObjectId().toString() this.doc_id = ObjectId().toString() + this.callback = sinon.stub() return (this.stubbedErr = new Error('hello world')) }) describe('findDoc', function () { - beforeEach(function (done) { - this.callback = sinon.stub().callsFake(() => done()) + beforeEach(function () { this.doc = { name: 'mock-doc' } this.db.docs.findOne = sinon.stub().callsArgWith(2, null, this.doc) this.filter = { lines: true } @@ -89,8 +85,7 @@ describe('MongoManager', function () { }) describe('with included_deleted = false', function () { - beforeEach(function (done) { - this.callback = sinon.stub().callsFake(() => done()) + beforeEach(function () { return this.MongoManager.getProjectsDocs( this.project_id, { include_deleted: false }, @@ -119,8 +114,7 @@ describe('MongoManager', function () { }) return describe('with included_deleted = true', function () { - beforeEach(function (done) { - this.callback = sinon.stub().callsFake(() => done()) + beforeEach(function () { return this.MongoManager.getProjectsDocs( this.project_id, { include_deleted: true }, @@ -239,8 +233,7 @@ describe('MongoManager', function () { describe('getDocVersion', function () { describe('when the doc exists', function () { - beforeEach(function (done) { - this.callback = sinon.stub().callsFake(() => done()) + beforeEach(function () { this.doc = { version: (this.version = 42) } this.db.docOps.findOne = sinon.stub().callsArgWith(2, null, this.doc) return this.MongoManager.getDocVersion(this.doc_id, this.callback) @@ -258,8 +251,7 @@ describe('MongoManager', function () { }) return describe("when the doc doesn't exist", function () { - beforeEach(function (done) { - this.callback = sinon.stub().callsFake(() => done()) + beforeEach(function () { this.db.docOps.findOne = sinon.stub().callsArgWith(2, null, null) return this.MongoManager.getDocVersion(this.doc_id, this.callback) }) @@ -271,8 +263,7 @@ describe('MongoManager', function () { }) return describe('setDocVersion', function () { - beforeEach(function (done) { - this.callback = sinon.stub().callsFake(() => done()) + beforeEach(function () { this.version = 42 this.db.docOps.updateOne = sinon.stub().callsArg(3) return this.MongoManager.setDocVersion(