From 4275a4a93baa341cd8661d0e6999b78bd400a048 Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Tue, 11 Mar 2025 11:20:06 +0000 Subject: [PATCH] Return a NotFoundError when filestore disabled GitOrigin-RevId: b470d75fc621e2e52180cf923d0ee818f4ec4cb2 --- .../FileStore/FileStoreController.mjs | 6 - .../Features/FileStore/FileStoreHandler.js | 6 + .../src/FileStore/FileStoreHandlerTests.js | 182 ++++++++++-------- 3 files changed, 108 insertions(+), 86 deletions(-) diff --git a/services/web/app/src/Features/FileStore/FileStoreController.mjs b/services/web/app/src/Features/FileStore/FileStoreController.mjs index 9d9a3a2f5e..e4e55c7704 100644 --- a/services/web/app/src/Features/FileStore/FileStoreController.mjs +++ b/services/web/app/src/Features/FileStore/FileStoreController.mjs @@ -67,12 +67,6 @@ async function getFile(req, res) { )) } else { // The file-hash is missing. Fall back to filestore. - if (!Features.hasFeature('filestore')) { - logger.warn( - { file, fileId, projectId }, - 'filestore feature is disabled but we will attempt to fetch the file from filestore' - ) - } stream = await FileStoreHandler.promises.getFileStream( projectId, fileId, diff --git a/services/web/app/src/Features/FileStore/FileStoreHandler.js b/services/web/app/src/Features/FileStore/FileStoreHandler.js index 663b04ed15..66ba94b37d 100644 --- a/services/web/app/src/Features/FileStore/FileStoreHandler.js +++ b/services/web/app/src/Features/FileStore/FileStoreHandler.js @@ -204,6 +204,12 @@ const FileStoreHandler = { }, getFileStream(projectId, fileId, query, callback) { + if (!Features.hasFeature('filestore')) { + return callback( + new Errors.NotFoundError('filestore is disabled, file not found') + ) + } + let queryString = '?from=getFileStream' if (query != null && query.format != null) { queryString += `&format=${query.format}` diff --git a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js index 32c3f35f5b..bf9fe6c900 100644 --- a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js +++ b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js @@ -464,66 +464,34 @@ describe('FileStoreHandler', function () { }) describe('getFileStream', function () { - beforeEach(function () { - this.query = {} - this.request.returns(this.readStream) - }) - - it('should get the stream with the correct params', function (done) { - const fileUrl = this.getFileUrl(this.projectId, this.fileId) - this.handler.getFileStream( - this.projectId, - this.fileId, - this.query, - (err, stream) => { - if (err) { - return done(err) - } - this.request.args[0][0].method.should.equal('get') - this.request.args[0][0].uri.should.equal( - fileUrl + '?from=getFileStream' - ) - done() - } - ) - }) - - it('should get stream from request', function (done) { - this.handler.getFileStream( - this.projectId, - this.fileId, - this.query, - (err, stream) => { - if (err) { - return done(err) - } - stream.should.equal(this.readStream) - done() - } - ) - }) - - it('should add an error handler', function (done) { - this.handler.getFileStream( - this.projectId, - this.fileId, - this.query, - (err, stream) => { - if (err) { - return done(err) - } - stream.on.calledWith('error').should.equal(true) - done() - } - ) - }) - - describe('when range is specified in query', function () { + describe('when filestore is disabled', function () { beforeEach(function () { - this.query = { range: '0-10' } + this.Features.hasFeature.withArgs('filestore').returns(false) }) - it('should add a range header', function (done) { + it('should callback with a NotFoundError', function (done) { + this.handler.getFileStream(this.projectId, this.fileId, {}, err => { + expect(err).to.be.instanceof(Errors.NotFoundError) + done() + }) + }) + + it('should not call request', function (done) { + this.handler.getFileStream(this.projectId, this.fileId, {}, () => { + this.request.called.should.equal(false) + done() + }) + }) + }) + describe('when filestore is enabled', function () { + beforeEach(function () { + this.query = {} + this.request.returns(this.readStream) + this.Features.hasFeature.withArgs('filestore').returns(true) + }) + + it('should get the stream with the correct params', function (done) { + const fileUrl = this.getFileUrl(this.projectId, this.fileId) this.handler.getFileStream( this.projectId, this.fileId, @@ -532,36 +500,90 @@ describe('FileStoreHandler', function () { if (err) { return done(err) } - this.request.callCount.should.equal(1) - const { headers } = this.request.firstCall.args[0] - expect(headers).to.have.keys('range') - expect(headers.range).to.equal('bytes=0-10') + this.request.args[0][0].method.should.equal('get') + this.request.args[0][0].uri.should.equal( + fileUrl + '?from=getFileStream' + ) done() } ) }) - describe('when range is invalid', function () { - ;['0-', '-100', 'one-two', 'nonsense'].forEach(r => { - beforeEach(function () { - this.query = { range: `${r}` } - }) + it('should get stream from request', function (done) { + this.handler.getFileStream( + this.projectId, + this.fileId, + this.query, + (err, stream) => { + if (err) { + return done(err) + } + stream.should.equal(this.readStream) + done() + } + ) + }) - it(`should not add a range header for '${r}'`, function (done) { - this.handler.getFileStream( - this.projectId, - this.fileId, - this.query, - (err, stream) => { - if (err) { - return done(err) - } - this.request.callCount.should.equal(1) - const { headers } = this.request.firstCall.args[0] - expect(headers).to.not.have.keys('range') - done() + it('should add an error handler', function (done) { + this.handler.getFileStream( + this.projectId, + this.fileId, + this.query, + (err, stream) => { + if (err) { + return done(err) + } + stream.on.calledWith('error').should.equal(true) + done() + } + ) + }) + + describe('when range is specified in query', function () { + beforeEach(function () { + this.query = { range: '0-10' } + }) + + it('should add a range header', function (done) { + this.handler.getFileStream( + this.projectId, + this.fileId, + this.query, + (err, stream) => { + if (err) { + return done(err) } - ) + this.request.callCount.should.equal(1) + const { headers } = this.request.firstCall.args[0] + expect(headers).to.have.keys('range') + expect(headers.range).to.equal('bytes=0-10') + done() + } + ) + }) + + describe('when range is invalid', function () { + ;['0-', '-100', 'one-two', 'nonsense'].forEach(r => { + beforeEach(function () { + this.query = { range: `${r}` } + }) + + it(`should not add a range header for '${r}'`, function (done) { + this.handler.getFileStream( + this.projectId, + this.fileId, + this.query, + (err, stream) => { + if (err) { + return done(err) + } + this.request.callCount.should.equal(1) + const { headers } = this.request.firstCall.args[0] + expect(headers).to.not.have.keys('range') + done() + } + ) + }) }) }) })