From 72b408331830b98bcaecb1f1f174314d6e93198a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 30 Jan 2025 11:42:38 +0000 Subject: [PATCH] [project-history] forward all the optional file attributes during resync (#23260) Notably, 'createdBlob' was missing. And others like metadata should not be added if not set. GitOrigin-RevId: c4a6746c4824d637f7c54b4c68a7025b60c15ff2 --- .../project-history/app/js/SyncManager.js | 14 +- services/project-history/app/js/types.ts | 3 +- .../test/acceptance/js/SyncTests.js | 222 ++++++++++++++++++ .../unit/js/SyncManager/SyncManagerTests.js | 86 ++++++- 4 files changed, 315 insertions(+), 10 deletions(-) diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 0a596be393..ca3963b9d2 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -530,9 +530,10 @@ class SyncUpdateExpander { this.files[update.pathname] = File.fromString('') } else { update.file = entity.file - update.url = entity.url - update.hash = entity._hash - update.metadata = entity.metadata + if (entity.url) update.url = entity.url + if (entity._hash) update.hash = entity._hash + if (entity.createdBlob) update.createdBlob = entity.createdBlob + if (entity.metadata) update.metadata = entity.metadata } this.expandedUpdates.push(update) @@ -619,10 +620,11 @@ class SyncUpdateExpander { ts: update.meta.ts, }, file: entity.file, - url: entity.url, - hash: entity._hash, - metadata: entity.metadata, } + if (entity.url) addUpdate.url = entity.url + if (entity._hash) addUpdate.hash = entity._hash + if (entity.createdBlob) addUpdate.createdBlob = entity.createdBlob + if (entity.metadata) addUpdate.metadata = entity.metadata this.expandedUpdates.push(addUpdate) Metrics.inc('project_history_resync_operation', 1, { status: 'update binary file contents', diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index 638fa30ca2..37cd50f9a9 100644 --- a/services/project-history/app/js/types.ts +++ b/services/project-history/app/js/types.ts @@ -212,7 +212,8 @@ export type File = { file: string url?: string path: string - _hash: string + _hash?: string + createdBlob?: boolean metadata?: LinkedFileData } diff --git a/services/project-history/test/acceptance/js/SyncTests.js b/services/project-history/test/acceptance/js/SyncTests.js index 2f04ed24e6..001c920e3e 100644 --- a/services/project-history/test/acceptance/js/SyncTests.js +++ b/services/project-history/test/acceptance/js/SyncTests.js @@ -622,6 +622,110 @@ describe('Syncing with web and doc-updater', function () { } ) }) + it('should add file w/o url', function (done) { + MockHistoryStore() + .get(`/api/projects/${historyId}/latest/history`) + .reply(200, { + chunk: { + history: { + snapshot: { + files: { + persistedFile: { hash: EMPTY_FILE_HASH, byteLength: 0 }, + }, + }, + changes: [], + }, + startVersion: 0, + }, + }) + + const fileContents = Buffer.from([1, 2, 3]) + const fileHash = 'aed2973e4b8a7ff1b30ff5c4751e5a2b38989e74' + + MockFileStore() + .get(`/project/${this.project_id}/file/${this.file_id}`) + .reply(200, fileContents) + const headBlob = MockHistoryStore() + .head(`/api/projects/${historyId}/blobs/${fileHash}`) + .reply(200) + const createBlob = MockHistoryStore() + .put(`/api/projects/${historyId}/blobs/${fileHash}`, fileContents) + .reply(201) + + const addFile = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + expect(body).to.deep.equal([ + { + v2Authors: [], + authors: [], + timestamp: this.timestamp.toJSON(), + operations: [ + { + pathname: 'test.png', + file: { + hash: fileHash, + }, + }, + ], + origin: { kind: 'test-origin' }, + }, + ]) + return true + }) + .query({ end_version: 0 }) + .reply(204) + + async.series( + [ + cb => { + ProjectHistoryClient.resyncHistory(this.project_id, cb) + }, + cb => { + const update = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [], + files: [ + { + file: this.file_id, + path: '/test.png', + _hash: fileHash, + createdBlob: true, + }, + { path: '/persistedFile' }, + ], + }, + meta: { + ts: this.timestamp, + }, + } + ProjectHistoryClient.pushRawUpdate(this.project_id, update, cb) + }, + cb => { + ProjectHistoryClient.flushProject(this.project_id, cb) + }, + ], + error => { + if (error) { + throw error + } + assert(!loggerWarn.called, 'no warning logged on 404') + assert( + headBlob.isDone(), + 'HEAD /api/projects/:historyId/blobs/:hash should have been called' + ) + assert( + !createBlob.isDone(), + '/api/projects/:historyId/blobs/:hash should have been skipped' + ) + assert( + addFile.isDone(), + `/api/projects/${historyId}/changes should have been called` + ) + done() + } + ) + }) describe('with filestore disabled', function () { before(function () { Settings.apis.filestore.enabled = false @@ -760,6 +864,124 @@ describe('Syncing with web and doc-updater', function () { }) }) + describe('when a file hash mismatches', function () { + it('should remove and re-add file w/o url', function (done) { + MockHistoryStore() + .get(`/api/projects/${historyId}/latest/history`) + .reply(200, { + chunk: { + history: { + snapshot: { + files: { + 'test.png': { hash: EMPTY_FILE_HASH, byteLength: 0 }, + }, + }, + changes: [], + }, + startVersion: 0, + }, + }) + + const fileContents = Buffer.from([1, 2, 3]) + const fileHash = 'aed2973e4b8a7ff1b30ff5c4751e5a2b38989e74' + + MockFileStore() + .get(`/project/${this.project_id}/file/${this.file_id}`) + .reply(200, fileContents) + const headBlob = MockHistoryStore() + .head(`/api/projects/${historyId}/blobs/${fileHash}`) + .reply(200) + const createBlob = MockHistoryStore() + .put(`/api/projects/${historyId}/blobs/${fileHash}`, fileContents) + .reply(201) + + const addFile = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + expect(body).to.deep.equal([ + { + v2Authors: [], + authors: [], + timestamp: this.timestamp.toJSON(), + operations: [ + { + pathname: 'test.png', + newPathname: '', + }, + ], + origin: { kind: 'test-origin' }, + }, + { + v2Authors: [], + authors: [], + timestamp: this.timestamp.toJSON(), + operations: [ + { + pathname: 'test.png', + file: { + hash: fileHash, + }, + }, + ], + origin: { kind: 'test-origin' }, + }, + ]) + return true + }) + .query({ end_version: 0 }) + .reply(204) + + async.series( + [ + cb => { + ProjectHistoryClient.resyncHistory(this.project_id, cb) + }, + cb => { + const update = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [], + files: [ + { + file: this.file_id, + path: '/test.png', + _hash: fileHash, + createdBlob: true, + }, + ], + }, + meta: { + ts: this.timestamp, + }, + } + ProjectHistoryClient.pushRawUpdate(this.project_id, update, cb) + }, + cb => { + ProjectHistoryClient.flushProject(this.project_id, cb) + }, + ], + error => { + if (error) { + throw error + } + assert(!loggerWarn.called, 'no warning logged on 404') + assert( + headBlob.isDone(), + 'HEAD /api/projects/:historyId/blobs/:hash should have been called' + ) + assert( + !createBlob.isDone(), + '/api/projects/:historyId/blobs/:hash should have been skipped' + ) + assert( + addFile.isDone(), + `/api/projects/${historyId}/changes should have been called` + ) + done() + } + ) + }) + }) + describe("when a file exists which shouldn't", function () { it('should send remove file updates to the history store', function (done) { MockHistoryStore() diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index cbf52ac15e..31ecb10b0b 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -631,7 +631,43 @@ describe('SyncManager', function () { file: newFile.file, url: newFile.url, hash: 'hash-42', - metadata: undefined, + meta: { + resync: true, + ts: TIMESTAMP, + origin: { kind: 'history-resync' }, + }, + }, + ]) + expect(this.extendLock).to.have.been.called + }) + + it('queues file additions for missing regular files w/o url', async function () { + const newFile = { + path: '2.png', + file: {}, + _hash: 'hash-42', + createdBlob: true, + } + const updates = [ + resyncProjectStructureUpdate( + [this.persistedDoc], + [this.persistedFile, newFile] + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + + expect(expandedUpdates).to.deep.equal([ + { + pathname: newFile.path, + file: newFile.file, + hash: 'hash-42', + createdBlob: true, meta: { resync: true, ts: TIMESTAMP, @@ -757,7 +793,6 @@ describe('SyncManager', function () { file: fileWichWasADoc.file, url: fileWichWasADoc.url, hash: 'other-hash', - metadata: undefined, meta: { resync: true, ts: TIMESTAMP, @@ -1035,7 +1070,52 @@ describe('SyncManager', function () { file: persistedFileWithNewContent.file, url: persistedFileWithNewContent.url, hash: 'anotherhashvalue', - metadata: undefined, + meta: { + resync: true, + ts: TIMESTAMP, + origin: { kind: 'history-resync' }, + }, + }, + ]) + expect(this.extendLock).to.have.been.called + }) + + it('removes and re-adds binary files w/o url if they do not have same hash', async function () { + const persistedFileWithNewContent = { + _hash: 'anotherhashvalue', + hello: 'world', + path: '1.png', + createdBlob: true, + } + const updates = [ + resyncProjectStructureUpdate( + [this.persistedDoc], + [persistedFileWithNewContent] + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + + expect(expandedUpdates).to.deep.equal([ + { + pathname: persistedFileWithNewContent.path, + new_pathname: '', + meta: { + resync: true, + ts: TIMESTAMP, + origin: { kind: 'history-resync' }, + }, + }, + { + pathname: persistedFileWithNewContent.path, + file: persistedFileWithNewContent.file, + hash: 'anotherhashvalue', + createdBlob: true, meta: { resync: true, ts: TIMESTAMP,