Return a NotFoundError when filestore disabled

GitOrigin-RevId: b470d75fc621e2e52180cf923d0ee818f4ec4cb2

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,

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}`

services/web/test/unit/src/FileStore/FileStoreHandlerTests.js

@@ -464,66 +464,67 @@ describe('FileStoreHandler', function () {
   })
 
   describe('getFileStream', function () {
-    beforeEach(function () {
-      this.query = {}
-      this.request.returns(this.readStream)
-    })
+    describe('when filestore is disabled', function () {
+      beforeEach(function () {
+        this.Features.hasFeature.withArgs('filestore').returns(false)
+      })
 
-    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'
-          )
+      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 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)
+      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 add an error handler', function (done) {
-      this.handler.getFileStream(
-        this.projectId,
-        this.fileId,
-        this.query,
-        (err, stream) => {
-          if (err) {
-            return done(err)
+      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()
           }
-          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 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 a range header', function (done) {
+      it('should add an error handler', function (done) {
         this.handler.getFileStream(
           this.projectId,
           this.fileId,
@@ -532,36 +533,57 @@ 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')
+            stream.on.calledWith('error').should.equal(true)
             done()
           }
         )
       })
 
-      describe('when range is invalid', function () {
-        ;['0-', '-100', 'one-two', 'nonsense'].forEach(r => {
-          beforeEach(function () {
-            this.query = { range: `${r}` }
-          })
+      describe('when range is specified in query', function () {
+        beforeEach(function () {
+          this.query = { range: '0-10' }
+        })
 
-          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 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()
+                }
+              )
+            })
           })
         })
       })