Merge pull request #22652 from overleaf/ar-handle-filestore-404-when-copying-in-saas

[web] Do not do filestore copy in SAAS

GitOrigin-RevId: 83456ca57f706246a86b997a038968aecfbae4c5

services/project-history/app/js/HistoryStoreManager.js

@@ -263,6 +263,29 @@ function _checkBlobExists(historyId, hash, callback) {
     })
 }
 
+function _rewriteFilestoreUrl(url, projectId, callback) {
+  if (!url) {
+    return { fileId: null, filestoreURL: null }
+  }
+  // Rewrite the filestore url to point to the location in the local
+  // settings for this service (this avoids problems with cross-
+  // datacentre requests when running filestore in multiple locations).
+  const { pathname: fileStorePath } = new URL(url)
+  const urlMatch = /^\/project\/([0-9a-f]{24})\/file\/([0-9a-f]{24})$/.exec(
+    fileStorePath
+  )
+  if (urlMatch == null) {
+    return callback(new OError('invalid file for blob creation'))
+  }
+  if (urlMatch[1] !== projectId) {
+    return callback(new OError('invalid project for blob creation'))
+  }
+
+  const fileId = urlMatch[2]
+  const filestoreURL = `${Settings.apis.filestore.url}/project/${projectId}/file/${fileId}`
+  return { filestoreURL, fileId }
+}
+
 export function createBlobForUpdate(projectId, historyId, update, callback) {
   callback = _.once(callback)
 
@@ -303,24 +326,15 @@ export function createBlobForUpdate(projectId, historyId, update, callback) {
         }
       }
     )
