From 3575c89d03216d537ba1e2fbdb0d05e3e7d944e0 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Thu, 13 Jun 2019 16:57:49 -0400 Subject: [PATCH] Return file size on HEAD request This will be used by the file preview feature when it gets partial content. --- services/filestore/app.coffee | 13 +-- .../app/coffee/FSPersistorManager.coffee | 15 +++- .../app/coffee/FileController.coffee | 24 ++++-- .../filestore/app/coffee/FileHandler.coffee | 3 + .../app/coffee/S3PersistorManager.coffee | 26 +++++- .../acceptance/coffee/SendingFileTest.coffee | 9 ++- .../coffee/FSPersistorManagerTests.coffee | 29 +++++++ .../unit/coffee/FileControllerTests.coffee | 37 ++++++++- .../test/unit/coffee/FileHandlerTests.coffee | 1 - .../coffee/S3PersistorManagerTests.coffee | 81 ++++++++++++------- 10 files changed, 191 insertions(+), 47 deletions(-) diff --git a/services/filestore/app.coffee b/services/filestore/app.coffee index 3e927a2b51..6c21686826 100644 --- a/services/filestore/app.coffee +++ b/services/filestore/app.coffee @@ -63,22 +63,23 @@ app.use (req, res, next) -> Metrics.injectMetricsRoute(app) +app.head "/project/:project_id/file/:file_id", keyBuilder.userFileKey, fileController.getFileHead app.get "/project/:project_id/file/:file_id", keyBuilder.userFileKey, fileController.getFile app.post "/project/:project_id/file/:file_id", keyBuilder.userFileKey, fileController.insertFile +app.put "/project/:project_id/file/:file_id", keyBuilder.userFileKey, bodyParser.json(), fileController.copyFile +app.del "/project/:project_id/file/:file_id", keyBuilder.userFileKey, fileController.deleteFile -app.put "/project/:project_id/file/:file_id", keyBuilder.userFileKey, bodyParser.json(), fileController.copyFile -app.del "/project/:project_id/file/:file_id", keyBuilder.userFileKey, fileController.deleteFile - +app.head "/template/:template_id/v/:version/:format", keyBuilder.templateFileKey, fileController.getFileHead app.get "/template/:template_id/v/:version/:format", keyBuilder.templateFileKey, fileController.getFile app.get "/template/:template_id/v/:version/:format/:sub_type", keyBuilder.templateFileKey, fileController.getFile app.post "/template/:template_id/v/:version/:format", keyBuilder.templateFileKey, fileController.insertFile +app.head "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.getFileHead app.get "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.getFile app.post "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.insertFile - -app.put "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, bodyParser.json(), fileController.copyFile -app.del "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.deleteFile +app.put "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, bodyParser.json(), fileController.copyFile +app.del "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.deleteFile app.get "/project/:project_id/size", keyBuilder.publicProjectKey, fileController.directorySize diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index 353e2ef099..b33d0d85bf 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -1,5 +1,6 @@ logger = require("logger-sharelatex") fs = require("fs") +path = require("path") LocalFileWriter = require("./LocalFileWriter") Errors = require('./Errors') rimraf = require("rimraf") @@ -35,7 +36,7 @@ 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, (err) -> + @sendFile location, target, fsPath, (err) -> # delete the temporary file created above and return the original error LocalFileWriter.deleteFile fsPath, () -> callback(err) @@ -57,6 +58,18 @@ module.exports = # ensures the callback is only called once return callback null, sourceStream + getFileSize: (location, filename, callback) -> + fullPath = path.join(location, filterName(filename)) + fs.stat fullPath, (err, stats) -> + if err? + if err.code == 'ENOENT' + logger.log({location:location, filename:filename}, "file not found") + callback(new Errors.NotFoundError(err.message)) + else + logger.err({err:err, location:location, filename:filename}, "failed to stat file") + callback(err) + return + callback(null, stats.size) copyFile: (location, fromName, toName, callback = (err)->)-> filteredFromName=filterName fromName diff --git a/services/filestore/app/coffee/FileController.coffee b/services/filestore/app/coffee/FileController.coffee index 60dcc207b7..f98dbd1e49 100644 --- a/services/filestore/app/coffee/FileController.coffee +++ b/services/filestore/app/coffee/FileController.coffee @@ -21,7 +21,7 @@ module.exports = FileController = style: style, } metrics.inc "getFile" - logger.log key:key, bucket:bucket, format:format, style: style, "reciving request to get file" + logger.log key:key, bucket:bucket, format:format, style: style, "receiving request to get file" if req.headers.range? range = FileController._get_range(req.headers.range) options.start = range.start @@ -41,10 +41,24 @@ module.exports = FileController = logger.log key:key, bucket:bucket, format:format, style:style, "sending file to response" fileStream.pipe res + getFileHead: (req, res) -> + {key, bucket} = req + metrics.inc("getFileSize") + logger.log({ key: key, bucket: bucket }, "receiving request to get file metadata") + FileHandler.getFileSize bucket, key, (err, fileSize) -> + if err? + if err instanceof Errors.NotFoundError + res.status(404).end() + else + res.status(500).end() + return + res.set("Content-Length", fileSize) + res.status(200).end() + insertFile: (req, res)-> metrics.inc "insertFile" {key, bucket} = req - logger.log key:key, bucket:bucket, "reciving request to insert file" + logger.log key:key, bucket:bucket, "receiving request to insert file" FileHandler.insertFile bucket, key, req, (err)-> if err? logger.log err: err, key: key, bucket: bucket, "error inserting file" @@ -57,7 +71,7 @@ module.exports = FileController = {key, bucket} = req oldProject_id = req.body.source.project_id oldFile_id = req.body.source.file_id - logger.log key:key, bucket:bucket, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "reciving request to copy file" + logger.log key:key, bucket:bucket, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "receiving request to copy file" PersistorManager.copyFile bucket, "#{oldProject_id}/#{oldFile_id}", key, (err)-> if err? if err instanceof Errors.NotFoundError @@ -71,7 +85,7 @@ module.exports = FileController = deleteFile: (req, res)-> metrics.inc "deleteFile" {key, bucket} = req - logger.log key:key, bucket:bucket, "reciving request to delete file" + logger.log key:key, bucket:bucket, "receiving request to delete file" FileHandler.deleteFile bucket, key, (err)-> if err? logger.log err:err, key:key, bucket:bucket, "something went wrong deleting file" @@ -90,7 +104,7 @@ module.exports = FileController = directorySize: (req, res)-> metrics.inc "projectSize" {project_id, bucket} = req - logger.log project_id:project_id, bucket:bucket, "reciving request to project size" + logger.log project_id:project_id, bucket:bucket, "receiving request to project size" FileHandler.getDirectorySize bucket, project_id, (err, size)-> if err? logger.log err: err, project_id: project_id, bucket: bucket, "error inserting file" diff --git a/services/filestore/app/coffee/FileHandler.coffee b/services/filestore/app/coffee/FileHandler.coffee index 6189c8a906..cb8d78a0fe 100644 --- a/services/filestore/app/coffee/FileHandler.coffee +++ b/services/filestore/app/coffee/FileHandler.coffee @@ -31,6 +31,9 @@ module.exports = FileHandler = else @_getConvertedFile bucket, key, opts, callback + getFileSize: (bucket, key, callback) -> + PersistorManager.getFileSize(bucket, key, callback) + _getStandardFile: (bucket, key, opts, callback)-> PersistorManager.getFileStream bucket, key, opts, (err, fileStream)-> if err? and !(err instanceof Errors.NotFoundError) diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee index 2b183730d6..50486a0ce3 100644 --- a/services/filestore/app/coffee/S3PersistorManager.coffee +++ b/services/filestore/app/coffee/S3PersistorManager.coffee @@ -91,12 +91,36 @@ module.exports = else if res.statusCode not in [200, 206] logger.log bucketName:bucketName, key:key, "error getting file from s3: #{res.statusCode}" return callback new Error("Got non-200 response from S3: #{res.statusCode}"), null - else + else return callback null, res s3Stream.on 'error', (err) -> logger.err err:err, bucketName:bucketName, key:key, "error getting file stream from s3" callback err + getFileSize: (bucketName, key, callback) -> + logger.log({ bucketName: bucketName, key: key }, "getting file size from S3") + s3.headObject { Bucket: bucketName, Key: key }, (err, data) -> + if err? + if err.statusCode in [403, 404] + # S3 returns a 403 instead of a 404 when the user doesn't have + # permission to list the bucket contents. + logger.log({ + bucketName: bucketName, + key: key + }, "file not found in s3") + callback( + new Errors.NotFoundError("File not found in S3: #{bucketName}:#{key}") + ) + else + logger.err({ + bucketName: bucketName, + key: key, + err: err + }, "error performing S3 HeadObject") + callback(err) + return + callback(null, data.ContentLength) + copyFile: (bucketName, sourceKey, destKey, callback)-> logger.log bucketName:bucketName, sourceKey:sourceKey, destKey: destKey, "copying file in s3" source = bucketName + '/' + sourceKey diff --git a/services/filestore/test/acceptance/coffee/SendingFileTest.coffee b/services/filestore/test/acceptance/coffee/SendingFileTest.coffee index cee06e82a3..b77afb866b 100644 --- a/services/filestore/test/acceptance/coffee/SendingFileTest.coffee +++ b/services/filestore/test/acceptance/coffee/SendingFileTest.coffee @@ -32,7 +32,7 @@ describe "Filestore", -> - it "should send a 200 for status endpoing", (done)-> + it "should send a 200 for status endpoint", (done)-> request "#{@filestoreUrl}/status", (err, response, body)-> response.statusCode.should.equal 200 body.indexOf("filestore").should.not.equal -1 @@ -59,6 +59,13 @@ describe "Filestore", -> response.statusCode.should.equal 404 done() + it "should return the file size on a HEAD request", (done) -> + expectedLength = Buffer.byteLength(@constantFileContent) + request.head @fileUrl, (err, res) => + expect(res.statusCode).to.equal(200) + expect(res.headers['content-length']).to.equal(expectedLength.toString()) + done() + it "should be able get the file back", (done)-> @timeout(1000 * 10) request.get @fileUrl, (err, response, body)=> diff --git a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee index 7df7adfd8b..a08fc86682 100644 --- a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee @@ -22,6 +22,7 @@ describe "FSPersistorManagerTests", -> openSync:sinon.stub() fstatSync:sinon.stub() closeSync:sinon.stub() + stat:sinon.stub() @Rimraf = sinon.stub() @LocalFileWriter = writeStream: sinon.stub() @@ -156,7 +157,35 @@ describe "FSPersistorManagerTests", -> expect(err instanceof Error).to.equal true done() + describe "getFileSize", -> + it "should return the file size", (done) -> + expectedFileSize = 75382 + @Fs.stat.yields(new Error("fs.stat got unexpected arguments")) + @Fs.stat.withArgs("#{@location}/#{@name1Filtered}") + .yields(null, { size: expectedFileSize }) + @FSPersistorManager.getFileSize @location, @name1, (err, fileSize) => + if err? + return done(err) + expect(fileSize).to.equal(expectedFileSize) + done() + + it "should throw a NotFoundError if the file does not exist", (done) -> + error = new Error() + error.code = "ENOENT" + @Fs.stat.yields(error) + + @FSPersistorManager.getFileSize @location, @name1, (err, fileSize) => + expect(err).to.be.instanceof(@Errors.NotFoundError) + done() + + it "should rethrow any other error", (done) -> + error = new Error() + @Fs.stat.yields(error) + + @FSPersistorManager.getFileSize @location, @name1, (err, fileSize) => + expect(err).to.equal(error) + done() describe "copyFile", -> beforeEach -> diff --git a/services/filestore/test/unit/coffee/FileControllerTests.coffee b/services/filestore/test/unit/coffee/FileControllerTests.coffee index 09398b8c01..821aadb68d 100644 --- a/services/filestore/test/unit/coffee/FileControllerTests.coffee +++ b/services/filestore/test/unit/coffee/FileControllerTests.coffee @@ -20,6 +20,7 @@ describe "FileController", -> user_files:"user_files" @FileHandler = getFile: sinon.stub() + getFileSize: sinon.stub() deleteFile: sinon.stub() insertFile: sinon.stub() getDirectorySize: sinon.stub() @@ -49,7 +50,8 @@ describe "FileController", -> file_id:@file_id headers: {} @res = - setHeader: -> + set: sinon.stub().returnsThis() + status: sinon.stub().returnsThis() @fileStream = {} describe "getFile", -> @@ -89,6 +91,39 @@ describe "FileController", -> done() @controller.getFile @req, @res + describe "getFileHead", -> + it "should return the file size in a Content-Length header", (done) -> + expectedFileSize = 84921 + @FileHandler.getFileSize.yields( + new Error("FileHandler.getFileSize: unexpected arguments") + ) + @FileHandler.getFileSize.withArgs(@bucket, @key).yields(null, expectedFileSize) + + @res.end = () => + expect(@res.status.lastCall.args[0]).to.equal(200) + expect(@res.set.calledWith("Content-Length", expectedFileSize)).to.equal(true) + done() + + @controller.getFileHead(@req, @res) + + it "should return a 404 is the file is not found", (done) -> + @FileHandler.getFileSize.yields(new @Errors.NotFoundError()) + + @res.end = () => + expect(@res.status.lastCall.args[0]).to.equal(404) + done() + + @controller.getFileHead(@req, @res) + + it "should return a 500 on internal errors", (done) -> + @FileHandler.getFileSize.yields(new Error()) + + @res.end = () => + expect(@res.status.lastCall.args[0]).to.equal(500) + done() + + @controller.getFileHead(@req, @res) + describe "insertFile", -> it "should send bucket name key and res to PersistorManager", (done)-> diff --git a/services/filestore/test/unit/coffee/FileHandlerTests.coffee b/services/filestore/test/unit/coffee/FileHandlerTests.coffee index 50b8a17524..754366195e 100644 --- a/services/filestore/test/unit/coffee/FileHandlerTests.coffee +++ b/services/filestore/test/unit/coffee/FileHandlerTests.coffee @@ -108,7 +108,6 @@ describe "FileHandler", -> @handler._getConvertedFile.called.should.equal true done() - describe "_getStandardFile", -> beforeEach -> diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee index 1eee09dc29..2554461fd7 100644 --- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee @@ -26,11 +26,13 @@ describe "S3PersistorManagerTests", -> @knox = createClient: sinon.stub().returns(@stubbedKnoxClient) @stubbedS3Client = - copyObject:sinon.stub() + copyObject: sinon.stub() + headObject: sinon.stub() @awsS3 = sinon.stub().returns @stubbedS3Client @LocalFileWriter = writeStream: sinon.stub() deleteFile: sinon.stub() + @request = sinon.stub() @requires = "knox": @knox "aws-sdk/clients/s3": @awsS3 @@ -39,15 +41,16 @@ describe "S3PersistorManagerTests", -> "logger-sharelatex": log:-> err:-> + "request": @request "./Errors": @Errors = NotFoundError: sinon.stub() @key = "my/key" @bucketName = "my-bucket" @error = "my errror" + @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires describe "getFileStream", -> beforeEach -> - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires @opts = {} it "should use correct key", (done)-> @@ -74,7 +77,6 @@ describe "S3PersistorManagerTests", -> describe "with supplied auth", -> beforeEach -> - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires @credentials = auth_key: "that_key" auth_secret: "that_secret" @@ -156,10 +158,45 @@ describe "S3PersistorManagerTests", -> @Errors.NotFoundError.called.should.equal false done() + describe "getFileSize", -> + it "should obtain the file size from S3", (done) -> + expectedFileSize = 123 + @stubbedS3Client.headObject.yields(new Error( + "s3Client.headObject got unexpected arguments" + )) + @stubbedS3Client.headObject.withArgs({ + Bucket: @bucketName + Key: @key + }).yields(null, { ContentLength: expectedFileSize }) + + @S3PersistorManager.getFileSize @bucketName, @key, (err, fileSize) => + if err? + return done(err) + expect(fileSize).to.equal(expectedFileSize) + done() + + [403, 404].forEach (statusCode) -> + it "should throw NotFoundError when S3 responds with #{statusCode}", (done) -> + error = new Error() + error.statusCode = statusCode + @stubbedS3Client.headObject.yields(error) + + @S3PersistorManager.getFileSize @bucketName, @key, (err, fileSize) => + expect(err).to.be.an.instanceof(@Errors.NotFoundError) + done() + + it "should rethrow any other error", (done) -> + error = new Error() + @stubbedS3Client.headObject.yields(error) + @stubbedS3Client.headObject.yields(error) + + @S3PersistorManager.getFileSize @bucketName, @key, (err, fileSize) => + expect(err).to.equal(error) + done() + describe "sendFile", -> beforeEach -> - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires @stubbedKnoxClient.putFile.returns on:-> it "should put file with knox", (done)-> @@ -183,7 +220,6 @@ describe "S3PersistorManagerTests", -> @fsPath = "to/some/where" @origin = on:-> - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires @S3PersistorManager.sendFile = sinon.stub().callsArgWith(3) it "should send stream to LocalFileWriter", (done)-> @@ -211,7 +247,6 @@ describe "S3PersistorManagerTests", -> beforeEach -> @sourceKey = "my/key" @destKey = "my/dest/key" - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires it "should use AWS SDK to copy file", (done)-> @stubbedS3Client.copyObject.callsArgWith(1, @error) @@ -229,9 +264,6 @@ describe "S3PersistorManagerTests", -> describe "deleteDirectory", -> - beforeEach -> - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires - it "should list the contents passing them onto multi delete", (done)-> data = Contents: [{Key:"1234"}, {Key: "456"}] @@ -244,9 +276,7 @@ describe "S3PersistorManagerTests", -> describe "deleteFile", -> it "should use correct options", (done)-> - @request = sinon.stub().callsArgWith(1) - @requires["request"] = @request - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @request.callsArgWith(1) @S3PersistorManager.deleteFile @bucketName, @key, (err)=> opts = @request.args[0][0] @@ -257,9 +287,7 @@ describe "S3PersistorManagerTests", -> done() it "should return the error", (done)-> - @request = sinon.stub().callsArgWith(1, @error) - @requires["request"] = @request - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @request.callsArgWith(1, @error) @S3PersistorManager.deleteFile @bucketName, @key, (err)=> err.should.equal @error @@ -268,9 +296,7 @@ describe "S3PersistorManagerTests", -> describe "checkIfFileExists", -> it "should use correct options", (done)-> - @request = sinon.stub().callsArgWith(1, null, statusCode:200) - @requires["request"] = @request - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @request.callsArgWith(1, null, statusCode:200) @S3PersistorManager.checkIfFileExists @bucketName, @key, (err)=> opts = @request.args[0][0] @@ -281,25 +307,21 @@ describe "S3PersistorManagerTests", -> done() it "should return true for a 200", (done)-> - @request = sinon.stub().callsArgWith(1, null, statusCode:200) - @requires["request"] = @request - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @request.callsArgWith(1, null, statusCode:200) + @S3PersistorManager.checkIfFileExists @bucketName, @key, (err, exists)=> exists.should.equal true done() it "should return false for a non 200", (done)-> - @request = sinon.stub().callsArgWith(1, null, statusCode:404) - @requires["request"] = @request - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @request.callsArgWith(1, null, statusCode:404) + @S3PersistorManager.checkIfFileExists @bucketName, @key, (err, exists)=> exists.should.equal false done() it "should return the error", (done)-> - @request = sinon.stub().callsArgWith(1, @error, {}) - @requires["request"] = @request - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @request.callsArgWith(1, @error, {}) @S3PersistorManager.checkIfFileExists @bucketName, @key, (err)=> err.should.equal @error @@ -307,13 +329,10 @@ describe "S3PersistorManagerTests", -> describe "directorySize", -> - beforeEach -> - @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires - it "should sum directory files size", (done) -> data = Contents: [ {Size: 1024}, {Size: 2048} ] @stubbedKnoxClient.list.callsArgWith(1, null, data) @S3PersistorManager.directorySize @bucketName, @key, (err, totalSize)=> totalSize.should.equal 3072 - done() \ No newline at end of file + done()