Merge pull request #67 from overleaf/jpa-mongodb-shared-db

[misc] simplify mongodb collection access using a shared db construct

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)
   }

services/docstore/app/js/MongoManager.js

@@ -11,28 +11,25 @@
  * 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())
+      },
+      {
+        projection: filter
+      },
+      callback
     )
   },
 
@@ -44,9 +41,11 @@ module.exports = MongoManager = {
     if (!options.include_deleted) {
       query.deleted = { $ne: true }
     }
-    docsCollectionPromise.then((docs) =>
-      docs.find(query, filter).toArray(callback)
-    )
+    db.docs
+      .find(query, {
+        projection: filter
+      })
+      .toArray(callback)
   },
 
   getArchivedProjectDocs(project_id, callback) {
@@ -54,7 +53,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 +67,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 +100,28 @@ 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)
-        },
-        {
+    db.docOps.findOne(
+      {
+        doc_id: ObjectId(doc_id)
+      },
+      {
+        projection: {
           version: 1
-        },
-        function (error, doc) {
-          if (error != null) {
-            return callback(error)
-          }
-          callback(null, (doc && doc.version) || 0)
         }
-      )
+      },
+      function (error, doc) {
+        if (error != null) {
+          return callback(error)
+        }
+        callback(null, (doc && doc.version) || 0)
+      }
     )
   },
 
@@ -136,42 +129,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
+        )
+      }
     )
   }
 }

services/docstore/app/js/mongodb.js

@@ -1,19 +1,36 @@
 const Settings = require('settings-sharelatex')
 const { MongoClient, ObjectId } = require('mongodb')
 
-const clientPromise = MongoClient.connect(Settings.mongo.url)
-const dbPromise = clientPromise.then((client) => client.db())
+const clientPromise = MongoClient.connect(
+  Settings.mongo.url,
+  Settings.mongo.options
+)
 
-async function getCollection(name) {
-  return (await dbPromise).collection(name)
+let setupDbPromise
+async function waitForDb() {
+  if (!setupDbPromise) {
+    setupDbPromise = setupDb()
+  }
+  await setupDbPromise
 }
 
-async function waitForDb() {
-  await clientPromise
+const db = {}
+async function setupDb() {
+  const internalDb = (await clientPromise).db()
+
+  db.docs = internalDb.collection('docs')
+  db.docOps = internalDb.collection('docOps')
+}
+async function addCollection(name) {
+  await waitForDb()
+  const internalDb = (await clientPromise).db()
+
+  db[name] = internalDb.collection(name)
 }
 
 module.exports = {
+  db,
   ObjectId,
-  getCollection,
+  addCollection,
   waitForDb
 }

services/docstore/config/settings.defaults.js

@@ -14,7 +14,12 @@ const Settings = {
     }
   },
 
-  mongo: {},
+  mongo: {
+    options: {
+      useUnifiedTopology:
+        (process.env.MONGO_USE_UNIFIED_TOPOLOGY || 'true') === 'true'
+    }
+  },
 
   docstore: {
     backend: process.env.BACKEND || 's3',

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)

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()

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 = {
           })()
         }
       )
-    }
+    })
   }
 }

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 }
@@ -64,7 +60,9 @@ describe('MongoManager', function () {
             _id: ObjectId(this.doc_id),
             project_id: ObjectId(this.project_id)
           },
-          this.filter
+          {
+            projection: this.filter
+          }
         )
         .should.equal(true)
     })
@@ -89,8 +87,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 },
@@ -106,7 +103,9 @@ describe('MongoManager', function () {
               project_id: ObjectId(this.project_id),
               deleted: { $ne: true }
             },
-            this.filter
+            {
+              projection: this.filter
+            }
           )
           .should.equal(true)
       })
@@ -119,8 +118,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 },
@@ -135,7 +133,9 @@ describe('MongoManager', function () {
             {
               project_id: ObjectId(this.project_id)
             },
-            this.filter
+            {
+              projection: this.filter
+            }
           )
           .should.equal(true)
       })
@@ -239,8 +239,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)
@@ -248,7 +247,12 @@ describe('MongoManager', function () {
 
       it('should look for the doc in the database', function () {
         return this.db.docOps.findOne
-          .calledWith({ doc_id: ObjectId(this.doc_id) }, { version: 1 })
+          .calledWith(
+            { doc_id: ObjectId(this.doc_id) },
+            {
+              projection: { version: 1 }
+            }
+          )
           .should.equal(true)
       })
 
@@ -258,8 +262,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 +274,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(