[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.
This commit is contained in:
Jakob Ackermann 2020-08-28 13:13:19 +01:00 committed by Simon Detheridge
parent f90d12ed21
commit 8a8a830ad3
7 changed files with 89 additions and 139 deletions

View file

@ -10,7 +10,7 @@
* DS207: Consider shorter variations of null checks * DS207: Consider shorter variations of null checks
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md * 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 request = require('request')
const async = require('async') const async = require('async')
const _ = require('underscore') const _ = require('underscore')
@ -19,9 +19,6 @@ const settings = require('settings-sharelatex')
const { port } = settings.internal.docstore const { port } = settings.internal.docstore
const logger = require('logger-sharelatex') const logger = require('logger-sharelatex')
const docsCollectionPromise = getCollection('docs')
const docOpsCollectionPromise = getCollection('docOps')
module.exports = { module.exports = {
check(callback) { check(callback) {
const doc_id = ObjectId() const doc_id = ObjectId()
@ -63,14 +60,8 @@ module.exports = {
} }
}) })
}, },
(cb) => (cb) => db.docs.deleteOne({ _id: doc_id, project_id }, cb),
docsCollectionPromise.then((docs) => (cb) => db.docOps.deleteOne({ doc_id }, cb)
docs.deleteOne({ _id: doc_id, project_id }, cb)
),
(cb) =>
docOpsCollectionPromise.then((docOps) =>
docOps.deleteOne({ doc_id }, cb)
)
] ]
return async.series(jobs, callback) return async.series(jobs, callback)
} }

View file

@ -11,28 +11,23 @@
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/ */
let MongoManager let MongoManager
const { getCollection, ObjectId } = require('./mongodb') const { db, ObjectId } = require('./mongodb')
const logger = require('logger-sharelatex') const logger = require('logger-sharelatex')
const metrics = require('@overleaf/metrics') const metrics = require('@overleaf/metrics')
const { promisify } = require('util') const { promisify } = require('util')
const docsCollectionPromise = getCollection('docs')
const docOpsCollectionPromise = getCollection('docOps')
module.exports = MongoManager = { module.exports = MongoManager = {
findDoc(project_id, doc_id, filter, callback) { findDoc(project_id, doc_id, filter, callback) {
if (callback == null) { if (callback == null) {
callback = function (error, doc) {} callback = function (error, doc) {}
} }
docsCollectionPromise.then((docs) => db.docs.findOne(
docs.findOne( {
{ _id: ObjectId(doc_id.toString()),
_id: ObjectId(doc_id.toString()), project_id: ObjectId(project_id.toString())
project_id: ObjectId(project_id.toString()) },
}, filter,
filter, callback
callback
)
) )
}, },
@ -44,9 +39,7 @@ module.exports = MongoManager = {
if (!options.include_deleted) { if (!options.include_deleted) {
query.deleted = { $ne: true } query.deleted = { $ne: true }
} }
docsCollectionPromise.then((docs) => db.docs.find(query, filter).toArray(callback)
docs.find(query, filter).toArray(callback)
)
}, },
getArchivedProjectDocs(project_id, callback) { getArchivedProjectDocs(project_id, callback) {
@ -54,7 +47,7 @@ module.exports = MongoManager = {
project_id: ObjectId(project_id.toString()), project_id: ObjectId(project_id.toString()),
inS3: true inS3: true
} }
docsCollectionPromise.then((docs) => docs.find(query).toArray(callback)) db.docs.find(query).toArray(callback)
}, },
upsertIntoDocCollection(project_id, doc_id, updates, callback) { upsertIntoDocCollection(project_id, doc_id, updates, callback) {
@ -68,28 +61,24 @@ module.exports = MongoManager = {
} }
} }
update.$set.project_id = ObjectId(project_id) update.$set.project_id = ObjectId(project_id)
docsCollectionPromise.then((docs) => db.docs.updateOne(
docs.updateOne( { _id: ObjectId(doc_id) },
{ _id: ObjectId(doc_id) }, update,
update, { upsert: true },
{ upsert: true }, callback
callback
)
) )
}, },
markDocAsDeleted(project_id, doc_id, callback) { markDocAsDeleted(project_id, doc_id, callback) {
docsCollectionPromise.then((docs) => db.docs.updateOne(
docs.updateOne( {
{ _id: ObjectId(doc_id),
_id: ObjectId(doc_id), project_id: ObjectId(project_id)
project_id: ObjectId(project_id) },
}, {
{ $set: { deleted: true }
$set: { deleted: true } },
}, callback
callback
)
) )
}, },
@ -105,30 +94,26 @@ module.exports = MongoManager = {
_id: doc_id, _id: doc_id,
rev rev
} }
docsCollectionPromise.then((docs) => db.docs.updateOne(query, update, callback)
docs.updateOne(query, update, callback)
)
}, },
getDocVersion(doc_id, callback) { getDocVersion(doc_id, callback) {
if (callback == null) { if (callback == null) {
callback = function (error, version) {} callback = function (error, version) {}
} }
docOpsCollectionPromise.then((docOps) => db.docOps.findOne(
docOps.findOne( {
{ doc_id: ObjectId(doc_id)
doc_id: ObjectId(doc_id) },
}, {
{ version: 1
version: 1 },
}, function (error, doc) {
function (error, doc) { if (error != null) {
if (error != null) { return callback(error)
return callback(error)
}
callback(null, (doc && doc.version) || 0)
} }
) callback(null, (doc && doc.version) || 0)
}
) )
}, },
@ -136,42 +121,36 @@ module.exports = MongoManager = {
if (callback == null) { if (callback == null) {
callback = function (error) {} callback = function (error) {}
} }
docOpsCollectionPromise.then((docOps) => db.docOps.updateOne(
docOps.updateOne( {
{ doc_id: ObjectId(doc_id)
doc_id: ObjectId(doc_id) },
}, {
{ $set: { version }
$set: { version } },
}, {
{ upsert: true
upsert: true },
}, callback
callback
)
) )
}, },
destroyDoc(doc_id, callback) { destroyDoc(doc_id, callback) {
docsCollectionPromise.then((docs) => db.docs.deleteOne(
docs.deleteOne( {
{ _id: ObjectId(doc_id)
_id: ObjectId(doc_id) },
}, function (err) {
function (err) { if (err != null) {
if (err != null) { return callback(err)
return callback(err)
}
docOpsCollectionPromise.then((docOps) =>
docOps.deleteOne(
{
doc_id: ObjectId(doc_id)
},
callback
)
)
} }
) db.docOps.deleteOne(
{
doc_id: ObjectId(doc_id)
},
callback
)
}
) )
} }
} }

