Merge pull request #34 from sharelatex/bg-make-backends-consistent

make filestore backends consistent

services/filestore/app/coffee/FSPersistorManager.coffee

@@ -13,10 +13,19 @@ module.exports =
   sendFile: ( location, target, source, callback = (err)->) ->
     filteredTarget = filterName target
     logger.log location:location, target:filteredTarget, source:source, "sending file"
-    fs.rename source, "#{location}/#{filteredTarget}", (err) ->
-      if err!=null
+    done = _.once (err) ->
+      if err?
         logger.err err:err, location:location, target:filteredTarget, source:source, "Error on put of file"
-      callback err
+      callback(err)
+    # actually copy the file (instead of moving it) to maintain consistent behaviour
+    # between the different implementations
+    sourceStream = fs.createReadStream source
+    sourceStream.on 'error', done
+    targetStream = fs.createWriteStream "#{location}/#{filteredTarget}"
+    targetStream.on 'error', done
+    targetStream.on 'finish', () ->
+      done()
+    sourceStream.pipe targetStream
 
   sendStream: ( location, target, sourceStream, callback = (err)->) ->
     logger.log location:location, target:target, "sending file stream"
@@ -26,7 +35,10 @@ module.exports =
       if err?
         logger.err  location:location, target:target, fsPath:fsPath, err:err, "something went wrong writing stream to disk"
         return callback err
-      @sendFile location, target, fsPath, callback
+      @sendFile location, target, fsPath, (err) -> 
+        # delete the temporary file created above and return the original error
+        LocalFileWriter.deleteFile fsPath, () ->
+          callback(err)
 
   # opts may be {start: Number, end: Number}
   getFileStream: (location, name, opts, _callback = (err, res)->) ->

services/filestore/app/coffee/S3PersistorManager.coffee

@@ -42,9 +42,8 @@ module.exports =
 			if res.statusCode != 200
 				logger.err bucketName:bucketName, key:key, fsPath:fsPath, "non 200 response from s3 putting file"
 				return callback("non 200 response from s3 on put file")
-			LocalFileWriter.deleteFile fsPath, (err)->
-				logger.log res:res,  bucketName:bucketName, key:key, fsPath:fsPath,"file uploaded to s3"
-				callback(err)
+			logger.log res:res,  bucketName:bucketName, key:key, fsPath:fsPath,"file uploaded to s3"
+			callback(err)
 		putEventEmiter.on "error", (err)->
 			logger.err err:err,  bucketName:bucketName, key:key, fsPath:fsPath, "error emmited on put of file"
 			callback err
@@ -57,7 +56,10 @@ module.exports =
 			if err?
 				logger.err  bucketName:bucketName, key:key, fsPath:fsPath, err:err, "something went wrong writing stream to disk"
 				return callback(err)
-			@sendFile bucketName, key, fsPath, callback
+			@sendFile bucketName, key, fsPath, (err) ->
+				# delete the temporary file created above and return the original error
+				LocalFileWriter.deleteFile fsPath, () ->
+					callback(err)
 
 	# opts may be {start: Number, end: Number}
 	getFileStream: (bucketName, key, opts, callback = (err, res)->)->

services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee

@@ -25,6 +25,7 @@ describe "FSPersistorManagerTests", ->
     @Rimraf = sinon.stub()
     @LocalFileWriter =
       writeStream: sinon.stub()
+      deleteFile: sinon.stub()
     @requires =
       "./LocalFileWriter":@LocalFileWriter
       "fs":@Fs
@@ -43,10 +44,32 @@ describe "FSPersistorManagerTests", ->
     @FSPersistorManager = SandboxedModule.require modulePath, requires: @requires
 
   describe "sendFile", ->
-    it "should put the file", (done) ->
-      @Fs.rename.callsArgWith(2,@error)
+    beforeEach ->
+      @Fs.createReadStream = sinon.stub().returns({
+        on: ->
+        pipe: ->
+      })
+
+    it "should copy the file", (done) ->
+      @Fs.createWriteStream =sinon.stub().returns({
+        on: (event, handler) ->
+          process.nextTick(handler) if event is 'finish'
+      })
+      @FSPersistorManager.sendFile @location, @name1, @name2, (err)=>
+        @Fs.createReadStream.calledWith(@name2).should.equal true
+        @Fs.createWriteStream.calledWith("#{@location}/#{@name1Filtered}" ).should.equal true
+        done()
+
+    it "should return an error if the file cannot be stored", (done) ->
+      @Fs.createWriteStream =sinon.stub().returns({
+        on: (event, handler) =>
+         if event is 'error'
+          process.nextTick () =>
+            handler(@error)
+      })
       @FSPersistorManager.sendFile @location, @name1, @name2, (err)=>
-        @Fs.rename.calledWith( @name2, "#{@location}/#{@name1Filtered}" ).should.equal true
+        @Fs.createReadStream.calledWith(@name2).should.equal true
+        @Fs.createWriteStream.calledWith("#{@location}/#{@name1Filtered}" ).should.equal true
         err.should.equal @error
         done()
 
@@ -54,6 +77,7 @@ describe "FSPersistorManagerTests", ->
     beforeEach ->
       @FSPersistorManager.sendFile = sinon.stub().callsArgWith(3)
       @LocalFileWriter.writeStream.callsArgWith(2, null, @name1)
+      @LocalFileWriter.deleteFile.callsArg(1)
       @SourceStream =
         on:->