From 23738540ed745c5c2632b535b05a8191abf9df1f Mon Sep 17 00:00:00 2001 From: Thomas Date: Wed, 24 Feb 2021 15:09:19 +0100 Subject: [PATCH] Fix API request errors which could contain API hostname or address (#160) Wrap errors produced by failing requests to web API, and remove the url/hostname from thrown error messages. (But keep the URL path for info.) --- .../app/js/PersistenceManager.js | 32 +++++++---- .../js/ApplyingUpdatesToADocTests.js | 56 ++++++++++++++++++- .../PersistenceManagerTests.js | 23 ++++++-- 3 files changed, 95 insertions(+), 16 deletions(-) diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index a9f384afa1..fca23a7c47 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -69,10 +69,10 @@ module.exports = PersistenceManager = { return _callback(...Array.from(args || [])) } - const url = `${Settings.apis.web.url}/project/${project_id}/doc/${doc_id}` + const urlPath = `/project/${project_id}/doc/${doc_id}` return request( { - url, + url: `${Settings.apis.web.url}${urlPath}`, method: 'GET', headers: { accept: 'application/json' @@ -88,7 +88,11 @@ module.exports = PersistenceManager = { function (error, res, body) { updateMetric('getDoc', error, res) if (error != null) { - return callback(error) + logger.error( + { err: error, project_id, doc_id }, + 'web API request failed' + ) + return callback(new Error('error connecting to web API')) } if (res.statusCode >= 200 && res.statusCode < 300) { try { @@ -119,10 +123,12 @@ module.exports = PersistenceManager = { body.projectHistoryType ) } else if (res.statusCode === 404) { - return callback(new Errors.NotFoundError(`doc not not found: ${url}`)) + return callback( + new Errors.NotFoundError(`doc not not found: ${urlPath}`) + ) } else { return callback( - new Error(`error accessing web API: ${url} ${res.statusCode}`) + new Error(`error accessing web API: ${urlPath} ${res.statusCode}`) ) } } @@ -148,10 +154,10 @@ module.exports = PersistenceManager = { return _callback(...Array.from(args || [])) } - const url = `${Settings.apis.web.url}/project/${project_id}/doc/${doc_id}` + const urlPath = `/project/${project_id}/doc/${doc_id}` return request( { - url, + url: `${Settings.apis.web.url}${urlPath}`, method: 'POST', json: { lines, @@ -171,15 +177,21 @@ module.exports = PersistenceManager = { function (error, res, body) { updateMetric('setDoc', error, res) if (error != null) { - return callback(error) + logger.error( + { err: error, project_id, doc_id }, + 'web API request failed' + ) + return callback(new Error('error connecting to web API')) } if (res.statusCode >= 200 && res.statusCode < 300) { return callback(null) } else if (res.statusCode === 404) { - return callback(new Errors.NotFoundError(`doc not not found: ${url}`)) + return callback( + new Errors.NotFoundError(`doc not not found: ${urlPath}`) + ) } else { return callback( - new Error(`error accessing web API: ${url} ${res.statusCode}`) + new Error(`error accessing web API: ${urlPath} ${res.statusCode}`) ) } } diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js index 918baba141..f460a2d3ff 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js @@ -703,7 +703,7 @@ describe('Applying updates to a doc', function () { }) }) - return describe('when the sending duplicate ops', function () { + describe('when the sending duplicate ops', function () { before(function (done) { ;[this.project_id, this.doc_id] = Array.from([ DocUpdaterClient.randomId(), @@ -792,4 +792,58 @@ describe('Applying updates to a doc', function () { ).to.equal(true) }) }) + + return describe('when sending updates for a non-existing doc id', function () { + before(function (done) { + ;[this.project_id, this.doc_id] = Array.from([ + DocUpdaterClient.randomId(), + DocUpdaterClient.randomId() + ]) + this.non_existing = { + doc_id: this.doc_id, + v: this.version, + op: [{ d: 'content', p: 0 }] + } + + DocUpdaterClient.subscribeToAppliedOps( + (this.messageCallback = sinon.stub()) + ) + + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + this.non_existing, + (error) => { + if (error != null) { + throw error + } + return setTimeout(done, 200) + } + ) + return null + }) + + it('should not update or create a doc', function (done) { + DocUpdaterClient.getDoc( + this.project_id, + this.doc_id, + (error, res, doc) => { + res.statusCode.should.equal(404) + return done() + } + ) + return null + }) + + return it('should send a message with an error', function () { + this.messageCallback.called.should.equal(true) + const [channel, message] = Array.from(this.messageCallback.args[0]) + channel.should.equal('applied-ops') + return JSON.parse(message).should.deep.include({ + project_id: this.project_id, + doc_id: this.doc_id, + error: `doc not not found: /project/${this.project_id}/doc/${this.doc_id}` + }) + }) + }) }) diff --git a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js index 645ee8a59b..1013752dee 100644 --- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js +++ b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js @@ -40,7 +40,8 @@ describe('PersistenceManager', function () { }), 'logger-sharelatex': (this.logger = { log: sinon.stub(), - err: sinon.stub() + err: sinon.stub(), + error: sinon.stub() }), './Errors': Errors } @@ -145,8 +146,14 @@ describe('PersistenceManager', function () { ) }) - it('should return the error', function () { - return this.callback.calledWith(this.error).should.equal(true) + it('should return a generic connection error', function () { + return this.callback + .calledWith( + sinon.match + .instanceOf(Error) + .and(sinon.match.has('message', 'error connecting to web API')) + ) + .should.equal(true) }) it('should time the execution', function () { @@ -355,8 +362,14 @@ describe('PersistenceManager', function () { ) }) - it('should return the error', function () { - return this.callback.calledWith(this.error).should.equal(true) + it('should return a generic connection error', function () { + return this.callback + .calledWith( + sinon.match + .instanceOf(Error) + .and(sinon.match.has('message', 'error connecting to web API')) + ) + .should.equal(true) }) it('should time the execution', function () {