From cf12ec1154567877a63cf0edc8eb62719e7ff138 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 7 Dec 2018 17:02:34 +0000 Subject: [PATCH 1/2] use the aws sdk to copy files in S3PersistorManager to work around problems with knox --- .../app/coffee/S3PersistorManager.coffee | 17 +++++++++++------ .../unit/coffee/S3PersistorManagerTests.coffee | 10 +++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee index d6ab47c200..8debdaad4f 100644 --- a/services/filestore/app/coffee/S3PersistorManager.coffee +++ b/services/filestore/app/coffee/S3PersistorManager.coffee @@ -11,6 +11,7 @@ path = require("path") LocalFileWriter = require("./LocalFileWriter") Errors = require("./Errors") _ = require("underscore") +awsS3 = require "aws-sdk/clients/s3" thirtySeconds = 30 * 1000 @@ -25,6 +26,12 @@ buildDefaultOptions = (bucketName, method, key)-> uri:"https://#{bucketName}.s3.amazonaws.com/#{key}" } +s3 = new awsS3({ + credentials: + accessKeyId: settings.filestore.s3.key, + secretAccessKey: settings.filestore.s3.secret +}) + module.exports = sendFile: (bucketName, key, fsPath, callback)-> @@ -90,12 +97,10 @@ module.exports = callback err copyFile: (bucketName, sourceKey, destKey, callback)-> - logger.log bucketName:bucketName, sourceKey:sourceKey, destKey:destKey, "copying file in s3" - s3Client = knox.createClient - key: settings.filestore.s3.key - secret: settings.filestore.s3.secret - bucket: bucketName - s3Client.copyFile sourceKey, destKey, (err)-> + logger.log bucketName:bucketName, sourceKey:sourceKey, destKey: destKey, "copying file in s3" + source = bucketName + '/' + sourceKey + # use the AWS SDK instead of knox due to problems with error handling (https://github.com/Automattic/knox/issues/114) + s3.copyObject {Bucket: bucketName, Key: destKey, CopySource: source}, (err) -> if err? logger.err err:err, bucketName:bucketName, sourceKey:sourceKey, destKey:destKey, "something went wrong copying file in aws" callback(err) diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee index b48fde7820..0860514180 100644 --- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee @@ -25,11 +25,15 @@ describe "S3PersistorManagerTests", -> get: sinon.stub() @knox = createClient: sinon.stub().returns(@stubbedKnoxClient) + @stubbedS3Client = + copyObject:sinon.stub() + @awsS3 = sinon.stub().returns @stubbedS3Client @LocalFileWriter = writeStream: sinon.stub() deleteFile: sinon.stub() @requires = "knox": @knox + "aws-sdk/clients/s3": @awsS3 "settings-sharelatex": @settings "./LocalFileWriter":@LocalFileWriter "logger-sharelatex": @@ -207,11 +211,11 @@ describe "S3PersistorManagerTests", -> @destKey = "my/dest/key" @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires - it "should use knox to copy file", (done)-> - @stubbedKnoxClient.copyFile.callsArgWith(2, @error) + it "should use AWS SDK to copy file", (done)-> + @stubbedS3Client.copyObject.callsArgWith(1, @error) @S3PersistorManager.copyFile @bucketName, @sourceKey, @destKey, (err)=> err.should.equal @error - @stubbedKnoxClient.copyFile.calledWith(@sourceKey, @destKey).should.equal true + @stubbedS3Client.copyObject.calledWith({Bucket: @bucketName, Key: @destKey, CopySource: @bucketName + '/' + @key}).should.equal true done() describe "deleteDirectory", -> From 9d93eee3e8bfeb0dbd32bbc458b674504e49b811 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 9 Jan 2019 10:31:59 +0000 Subject: [PATCH 2/2] return a 404 error (instead of a 500) when copying a missing file --- services/filestore/app/coffee/FileController.coffee | 7 +++++-- .../filestore/app/coffee/S3PersistorManager.coffee | 10 ++++++++-- .../test/unit/coffee/FileControllerTests.coffee | 9 +++++++++ .../test/unit/coffee/S3PersistorManagerTests.coffee | 7 +++++++ 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/services/filestore/app/coffee/FileController.coffee b/services/filestore/app/coffee/FileController.coffee index 24fd5229de..60dcc207b7 100644 --- a/services/filestore/app/coffee/FileController.coffee +++ b/services/filestore/app/coffee/FileController.coffee @@ -60,8 +60,11 @@ module.exports = FileController = logger.log key:key, bucket:bucket, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "reciving request to copy file" PersistorManager.copyFile bucket, "#{oldProject_id}/#{oldFile_id}", key, (err)-> if err? - logger.log err:err, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "something went wrong copying file" - res.send 500 + if err instanceof Errors.NotFoundError + res.send 404 + else + logger.log err:err, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "something went wrong copying file" + res.send 500 else res.send 200 diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee index 8debdaad4f..dd9aae3bf7 100644 --- a/services/filestore/app/coffee/S3PersistorManager.coffee +++ b/services/filestore/app/coffee/S3PersistorManager.coffee @@ -102,8 +102,14 @@ module.exports = # use the AWS SDK instead of knox due to problems with error handling (https://github.com/Automattic/knox/issues/114) s3.copyObject {Bucket: bucketName, Key: destKey, CopySource: source}, (err) -> if err? - logger.err err:err, bucketName:bucketName, sourceKey:sourceKey, destKey:destKey, "something went wrong copying file in aws" - callback(err) + if err.code is 'NoSuchKey' + logger.err bucketName:bucketName, sourceKey:sourceKey, "original file not found in s3 when copying" + callback(new Errors.NotFoundError("original file not found in S3 when copying")) + else + logger.err err:err, bucketName:bucketName, sourceKey:sourceKey, destKey:destKey, "something went wrong copying file in aws" + callback(err) + else + callback() deleteFile: (bucketName, key, callback)-> logger.log bucketName:bucketName, key:key, "delete file in s3" diff --git a/services/filestore/test/unit/coffee/FileControllerTests.coffee b/services/filestore/test/unit/coffee/FileControllerTests.coffee index 591644de60..0645aff27c 100644 --- a/services/filestore/test/unit/coffee/FileControllerTests.coffee +++ b/services/filestore/test/unit/coffee/FileControllerTests.coffee @@ -28,6 +28,8 @@ describe "FileController", -> "./LocalFileWriter":@LocalFileWriter "./FileHandler": @FileHandler "./PersistorManager":@PersistorManager + "./Errors": @Errors = + NotFoundError: sinon.stub() "settings-sharelatex": @settings "logger-sharelatex": log:-> @@ -111,6 +113,13 @@ describe "FileController", -> done() @controller.copyFile @req, @res + it "should send a 404 if the original file was not found", (done) -> + @PersistorManager.copyFile.callsArgWith(3, new @Errors.NotFoundError()) + @res.send = (code)=> + code.should.equal 404 + done() + @controller.copyFile @req, @res + it "should send a 500 if there was an error", (done)-> @PersistorManager.copyFile.callsArgWith(3, "error") @res.send = (code)=> diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee index 0860514180..7fc70c5065 100644 --- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee @@ -218,6 +218,13 @@ describe "S3PersistorManagerTests", -> @stubbedS3Client.copyObject.calledWith({Bucket: @bucketName, Key: @destKey, CopySource: @bucketName + '/' + @key}).should.equal true done() + it "should return a NotFoundError object if the original file does not exist", (done)-> + NoSuchKeyError = {code: "NoSuchKey"} + @stubbedS3Client.copyObject.callsArgWith(1, NoSuchKeyError) + @S3PersistorManager.copyFile @bucketName, @sourceKey, @destKey, (err)=> + expect(err instanceof @Errors.NotFoundError).to.equal true + done() + describe "deleteDirectory", -> beforeEach ->