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

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

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

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 () {
+      it('should return a generic connection error', function () {
-        return this.callback.calledWith(this.error).should.equal(true)
+        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 () {
+      it('should return a generic connection error', function () {
-        return this.callback.calledWith(this.error).should.equal(true)
+        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 () {