Merge pull request #28533 from overleaf/mj-blob-ranges-maxsize

[web+history-v1] Use range request for binary blobs in project snapshot

GitOrigin-RevId: e72a2d481bf163b3e85361b42e30e82cf867f1cf

services/web/test/frontend/infrastructure/project-snapshot.test.ts

@@ -44,6 +44,10 @@ describe('ProjectSnapshot', function () {
         ), // 6.5MB
       hash: 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee',
     },
+    'empty.png': {
+      contents: '',
+      hash: 'ffffffffffffffffffffffffffffffffffffffff',
+    },
   }
 
   const chunk = {
@@ -82,6 +86,13 @@ describe('ProjectSnapshot', function () {
                 byteLength: files['bibliography.bib'].contents.length,
               },
             },
+            {
+              pathname: 'empty.png',
+              file: {
+                hash: files['empty.png'].hash,
+                byteLength: files['empty.png'].contents.length,
+              },
+            },
           ],
           timestamp: '2025-01-01T12:00:00.000Z',
         },
@@ -145,10 +156,24 @@ describe('ProjectSnapshot', function () {
     })
   }
 
+  // fetch-mock doesn't seem to expose the header to the response function,
+  // so we just use a constant here
+  const MOCKED_MAX_SIZE = 100
+
   function mockBlobs(paths = Object.keys(files) as (keyof typeof files)[]) {
     for (const path of paths) {
       const file = files[path]
-      fetchMock.get(`/project/${projectId}/blob/${file.hash}`, file.contents)
+      fetchMock
+        .get({
+          url: `/project/${projectId}/blob/${file.hash}`,
+          missingHeaders: ['Range'],
+          response: file.contents,
+        })
+        .get({
+          url: `/project/${projectId}/blob/${file.hash}`,
+          headers: { Range: `bytes=0-${MOCKED_MAX_SIZE - 1}` },
+          response: file.contents.slice(0, MOCKED_MAX_SIZE),
+        })
     }
   }
 
@@ -243,13 +268,18 @@ describe('ProjectSnapshot', function () {
             hash: files['bibliography.bib'].hash,
             size: files['bibliography.bib'].contents.length,
           },
+          {
+            path: 'empty.png',
+            hash: 'ffffffffffffffffffffffffffffffffffffffff',
+            size: 0,
+          },
         ])
       })
     })
 
     describe('getBinaryFileContents', function () {
       beforeEach(function () {
-        mockBlobs(['bibliography.bib'])
+        mockBlobs(['bibliography.bib', 'empty.png'])
       })
 
       it('can fetch whole file', async function () {
@@ -257,14 +287,20 @@ describe('ProjectSnapshot', function () {
         expect(blob).to.equal(files['bibliography.bib'].contents)
       })
 
-      // NOTE: fetch-mock does not support the .response.body.pipeThrough API,
-      // so this test is skipped for now.
-      // eslint-disable-next-line mocha/no-skipped-tests
-      it.skip('can fetch part of file', async function () {
+      it('can fetch part of file', async function () {
         const blob = await snapshot.getBinaryFileContents('bibliography.bib', {
-          maxSize: 100,
+          maxSize: MOCKED_MAX_SIZE,
         })
-        expect(blob).to.equal(files['bibliography.bib'].contents.slice(0, 100))
+        expect(blob).to.equal(
+          files['bibliography.bib'].contents.slice(0, MOCKED_MAX_SIZE)
+        )
+      })
+
+      it('can fetch empty file with maxSize', async function () {
+        const blob = await snapshot.getBinaryFileContents('empty.png', {
+          maxSize: 200,
+        })
+        expect(blob).to.equal(files['empty.png'].contents)
       })
     })
   })

services/web/test/unit/src/History/HistoryController.test.mjs

@@ -32,6 +32,7 @@ describe('HistoryController', function () {
     ctx.HistoryManager = {
       promises: {
         injectUserDetails: sinon.stub(),
+        requestBlobWithProjectId: sinon.stub(),
       },
     }
 
@@ -297,4 +298,81 @@ describe('HistoryController', function () {
       })
     })
   })
+
+  describe('requestBlob', function () {
+    describe('With Range header', function () {
+      beforeEach(async function (ctx) {
+        ctx.req = {
+          params: { project_id: ctx.project_id, hash: 'foo' },
+          body: {},
+          get: sinon.stub(),
+        }
+        ctx.req.get.withArgs('Range').returns('bytes=0-42')
+        ctx.res = { setHeader: sinon.stub(), status: sinon.stub() }
+        ctx.HistoryManager.promises.requestBlobWithProjectId.resolves({
+          stream: null,
+          contentLength: '43',
+          contentRange: 'bytes 0-42/100',
+        })
+        await ctx.HistoryController.getBlob(ctx.req, ctx.res, ctx.next)
+      })
+
+      it('should forward the range request', function (ctx) {
+        ctx.HistoryManager.promises.requestBlobWithProjectId.should.have.been.calledWith(
+          ctx.project_id,
+          'foo',
+          'GET',
+          'bytes=0-42'
+        )
+      })
+
+      it('should forward the Content-Range header', function (ctx) {
+        ctx.res.setHeader.should.have.been.calledWith(
+          'Content-Range',
+          'bytes 0-42/100'
+        )
+      })
+
+      it('should forward the Content-Length header', function (ctx) {
+        ctx.res.setHeader.should.have.been.calledWith('Content-Length', '43')
+      })
+
+      it('should have status 206', function (ctx) {
+        ctx.res.status.should.have.been.calledWith(206)
+      })
+    })
+
+    describe('Without Range header', function () {
+      beforeEach(async function (ctx) {
+        ctx.req = {
+          params: { project_id: ctx.project_id, hash: 'foo' },
+          body: {},
+          get: sinon.stub(),
+        }
+        ctx.req.get.withArgs('Range').returns(null)
+        ctx.res = { setHeader: sinon.stub(), status: sinon.stub() }
+        ctx.HistoryManager.promises.requestBlobWithProjectId.resolves({
+          stream: null,
+          contentLength: '100',
+          range: null,
+        })
+        await ctx.HistoryController.getBlob(ctx.req, ctx.res, ctx.next)
+      })
+
+      it('should not have a Content-Range header', function (ctx) {
+        expect(ctx.res.setHeader).to.not.have.been.calledWith(
+          'Content-Range',
+          sinon.match.string
+        )
+      })
+
+      it('should forward the Content-Length header', function (ctx) {
+        ctx.res.setHeader.should.have.been.calledWith('Content-Length', '100')
+      })
+
+      it('should not have status 206', function (ctx) {
+        ctx.res.status.should.not.have.been.calledWith(206)
+      })
+    })
+  })
 })