From 71709bf2b0baaade8f59a177577b59098ef3bd6c Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 7 Feb 2025 08:38:49 +0000 Subject: [PATCH] [web] restore downloading from history-v1 via legacy file endpoint (#23450) GitOrigin-RevId: f6d12cfe445c7c41a62a563c9a5e7089bf94344f --- .../FileStore/FileStoreController.mjs | 23 +-- .../web/test/acceptance/src/HistoryTests.mjs | 139 +++++++++++++----- .../FileStore/FileStoreControllerTests.mjs | 7 +- .../web/test/unit/src/helpers/MockResponse.js | 8 + 4 files changed, 132 insertions(+), 45 deletions(-) diff --git a/services/web/app/src/Features/FileStore/FileStoreController.mjs b/services/web/app/src/Features/FileStore/FileStoreController.mjs index 8580dcd521..e4e55c7704 100644 --- a/services/web/app/src/Features/FileStore/FileStoreController.mjs +++ b/services/web/app/src/Features/FileStore/FileStoreController.mjs @@ -20,11 +20,11 @@ async function getFile(req, res) { let file try { - file = await ProjectLocator.promises.findElement({ + ;({ element: file } = await ProjectLocator.promises.findElement({ project_id: projectId, element_id: fileId, type: 'file', - }) + })) } catch (err) { if (err instanceof Errors.NotFoundError) { logger.warn( @@ -55,11 +55,11 @@ async function getFile(req, res) { status: Boolean(file?.hash), }) - let stream, contentLength + let source, stream, contentLength try { if (Features.hasFeature('project-history-blobs') && file?.hash) { // Get the file from history - ;({ stream, contentLength } = + ;({ source, stream, contentLength } = await HistoryManager.promises.requestBlobWithFallback( projectId, file.hash, @@ -72,6 +72,7 @@ async function getFile(req, res) { fileId, queryString ) + source = 'filestore' } } catch (err) { if (err instanceof Errors.NotFoundError) { @@ -96,6 +97,7 @@ async function getFile(req, res) { // allow the browser to cache these immutable files // note: both "private" and "max-age" appear to be required for caching res.setHeader('Cache-Control', 'private, max-age=3600') + res.appendHeader('X-Served-By', source) try { await pipeline(stream, res) } catch (err) { @@ -117,11 +119,11 @@ async function getFileHead(req, res) { let file try { - file = await ProjectLocator.promises.findElement({ + ;({ element: file } = await ProjectLocator.promises.findElement({ project_id: projectId, element_id: fileId, type: 'file', - }) + })) } catch (err) { if (err instanceof Errors.NotFoundError) { // res.sendStatus() sends a description of the status as body. @@ -148,19 +150,19 @@ async function getFileHead(req, res) { status: Boolean(file?.hash), }) - let fileSize + let fileSize, source try { if (Features.hasFeature('project-history-blobs') && file?.hash) { - const { contentLength } = + ;({ source, contentLength: fileSize } = await HistoryManager.promises.requestBlobWithFallback( projectId, file.hash, fileId, 'HEAD' - ) - fileSize = contentLength + )) } else { fileSize = await FileStoreHandler.promises.getFileSize(projectId, fileId) + source = 'filestore' } } catch (err) { if (err instanceof Errors.NotFoundError) { @@ -172,6 +174,7 @@ async function getFileHead(req, res) { } res.setHeader('Content-Length', fileSize) + res.appendHeader('X-Served-By', source) res.status(200).end() } diff --git a/services/web/test/acceptance/src/HistoryTests.mjs b/services/web/test/acceptance/src/HistoryTests.mjs index 5d62e070fc..9ac8adfe3a 100644 --- a/services/web/test/acceptance/src/HistoryTests.mjs +++ b/services/web/test/acceptance/src/HistoryTests.mjs @@ -26,7 +26,7 @@ const fileContent = fs.readFileSync( ) describe('HistoryTests', function () { - let user, projectId, fileId, fileHash, fileURL, fileURLWithFallback + let user, projectId, fileId, fileHash, fileURL, blobURL, blobURLWithFallback let historySource, filestoreSource async function getSourceMetric(source) { @@ -48,8 +48,9 @@ describe('HistoryTests', function () { '2pixel.png', 'image/png' )) - fileURL = `/project/${projectId}/blob/${fileHash}` - fileURLWithFallback = `${fileURL}?fallback=${fileId}` + fileURL = `/project/${projectId}/file/${fileId}` + blobURL = `/project/${projectId}/blob/${fileHash}` + blobURLWithFallback = `${blobURL}?fallback=${fileId}` historySource = await getSourceMetric('history-v1') filestoreSource = await getSourceMetric('filestore') }) @@ -108,7 +109,7 @@ describe('HistoryTests', function () { describe('HEAD', function () { if (Features.hasFeature('project-history-blobs')) { it('should fetch the file size from history-v1', async function () { - const { response } = await user.doRequest('HEAD', fileURL) + const { response } = await user.doRequest('HEAD', blobURL) expect(response.statusCode).to.equal(200) expect(response.headers['x-served-by']).to.include('history-v1') expect(response.headers['content-length']).to.equal('3694') @@ -117,13 +118,13 @@ describe('HistoryTests', function () { } it('should return 404 without fallback', async function () { MockV1HistoryApi.reset() - const { response } = await user.doRequest('HEAD', fileURL) + const { response } = await user.doRequest('HEAD', blobURL) expect(response.statusCode).to.equal(404) await expectNoIncrement() }) it('should fetch the file size from filestore when missing in history-v1', async function () { MockV1HistoryApi.reset() - const { response } = await user.doRequest('HEAD', fileURLWithFallback) + const { response } = await user.doRequest('HEAD', blobURLWithFallback) expect(response.statusCode).to.equal(200) expect(response.headers['x-served-by']).to.include('filestore') expect(response.headers['content-length']).to.equal('3694') @@ -132,11 +133,97 @@ describe('HistoryTests', function () { it('should return 404 with both files missing', async function () { MockFilestoreApi.reset() MockV1HistoryApi.reset() - const { response } = await user.doRequest('HEAD', fileURLWithFallback) + const { response } = await user.doRequest('HEAD', blobURLWithFallback) expect(response.statusCode).to.equal(404) await expectNoIncrement() }) }) + describe('GET', function () { + if (Features.hasFeature('project-history-blobs')) { + it('should fetch the file from history-v1', async function () { + const { response, body } = await user.doRequest('GET', blobURL) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('history-v1') + expect(body).to.equal(fileContent) + await expectHistoryV1Hit() + }) + it('should set cache headers', async function () { + const { response } = await user.doRequest('GET', blobURL) + expect(response.headers['cache-control']).to.equal( + 'private, max-age=86400, stale-while-revalidate=31536000' + ) + expect(response.headers.etag).to.equal(fileHash) + }) + it('should return a 304 when revalidating', async function () { + const { response, body } = await user.doRequest('GET', { + url: blobURL, + headers: { 'If-None-Match': fileHash }, + }) + expect(response.statusCode).to.equal(304) + expect(response.headers.etag).to.equal(fileHash) + expect(body).to.equal('') + }) + } + it('should return 404 without fallback', async function () { + MockV1HistoryApi.reset() + const { response } = await user.doRequest('GET', blobURL) + expect(response.statusCode).to.equal(404) + await expectNoIncrement() + }) + it('should not set cache headers on 404', async function () { + MockV1HistoryApi.reset() + const { response } = await user.doRequest('GET', blobURL) + expect(response.statusCode).to.equal(404) + expect(response.headers).not.to.have.property('cache-control') + expect(response.headers).not.to.have.property('etag') + }) + it('should fetch the file size from filestore when missing in history-v1', async function () { + MockV1HistoryApi.reset() + const { response, body } = await user.doRequest( + 'GET', + blobURLWithFallback + ) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('filestore') + expect(body).to.equal(fileContent) + await expectFilestoreHit() + }) + it('should return 404 with both files missing', async function () { + MockFilestoreApi.reset() + MockV1HistoryApi.reset() + const { response } = await user.doRequest('GET', blobURLWithFallback) + expect(response.statusCode).to.equal(404) + await expectNoIncrement() + }) + }) + }) + + // Legacy endpoint that is powered by history-v1 in SaaS + describe('/project/:projectId/file/:fileId', function () { + describe('HEAD', function () { + if (Features.hasFeature('project-history-blobs')) { + it('should fetch the file size from history-v1', async function () { + const { response } = await user.doRequest('HEAD', fileURL) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('history-v1') + expect(response.headers['content-length']).to.equal('3694') + await expectHistoryV1Hit() + }) + } + it('should fetch the file size from filestore when missing in history-v1', async function () { + MockV1HistoryApi.reset() + const { response } = await user.doRequest('HEAD', blobURLWithFallback) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('filestore') + expect(response.headers['content-length']).to.equal('3694') + }) + it('should return 404 with both files missing', async function () { + MockFilestoreApi.reset() + MockV1HistoryApi.reset() + const { response } = await user.doRequest('HEAD', blobURLWithFallback) + expect(response.statusCode).to.equal(404) + }) + }) describe('GET', function () { if (Features.hasFeature('project-history-blobs')) { it('should fetch the file from history-v1', async function () { @@ -146,52 +233,36 @@ describe('HistoryTests', function () { expect(body).to.equal(fileContent) await expectHistoryV1Hit() }) - it('should set cache headers', async function () { - const { response } = await user.doRequest('GET', fileURL) - expect(response.headers['cache-control']).to.equal( - 'private, max-age=86400, stale-while-revalidate=31536000' - ) - expect(response.headers.etag).to.equal(fileHash) - }) - it('should return a 304 when revalidating', async function () { - const { response, body } = await user.doRequest('GET', { - url: fileURL, - headers: { 'If-None-Match': fileHash }, - }) - expect(response.statusCode).to.equal(304) - expect(response.headers.etag).to.equal(fileHash) - expect(body).to.equal('') - }) } - it('should return 404 without fallback', async function () { - MockV1HistoryApi.reset() + it('should set cache headers', async function () { const { response } = await user.doRequest('GET', fileURL) - expect(response.statusCode).to.equal(404) - await expectNoIncrement() + expect(response.headers['cache-control']).to.equal( + 'private, max-age=3600' + ) }) it('should not set cache headers on 404', async function () { MockV1HistoryApi.reset() + MockFilestoreApi.reset() + // The legacy filestore downloads are not properly handling 404s, so delete the file from the file-tree to trigger the 404. All the filestore code will be removed soon. + await user.doRequest('DELETE', fileURL) + const { response } = await user.doRequest('GET', fileURL) + expect(response.statusCode).to.equal(404) expect(response.headers).not.to.have.property('cache-control') expect(response.headers).not.to.have.property('etag') }) it('should fetch the file size from filestore when missing in history-v1', async function () { MockV1HistoryApi.reset() - const { response, body } = await user.doRequest( - 'GET', - fileURLWithFallback - ) + const { response, body } = await user.doRequest('GET', fileURL) expect(response.statusCode).to.equal(200) expect(response.headers['x-served-by']).to.include('filestore') expect(body).to.equal(fileContent) - await expectFilestoreHit() }) it('should return 404 with both files missing', async function () { MockFilestoreApi.reset() MockV1HistoryApi.reset() - const { response } = await user.doRequest('GET', fileURLWithFallback) + const { response } = await user.doRequest('GET', fileURL) expect(response.statusCode).to.equal(404) - await expectNoIncrement() }) }) }) diff --git a/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs b/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs index 4d11e88007..2758068ce3 100644 --- a/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs +++ b/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs @@ -9,6 +9,7 @@ const MODULE_PATH = const expectedFileHeaders = { 'Cache-Control': 'private, max-age=3600', + 'X-Served-By': 'filestore', } describe('FileStoreController', function () { @@ -56,7 +57,7 @@ describe('FileStoreController', function () { describe('getFile', function () { beforeEach(function () { this.FileStoreHandler.promises.getFileStream.resolves(this.stream) - this.ProjectLocator.promises.findElement.resolves(this.file) + this.ProjectLocator.promises.findElement.resolves({ element: this.file }) }) it('should call the file store handler with the project_id file_id and any query string', async function () { @@ -193,6 +194,10 @@ describe('FileStoreController', function () { }) describe('getFileHead', function () { + beforeEach(function () { + this.ProjectLocator.promises.findElement.resolves({ element: this.file }) + }) + it('reports the file size', function (done) { const expectedFileSize = 99393 this.FileStoreHandler.promises.getFileSize.rejects( diff --git a/services/web/test/unit/src/helpers/MockResponse.js b/services/web/test/unit/src/helpers/MockResponse.js index 1fbac41a0c..80e7ae98af 100644 --- a/services/web/test/unit/src/helpers/MockResponse.js +++ b/services/web/test/unit/src/helpers/MockResponse.js @@ -139,6 +139,14 @@ class MockResponse { this.header(header, value) } + appendHeader(header, value) { + if (this.headers[header]) { + this.headers[header] += `, ${value}` + } else { + this.headers[header] = value + } + } + setTimeout(timout) { this.timout = timout }