From 8a825bb065e0f4ed21afb6ffa3c66c78e9cf468c Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Thu, 27 Jun 2019 12:10:28 +0200 Subject: [PATCH] Merge pull request #1902 from overleaf/msm-return-429-tdps-update-too-many-connections Project on cooldown returns 429 on TDPS update GitOrigin-RevId: b8da35a61be7508bc56d7798d233c186f694c364 --- .../ThirdPartyDataStore/TpdsController.js | 18 ++-- .../TpdsControllerTests.js | 83 +++++++++++++++---- 2 files changed, 78 insertions(+), 23 deletions(-) diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js index effafd4e81..1ed89b29df 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js @@ -42,11 +42,19 @@ module.exports = { 'sending response that tpdsUpdate has been completed' ) if (err != null) { - logger.err( - { err, user_id, filePath }, - 'error reciving update from tpds' - ) - return res.sendStatus(500) + if (err.name === 'TooManyRequestsError') { + logger.warn( + { err, user_id, filePath }, + 'tpds update failed to be processed, too many requests' + ) + return res.sendStatus(429) + } else { + logger.err( + { err, user_id, filePath }, + 'error reciving update from tpds' + ) + return res.sendStatus(500) + } } else { logger.log( { user_id, filePath, projectName }, diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js index 28b7f2e615..42422b47a0 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js @@ -12,6 +12,7 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') require('chai').should() +const Errors = require('../../../../app/src/Features/Errors/Errors') const modulePath = require('path').join( __dirname, '../../../../app/src/Features/ThirdPartyDataStore/TpdsController.js' @@ -26,6 +27,7 @@ describe('TpdsController', function() { './UpdateMerger': (this.UpdateMerger = {}), 'logger-sharelatex': { log() {}, + warn() {}, err() {} }, 'metrics-sharelatex': { @@ -34,11 +36,11 @@ describe('TpdsController', function() { } }) - return (this.user_id = 'dsad29jlkjas') + this.user_id = 'dsad29jlkjas' }) - describe('getting an update', () => - it('should process the update with the update reciver', function(done) { + describe('getting an update', () => { + it('should process the update with the update receiver', function(done) { const path = '/projectName/here.txt' const req = { pause() {}, @@ -62,11 +64,56 @@ describe('TpdsController', function() { this.source ) .should.equal(true) - return done() + done() } } - return this.TpdsController.mergeUpdate(req, res) - })) + this.TpdsController.mergeUpdate(req, res) + }) + + it('should return a 500 error when the update receiver fails', function() { + const path = '/projectName/here.txt' + const req = { + pause() {}, + params: { 0: path, user_id: this.user_id }, + session: { + destroy() {} + }, + headers: { + 'x-sl-update-source': (this.source = 'dropbox') + } + } + this.TpdsUpdateHandler.newUpdate = sinon + .stub() + .callsArgWith(5, 'update-receiver-error') + const res = { + sendStatus: sinon.stub() + } + this.TpdsController.mergeUpdate(req, res) + res.sendStatus.calledWith(500).should.equal(true) + }) + + it('should return a 429 error when the update receiver fails due to too many requests error', function() { + const path = '/projectName/here.txt' + const req = { + pause() {}, + params: { 0: path, user_id: this.user_id }, + session: { + destroy() {} + }, + headers: { + 'x-sl-update-source': (this.source = 'dropbox') + } + } + this.TpdsUpdateHandler.newUpdate = sinon + .stub() + .callsArgWith(5, new Errors.TooManyRequestsError('project on cooldown')) + const res = { + sendStatus: sinon.stub() + } + this.TpdsController.mergeUpdate(req, res) + res.sendStatus.calledWith(429).should.equal(true) + }) + }) describe('getting a delete update', () => it('should process the delete with the update reciver', function(done) { @@ -86,10 +133,10 @@ describe('TpdsController', function() { this.TpdsUpdateHandler.deleteUpdate .calledWith(this.user_id, 'projectName', '/here.txt', this.source) .should.equal(true) - return done() + done() } } - return this.TpdsController.deleteUpdate(req, res) + this.TpdsController.deleteUpdate(req, res) })) describe('parseParams', function() { @@ -99,16 +146,16 @@ describe('TpdsController', function() { const result = this.TpdsController.parseParams(req) result.user_id.should.equal(this.user_id) result.filePath.should.equal('/') - return result.projectName.should.equal(path) + result.projectName.should.equal(path) }) - it('should take the project name off the start and it with no slashes in', function() { + it('should take the project name off the start and return it with no slashes in', function() { const path = '/project/file.tex' const req = { params: { 0: path, user_id: this.user_id } } const result = this.TpdsController.parseParams(req) result.user_id.should.equal(this.user_id) result.filePath.should.equal('/file.tex') - return result.projectName.should.equal('project') + result.projectName.should.equal('project') }) it('should take the project name of and return a slash for the file path', function() { @@ -116,7 +163,7 @@ describe('TpdsController', function() { const req = { params: { 0: path, user_id: this.user_id } } const result = this.TpdsController.parseParams(req) result.projectName.should.equal('project_name') - return result.filePath.should.equal('/') + result.filePath.should.equal('/') }) }) @@ -137,11 +184,11 @@ describe('TpdsController', function() { } this.res = { sendStatus: sinon.stub() } - return this.TpdsController.updateProjectContents(this.req, this.res) + this.TpdsController.updateProjectContents(this.req, this.res) }) it('should merge the update', function() { - return this.UpdateMerger.mergeUpdate + this.UpdateMerger.mergeUpdate .calledWith( null, this.project_id, @@ -153,7 +200,7 @@ describe('TpdsController', function() { }) it('should return a success', function() { - return this.res.sendStatus.calledWith(200).should.equal(true) + this.res.sendStatus.calledWith(200).should.equal(true) }) }) @@ -174,17 +221,17 @@ describe('TpdsController', function() { } this.res = { sendStatus: sinon.stub() } - return this.TpdsController.deleteProjectContents(this.req, this.res) + this.TpdsController.deleteProjectContents(this.req, this.res) }) it('should delete the file', function() { - return this.UpdateMerger.deleteUpdate + this.UpdateMerger.deleteUpdate .calledWith(null, this.project_id, `/${this.path}`, this.source) .should.equal(true) }) it('should return a success', function() { - return this.res.sendStatus.calledWith(200).should.equal(true) + this.res.sendStatus.calledWith(200).should.equal(true) }) }) })