From af4c72150f0fa93f33eb156b699e9e73235cb441 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 19 Dec 2014 16:57:44 +0000 Subject: [PATCH 01/13] fix path in directory deletion --- services/filestore/app/coffee/FileHandler.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/filestore/app/coffee/FileHandler.coffee b/services/filestore/app/coffee/FileHandler.coffee index 8968470300..51aec4bba6 100644 --- a/services/filestore/app/coffee/FileHandler.coffee +++ b/services/filestore/app/coffee/FileHandler.coffee @@ -16,10 +16,10 @@ module.exports = PersistorManager.sendStream bucket, key, stream, callback deleteFile: (bucket, key, callback)-> - convetedKey = KeyBuilder.getConvertedFolderKey(bucket, key) + convetedKey = KeyBuilder.getConvertedFolderKey(key) async.parallel [ (done)-> PersistorManager.deleteFile bucket, key, done - (done)-> PersistorManager.deleteFile bucket, convetedKey, done + (done)-> PersistorManager.deleteDirectory bucket, convetedKey, done ], callback getFile: (bucket, key, opts = {}, callback)-> From 8744cf94a55f27df0f0d63f1db52afb0b16d7333 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 19 Dec 2014 16:58:04 +0000 Subject: [PATCH 02/13] delete directory with rimraf --- services/filestore/app/coffee/FSPersistorManager.coffee | 9 +++++---- services/filestore/package.json | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index cbdad516d6..1f2d64b8cb 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -1,6 +1,7 @@ logger = require("logger-sharelatex") fs = require("fs") LocalFileWriter = require("./LocalFileWriter") +rimraf = require("rimraf") filterName = (key) -> return key.replace /\//g, "_" @@ -57,10 +58,10 @@ module.exports = callback err deleteDirectory: (location, name, callback = (err)->)-> - filteredName = filterName name - fs.rmdir "#{location}/#{filteredName}", (err) -> - logger.err err:err, location:location, name:filteredName, "Error on rmdir." - if err and err.code != 'ENOENT' + filteredName = filterName name.replace(/\/$/,'') + rimraf "#{location}/#{filteredName}", (err) -> + if err + logger.err err:err, location:location, name:filteredName, "Error on rimraf rmdir." callback err else callback() diff --git a/services/filestore/package.json b/services/filestore/package.json index be103715f7..c2fcaff080 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -21,6 +21,7 @@ "node-uuid": "~1.4.1", "pngcrush": "0.0.3", "request": "2.14.0", + "rimraf": "2.2.8", "settings-sharelatex": "git+https://github.com/sharelatex/settings-sharelatex.git#master", "stream-buffers": "~0.2.5", "underscore": "~1.5.2" From 7d96f3aadd58550571c56696ea7ac534c71f1df3 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 11:10:05 +0000 Subject: [PATCH 03/13] use 'response' to send 'NoSuchKey' when local file is not found --- services/filestore/app/coffee/FSPersistorManager.coffee | 9 +++++++-- services/filestore/package.json | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index 1f2d64b8cb..94ededd700 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -2,6 +2,7 @@ logger = require("logger-sharelatex") fs = require("fs") LocalFileWriter = require("./LocalFileWriter") rimraf = require("rimraf") +response = require ("response") filterName = (key) -> return key.replace /\//g, "_" @@ -32,8 +33,12 @@ module.exports = sourceStream = fs.createReadStream "#{location}/#{filteredName}" sourceStream.on 'error', (err) -> logger.err err:err, location:location, name:name, "Error reading from file" - callback err - callback null,sourceStream + if err.code = 'ENOENT' + callback null, response().html('NoSuchKey: file not found\n') + else + callback err + sourceStream.on 'readable', () -> + callback null, sourceStream copyFile: (location, fromName, toName, callback = (err)->)-> diff --git a/services/filestore/package.json b/services/filestore/package.json index c2fcaff080..530f95d00b 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -21,6 +21,7 @@ "node-uuid": "~1.4.1", "pngcrush": "0.0.3", "request": "2.14.0", + "response": "0.14.0", "rimraf": "2.2.8", "settings-sharelatex": "git+https://github.com/sharelatex/settings-sharelatex.git#master", "stream-buffers": "~0.2.5", From 72ebaea5e24d355d1c2ebb0b8983a301f52a431f Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 11:11:31 +0000 Subject: [PATCH 04/13] log FS delete errors only on failure --- services/filestore/app/coffee/FSPersistorManager.coffee | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index 94ededd700..d00b112c27 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -59,13 +59,16 @@ module.exports = filteredName = filterName name logger.log location:location, name:filteredName, "delete file" fs.unlink "#{location}/#{filteredName}", (err) -> - logger.err err:err, location:location, name:filteredName, "Error on delete." - callback err + if err? + logger.err err:err, location:location, name:filteredName, "Error on delete." + callback err + else + callback() deleteDirectory: (location, name, callback = (err)->)-> filteredName = filterName name.replace(/\/$/,'') rimraf "#{location}/#{filteredName}", (err) -> - if err + if err? logger.err err:err, location:location, name:filteredName, "Error on rimraf rmdir." callback err else From dad209262f15115581e1b47b03a7f795ed58ac2b Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 11:27:00 +0000 Subject: [PATCH 05/13] execute success callback when filesystem copyfile completed --- services/filestore/app/coffee/FSPersistorManager.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index d00b112c27..4d6d79b1a7 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -53,6 +53,8 @@ module.exports = targetStream.on 'error', (err) -> logger.err err:err, location:location, key:filteredToName, "Error writing to file" callback err + targetStream.on 'finish', () -> + callback null sourceStream.pipe targetStream deleteFile: (location, name, callback)-> From de465aee945e84b6f753918917bf79783fa1664c Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 11:27:48 +0000 Subject: [PATCH 06/13] change copy test to match usage in web, check http status --- .../filestore/test/acceptence/coffee/SendingFileTest.coffee | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/filestore/test/acceptence/coffee/SendingFileTest.coffee b/services/filestore/test/acceptence/coffee/SendingFileTest.coffee index 781a1ef6ad..a3958ebf90 100644 --- a/services/filestore/test/acceptence/coffee/SendingFileTest.coffee +++ b/services/filestore/test/acceptence/coffee/SendingFileTest.coffee @@ -70,13 +70,15 @@ describe "Filestore", -> newProjectID = "acceptence_tests_copyied_project" newFileId = Math.random() newFileUrl = "#{@filestoreUrl}/project/#{newProjectID}/file/#{newFileId}" - opts = + opts = + method: 'put' uri: newFileUrl json: source: project_id:"acceptence_tests" file_id: @file_id - request.put opts, (err)=> + request opts, (err, response, body)=> + response.statusCode.should.equal 200 request.del @fileUrl, (err, response, body)=> request.get newFileUrl, (err, response, body)=> body.should.equal @constantFileContent From 64c6341844c7135aefc13ae7c4b55a4ed151163a Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 11:34:13 +0000 Subject: [PATCH 07/13] check delete status code in copy test --- services/filestore/test/acceptence/coffee/SendingFileTest.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/filestore/test/acceptence/coffee/SendingFileTest.coffee b/services/filestore/test/acceptence/coffee/SendingFileTest.coffee index a3958ebf90..11668f230e 100644 --- a/services/filestore/test/acceptence/coffee/SendingFileTest.coffee +++ b/services/filestore/test/acceptence/coffee/SendingFileTest.coffee @@ -80,6 +80,7 @@ describe "Filestore", -> request opts, (err, response, body)=> response.statusCode.should.equal 200 request.del @fileUrl, (err, response, body)=> + response.statusCode.should.equal 204 request.get newFileUrl, (err, response, body)=> body.should.equal @constantFileContent done() From 3f18f6a7142fbb023c9683072aab2f4140172d70 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 12:58:11 +0000 Subject: [PATCH 08/13] update unit tests - FileHandler now calls deleteDirectory on cache directory --- services/filestore/test/unit/coffee/FileHandlerTests.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/filestore/test/unit/coffee/FileHandlerTests.coffee b/services/filestore/test/unit/coffee/FileHandlerTests.coffee index b00f23af0e..2a2bf6e17d 100644 --- a/services/filestore/test/unit/coffee/FileHandlerTests.coffee +++ b/services/filestore/test/unit/coffee/FileHandlerTests.coffee @@ -69,6 +69,7 @@ describe "FileHandler", -> beforeEach -> @keyBuilder.getConvertedFolderKey.returns(@stubbedConvetedKey) @PersistorManager.deleteFile.callsArgWith(2) + @PersistorManager.deleteDirectory.callsArgWith(2) it "should tell the filestore manager to delete the file", (done)-> @handler.deleteFile @bucket, @key, => @@ -77,7 +78,7 @@ describe "FileHandler", -> it "should tell the filestore manager to delete the cached foler", (done)-> @handler.deleteFile @bucket, @key, => - @PersistorManager.deleteFile.calledWith(@bucket, @stubbedConvetedKey).should.equal true + @PersistorManager.deleteDirectory.calledWith(@bucket, @stubbedConvetedKey).should.equal true done() describe "getFile", -> From efd603584936147cf1f6a5ce6f8db83a69849164 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 13:01:39 +0000 Subject: [PATCH 09/13] test against rimraf module when deleting directory recursively --- .../test/unit/coffee/FSPersistorManagerTests.coffee | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee index bf5f08ea9d..b69b17c1aa 100644 --- a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee @@ -6,6 +6,7 @@ expect = chai.expect modulePath = "../../../app/js/FSPersistorManager.js" SandboxedModule = require('sandboxed-module') fs = require("fs") +response = require("response") describe "FSPersistorManagerTests", -> @@ -17,6 +18,7 @@ describe "FSPersistorManagerTests", -> unlink:sinon.stub() rmdir:sinon.stub() exists:sinon.stub() + @Rimraf = sinon.stub() @LocalFileWriter = writeStream: sinon.stub() @requires = @@ -25,6 +27,8 @@ describe "FSPersistorManagerTests", -> "logger-sharelatex": log:-> err:-> + "response":response + "rimraf":@Rimraf @location = "/tmp" @name1 = "530f2407e7ef165704000007/530f838b46d9a9e859000008" @name1Filtered ="530f2407e7ef165704000007_530f838b46d9a9e859000008" @@ -115,11 +119,11 @@ describe "FSPersistorManagerTests", -> describe "deleteDirectory", -> beforeEach -> - @Fs.rmdir.callsArgWith(1,@error) + @Rimraf.callsArgWith(1,@error) - it "Should call rmdir with correct options", (done) -> + it "Should call rmdir(rimraf) with correct options", (done) -> @FSPersistorManager.deleteDirectory @location, @name1, (err) => - @Fs.rmdir.calledWith("#{@location}/#{@name1}").should.equal.true + @Rimraf.calledWith("#{@location}/#{@name1}").should.equal.true done() it "Should propogate the error", (done) -> From 8b9213d09bd5d2a34bce23e355990cfaeeaa3b17 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 13:07:03 +0000 Subject: [PATCH 10/13] fix getFileStream unit test as callback no longer fires immediately success callback is now called only when file is readable --- .../filestore/test/unit/coffee/FSPersistorManagerTests.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee index b69b17c1aa..786f6be323 100644 --- a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee @@ -74,8 +74,8 @@ describe "FSPersistorManagerTests", -> on:-> ) @FSPersistorManager.getFileStream @location, @name1, (err,res)=> - @Fs.createReadStream.calledWith("#{@location}/#{@name1Filtered}").should.equal.true - done() + @Fs.createReadStream.calledWith("#{@location}/#{@name1Filtered}").should.equal.true + done() describe "copyFile", -> beforeEach -> From d4c281e22c9df2f59a8e6ec77e2f997647532e53 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 13:10:40 +0000 Subject: [PATCH 11/13] add acceptance tests to travis.yml --- services/filestore/.travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/services/filestore/.travis.yml b/services/filestore/.travis.yml index 29f5884d60..ee2f36ad1c 100644 --- a/services/filestore/.travis.yml +++ b/services/filestore/.travis.yml @@ -12,6 +12,7 @@ install: script: - grunt test:unit + - grunt test:acceptance services: - redis-server From ad379ec1034bd8636b8333935634f6e35fcfffa1 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 13:13:36 +0000 Subject: [PATCH 12/13] add grunt target for test:acceptance --- services/filestore/Gruntfile.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/filestore/Gruntfile.coffee b/services/filestore/Gruntfile.coffee index aa40cf7460..d2d066b543 100644 --- a/services/filestore/Gruntfile.coffee +++ b/services/filestore/Gruntfile.coffee @@ -76,6 +76,7 @@ module.exports = (grunt) -> grunt.registerTask "test:unit", ["coffee", "mochaTest:unit"] grunt.registerTask "test:acceptence", ["coffee", "mochaTest:acceptence"] + grunt.registerTask "test:acceptance", ["test:acceptence"] grunt.registerTask "ci", "test:unit" grunt.registerTask 'default', ['coffee', 'concurrent'] From f4ac7d422b8586a1b80599ce95888f2f758c4879 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 22 Dec 2014 13:41:07 +0000 Subject: [PATCH 13/13] run acceptance tests on travis under forever --- services/filestore/.travis.yml | 3 +++ services/filestore/Gruntfile.coffee | 6 ++++++ services/filestore/package.json | 3 ++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/services/filestore/.travis.yml b/services/filestore/.travis.yml index ee2f36ad1c..6adc08643a 100644 --- a/services/filestore/.travis.yml +++ b/services/filestore/.travis.yml @@ -10,6 +10,9 @@ install: - npm install - grunt install +before_script: + - grunt forever:app:start + script: - grunt test:unit - grunt test:acceptance diff --git a/services/filestore/Gruntfile.coffee b/services/filestore/Gruntfile.coffee index d2d066b543..91ae43bed1 100644 --- a/services/filestore/Gruntfile.coffee +++ b/services/filestore/Gruntfile.coffee @@ -2,6 +2,11 @@ module.exports = (grunt) -> # Project configuration. grunt.initConfig + forever: + app: + options: + index: "app.js" + coffee: server: expand: true, @@ -73,6 +78,7 @@ module.exports = (grunt) -> grunt.loadNpmTasks 'grunt-contrib-clean' grunt.loadNpmTasks 'grunt-concurrent' grunt.loadNpmTasks 'grunt-mocha-test' + grunt.loadNpmTasks 'grunt-forever' grunt.registerTask "test:unit", ["coffee", "mochaTest:unit"] grunt.registerTask "test:acceptence", ["coffee", "mochaTest:acceptence"] diff --git a/services/filestore/package.json b/services/filestore/package.json index 530f95d00b..919adc488f 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -37,6 +37,7 @@ "grunt-contrib-watch": "0.5.3", "grunt-nodemon": "0.2.1", "grunt-contrib-clean": "0.5.0", - "grunt-concurrent": "0.4.2" + "grunt-concurrent": "0.4.2", + "grunt-forever": "~0.4.4" } }