From 53d79d86a94cb370ecd6e411f31da220d8f725cc Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 11:21:36 -0400 Subject: [PATCH 01/23] Decaf cleanup: remove Array.from() --- services/document-updater/app.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index 9e772d3e9a..da2e745880 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -1,6 +1,5 @@ /* * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from * DS102: Remove unnecessary code created because of implicit returns * DS103: Rewrite code to no longer use __guard__ * DS207: Consider shorter variations of null checks @@ -211,7 +210,7 @@ const watchForEvent = (eventName) => ) const events = ['connect', 'ready', 'error', 'close', 'reconnecting', 'end'] -for (const eventName of Array.from(events)) { +for (const eventName of events) { watchForEvent(eventName) } From 7b2420413c1c40b5abccc04b6d162623801b956a Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 11:28:31 -0400 Subject: [PATCH 02/23] Decaf cleanup: unnecessary returns and arrow functions in callbacks --- services/document-updater/app.js | 89 ++++++++++++++++---------------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index da2e745880..34d116ee9d 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -1,6 +1,5 @@ /* * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns * DS103: Rewrite code to no longer use __guard__ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md @@ -43,7 +42,7 @@ Metrics.injectMetricsRoute(app) DispatchManager.createAndStartDispatchers(Settings.dispatcherCount || 10) -app.param('project_id', function (req, res, next, projectId) { +app.param('project_id', (req, res, next, projectId) => { if (projectId != null ? projectId.match(/^[0-9a-f]{24}$/) : undefined) { return next() } else { @@ -51,7 +50,7 @@ app.param('project_id', function (req, res, next, projectId) { } }) -app.param('doc_id', function (req, res, next, docId) { +app.param('doc_id', (req, res, next, docId) => { if (docId != null ? docId.match(/^[0-9a-f]{24}$/) : undefined) { return next() } else { @@ -98,18 +97,18 @@ app.delete( app.get('/flush_all_projects', HttpController.flushAllProjects) app.get('/flush_queued_projects', HttpController.flushQueuedProjects) -app.get('/total', function (req, res, next) { +app.get('/total', (req, res, next) => { const timer = new Metrics.Timer('http.allDocList') - return RedisManager.getCountOfDocsInMemory(function (err, count) { + RedisManager.getCountOfDocsInMemory((err, count) => { if (err) { return next(err) } timer.done() - return res.send({ total: count }) + res.send({ total: count }) }) }) -app.get('/status', function (req, res) { +app.get('/status', (req, res) => { if (Settings.shuttingDown) { return res.sendStatus(503) // Service unavailable } else { @@ -120,8 +119,8 @@ app.get('/status', function (req, res) { const pubsubClient = require('redis-sharelatex').createClient( Settings.redis.pubsub ) -app.get('/health_check/redis', (req, res, next) => - pubsubClient.healthCheck(function (error) { +app.get('/health_check/redis', (req, res, next) => { + pubsubClient.healthCheck((error) => { if (error != null) { logger.err({ err: error }, 'failed redis health check') return res.sendStatus(500) @@ -129,13 +128,13 @@ app.get('/health_check/redis', (req, res, next) => return res.sendStatus(200) } }) -) +}) const docUpdaterRedisClient = require('redis-sharelatex').createClient( Settings.redis.documentupdater ) -app.get('/health_check/redis_cluster', (req, res, next) => - docUpdaterRedisClient.healthCheck(function (error) { +app.get('/health_check/redis_cluster', (req, res, next) => { + docUpdaterRedisClient.healthCheck((error) => { if (error != null) { logger.err({ err: error }, 'failed redis cluster health check') return res.sendStatus(500) @@ -143,34 +142,37 @@ app.get('/health_check/redis_cluster', (req, res, next) => return res.sendStatus(200) } }) -) +}) -app.get('/health_check', (req, res, next) => +app.get('/health_check', (req, res, next) => { async.series( [ - (cb) => - pubsubClient.healthCheck(function (error) { + (cb) => { + pubsubClient.healthCheck((error) => { if (error != null) { logger.err({ err: error }, 'failed redis health check') } - return cb(error) - }), - (cb) => - docUpdaterRedisClient.healthCheck(function (error) { + cb(error) + }) + }, + (cb) => { + docUpdaterRedisClient.healthCheck((error) => { if (error != null) { logger.err({ err: error }, 'failed redis cluster health check') } - return cb(error) - }), - (cb) => - mongojs.healthCheck(function (error) { + cb(error) + }) + }, + (cb) => { + mongojs.healthCheck((error) => { if (error != null) { logger.err({ err: error }, 'failed mongo health check') } - return cb(error) + cb(error) }) + } ], - function (error) { + (error) => { if (error != null) { return res.sendStatus(500) } else { @@ -178,9 +180,9 @@ app.get('/health_check', (req, res, next) => } } ) -) +}) -app.use(function (error, req, res, next) { +app.use((error, req, res, next) => { if (error instanceof Errors.NotFoundError) { return res.sendStatus(404) } else if (error instanceof Errors.OpRangeNotAvailableError) { @@ -193,21 +195,20 @@ app.use(function (error, req, res, next) { } }) -const shutdownCleanly = (signal) => - function () { - logger.log({ signal }, 'received interrupt, cleaning up') - Settings.shuttingDown = true - return setTimeout(function () { - logger.log({ signal }, 'shutting down') - return process.exit() - }, 10000) - } +const shutdownCleanly = (signal) => () => { + logger.log({ signal }, 'received interrupt, cleaning up') + Settings.shuttingDown = true + setTimeout(() => { + logger.log({ signal }, 'shutting down') + process.exit() + }, 10000) +} -const watchForEvent = (eventName) => - docUpdaterRedisClient.on( - eventName, - (e) => console.log(`redis event: ${eventName} ${e}`) // eslint-disable-line no-console - ) +const watchForEvent = (eventName) => { + docUpdaterRedisClient.on(eventName, (e) => { + console.log(`redis event: ${eventName} ${e}`) // eslint-disable-line no-console + }) +} const events = ['connect', 'ready', 'error', 'close', 'reconnecting', 'end'] for (const eventName of events) { @@ -227,11 +228,11 @@ const port = const host = Settings.internal.documentupdater.host || 'localhost' if (!module.parent) { // Called directly - app.listen(port, host, function () { + app.listen(port, host, () => { logger.info(`Document-updater starting up, listening on ${host}:${port}`) if (Settings.continuousBackgroundFlush) { logger.info('Starting continuous background flush') - return DeleteQueueManager.startBackgroundFlush() + DeleteQueueManager.startBackgroundFlush() } }) } From 18b92adcef4d58048da9a873b932f45a11b98b77 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 11:32:03 -0400 Subject: [PATCH 03/23] Decaf cleanup: remove __guard__() --- services/document-updater/app.js | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index 34d116ee9d..b0a2d8ae9f 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -1,6 +1,5 @@ /* * decaffeinate suggestions: - * DS103: Rewrite code to no longer use __guard__ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ @@ -216,16 +215,13 @@ for (const eventName of events) { } const port = - __guard__( - Settings.internal != null ? Settings.internal.documentupdater : undefined, - (x) => x.port - ) || - __guard__( - Settings.apis != null ? Settings.apis.documentupdater : undefined, - (x1) => x1.port - ) || + Settings.internal.documentupdater.port || + (Settings.api && + Settings.api.documentupdater && + Settings.api.documentupdater.port) || 3003 const host = Settings.internal.documentupdater.host || 'localhost' + if (!module.parent) { // Called directly app.listen(port, host, () => { @@ -250,9 +246,3 @@ for (const signal of [ ]) { process.on(signal, shutdownCleanly(signal)) } - -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined -} From e60d9237d0d19803a32c8a80e007c9f9e51a54b8 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 11:34:01 -0400 Subject: [PATCH 04/23] Decaf cleanup: simplify null checks --- services/document-updater/app.js | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index b0a2d8ae9f..7555ad666b 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -1,8 +1,3 @@ -/* - * decaffeinate suggestions: - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const Metrics = require('metrics-sharelatex') Metrics.initialize('doc-updater') @@ -13,7 +8,7 @@ logger.initialize('document-updater') logger.logger.addSerializers(require('./app/js/LoggerSerializers')) -if ((Settings.sentry != null ? Settings.sentry.dsn : undefined) != null) { +if (Settings.sentry != null && Settings.sentry.dsn != null) { logger.initializeErrorReporting(Settings.sentry.dsn) } @@ -42,7 +37,7 @@ Metrics.injectMetricsRoute(app) DispatchManager.createAndStartDispatchers(Settings.dispatcherCount || 10) app.param('project_id', (req, res, next, projectId) => { - if (projectId != null ? projectId.match(/^[0-9a-f]{24}$/) : undefined) { + if (projectId != null && projectId.match(/^[0-9a-f]{24}$/)) { return next() } else { return next(new Error('invalid project id')) @@ -50,7 +45,7 @@ app.param('project_id', (req, res, next, projectId) => { }) app.param('doc_id', (req, res, next, docId) => { - if (docId != null ? docId.match(/^[0-9a-f]{24}$/) : undefined) { + if (docId != null && docId.match(/^[0-9a-f]{24}$/)) { return next() } else { return next(new Error('invalid doc id')) @@ -120,7 +115,7 @@ const pubsubClient = require('redis-sharelatex').createClient( ) app.get('/health_check/redis', (req, res, next) => { pubsubClient.healthCheck((error) => { - if (error != null) { + if (error) { logger.err({ err: error }, 'failed redis health check') return res.sendStatus(500) } else { @@ -134,7 +129,7 @@ const docUpdaterRedisClient = require('redis-sharelatex').createClient( ) app.get('/health_check/redis_cluster', (req, res, next) => { docUpdaterRedisClient.healthCheck((error) => { - if (error != null) { + if (error) { logger.err({ err: error }, 'failed redis cluster health check') return res.sendStatus(500) } else { @@ -148,7 +143,7 @@ app.get('/health_check', (req, res, next) => { [ (cb) => { pubsubClient.healthCheck((error) => { - if (error != null) { + if (error) { logger.err({ err: error }, 'failed redis health check') } cb(error) @@ -156,7 +151,7 @@ app.get('/health_check', (req, res, next) => { }, (cb) => { docUpdaterRedisClient.healthCheck((error) => { - if (error != null) { + if (error) { logger.err({ err: error }, 'failed redis cluster health check') } cb(error) @@ -164,7 +159,7 @@ app.get('/health_check', (req, res, next) => { }, (cb) => { mongojs.healthCheck((error) => { - if (error != null) { + if (error) { logger.err({ err: error }, 'failed mongo health check') } cb(error) @@ -172,7 +167,7 @@ app.get('/health_check', (req, res, next) => { } ], (error) => { - if (error != null) { + if (error) { return res.sendStatus(500) } else { return res.sendStatus(200) From e8f935d046952dfdf49e21b92c8e20b39b0f50a6 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 11:53:02 -0400 Subject: [PATCH 05/23] Make max JSON request size configurable and default to 8 MB This is to allow multi-document updates, for example when creating a new project from a zip file. --- services/document-updater/app.js | 2 +- services/document-updater/config/settings.defaults.js | 2 ++ .../test/acceptance/js/SettingADocumentTests.js | 3 +-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index 7555ad666b..e23fa3ca7b 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -31,7 +31,7 @@ Metrics.event_loop.monitor(logger, 100) const app = express() app.use(Metrics.http.monitor(logger)) -app.use(bodyParser.json({ limit: Settings.max_doc_length + 64 * 1024 })) +app.use(bodyParser.json({ limit: Settings.maxJsonRequestSize })) Metrics.injectMetricsRoute(app) DispatchManager.createAndStartDispatchers(Settings.dispatcherCount || 10) diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index ff5a35a515..21c3219a33 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -168,6 +168,8 @@ module.exports = { }, max_doc_length: 2 * 1024 * 1024, // 2mb + maxJsonRequestSize: + parseInt(process.env.MAX_JSON_REQUEST_SIZE, 10) || 8 * 1024 * 1024, dispatcherCount: process.env.DISPATCHER_COUNT, diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 6c13282ba5..2107f46e92 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -230,8 +230,7 @@ describe('Setting a document', function () { }) this.newLines = [] while ( - JSON.stringify(this.newLines).length < - Settings.max_doc_length + 64 * 1024 + JSON.stringify(this.newLines).length <= Settings.maxJsonRequestSize ) { this.newLines.push('(a long line of text)'.repeat(10000)) } From ff2d31c066ddfdd0484a81c83fb5afbad4e1c58e Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 12:01:25 -0400 Subject: [PATCH 06/23] Decaf cleanup: remove Array.from() --- .../acceptance/js/SettingADocumentTests.js | 37 ++++++------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 2107f46e92..25a46f8019 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -7,7 +7,6 @@ // Fix any style issues and re-enable lint. /* * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from * DS102: Remove unnecessary code created because of implicit returns * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md @@ -61,10 +60,8 @@ describe('Setting a document', function () { describe('when the updated doc exists in the doc updater', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId() - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version @@ -155,10 +152,8 @@ describe('Setting a document', function () { describe('when the updated doc does not exist in the doc updater', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId() - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version @@ -220,10 +215,8 @@ describe('Setting a document', function () { describe('when the updated doc is too large for the body parser', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId() - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version @@ -274,10 +267,8 @@ describe('Setting a document', function () { describe('when the updated doc is large but under the bodyParser and HTTPController size limit', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId() - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version @@ -343,10 +334,8 @@ describe('Setting a document', function () { describe('with the undo flag', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId() - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version @@ -407,10 +396,8 @@ describe('Setting a document', function () { return describe('without the undo flag', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId() - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version From dc5d77998c60a9aa61c329e5ab019c6e733cef69 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 12:03:54 -0400 Subject: [PATCH 07/23] Decaf cleanup: remove unnecessary returns --- .../acceptance/js/SettingADocumentTests.js | 113 ++++++++---------- 1 file changed, 49 insertions(+), 64 deletions(-) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 25a46f8019..ddbbf0e9c1 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -7,7 +7,6 @@ // Fix any style issues and re-enable lint. /* * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ @@ -49,13 +48,13 @@ describe('Setting a document', function () { sinon.spy(MockTrackChangesApi, 'flushDoc') sinon.spy(MockProjectHistoryApi, 'flushProject') sinon.spy(MockWebApi, 'setDocument') - return DocUpdaterApp.ensureRunning(done) + DocUpdaterApp.ensureRunning(done) }) after(function () { MockTrackChangesApi.flushDoc.restore() MockProjectHistoryApi.flushProject.restore() - return MockWebApi.setDocument.restore() + MockWebApi.setDocument.restore() }) describe('when the updated doc exists in the doc updater', function () { @@ -70,7 +69,7 @@ describe('Setting a document', function () { if (error != null) { throw error } - return DocUpdaterClient.sendUpdate( + DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, this.update, @@ -78,8 +77,8 @@ describe('Setting a document', function () { if (error != null) { throw error } - return setTimeout(() => { - return DocUpdaterClient.setDocLines( + setTimeout(() => { + DocUpdaterClient.setDocLines( this.project_id, this.doc_id, this.newLines, @@ -88,28 +87,27 @@ describe('Setting a document', function () { false, (error, res, body) => { this.statusCode = res.statusCode - return done() + done() } ) }, 200) } ) }) - return null }) after(function () { MockTrackChangesApi.flushDoc.reset() MockProjectHistoryApi.flushProject.reset() - return MockWebApi.setDocument.reset() + MockWebApi.setDocument.reset() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should send the updated doc lines and version to the web api', function () { - return MockWebApi.setDocument + MockWebApi.setDocument .calledWith(this.project_id, this.doc_id, this.newLines) .should.equal(true) }) @@ -120,10 +118,9 @@ describe('Setting a document', function () { this.doc_id, (error, res, doc) => { doc.lines.should.deep.equal(this.newLines) - return done() + done() } ) - return null }) it('should bump the version in the doc updater', function (done) { @@ -132,21 +129,19 @@ describe('Setting a document', function () { this.doc_id, (error, res, doc) => { doc.version.should.equal(this.version + 2) - return done() + done() } ) - return null }) - return it('should leave the document in redis', function (done) { + it('should leave the document in redis', function (done) { rclient_du.get(Keys.docLines({ doc_id: this.doc_id }), (error, lines) => { if (error != null) { throw error } expect(JSON.parse(lines)).to.deep.equal(this.newLines) - return done() + done() }) - return null }) }) @@ -167,49 +162,45 @@ describe('Setting a document', function () { false, (error, res, body) => { this.statusCode = res.statusCode - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) after(function () { MockTrackChangesApi.flushDoc.reset() MockProjectHistoryApi.flushProject.reset() - return MockWebApi.setDocument.reset() + MockWebApi.setDocument.reset() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) it('should send the updated doc lines to the web api', function () { - return MockWebApi.setDocument + MockWebApi.setDocument .calledWith(this.project_id, this.doc_id, this.newLines) .should.equal(true) }) it('should flush track changes', function () { - return MockTrackChangesApi.flushDoc - .calledWith(this.doc_id) - .should.equal(true) + MockTrackChangesApi.flushDoc.calledWith(this.doc_id).should.equal(true) }) it('should flush project history', function () { - return MockProjectHistoryApi.flushProject + MockProjectHistoryApi.flushProject .calledWith(this.project_id) .should.equal(true) }) - return it('should remove the document from redis', function (done) { + it('should remove the document from redis', function (done) { rclient_du.get(Keys.docLines({ doc_id: this.doc_id }), (error, lines) => { if (error != null) { throw error } expect(lines).to.not.exist - return done() + done() }) - return null }) }) @@ -236,32 +227,31 @@ describe('Setting a document', function () { false, (error, res, body) => { this.statusCode = res.statusCode - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) after(function () { MockTrackChangesApi.flushDoc.reset() MockProjectHistoryApi.flushProject.reset() - return MockWebApi.setDocument.reset() + MockWebApi.setDocument.reset() }) it('should return a 413 status code', function () { - return this.statusCode.should.equal(413) + this.statusCode.should.equal(413) }) it('should not send the updated doc lines to the web api', function () { - return MockWebApi.setDocument.called.should.equal(false) + MockWebApi.setDocument.called.should.equal(false) }) it('should not flush track changes', function () { - return MockTrackChangesApi.flushDoc.called.should.equal(false) + MockTrackChangesApi.flushDoc.called.should.equal(false) }) - return it('should not flush project history', function () { - return MockProjectHistoryApi.flushProject.called.should.equal(false) + it('should not flush project history', function () { + MockProjectHistoryApi.flushProject.called.should.equal(false) }) }) @@ -289,34 +279,33 @@ describe('Setting a document', function () { false, (error, res, body) => { this.statusCode = res.statusCode - return setTimeout(done, 200) + setTimeout(done, 200) } ) - return null }) after(function () { MockTrackChangesApi.flushDoc.reset() MockProjectHistoryApi.flushProject.reset() - return MockWebApi.setDocument.reset() + MockWebApi.setDocument.reset() }) it('should return a 204 status code', function () { - return this.statusCode.should.equal(204) + this.statusCode.should.equal(204) }) - return it('should send the updated doc lines to the web api', function () { - return MockWebApi.setDocument + it('should send the updated doc lines to the web api', function () { + MockWebApi.setDocument .calledWith(this.project_id, this.doc_id, this.newLines) .should.equal(true) }) }) - return describe('with track changes', function () { + describe('with track changes', function () { before(function () { this.lines = ['one', 'one and a half', 'two', 'three'] this.id_seed = '587357bd35e64f6157' - return (this.update = { + this.update = { doc: this.doc_id, op: [ { @@ -329,7 +318,7 @@ describe('Setting a document', function () { user_id: this.user_id }, v: this.version - }) + } }) describe('with the undo flag', function () { @@ -344,7 +333,7 @@ describe('Setting a document', function () { if (error != null) { throw error } - return DocUpdaterClient.sendUpdate( + DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, this.update, @@ -353,7 +342,7 @@ describe('Setting a document', function () { throw error } // Go back to old lines, with undo flag - return DocUpdaterClient.setDocLines( + DocUpdaterClient.setDocLines( this.project_id, this.doc_id, this.lines, @@ -362,22 +351,21 @@ describe('Setting a document', function () { true, (error, res, body) => { this.statusCode = res.statusCode - return setTimeout(done, 200) + setTimeout(done, 200) } ) } ) }) - return null }) after(function () { MockTrackChangesApi.flushDoc.reset() MockProjectHistoryApi.flushProject.reset() - return MockWebApi.setDocument.reset() + MockWebApi.setDocument.reset() }) - return it('should undo the tracked changes', function (done) { + it('should undo the tracked changes', function (done) { DocUpdaterClient.getDoc( this.project_id, this.doc_id, @@ -387,14 +375,13 @@ describe('Setting a document', function () { } const { ranges } = data expect(ranges.changes).to.be.undefined - return done() + done() } ) - return null }) }) - return describe('without the undo flag', function () { + describe('without the undo flag', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.doc_id = DocUpdaterClient.randomId() @@ -406,7 +393,7 @@ describe('Setting a document', function () { if (error != null) { throw error } - return DocUpdaterClient.sendUpdate( + DocUpdaterClient.sendUpdate( this.project_id, this.doc_id, this.update, @@ -415,7 +402,7 @@ describe('Setting a document', function () { throw error } // Go back to old lines, without undo flag - return DocUpdaterClient.setDocLines( + DocUpdaterClient.setDocLines( this.project_id, this.doc_id, this.lines, @@ -424,22 +411,21 @@ describe('Setting a document', function () { false, (error, res, body) => { this.statusCode = res.statusCode - return setTimeout(done, 200) + setTimeout(done, 200) } ) } ) }) - return null }) after(function () { MockTrackChangesApi.flushDoc.reset() MockProjectHistoryApi.flushProject.reset() - return MockWebApi.setDocument.reset() + MockWebApi.setDocument.reset() }) - return it('should not undo the tracked changes', function (done) { + it('should not undo the tracked changes', function (done) { DocUpdaterClient.getDoc( this.project_id, this.doc_id, @@ -449,10 +435,9 @@ describe('Setting a document', function () { } const { ranges } = data expect(ranges.changes.length).to.equal(1) - return done() + done() } ) - return null }) }) }) From 150c4a88f10cb40a53e48e4510d4e063e07754cd Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 14:06:25 -0400 Subject: [PATCH 08/23] Decaf cleanup: simplify null checks --- .../acceptance/js/SettingADocumentTests.js | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index ddbbf0e9c1..0e99770f58 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -3,13 +3,6 @@ handle-callback-err, no-return-assign, */ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const chai = require('chai') chai.should() @@ -66,7 +59,7 @@ describe('Setting a document', function () { version: this.version }) DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, (error) => { - if (error != null) { + if (error) { throw error } DocUpdaterClient.sendUpdate( @@ -74,7 +67,7 @@ describe('Setting a document', function () { this.doc_id, this.update, (error) => { - if (error != null) { + if (error) { throw error } setTimeout(() => { @@ -136,7 +129,7 @@ describe('Setting a document', function () { it('should leave the document in redis', function (done) { rclient_du.get(Keys.docLines({ doc_id: this.doc_id }), (error, lines) => { - if (error != null) { + if (error) { throw error } expect(JSON.parse(lines)).to.deep.equal(this.newLines) @@ -195,7 +188,7 @@ describe('Setting a document', function () { it('should remove the document from redis', function (done) { rclient_du.get(Keys.docLines({ doc_id: this.doc_id }), (error, lines) => { - if (error != null) { + if (error) { throw error } expect(lines).to.not.exist @@ -330,7 +323,7 @@ describe('Setting a document', function () { version: this.version }) DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, (error) => { - if (error != null) { + if (error) { throw error } DocUpdaterClient.sendUpdate( @@ -338,7 +331,7 @@ describe('Setting a document', function () { this.doc_id, this.update, (error) => { - if (error != null) { + if (error) { throw error } // Go back to old lines, with undo flag @@ -370,7 +363,7 @@ describe('Setting a document', function () { this.project_id, this.doc_id, (error, res, data) => { - if (error != null) { + if (error) { throw error } const { ranges } = data @@ -390,7 +383,7 @@ describe('Setting a document', function () { version: this.version }) DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, (error) => { - if (error != null) { + if (error) { throw error } DocUpdaterClient.sendUpdate( @@ -398,7 +391,7 @@ describe('Setting a document', function () { this.doc_id, this.update, (error) => { - if (error != null) { + if (error) { throw error } // Go back to old lines, without undo flag @@ -430,7 +423,7 @@ describe('Setting a document', function () { this.project_id, this.doc_id, (error, res, data) => { - if (error != null) { + if (error) { throw error } const { ranges } = data From 75f9b0ff10477fd2849fa37bd526b0041d40fbb6 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 14:08:31 -0400 Subject: [PATCH 09/23] Decaf cleanup: handle errors --- .../acceptance/js/SettingADocumentTests.js | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 0e99770f58..54e7541b33 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -1,7 +1,5 @@ /* eslint-disable camelcase, - handle-callback-err, - no-return-assign, */ const sinon = require('sinon') const chai = require('chai') @@ -79,6 +77,9 @@ describe('Setting a document', function () { this.user_id, false, (error, res, body) => { + if (error) { + return done(error) + } this.statusCode = res.statusCode done() } @@ -110,6 +111,9 @@ describe('Setting a document', function () { this.project_id, this.doc_id, (error, res, doc) => { + if (error) { + return done(error) + } doc.lines.should.deep.equal(this.newLines) done() } @@ -121,6 +125,9 @@ describe('Setting a document', function () { this.project_id, this.doc_id, (error, res, doc) => { + if (error) { + return done(error) + } doc.version.should.equal(this.version + 2) done() } @@ -154,6 +161,9 @@ describe('Setting a document', function () { this.user_id, false, (error, res, body) => { + if (error) { + return done(error) + } this.statusCode = res.statusCode setTimeout(done, 200) } @@ -219,6 +229,9 @@ describe('Setting a document', function () { this.user_id, false, (error, res, body) => { + if (error) { + return done(error) + } this.statusCode = res.statusCode setTimeout(done, 200) } @@ -271,6 +284,9 @@ describe('Setting a document', function () { this.user_id, false, (error, res, body) => { + if (error) { + return done(error) + } this.statusCode = res.statusCode setTimeout(done, 200) } @@ -343,6 +359,9 @@ describe('Setting a document', function () { this.user_id, true, (error, res, body) => { + if (error) { + return done(error) + } this.statusCode = res.statusCode setTimeout(done, 200) } @@ -403,6 +422,9 @@ describe('Setting a document', function () { this.user_id, false, (error, res, body) => { + if (error) { + return done(error) + } this.statusCode = res.statusCode setTimeout(done, 200) } From f99125c65ac0f3627221570b3d9fe08b4e7fd405 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 8 May 2020 14:09:54 -0400 Subject: [PATCH 10/23] Decaf cleanup: camel case variables --- .../acceptance/js/SettingADocumentTests.js | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 54e7541b33..6d57259891 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -1,12 +1,9 @@ -/* eslint-disable - camelcase, -*/ const sinon = require('sinon') const chai = require('chai') chai.should() const { expect } = require('chai') const Settings = require('settings-sharelatex') -const rclient_du = require('redis-sharelatex').createClient( +const docUpdaterRedis = require('redis-sharelatex').createClient( Settings.redis.documentupdater ) const Keys = Settings.redis.documentupdater.key_schema @@ -135,13 +132,16 @@ describe('Setting a document', function () { }) it('should leave the document in redis', function (done) { - rclient_du.get(Keys.docLines({ doc_id: this.doc_id }), (error, lines) => { - if (error) { - throw error + docUpdaterRedis.get( + Keys.docLines({ doc_id: this.doc_id }), + (error, lines) => { + if (error) { + throw error + } + expect(JSON.parse(lines)).to.deep.equal(this.newLines) + done() } - expect(JSON.parse(lines)).to.deep.equal(this.newLines) - done() - }) + ) }) }) @@ -197,13 +197,16 @@ describe('Setting a document', function () { }) it('should remove the document from redis', function (done) { - rclient_du.get(Keys.docLines({ doc_id: this.doc_id }), (error, lines) => { - if (error) { - throw error + docUpdaterRedis.get( + Keys.docLines({ doc_id: this.doc_id }), + (error, lines) => { + if (error) { + throw error + } + expect(lines).to.not.exist + done() } - expect(lines).to.not.exist - done() - }) + ) }) }) From 41c0899b0ce0a847c0f7b6110d1a1a13ab0793d9 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 10:41:32 -0400 Subject: [PATCH 11/23] Add a test for document size slightly over max doc length --- .../acceptance/js/SettingADocumentTests.js | 103 ++++++++++-------- 1 file changed, 58 insertions(+), 45 deletions(-) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 6d57259891..484d51b57c 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -210,57 +210,70 @@ describe('Setting a document', function () { }) }) - describe('when the updated doc is too large for the body parser', function () { - before(function (done) { - this.project_id = DocUpdaterClient.randomId() - this.doc_id = DocUpdaterClient.randomId() - MockWebApi.insertDoc(this.project_id, this.doc_id, { - lines: this.lines, - version: this.version - }) - this.newLines = [] - while ( - JSON.stringify(this.newLines).length <= Settings.maxJsonRequestSize - ) { - this.newLines.push('(a long line of text)'.repeat(10000)) - } - DocUpdaterClient.setDocLines( - this.project_id, - this.doc_id, - this.newLines, - this.source, - this.user_id, - false, - (error, res, body) => { - if (error) { - return done(error) - } - this.statusCode = res.statusCode - setTimeout(done, 200) + const DOC_TOO_LARGE_TEST_CASES = [ + { + desc: 'when the updated doc is too large for the body parser', + size: Settings.maxJsonRequestSize, + expectedStatusCode: 413 + }, + { + desc: 'when the updated doc is larger than the HTTP controller limit', + size: Settings.max_doc_length, + expectedStatusCode: 406 + } + ] + + DOC_TOO_LARGE_TEST_CASES.forEach((testCase) => { + describe(testCase.desc, function () { + before(function (done) { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version + }) + this.newLines = [] + while (JSON.stringify(this.newLines).length <= testCase.size) { + this.newLines.push('(a long line of text)'.repeat(10000)) } - ) - }) + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false, + (error, res, body) => { + if (error) { + return done(error) + } + this.statusCode = res.statusCode + setTimeout(done, 200) + } + ) + }) - after(function () { - MockTrackChangesApi.flushDoc.reset() - MockProjectHistoryApi.flushProject.reset() - MockWebApi.setDocument.reset() - }) + after(function () { + MockTrackChangesApi.flushDoc.reset() + MockProjectHistoryApi.flushProject.reset() + MockWebApi.setDocument.reset() + }) - it('should return a 413 status code', function () { - this.statusCode.should.equal(413) - }) + it(`should return a ${testCase.expectedStatusCode} status code`, function () { + this.statusCode.should.equal(testCase.expectedStatusCode) + }) - it('should not send the updated doc lines to the web api', function () { - MockWebApi.setDocument.called.should.equal(false) - }) + it('should not send the updated doc lines to the web api', function () { + MockWebApi.setDocument.called.should.equal(false) + }) - it('should not flush track changes', function () { - MockTrackChangesApi.flushDoc.called.should.equal(false) - }) + it('should not flush track changes', function () { + MockTrackChangesApi.flushDoc.called.should.equal(false) + }) - it('should not flush project history', function () { - MockProjectHistoryApi.flushProject.called.should.equal(false) + it('should not flush project history', function () { + MockProjectHistoryApi.flushProject.called.should.equal(false) + }) }) }) From 3385ec5f263f0f46c4a37c69b57262a2e47cb13b Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 10:43:22 -0400 Subject: [PATCH 12/23] Decaf cleanup: unnecessary Array.from() --- services/document-updater/app/js/HttpController.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index b6bd00214e..9ad2fe3d65 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -6,7 +6,6 @@ // Fix any style issues and re-enable lint. /* * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from * DS102: Remove unnecessary code created because of implicit returns * DS103: Rewrite code to no longer use __guard__ * DS207: Consider shorter variations of null checks @@ -69,7 +68,7 @@ module.exports = HttpController = { _getTotalSizeOfLines(lines) { let size = 0 - for (const line of Array.from(lines)) { + for (const line of lines) { size += line.length + 1 } return size @@ -89,10 +88,8 @@ module.exports = HttpController = { logger.log({ project_id, exclude: excludeItems }, 'getting docs via http') const timer = new Metrics.Timer('http.getAllDocs') const excludeVersions = {} - for (const item of Array.from(excludeItems)) { - const [id, version] = Array.from( - item != null ? item.split(':') : undefined - ) + for (const item of excludeItems) { + const [id, version] = item.split(':') excludeVersions[id] = version } logger.log( @@ -113,7 +110,7 @@ module.exports = HttpController = { logger.log( { project_id, - result: Array.from(result).map((doc) => `${doc._id}:${doc.v}`) + result: result.map((doc) => `${doc._id}:${doc.v}`) }, 'got docs via http' ) From 814ac40e07f303ccc782d3e9e2d02bd853262d76 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 10:45:39 -0400 Subject: [PATCH 13/23] Decaf cleanup: unnecessary returns --- .../document-updater/app/js/HttpController.js | 98 +++++++++---------- 1 file changed, 45 insertions(+), 53 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 9ad2fe3d65..3d029461b0 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -6,7 +6,6 @@ // Fix any style issues and re-enable lint. /* * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns * DS103: Rewrite code to no longer use __guard__ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md @@ -41,7 +40,7 @@ module.exports = HttpController = { fromVersion = -1 } - return DocumentManager.getDocAndRecentOpsWithLock( + DocumentManager.getDocAndRecentOpsWithLock( project_id, doc_id, fromVersion, @@ -54,7 +53,7 @@ module.exports = HttpController = { if (lines == null || version == null) { return next(new Errors.NotFoundError('document not found')) } - return res.json({ + res.json({ id: doc_id, lines, version, @@ -96,16 +95,16 @@ module.exports = HttpController = { { project_id, projectStateHash, excludeVersions }, 'excluding versions' ) - return ProjectManager.getProjectDocsAndFlushIfOld( + ProjectManager.getProjectDocsAndFlushIfOld( project_id, projectStateHash, excludeVersions, function (error, result) { timer.done() if (error instanceof Errors.ProjectStateChangedError) { - return res.sendStatus(409) // conflict + res.sendStatus(409) // conflict } else if (error != null) { - return next(error) + next(error) } else { logger.log( { @@ -114,7 +113,7 @@ module.exports = HttpController = { }, 'got docs via http' ) - return res.send(result) + res.send(result) } } ) @@ -127,12 +126,12 @@ module.exports = HttpController = { const { project_id } = req.params const timer = new Metrics.Timer('http.clearProjectState') logger.log({ project_id }, 'clearing project state via http') - return ProjectManager.clearProjectState(project_id, function (error) { + ProjectManager.clearProjectState(project_id, function (error) { timer.done() if (error != null) { - return next(error) + next(error) } else { - return res.sendStatus(200) + res.sendStatus(200) } }) }, @@ -157,7 +156,7 @@ module.exports = HttpController = { 'setting doc via http' ) const timer = new Metrics.Timer('http.setDoc') - return DocumentManager.setDocWithLock( + DocumentManager.setDocWithLock( project_id, doc_id, lines, @@ -170,7 +169,7 @@ module.exports = HttpController = { return next(error) } logger.log({ project_id, doc_id }, 'set doc via http') - return res.sendStatus(204) + res.sendStatus(204) } ) }, // No Content @@ -183,18 +182,16 @@ module.exports = HttpController = { const { project_id } = req.params logger.log({ project_id, doc_id }, 'flushing doc via http') const timer = new Metrics.Timer('http.flushDoc') - return DocumentManager.flushDocIfLoadedWithLock( - project_id, - doc_id, - function (error) { - timer.done() - if (error != null) { - return next(error) - } - logger.log({ project_id, doc_id }, 'flushed doc via http') - return res.sendStatus(204) + DocumentManager.flushDocIfLoadedWithLock(project_id, doc_id, function ( + error + ) { + timer.done() + if (error != null) { + return next(error) } - ) + logger.log({ project_id, doc_id }, 'flushed doc via http') + res.sendStatus(204) + }) }, // No Content deleteDoc(req, res, next) { @@ -206,7 +203,7 @@ module.exports = HttpController = { const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' const timer = new Metrics.Timer('http.deleteDoc') logger.log({ project_id, doc_id }, 'deleting doc via http') - return DocumentManager.flushAndDeleteDocWithLock( + DocumentManager.flushAndDeleteDocWithLock( project_id, doc_id, { ignoreFlushErrors }, @@ -220,7 +217,7 @@ module.exports = HttpController = { return next(error) } logger.log({ project_id, doc_id }, 'deleted doc via http') - return res.sendStatus(204) + res.sendStatus(204) } ) }, // No Content @@ -232,13 +229,13 @@ module.exports = HttpController = { const { project_id } = req.params logger.log({ project_id }, 'flushing project via http') const timer = new Metrics.Timer('http.flushProject') - return ProjectManager.flushProjectWithLocks(project_id, function (error) { + ProjectManager.flushProjectWithLocks(project_id, function (error) { timer.done() if (error != null) { return next(error) } logger.log({ project_id }, 'flushed project via http') - return res.sendStatus(204) + res.sendStatus(204) }) }, // No Content @@ -256,18 +253,16 @@ module.exports = HttpController = { options.skip_history_flush = true } // don't flush history when realtime shuts down if (req.query != null ? req.query.background : undefined) { - return ProjectManager.queueFlushAndDeleteProject(project_id, function ( - error - ) { + ProjectManager.queueFlushAndDeleteProject(project_id, function (error) { if (error != null) { return next(error) } logger.log({ project_id }, 'queue delete of project via http') - return res.sendStatus(204) + res.sendStatus(204) }) // No Content } else { const timer = new Metrics.Timer('http.deleteProject') - return ProjectManager.flushAndDeleteProjectWithLocks( + ProjectManager.flushAndDeleteProjectWithLocks( project_id, options, function (error) { @@ -276,7 +271,7 @@ module.exports = HttpController = { return next(error) } logger.log({ project_id }, 'deleted project via http') - return res.sendStatus(204) + res.sendStatus(204) } ) } @@ -289,17 +284,17 @@ module.exports = HttpController = { const project_ids = (req.body != null ? req.body.project_ids : undefined) || [] logger.log({ project_ids }, 'deleting multiple projects via http') - return async.eachSeries( + async.eachSeries( project_ids, function (project_id, cb) { logger.log({ project_id }, 'queue delete of project via http') - return ProjectManager.queueFlushAndDeleteProject(project_id, cb) + ProjectManager.queueFlushAndDeleteProject(project_id, cb) }, function (error) { if (error != null) { return next(error) } - return res.sendStatus(204) + res.sendStatus(204) } ) }, // No Content @@ -318,7 +313,7 @@ module.exports = HttpController = { `accepting ${change_ids.length} changes via http` ) const timer = new Metrics.Timer('http.acceptChanges') - return DocumentManager.acceptChangesWithLock( + DocumentManager.acceptChangesWithLock( project_id, doc_id, change_ids, @@ -331,7 +326,7 @@ module.exports = HttpController = { { project_id, doc_id }, `accepted ${change_ids.length} changes via http` ) - return res.sendStatus(204) + res.sendStatus(204) } ) }, // No Content @@ -343,7 +338,7 @@ module.exports = HttpController = { const { project_id, doc_id, comment_id } = req.params logger.log({ project_id, doc_id, comment_id }, 'deleting comment via http') const timer = new Metrics.Timer('http.deleteComment') - return DocumentManager.deleteCommentWithLock( + DocumentManager.deleteCommentWithLock( project_id, doc_id, comment_id, @@ -356,7 +351,7 @@ module.exports = HttpController = { { project_id, doc_id, comment_id }, 'deleted comment via http' ) - return res.sendStatus(204) + res.sendStatus(204) } ) }, // No Content @@ -379,7 +374,7 @@ module.exports = HttpController = { 'updating project via http' ) - return ProjectManager.updateProjectWithLocks( + ProjectManager.updateProjectWithLocks( project_id, projectHistoryId, userId, @@ -392,7 +387,7 @@ module.exports = HttpController = { return next(error) } logger.log({ project_id }, 'updated project via http') - return res.sendStatus(204) + res.sendStatus(204) } ) }, // No Content @@ -408,7 +403,7 @@ module.exports = HttpController = { { project_id, docs, files }, 'queuing project history resync via http' ) - return HistoryManager.resyncProjectHistory( + HistoryManager.resyncProjectHistory( project_id, projectHistoryId, docs, @@ -418,7 +413,7 @@ module.exports = HttpController = { return next(error) } logger.log({ project_id }, 'queued project history resync via http') - return res.sendStatus(204) + res.sendStatus(204) } ) }, @@ -433,15 +428,12 @@ module.exports = HttpController = { concurrency: req.query.concurrency || 5, dryRun: req.query.dryRun || false } - return ProjectFlusher.flushAllProjects(options, function ( - err, - project_ids - ) { + ProjectFlusher.flushAllProjects(options, function (err, project_ids) { if (err != null) { logger.err({ err }, 'error bulk flushing projects') - return res.sendStatus(500) + res.sendStatus(500) } else { - return res.send(project_ids) + res.send(project_ids) } }) }, @@ -456,16 +448,16 @@ module.exports = HttpController = { timeout: 5 * 60 * 1000, min_delete_age: req.query.min_delete_age || 5 * 60 * 1000 } - return DeleteQueueManager.flushAndDeleteOldProjects(options, function ( + DeleteQueueManager.flushAndDeleteOldProjects(options, function ( err, flushed ) { if (err != null) { logger.err({ err }, 'error flushing old projects') - return res.sendStatus(500) + res.sendStatus(500) } else { logger.log({ flushed }, 'flush of queued projects completed') - return res.send({ flushed }) + res.send({ flushed }) } }) } From 80ea49c69cf64e6fa4c90abfc3b91a0f017d7c7d Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 10:47:27 -0400 Subject: [PATCH 14/23] Decaf cleanup: remove __guard__ --- services/document-updater/app/js/HttpController.js | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 3d029461b0..8c2501fdbe 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -6,7 +6,6 @@ // Fix any style issues and re-enable lint. /* * decaffeinate suggestions: - * DS103: Rewrite code to no longer use __guard__ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ @@ -81,9 +80,7 @@ module.exports = HttpController = { const projectStateHash = req.query != null ? req.query.state : undefined // exclude is string of existing docs "id:version,id:version,..." const excludeItems = - __guard__(req.query != null ? req.query.exclude : undefined, (x) => - x.split(',') - ) || [] + req.query.exclude != null ? req.query.exclude.split(',') : [] logger.log({ project_id, exclude: excludeItems }, 'getting docs via http') const timer = new Metrics.Timer('http.getAllDocs') const excludeVersions = {} @@ -462,9 +459,3 @@ module.exports = HttpController = { }) } } - -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined -} From fc73bbe1a534f64c9b65d275c896335aac389794 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 10:52:06 -0400 Subject: [PATCH 15/23] Decaf cleanup: simplify null checks --- .../document-updater/app/js/HttpController.js | 99 +++++-------------- 1 file changed, 23 insertions(+), 76 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 8c2501fdbe..800aee4c44 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -2,13 +2,6 @@ camelcase, handle-callback-err, */ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ let HttpController const DocumentManager = require('./DocumentManager') const HistoryManager = require('./HistoryManager') @@ -25,15 +18,12 @@ const TWO_MEGABYTES = 2 * 1024 * 1024 module.exports = HttpController = { getDoc(req, res, next) { let fromVersion - if (next == null) { - next = function (error) {} - } const { doc_id } = req.params const { project_id } = req.params logger.log({ project_id, doc_id }, 'getting doc via http') const timer = new Metrics.Timer('http.getDoc') - if ((req.query != null ? req.query.fromVersion : undefined) != null) { + if (req.query.fromVersion != null) { fromVersion = parseInt(req.query.fromVersion, 10) } else { fromVersion = -1 @@ -45,7 +35,7 @@ module.exports = HttpController = { fromVersion, function (error, lines, version, ops, ranges, pathname) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log({ project_id, doc_id }, 'got doc via http') @@ -73,11 +63,8 @@ module.exports = HttpController = { }, getProjectDocsAndFlushIfOld(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id } = req.params - const projectStateHash = req.query != null ? req.query.state : undefined + const projectStateHash = req.query.state // exclude is string of existing docs "id:version,id:version,..." const excludeItems = req.query.exclude != null ? req.query.exclude.split(',') : [] @@ -100,7 +87,7 @@ module.exports = HttpController = { timer.done() if (error instanceof Errors.ProjectStateChangedError) { res.sendStatus(409) // conflict - } else if (error != null) { + } else if (error) { next(error) } else { logger.log( @@ -117,15 +104,12 @@ module.exports = HttpController = { }, clearProjectState(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id } = req.params const timer = new Metrics.Timer('http.clearProjectState') logger.log({ project_id }, 'clearing project state via http') ProjectManager.clearProjectState(project_id, function (error) { timer.done() - if (error != null) { + if (error) { next(error) } else { res.sendStatus(200) @@ -134,9 +118,6 @@ module.exports = HttpController = { }, setDoc(req, res, next) { - if (next == null) { - next = function (error) {} - } const { doc_id } = req.params const { project_id } = req.params const { lines, source, user_id, undoing } = req.body @@ -162,7 +143,7 @@ module.exports = HttpController = { undoing, function (error) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log({ project_id, doc_id }, 'set doc via http') @@ -172,9 +153,6 @@ module.exports = HttpController = { }, // No Content flushDocIfLoaded(req, res, next) { - if (next == null) { - next = function (error) {} - } const { doc_id } = req.params const { project_id } = req.params logger.log({ project_id, doc_id }, 'flushing doc via http') @@ -183,7 +161,7 @@ module.exports = HttpController = { error ) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log({ project_id, doc_id }, 'flushed doc via http') @@ -192,9 +170,6 @@ module.exports = HttpController = { }, // No Content deleteDoc(req, res, next) { - if (next == null) { - next = function (error) {} - } const { doc_id } = req.params const { project_id } = req.params const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' @@ -210,7 +185,7 @@ module.exports = HttpController = { // failed and sometimes it is required HistoryManager.flushProjectChangesAsync(project_id) - if (error != null) { + if (error) { return next(error) } logger.log({ project_id, doc_id }, 'deleted doc via http') @@ -220,15 +195,12 @@ module.exports = HttpController = { }, // No Content flushProject(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id } = req.params logger.log({ project_id }, 'flushing project via http') const timer = new Metrics.Timer('http.flushProject') ProjectManager.flushProjectWithLocks(project_id, function (error) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log({ project_id }, 'flushed project via http') @@ -237,21 +209,18 @@ module.exports = HttpController = { }, // No Content deleteProject(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id } = req.params logger.log({ project_id }, 'deleting project via http') const options = {} - if (req.query != null ? req.query.background : undefined) { + if (req.query.background) { options.background = true } // allow non-urgent flushes to be queued - if (req.query != null ? req.query.shutdown : undefined) { + if (req.query.shutdown) { options.skip_history_flush = true } // don't flush history when realtime shuts down - if (req.query != null ? req.query.background : undefined) { + if (req.query.background) { ProjectManager.queueFlushAndDeleteProject(project_id, function (error) { - if (error != null) { + if (error) { return next(error) } logger.log({ project_id }, 'queue delete of project via http') @@ -264,7 +233,7 @@ module.exports = HttpController = { options, function (error) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log({ project_id }, 'deleted project via http') @@ -275,11 +244,7 @@ module.exports = HttpController = { }, // No Content deleteMultipleProjects(req, res, next) { - if (next == null) { - next = function (error) {} - } - const project_ids = - (req.body != null ? req.body.project_ids : undefined) || [] + const project_ids = req.body.project_ids || [] logger.log({ project_ids }, 'deleting multiple projects via http') async.eachSeries( project_ids, @@ -288,7 +253,7 @@ module.exports = HttpController = { ProjectManager.queueFlushAndDeleteProject(project_id, cb) }, function (error) { - if (error != null) { + if (error) { return next(error) } res.sendStatus(204) @@ -297,11 +262,8 @@ module.exports = HttpController = { }, // No Content acceptChanges(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id, doc_id } = req.params - let change_ids = req.body != null ? req.body.change_ids : undefined + let change_ids = req.body.change_ids if (change_ids == null) { change_ids = [req.params.change_id] } @@ -316,7 +278,7 @@ module.exports = HttpController = { change_ids, function (error) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log( @@ -329,9 +291,6 @@ module.exports = HttpController = { }, // No Content deleteComment(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id, doc_id, comment_id } = req.params logger.log({ project_id, doc_id, comment_id }, 'deleting comment via http') const timer = new Metrics.Timer('http.deleteComment') @@ -341,7 +300,7 @@ module.exports = HttpController = { comment_id, function (error) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log( @@ -354,9 +313,6 @@ module.exports = HttpController = { }, // No Content updateProject(req, res, next) { - if (next == null) { - next = function (error) {} - } const timer = new Metrics.Timer('http.updateProject') const { project_id } = req.params const { @@ -380,7 +336,7 @@ module.exports = HttpController = { version, function (error) { timer.done() - if (error != null) { + if (error) { return next(error) } logger.log({ project_id }, 'updated project via http') @@ -390,9 +346,6 @@ module.exports = HttpController = { }, // No Content resyncProjectHistory(req, res, next) { - if (next == null) { - next = function (error) {} - } const { project_id } = req.params const { projectHistoryId, docs, files } = req.body @@ -406,7 +359,7 @@ module.exports = HttpController = { docs, files, function (error) { - if (error != null) { + if (error) { return next(error) } logger.log({ project_id }, 'queued project history resync via http') @@ -416,9 +369,6 @@ module.exports = HttpController = { }, flushAllProjects(req, res, next) { - if (next == null) { - next = function (error) {} - } res.setTimeout(5 * 60 * 1000) const options = { limit: req.query.limit || 1000, @@ -426,7 +376,7 @@ module.exports = HttpController = { dryRun: req.query.dryRun || false } ProjectFlusher.flushAllProjects(options, function (err, project_ids) { - if (err != null) { + if (err) { logger.err({ err }, 'error bulk flushing projects') res.sendStatus(500) } else { @@ -436,9 +386,6 @@ module.exports = HttpController = { }, flushQueuedProjects(req, res, next) { - if (next == null) { - next = function (error) {} - } res.setTimeout(10 * 60 * 1000) const options = { limit: req.query.limit || 1000, @@ -449,7 +396,7 @@ module.exports = HttpController = { err, flushed ) { - if (err != null) { + if (err) { logger.err({ err }, 'error flushing old projects') res.sendStatus(500) } else { From 64a881461f761a1ca887a1ff48a46da6e39b55ea Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:07:15 -0400 Subject: [PATCH 16/23] Decaf cleanup: camel case variables --- .../document-updater/app/js/HttpController.js | 173 +++++++++--------- 1 file changed, 85 insertions(+), 88 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 800aee4c44..61258ad172 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -1,7 +1,3 @@ -/* eslint-disable - camelcase, - handle-callback-err, -*/ let HttpController const DocumentManager = require('./DocumentManager') const HistoryManager = require('./HistoryManager') @@ -18,9 +14,9 @@ const TWO_MEGABYTES = 2 * 1024 * 1024 module.exports = HttpController = { getDoc(req, res, next) { let fromVersion - const { doc_id } = req.params - const { project_id } = req.params - logger.log({ project_id, doc_id }, 'getting doc via http') + const docId = req.params.doc_id + const projectId = req.params.project_id + logger.log({ projectId, docId }, 'getting doc via http') const timer = new Metrics.Timer('http.getDoc') if (req.query.fromVersion != null) { @@ -30,20 +26,20 @@ module.exports = HttpController = { } DocumentManager.getDocAndRecentOpsWithLock( - project_id, - doc_id, + projectId, + docId, fromVersion, function (error, lines, version, ops, ranges, pathname) { timer.done() if (error) { return next(error) } - logger.log({ project_id, doc_id }, 'got doc via http') + logger.log({ projectId, docId }, 'got doc via http') if (lines == null || version == null) { return next(new Errors.NotFoundError('document not found')) } res.json({ - id: doc_id, + id: docId, lines, version, ops, @@ -63,12 +59,12 @@ module.exports = HttpController = { }, getProjectDocsAndFlushIfOld(req, res, next) { - const { project_id } = req.params + const projectId = req.params.project_id const projectStateHash = req.query.state // exclude is string of existing docs "id:version,id:version,..." const excludeItems = req.query.exclude != null ? req.query.exclude.split(',') : [] - logger.log({ project_id, exclude: excludeItems }, 'getting docs via http') + logger.log({ projectId, exclude: excludeItems }, 'getting docs via http') const timer = new Metrics.Timer('http.getAllDocs') const excludeVersions = {} for (const item of excludeItems) { @@ -76,11 +72,11 @@ module.exports = HttpController = { excludeVersions[id] = version } logger.log( - { project_id, projectStateHash, excludeVersions }, + { projectId, projectStateHash, excludeVersions }, 'excluding versions' ) ProjectManager.getProjectDocsAndFlushIfOld( - project_id, + projectId, projectStateHash, excludeVersions, function (error, result) { @@ -92,7 +88,7 @@ module.exports = HttpController = { } else { logger.log( { - project_id, + projectId, result: result.map((doc) => `${doc._id}:${doc.v}`) }, 'got docs via http' @@ -104,10 +100,10 @@ module.exports = HttpController = { }, clearProjectState(req, res, next) { - const { project_id } = req.params + const projectId = req.params.project_id const timer = new Metrics.Timer('http.clearProjectState') - logger.log({ project_id }, 'clearing project state via http') - ProjectManager.clearProjectState(project_id, function (error) { + logger.log({ projectId }, 'clearing project state via http') + ProjectManager.clearProjectState(projectId, function (error) { timer.done() if (error) { next(error) @@ -118,99 +114,99 @@ module.exports = HttpController = { }, setDoc(req, res, next) { - const { doc_id } = req.params - const { project_id } = req.params - const { lines, source, user_id, undoing } = req.body + const docId = req.params.doc_id + const projectId = req.params.project_id + const { lines, source, user_id: userId, undoing } = req.body const lineSize = HttpController._getTotalSizeOfLines(lines) if (lineSize > TWO_MEGABYTES) { logger.log( - { project_id, doc_id, source, lineSize, user_id }, + { projectId, docId, source, lineSize, userId }, 'document too large, returning 406 response' ) return res.sendStatus(406) } logger.log( - { project_id, doc_id, lines, source, user_id, undoing }, + { projectId, docId, lines, source, userId, undoing }, 'setting doc via http' ) const timer = new Metrics.Timer('http.setDoc') DocumentManager.setDocWithLock( - project_id, - doc_id, + projectId, + docId, lines, source, - user_id, + userId, undoing, function (error) { timer.done() if (error) { return next(error) } - logger.log({ project_id, doc_id }, 'set doc via http') + logger.log({ projectId, docId }, 'set doc via http') res.sendStatus(204) } ) }, // No Content flushDocIfLoaded(req, res, next) { - const { doc_id } = req.params - const { project_id } = req.params - logger.log({ project_id, doc_id }, 'flushing doc via http') + const docId = req.params.doc_id + const projectId = req.params.project_id + logger.log({ projectId, docId }, 'flushing doc via http') const timer = new Metrics.Timer('http.flushDoc') - DocumentManager.flushDocIfLoadedWithLock(project_id, doc_id, function ( + DocumentManager.flushDocIfLoadedWithLock(projectId, docId, function ( error ) { timer.done() if (error) { return next(error) } - logger.log({ project_id, doc_id }, 'flushed doc via http') + logger.log({ projectId, docId }, 'flushed doc via http') res.sendStatus(204) }) }, // No Content deleteDoc(req, res, next) { - const { doc_id } = req.params - const { project_id } = req.params + const docId = req.params.doc_id + const projectId = req.params.project_id const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' const timer = new Metrics.Timer('http.deleteDoc') - logger.log({ project_id, doc_id }, 'deleting doc via http') + logger.log({ projectId, docId }, 'deleting doc via http') DocumentManager.flushAndDeleteDocWithLock( - project_id, - doc_id, + projectId, + docId, { ignoreFlushErrors }, function (error) { timer.done() // There is no harm in flushing project history if the previous call // failed and sometimes it is required - HistoryManager.flushProjectChangesAsync(project_id) + HistoryManager.flushProjectChangesAsync(projectId) if (error) { return next(error) } - logger.log({ project_id, doc_id }, 'deleted doc via http') + logger.log({ projectId, docId }, 'deleted doc via http') res.sendStatus(204) } ) }, // No Content flushProject(req, res, next) { - const { project_id } = req.params - logger.log({ project_id }, 'flushing project via http') + const projectId = req.params.project_id + logger.log({ projectId }, 'flushing project via http') const timer = new Metrics.Timer('http.flushProject') - ProjectManager.flushProjectWithLocks(project_id, function (error) { + ProjectManager.flushProjectWithLocks(projectId, function (error) { timer.done() if (error) { return next(error) } - logger.log({ project_id }, 'flushed project via http') + logger.log({ projectId }, 'flushed project via http') res.sendStatus(204) }) }, // No Content deleteProject(req, res, next) { - const { project_id } = req.params - logger.log({ project_id }, 'deleting project via http') + const projectId = req.params.project_id + logger.log({ projectId }, 'deleting project via http') const options = {} if (req.query.background) { options.background = true @@ -219,24 +215,24 @@ module.exports = HttpController = { options.skip_history_flush = true } // don't flush history when realtime shuts down if (req.query.background) { - ProjectManager.queueFlushAndDeleteProject(project_id, function (error) { + ProjectManager.queueFlushAndDeleteProject(projectId, function (error) { if (error) { return next(error) } - logger.log({ project_id }, 'queue delete of project via http') + logger.log({ projectId }, 'queue delete of project via http') res.sendStatus(204) }) // No Content } else { const timer = new Metrics.Timer('http.deleteProject') ProjectManager.flushAndDeleteProjectWithLocks( - project_id, + projectId, options, function (error) { timer.done() if (error) { return next(error) } - logger.log({ project_id }, 'deleted project via http') + logger.log({ projectId }, 'deleted project via http') res.sendStatus(204) } ) @@ -244,13 +240,13 @@ module.exports = HttpController = { }, // No Content deleteMultipleProjects(req, res, next) { - const project_ids = req.body.project_ids || [] - logger.log({ project_ids }, 'deleting multiple projects via http') + const projectIds = req.body.project_ids || [] + logger.log({ projectIds }, 'deleting multiple projects via http') async.eachSeries( - project_ids, - function (project_id, cb) { - logger.log({ project_id }, 'queue delete of project via http') - ProjectManager.queueFlushAndDeleteProject(project_id, cb) + projectIds, + function (projectId, cb) { + logger.log({ projectId }, 'queue delete of project via http') + ProjectManager.queueFlushAndDeleteProject(projectId, cb) }, function (error) { if (error) { @@ -262,51 +258,52 @@ module.exports = HttpController = { }, // No Content acceptChanges(req, res, next) { - const { project_id, doc_id } = req.params - let change_ids = req.body.change_ids - if (change_ids == null) { - change_ids = [req.params.change_id] + const { project_id: projectId, doc_id: docId } = req.params + let changeIds = req.body.change_ids + if (changeIds == null) { + changeIds = [req.params.change_id] } logger.log( - { project_id, doc_id }, - `accepting ${change_ids.length} changes via http` + { projectId, docId }, + `accepting ${changeIds.length} changes via http` ) const timer = new Metrics.Timer('http.acceptChanges') DocumentManager.acceptChangesWithLock( - project_id, - doc_id, - change_ids, + projectId, + docId, + changeIds, function (error) { timer.done() if (error) { return next(error) } logger.log( - { project_id, doc_id }, - `accepted ${change_ids.length} changes via http` + { projectId, docId }, + `accepted ${changeIds.length} changes via http` ) - res.sendStatus(204) + res.sendStatus(204) // No Content } ) - }, // No Content + }, deleteComment(req, res, next) { - const { project_id, doc_id, comment_id } = req.params - logger.log({ project_id, doc_id, comment_id }, 'deleting comment via http') + const { + project_id: projectId, + doc_id: docId, + comment_id: commentId + } = req.params + logger.log({ projectId, docId, commentId }, 'deleting comment via http') const timer = new Metrics.Timer('http.deleteComment') DocumentManager.deleteCommentWithLock( - project_id, - doc_id, - comment_id, + projectId, + docId, + commentId, function (error) { timer.done() if (error) { return next(error) } - logger.log( - { project_id, doc_id, comment_id }, - 'deleted comment via http' - ) + logger.log({ projectId, docId, commentId }, 'deleted comment via http') res.sendStatus(204) } ) @@ -314,7 +311,7 @@ module.exports = HttpController = { updateProject(req, res, next) { const timer = new Metrics.Timer('http.updateProject') - const { project_id } = req.params + const projectId = req.params.project_id const { projectHistoryId, userId, @@ -323,12 +320,12 @@ module.exports = HttpController = { version } = req.body logger.log( - { project_id, docUpdates, fileUpdates, version }, + { projectId, docUpdates, fileUpdates, version }, 'updating project via http' ) ProjectManager.updateProjectWithLocks( - project_id, + projectId, projectHistoryId, userId, docUpdates, @@ -339,22 +336,22 @@ module.exports = HttpController = { if (error) { return next(error) } - logger.log({ project_id }, 'updated project via http') + logger.log({ projectId }, 'updated project via http') res.sendStatus(204) } ) }, // No Content resyncProjectHistory(req, res, next) { - const { project_id } = req.params + const projectId = req.params.project_id const { projectHistoryId, docs, files } = req.body logger.log( - { project_id, docs, files }, + { projectId, docs, files }, 'queuing project history resync via http' ) HistoryManager.resyncProjectHistory( - project_id, + projectId, projectHistoryId, docs, files, @@ -362,7 +359,7 @@ module.exports = HttpController = { if (error) { return next(error) } - logger.log({ project_id }, 'queued project history resync via http') + logger.log({ projectId }, 'queued project history resync via http') res.sendStatus(204) } ) @@ -375,12 +372,12 @@ module.exports = HttpController = { concurrency: req.query.concurrency || 5, dryRun: req.query.dryRun || false } - ProjectFlusher.flushAllProjects(options, function (err, project_ids) { + ProjectFlusher.flushAllProjects(options, function (err, projectIds) { if (err) { logger.err({ err }, 'error bulk flushing projects') res.sendStatus(500) } else { - res.send(project_ids) + res.send(projectIds) } }) }, From e4ac63dd1942fb2d0976f7af2e1760b6ec93b7a9 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:12:15 -0400 Subject: [PATCH 17/23] Decaf cleanup: move functions to top level --- .../document-updater/app/js/HttpController.js | 739 +++++++++--------- 1 file changed, 372 insertions(+), 367 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 61258ad172..5895bececd 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -1,4 +1,3 @@ -let HttpController const DocumentManager = require('./DocumentManager') const HistoryManager = require('./HistoryManager') const ProjectManager = require('./ProjectManager') @@ -11,395 +10,401 @@ const async = require('async') const TWO_MEGABYTES = 2 * 1024 * 1024 -module.exports = HttpController = { - getDoc(req, res, next) { - let fromVersion - const docId = req.params.doc_id - const projectId = req.params.project_id - logger.log({ projectId, docId }, 'getting doc via http') - const timer = new Metrics.Timer('http.getDoc') +module.exports = { + getDoc, + getProjectDocsAndFlushIfOld, + clearProjectState, + setDoc, + flushDocIfLoaded, + deleteDoc, + flushProject, + deleteProject, + deleteMultipleProjects, + acceptChanges, + deleteComment, + updateProject, + resyncProjectHistory, + flushAllProjects, + flushQueuedProjects +} - if (req.query.fromVersion != null) { - fromVersion = parseInt(req.query.fromVersion, 10) - } else { - fromVersion = -1 - } +function getDoc(req, res, next) { + let fromVersion + const docId = req.params.doc_id + const projectId = req.params.project_id + logger.log({ projectId, docId }, 'getting doc via http') + const timer = new Metrics.Timer('http.getDoc') - DocumentManager.getDocAndRecentOpsWithLock( - projectId, - docId, - fromVersion, - function (error, lines, version, ops, ranges, pathname) { - timer.done() - if (error) { - return next(error) - } - logger.log({ projectId, docId }, 'got doc via http') - if (lines == null || version == null) { - return next(new Errors.NotFoundError('document not found')) - } - res.json({ - id: docId, - lines, - version, - ops, - ranges, - pathname - }) - } - ) - }, + if (req.query.fromVersion != null) { + fromVersion = parseInt(req.query.fromVersion, 10) + } else { + fromVersion = -1 + } - _getTotalSizeOfLines(lines) { - let size = 0 - for (const line of lines) { - size += line.length + 1 - } - return size - }, - - getProjectDocsAndFlushIfOld(req, res, next) { - const projectId = req.params.project_id - const projectStateHash = req.query.state - // exclude is string of existing docs "id:version,id:version,..." - const excludeItems = - req.query.exclude != null ? req.query.exclude.split(',') : [] - logger.log({ projectId, exclude: excludeItems }, 'getting docs via http') - const timer = new Metrics.Timer('http.getAllDocs') - const excludeVersions = {} - for (const item of excludeItems) { - const [id, version] = item.split(':') - excludeVersions[id] = version - } - logger.log( - { projectId, projectStateHash, excludeVersions }, - 'excluding versions' - ) - ProjectManager.getProjectDocsAndFlushIfOld( - projectId, - projectStateHash, - excludeVersions, - function (error, result) { - timer.done() - if (error instanceof Errors.ProjectStateChangedError) { - res.sendStatus(409) // conflict - } else if (error) { - next(error) - } else { - logger.log( - { - projectId, - result: result.map((doc) => `${doc._id}:${doc.v}`) - }, - 'got docs via http' - ) - res.send(result) - } - } - ) - }, - - clearProjectState(req, res, next) { - const projectId = req.params.project_id - const timer = new Metrics.Timer('http.clearProjectState') - logger.log({ projectId }, 'clearing project state via http') - ProjectManager.clearProjectState(projectId, function (error) { + DocumentManager.getDocAndRecentOpsWithLock( + projectId, + docId, + fromVersion, + function (error, lines, version, ops, ranges, pathname) { timer.done() if (error) { + return next(error) + } + logger.log({ projectId, docId }, 'got doc via http') + if (lines == null || version == null) { + return next(new Errors.NotFoundError('document not found')) + } + res.json({ + id: docId, + lines, + version, + ops, + ranges, + pathname + }) + } + ) +} + +function _getTotalSizeOfLines(lines) { + let size = 0 + for (const line of lines) { + size += line.length + 1 + } + return size +} + +function getProjectDocsAndFlushIfOld(req, res, next) { + const projectId = req.params.project_id + const projectStateHash = req.query.state + // exclude is string of existing docs "id:version,id:version,..." + const excludeItems = + req.query.exclude != null ? req.query.exclude.split(',') : [] + logger.log({ projectId, exclude: excludeItems }, 'getting docs via http') + const timer = new Metrics.Timer('http.getAllDocs') + const excludeVersions = {} + for (const item of excludeItems) { + const [id, version] = item.split(':') + excludeVersions[id] = version + } + logger.log( + { projectId, projectStateHash, excludeVersions }, + 'excluding versions' + ) + ProjectManager.getProjectDocsAndFlushIfOld( + projectId, + projectStateHash, + excludeVersions, + function (error, result) { + timer.done() + if (error instanceof Errors.ProjectStateChangedError) { + res.sendStatus(409) // conflict + } else if (error) { next(error) } else { - res.sendStatus(200) + logger.log( + { + projectId, + result: result.map((doc) => `${doc._id}:${doc.v}`) + }, + 'got docs via http' + ) + res.send(result) } - }) - }, - - setDoc(req, res, next) { - const docId = req.params.doc_id - const projectId = req.params.project_id - const { lines, source, user_id: userId, undoing } = req.body - const lineSize = HttpController._getTotalSizeOfLines(lines) - if (lineSize > TWO_MEGABYTES) { - logger.log( - { projectId, docId, source, lineSize, userId }, - 'document too large, returning 406 response' - ) - return res.sendStatus(406) } - logger.log( - { projectId, docId, lines, source, userId, undoing }, - 'setting doc via http' - ) - const timer = new Metrics.Timer('http.setDoc') - DocumentManager.setDocWithLock( - projectId, - docId, - lines, - source, - userId, - undoing, - function (error) { - timer.done() - if (error) { - return next(error) - } - logger.log({ projectId, docId }, 'set doc via http') - res.sendStatus(204) - } - ) - }, // No Content + ) +} - flushDocIfLoaded(req, res, next) { - const docId = req.params.doc_id - const projectId = req.params.project_id - logger.log({ projectId, docId }, 'flushing doc via http') - const timer = new Metrics.Timer('http.flushDoc') - DocumentManager.flushDocIfLoadedWithLock(projectId, docId, function ( +function clearProjectState(req, res, next) { + const projectId = req.params.project_id + const timer = new Metrics.Timer('http.clearProjectState') + logger.log({ projectId }, 'clearing project state via http') + ProjectManager.clearProjectState(projectId, function (error) { + timer.done() + if (error) { + next(error) + } else { + res.sendStatus(200) + } + }) +} + +function setDoc(req, res, next) { + const docId = req.params.doc_id + const projectId = req.params.project_id + const { lines, source, user_id: userId, undoing } = req.body + const lineSize = _getTotalSizeOfLines(lines) + if (lineSize > TWO_MEGABYTES) { + logger.log( + { projectId, docId, source, lineSize, userId }, + 'document too large, returning 406 response' + ) + return res.sendStatus(406) + } + logger.log( + { projectId, docId, lines, source, userId, undoing }, + 'setting doc via http' + ) + const timer = new Metrics.Timer('http.setDoc') + DocumentManager.setDocWithLock( + projectId, + docId, + lines, + source, + userId, + undoing, + function (error) { + timer.done() + if (error) { + return next(error) + } + logger.log({ projectId, docId }, 'set doc via http') + res.sendStatus(204) // No Content + } + ) +} + +function flushDocIfLoaded(req, res, next) { + const docId = req.params.doc_id + const projectId = req.params.project_id + logger.log({ projectId, docId }, 'flushing doc via http') + const timer = new Metrics.Timer('http.flushDoc') + DocumentManager.flushDocIfLoadedWithLock(projectId, docId, function (error) { + timer.done() + if (error) { + return next(error) + } + logger.log({ projectId, docId }, 'flushed doc via http') + res.sendStatus(204) // No Content + }) +} + +function deleteDoc(req, res, next) { + const docId = req.params.doc_id + const projectId = req.params.project_id + const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' + const timer = new Metrics.Timer('http.deleteDoc') + logger.log({ projectId, docId }, 'deleting doc via http') + DocumentManager.flushAndDeleteDocWithLock( + projectId, + docId, + { ignoreFlushErrors }, + function (error) { + timer.done() + // There is no harm in flushing project history if the previous call + // failed and sometimes it is required + HistoryManager.flushProjectChangesAsync(projectId) + + if (error) { + return next(error) + } + logger.log({ projectId, docId }, 'deleted doc via http') + res.sendStatus(204) // No Content + } + ) +} + +function flushProject(req, res, next) { + const projectId = req.params.project_id + logger.log({ projectId }, 'flushing project via http') + const timer = new Metrics.Timer('http.flushProject') + ProjectManager.flushProjectWithLocks(projectId, function (error) { + timer.done() + if (error) { + return next(error) + } + logger.log({ projectId }, 'flushed project via http') + res.sendStatus(204) // No Content + }) +} + +function deleteProject(req, res, next) { + const projectId = req.params.project_id + logger.log({ projectId }, 'deleting project via http') + const options = {} + if (req.query.background) { + options.background = true + } // allow non-urgent flushes to be queued + if (req.query.shutdown) { + options.skip_history_flush = true + } // don't flush history when realtime shuts down + if (req.query.background) { + ProjectManager.queueFlushAndDeleteProject(projectId, function (error) { + if (error) { + return next(error) + } + logger.log({ projectId }, 'queue delete of project via http') + res.sendStatus(204) + }) // No Content + } else { + const timer = new Metrics.Timer('http.deleteProject') + ProjectManager.flushAndDeleteProjectWithLocks(projectId, options, function ( error ) { timer.done() if (error) { return next(error) } - logger.log({ projectId, docId }, 'flushed doc via http') - res.sendStatus(204) + logger.log({ projectId }, 'deleted project via http') + res.sendStatus(204) // No Content }) - }, // No Content + } +} - deleteDoc(req, res, next) { - const docId = req.params.doc_id - const projectId = req.params.project_id - const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' - const timer = new Metrics.Timer('http.deleteDoc') - logger.log({ projectId, docId }, 'deleting doc via http') - DocumentManager.flushAndDeleteDocWithLock( - projectId, - docId, - { ignoreFlushErrors }, - function (error) { - timer.done() - // There is no harm in flushing project history if the previous call - // failed and sometimes it is required - HistoryManager.flushProjectChangesAsync(projectId) - - if (error) { - return next(error) - } - logger.log({ projectId, docId }, 'deleted doc via http') - res.sendStatus(204) +function deleteMultipleProjects(req, res, next) { + const projectIds = req.body.project_ids || [] + logger.log({ projectIds }, 'deleting multiple projects via http') + async.eachSeries( + projectIds, + function (projectId, cb) { + logger.log({ projectId }, 'queue delete of project via http') + ProjectManager.queueFlushAndDeleteProject(projectId, cb) + }, + function (error) { + if (error) { + return next(error) } - ) - }, // No Content + res.sendStatus(204) // No Content + } + ) +} - flushProject(req, res, next) { - const projectId = req.params.project_id - logger.log({ projectId }, 'flushing project via http') - const timer = new Metrics.Timer('http.flushProject') - ProjectManager.flushProjectWithLocks(projectId, function (error) { +function acceptChanges(req, res, next) { + const { project_id: projectId, doc_id: docId } = req.params + let changeIds = req.body.change_ids + if (changeIds == null) { + changeIds = [req.params.change_id] + } + logger.log( + { projectId, docId }, + `accepting ${changeIds.length} changes via http` + ) + const timer = new Metrics.Timer('http.acceptChanges') + DocumentManager.acceptChangesWithLock(projectId, docId, changeIds, function ( + error + ) { + timer.done() + if (error) { + return next(error) + } + logger.log( + { projectId, docId }, + `accepted ${changeIds.length} changes via http` + ) + res.sendStatus(204) // No Content + }) +} + +function deleteComment(req, res, next) { + const { + project_id: projectId, + doc_id: docId, + comment_id: commentId + } = req.params + logger.log({ projectId, docId, commentId }, 'deleting comment via http') + const timer = new Metrics.Timer('http.deleteComment') + DocumentManager.deleteCommentWithLock(projectId, docId, commentId, function ( + error + ) { + timer.done() + if (error) { + return next(error) + } + logger.log({ projectId, docId, commentId }, 'deleted comment via http') + res.sendStatus(204) // No Content + }) +} + +function updateProject(req, res, next) { + const timer = new Metrics.Timer('http.updateProject') + const projectId = req.params.project_id + const { + projectHistoryId, + userId, + docUpdates, + fileUpdates, + version + } = req.body + logger.log( + { projectId, docUpdates, fileUpdates, version }, + 'updating project via http' + ) + + ProjectManager.updateProjectWithLocks( + projectId, + projectHistoryId, + userId, + docUpdates, + fileUpdates, + version, + function (error) { timer.done() if (error) { return next(error) } - logger.log({ projectId }, 'flushed project via http') - res.sendStatus(204) - }) - }, // No Content - - deleteProject(req, res, next) { - const projectId = req.params.project_id - logger.log({ projectId }, 'deleting project via http') - const options = {} - if (req.query.background) { - options.background = true - } // allow non-urgent flushes to be queued - if (req.query.shutdown) { - options.skip_history_flush = true - } // don't flush history when realtime shuts down - if (req.query.background) { - ProjectManager.queueFlushAndDeleteProject(projectId, function (error) { - if (error) { - return next(error) - } - logger.log({ projectId }, 'queue delete of project via http') - res.sendStatus(204) - }) // No Content - } else { - const timer = new Metrics.Timer('http.deleteProject') - ProjectManager.flushAndDeleteProjectWithLocks( - projectId, - options, - function (error) { - timer.done() - if (error) { - return next(error) - } - logger.log({ projectId }, 'deleted project via http') - res.sendStatus(204) - } - ) + logger.log({ projectId }, 'updated project via http') + res.sendStatus(204) // No Content } - }, // No Content - - deleteMultipleProjects(req, res, next) { - const projectIds = req.body.project_ids || [] - logger.log({ projectIds }, 'deleting multiple projects via http') - async.eachSeries( - projectIds, - function (projectId, cb) { - logger.log({ projectId }, 'queue delete of project via http') - ProjectManager.queueFlushAndDeleteProject(projectId, cb) - }, - function (error) { - if (error) { - return next(error) - } - res.sendStatus(204) - } - ) - }, // No Content - - acceptChanges(req, res, next) { - const { project_id: projectId, doc_id: docId } = req.params - let changeIds = req.body.change_ids - if (changeIds == null) { - changeIds = [req.params.change_id] - } - logger.log( - { projectId, docId }, - `accepting ${changeIds.length} changes via http` - ) - const timer = new Metrics.Timer('http.acceptChanges') - DocumentManager.acceptChangesWithLock( - projectId, - docId, - changeIds, - function (error) { - timer.done() - if (error) { - return next(error) - } - logger.log( - { projectId, docId }, - `accepted ${changeIds.length} changes via http` - ) - res.sendStatus(204) // No Content - } - ) - }, - - deleteComment(req, res, next) { - const { - project_id: projectId, - doc_id: docId, - comment_id: commentId - } = req.params - logger.log({ projectId, docId, commentId }, 'deleting comment via http') - const timer = new Metrics.Timer('http.deleteComment') - DocumentManager.deleteCommentWithLock( - projectId, - docId, - commentId, - function (error) { - timer.done() - if (error) { - return next(error) - } - logger.log({ projectId, docId, commentId }, 'deleted comment via http') - res.sendStatus(204) - } - ) - }, // No Content - - updateProject(req, res, next) { - const timer = new Metrics.Timer('http.updateProject') - const projectId = req.params.project_id - const { - projectHistoryId, - userId, - docUpdates, - fileUpdates, - version - } = req.body - logger.log( - { projectId, docUpdates, fileUpdates, version }, - 'updating project via http' - ) - - ProjectManager.updateProjectWithLocks( - projectId, - projectHistoryId, - userId, - docUpdates, - fileUpdates, - version, - function (error) { - timer.done() - if (error) { - return next(error) - } - logger.log({ projectId }, 'updated project via http') - res.sendStatus(204) - } - ) - }, // No Content - - resyncProjectHistory(req, res, next) { - const projectId = req.params.project_id - const { projectHistoryId, docs, files } = req.body - - logger.log( - { projectId, docs, files }, - 'queuing project history resync via http' - ) - HistoryManager.resyncProjectHistory( - projectId, - projectHistoryId, - docs, - files, - function (error) { - if (error) { - return next(error) - } - logger.log({ projectId }, 'queued project history resync via http') - res.sendStatus(204) - } - ) - }, - - flushAllProjects(req, res, next) { - res.setTimeout(5 * 60 * 1000) - const options = { - limit: req.query.limit || 1000, - concurrency: req.query.concurrency || 5, - dryRun: req.query.dryRun || false - } - ProjectFlusher.flushAllProjects(options, function (err, projectIds) { - if (err) { - logger.err({ err }, 'error bulk flushing projects') - res.sendStatus(500) - } else { - res.send(projectIds) - } - }) - }, - - flushQueuedProjects(req, res, next) { - res.setTimeout(10 * 60 * 1000) - const options = { - limit: req.query.limit || 1000, - timeout: 5 * 60 * 1000, - min_delete_age: req.query.min_delete_age || 5 * 60 * 1000 - } - DeleteQueueManager.flushAndDeleteOldProjects(options, function ( - err, - flushed - ) { - if (err) { - logger.err({ err }, 'error flushing old projects') - res.sendStatus(500) - } else { - logger.log({ flushed }, 'flush of queued projects completed') - res.send({ flushed }) - } - }) - } + ) +} + +function resyncProjectHistory(req, res, next) { + const projectId = req.params.project_id + const { projectHistoryId, docs, files } = req.body + + logger.log( + { projectId, docs, files }, + 'queuing project history resync via http' + ) + HistoryManager.resyncProjectHistory( + projectId, + projectHistoryId, + docs, + files, + function (error) { + if (error) { + return next(error) + } + logger.log({ projectId }, 'queued project history resync via http') + res.sendStatus(204) + } + ) +} + +function flushAllProjects(req, res, next) { + res.setTimeout(5 * 60 * 1000) + const options = { + limit: req.query.limit || 1000, + concurrency: req.query.concurrency || 5, + dryRun: req.query.dryRun || false + } + ProjectFlusher.flushAllProjects(options, function (err, projectIds) { + if (err) { + logger.err({ err }, 'error bulk flushing projects') + res.sendStatus(500) + } else { + res.send(projectIds) + } + }) +} + +function flushQueuedProjects(req, res, next) { + res.setTimeout(10 * 60 * 1000) + const options = { + limit: req.query.limit || 1000, + timeout: 5 * 60 * 1000, + min_delete_age: req.query.min_delete_age || 5 * 60 * 1000 + } + DeleteQueueManager.flushAndDeleteOldProjects(options, function ( + err, + flushed + ) { + if (err) { + logger.err({ err }, 'error flushing old projects') + res.sendStatus(500) + } else { + logger.log({ flushed }, 'flush of queued projects completed') + res.send({ flushed }) + } + }) } From cb959ddfc168015773d170fa68f1e54aadfa5e3f Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:14:37 -0400 Subject: [PATCH 18/23] Decaf cleanup: use arrow functions for callbacks --- .../document-updater/app/js/HttpController.js | 99 ++++++++++--------- 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 5895bececd..2e8bd084a5 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -45,7 +45,7 @@ function getDoc(req, res, next) { projectId, docId, fromVersion, - function (error, lines, version, ops, ranges, pathname) { + (error, lines, version, ops, ranges, pathname) => { timer.done() if (error) { return next(error) @@ -95,7 +95,7 @@ function getProjectDocsAndFlushIfOld(req, res, next) { projectId, projectStateHash, excludeVersions, - function (error, result) { + (error, result) => { timer.done() if (error instanceof Errors.ProjectStateChangedError) { res.sendStatus(409) // conflict @@ -119,7 +119,7 @@ function clearProjectState(req, res, next) { const projectId = req.params.project_id const timer = new Metrics.Timer('http.clearProjectState') logger.log({ projectId }, 'clearing project state via http') - ProjectManager.clearProjectState(projectId, function (error) { + ProjectManager.clearProjectState(projectId, (error) => { timer.done() if (error) { next(error) @@ -153,7 +153,7 @@ function setDoc(req, res, next) { source, userId, undoing, - function (error) { + (error) => { timer.done() if (error) { return next(error) @@ -169,7 +169,7 @@ function flushDocIfLoaded(req, res, next) { const projectId = req.params.project_id logger.log({ projectId, docId }, 'flushing doc via http') const timer = new Metrics.Timer('http.flushDoc') - DocumentManager.flushDocIfLoadedWithLock(projectId, docId, function (error) { + DocumentManager.flushDocIfLoadedWithLock(projectId, docId, (error) => { timer.done() if (error) { return next(error) @@ -189,7 +189,7 @@ function deleteDoc(req, res, next) { projectId, docId, { ignoreFlushErrors }, - function (error) { + (error) => { timer.done() // There is no harm in flushing project history if the previous call // failed and sometimes it is required @@ -208,7 +208,7 @@ function flushProject(req, res, next) { const projectId = req.params.project_id logger.log({ projectId }, 'flushing project via http') const timer = new Metrics.Timer('http.flushProject') - ProjectManager.flushProjectWithLocks(projectId, function (error) { + ProjectManager.flushProjectWithLocks(projectId, (error) => { timer.done() if (error) { return next(error) @@ -229,7 +229,7 @@ function deleteProject(req, res, next) { options.skip_history_flush = true } // don't flush history when realtime shuts down if (req.query.background) { - ProjectManager.queueFlushAndDeleteProject(projectId, function (error) { + ProjectManager.queueFlushAndDeleteProject(projectId, (error) => { if (error) { return next(error) } @@ -238,16 +238,18 @@ function deleteProject(req, res, next) { }) // No Content } else { const timer = new Metrics.Timer('http.deleteProject') - ProjectManager.flushAndDeleteProjectWithLocks(projectId, options, function ( - error - ) { - timer.done() - if (error) { - return next(error) + ProjectManager.flushAndDeleteProjectWithLocks( + projectId, + options, + (error) => { + timer.done() + if (error) { + return next(error) + } + logger.log({ projectId }, 'deleted project via http') + res.sendStatus(204) // No Content } - logger.log({ projectId }, 'deleted project via http') - res.sendStatus(204) // No Content - }) + ) } } @@ -256,11 +258,11 @@ function deleteMultipleProjects(req, res, next) { logger.log({ projectIds }, 'deleting multiple projects via http') async.eachSeries( projectIds, - function (projectId, cb) { + (projectId, cb) => { logger.log({ projectId }, 'queue delete of project via http') ProjectManager.queueFlushAndDeleteProject(projectId, cb) }, - function (error) { + (error) => { if (error) { return next(error) } @@ -280,19 +282,22 @@ function acceptChanges(req, res, next) { `accepting ${changeIds.length} changes via http` ) const timer = new Metrics.Timer('http.acceptChanges') - DocumentManager.acceptChangesWithLock(projectId, docId, changeIds, function ( - error - ) { - timer.done() - if (error) { - return next(error) + DocumentManager.acceptChangesWithLock( + projectId, + docId, + changeIds, + (error) => { + timer.done() + if (error) { + return next(error) + } + logger.log( + { projectId, docId }, + `accepted ${changeIds.length} changes via http` + ) + res.sendStatus(204) // No Content } - logger.log( - { projectId, docId }, - `accepted ${changeIds.length} changes via http` - ) - res.sendStatus(204) // No Content - }) + ) } function deleteComment(req, res, next) { @@ -303,16 +308,19 @@ function deleteComment(req, res, next) { } = req.params logger.log({ projectId, docId, commentId }, 'deleting comment via http') const timer = new Metrics.Timer('http.deleteComment') - DocumentManager.deleteCommentWithLock(projectId, docId, commentId, function ( - error - ) { - timer.done() - if (error) { - return next(error) + DocumentManager.deleteCommentWithLock( + projectId, + docId, + commentId, + (error) => { + timer.done() + if (error) { + return next(error) + } + logger.log({ projectId, docId, commentId }, 'deleted comment via http') + res.sendStatus(204) // No Content } - logger.log({ projectId, docId, commentId }, 'deleted comment via http') - res.sendStatus(204) // No Content - }) + ) } function updateProject(req, res, next) { @@ -337,7 +345,7 @@ function updateProject(req, res, next) { docUpdates, fileUpdates, version, - function (error) { + (error) => { timer.done() if (error) { return next(error) @@ -361,7 +369,7 @@ function resyncProjectHistory(req, res, next) { projectHistoryId, docs, files, - function (error) { + (error) => { if (error) { return next(error) } @@ -378,7 +386,7 @@ function flushAllProjects(req, res, next) { concurrency: req.query.concurrency || 5, dryRun: req.query.dryRun || false } - ProjectFlusher.flushAllProjects(options, function (err, projectIds) { + ProjectFlusher.flushAllProjects(options, (err, projectIds) => { if (err) { logger.err({ err }, 'error bulk flushing projects') res.sendStatus(500) @@ -395,10 +403,7 @@ function flushQueuedProjects(req, res, next) { timeout: 5 * 60 * 1000, min_delete_age: req.query.min_delete_age || 5 * 60 * 1000 } - DeleteQueueManager.flushAndDeleteOldProjects(options, function ( - err, - flushed - ) { + DeleteQueueManager.flushAndDeleteOldProjects(options, (err, flushed) => { if (err) { logger.err({ err }, 'error flushing old projects') res.sendStatus(500) From a2a1914a53342ba20b4ca0ef2c75a27b5493242d Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:15:37 -0400 Subject: [PATCH 19/23] Use max_doc_length setting to limit incoming doc size --- services/document-updater/app/js/HttpController.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 2e8bd084a5..646a8578df 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -3,13 +3,12 @@ const HistoryManager = require('./HistoryManager') const ProjectManager = require('./ProjectManager') const Errors = require('./Errors') const logger = require('logger-sharelatex') +const Settings = require('settings-sharelatex') const Metrics = require('./Metrics') const ProjectFlusher = require('./ProjectFlusher') const DeleteQueueManager = require('./DeleteQueueManager') const async = require('async') -const TWO_MEGABYTES = 2 * 1024 * 1024 - module.exports = { getDoc, getProjectDocsAndFlushIfOld, @@ -134,7 +133,7 @@ function setDoc(req, res, next) { const projectId = req.params.project_id const { lines, source, user_id: userId, undoing } = req.body const lineSize = _getTotalSizeOfLines(lines) - if (lineSize > TWO_MEGABYTES) { + if (lineSize > Settings.max_doc_length) { logger.log( { projectId, docId, source, lineSize, userId }, 'document too large, returning 406 response' From 526ef25fcf5ef8069b40d66178539978846ae7de Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:20:55 -0400 Subject: [PATCH 20/23] Decaf cleanup: unnecessary returns --- .../js/HttpController/HttpControllerTests.js | 341 +++++++++--------- 1 file changed, 162 insertions(+), 179 deletions(-) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 8f4125fcfa..18c74691bc 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -6,7 +6,6 @@ // Fix any style issues and re-enable lint. /* * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns * DS206: Consider reworking classes to avoid initClass * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ @@ -46,11 +45,11 @@ describe('HttpController', function () { this.project_id = 'project-id-123' this.doc_id = 'doc-id-123' this.next = sinon.stub() - return (this.res = { + this.res = { send: sinon.stub(), sendStatus: sinon.stub(), json: sinon.stub() - }) + } }) describe('getDoc', function () { @@ -61,12 +60,12 @@ describe('HttpController', function () { this.fromVersion = 42 this.ranges = { changes: 'mock', comments: 'mock' } this.pathname = '/a/b/c' - return (this.req = { + this.req = { params: { project_id: this.project_id, doc_id: this.doc_id } - }) + } }) describe('when the document exists and no recent ops are requested', function () { @@ -82,17 +81,17 @@ describe('HttpController', function () { this.ranges, this.pathname ) - return this.HttpController.getDoc(this.req, this.res, this.next) + this.HttpController.getDoc(this.req, this.res, this.next) }) it('should get the doc', function () { - return this.DocumentManager.getDocAndRecentOpsWithLock + this.DocumentManager.getDocAndRecentOpsWithLock .calledWith(this.project_id, this.doc_id, -1) .should.equal(true) }) it('should return the doc as JSON', function () { - return this.res.json + this.res.json .calledWith({ id: this.doc_id, lines: this.lines, @@ -105,7 +104,7 @@ describe('HttpController', function () { }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { doc_id: this.doc_id, project_id: this.project_id }, 'getting doc via http' @@ -113,8 +112,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -132,17 +131,17 @@ describe('HttpController', function () { this.pathname ) this.req.query = { fromVersion: `${this.fromVersion}` } - return this.HttpController.getDoc(this.req, this.res, this.next) + this.HttpController.getDoc(this.req, this.res, this.next) }) it('should get the doc', function () { - return this.DocumentManager.getDocAndRecentOpsWithLock + this.DocumentManager.getDocAndRecentOpsWithLock .calledWith(this.project_id, this.doc_id, this.fromVersion) .should.equal(true) }) it('should return the doc as JSON', function () { - return this.res.json + this.res.json .calledWith({ id: this.doc_id, lines: this.lines, @@ -155,7 +154,7 @@ describe('HttpController', function () { }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { doc_id: this.doc_id, project_id: this.project_id }, 'getting doc via http' @@ -163,8 +162,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -173,26 +172,26 @@ describe('HttpController', function () { this.DocumentManager.getDocAndRecentOpsWithLock = sinon .stub() .callsArgWith(3, null, null, null) - return this.HttpController.getDoc(this.req, this.res, this.next) + this.HttpController.getDoc(this.req, this.res, this.next) }) - return it('should call next with NotFoundError', function () { - return this.next + it('should call next with NotFoundError', function () { + this.next .calledWith(new Errors.NotFoundError('not found')) .should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.DocumentManager.getDocAndRecentOpsWithLock = sinon .stub() .callsArgWith(3, new Error('oops'), null, null) - return this.HttpController.getDoc(this.req, this.res, this.next) + this.HttpController.getDoc(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) @@ -202,7 +201,7 @@ describe('HttpController', function () { this.lines = ['one', 'two', 'three'] this.source = 'dropbox' this.user_id = 'user-id-123' - return (this.req = { + this.req = { headers: {}, params: { project_id: this.project_id, @@ -214,17 +213,17 @@ describe('HttpController', function () { user_id: this.user_id, undoing: (this.undoing = true) } - }) + } }) describe('successfully', function () { beforeEach(function () { this.DocumentManager.setDocWithLock = sinon.stub().callsArgWith(6) - return this.HttpController.setDoc(this.req, this.res, this.next) + this.HttpController.setDoc(this.req, this.res, this.next) }) it('should set the doc', function () { - return this.DocumentManager.setDocWithLock + this.DocumentManager.setDocWithLock .calledWith( this.project_id, this.doc_id, @@ -237,11 +236,11 @@ describe('HttpController', function () { }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { doc_id: this.doc_id, @@ -256,8 +255,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -266,15 +265,15 @@ describe('HttpController', function () { this.DocumentManager.setDocWithLock = sinon .stub() .callsArgWith(6, new Error('oops')) - return this.HttpController.setDoc(this.req, this.res, this.next) + this.HttpController.setDoc(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) - return describe('when the payload is too large', function () { + describe('when the payload is too large', function () { beforeEach(function () { const lines = [] for (let _ = 0; _ <= 200000; _++) { @@ -282,46 +281,46 @@ describe('HttpController', function () { } this.req.body.lines = lines this.DocumentManager.setDocWithLock = sinon.stub().callsArgWith(6) - return this.HttpController.setDoc(this.req, this.res, this.next) + this.HttpController.setDoc(this.req, this.res, this.next) }) it('should send back a 406 response', function () { - return this.res.sendStatus.calledWith(406).should.equal(true) + this.res.sendStatus.calledWith(406).should.equal(true) }) - return it('should not call setDocWithLock', function () { - return this.DocumentManager.setDocWithLock.callCount.should.equal(0) + it('should not call setDocWithLock', function () { + this.DocumentManager.setDocWithLock.callCount.should.equal(0) }) }) }) describe('flushProject', function () { beforeEach(function () { - return (this.req = { + this.req = { params: { project_id: this.project_id } - }) + } }) describe('successfully', function () { beforeEach(function () { this.ProjectManager.flushProjectWithLocks = sinon.stub().callsArgWith(1) - return this.HttpController.flushProject(this.req, this.res, this.next) + this.HttpController.flushProject(this.req, this.res, this.next) }) it('should flush the project', function () { - return this.ProjectManager.flushProjectWithLocks + this.ProjectManager.flushProjectWithLocks .calledWith(this.project_id) .should.equal(true) }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { project_id: this.project_id }, 'flushing project via http' @@ -329,21 +328,21 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.ProjectManager.flushProjectWithLocks = sinon .stub() .callsArgWith(1, new Error('oops')) - return this.HttpController.flushProject(this.req, this.res, this.next) + this.HttpController.flushProject(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) @@ -352,12 +351,12 @@ describe('HttpController', function () { beforeEach(function () { this.lines = ['one', 'two', 'three'] this.version = 42 - return (this.req = { + this.req = { params: { project_id: this.project_id, doc_id: this.doc_id } - }) + } }) describe('successfully', function () { @@ -365,25 +364,21 @@ describe('HttpController', function () { this.DocumentManager.flushDocIfLoadedWithLock = sinon .stub() .callsArgWith(2) - return this.HttpController.flushDocIfLoaded( - this.req, - this.res, - this.next - ) + this.HttpController.flushDocIfLoaded(this.req, this.res, this.next) }) it('should flush the doc', function () { - return this.DocumentManager.flushDocIfLoadedWithLock + this.DocumentManager.flushDocIfLoadedWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { doc_id: this.doc_id, project_id: this.project_id }, 'flushing doc via http' @@ -391,38 +386,34 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.DocumentManager.flushDocIfLoadedWithLock = sinon .stub() .callsArgWith(2, new Error('oops')) - return this.HttpController.flushDocIfLoaded( - this.req, - this.res, - this.next - ) + this.HttpController.flushDocIfLoaded(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) describe('deleteDoc', function () { beforeEach(function () { - return (this.req = { + this.req = { params: { project_id: this.project_id, doc_id: this.doc_id }, query: {} - }) + } }) describe('successfully', function () { @@ -430,11 +421,11 @@ describe('HttpController', function () { this.DocumentManager.flushAndDeleteDocWithLock = sinon .stub() .callsArgWith(3) - return this.HttpController.deleteDoc(this.req, this.res, this.next) + this.HttpController.deleteDoc(this.req, this.res, this.next) }) it('should flush and delete the doc', function () { - return this.DocumentManager.flushAndDeleteDocWithLock + this.DocumentManager.flushAndDeleteDocWithLock .calledWith(this.project_id, this.doc_id, { ignoreFlushErrors: false }) @@ -442,17 +433,17 @@ describe('HttpController', function () { }) it('should flush project history', function () { - return this.HistoryManager.flushProjectChangesAsync + this.HistoryManager.flushProjectChangesAsync .calledWithExactly(this.project_id) .should.equal(true) }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { doc_id: this.doc_id, project_id: this.project_id }, 'deleting doc via http' @@ -460,8 +451,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -469,47 +460,47 @@ describe('HttpController', function () { beforeEach(function () { this.req.query.ignore_flush_errors = 'true' this.DocumentManager.flushAndDeleteDocWithLock = sinon.stub().yields() - return this.HttpController.deleteDoc(this.req, this.res, this.next) + this.HttpController.deleteDoc(this.req, this.res, this.next) }) it('should delete the doc', function () { - return this.DocumentManager.flushAndDeleteDocWithLock + this.DocumentManager.flushAndDeleteDocWithLock .calledWith(this.project_id, this.doc_id, { ignoreFlushErrors: true }) .should.equal(true) }) - return it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + it('should return a successful No Content response', function () { + this.res.sendStatus.calledWith(204).should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.DocumentManager.flushAndDeleteDocWithLock = sinon .stub() .callsArgWith(3, new Error('oops')) - return this.HttpController.deleteDoc(this.req, this.res, this.next) + this.HttpController.deleteDoc(this.req, this.res, this.next) }) it('should flush project history', function () { - return this.HistoryManager.flushProjectChangesAsync + this.HistoryManager.flushProjectChangesAsync .calledWithExactly(this.project_id) .should.equal(true) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) describe('deleteProject', function () { beforeEach(function () { - return (this.req = { + this.req = { params: { project_id: this.project_id } - }) + } }) describe('successfully', function () { @@ -517,21 +508,21 @@ describe('HttpController', function () { this.ProjectManager.flushAndDeleteProjectWithLocks = sinon .stub() .callsArgWith(2) - return this.HttpController.deleteProject(this.req, this.res, this.next) + this.HttpController.deleteProject(this.req, this.res, this.next) }) it('should delete the project', function () { - return this.ProjectManager.flushAndDeleteProjectWithLocks + this.ProjectManager.flushAndDeleteProjectWithLocks .calledWith(this.project_id) .should.equal(true) }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { project_id: this.project_id }, 'deleting project via http' @@ -539,8 +530,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -550,39 +541,39 @@ describe('HttpController', function () { .stub() .callsArgWith(1) this.req.query = { background: true, shutdown: true } - return this.HttpController.deleteProject(this.req, this.res, this.next) + this.HttpController.deleteProject(this.req, this.res, this.next) }) - return it('should queue the flush and delete', function () { - return this.ProjectManager.queueFlushAndDeleteProject + it('should queue the flush and delete', function () { + this.ProjectManager.queueFlushAndDeleteProject .calledWith(this.project_id) .should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.ProjectManager.flushAndDeleteProjectWithLocks = sinon .stub() .callsArgWith(2, new Error('oops')) - return this.HttpController.deleteProject(this.req, this.res, this.next) + this.HttpController.deleteProject(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) describe('acceptChanges', function () { beforeEach(function () { - return (this.req = { + this.req = { params: { project_id: this.project_id, doc_id: this.doc_id, change_id: (this.change_id = 'mock-change-od-1') } - }) + } }) describe('successfully with a single change', function () { @@ -590,21 +581,21 @@ describe('HttpController', function () { this.DocumentManager.acceptChangesWithLock = sinon .stub() .callsArgWith(3) - return this.HttpController.acceptChanges(this.req, this.res, this.next) + this.HttpController.acceptChanges(this.req, this.res, this.next) }) it('should accept the change', function () { - return this.DocumentManager.acceptChangesWithLock + this.DocumentManager.acceptChangesWithLock .calledWith(this.project_id, this.doc_id, [this.change_id]) .should.equal(true) }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { project_id: this.project_id, doc_id: this.doc_id }, 'accepting 1 changes via http' @@ -612,8 +603,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -629,17 +620,17 @@ describe('HttpController', function () { this.DocumentManager.acceptChangesWithLock = sinon .stub() .callsArgWith(3) - return this.HttpController.acceptChanges(this.req, this.res, this.next) + this.HttpController.acceptChanges(this.req, this.res, this.next) }) it('should accept the changes in the body payload', function () { - return this.DocumentManager.acceptChangesWithLock + this.DocumentManager.acceptChangesWithLock .calledWith(this.project_id, this.doc_id, this.change_ids) .should.equal(true) }) - return it('should log the request with the correct number of changes', function () { - return this.logger.log + it('should log the request with the correct number of changes', function () { + this.logger.log .calledWith( { project_id: this.project_id, doc_id: this.doc_id }, `accepting ${this.change_ids.length} changes via http` @@ -648,29 +639,29 @@ describe('HttpController', function () { }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.DocumentManager.acceptChangesWithLock = sinon .stub() .callsArgWith(3, new Error('oops')) - return this.HttpController.acceptChanges(this.req, this.res, this.next) + this.HttpController.acceptChanges(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) describe('deleteComment', function () { beforeEach(function () { - return (this.req = { + this.req = { params: { project_id: this.project_id, doc_id: this.doc_id, comment_id: (this.comment_id = 'mock-comment-id') } - }) + } }) describe('successfully', function () { @@ -678,21 +669,21 @@ describe('HttpController', function () { this.DocumentManager.deleteCommentWithLock = sinon .stub() .callsArgWith(3) - return this.HttpController.deleteComment(this.req, this.res, this.next) + this.HttpController.deleteComment(this.req, this.res, this.next) }) it('should accept the change', function () { - return this.DocumentManager.deleteCommentWithLock + this.DocumentManager.deleteCommentWithLock .calledWith(this.project_id, this.doc_id, this.comment_id) .should.equal(true) }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { project_id: this.project_id, @@ -704,21 +695,21 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.DocumentManager.deleteCommentWithLock = sinon .stub() .callsArgWith(3, new Error('oops')) - return this.HttpController.deleteComment(this.req, this.res, this.next) + this.HttpController.deleteComment(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) @@ -730,14 +721,14 @@ describe('HttpController', function () { { _id: '1234', lines: 'hello', v: 23 }, { _id: '4567', lines: 'world', v: 45 } ] - return (this.req = { + this.req = { params: { project_id: this.project_id }, query: { state: this.state } - }) + } }) describe('successfully', function () { @@ -745,7 +736,7 @@ describe('HttpController', function () { this.ProjectManager.getProjectDocsAndFlushIfOld = sinon .stub() .callsArgWith(3, null, this.docs) - return this.HttpController.getProjectDocsAndFlushIfOld( + this.HttpController.getProjectDocsAndFlushIfOld( this.req, this.res, this.next @@ -753,17 +744,17 @@ describe('HttpController', function () { }) it('should get docs from the project manager', function () { - return this.ProjectManager.getProjectDocsAndFlushIfOld + this.ProjectManager.getProjectDocsAndFlushIfOld .calledWith(this.project_id, this.state, {}) .should.equal(true) }) it('should return a successful response', function () { - return this.res.send.calledWith(this.docs).should.equal(true) + this.res.send.calledWith(this.docs).should.equal(true) }) it('should log the request', function () { - return this.logger.log + this.logger.log .calledWith( { project_id: this.project_id, exclude: [] }, 'getting docs via http' @@ -772,7 +763,7 @@ describe('HttpController', function () { }) it('should log the response', function () { - return this.logger.log + this.logger.log .calledWith( { project_id: this.project_id, result: ['1234:23', '4567:45'] }, 'got docs via http' @@ -780,8 +771,8 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) @@ -793,32 +784,32 @@ describe('HttpController', function () { 3, new Errors.ProjectStateChangedError('project state changed') ) - return this.HttpController.getProjectDocsAndFlushIfOld( + this.HttpController.getProjectDocsAndFlushIfOld( this.req, this.res, this.next ) }) - return it('should return an HTTP 409 Conflict response', function () { - return this.res.sendStatus.calledWith(409).should.equal(true) + it('should return an HTTP 409 Conflict response', function () { + this.res.sendStatus.calledWith(409).should.equal(true) }) }) - return describe('when an error occurs', function () { + describe('when an error occurs', function () { beforeEach(function () { this.ProjectManager.getProjectDocsAndFlushIfOld = sinon .stub() .callsArgWith(3, new Error('oops')) - return this.HttpController.getProjectDocsAndFlushIfOld( + this.HttpController.getProjectDocsAndFlushIfOld( this.req, this.res, this.next ) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) @@ -830,7 +821,7 @@ describe('HttpController', function () { this.docUpdates = sinon.stub() this.fileUpdates = sinon.stub() this.version = 1234567 - return (this.req = { + this.req = { body: { projectHistoryId: this.projectHistoryId, userId: this.userId, @@ -841,7 +832,7 @@ describe('HttpController', function () { params: { project_id: this.project_id } - }) + } }) describe('successfully', function () { @@ -849,11 +840,11 @@ describe('HttpController', function () { this.ProjectManager.updateProjectWithLocks = sinon .stub() .callsArgWith(6) - return this.HttpController.updateProject(this.req, this.res, this.next) + this.HttpController.updateProject(this.req, this.res, this.next) }) it('should accept the change', function () { - return this.ProjectManager.updateProjectWithLocks + this.ProjectManager.updateProjectWithLocks .calledWith( this.project_id, this.projectHistoryId, @@ -866,35 +857,35 @@ describe('HttpController', function () { }) it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + this.res.sendStatus.calledWith(204).should.equal(true) }) - return it('should time the request', function () { - return this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should time the request', function () { + this.Metrics.Timer.prototype.done.called.should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.ProjectManager.updateProjectWithLocks = sinon .stub() .callsArgWith(6, new Error('oops')) - return this.HttpController.updateProject(this.req, this.res, this.next) + this.HttpController.updateProject(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) - return describe('resyncProjectHistory', function () { + describe('resyncProjectHistory', function () { beforeEach(function () { this.projectHistoryId = 'history-id-123' this.docs = sinon.stub() this.files = sinon.stub() this.fileUpdates = sinon.stub() - return (this.req = { + this.req = { body: { projectHistoryId: this.projectHistoryId, docs: this.docs, @@ -903,21 +894,17 @@ describe('HttpController', function () { params: { project_id: this.project_id } - }) + } }) describe('successfully', function () { beforeEach(function () { this.HistoryManager.resyncProjectHistory = sinon.stub().callsArgWith(4) - return this.HttpController.resyncProjectHistory( - this.req, - this.res, - this.next - ) + this.HttpController.resyncProjectHistory(this.req, this.res, this.next) }) it('should accept the change', function () { - return this.HistoryManager.resyncProjectHistory + this.HistoryManager.resyncProjectHistory .calledWith( this.project_id, this.projectHistoryId, @@ -927,25 +914,21 @@ describe('HttpController', function () { .should.equal(true) }) - return it('should return a successful No Content response', function () { - return this.res.sendStatus.calledWith(204).should.equal(true) + it('should return a successful No Content response', function () { + this.res.sendStatus.calledWith(204).should.equal(true) }) }) - return describe('when an errors occurs', function () { + describe('when an errors occurs', function () { beforeEach(function () { this.HistoryManager.resyncProjectHistory = sinon .stub() .callsArgWith(4, new Error('oops')) - return this.HttpController.resyncProjectHistory( - this.req, - this.res, - this.next - ) + this.HttpController.resyncProjectHistory(this.req, this.res, this.next) }) - return it('should call next with the error', function () { - return this.next.calledWith(new Error('oops')).should.equal(true) + it('should call next with the error', function () { + this.next.calledWith(new Error('oops')).should.equal(true) }) }) }) From 3acb970442b1ee714869b6d973b9dc6caf653410 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:24:07 -0400 Subject: [PATCH 21/23] Decaf cleanup: simplify stubbed class --- .../js/HttpController/HttpControllerTests.js | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 18c74691bc..c913a107cd 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -2,13 +2,6 @@ no-return-assign, no-unused-vars, */ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS206: Consider reworking classes to avoid initClass - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const chai = require('chai') const should = chai.should() @@ -33,15 +26,9 @@ describe('HttpController', function () { './Errors': Errors } }) - this.Metrics.Timer = Timer = (function () { - Timer = class Timer { - static initClass() { - this.prototype.done = sinon.stub() - } - } - Timer.initClass() - return Timer - })() + this.Metrics.Timer = class Timer {} + Timer.prototype.done = sinon.stub() + this.project_id = 'project-id-123' this.doc_id = 'doc-id-123' this.next = sinon.stub() From 0b1c7e90af0feec57edd00f7d066ebefde06cec5 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:26:05 -0400 Subject: [PATCH 22/23] Decaf cleanup: remove unused variables --- .../test/unit/js/HttpController/HttpControllerTests.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index c913a107cd..091addc78a 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -1,17 +1,10 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ const sinon = require('sinon') -const chai = require('chai') -const should = chai.should() const modulePath = '../../../../app/js/HttpController.js' const SandboxedModule = require('sandboxed-module') const Errors = require('../../../../app/js/Errors.js') describe('HttpController', function () { beforeEach(function () { - let Timer this.HttpController = SandboxedModule.require(modulePath, { requires: { './DocumentManager': (this.DocumentManager = {}), @@ -27,7 +20,7 @@ describe('HttpController', function () { } }) this.Metrics.Timer = class Timer {} - Timer.prototype.done = sinon.stub() + this.Metrics.Timer.prototype.done = sinon.stub() this.project_id = 'project-id-123' this.doc_id = 'doc-id-123' From 16c0ed23db1fd88736d15bfd6ad22658f8a7e650 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 11 May 2020 11:37:59 -0400 Subject: [PATCH 23/23] Fix tests after decaf cleanup * Camel casing in logs * The Express request object always has query and body properties --- .../js/HttpController/HttpControllerTests.js | 65 ++++++++++++------- 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 091addc78a..64751e55db 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -44,7 +44,9 @@ describe('HttpController', function () { params: { project_id: this.project_id, doc_id: this.doc_id - } + }, + query: {}, + body: {} } }) @@ -86,7 +88,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { doc_id: this.doc_id, project_id: this.project_id }, + { docId: this.doc_id, projectId: this.project_id }, 'getting doc via http' ) .should.equal(true) @@ -136,7 +138,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { doc_id: this.doc_id, project_id: this.project_id }, + { docId: this.doc_id, projectId: this.project_id }, 'getting doc via http' ) .should.equal(true) @@ -187,6 +189,7 @@ describe('HttpController', function () { project_id: this.project_id, doc_id: this.doc_id }, + query: {}, body: { lines: this.lines, source: this.source, @@ -223,11 +226,11 @@ describe('HttpController', function () { this.logger.log .calledWith( { - doc_id: this.doc_id, - project_id: this.project_id, + docId: this.doc_id, + projectId: this.project_id, lines: this.lines, source: this.source, - user_id: this.user_id, + userId: this.user_id, undoing: this.undoing }, 'setting doc via http' @@ -279,7 +282,9 @@ describe('HttpController', function () { this.req = { params: { project_id: this.project_id - } + }, + query: {}, + body: {} } }) @@ -302,7 +307,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { project_id: this.project_id }, + { projectId: this.project_id }, 'flushing project via http' ) .should.equal(true) @@ -335,7 +340,9 @@ describe('HttpController', function () { params: { project_id: this.project_id, doc_id: this.doc_id - } + }, + query: {}, + body: {} } }) @@ -360,7 +367,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { doc_id: this.doc_id, project_id: this.project_id }, + { docId: this.doc_id, projectId: this.project_id }, 'flushing doc via http' ) .should.equal(true) @@ -392,7 +399,8 @@ describe('HttpController', function () { project_id: this.project_id, doc_id: this.doc_id }, - query: {} + query: {}, + body: {} } }) @@ -425,7 +433,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { doc_id: this.doc_id, project_id: this.project_id }, + { docId: this.doc_id, projectId: this.project_id }, 'deleting doc via http' ) .should.equal(true) @@ -479,7 +487,9 @@ describe('HttpController', function () { this.req = { params: { project_id: this.project_id - } + }, + query: {}, + body: {} } }) @@ -504,7 +514,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { project_id: this.project_id }, + { projectId: this.project_id }, 'deleting project via http' ) .should.equal(true) @@ -552,7 +562,9 @@ describe('HttpController', function () { project_id: this.project_id, doc_id: this.doc_id, change_id: (this.change_id = 'mock-change-od-1') - } + }, + query: {}, + body: {} } }) @@ -577,7 +589,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { project_id: this.project_id, doc_id: this.doc_id }, + { projectId: this.project_id, docId: this.doc_id }, 'accepting 1 changes via http' ) .should.equal(true) @@ -612,7 +624,7 @@ describe('HttpController', function () { it('should log the request with the correct number of changes', function () { this.logger.log .calledWith( - { project_id: this.project_id, doc_id: this.doc_id }, + { projectId: this.project_id, docId: this.doc_id }, `accepting ${this.change_ids.length} changes via http` ) .should.equal(true) @@ -640,7 +652,9 @@ describe('HttpController', function () { project_id: this.project_id, doc_id: this.doc_id, comment_id: (this.comment_id = 'mock-comment-id') - } + }, + query: {}, + body: {} } }) @@ -666,9 +680,9 @@ describe('HttpController', function () { this.logger.log .calledWith( { - project_id: this.project_id, - doc_id: this.doc_id, - comment_id: this.comment_id + projectId: this.project_id, + docId: this.doc_id, + commentId: this.comment_id }, 'deleting comment via http' ) @@ -707,7 +721,8 @@ describe('HttpController', function () { }, query: { state: this.state - } + }, + body: {} } }) @@ -736,7 +751,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.log .calledWith( - { project_id: this.project_id, exclude: [] }, + { projectId: this.project_id, exclude: [] }, 'getting docs via http' ) .should.equal(true) @@ -745,7 +760,7 @@ describe('HttpController', function () { it('should log the response', function () { this.logger.log .calledWith( - { project_id: this.project_id, result: ['1234:23', '4567:45'] }, + { projectId: this.project_id, result: ['1234:23', '4567:45'] }, 'got docs via http' ) .should.equal(true) @@ -802,6 +817,7 @@ describe('HttpController', function () { this.fileUpdates = sinon.stub() this.version = 1234567 this.req = { + query: {}, body: { projectHistoryId: this.projectHistoryId, userId: this.userId, @@ -866,6 +882,7 @@ describe('HttpController', function () { this.files = sinon.stub() this.fileUpdates = sinon.stub() this.req = { + query: {}, body: { projectHistoryId: this.projectHistoryId, docs: this.docs,