diff --git a/libraries/overleaf-editor-core/index.js b/libraries/overleaf-editor-core/index.js index baf10e31d8..dce4aa2f00 100644 --- a/libraries/overleaf-editor-core/index.js +++ b/libraries/overleaf-editor-core/index.js @@ -2,6 +2,7 @@ const AddCommentOperation = require('./lib/operation/add_comment_operation') const Author = require('./lib/author') const AuthorList = require('./lib/author_list') const Blob = require('./lib/blob') +const BlobStoreBase = require('./lib/blob_store_base') const Change = require('./lib/change') const ChangeRequest = require('./lib/change_request') const ChangeNote = require('./lib/change_note') @@ -66,6 +67,7 @@ exports.AddCommentOperation = AddCommentOperation exports.Author = Author exports.AuthorList = AuthorList exports.Blob = Blob +exports.BlobStoreBase = BlobStoreBase exports.Change = Change exports.ChangeRequest = ChangeRequest exports.ChangeNote = ChangeNote diff --git a/libraries/overleaf-editor-core/lib/blob_store_base.js b/libraries/overleaf-editor-core/lib/blob_store_base.js new file mode 100644 index 0000000000..59386b9956 --- /dev/null +++ b/libraries/overleaf-editor-core/lib/blob_store_base.js @@ -0,0 +1,68 @@ +// @ts-check +'use strict' + +const Blob = require('./blob') + +/** + * The parts of reading a blob that every blob store answers the same way. + * + * The hash of empty content is a property of the content rather than of anything + * stored: every project's empty file has it, whether or not anything ever wrote a + * blob under it, and a file may reference it before anything has. So no store has + * to look it up, and every store that did was paying a lookup -- a database query, + * an HTTP request, a read of a cache that offline mode may be the only thing + * standing in for -- to be told what the hash already says. Deserializing a blob + * that holds JSON is likewise the same wherever it is read from. + * + * Implementations extend this and provide `fetchString`, which is only ever called + * for a hash that needs fetching. Spelling either of these out in each store + * instead is a line per store, and the store written next is the one that forgets. + * + * `getObject` inherits the empty-hash answer through `getString`, so it parses `''` + * and fails. That is right: a ranges blob is written from `JSON.stringify`, which + * never yields empty content, so a hash reaching `getObject` is one something + * stored. A store that reads or repairs its JSON differently overrides it. + */ +class BlobStoreBase { + /** + * Fetch a blob's content as a string. + * + * @param {string} hash hexadecimal SHA-1 hash + * @return {Promise} + */ + async getString(hash) { + if (hash === Blob.EMPTY_HASH) { + return '' + } + return await this.fetchString(hash) + } + + /** + * Fetch a blob holding JSON and deserialize it. + * + * @template [T=unknown] + * @param {string} hash hexadecimal SHA-1 hash + * @return {Promise} + */ + async getObject(hash) { + return /** @type {T} */ (JSON.parse(await this.getString(hash))) + } + + /** + * Fetch a blob's content as a string from wherever this store keeps it. + * + * Called only for a hash that something needs to have stored, so an + * implementation may treat a miss as an error. + * + * @abstract + * @param {string} hash hexadecimal SHA-1 hash + * @return {Promise} + */ + async fetchString(hash) { + throw new Error( + `${this.constructor.name} does not implement fetchString(hash)` + ) + } +} + +module.exports = BlobStoreBase diff --git a/libraries/overleaf-editor-core/test/unit/blob_store_base.test.js b/libraries/overleaf-editor-core/test/unit/blob_store_base.test.js new file mode 100644 index 0000000000..227d32d76b --- /dev/null +++ b/libraries/overleaf-editor-core/test/unit/blob_store_base.test.js @@ -0,0 +1,95 @@ +'use strict' + +const { expect } = require('chai') + +const { Blob, BlobStoreBase } = require('../..') + +class TestBlobStore extends BlobStoreBase { + constructor(contentByHash = {}) { + super() + this.contentByHash = contentByHash + this.fetched = [] + } + + async fetchString(hash) { + this.fetched.push(hash) + if (!(hash in this.contentByHash)) { + throw new Error(`nothing stored under ${hash}`) + } + return this.contentByHash[hash] + } +} + +const HASH = 'a1b2c3d4e5f60718293a4b5c6d7e8f9012345678' + +describe('BlobStoreBase', function () { + describe('getString', function () { + it('fetches the content of a hash something stored', async function () { + const blobStore = new TestBlobStore({ [HASH]: 'hello world' }) + + expect(await blobStore.getString(HASH)).to.equal('hello world') + expect(blobStore.fetched).to.deep.equal([HASH]) + }) + + it('answers the hash of empty content without fetching', async function () { + // The store below has nothing stored under it and throws when asked, which is + // the point: the hash says what the content is, so no store has to be asked. + const blobStore = new TestBlobStore() + + expect(await blobStore.getString(Blob.EMPTY_HASH)).to.equal('') + expect(blobStore.fetched).to.have.length(0) + }) + + it('passes on what the implementation throws', async function () { + const blobStore = new TestBlobStore() + + try { + await blobStore.getString(HASH) + expect.fail('should have thrown') + } catch (err) { + expect(err.message).to.equal(`nothing stored under ${HASH}`) + } + }) + }) + + describe('getObject', function () { + it('deserializes the content of the hash', async function () { + const blobStore = new TestBlobStore({ + [HASH]: '{"comments":[],"trackedChanges":[]}', + }) + + expect(await blobStore.getObject(HASH)).to.deep.equal({ + comments: [], + trackedChanges: [], + }) + }) + + it('fails on the hash of empty content, which is not JSON', async function () { + // Nothing writes an empty ranges blob -- JSON.stringify never yields empty + // content -- so a hash reaching here is one something stored. + const blobStore = new TestBlobStore() + + try { + await blobStore.getObject(Blob.EMPTY_HASH) + expect.fail('should have thrown') + } catch (err) { + expect(err).to.be.an.instanceof(SyntaxError) + } + }) + }) + + describe('fetchString', function () { + it('names the store that has not implemented it', async function () { + class IncompleteBlobStore extends BlobStoreBase {} + + try { + await new IncompleteBlobStore().getString(HASH) + expect.fail('should have thrown') + } catch (err) { + expect(err.message).to.equal( + 'IncompleteBlobStore does not implement fetchString(hash)' + ) + } + }) + }) +}) diff --git a/services/clsi/app/js/HistoryResourceWriter.js b/services/clsi/app/js/HistoryResourceWriter.js index a50cba07cc..424e6b0d02 100644 --- a/services/clsi/app/js/HistoryResourceWriter.js +++ b/services/clsi/app/js/HistoryResourceWriter.js @@ -9,9 +9,9 @@ import Errors from './Errors.js' import { callbackify, promisify } from 'node:util' import { AddFileOperation, + BlobStoreBase, Change, EditFileOperation, - File, MoveFileOperation, Snapshot, } from 'overleaf-editor-core' @@ -799,7 +799,7 @@ export async function syncResourcesToDisk( } } -class BlobStore { +class BlobStore extends BlobStoreBase { /** @type {string} */ #historyId /** @type {string[]} */ @@ -816,6 +816,7 @@ class BlobStore { * @param {string[]} globalBlobs */ constructor(historyId, filestoreBlobPrefix, clsiPerfVariant, globalBlobs) { + super() this.#historyId = historyId this.#filestoreBlobPrefix = filestoreBlobPrefix this.#clsiPerfVariant = clsiPerfVariant @@ -845,8 +846,7 @@ class BlobStore { * @param {string} hash * @return {Promise} */ - async getString(hash) { - if (hash === File.EMPTY_FILE_HASH) return '' + async fetchString(hash) { const u = this.getBlobURL(hash) let remainingAttempts = 3 while (true) { @@ -866,13 +866,4 @@ class BlobStore { } } } - - /** - * @param {string} hash - * @return {Promise} - */ - async getObject(hash) { - const string = await this.getString(hash) - return JSON.parse(string) - } } diff --git a/services/history-v1/storage/lib/backupArchiver.mjs b/services/history-v1/storage/lib/backupArchiver.mjs index 376f0af9e0..33f366cee8 100644 --- a/services/history-v1/storage/lib/backupArchiver.mjs +++ b/services/history-v1/storage/lib/backupArchiver.mjs @@ -29,7 +29,7 @@ import { NoKEKMatchedError } from '@overleaf/object-persistor/src/Errors.js' const globalBlobsBucket = config.get('blobStore.globalBucket') -class BackupBlobStore { +class BackupBlobStore extends core.BlobStoreBase { /** * * @param {string} historyId @@ -38,6 +38,7 @@ class BackupBlobStore { * @param {boolean} useBackupGlobalBlobs */ constructor(historyId, tmp, persistor, useBackupGlobalBlobs) { + super() this.historyId = historyId this.tmp = tmp this.blobs = new Map() @@ -46,17 +47,13 @@ class BackupBlobStore { } /** - * Required for BlobStore interface - not supported. - * * @template T * @param {string} hash * @return {Promise} */ async getObject(hash) { try { - const stream = await this.getStream(hash) - const buffer = await streams.readStreamToBuffer(stream) - return JSON.parse(buffer.toString()) + return await super.getObject(hash) } catch (err) { logger.warn({ err, hash }, 'Failed to fetch chunk blob') throw err @@ -146,7 +143,7 @@ class BackupBlobStore { * @param {string} hash * @return {Promise} */ - async getString(hash) { + async fetchString(hash) { const stream = await this.getStream(hash) const buffer = await streams.readStreamToBuffer(stream) return buffer.toString() diff --git a/services/history-v1/storage/lib/blob_store/index.js b/services/history-v1/storage/lib/blob_store/index.js index 759a20584f..dc65abc008 100644 --- a/services/history-v1/storage/lib/blob_store/index.js +++ b/services/history-v1/storage/lib/blob_store/index.js @@ -28,6 +28,21 @@ const { promiseMapWithLimit } = require('@overleaf/promise-utils') /** @type {Map} */ const GLOBAL_BLOBS = new Map() +/** + * Empty content, which reads need nothing stored to answer. + * + * Its hash is a property of the content rather than of anything written, and every + * project's empty file has it, so a read answers it from here rather than looking for + * it: a file may reference it before anything has stored it. Writes are unchanged and + * still store it, so a project that wrote empty content also has a blob for it; a read + * answers the same either way. + * + * Left out of `GLOBAL_BLOBS` deliberately, because that is loaded from the database and + * can be configured away, and because a global blob is fetched from the global bucket + * rather than not fetched at all. + */ +const EMPTY_BLOB = new Blob(Blob.EMPTY_HASH, 0, 0) + function makeGlobalKey(hash) { return `${hash.slice(0, 2)}/${hash.slice(2, 4)}/${hash.slice(4)}` } @@ -163,12 +178,13 @@ async function getProjectBlobsBatch(projectIds) { * blob store manages both content and metadata (byte and UTF-8 length) for * blobs. */ -class BlobStore { +class BlobStore extends core.BlobStoreBase { /** * @constructor * @param {string} projectId the project for which we'd like to find blobs */ constructor(projectId) { + super() assert.projectId(projectId) this.projectId = projectId this.backend = getBackend(this.projectId) @@ -279,7 +295,7 @@ class BlobStore { * @param {string} hash hexadecimal SHA-1 hash * @return {Promise.} promise for the content of the file */ - async getString(hash) { + async fetchString(hash) { assert.blobHash(hash, 'bad hash') const projectId = this.projectId @@ -333,6 +349,10 @@ class BlobStore { */ async getStream(hash, opts = {}) { assert.blobHash(hash, 'bad hash') + if (hash === Blob.EMPTY_HASH) { + // Any range of empty content is empty, so opts needs no honouring. + return new ReadableString('') + } const { bucket, key } = getBlobLocation(this.projectId, hash) try { @@ -354,6 +374,9 @@ class BlobStore { */ async getBlob(hash) { assert.blobHash(hash, 'bad hash') + if (hash === Blob.EMPTY_HASH) { + return EMPTY_BLOB + } const globalBlob = GLOBAL_BLOBS.get(hash) if (globalBlob != null) { return globalBlob.blob @@ -372,6 +395,10 @@ class BlobStore { const nonGlobalHashes = [] const blobs = [] for (const hash of hashes) { + if (hash === Blob.EMPTY_HASH) { + blobs.push(EMPTY_BLOB) + continue + } const globalBlob = GLOBAL_BLOBS.get(hash) if (globalBlob != null) { blobs.push(globalBlob.blob) diff --git a/services/history-v1/storage/lib/hash_check_blob_store.js b/services/history-v1/storage/lib/hash_check_blob_store.js index 5f233d5109..597b4b2de0 100644 --- a/services/history-v1/storage/lib/hash_check_blob_store.js +++ b/services/history-v1/storage/lib/hash_check_blob_store.js @@ -1,30 +1,52 @@ -const Blob = require('overleaf-editor-core').Blob +const { Blob, BlobStoreBase } = require('overleaf-editor-core') const blobHash = require('./blob_hash') -const BPromise = require('bluebird') // We want to simulate applying all of the operations so we can return the // resulting hashes to the caller for them to check. To do this, we need to be // able to take the lazy files in the final snapshot, fetch their content, and // compute the new content hashes. We don't, however, need to actually store // that content; we just need to get the hash. -function HashCheckBlobStore(realBlobStore) { - this.realBlobStore = realBlobStore -} - -HashCheckBlobStore.prototype.getString = BPromise.method( - function hashCheckBlobStoreGetString(hash) { - return this.realBlobStore.getString(hash) +// +// A file's ranges go through here the same way its content does: reading one +// needs getObject, which the base class builds on fetchString, and hashing one +// again needs putObject, which is putString of its JSON. Without both, a project +// holding a tracked change or a comment could not be hashed at all. +class HashCheckBlobStore extends BlobStoreBase { + /** + * @param {{ getString(hash: string): Promise }} realBlobStore + */ + constructor(realBlobStore) { + super() + this.realBlobStore = realBlobStore } -) -HashCheckBlobStore.prototype.putString = BPromise.method( - function hashCheckBlobStorePutString(string) { + /** + * @param {string} hash + * @return {Promise} + */ + async fetchString(hash) { + return await this.realBlobStore.getString(hash) + } + + /** + * @param {string} string + * @return {Promise} + */ + async putString(string) { return new Blob( blobHash.fromString(string), Buffer.byteLength(string), string.length ) } -) + + /** + * @param {object} obj + * @return {Promise} + */ + async putObject(obj) { + return await this.putString(JSON.stringify(obj)) + } +} module.exports = HashCheckBlobStore diff --git a/services/history-v1/storage/scripts/recover_zip.js b/services/history-v1/storage/scripts/recover_zip.js index 56d7613f17..820c0c0b03 100644 --- a/services/history-v1/storage/scripts/recover_zip.js +++ b/services/history-v1/storage/scripts/recover_zip.js @@ -114,8 +114,9 @@ async function loadChunk(historyPathname, blobStore) { return new core.Chunk(history, 0) } -class RecoveryBlobStore { +class RecoveryBlobStore extends core.BlobStoreBase { constructor(historyId, tmp) { + super() this.historyId = historyId this.tmp = tmp this.blobs = new Map() @@ -166,7 +167,7 @@ class RecoveryBlobStore { return new core.Blob(hash, byteLength, stringLength) } - async getString(hash) { + async fetchString(hash) { const stream = await this.getStream(hash) const buffer = await streams.readStreamToBuffer(stream) return buffer.toString() diff --git a/services/history-v1/test/acceptance/js/api/project_hashed_content.test.js b/services/history-v1/test/acceptance/js/api/project_hashed_content.test.js new file mode 100644 index 0000000000..b32514630d --- /dev/null +++ b/services/history-v1/test/acceptance/js/api/project_hashed_content.test.js @@ -0,0 +1,98 @@ +'use strict' + +const { expect } = require('chai') + +const cleanup = require('../storage/support/cleanup') +const fixtures = require('../storage/support/fixtures') +const testServer = require('./support/test_server') +const testProjects = require('./support/test_projects') + +const core = require('overleaf-editor-core') +const Change = core.Change +const File = core.File +const Operation = core.Operation + +const blobHash = require('../../../../storage/lib/blob_hash') +const persistChanges = require('../../../../storage/lib/persist_changes') + +describe('project hashed content', function () { + beforeEach(cleanup.everything) + beforeEach(fixtures.create) + + let basicAuthClient + let limitsToPersistImmediately + + before(async function () { + basicAuthClient = testServer.basicAuthClient + + const farFuture = new Date() + farFuture.setTime(farFuture.getTime() + 7 * 24 * 3600 * 1000) + limitsToPersistImmediately = { + minChangeTimestamp: farFuture, + maxChangeTimestamp: farFuture, + maxChanges: 10, + maxChunkChanges: 10, + } + }) + + it('hashes a file that carries tracked changes', async function () { + // A file with tracked changes keeps its ranges in a second blob, which it needs + // alongside its content both to be read back and to be hashed again. + const projectId = await testProjects.createEmptyProject() + const content = 'the quick brown fox' + const change = new Change( + [ + Operation.addFile( + 'tracked.tex', + File.fromRaw({ + content, + trackedChanges: [ + { + range: { pos: 4, length: 'quick '.length }, + tracking: { + type: 'delete', + ts: '2024-01-01T00:00:00.000Z', + userId: 'user1', + }, + }, + ], + }) + ), + ], + new Date(), + [] + ) + await persistChanges(projectId, [change], limitsToPersistImmediately, 0) + + const response = await basicAuthClient.apis.Project.getLatestHashedContent({ + project_id: projectId, + }) + + expect(response.status).to.equal(200) + const file = response.obj.files['tracked.tex'] + // The content is hashed as it stands, tracked delete included, and the ranges + // are hashed separately. + expect(file.hash).to.equal(blobHash.fromString(content)) + expect(file.rangesHash).to.match(/^[0-9a-f]{40}$/) + }) + + it('hashes a file that carries no ranges', async function () { + const projectId = await testProjects.createEmptyProject() + const content = 'hello world' + const change = new Change( + [Operation.addFile('main.tex', File.fromString(content))], + new Date(), + [] + ) + await persistChanges(projectId, [change], limitsToPersistImmediately, 0) + + const response = await basicAuthClient.apis.Project.getLatestHashedContent({ + project_id: projectId, + }) + + expect(response.status).to.equal(200) + const file = response.obj.files['main.tex'] + expect(file.hash).to.equal(blobHash.fromString(content)) + expect(file.rangesHash).to.be.undefined + }) +}) diff --git a/services/history-v1/test/acceptance/js/storage/batch_blob_store.test.js b/services/history-v1/test/acceptance/js/storage/batch_blob_store.test.js index 645ea5919e..430e7b493e 100644 --- a/services/history-v1/test/acceptance/js/storage/batch_blob_store.test.js +++ b/services/history-v1/test/acceptance/js/storage/batch_blob_store.test.js @@ -5,9 +5,6 @@ const { expect } = require('chai') const cleanup = require('./support/cleanup') const testFiles = require('./support/test_files') -const core = require('overleaf-editor-core') -const File = core.File - const storage = require('../../../../storage') const BatchBlobStore = storage.BatchBlobStore const BlobStore = storage.BlobStore @@ -16,6 +13,10 @@ const projectId = '123' const blobStore = new BlobStore(projectId) const batchBlobStore = new BatchBlobStore(blobStore) +// A well-formed hash that nothing has been stored under. Not the empty file's hash, +// which the blob store answers from the content it stands for rather than looking it up. +const MISSING_HASH = '0'.repeat(40) + describe('BatchBlobStore', function () { beforeEach(cleanup.everything) @@ -29,7 +30,7 @@ describe('BatchBlobStore', function () { // Cache some blobs (one that exists and another that doesn't) await batchBlobStore.preload([ testFiles.GRAPH_PNG_HASH, - File.EMPTY_FILE_HASH, // not found + MISSING_HASH, // not found ]) expect(batchBlobStore.blobs.size).to.equal(1) @@ -37,7 +38,7 @@ describe('BatchBlobStore', function () { await Promise.all([ batchBlobStore.getBlob(testFiles.GRAPH_PNG_HASH), // cached batchBlobStore.getBlob(testFiles.HELLO_TXT_HASH), // not cached; exists - batchBlobStore.getBlob(File.EMPTY_FILE_HASH), // not cached; not exists + batchBlobStore.getBlob(MISSING_HASH), // not cached; not exists batchBlobStore.getBlob(testFiles.GRAPH_PNG_HASH), // duplicate ]) diff --git a/services/history-v1/test/acceptance/js/storage/blob_store.test.js b/services/history-v1/test/acceptance/js/storage/blob_store.test.js index 531b42e400..a66bacc122 100644 --- a/services/history-v1/test/acceptance/js/storage/blob_store.test.js +++ b/services/history-v1/test/acceptance/js/storage/blob_store.test.js @@ -109,6 +109,45 @@ describe('BlobStore', function () { expect(content).to.equal(helloWorldString) }) + describe('empty content', function () { + // A read answers the hash of empty content without looking anything up: the + // hash is a property of the content, so every project has it whether or not + // anything wrote it, and a file may reference it before anything has. Writing + // it is unchanged and still stores a blob. + const emptyHash = Blob.EMPTY_HASH + + it('is fetched without anything having stored it', async function () { + const blob = await blobStore.getBlob(emptyHash) + + expect(blob.getHash()).to.equal(emptyHash) + expect(blob.getByteLength()).to.equal(0) + expect(blob.getStringLength()).to.equal(0) + expect(await blobStore.getString(emptyHash)).to.equal('') + const stream = await blobStore.getStream(emptyHash) + expect(await streams.readStreamToBuffer(stream)).to.have.length(0) + }) + + it('is fetched alongside blobs that were stored', async function () { + await blobStore.putString(helloWorldString) + + const blobs = await blobStore.getBlobs([emptyHash, helloWorldHash]) + + expect(blobs.map(blob => blob.getHash())).to.have.members([ + emptyHash, + helloWorldHash, + ]) + }) + + it('is still stored when it is written', async function () { + const blob = await blobStore.putString('') + + expect(blob.getHash()).to.equal(emptyHash) + expect( + (await blobStore.getProjectBlobs()).map(blob => blob.getHash()) + ).to.include(emptyHash) + }) + }) + it('can store and fetch utf-8 files', async function () { const testFile = 'hello.txt' diff --git a/services/history-v1/test/acceptance/js/storage/recover_zip.test.mjs b/services/history-v1/test/acceptance/js/storage/recover_zip.test.mjs index 94b9723b6b..5fd1a8c112 100644 --- a/services/history-v1/test/acceptance/js/storage/recover_zip.test.mjs +++ b/services/history-v1/test/acceptance/js/storage/recover_zip.test.mjs @@ -15,7 +15,7 @@ import ChunkStore from '../../../../storage/lib/chunk_store/index.js' import persistChanges from '../../../../storage/lib/persist_changes.js' import testFiles from '../storage/support/test_files.js' import cleanup from './support/cleanup.js' -import { getZipEntries } from './support/unzip.js' +import { getZipEntries, getZipEntryContents } from './support/unzip.js' describe('recover_zip script', function () { let projectId @@ -119,6 +119,54 @@ describe('recover_zip script', function () { } }) + it('recovers a file that carries tracked changes', async function () { + this.timeout(30 * 1000) + + // A file with tracked changes has its ranges in a second blob, which the file + // needs alongside its content to be loaded at all. The zip holds the content as + // accepting the changes would leave it, so a tracked delete is not in it. + const content = 'the quick brown fox' + const addTracked = Operation.addFile( + 'tracked.tex', + File.fromRaw({ + content, + trackedChanges: [ + { + range: { pos: 4, length: 'quick '.length }, + tracking: { + type: 'delete', + ts: '2024-01-01T00:00:00.000Z', + userId: 'user1', + }, + }, + ], + }) + ) + const change3 = new Change([addTracked], new Date(), []) + await persistChanges(projectId, [change3], limitsToPersistImmediately, 2) + + const zipPath = `/tmp/test-recover-zip-tracked-${projectId}.zip` + try { + const { stdout } = await runRecoverZipScript([projectId]) + + const urlMatch = stdout.match(/(https?:\/\/[^\s]+)/) + expect(urlMatch).to.not.be.null + const res = await fetch(urlMatch[1]) + expect(res.ok).to.be.true + await fs.promises.writeFile(zipPath, Buffer.from(await res.arrayBuffer())) + + const contents = await getZipEntryContents(zipPath) + expect([...contents.keys()].sort()).to.deep.equal([ + 'graph.png', + 'main.tex', + 'tracked.tex', + ]) + expect(contents.get('tracked.tex').toString()).to.equal('the brown fox') + } finally { + await fs.promises.unlink(zipPath).catch(() => {}) + } + }) + it('supports the --verbose flag', async function () { this.timeout(30 * 1000) diff --git a/services/history-v1/test/acceptance/js/storage/support/unzip.js b/services/history-v1/test/acceptance/js/storage/support/unzip.js index c89c3d4e62..0ba206b378 100644 --- a/services/history-v1/test/acceptance/js/storage/support/unzip.js +++ b/services/history-v1/test/acceptance/js/storage/support/unzip.js @@ -19,4 +19,35 @@ function getZipEntries(pathname) { return yauzl.openAsync(pathname).then(readEntries) } +/** + * The contents of every entry in a zip, by name. + * + * @param {string} pathname + * @return {Promise>} + */ +function getZipEntryContents(pathname) { + return yauzl.openAsync(pathname, { lazyEntries: true }).then( + zip => + new BPromise((resolve, reject) => { + const contents = new Map() + zip.on('entry', entry => { + zip.openReadStream(entry, (err, stream) => { + if (err) return reject(err) + const chunks = [] + stream.on('data', chunk => chunks.push(chunk)) + stream.on('error', reject) + stream.on('end', () => { + contents.set(entry.fileName, Buffer.concat(chunks)) + zip.readEntry() + }) + }) + }) + zip.on('error', reject) + zip.on('end', () => resolve(contents)) + zip.readEntry() + }) + ) +} + exports.getZipEntries = getZipEntries +exports.getZipEntryContents = getZipEntryContents diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 9308edb3c7..b5c775294f 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -18,6 +18,7 @@ import * as LocalFileWriter from './LocalFileWriter.js' import * as HashManager from './HashManager.js' import * as HistoryBlobTranslator from './HistoryBlobTranslator.js' import { callbackify, promisifyMultiResult } from '@overleaf/promise-utils' +import { BlobStoreBase } from 'overleaf-editor-core' const HTTP_REQUEST_TIMEOUT = Settings.overleaf.history.requestTimeout @@ -555,19 +556,15 @@ export function deleteProject(projectId, callback) { const getProjectBlobAsync = promisify(getProjectBlob) -class BlobStore { +class BlobStore extends BlobStoreBase { constructor(projectId) { + super() this.projectId = projectId } - async getString(hash) { + async fetchString(hash) { return await getProjectBlobAsync(this.projectId, hash) } - - async getObject(hash) { - const string = await this.getString(hash) - return JSON.parse(string) - } } export function getBlobStore(projectId) { diff --git a/services/web/app/src/Features/History/HistoryBlobStore.mjs b/services/web/app/src/Features/History/HistoryBlobStore.mjs index fd6736f845..383f1903e8 100644 --- a/services/web/app/src/Features/History/HistoryBlobStore.mjs +++ b/services/web/app/src/Features/History/HistoryBlobStore.mjs @@ -1,4 +1,5 @@ import { text } from 'node:stream/consumers' +import { BlobStoreBase } from 'overleaf-editor-core' import HistoryManager from './HistoryManager.mjs' /** @@ -8,11 +9,12 @@ import HistoryManager from './HistoryManager.mjs' * eagerly here -- needed for the ones with pending edit operations, whose content * is not in a single blob. */ -class HistoryBlobStore { +class HistoryBlobStore extends BlobStoreBase { /** * @param {string} historyId */ constructor(historyId) { + super() this.historyId = historyId } @@ -20,21 +22,13 @@ class HistoryBlobStore { * @param {string} hash * @return {Promise} */ - async getString(hash) { + async fetchString(hash) { const { stream } = await HistoryManager.promises.requestBlob( this.historyId, hash ) return await text(stream) } - - /** - * @param {string} hash - * @return {Promise} - */ - async getObject(hash) { - return JSON.parse(await this.getString(hash)) - } } export default HistoryBlobStore diff --git a/services/web/frontend/js/infrastructure/project-snapshot.ts b/services/web/frontend/js/infrastructure/project-snapshot.ts index 2db8d34dd1..afc9cf7ac9 100644 --- a/services/web/frontend/js/infrastructure/project-snapshot.ts +++ b/services/web/frontend/js/infrastructure/project-snapshot.ts @@ -1,5 +1,11 @@ import pLimit from 'p-limit' -import { Change, Chunk, Snapshot, File } from 'overleaf-editor-core' +import { + BlobStoreBase, + Change, + Chunk, + Snapshot, + File, +} from 'overleaf-editor-core' import type { RawChange, RawChunk } from 'overleaf-editor-core/lib/types' import { FetchError, getJSON, postJSON } from '@/infrastructure/fetch-json' import path from 'path-browserify' @@ -151,11 +157,13 @@ export class ProjectSnapshot { if (byteLength == null) { return null } - let blobStoreOptions if (options?.maxSize != null && byteLength > options?.maxSize) { - blobStoreOptions = { maxSize: options.maxSize } + const blobStoreOptions = { maxSize: options.maxSize } + // Straight to fetchString: a binary file large enough to be read with a size + // limit is never the empty content the base class answers for. + return await this.blobStore.fetchString(hash, blobStoreOptions) } - return await this.blobStore.getString(hash, blobStoreOptions) + return await this.blobStore.getString(hash) } getDocs(): Map { @@ -256,24 +264,20 @@ export class ProjectSnapshot { /** * Blob store that fetches blobs from the history service */ -export class SimpleBlobStore { +export class SimpleBlobStore extends BlobStoreBase { private projectId: string constructor(projectId: string) { + super() this.projectId = projectId } - async getString( + async fetchString( hash: string, options?: { maxSize?: number } ): Promise { return await fetchBlob(this.projectId, hash, options) } - - async getObject(hash: string) { - const blob = await this.getString(hash) - return JSON.parse(blob) - } } async function flushHistory(projectId: string) { diff --git a/services/web/test/frontend/infrastructure/project-snapshot.test.ts b/services/web/test/frontend/infrastructure/project-snapshot.test.ts index 3fa1aea527..f72e017ab7 100644 --- a/services/web/test/frontend/infrastructure/project-snapshot.test.ts +++ b/services/web/test/frontend/infrastructure/project-snapshot.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai' import fetchMock from 'fetch-mock' +import { File } from 'overleaf-editor-core' import { ProjectSnapshot } from '@/infrastructure/project-snapshot' describe('ProjectSnapshot', function () { @@ -714,4 +715,90 @@ describe('ProjectSnapshot', function () { ) }) }) + + describe('blob with the hash of empty content', function () { + // Nothing is stored under that hash, on the server or here, so the content is + // answered from the hash rather than fetched. A new empty doc references it before + // anything has written it, so a fetch would 404 and take the whole snapshot down. + + afterEach(function () { + fetchMock.removeRoutes().clearHistory() + }) + + it('loads an empty doc without fetching the blob', async function () { + const emptyChunk = { + history: { + snapshot: { files: {} }, + changes: [ + { + operations: [ + { + pathname: 'fresh.tex', + file: { hash: File.EMPTY_FILE_HASH, stringLength: 0 }, + }, + ], + timestamp: '2025-01-01T12:00:00.000Z', + }, + ], + }, + startVersion: 0, + } + + fetchMock.post(`/project/${projectId}/flush`, 200) + fetchMock.getOnce(`/project/${projectId}/latest/history`, { + chunk: emptyChunk, + }) + // Served only if the short circuit does not answer first, in which case the + // content below is what comes back. + fetchMock.get( + `/project/${projectId}/blob/${File.EMPTY_FILE_HASH}`, + 'fetched from the server' + ) + + await snapshot.refresh() + + expect(snapshot.getDocPaths()).to.have.members(['fresh.tex']) + expect(snapshot.getDocContents('fresh.tex')).to.equal('') + }) + + it('applies edits made on top of an empty doc', async function () { + const insertedText = '\\documentclass{article}\n' + const emptyChunk = { + history: { + snapshot: { files: {} }, + changes: [ + { + operations: [ + { + pathname: 'fresh.tex', + file: { hash: File.EMPTY_FILE_HASH, stringLength: 0 }, + }, + ], + timestamp: '2025-01-01T12:00:00.000Z', + }, + { + operations: [ + { pathname: 'fresh.tex', textOperation: [insertedText] }, + ], + timestamp: '2025-01-01T12:01:00.000Z', + }, + ], + }, + startVersion: 0, + } + + fetchMock.post(`/project/${projectId}/flush`, 200) + fetchMock.getOnce(`/project/${projectId}/latest/history`, { + chunk: emptyChunk, + }) + fetchMock.get( + `/project/${projectId}/blob/${File.EMPTY_FILE_HASH}`, + 'fetched from the server' + ) + + await snapshot.refresh() + + expect(snapshot.getDocContents('fresh.tex')).to.equal(insertedText) + }) + }) }) diff --git a/services/web/test/unit/src/History/HistoryBlobStore.test.mjs b/services/web/test/unit/src/History/HistoryBlobStore.test.mjs new file mode 100644 index 0000000000..a50bd0c3f7 --- /dev/null +++ b/services/web/test/unit/src/History/HistoryBlobStore.test.mjs @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import sinon from 'sinon' +import { Readable } from 'node:stream' +import { Blob } from 'overleaf-editor-core' + +const modulePath = '../../../../app/src/Features/History/HistoryBlobStore.mjs' + +const HASH = 'a1b2c3d4e5f60718293a4b5c6d7e8f9012345678' + +describe('HistoryBlobStore', function () { + beforeEach(async function (ctx) { + ctx.HistoryManager = { + promises: { requestBlob: sinon.stub() }, + } + vi.doMock( + '../../../../app/src/Features/History/HistoryManager.mjs', + () => ({ + default: ctx.HistoryManager, + }) + ) + + const HistoryBlobStore = (await import(modulePath)).default + ctx.blobStore = new HistoryBlobStore('history-id') + }) + + describe('getString', function () { + it('reads the blob of the given hash', async function (ctx) { + ctx.HistoryManager.promises.requestBlob.resolves({ + stream: Readable.from(['hello ', 'world']), + }) + + expect(await ctx.blobStore.getString(HASH)).to.equal('hello world') + expect(ctx.HistoryManager.promises.requestBlob).to.have.been.calledWith( + 'history-id', + HASH + ) + }) + + it('passes on errors from history', async function (ctx) { + ctx.HistoryManager.promises.requestBlob.rejects(new Error('boom')) + + await expect(ctx.blobStore.getString(HASH)).to.be.rejectedWith('boom') + }) + + it('answers the hash of empty content without asking history', async function (ctx) { + // Nothing is stored under it, so history-v1 answers it without a lookup of its + // own: the round-trip to ask can only be slow or fail. + ctx.HistoryManager.promises.requestBlob.resolves({ + stream: Readable.from(['fetched from history-v1']), + }) + + expect(await ctx.blobStore.getString(Blob.EMPTY_HASH)).to.equal('') + }) + + it('answers the hash of empty content when history is down', async function (ctx) { + ctx.HistoryManager.promises.requestBlob.rejects(new Error('boom')) + + expect(await ctx.blobStore.getString(Blob.EMPTY_HASH)).to.equal('') + }) + }) + + describe('getObject', function () { + it('parses the blob of the given hash as JSON', async function (ctx) { + ctx.HistoryManager.promises.requestBlob.resolves({ + stream: Readable.from(['{"comments":[]}']), + }) + + expect(await ctx.blobStore.getObject(HASH)).to.deep.equal({ + comments: [], + }) + }) + }) +})