From 7e1d3d98e701eca3ea6ab507ed22c7c9f93d04bc Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 1 Aug 2017 14:35:55 +0100 Subject: [PATCH 01/17] write files incrementally --- app/coffee/CompileController.coffee | 5 ++++- app/coffee/CompileManager.coffee | 2 +- app/coffee/RequestParser.coffee | 6 +++++ app/coffee/ResourceWriter.coffee | 34 ++++++++++++++++++++++++++++- 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/app/coffee/CompileController.coffee b/app/coffee/CompileController.coffee index 250f9b8..aefa70d 100644 --- a/app/coffee/CompileController.coffee +++ b/app/coffee/CompileController.coffee @@ -15,7 +15,10 @@ module.exports = CompileController = ProjectPersistenceManager.markProjectAsJustAccessed request.project_id, (error) -> return next(error) if error? CompileManager.doCompile request, (error, outputFiles = []) -> - if error?.terminated + if error?.message is "invalid state" + code = 409 # Http 409 Conflict + status = "retry" + else if error?.terminated status = "terminated" else if error?.validate status = "validation-#{error.validate}" diff --git a/app/coffee/CompileManager.coffee b/app/coffee/CompileManager.coffee index ae40bf7..8673b52 100644 --- a/app/coffee/CompileManager.coffee +++ b/app/coffee/CompileManager.coffee @@ -31,7 +31,7 @@ module.exports = CompileManager = timer = new Metrics.Timer("write-to-disk") logger.log project_id: request.project_id, user_id: request.user_id, "syncing resources to disk" - ResourceWriter.syncResourcesToDisk request.project_id, request.resources, compileDir, (error) -> + ResourceWriter.syncResourcesToDisk request, compileDir, (error) -> if error? logger.err err:error, project_id: request.project_id, user_id: request.user_id, "error writing resources to disk" return callback(error) diff --git a/app/coffee/RequestParser.coffee b/app/coffee/RequestParser.coffee index 8fc4ecf..fe22982 100644 --- a/app/coffee/RequestParser.coffee +++ b/app/coffee/RequestParser.coffee @@ -31,6 +31,12 @@ module.exports = RequestParser = response.check = @_parseAttribute "check", compile.options.check, type: "string" + response.incremental = @_parseAttribute "incremental", + compile.options.incremental, + type: "string" + response.state = @_parseAttribute "state", + compile.options.state, + type: "string" if response.timeout > RequestParser.MAX_TIMEOUT response.timeout = RequestParser.MAX_TIMEOUT diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index e2a0e1f..a4ae425 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -11,7 +11,39 @@ settings = require("settings-sharelatex") parallelFileDownloads = settings.parallelFileDownloads or 1 module.exports = ResourceWriter = - syncResourcesToDisk: (project_id, resources, basePath, callback = (error) ->) -> + + syncResourcesToDisk: (request, basePath, callback = (error) ->) -> + if request.incremental? + ResourceWriter.checkState request.incremental, basePath, (error, ok) -> + logger.log state: request.state, result:ok, "checked state on incremental request" + return callback new Error("invalid state") if not ok + ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, callback + else + @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> + return callback(error) if error? + ResourceWriter.storeState request.state, basePath, callback + + storeState: (state, basePath, callback) -> + logger.log state:state, basePath:basePath, "writing state" + fs.writeFile Path.join(basePath, ".resource-state"), state, {encoding: 'ascii'}, callback + + checkState: (state, basePath, callback) -> + fs.readFile Path.join(basePath, ".resource-state"), {encoding:'ascii'}, (err, oldState) -> + logger.log state:state, oldState: oldState, basePath:basePath, err:err, "checking state" + if state is oldState + return callback(null, true) + else + return callback(null, false) + + saveIncrementalResourcesToDisk: (project_id, resources, basePath, callback = (error) ->) -> + @_createDirectory basePath, (error) => + return callback(error) if error? + jobs = for resource in resources + do (resource) => + (callback) => @_writeResourceToDisk(project_id, resource, basePath, callback) + async.parallelLimit jobs, parallelFileDownloads, callback + + saveAllResourcesToDisk: (project_id, resources, basePath, callback = (error) ->) -> @_createDirectory basePath, (error) => return callback(error) if error? @_removeExtraneousFiles resources, basePath, (error) => From 74c26120b2e9d6989759987018328e9dcd4d9a63 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 3 Aug 2017 11:51:58 +0100 Subject: [PATCH 02/17] use syncType and syncState for clsi state options --- app/coffee/RequestParser.coffee | 8 ++++---- app/coffee/ResourceWriter.coffee | 20 ++++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/coffee/RequestParser.coffee b/app/coffee/RequestParser.coffee index fe22982..d2e67bc 100644 --- a/app/coffee/RequestParser.coffee +++ b/app/coffee/RequestParser.coffee @@ -31,11 +31,11 @@ module.exports = RequestParser = response.check = @_parseAttribute "check", compile.options.check, type: "string" - response.incremental = @_parseAttribute "incremental", - compile.options.incremental, + response.syncType = @_parseAttribute "syncType", + compile.options.syncType, type: "string" - response.state = @_parseAttribute "state", - compile.options.state, + response.syncState = @_parseAttribute "syncState", + compile.options.syncState, type: "string" if response.timeout > RequestParser.MAX_TIMEOUT diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index a4ae425..fca2c43 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -13,23 +13,23 @@ parallelFileDownloads = settings.parallelFileDownloads or 1 module.exports = ResourceWriter = syncResourcesToDisk: (request, basePath, callback = (error) ->) -> - if request.incremental? - ResourceWriter.checkState request.incremental, basePath, (error, ok) -> - logger.log state: request.state, result:ok, "checked state on incremental request" + if request.syncType? is "incremental" + ResourceWriter.checkSyncState request.syncState, basePath, (error, ok) -> + logger.log syncState: request.syncState, result:ok, "checked state on incremental request" return callback new Error("invalid state") if not ok ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, callback else @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> return callback(error) if error? - ResourceWriter.storeState request.state, basePath, callback + ResourceWriter.storeSyncState request.syncState, basePath, callback - storeState: (state, basePath, callback) -> - logger.log state:state, basePath:basePath, "writing state" - fs.writeFile Path.join(basePath, ".resource-state"), state, {encoding: 'ascii'}, callback + storeSyncState: (state, basePath, callback) -> + logger.log state:state, basePath:basePath, "writing sync state" + fs.writeFile Path.join(basePath, ".resource-sync-state"), state, {encoding: 'ascii'}, callback - checkState: (state, basePath, callback) -> - fs.readFile Path.join(basePath, ".resource-state"), {encoding:'ascii'}, (err, oldState) -> - logger.log state:state, oldState: oldState, basePath:basePath, err:err, "checking state" + checkSyncState: (state, basePath, callback) -> + fs.readFile Path.join(basePath, ".resource-sync-state"), {encoding:'ascii'}, (err, oldState) -> + logger.log state:state, oldState: oldState, basePath:basePath, err:err, "checking sync state" if state is oldState return callback(null, true) else From 11898b897e4e495d5ce35da66d15fb29eec6d5b1 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 3 Aug 2017 15:56:59 +0100 Subject: [PATCH 03/17] added files out of sync error object --- app/coffee/CompileController.coffee | 3 ++- app/coffee/Errors.coffee | 7 +++++++ app/coffee/ResourceWriter.coffee | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/coffee/CompileController.coffee b/app/coffee/CompileController.coffee index aefa70d..6f93bfe 100644 --- a/app/coffee/CompileController.coffee +++ b/app/coffee/CompileController.coffee @@ -4,6 +4,7 @@ Settings = require "settings-sharelatex" Metrics = require "./Metrics" ProjectPersistenceManager = require "./ProjectPersistenceManager" logger = require "logger-sharelatex" +Errors = require "./Errors" module.exports = CompileController = compile: (req, res, next = (error) ->) -> @@ -15,7 +16,7 @@ module.exports = CompileController = ProjectPersistenceManager.markProjectAsJustAccessed request.project_id, (error) -> return next(error) if error? CompileManager.doCompile request, (error, outputFiles = []) -> - if error?.message is "invalid state" + if error instanceOf Errors.FilesOutOfSyncError code = 409 # Http 409 Conflict status = "retry" else if error?.terminated diff --git a/app/coffee/Errors.coffee b/app/coffee/Errors.coffee index 4abea0b..2e3ae75 100644 --- a/app/coffee/Errors.coffee +++ b/app/coffee/Errors.coffee @@ -5,6 +5,13 @@ NotFoundError = (message) -> return error NotFoundError.prototype.__proto__ = Error.prototype +FilesOutOfSyncError = (message) -> + error = new Error(message) + error.name = "FilesOutOfSyncError" + error.__proto__ = FilesOutOfSyncError.prototype + return error +FilesOutOfSyncError.prototype.__proto__ = Error.prototype module.exports = Errors = NotFoundError: NotFoundError + FilesOutOfSyncError: FilesOutOfSyncError diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index fca2c43..725d069 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -5,6 +5,7 @@ async = require "async" mkdirp = require "mkdirp" OutputFileFinder = require "./OutputFileFinder" Metrics = require "./Metrics" +Errors = require "./Errors" logger = require "logger-sharelatex" settings = require("settings-sharelatex") @@ -16,7 +17,7 @@ module.exports = ResourceWriter = if request.syncType? is "incremental" ResourceWriter.checkSyncState request.syncState, basePath, (error, ok) -> logger.log syncState: request.syncState, result:ok, "checked state on incremental request" - return callback new Error("invalid state") if not ok + return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") if not ok ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, callback else @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> From b4be40d0619823c455955a25f5eb581e16f3df7a Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 7 Aug 2017 10:19:56 +0100 Subject: [PATCH 04/17] restrict syncType values to full/incremental --- app/coffee/RequestParser.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/app/coffee/RequestParser.coffee b/app/coffee/RequestParser.coffee index d2e67bc..8b8de76 100644 --- a/app/coffee/RequestParser.coffee +++ b/app/coffee/RequestParser.coffee @@ -33,6 +33,7 @@ module.exports = RequestParser = type: "string" response.syncType = @_parseAttribute "syncType", compile.options.syncType, + validValues: ["full", "incremental"] type: "string" response.syncState = @_parseAttribute "syncState", compile.options.syncState, From 6542ce20b6b91b98acf5e017d533e8696d337830 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 7 Aug 2017 14:26:13 +0100 Subject: [PATCH 05/17] fix incremental request --- app/coffee/CompileController.coffee | 2 +- app/coffee/CompileManager.coffee | 7 +++++-- app/coffee/ResourceWriter.coffee | 15 ++++++--------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/coffee/CompileController.coffee b/app/coffee/CompileController.coffee index 6f93bfe..f4dcb0e 100644 --- a/app/coffee/CompileController.coffee +++ b/app/coffee/CompileController.coffee @@ -16,7 +16,7 @@ module.exports = CompileController = ProjectPersistenceManager.markProjectAsJustAccessed request.project_id, (error) -> return next(error) if error? CompileManager.doCompile request, (error, outputFiles = []) -> - if error instanceOf Errors.FilesOutOfSyncError + if error instanceof Errors.FilesOutOfSyncError code = 409 # Http 409 Conflict status = "retry" else if error?.terminated diff --git a/app/coffee/CompileManager.coffee b/app/coffee/CompileManager.coffee index 8673b52..2b56330 100644 --- a/app/coffee/CompileManager.coffee +++ b/app/coffee/CompileManager.coffee @@ -32,9 +32,12 @@ module.exports = CompileManager = timer = new Metrics.Timer("write-to-disk") logger.log project_id: request.project_id, user_id: request.user_id, "syncing resources to disk" ResourceWriter.syncResourcesToDisk request, compileDir, (error) -> - if error? + if error? and error instanceof Errors.FilesOutOfSyncError + logger.warn project_id: request.project_id, user_id: request.user_id, "files out of sync, please retry" + return callback(error) + else if error? logger.err err:error, project_id: request.project_id, user_id: request.user_id, "error writing resources to disk" - return callback(error) + return callback(error) logger.log project_id: request.project_id, user_id: request.user_id, time_taken: Date.now() - timer.start, "written files to disk" timer.done() diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index 725d069..29e0e57 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -14,10 +14,10 @@ parallelFileDownloads = settings.parallelFileDownloads or 1 module.exports = ResourceWriter = syncResourcesToDisk: (request, basePath, callback = (error) ->) -> - if request.syncType? is "incremental" - ResourceWriter.checkSyncState request.syncState, basePath, (error, ok) -> - logger.log syncState: request.syncState, result:ok, "checked state on incremental request" - return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") if not ok + if request.syncType is "incremental" + ResourceWriter.checkSyncState request.syncState, basePath, (error, syncStateOk) -> + logger.log syncState: request.syncState, result:syncStateOk, "checked state on incremental request" + return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") if not syncStateOk ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, callback else @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> @@ -30,11 +30,8 @@ module.exports = ResourceWriter = checkSyncState: (state, basePath, callback) -> fs.readFile Path.join(basePath, ".resource-sync-state"), {encoding:'ascii'}, (err, oldState) -> - logger.log state:state, oldState: oldState, basePath:basePath, err:err, "checking sync state" - if state is oldState - return callback(null, true) - else - return callback(null, false) + # ignore errors, return true if state matches, false otherwise (including errors) + return callback(null, if state is oldState then true else false) saveIncrementalResourcesToDisk: (project_id, resources, basePath, callback = (error) ->) -> @_createDirectory basePath, (error) => From 206adc2d048fe0985782af55414e07bb8dc71ad1 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 7 Aug 2017 15:00:16 +0100 Subject: [PATCH 06/17] fix broken unit tests --- test/unit/coffee/CompileManagerTests.coffee | 4 ++-- test/unit/coffee/ResourceWriterTests.coffee | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/test/unit/coffee/CompileManagerTests.coffee b/test/unit/coffee/CompileManagerTests.coffee index de33166..25109b6 100644 --- a/test/unit/coffee/CompileManagerTests.coffee +++ b/test/unit/coffee/CompileManagerTests.coffee @@ -51,7 +51,7 @@ describe "CompileManager", -> @env = {} @Settings.compileDir = "compiles" @compileDir = "#{@Settings.path.compilesDir}/#{@project_id}-#{@user_id}" - @ResourceWriter.syncResourcesToDisk = sinon.stub().callsArg(3) + @ResourceWriter.syncResourcesToDisk = sinon.stub().callsArg(2) @LatexRunner.runLatex = sinon.stub().callsArg(2) @OutputFileFinder.findOutputFiles = sinon.stub().callsArgWith(2, null, @output_files) @OutputCacheManager.saveOutputFiles = sinon.stub().callsArgWith(2, null, @build_files) @@ -64,7 +64,7 @@ describe "CompileManager", -> it "should write the resources to disk", -> @ResourceWriter.syncResourcesToDisk - .calledWith(@project_id, @resources, @compileDir) + .calledWith(@request, @compileDir) .should.equal true it "should run LaTeX", -> diff --git a/test/unit/coffee/ResourceWriterTests.coffee b/test/unit/coffee/ResourceWriterTests.coffee index fd1ae30..efaf36a 100644 --- a/test/unit/coffee/ResourceWriterTests.coffee +++ b/test/unit/coffee/ResourceWriterTests.coffee @@ -29,7 +29,9 @@ describe "ResourceWriter", -> ] @ResourceWriter._writeResourceToDisk = sinon.stub().callsArg(3) @ResourceWriter._removeExtraneousFiles = sinon.stub().callsArg(2) - @ResourceWriter.syncResourcesToDisk(@project_id, @resources, @basePath, @callback) + @ResourceWriter.checkSyncState = sinon.stub().callsArg(2) + @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) + @ResourceWriter.syncResourcesToDisk({project_id: @project_id, resources: @resources}, @basePath, @callback) it "should remove old files", -> @ResourceWriter._removeExtraneousFiles From 86fa940c97deb7e4cbce5e1673de59828167beab Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 8 Aug 2017 16:27:53 +0100 Subject: [PATCH 07/17] clean up the state file if no state passed in --- app/coffee/ResourceWriter.coffee | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index 29e0e57..ad14b91 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -25,11 +25,21 @@ module.exports = ResourceWriter = ResourceWriter.storeSyncState request.syncState, basePath, callback storeSyncState: (state, basePath, callback) -> - logger.log state:state, basePath:basePath, "writing sync state" - fs.writeFile Path.join(basePath, ".resource-sync-state"), state, {encoding: 'ascii'}, callback + stateFile = Path.join(basePath, ".resource-sync-state") + if not state? # remove the file if no state passed in + logger.log state:state, basePath:basePath, "clearing sync state" + fs.unlink stateFile, (err) -> + if err? and err.code isnt 'ENOENT' + return callback(err) + else + return callback() + else + logger.log state:state, basePath:basePath, "writing sync state" + fs.writeFile stateFile, state, {encoding: 'ascii'}, callback checkSyncState: (state, basePath, callback) -> - fs.readFile Path.join(basePath, ".resource-sync-state"), {encoding:'ascii'}, (err, oldState) -> + stateFile = Path.join(basePath, ".resource-sync-state") + fs.readFile stateFile, {encoding:'ascii'}, (err, oldState) -> # ignore errors, return true if state matches, false otherwise (including errors) return callback(null, if state is oldState then true else false) From c25e96bbc3f62cf3c8836ad28fbfc5509e8cbe5f Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 9 Aug 2017 15:10:24 +0100 Subject: [PATCH 08/17] add comment about syncType/syncState --- app/coffee/RequestParser.coffee | 14 ++++++++++++++ app/coffee/ResourceWriter.coffee | 26 ++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/app/coffee/RequestParser.coffee b/app/coffee/RequestParser.coffee index 8b8de76..596b529 100644 --- a/app/coffee/RequestParser.coffee +++ b/app/coffee/RequestParser.coffee @@ -31,10 +31,24 @@ module.exports = RequestParser = response.check = @_parseAttribute "check", compile.options.check, type: "string" + + # The syncType specifies whether the request contains all + # resources (full) or only those resources to be updated + # in-place (incremental). response.syncType = @_parseAttribute "syncType", compile.options.syncType, validValues: ["full", "incremental"] type: "string" + + # The syncState is an identifier passed in with the request + # which has the property that it changes when any resource is + # added, deleted, moved or renamed. + # + # on syncType full the syncState identifier is passed in and + # stored + # + # on syncType incremental the syncState identifier must match + # the stored value response.syncState = @_parseAttribute "syncState", compile.options.syncState, type: "string" diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index ad14b91..9a78671 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -24,8 +24,23 @@ module.exports = ResourceWriter = return callback(error) if error? ResourceWriter.storeSyncState request.syncState, basePath, callback + # The sync state is an identifier which must match for an + # incremental update to be allowed. + # + # The initial value is passed in and stored on a full + # compile. + # + # Subsequent incremental compiles must come with the same value - if + # not they will be rejected with a 409 Conflict response. + # + # An incremental compile can only update existing files with new + # content. The sync state identifier must change if any docs or + # files are moved, added, deleted or renamed. + + SYNC_STATE_FILE: ".project-sync-state" + storeSyncState: (state, basePath, callback) -> - stateFile = Path.join(basePath, ".resource-sync-state") + stateFile = Path.join(basePath, @SYNC_STATE_FILE) if not state? # remove the file if no state passed in logger.log state:state, basePath:basePath, "clearing sync state" fs.unlink stateFile, (err) -> @@ -38,10 +53,13 @@ module.exports = ResourceWriter = fs.writeFile stateFile, state, {encoding: 'ascii'}, callback checkSyncState: (state, basePath, callback) -> - stateFile = Path.join(basePath, ".resource-sync-state") + stateFile = Path.join(basePath, @SYNC_STATE_FILE) fs.readFile stateFile, {encoding:'ascii'}, (err, oldState) -> - # ignore errors, return true if state matches, false otherwise (including errors) - return callback(null, if state is oldState then true else false) + if err? and err.code isnt 'ENOENT' + return callback(err) + else + # return true if state matches, false otherwise (including file not existing) + callback(null, if state is oldState then true else false) saveIncrementalResourcesToDisk: (project_id, resources, basePath, callback = (error) ->) -> @_createDirectory basePath, (error) => From 00ddfdf42bce5807f614bdd55eb0877ebd48ac55 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 9 Aug 2017 15:22:44 +0100 Subject: [PATCH 09/17] fix unit tests --- test/unit/coffee/ResourceWriterTests.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/coffee/ResourceWriterTests.coffee b/test/unit/coffee/ResourceWriterTests.coffee index efaf36a..d4dd9c1 100644 --- a/test/unit/coffee/ResourceWriterTests.coffee +++ b/test/unit/coffee/ResourceWriterTests.coffee @@ -7,7 +7,9 @@ path = require "path" describe "ResourceWriter", -> beforeEach -> @ResourceWriter = SandboxedModule.require modulePath, requires: - "fs": @fs = { mkdir: sinon.stub().callsArg(1) } + "fs": @fs = + mkdir: sinon.stub().callsArg(1) + unlink: sinon.stub().callsArg(1) "wrench": @wrench = {} "./UrlCache" : @UrlCache = {} "mkdirp" : @mkdirp = sinon.stub().callsArg(1) From 2b610030d54b9be2f1302def001e70464e7492c7 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 17 Aug 2017 14:53:35 +0100 Subject: [PATCH 10/17] store the resource list in a file --- app/coffee/CompileManager.coffee | 13 ++++++----- app/coffee/ResourceListManager.coffee | 24 +++++++++++++++++++++ app/coffee/ResourceWriter.coffee | 13 ++++++++--- test/unit/coffee/CompileManagerTests.coffee | 2 +- test/unit/coffee/ResourceWriterTests.coffee | 3 +++ 5 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 app/coffee/ResourceListManager.coffee diff --git a/app/coffee/CompileManager.coffee b/app/coffee/CompileManager.coffee index 2b56330..d6c79da 100644 --- a/app/coffee/CompileManager.coffee +++ b/app/coffee/CompileManager.coffee @@ -31,7 +31,8 @@ module.exports = CompileManager = timer = new Metrics.Timer("write-to-disk") logger.log project_id: request.project_id, user_id: request.user_id, "syncing resources to disk" - ResourceWriter.syncResourcesToDisk request, compileDir, (error) -> + ResourceWriter.syncResourcesToDisk request, compileDir, (error, resourceList) -> + # NOTE: resourceList is insecure, it should only be used to exclude files from the output list if error? and error instanceof Errors.FilesOutOfSyncError logger.warn project_id: request.project_id, user_id: request.user_id, "files out of sync, please retry" return callback(error) @@ -40,15 +41,17 @@ module.exports = CompileManager = return callback(error) logger.log project_id: request.project_id, user_id: request.user_id, time_taken: Date.now() - timer.start, "written files to disk" timer.done() - + + # FIXME - for incremental compiles we don't want to inject this multiple times injectDraftModeIfRequired = (callback) -> if request.draft DraftModeManager.injectDraftMode Path.join(compileDir, request.rootResourcePath), callback else callback() + # FIXME - for incremental compiles we may need to update output.tex every time createTikzFileIfRequired = (callback) -> - if TikzManager.needsOutputFile(request.rootResourcePath, request.resources) + if TikzManager.needsOutputFile(request.rootResourcePath, resourceList) TikzManager.injectOutputFile compileDir, request.rootResourcePath, callback else callback() @@ -94,7 +97,7 @@ module.exports = CompileManager = error.validate = "fail" # compile was killed by user, was a validation, or a compile which failed validation if error?.terminated or error?.validate - OutputFileFinder.findOutputFiles request.resources, compileDir, (err, outputFiles) -> + OutputFileFinder.findOutputFiles resourceList, compileDir, (err, outputFiles) -> return callback(err) if err? callback(error, outputFiles) # return output files so user can check logs return @@ -114,7 +117,7 @@ module.exports = CompileManager = if stats?["latex-runs"] > 0 and timings?["cpu-time"] > 0 Metrics.timing("run-compile-cpu-time-per-pass", timings["cpu-time"] / stats["latex-runs"]) - OutputFileFinder.findOutputFiles request.resources, compileDir, (error, outputFiles) -> + OutputFileFinder.findOutputFiles resourceList, compileDir, (error, outputFiles) -> return callback(error) if error? OutputCacheManager.saveOutputFiles outputFiles, compileDir, (error, newOutputFiles) -> callback null, newOutputFiles diff --git a/app/coffee/ResourceListManager.coffee b/app/coffee/ResourceListManager.coffee new file mode 100644 index 0000000..d68f5d1 --- /dev/null +++ b/app/coffee/ResourceListManager.coffee @@ -0,0 +1,24 @@ +Path = require "path" +fs = require "fs" +mkdirp = require "mkdirp" +logger = require "logger-sharelatex" +settings = require("settings-sharelatex") + +module.exports = ResourceListManager = + + # This file is a list of the input files for the project, one per + # line, used to identify output files (i.e. files not on this list) + # when the incoming request is incremental. + RESOURCE_LIST_FILE: ".project-resource-list" + + saveResourceList: (resources, basePath, callback = (error) ->) -> + resourceListFile = Path.join(basePath, @RESOURCE_LIST_FILE) + resourceList = (resource.path for resource in resources) + fs.writeFile resourceListFile, resourceList.join("\n"), callback + + loadResourceList: (basePath, callback = (error) ->) -> + resourceListFile = Path.join(basePath, @RESOURCE_LIST_FILE) + fs.readFile resourceListFile, (err, resourceList) -> + return callback(err) if err? + resources = ({path: path} for path in resourceList?.toString()?.split("\n") or []) + callback(null, resources) diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index 9a78671..18b8122 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -4,6 +4,7 @@ fs = require "fs" async = require "async" mkdirp = require "mkdirp" OutputFileFinder = require "./OutputFileFinder" +ResourceListManager = require "./ResourceListManager" Metrics = require "./Metrics" Errors = require "./Errors" logger = require "logger-sharelatex" @@ -13,16 +14,22 @@ parallelFileDownloads = settings.parallelFileDownloads or 1 module.exports = ResourceWriter = - syncResourcesToDisk: (request, basePath, callback = (error) ->) -> + syncResourcesToDisk: (request, basePath, callback = (error, resourceList) ->) -> if request.syncType is "incremental" ResourceWriter.checkSyncState request.syncState, basePath, (error, syncStateOk) -> logger.log syncState: request.syncState, result:syncStateOk, "checked state on incremental request" return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") if not syncStateOk - ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, callback + ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, (error) -> + return callback(error) if error? + ResourceListManager.loadResourceList basePath, callback else @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> return callback(error) if error? - ResourceWriter.storeSyncState request.syncState, basePath, callback + ResourceWriter.storeSyncState request.syncState, basePath, (error) -> + return callback(error) if error? + ResourceListManager.saveResourceList request.resources, basePath, (error) => + return callback(error) if error? + callback(null, request.resources) # The sync state is an identifier which must match for an # incremental update to be allowed. diff --git a/test/unit/coffee/CompileManagerTests.coffee b/test/unit/coffee/CompileManagerTests.coffee index 25109b6..ff671b2 100644 --- a/test/unit/coffee/CompileManagerTests.coffee +++ b/test/unit/coffee/CompileManagerTests.coffee @@ -51,7 +51,7 @@ describe "CompileManager", -> @env = {} @Settings.compileDir = "compiles" @compileDir = "#{@Settings.path.compilesDir}/#{@project_id}-#{@user_id}" - @ResourceWriter.syncResourcesToDisk = sinon.stub().callsArg(2) + @ResourceWriter.syncResourcesToDisk = sinon.stub().callsArgWith(2, null, @resources) @LatexRunner.runLatex = sinon.stub().callsArg(2) @OutputFileFinder.findOutputFiles = sinon.stub().callsArgWith(2, null, @output_files) @OutputCacheManager.saveOutputFiles = sinon.stub().callsArgWith(2, null, @build_files) diff --git a/test/unit/coffee/ResourceWriterTests.coffee b/test/unit/coffee/ResourceWriterTests.coffee index d4dd9c1..0e60250 100644 --- a/test/unit/coffee/ResourceWriterTests.coffee +++ b/test/unit/coffee/ResourceWriterTests.coffee @@ -10,6 +10,7 @@ describe "ResourceWriter", -> "fs": @fs = mkdir: sinon.stub().callsArg(1) unlink: sinon.stub().callsArg(1) + "./ResourceListManager": @ResourceListManager = {} "wrench": @wrench = {} "./UrlCache" : @UrlCache = {} "mkdirp" : @mkdirp = sinon.stub().callsArg(1) @@ -33,6 +34,8 @@ describe "ResourceWriter", -> @ResourceWriter._removeExtraneousFiles = sinon.stub().callsArg(2) @ResourceWriter.checkSyncState = sinon.stub().callsArg(2) @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) + @ResourceListManager.saveResourceList = sinon.stub().callsArg(2) + @ResourceListManager.loadResourceList = sinon.stub().callsArg(1) @ResourceWriter.syncResourcesToDisk({project_id: @project_id, resources: @resources}, @basePath, @callback) it "should remove old files", -> From 5b5f7b06905aea03541ba4814d3038df26520604 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 17 Aug 2017 15:03:37 +0100 Subject: [PATCH 11/17] avoid adding draft mode more than once --- app/coffee/CompileManager.coffee | 2 -- app/coffee/DraftModeManager.coffee | 5 ++++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/coffee/CompileManager.coffee b/app/coffee/CompileManager.coffee index d6c79da..56d04db 100644 --- a/app/coffee/CompileManager.coffee +++ b/app/coffee/CompileManager.coffee @@ -42,14 +42,12 @@ module.exports = CompileManager = logger.log project_id: request.project_id, user_id: request.user_id, time_taken: Date.now() - timer.start, "written files to disk" timer.done() - # FIXME - for incremental compiles we don't want to inject this multiple times injectDraftModeIfRequired = (callback) -> if request.draft DraftModeManager.injectDraftMode Path.join(compileDir, request.rootResourcePath), callback else callback() - # FIXME - for incremental compiles we may need to update output.tex every time createTikzFileIfRequired = (callback) -> if TikzManager.needsOutputFile(request.rootResourcePath, resourceList) TikzManager.injectOutputFile compileDir, request.rootResourcePath, callback diff --git a/app/coffee/DraftModeManager.coffee b/app/coffee/DraftModeManager.coffee index 253cbff..2f9e931 100644 --- a/app/coffee/DraftModeManager.coffee +++ b/app/coffee/DraftModeManager.coffee @@ -5,6 +5,9 @@ module.exports = DraftModeManager = injectDraftMode: (filename, callback = (error) ->) -> fs.readFile filename, "utf8", (error, content) -> return callback(error) if error? + # avoid adding draft mode more than once + if content?.indexOf("\\documentclass\[draft") >= 0 + return callback() modified_content = DraftModeManager._injectDraftOption content logger.log { content: content.slice(0,1024), # \documentclass is normally v near the top @@ -18,4 +21,4 @@ module.exports = DraftModeManager = # With existing options (must be first, otherwise both are applied) .replace(/\\documentclass\[/g, "\\documentclass[draft,") # Without existing options - .replace(/\\documentclass\{/g, "\\documentclass[draft]{") \ No newline at end of file + .replace(/\\documentclass\{/g, "\\documentclass[draft]{") From a8aaf58e64560bc1d0fbf8e246fa4fe1ed9690ea Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 17 Aug 2017 15:57:05 +0100 Subject: [PATCH 12/17] test syncType in RequestParser --- test/unit/coffee/RequestParserTests.coffee | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unit/coffee/RequestParserTests.coffee b/test/unit/coffee/RequestParserTests.coffee index 1cd931b..0b420b3 100644 --- a/test/unit/coffee/RequestParserTests.coffee +++ b/test/unit/coffee/RequestParserTests.coffee @@ -242,3 +242,13 @@ describe "RequestParser", -> it "should return an error", -> @callback.calledWith("relative path in root resource") .should.equal true + + describe "with an unknown syncType", -> + beforeEach -> + @validRequest.compile.options.syncType = "unexpected" + @RequestParser.parse @validRequest, @callback + @data = @callback.args[0][1] + + it "should return an error", -> + @callback.calledWith("syncType attribute should be one of: full, incremental") + .should.equal true From e4aad90f338becac7d8b3e7452a2679e654c061e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 17 Aug 2017 16:59:37 +0100 Subject: [PATCH 13/17] ResourceWriter unit tests (wip) --- app/coffee/ResourceWriter.coffee | 11 +++- test/unit/coffee/ResourceWriterTests.coffee | 56 ++++++++++++++++++++- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index 18b8122..d0d1988 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -19,9 +19,14 @@ module.exports = ResourceWriter = ResourceWriter.checkSyncState request.syncState, basePath, (error, syncStateOk) -> logger.log syncState: request.syncState, result:syncStateOk, "checked state on incremental request" return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") if not syncStateOk - ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, (error) -> + ResourceListManager.loadResourceList basePath, (error, resourceList) -> return callback(error) if error? - ResourceListManager.loadResourceList basePath, callback + ResourceWriter._removeExtraneousFiles resourceList, basePath, (error) => + return callback(error) if error? + ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, (error) -> + return callback(error) if error? + callback(null, resourceList) + else @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> return callback(error) if error? @@ -113,6 +118,8 @@ module.exports = ResourceWriter = should_delete = true if path.match(/^output\./) or path.match(/\.aux$/) or path.match(/^cache\//) # knitr cache should_delete = false + if path in ['.project-resource-list', '.project-sync-state'] + should_delete = false if path == "output.pdf" or path == "output.dvi" or path == "output.log" or path == "output.xdv" should_delete = true if path == "output.tex" # created by TikzManager if present in output files diff --git a/test/unit/coffee/ResourceWriterTests.coffee b/test/unit/coffee/ResourceWriterTests.coffee index 0e60250..32ffb13 100644 --- a/test/unit/coffee/ResourceWriterTests.coffee +++ b/test/unit/coffee/ResourceWriterTests.coffee @@ -23,7 +23,7 @@ describe "ResourceWriter", -> @basePath = "/path/to/write/files/to" @callback = sinon.stub() - describe "syncResourcesToDisk", -> + describe "syncResourcesToDisk on a full request", -> beforeEach -> @resources = [ "resource-1-mock" @@ -36,7 +36,59 @@ describe "ResourceWriter", -> @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) @ResourceListManager.saveResourceList = sinon.stub().callsArg(2) @ResourceListManager.loadResourceList = sinon.stub().callsArg(1) - @ResourceWriter.syncResourcesToDisk({project_id: @project_id, resources: @resources}, @basePath, @callback) + @ResourceWriter.syncResourcesToDisk({ + project_id: @project_id + syncState: @syncState = "0123456789abcdef" + resources: @resources + }, @basePath, @callback) + + it "should remove old files", -> + @ResourceWriter._removeExtraneousFiles + .calledWith(@resources, @basePath) + .should.equal true + + it "should write each resource to disk", -> + for resource in @resources + @ResourceWriter._writeResourceToDisk + .calledWith(@project_id, resource, @basePath) + .should.equal true + + it "should store the sync state", -> + console.log "CHECKING", @syncState, @basePath + @ResourceWriter.storeSyncState + .calledWith(@syncState, @basePath) + .should.equal true + + it "should save the resource list", -> + @ResourceListManager.saveResourceList + .calledWith(@resources, @basePath) + .should.equal true + + it "should call the callback", -> + @callback.called.should.equal true + + describe "syncResourcesToDisk on an incremental update", -> + beforeEach -> + @resources = [ + "resource-1-mock" + ] + @ResourceWriter._writeResourceToDisk = sinon.stub().callsArg(3) + @ResourceWriter._removeExtraneousFiles = sinon.stub().callsArg(2) + @ResourceWriter.checkSyncState = sinon.stub().callsArgWith(2, null, true) + @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) + @ResourceListManager.saveResourceList = sinon.stub().callsArg(2) + @ResourceListManager.loadResourceList = sinon.stub().callsArg(1) + @ResourceWriter.syncResourcesToDisk({ + project_id: @project_id, + syncType: "incremental", + syncState: @syncState = "1234567890abcdef", + resources: @resources + }, @basePath, @callback) + + it "should check the sync state matches", -> + @ResourceWriter.checkSyncState + .calledWith(@syncState, @basePath) + .should.equal true it "should remove old files", -> @ResourceWriter._removeExtraneousFiles From e8064f12a12e5ab2844520db45cccdb785895dfe Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 18 Aug 2017 09:41:43 +0100 Subject: [PATCH 14/17] finish unit test for incremental update --- test/unit/coffee/ResourceWriterTests.coffee | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/unit/coffee/ResourceWriterTests.coffee b/test/unit/coffee/ResourceWriterTests.coffee index 32ffb13..9a17b05 100644 --- a/test/unit/coffee/ResourceWriterTests.coffee +++ b/test/unit/coffee/ResourceWriterTests.coffee @@ -54,7 +54,6 @@ describe "ResourceWriter", -> .should.equal true it "should store the sync state", -> - console.log "CHECKING", @syncState, @basePath @ResourceWriter.storeSyncState .calledWith(@syncState, @basePath) .should.equal true @@ -77,7 +76,7 @@ describe "ResourceWriter", -> @ResourceWriter.checkSyncState = sinon.stub().callsArgWith(2, null, true) @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) @ResourceListManager.saveResourceList = sinon.stub().callsArg(2) - @ResourceListManager.loadResourceList = sinon.stub().callsArg(1) + @ResourceListManager.loadResourceList = sinon.stub().callsArgWith(1, null, @resources) @ResourceWriter.syncResourcesToDisk({ project_id: @project_id, syncType: "incremental", From 0b9ddb8efe323e90bc336631b15a3850b3503375 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 18 Aug 2017 09:41:59 +0100 Subject: [PATCH 15/17] fix whitespace --- app/coffee/ResourceWriter.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index d0d1988..fae570f 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -53,7 +53,7 @@ module.exports = ResourceWriter = storeSyncState: (state, basePath, callback) -> stateFile = Path.join(basePath, @SYNC_STATE_FILE) - if not state? # remove the file if no state passed in + if not state? # remove the file if no state passed in logger.log state:state, basePath:basePath, "clearing sync state" fs.unlink stateFile, (err) -> if err? and err.code isnt 'ENOENT' From 6921cf25b896d5587367a8a0009b657ab684d693 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 18 Aug 2017 10:22:17 +0100 Subject: [PATCH 16/17] splice state management into ResourceStateManager --- app/coffee/ResourceStateManager.coffee | 46 ++++++++++++++++++++ app/coffee/ResourceWriter.coffee | 47 ++------------------- test/unit/coffee/ResourceWriterTests.coffee | 35 ++++++++++++--- 3 files changed, 79 insertions(+), 49 deletions(-) create mode 100644 app/coffee/ResourceStateManager.coffee diff --git a/app/coffee/ResourceStateManager.coffee b/app/coffee/ResourceStateManager.coffee new file mode 100644 index 0000000..2a6d177 --- /dev/null +++ b/app/coffee/ResourceStateManager.coffee @@ -0,0 +1,46 @@ +Path = require "path" +fs = require "fs" +mkdirp = require "mkdirp" +logger = require "logger-sharelatex" +settings = require("settings-sharelatex") +Errors = require "./Errors" + +module.exports = ResourceStateManager = + + # The sync state is an identifier which must match for an + # incremental update to be allowed. + # + # The initial value is passed in and stored on a full + # compile. + # + # Subsequent incremental compiles must come with the same value - if + # not they will be rejected with a 409 Conflict response. + # + # An incremental compile can only update existing files with new + # content. The sync state identifier must change if any docs or + # files are moved, added, deleted or renamed. + + SYNC_STATE_FILE: ".project-sync-state" + + saveProjectStateHash: (state, basePath, callback) -> + stateFile = Path.join(basePath, @SYNC_STATE_FILE) + if not state? # remove the file if no state passed in + logger.log state:state, basePath:basePath, "clearing sync state" + fs.unlink stateFile, (err) -> + if err? and err.code isnt 'ENOENT' + return callback(err) + else + return callback() + else + logger.log state:state, basePath:basePath, "writing sync state" + fs.writeFile stateFile, state, {encoding: 'ascii'}, callback + + checkProjectStateHashMatches: (state, basePath, callback) -> + stateFile = Path.join(basePath, @SYNC_STATE_FILE) + fs.readFile stateFile, {encoding:'ascii'}, (err, oldState) -> + if err? and err.code isnt 'ENOENT' + return callback(err) + else if state isnt oldState + return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") + else if state is oldState + callback(null) diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index fae570f..7f67026 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -4,9 +4,9 @@ fs = require "fs" async = require "async" mkdirp = require "mkdirp" OutputFileFinder = require "./OutputFileFinder" +ResourceStateManager = require "./ResourceStateManager" ResourceListManager = require "./ResourceListManager" Metrics = require "./Metrics" -Errors = require "./Errors" logger = require "logger-sharelatex" settings = require("settings-sharelatex") @@ -16,9 +16,8 @@ module.exports = ResourceWriter = syncResourcesToDisk: (request, basePath, callback = (error, resourceList) ->) -> if request.syncType is "incremental" - ResourceWriter.checkSyncState request.syncState, basePath, (error, syncStateOk) -> - logger.log syncState: request.syncState, result:syncStateOk, "checked state on incremental request" - return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") if not syncStateOk + ResourceStateManager.checkProjectStateHashMatches request.syncState, basePath, (error) -> + return callback(error) if error? ResourceListManager.loadResourceList basePath, (error, resourceList) -> return callback(error) if error? ResourceWriter._removeExtraneousFiles resourceList, basePath, (error) => @@ -26,53 +25,15 @@ module.exports = ResourceWriter = ResourceWriter.saveIncrementalResourcesToDisk request.project_id, request.resources, basePath, (error) -> return callback(error) if error? callback(null, resourceList) - else @saveAllResourcesToDisk request.project_id, request.resources, basePath, (error) -> return callback(error) if error? - ResourceWriter.storeSyncState request.syncState, basePath, (error) -> + ResourceStateManager.saveProjectStateHash request.syncState, basePath, (error) -> return callback(error) if error? ResourceListManager.saveResourceList request.resources, basePath, (error) => return callback(error) if error? callback(null, request.resources) - # The sync state is an identifier which must match for an - # incremental update to be allowed. - # - # The initial value is passed in and stored on a full - # compile. - # - # Subsequent incremental compiles must come with the same value - if - # not they will be rejected with a 409 Conflict response. - # - # An incremental compile can only update existing files with new - # content. The sync state identifier must change if any docs or - # files are moved, added, deleted or renamed. - - SYNC_STATE_FILE: ".project-sync-state" - - storeSyncState: (state, basePath, callback) -> - stateFile = Path.join(basePath, @SYNC_STATE_FILE) - if not state? # remove the file if no state passed in - logger.log state:state, basePath:basePath, "clearing sync state" - fs.unlink stateFile, (err) -> - if err? and err.code isnt 'ENOENT' - return callback(err) - else - return callback() - else - logger.log state:state, basePath:basePath, "writing sync state" - fs.writeFile stateFile, state, {encoding: 'ascii'}, callback - - checkSyncState: (state, basePath, callback) -> - stateFile = Path.join(basePath, @SYNC_STATE_FILE) - fs.readFile stateFile, {encoding:'ascii'}, (err, oldState) -> - if err? and err.code isnt 'ENOENT' - return callback(err) - else - # return true if state matches, false otherwise (including file not existing) - callback(null, if state is oldState then true else false) - saveIncrementalResourcesToDisk: (project_id, resources, basePath, callback = (error) ->) -> @_createDirectory basePath, (error) => return callback(error) if error? diff --git a/test/unit/coffee/ResourceWriterTests.coffee b/test/unit/coffee/ResourceWriterTests.coffee index 9a17b05..0804438 100644 --- a/test/unit/coffee/ResourceWriterTests.coffee +++ b/test/unit/coffee/ResourceWriterTests.coffee @@ -11,6 +11,7 @@ describe "ResourceWriter", -> mkdir: sinon.stub().callsArg(1) unlink: sinon.stub().callsArg(1) "./ResourceListManager": @ResourceListManager = {} + "./ResourceStateManager": @ResourceStateManager = {} "wrench": @wrench = {} "./UrlCache" : @UrlCache = {} "mkdirp" : @mkdirp = sinon.stub().callsArg(1) @@ -32,8 +33,8 @@ describe "ResourceWriter", -> ] @ResourceWriter._writeResourceToDisk = sinon.stub().callsArg(3) @ResourceWriter._removeExtraneousFiles = sinon.stub().callsArg(2) - @ResourceWriter.checkSyncState = sinon.stub().callsArg(2) - @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) + @ResourceStateManager.checkProjectStateHashMatches = sinon.stub().callsArg(2) + @ResourceStateManager.saveProjectStateHash = sinon.stub().callsArg(2) @ResourceListManager.saveResourceList = sinon.stub().callsArg(2) @ResourceListManager.loadResourceList = sinon.stub().callsArg(1) @ResourceWriter.syncResourcesToDisk({ @@ -54,7 +55,7 @@ describe "ResourceWriter", -> .should.equal true it "should store the sync state", -> - @ResourceWriter.storeSyncState + @ResourceStateManager.saveProjectStateHash .calledWith(@syncState, @basePath) .should.equal true @@ -73,8 +74,8 @@ describe "ResourceWriter", -> ] @ResourceWriter._writeResourceToDisk = sinon.stub().callsArg(3) @ResourceWriter._removeExtraneousFiles = sinon.stub().callsArg(2) - @ResourceWriter.checkSyncState = sinon.stub().callsArgWith(2, null, true) - @ResourceWriter.storeSyncState = sinon.stub().callsArg(2) + @ResourceStateManager.checkProjectStateHashMatches = sinon.stub().callsArg(2) + @ResourceStateManager.saveProjectStateHash = sinon.stub().callsArg(2) @ResourceListManager.saveResourceList = sinon.stub().callsArg(2) @ResourceListManager.loadResourceList = sinon.stub().callsArgWith(1, null, @resources) @ResourceWriter.syncResourcesToDisk({ @@ -85,7 +86,7 @@ describe "ResourceWriter", -> }, @basePath, @callback) it "should check the sync state matches", -> - @ResourceWriter.checkSyncState + @ResourceStateManager.checkProjectStateHashMatches .calledWith(@syncState, @basePath) .should.equal true @@ -103,6 +104,28 @@ describe "ResourceWriter", -> it "should call the callback", -> @callback.called.should.equal true + describe "syncResourcesToDisk on an incremental update when the state does not match", -> + beforeEach -> + @resources = [ + "resource-1-mock" + ] + @ResourceStateManager.checkProjectStateHashMatches = sinon.stub().callsArgWith(2, @error = new Error()) + @ResourceWriter.syncResourcesToDisk({ + project_id: @project_id, + syncType: "incremental", + syncState: @syncState = "1234567890abcdef", + resources: @resources + }, @basePath, @callback) + + it "should check whether the sync state matches", -> + @ResourceStateManager.checkProjectStateHashMatches + .calledWith(@syncState, @basePath) + .should.equal true + + it "should call the callback with an error", -> + @callback.calledWith(@error).should.equal true + + describe "_removeExtraneousFiles", -> beforeEach -> @output_files = [{ From fc1782e74c6a782cf1d63491b5aff719685adc03 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 18 Aug 2017 11:17:01 +0100 Subject: [PATCH 17/17] read resource files safely put a limit on the amount of data read --- app/coffee/ResourceListManager.coffee | 5 +++-- app/coffee/ResourceStateManager.coffee | 11 +++++------ app/coffee/SafeReader.coffee | 24 ++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 8 deletions(-) create mode 100644 app/coffee/SafeReader.coffee diff --git a/app/coffee/ResourceListManager.coffee b/app/coffee/ResourceListManager.coffee index d68f5d1..aef2d2f 100644 --- a/app/coffee/ResourceListManager.coffee +++ b/app/coffee/ResourceListManager.coffee @@ -1,8 +1,8 @@ Path = require "path" fs = require "fs" -mkdirp = require "mkdirp" logger = require "logger-sharelatex" settings = require("settings-sharelatex") +SafeReader = require "./SafeReader" module.exports = ResourceListManager = @@ -18,7 +18,8 @@ module.exports = ResourceListManager = loadResourceList: (basePath, callback = (error) ->) -> resourceListFile = Path.join(basePath, @RESOURCE_LIST_FILE) - fs.readFile resourceListFile, (err, resourceList) -> + # limit file to 128K, compile directory is user accessible + SafeReader.readFile resourceListFile, 128*1024, 'utf8', (err, resourceList) -> return callback(err) if err? resources = ({path: path} for path in resourceList?.toString()?.split("\n") or []) callback(null, resources) diff --git a/app/coffee/ResourceStateManager.coffee b/app/coffee/ResourceStateManager.coffee index 2a6d177..eb193b8 100644 --- a/app/coffee/ResourceStateManager.coffee +++ b/app/coffee/ResourceStateManager.coffee @@ -1,9 +1,9 @@ Path = require "path" fs = require "fs" -mkdirp = require "mkdirp" logger = require "logger-sharelatex" settings = require("settings-sharelatex") Errors = require "./Errors" +SafeReader = require "./SafeReader" module.exports = ResourceStateManager = @@ -37,10 +37,9 @@ module.exports = ResourceStateManager = checkProjectStateHashMatches: (state, basePath, callback) -> stateFile = Path.join(basePath, @SYNC_STATE_FILE) - fs.readFile stateFile, {encoding:'ascii'}, (err, oldState) -> - if err? and err.code isnt 'ENOENT' - return callback(err) - else if state isnt oldState + SafeReader.readFile stateFile, 64, 'ascii', (err, oldState) -> + return callback(err) if err? + if state isnt oldState return callback new Errors.FilesOutOfSyncError("invalid state for incremental update") - else if state is oldState + else callback(null) diff --git a/app/coffee/SafeReader.coffee b/app/coffee/SafeReader.coffee new file mode 100644 index 0000000..7ed3693 --- /dev/null +++ b/app/coffee/SafeReader.coffee @@ -0,0 +1,24 @@ +fs = require "fs" + +module.exports = SafeReader = + + # safely read up to size bytes from a file and return result as a + # string + + readFile: (file, size, encoding, callback = (error, result) ->) -> + fs.open file, 'r', (err, fd) -> + return callback() if err? and err.code is 'ENOENT' + return callback(err) if err? + + # safely return always closing the file + callbackWithClose = (err, result) -> + fs.close fd, (err1) -> + return callback(err) if err? + return callback(err1) if err1? + callback(null, result) + + buff = new Buffer(size, 0) # fill with zeros + fs.read fd, buff, 0, buff.length, 0, (err, bytesRead, buffer) -> + return callbackWithClose(err) if err? + result = buffer.toString(encoding, 0, bytesRead) + callbackWithClose(null, result)