From 8bbf81f4bcf87f8f0eaaa1f521275255a0640224 Mon Sep 17 00:00:00 2001
From: Shane Kilkelly <shane@kilkelly.me>
Date: Tue, 1 Sep 2015 11:36:28 +0100
Subject: [PATCH] If the S3 service produces an error, handle it.

---
 .../app/coffee/S3PersistorManager.coffee      |  8 +++-
 .../coffee/S3PersistorManagerTests.coffee     | 45 +++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee
index 8d40113200..ede27b20be 100644
--- a/services/filestore/app/coffee/S3PersistorManager.coffee
+++ b/services/filestore/app/coffee/S3PersistorManager.coffee
@@ -9,6 +9,7 @@ fs = require("fs")
 knox = require("knox")
 path = require("path")
 LocalFileWriter = require("./LocalFileWriter")
+Errors = require("./Errors")
 _ = require("underscore")
 
 thirtySeconds = 30 * 1000
@@ -73,6 +74,12 @@ module.exports =
 		s3Stream = s3Client.get(key, headers)
 		s3Stream.end()
 		s3Stream.on 'response', (res) ->
+			if res.statusCode == 404
+				logger.log bucketName:bucketName, key:key, "file not found in s3"
+				return callback new Errors.NotFoundError("File not found in S3: #{bucketName}:#{key}"), null
+			if res.stausCode != 200
+				logger.log bucketName:bucketName, key:key, "error getting file from s3"
+				return callback new Error("Got non-200 response from S3: #{res.statusCode}"), null
 			callback null, res
 		s3Stream.on 'error', (err) ->
 			logger.err err:err, bucketName:bucketName, key:key, "error getting file stream from s3"
@@ -130,4 +137,3 @@ module.exports =
 			exists = res.statusCode == 200
 			logger.log bucketName:bucketName, key:key, exists:exists, "checked if file exsists in s3"
 			callback(err, exists)
-
diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
index ca324ae212..78fec6cea5 100644
--- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
+++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
@@ -35,6 +35,8 @@ describe "S3PersistorManagerTests", ->
 			"logger-sharelatex":
 				log:->
 				err:->
+			"./Errors": @Errors =
+				NotFoundError: sinon.stub()
 		@key = "my/key"
 		@bucketName = "my-bucket"
 		@error = "my errror"
@@ -67,8 +69,51 @@ describe "S3PersistorManagerTests", ->
 				@stubbedKnoxClient.get.calledWith(@key, {'Range': 'bytes=0-8'}).should.equal true
 				done()
 
+		describe "error conditions", ->
 
+			beforeEach ->
+				@fakeResponse =
+					statusCode: 500
+				@stubbedKnoxClient.get.returns(
+					on: (key, callback) =>
+						if key == 'response'
+							callback(@fakeResponse)
+					end: ->
+				)
 
+			describe "when the file doesn't exist", ->
+
+				beforeEach ->
+					@fakeResponse =
+						statusCode: 404
+
+				it "should produce a NotFoundError", (done) ->
+					@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err, stream)=> # empty callback
+						expect(stream).to.equal null
+						expect(err).to.not.equal null
+						expect(err instanceof @Errors.NotFoundError).to.equal true
+						done()
+
+				it "should have bucket and key in the Error message", (done) ->
+					@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err, stream)=> # empty callback
+						error_message = @Errors.NotFoundError.lastCall.args[0]
+						expect(error_message).to.not.equal null
+						error_message.should.match(new RegExp(".*#{@bucketName}.*"))
+						error_message.should.match(new RegExp(".*#{@key}.*"))
+						done()
+
+			describe "when the S3 service produces an error", ->
+				beforeEach ->
+					@fakeResponse =
+						statusCode: 500
+
+				it "should produce an error", (done) ->
+					@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err, stream)=> # empty callback
+						expect(stream).to.equal null
+						expect(err).to.not.equal null
+						expect(err instanceof Error).to.equal true
+						@Errors.NotFoundError.called.should.equal false
+						done()
 
 	describe "sendFile", ->