From e64e69f539aebeb13788d287c2c27675dd4a8ab6 Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Mon, 23 Dec 2024 12:01:06 +0000 Subject: [PATCH] Merge pull request #22652 from overleaf/ar-handle-filestore-404-when-copying-in-saas [web] Do not do filestore copy in SAAS GitOrigin-RevId: 83456ca57f706246a86b997a038968aecfbae4c5 --- .../app/js/HistoryStoreManager.js | 55 +++++++++---- .../app/js/UpdateTranslator.js | 4 +- .../test/acceptance/js/SendingUpdatesTests.js | 80 +++++++++++++++++++ .../src/Features/Project/ProjectDuplicator.js | 5 ++ .../acceptance/src/ProjectStructureTests.mjs | 7 +- .../src/Project/ProjectDuplicatorTests.js | 13 ++- 6 files changed, 142 insertions(+), 22 deletions(-) diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index a2ddf5fe2f..641eb0f2fd 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -263,6 +263,29 @@ function _checkBlobExists(historyId, hash, callback) { }) } +function _rewriteFilestoreUrl(url, projectId, callback) { + if (!url) { + return { fileId: null, filestoreURL: null } + } + // Rewrite the filestore url to point to the location in the local + // settings for this service (this avoids problems with cross- + // datacentre requests when running filestore in multiple locations). + const { pathname: fileStorePath } = new URL(url) + const urlMatch = /^\/project\/([0-9a-f]{24})\/file\/([0-9a-f]{24})$/.exec( + fileStorePath + ) + if (urlMatch == null) { + return callback(new OError('invalid file for blob creation')) + } + if (urlMatch[1] !== projectId) { + return callback(new OError('invalid project for blob creation')) + } + + const fileId = urlMatch[2] + const filestoreURL = `${Settings.apis.filestore.url}/project/${projectId}/file/${fileId}` + return { filestoreURL, fileId } +} + export function createBlobForUpdate(projectId, historyId, update, callback) { callback = _.once(callback) @@ -303,24 +326,15 @@ export function createBlobForUpdate(projectId, historyId, update, callback) { } } ) - } else if (update.file != null && update.url != null) { - // Rewrite the filestore url to point to the location in the local - // settings for this service (this avoids problems with cross- - // datacentre requests when running filestore in multiple locations). - const { pathname: fileStorePath } = new URL(update.url) - const urlMatch = /^\/project\/([0-9a-f]{24})\/file\/([0-9a-f]{24})$/.exec( - fileStorePath + } else if ( + update.file != null && + (update.url != null || update.createdBlob) + ) { + const { fileId, filestoreURL } = _rewriteFilestoreUrl( + update.url, + projectId, + callback ) - if (urlMatch == null) { - return callback(new OError('invalid file for blob creation')) - } - if (urlMatch[1] !== projectId) { - return callback(new OError('invalid project for blob creation')) - } - - const fileId = urlMatch[2] - const filestoreURL = `${Settings.apis.filestore.url}/project/${projectId}/file/${fileId}` - _checkBlobExists(historyId, update.hash, (err, blobExists) => { if (err) { logger.warn( @@ -339,6 +353,13 @@ export function createBlobForUpdate(projectId, historyId, update, callback) { 'created blob does not exist, reading from filestore' ) } + + if (!filestoreURL) { + return callback( + new OError('no filestore URL provided and blob was not created') + ) + } + fetchStream(filestoreURL, { signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT), }) diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index 7755731e15..82a1de05ff 100644 --- a/services/project-history/app/js/UpdateTranslator.js +++ b/services/project-history/app/js/UpdateTranslator.js @@ -166,8 +166,8 @@ function _isAddFileUpdate(update) { return ( 'file' in update && update.file != null && - 'url' in update && - update.url != null + (('createdBlob' in update && update.createdBlob) || + ('url' in update && update.url != null)) ) } diff --git a/services/project-history/test/acceptance/js/SendingUpdatesTests.js b/services/project-history/test/acceptance/js/SendingUpdatesTests.js index c4e0c990e4..dce5474cf0 100644 --- a/services/project-history/test/acceptance/js/SendingUpdatesTests.js +++ b/services/project-history/test/acceptance/js/SendingUpdatesTests.js @@ -66,6 +66,19 @@ function slAddFileUpdate(historyId, file, userId, ts, projectId) { } } +function createdBlobFileUpdate(historyId, file, userId, ts, projectId) { + return { + projectHistoryId: historyId, + pathname: file.pathname, + createdBlob: true, + url: null, + file: file.id, + hash: file.hash, + ranges: undefined, + meta: { user_id: userId, ts: ts.getTime() }, + } +} + function slRenameUpdate(historyId, doc, userId, ts, pathname, newPathname) { return { projectHistoryId: historyId, @@ -554,6 +567,73 @@ describe('Sending Updates', function () { ) }) + it('should not get file from filestore if no url provided', function (done) { + const file = { + id: new ObjectId().toString(), + pathname: '/test.png', + contents: Buffer.from([1, 2, 3]), + hash: 'aed2973e4b8a7ff1b30ff5c4751e5a2b38989e74', + } + + const fileStoreRequest = MockFileStore() + .get(`/project/${this.projectId}/file/${file.id}`) + .reply(200, file.contents) + + const checkBlob = MockHistoryStore() + .head(`/api/projects/${historyId}/blobs/${file.hash}`) + .reply(200) + + const addFile = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + expect(body).to.deep.equal([ + olAddFileUpdate(file, this.userId, this.timestamp, file.hash), + ]) + return true + }) + .query({ end_version: 0 }) + .reply(204) + + async.series( + [ + cb => { + ProjectHistoryClient.pushRawUpdate( + this.projectId, + createdBlobFileUpdate( + historyId, + file, + this.userId, + this.timestamp, + this.projectId + ), + cb + ) + }, + cb => { + ProjectHistoryClient.flushProject(this.projectId, cb) + }, + ], + error => { + if (error) { + return done(error) + } + assert( + !fileStoreRequest.isDone(), + 'filestore should not have been called' + ) + + assert( + checkBlob.isDone(), + `HEAD /api/projects/${historyId}/blobs/${file.hash} should have been called` + ) + assert( + addFile.isDone(), + `/api/projects/${historyId}/latest/files should have been called` + ) + done() + } + ) + }) + it('should send add file updates to the history store', function (done) { const file = { id: new ObjectId().toString(), diff --git a/services/web/app/src/Features/Project/ProjectDuplicator.js b/services/web/app/src/Features/Project/ProjectDuplicator.js index 25e66c55ba..bb38b2cda8 100644 --- a/services/web/app/src/Features/Project/ProjectDuplicator.js +++ b/services/web/app/src/Features/Project/ProjectDuplicator.js @@ -20,6 +20,7 @@ const SafePath = require('./SafePath') const TpdsProjectFlusher = require('../ThirdPartyDataStore/TpdsProjectFlusher') const _ = require('lodash') const TagsHandler = require('../Tags/TagsHandler') +const Features = require('../../infrastructure/Features') module.exports = { duplicate: callbackify(duplicate), @@ -233,12 +234,16 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { ) } } + if (createdBlob && Features.hasFeature('saas')) { + return { createdBlob, file, path, url: null } + } const url = await FileStoreHandler.promises.copyFile( sourceProject._id, sourceFile._id, targetProject._id, file._id ) + return { createdBlob, file, path, url } } ) diff --git a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs index 73be773da9..3ac572ee1d 100644 --- a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs +++ b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs @@ -8,6 +8,7 @@ import _ from 'lodash' import ProjectGetter from '../../../../../app/src/Features/Project/ProjectGetter.js' import User from '../../../../../test/acceptance/src/helpers/User.js' import MockDocUpdaterApiClass from '../../../../../test/acceptance/src/mocks/MockDocUpdaterApi.js' +import Features from '../../../../../app/src/infrastructure/Features.js' const { ObjectId } = mongodb @@ -256,7 +257,11 @@ describe('ProjectStructureChanges', function () { expect(updates[2].type).to.equal('add-file') expect(updates[2].userId).to.equal(owner._id) expect(updates[2].pathname).to.equal('/frog.jpg') - expect(updates[2].url).to.be.a('string') + if (Features.hasFeature('saas')) { + expect(updates[2].url).to.be.null + } else { + expect(updates[2].url).to.be.a('string') + } expect(version).to.equal(1) }) }) diff --git a/services/web/test/unit/src/Project/ProjectDuplicatorTests.js b/services/web/test/unit/src/Project/ProjectDuplicatorTests.js index 3561d28809..b272f4bdd9 100644 --- a/services/web/test/unit/src/Project/ProjectDuplicatorTests.js +++ b/services/web/test/unit/src/Project/ProjectDuplicatorTests.js @@ -120,7 +120,7 @@ describe('ProjectDuplicator', function () { createdBlob: true, path: this.file2Path, file: this.newFile2, - url: this.filestoreUrl, + url: null, }, ] @@ -304,7 +304,7 @@ describe('ProjectDuplicator', function () { }) it('should copy files to the filestore', function () { - for (const file of [this.file0, this.file1, this.file2]) { + for (const file of [this.file0, this.file1]) { this.FileStoreHandler.promises.copyFile.should.have.been.calledWith( this.project._id, file._id, @@ -314,6 +314,15 @@ describe('ProjectDuplicator', function () { } }) + it('should not copy files that have been sent to history-v1 to the filestore', function () { + this.FileStoreHandler.promises.copyFile.should.not.have.been.calledWith( + this.project._id, + this.file2._id, + this.newProject._id, + this.newFileId + ) + }) + it('should create a blank project', function () { this.ProjectCreationHandler.promises.createBlankProject.should.have.been.calledWith( this.owner._id,