From ffa28b1a5852df4927b0b7325bdec510fac79e47 Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Wed, 4 Dec 2024 15:07:44 +0000 Subject: [PATCH] Merge pull request #22320 from overleaf/revert-22213-ar-avoid-duplicate-blob-writes Revert "Avoid duplicate blob writes" GitOrigin-RevId: 9f86bcea654cd3fa5f66fbdf42080e7f6e2861a7 --- .../app/js/ProjectHistoryRedisManager.js | 1 - .../ProjectHistoryRedisManagerTests.js | 66 ------------------- .../app/js/HistoryStoreManager.js | 10 --- services/project-history/app/js/types.ts | 1 - .../HistoryStoreManagerTests.js | 38 ----------- .../DocumentUpdater/DocumentUpdaterHandler.js | 1 - .../Features/FileStore/FileStoreHandler.js | 4 +- .../src/Features/Project/ProjectDuplicator.js | 4 +- .../Features/Uploads/ProjectUploadManager.js | 4 +- .../DocumentUpdaterHandlerTests.js | 5 -- .../src/Project/ProjectDuplicatorTests.js | 3 - .../src/Uploads/ProjectUploadManagerTests.js | 15 ++--- 12 files changed, 9 insertions(+), 143 deletions(-) diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 888948af09..b84edfe74a 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -122,7 +122,6 @@ const ProjectHistoryRedisManager = { hash: projectUpdate.hash, metadata: projectUpdate.metadata, projectHistoryId, - createdBlob: projectUpdate.createdBlob ?? false, } if (ranges) { projectUpdate.ranges = ranges diff --git a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js index 760385b176..0f5df2e29f 100644 --- a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js +++ b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js @@ -162,7 +162,6 @@ describe('ProjectHistoryRedisManager', function () { }, version: this.version, projectHistoryId: this.projectHistoryId, - createdBlob: false, doc: this.doc_id, } @@ -208,7 +207,6 @@ describe('ProjectHistoryRedisManager', function () { hash: '1337', metadata, projectHistoryId: this.projectHistoryId, - createdBlob: false, file: fileId, } @@ -293,7 +291,6 @@ describe('ProjectHistoryRedisManager', function () { }, version: this.version, projectHistoryId: this.projectHistoryId, - createdBlob: false, ranges: historyCompatibleRanges, doc: this.doc_id, } @@ -346,7 +343,6 @@ describe('ProjectHistoryRedisManager', function () { }, version: this.version, projectHistoryId: this.projectHistoryId, - createdBlob: false, doc: this.doc_id, } @@ -398,68 +394,6 @@ describe('ProjectHistoryRedisManager', function () { }, version: this.version, projectHistoryId: this.projectHistoryId, - createdBlob: false, - doc: this.doc_id, - } - - this.ProjectHistoryRedisManager.promises.queueOps - .calledWithExactly(this.project_id, JSON.stringify(update)) - .should.equal(true) - }) - - it('should pass "false" as the createdBlob field if not provided', async function () { - await this.ProjectHistoryRedisManager.promises.queueAddEntity( - this.project_id, - this.projectHistoryId, - 'doc', - this.doc_id, - this.user_id, - this.rawUpdate, - this.source - ) - - const update = { - pathname: this.pathname, - docLines: this.docLines, - meta: { - user_id: this.user_id, - ts: new Date(), - source: this.source, - }, - version: this.version, - projectHistoryId: this.projectHistoryId, - createdBlob: false, - doc: this.doc_id, - } - - this.ProjectHistoryRedisManager.promises.queueOps - .calledWithExactly(this.project_id, JSON.stringify(update)) - .should.equal(true) - }) - - it('should pass through the value of the createdBlob field', async function () { - this.rawUpdate.createdBlob = true - await this.ProjectHistoryRedisManager.promises.queueAddEntity( - this.project_id, - this.projectHistoryId, - 'doc', - this.doc_id, - this.user_id, - this.rawUpdate, - this.source - ) - - const update = { - pathname: this.pathname, - docLines: this.docLines, - meta: { - user_id: this.user_id, - ts: new Date(), - source: this.source, - }, - version: this.version, - projectHistoryId: this.projectHistoryId, - createdBlob: true, doc: this.doc_id, } diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 50bd52d793..1e74912ed3 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -300,18 +300,8 @@ export function createBlobForUpdate(projectId, historyId, update, callback) { 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}` - - if (update.createdBlob) { - logger.debug( - { projectId, fileId }, - 'Skipping blob creation as it has already been created' - ) - return callback(null, { file: update.hash }) - } - fetchStream(filestoreURL, { signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT), }) diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index c5f88e66e1..baf70b59d4 100644 --- a/services/project-history/app/js/types.ts +++ b/services/project-history/app/js/types.ts @@ -76,7 +76,6 @@ export type AddFileUpdate = ProjectUpdateBase & { file: string url: string hash: string - createdBlob?: boolean metadata?: LinkedFileData } diff --git a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js index d98c6eb7a5..f8b560a56f 100644 --- a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js +++ b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js @@ -491,44 +491,6 @@ describe('HistoryStoreManager', function () { expect(this.actualHash).to.equal(this.hash) }) }) - describe('when the createdBlob flag is set on the update', function () { - beforeEach(function (done) { - this.file_id = '012345678901234567890123' - this.update = { - file: true, - createdBlob: true, - url: `http://filestore.other.cloud.provider/project/${this.projectId}/file/${this.file_id}`, - hash: this.hash, - } - this.HistoryStoreManager.createBlobForUpdate( - this.projectId, - this.historyId, - this.update, - (err, { file: hash }) => { - if (err) { - return done(err) - } - this.actualHash = hash - done() - } - ) - }) - - it('should call the callback with the existing hash', function () { - expect(this.actualHash).to.equal(this.hash) - }) - - it('should not request the file from the filestore', function () { - expect(this.FetchUtils.fetchStream).to.not.have.been.called - }) - - it('should log a debug level message', function () { - expect(this.logger.debug).to.have.been.calledWith( - { projectId: this.projectId, fileId: this.file_id }, - 'Skipping blob creation as it has already been created' - ) - }) - }) }) describe('getProjectBlob', function () { diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js index 7130b7131d..ea7efcf8ee 100644 --- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js +++ b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js @@ -490,7 +490,6 @@ function _getUpdates( url: newEntity.url, hash: newEntity.file != null ? newEntity.file.hash : undefined, metadata: buildFileMetadataForHistory(newEntity.file), - createdBlob: newEntity.createdBlob ?? false, }) } else if (newEntity.path !== oldEntity.path) { // entity renamed diff --git a/services/web/app/src/Features/FileStore/FileStoreHandler.js b/services/web/app/src/Features/FileStore/FileStoreHandler.js index dc2210af4d..af75f71d84 100644 --- a/services/web/app/src/Features/FileStore/FileStoreHandler.js +++ b/services/web/app/src/Features/FileStore/FileStoreHandler.js @@ -100,7 +100,7 @@ const FileStoreHandler = { }) return callback(err) } - callback(err, result.url, result.fileRef, true) + callback(err, result.url, result.fileRef) } ) } @@ -303,6 +303,6 @@ module.exports = FileStoreHandler module.exports.promises = promisifyAll(FileStoreHandler, { multiResult: { uploadFileFromDisk: ['url', 'fileRef'], - uploadFileFromDiskWithHistoryId: ['url', 'fileRef', 'createdBlob'], + uploadFileFromDiskWithHistoryId: ['url', 'fileRef'], }, }) diff --git a/services/web/app/src/Features/Project/ProjectDuplicator.js b/services/web/app/src/Features/Project/ProjectDuplicator.js index 25e66c55ba..18594d3ff8 100644 --- a/services/web/app/src/Features/Project/ProjectDuplicator.js +++ b/services/web/app/src/Features/Project/ProjectDuplicator.js @@ -211,7 +211,6 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { if (sourceFile.hash != null) { file.hash = sourceFile.hash } - let createdBlob = false if (file.hash != null) { try { await HistoryManager.promises.copyBlob( @@ -219,7 +218,6 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { targetHistoryId, file.hash ) - createdBlob = true } catch (err) { logger.error( { @@ -239,7 +237,7 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { targetProject._id, file._id ) - return { createdBlob, file, path, url } + return { file, path, url } } ) return targetEntries diff --git a/services/web/app/src/Features/Uploads/ProjectUploadManager.js b/services/web/app/src/Features/Uploads/ProjectUploadManager.js index d42bc0eb22..8af189254f 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadManager.js +++ b/services/web/app/src/Features/Uploads/ProjectUploadManager.js @@ -194,14 +194,14 @@ async function _createFile(project, projectPath, fsPath) { throw new OError('missing history id') } const fileName = Path.basename(projectPath) - const { createdBlob, fileRef, url } = + const { fileRef, url } = await FileStoreHandler.promises.uploadFileFromDiskWithHistoryId( projectId, historyId, { name: fileName }, fsPath ) - return { createdBlob, file: fileRef, path: projectPath, url } + return { file: fileRef, path: projectPath, url } } async function _notifyDocumentUpdater(project, userId, changes) { diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js index cec68eac72..bdfe6ab9db 100644 --- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js +++ b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js @@ -1132,7 +1132,6 @@ describe('DocumentUpdaterHandler', function () { hash: undefined, ranges: undefined, metadata: undefined, - createdBlob: false, }, ] @@ -1185,7 +1184,6 @@ describe('DocumentUpdaterHandler', function () { historyRangesSupport: false, hash: '12345', ranges: undefined, - createdBlob: false, metadata: undefined, }, ] @@ -1298,7 +1296,6 @@ describe('DocumentUpdaterHandler', function () { hash: undefined, ranges: undefined, metadata: undefined, - createdBlob: false, }, ] @@ -1401,7 +1398,6 @@ describe('DocumentUpdaterHandler', function () { hash: undefined, ranges: this.ranges, metadata: undefined, - createdBlob: false, }, ] @@ -1448,7 +1444,6 @@ describe('DocumentUpdaterHandler', function () { hash: undefined, ranges: this.ranges, metadata: undefined, - createdBlob: false, }, ] diff --git a/services/web/test/unit/src/Project/ProjectDuplicatorTests.js b/services/web/test/unit/src/Project/ProjectDuplicatorTests.js index 3561d28809..df53bbb044 100644 --- a/services/web/test/unit/src/Project/ProjectDuplicatorTests.js +++ b/services/web/test/unit/src/Project/ProjectDuplicatorTests.js @@ -105,19 +105,16 @@ describe('ProjectDuplicator', function () { ] this.fileEntries = [ { - createdBlob: false, path: this.file0Path, file: this.newFile0, url: this.filestoreUrl, }, { - createdBlob: false, path: this.file1Path, file: this.newFile1, url: this.filestoreUrl, }, { - createdBlob: true, path: this.file2Path, file: this.newFile2, url: this.filestoreUrl, diff --git a/services/web/test/unit/src/Uploads/ProjectUploadManagerTests.js b/services/web/test/unit/src/Uploads/ProjectUploadManagerTests.js index 2cfeb5a5cd..abdec03d11 100644 --- a/services/web/test/unit/src/Uploads/ProjectUploadManagerTests.js +++ b/services/web/test/unit/src/Uploads/ProjectUploadManagerTests.js @@ -58,12 +58,7 @@ describe('ProjectUploadManager', function () { }, ] this.fileEntries = [ - { - file: this.file, - path: `/${this.file.name}`, - url: this.fileStoreUrl, - createdBlob: true, - }, + { file: this.file, path: `/${this.file.name}`, url: this.fileStoreUrl }, ] this.fs = { @@ -99,11 +94,9 @@ describe('ProjectUploadManager', function () { } this.FileStoreHandler = { promises: { - uploadFileFromDiskWithHistoryId: sinon.stub().resolves({ - fileRef: this.file, - url: this.fileStoreUrl, - createdBlob: true, - }), + uploadFileFromDiskWithHistoryId: sinon + .stub() + .resolves({ fileRef: this.file, url: this.fileStoreUrl }), }, } this.FileSystemImportManager = {