-  } else if (update.file != null && update.url != null) {
-    // Rewrite the filestore url to point to the location in the local
-    // settings for this service (this avoids problems with cross-
-    // datacentre requests when running filestore in multiple locations).
-    const { pathname: fileStorePath } = new URL(update.url)
-    const urlMatch = /^\/project\/([0-9a-f]{24})\/file\/([0-9a-f]{24})$/.exec(
-      fileStorePath
+  } else if (
+    update.file != null &&
+    (update.url != null || update.createdBlob)
+  ) {
+    const { fileId, filestoreURL } = _rewriteFilestoreUrl(
+      update.url,
+      projectId,
+      callback
     )
-    if (urlMatch == null) {
-      return callback(new OError('invalid file for blob creation'))
-    }
-    if (urlMatch[1] !== projectId) {
-      return callback(new OError('invalid project for blob creation'))
-    }
-
-    const fileId = urlMatch[2]
-    const filestoreURL = `${Settings.apis.filestore.url}/project/${projectId}/file/${fileId}`
-
     _checkBlobExists(historyId, update.hash, (err, blobExists) => {
       if (err) {
         logger.warn(
@@ -339,6 +353,13 @@ export function createBlobForUpdate(projectId, historyId, update, callback) {
           'created blob does not exist, reading from filestore'
         )
       }
+
+      if (!filestoreURL) {
+        return callback(
+          new OError('no filestore URL provided and blob was not created')
+        )
+      }
+
       fetchStream(filestoreURL, {
         signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT),
       })

services/project-history/app/js/UpdateTranslator.js

@@ -166,8 +166,8 @@ function _isAddFileUpdate(update) {
   return (
     'file' in update &&
     update.file != null &&
-    'url' in update &&
-    update.url != null
+    (('createdBlob' in update && update.createdBlob) ||
+      ('url' in update && update.url != null))
   )
 }
 

services/project-history/test/acceptance/js/SendingUpdatesTests.js

@@ -66,6 +66,19 @@ function slAddFileUpdate(historyId, file, userId, ts, projectId) {
   }
 }
 
+function createdBlobFileUpdate(historyId, file, userId, ts, projectId) {
+  return {
+    projectHistoryId: historyId,
+    pathname: file.pathname,
+    createdBlob: true,
+    url: null,
+    file: file.id,
+    hash: file.hash,
+    ranges: undefined,
+    meta: { user_id: userId, ts: ts.getTime() },
+  }
+}
+
 function slRenameUpdate(historyId, doc, userId, ts, pathname, newPathname) {
   return {
     projectHistoryId: historyId,
@@ -554,6 +567,73 @@ describe('Sending Updates', function () {
       )
     })
 
+    it('should not get file from filestore if no url provided', function (done) {
+      const file = {
+        id: new ObjectId().toString(),
+        pathname: '/test.png',
+        contents: Buffer.from([1, 2, 3]),
+        hash: 'aed2973e4b8a7ff1b30ff5c4751e5a2b38989e74',
+      }
+
+      const fileStoreRequest = MockFileStore()
+        .get(`/project/${this.projectId}/file/${file.id}`)
+        .reply(200, file.contents)
+
+      const checkBlob = MockHistoryStore()
+        .head(`/api/projects/${historyId}/blobs/${file.hash}`)
+        .reply(200)
+
+      const addFile = MockHistoryStore()
+        .post(`/api/projects/${historyId}/legacy_changes`, body => {
+          expect(body).to.deep.equal([
+            olAddFileUpdate(file, this.userId, this.timestamp, file.hash),
+          ])
+          return true
+        })
+        .query({ end_version: 0 })
+        .reply(204)
+
+      async.series(
+        [
+          cb => {
+            ProjectHistoryClient.pushRawUpdate(
+              this.projectId,
+              createdBlobFileUpdate(
+                historyId,
+                file,
+                this.userId,
+                this.timestamp,
+                this.projectId
+              ),
+              cb
+            )
+          },
+          cb => {
+            ProjectHistoryClient.flushProject(this.projectId, cb)
+          },
+        ],
+        error => {
+          if (error) {
+            return done(error)
+          }
+          assert(
+            !fileStoreRequest.isDone(),
+            'filestore should not have been called'
+          )
+
+          assert(
+            checkBlob.isDone(),
+            `HEAD /api/projects/${historyId}/blobs/${file.hash} should have been called`
+          )
+          assert(
+            addFile.isDone(),
+            `/api/projects/${historyId}/latest/files should have been called`
+          )
+          done()
+        }
+      )
+    })
+
     it('should send add file updates to the history store', function (done) {
       const file = {
         id: new ObjectId().toString(),

services/web/app/src/Features/Project/ProjectDuplicator.js

@@ -20,6 +20,7 @@ const SafePath = require('./SafePath')
 const TpdsProjectFlusher = require('../ThirdPartyDataStore/TpdsProjectFlusher')
 const _ = require('lodash')
 const TagsHandler = require('../Tags/TagsHandler')
+const Features = require('../../infrastructure/Features')
 
 module.exports = {
   duplicate: callbackify(duplicate),
@@ -233,12 +234,16 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) {
           )
         }
       }
+      if (createdBlob && Features.hasFeature('saas')) {
+        return { createdBlob, file, path, url: null }
+      }
       const url = await FileStoreHandler.promises.copyFile(
         sourceProject._id,
         sourceFile._id,
         targetProject._id,
         file._id
       )
+
       return { createdBlob, file, path, url }
     }
   )

services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs

@@ -8,6 +8,7 @@ import _ from 'lodash'
 import ProjectGetter from '../../../../../app/src/Features/Project/ProjectGetter.js'
 import User from '../../../../../test/acceptance/src/helpers/User.js'
 import MockDocUpdaterApiClass from '../../../../../test/acceptance/src/mocks/MockDocUpdaterApi.js'
+import Features from '../../../../../app/src/infrastructure/Features.js'
 
 const { ObjectId } = mongodb
 
@@ -256,7 +257,11 @@ describe('ProjectStructureChanges', function () {
       expect(updates[2].type).to.equal('add-file')
       expect(updates[2].userId).to.equal(owner._id)
       expect(updates[2].pathname).to.equal('/frog.jpg')
-      expect(updates[2].url).to.be.a('string')
+      if (Features.hasFeature('saas')) {
+        expect(updates[2].url).to.be.null
+      } else {
+        expect(updates[2].url).to.be.a('string')
+      }
       expect(version).to.equal(1)
     })
   })

services/web/test/unit/src/Project/ProjectDuplicatorTests.js

@@ -120,7 +120,7 @@ describe('ProjectDuplicator', function () {
         createdBlob: true,
         path: this.file2Path,
         file: this.newFile2,
-        url: this.filestoreUrl,
+        url: null,
       },
     ]
 
@@ -304,7 +304,7 @@ describe('ProjectDuplicator', function () {
     })
 
     it('should copy files to the filestore', function () {
-      for (const file of [this.file0, this.file1, this.file2]) {
+      for (const file of [this.file0, this.file1]) {
         this.FileStoreHandler.promises.copyFile.should.have.been.calledWith(
           this.project._id,
           file._id,
@@ -314,6 +314,15 @@ describe('ProjectDuplicator', function () {
       }
     })
 
+    it('should not copy files that have been sent to history-v1 to the filestore', function () {
+      this.FileStoreHandler.promises.copyFile.should.not.have.been.calledWith(
+        this.project._id,
+        this.file2._id,
+        this.newProject._id,
+        this.newFileId
+      )
+    })
+
     it('should create a blank project', function () {
       this.ProjectCreationHandler.promises.createBlankProject.should.have.been.calledWith(
         this.owner._id,