View file

@ -2,18 +2,21 @@ const Settings = require('settings-sharelatex')
const { MongoClient, ObjectId } = require('mongodb') const { MongoClient, ObjectId } = require('mongodb')
const clientPromise = MongoClient.connect(Settings.mongo.url) 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() { async function waitForDb() {
await clientPromise 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 = { module.exports = {
db,
ObjectId, ObjectId,
getCollection,
waitForDb waitForDb
} }

View file

@ -17,7 +17,7 @@ const Settings = require('settings-sharelatex')
const chai = require('chai') const chai = require('chai')
const { expect } = chai const { expect } = chai
const should = chai.should() const should = chai.should()
const { getCollection, ObjectId } = require('../../../app/js/mongodb') const { db, ObjectId } = require('../../../app/js/mongodb')
const async = require('async') const async = require('async')
const DocstoreApp = require('./helpers/DocstoreApp') const DocstoreApp = require('./helpers/DocstoreApp')
const DocstoreClient = require('./helpers/DocstoreClient') const DocstoreClient = require('./helpers/DocstoreClient')
@ -32,14 +32,6 @@ function uploadContent(path, json, callback) {
.catch(callback) .catch(callback)
} }
let db
before(async function () {
db = {
docs: await getCollection('docs'),
docOps: await getCollection('docOps')
}
})
describe('Archiving', function () { describe('Archiving', function () {
before(function (done) { before(function (done) {
return DocstoreApp.ensureRunning(done) return DocstoreApp.ensureRunning(done)

View file

@ -13,21 +13,13 @@
*/ */
const chai = require('chai') const chai = require('chai')
chai.should() chai.should()
const { getCollection, ObjectId } = require('../../../app/js/mongodb') const { db, ObjectId } = require('../../../app/js/mongodb')
const { expect } = chai const { expect } = chai
const DocstoreApp = require('./helpers/DocstoreApp') const DocstoreApp = require('./helpers/DocstoreApp')
const Errors = require('../../../app/js/Errors') const Errors = require('../../../app/js/Errors')
const DocstoreClient = require('./helpers/DocstoreClient') const DocstoreClient = require('./helpers/DocstoreClient')
let db
before(async function () {
db = {
docs: await getCollection('docs'),
docOps: await getCollection('docOps')
}
})
describe('Deleting a doc', function () { describe('Deleting a doc', function () {
beforeEach(function (done) { beforeEach(function (done) {
this.project_id = ObjectId() this.project_id = ObjectId()

View file

@ -12,6 +12,7 @@
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/ */
const app = require('../../../../app') const app = require('../../../../app')
const { waitForDb } = require('../../../../app/js/mongodb')
require('logger-sharelatex').logger.level('error') require('logger-sharelatex').logger.level('error')
const settings = require('settings-sharelatex') const settings = require('settings-sharelatex')
@ -27,9 +28,10 @@ module.exports = {
return callback() return callback()
} else if (this.initing) { } else if (this.initing) {
return this.callbacks.push(callback) return this.callbacks.push(callback)
} else { }
this.initing = true this.initing = true
this.callbacks.push(callback) this.callbacks.push(callback)
waitForDb().then(() => {
return app.listen( return app.listen(
settings.internal.docstore.port, settings.internal.docstore.port,
'localhost', 'localhost',
@ -47,6 +49,6 @@ module.exports = {
})() })()
} }
) )
} })
} }
} }

View file

@ -21,13 +21,9 @@ const { assert } = require('chai')
describe('MongoManager', function () { describe('MongoManager', function () {
beforeEach(function () { beforeEach(function () {
this.db = {}
this.MongoManager = SandboxedModule.require(modulePath, { this.MongoManager = SandboxedModule.require(modulePath, {
requires: { requires: {
'./mongodb': { './mongodb': {
getCollection: sinon.stub().callsFake((name) => {
return Promise.resolve((this.db[name] = {}))
}),
db: (this.db = { docs: {}, docOps: {} }), db: (this.db = { docs: {}, docOps: {} }),
ObjectId ObjectId
}, },
@ -40,12 +36,12 @@ describe('MongoManager', function () {
}) })
this.project_id = ObjectId().toString() this.project_id = ObjectId().toString()
this.doc_id = ObjectId().toString() this.doc_id = ObjectId().toString()
this.callback = sinon.stub()
return (this.stubbedErr = new Error('hello world')) return (this.stubbedErr = new Error('hello world'))
}) })
describe('findDoc', function () { describe('findDoc', function () {
beforeEach(function (done) { beforeEach(function () {
this.callback = sinon.stub().callsFake(() => done())
this.doc = { name: 'mock-doc' } this.doc = { name: 'mock-doc' }
this.db.docs.findOne = sinon.stub().callsArgWith(2, null, this.doc) this.db.docs.findOne = sinon.stub().callsArgWith(2, null, this.doc)
this.filter = { lines: true } this.filter = { lines: true }
@ -89,8 +85,7 @@ describe('MongoManager', function () {
}) })
describe('with included_deleted = false', function () { describe('with included_deleted = false', function () {
beforeEach(function (done) { beforeEach(function () {
this.callback = sinon.stub().callsFake(() => done())
return this.MongoManager.getProjectsDocs( return this.MongoManager.getProjectsDocs(
this.project_id, this.project_id,
{ include_deleted: false }, { include_deleted: false },
@ -119,8 +114,7 @@ describe('MongoManager', function () {
}) })
return describe('with included_deleted = true', function () { return describe('with included_deleted = true', function () {
beforeEach(function (done) { beforeEach(function () {
this.callback = sinon.stub().callsFake(() => done())
return this.MongoManager.getProjectsDocs( return this.MongoManager.getProjectsDocs(
this.project_id, this.project_id,
{ include_deleted: true }, { include_deleted: true },
@ -239,8 +233,7 @@ describe('MongoManager', function () {
describe('getDocVersion', function () { describe('getDocVersion', function () {
describe('when the doc exists', function () { describe('when the doc exists', function () {
beforeEach(function (done) { beforeEach(function () {
this.callback = sinon.stub().callsFake(() => done())
this.doc = { version: (this.version = 42) } this.doc = { version: (this.version = 42) }
this.db.docOps.findOne = sinon.stub().callsArgWith(2, null, this.doc) this.db.docOps.findOne = sinon.stub().callsArgWith(2, null, this.doc)
return this.MongoManager.getDocVersion(this.doc_id, this.callback) 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 () { return describe("when the doc doesn't exist", function () {
beforeEach(function (done) { beforeEach(function () {
this.callback = sinon.stub().callsFake(() => done())
this.db.docOps.findOne = sinon.stub().callsArgWith(2, null, null) this.db.docOps.findOne = sinon.stub().callsArgWith(2, null, null)
return this.MongoManager.getDocVersion(this.doc_id, this.callback) return this.MongoManager.getDocVersion(this.doc_id, this.callback)
}) })
@ -271,8 +263,7 @@ describe('MongoManager', function () {
}) })
return describe('setDocVersion', function () { return describe('setDocVersion', function () {
beforeEach(function (done) { beforeEach(function () {
this.callback = sinon.stub().callsFake(() => done())
this.version = 42 this.version = 42
this.db.docOps.updateOne = sinon.stub().callsArg(3) this.db.docOps.updateOne = sinon.stub().callsArg(3)
return this.MongoManager.setDocVersion( return this.MongoManager.setDocVersion(