From bfd41fdaf9c6996bc8b281a70be22da4403f4c63 Mon Sep 17 00:00:00 2001 From: Xavier Trochu Date: Fri, 20 Nov 2015 12:02:22 +0100 Subject: [PATCH] Add aws-sdk unit test. Fix Aws-Sdk persistor to return a correct error on file not found. Fix FileHandler after some change were lost on a previous merge --- .../app/coffee/AWSSDKPersistorManager.coffee | 3 + .../filestore/app/coffee/FileHandler.coffee | 9 +- .../coffee/AWSSDKPersistorManagerTests.coffee | 249 ++++++++++++++++++ 3 files changed, 256 insertions(+), 5 deletions(-) create mode 100644 services/filestore/test/unit/coffee/AWSSDKPersistorManagerTests.coffee diff --git a/services/filestore/app/coffee/AWSSDKPersistorManager.coffee b/services/filestore/app/coffee/AWSSDKPersistorManager.coffee index 13ba460ae3..b1465c126d 100644 --- a/services/filestore/app/coffee/AWSSDKPersistorManager.coffee +++ b/services/filestore/app/coffee/AWSSDKPersistorManager.coffee @@ -2,6 +2,7 @@ logger = require "logger-sharelatex" aws = require "aws-sdk" _ = require "underscore" fs = require "fs" +Errors = require("./Errors") s3 = new aws.S3() @@ -35,6 +36,8 @@ module.exports = callback null, stream stream.on 'error', (err) -> logger.err err:err, bucketName:bucketName, key:key, "error getting file stream from s3" + if err.code == 'NoSuchKey' + return callback new Errors.NotFoundError "File not found in S3: #{bucketName}:#{key}" callback err copyFile: (bucketName, sourceKey, destKey, callback)-> diff --git a/services/filestore/app/coffee/FileHandler.coffee b/services/filestore/app/coffee/FileHandler.coffee index 917df653db..a7f9ed84b7 100644 --- a/services/filestore/app/coffee/FileHandler.coffee +++ b/services/filestore/app/coffee/FileHandler.coffee @@ -41,16 +41,15 @@ module.exports = if err? return callback err if exists - PersistorManager.getFileStream bucket, convertedKey, callback + PersistorManager.getFileStream bucket, convertedKey, null, callback else @_getConvertedFileAndCache bucket, key, convertedKey, opts, callback _getConvertedFileAndCache: (bucket, key, convertedKey, opts, callback)-> - self = @ convertedFsPath = "" async.series [ - (cb)-> - self._convertFile bucket, key, opts, (err, fileSystemPath)-> + (cb) => + @_convertFile bucket, key, opts, (err, fileSystemPath) -> convertedFsPath = fileSystemPath cb err (cb)-> @@ -60,7 +59,7 @@ module.exports = ], (err)-> if err? return callback(err) - PersistorManager.getFileStream bucket, convertedKey, callback + PersistorManager.getFileStream bucket, convertedKey, opts, callback _convertFile: (bucket, originalKey, opts, callback)-> @_writeS3FileToDisk bucket, originalKey, opts, (err, originalFsPath)-> diff --git a/services/filestore/test/unit/coffee/AWSSDKPersistorManagerTests.coffee b/services/filestore/test/unit/coffee/AWSSDKPersistorManagerTests.coffee new file mode 100644 index 0000000000..a3ec3db5ba --- /dev/null +++ b/services/filestore/test/unit/coffee/AWSSDKPersistorManagerTests.coffee @@ -0,0 +1,249 @@ +sinon = require 'sinon' +chai = require 'chai' + +should = chai.should() +expect = chai.expect + +modulePath = "../../../app/js/AWSSDKPersistorManager.js" +SandboxedModule = require 'sandboxed-module' + +describe "AWSSDKPersistorManager", -> + beforeEach -> + @settings = + filestore: + backend: "aws-sdk" + @s3 = + upload: sinon.stub() + getObject: sinon.stub() + copyObject: sinon.stub() + deleteObject: sinon.stub() + listObjects: sinon.stub() + deleteObjects: sinon.stub() + headObject: sinon.stub() + @awssdk = + S3: sinon.stub().returns @s3 + + @requires = + "aws-sdk": @awssdk + "settings-sharelatex": @settings + "logger-sharelatex": + log:-> + err:-> + "fs": @fs = + createReadStream: sinon.stub() + "./Errors": @Errors = + NotFoundError: sinon.stub() + @key = "my/key" + @bucketName = "my-bucket" + @error = "my error" + @AWSSDKPersistorManager = SandboxedModule.require modulePath, requires: @requires + + describe "sendFile", -> + beforeEach -> + @stream = {} + @fsPath = "/usr/local/some/file" + @fs.createReadStream.returns @stream + + it "should put the file with s3.upload", (done) -> + @s3.upload.callsArgWith 1 + @AWSSDKPersistorManager.sendFile @bucketName, @key, @fsPath, (err) => + expect(err).to.not.be.ok + expect(@s3.upload.calledOnce, "called only once").to.be.true + expect((@s3.upload.calledWith Bucket: @bucketName, Key: @key, Body: @stream) + , "called with correct arguments").to.be.true + done() + + it "should dispatch the error from s3.upload", (done) -> + @s3.upload.callsArgWith 1, @error + @AWSSDKPersistorManager.sendFile @bucketName, @key, @fsPath, (err) => + expect(err).to.equal @error + done() + + + describe "sendStream", -> + beforeEach -> + @stream = {} + + it "should put the file with s3.upload", (done) -> + @s3.upload.callsArgWith 1 + @AWSSDKPersistorManager.sendStream @bucketName, @key, @stream, (err) => + expect(err).to.not.be.ok + expect(@s3.upload.calledOnce, "called only once").to.be.true + expect((@s3.upload.calledWith Bucket: @bucketName, Key: @key, Body: @stream), + "called with correct arguments").to.be.true + done() + + it "should dispatch the error from s3.upload", (done) -> + @s3.upload.callsArgWith 1, @error + @AWSSDKPersistorManager.sendStream @bucketName, @key, @stream, (err) => + expect(err).to.equal @error + done() + + describe "getFileStream", -> + beforeEach -> + @opts = {} + @stream = {} + @read_stream = + on: @read_stream_on = sinon.stub() + @object = + createReadStream: sinon.stub().returns @read_stream + @s3.getObject.returns @object + + it "should return a stream from s3.getObject", (done) -> + @read_stream_on.withArgs('readable').callsArgWith 1 + + @AWSSDKPersistorManager.getFileStream @bucketName, @key, @opts, (err, stream) => + expect(@read_stream_on.calledTwice) + expect(err).to.not.be.ok + expect(stream, "returned the stream").to.equal @read_stream + expect((@s3.getObject.calledWith Bucket: @bucketName, Key: @key), + "called with correct arguments").to.be.true + done() + + describe "with start and end options", -> + beforeEach -> + @opts = + start: 0 + end: 8 + it "should pass headers to the s3.GetObject", (done) -> + @read_stream_on.withArgs('readable').callsArgWith 1 + @AWSSDKPersistorManager.getFileStream @bucketName, @key, @opts, (err, stream) => + expect((@s3.getObject.calledWith Bucket: @bucketName, Key: @key, Range: 'bytes=0-8'), + "called with correct arguments").to.be.true + done() + + describe "error conditions", -> + describe "when the file doesn't exist", -> + beforeEach -> + @error = new Error() + @error.code = 'NoSuchKey' + it "should produce a NotFoundError", (done) -> + @read_stream_on.withArgs('error').callsArgWith 1, @error + @AWSSDKPersistorManager.getFileStream @bucketName, @key, @opts, (err, stream) => + expect(stream).to.not.be.ok + expect(err).to.be.ok + expect(err instanceof @Errors.NotFoundError, "error is a correct instance").to.equal true + done() + + describe "when there is some other error", -> + beforeEach -> + @error = new Error() + it "should dispatch the error from s3 object stream", (done) -> + @read_stream_on.withArgs('error').callsArgWith 1, @error + @AWSSDKPersistorManager.getFileStream @bucketName, @key, @opts, (err, stream) => + expect(stream).to.not.be.ok + expect(err).to.be.ok + expect(err).to.equal @error + done() + + describe "copyFile", -> + beforeEach -> + @destKey = "some/key" + @stream = {} + + it "should copy the file with s3.copyObject", (done) -> + @s3.copyObject.callsArgWith 1 + @AWSSDKPersistorManager.copyFile @bucketName, @key, @destKey, (err) => + expect(err).to.not.be.ok + expect(@s3.copyObject.calledOnce, "called only once").to.be.true + expect((@s3.copyObject.calledWith Bucket: @bucketName, Key: @destKey, CopySource: @bucketName + '/' + @key), + "called with correct arguments").to.be.true + done() + + it "should dispatch the error from s3.copyObject", (done) -> + @s3.copyObject.callsArgWith 1, @error + @AWSSDKPersistorManager.copyFile @bucketName, @key, @destKey, (err) => + expect(err).to.equal @error + done() + + describe "deleteFile", -> + it "should delete the file with s3.deleteObject", (done) -> + @s3.deleteObject.callsArgWith 1 + @AWSSDKPersistorManager.deleteFile @bucketName, @key, (err) => + expect(err).to.not.be.ok + expect(@s3.deleteObject.calledOnce, "called only once").to.be.true + expect((@s3.deleteObject.calledWith Bucket: @bucketName, Key: @key), + "called with correct arguments").to.be.true + done() + + it "should dispatch the error from s3.deleteObject", (done) -> + @s3.deleteObject.callsArgWith 1, @error + @AWSSDKPersistorManager.deleteFile @bucketName, @key, (err) => + expect(err).to.equal @error + done() + + describe "deleteDirectory", -> + + it "should list the directory content using s3.listObjects", (done) -> + @s3.listObjects.callsArgWith 1, null, Contents: [] + @AWSSDKPersistorManager.deleteDirectory @bucketName, @key, (err) => + expect(err).to.not.be.ok + expect(@s3.listObjects.calledOnce, "called only once").to.be.true + expect((@s3.listObjects.calledWith Bucket: @bucketName, Prefix: @key), + "called with correct arguments").to.be.true + done() + + it "should dispatch the error from s3.listObjects", (done) -> + @s3.listObjects.callsArgWith 1, @error + @AWSSDKPersistorManager.deleteDirectory @bucketName, @key, (err) => + expect(err).to.equal @error + done() + + describe "with directory content", -> + beforeEach -> + @fileList = [ + Key: 'foo' + , Key: 'bar' + , Key: 'baz' + ] + + it "should forward the file keys to s3.deleteObjects", (done) -> + @s3.listObjects.callsArgWith 1, null, Contents: @fileList + @s3.deleteObjects.callsArgWith 1 + @AWSSDKPersistorManager.deleteDirectory @bucketName, @key, (err) => + expect(err).to.not.be.ok + expect(@s3.deleteObjects.calledOnce, "called only once").to.be.true + expect((@s3.deleteObjects.calledWith + Bucket: @bucketName + Delete: + Quiet: true + Objects: @fileList), + "called with correct arguments").to.be.true + done() + + it "should dispatch the error from s3.deleteObjects", (done) -> + @s3.listObjects.callsArgWith 1, null, Contents: @fileList + @s3.deleteObjects.callsArgWith 1, @error + @AWSSDKPersistorManager.deleteDirectory @bucketName, @key, (err) => + expect(err).to.equal @error + done() + + + describe "checkIfFileExists", -> + + it "should check for the file with s3.headObject", (done) -> + @s3.headObject.callsArgWith 1, null, {} + @AWSSDKPersistorManager.checkIfFileExists @bucketName, @key, (err, exists) => + expect(err).to.not.be.ok + expect(@s3.headObject.calledOnce, "called only once").to.be.true + expect((@s3.headObject.calledWith Bucket: @bucketName, Key: @key), + "called with correct arguments").to.be.true + done() + + it "should return false on an inexistant file", (done) -> + @s3.headObject.callsArgWith 1, null, {} + @AWSSDKPersistorManager.checkIfFileExists @bucketName, @key, (err, exists) => + expect(exists).to.be.false + done() + + it "should return true on an existing file", (done) -> + @s3.headObject.callsArgWith 1, null, ETag: "etag" + @AWSSDKPersistorManager.checkIfFileExists @bucketName, @key, (err, exists) => + expect(exists).to.be.true + done() + + it "should dispatch the error from s3.headObject", (done) -> + @s3.headObject.callsArgWith 1, @error + @AWSSDKPersistorManager.checkIfFileExists @bucketName, @key, (err, exists) => + expect(err).to.equal @error + done()