Merge pull request #8135 from overleaf/jpa-refactor-zonal-download

[misc] refactor handling of zone prefix in compile response

GitOrigin-RevId: f1f33d7d257854176f383bb5d786710f6b09f737

services/clsi/app/js/CompileController.js

@@ -122,10 +122,11 @@ module.exports = CompileController = {
                   error: (error != null ? error.message : undefined) || error,
                   stats,
                   timings,
+                  outputUrlPrefix: Settings.apis.clsi.outputUrlPrefix,
                   outputFiles: outputFiles.map(file => {
                     return {
                       url:
-                        `${Settings.apis.clsi.outputUrl}/project/${request.project_id}` +
+                        `${Settings.apis.clsi.url}/project/${request.project_id}` +
                         (request.user_id != null
                           ? `/user/${request.user_id}`
                           : '') +

services/clsi/config/settings.defaults.js

@@ -37,9 +37,7 @@ module.exports = {
       // Internal requests (used by tests only at the time of writing).
       url: `http://${process.env.CLSI_HOST || 'localhost'}:3013`,
       // External url prefix for output files, e.g. for requests via load-balancers.
-      outputUrl: `http://${process.env.CLSI_HOST || 'localhost'}:3013${
-        process.env.ZONE ? `/zone/${process.env.ZONE}` : ''
-      }`,
+      outputUrlPrefix: `${process.env.ZONE ? `/zone/${process.env.ZONE}` : ''}`,
     },
     clsiPerf: {
       host: `${process.env.CLSI_PERF_HOST || 'localhost'}:${

services/clsi/test/unit/js/CompileControllerTests.js

@@ -69,7 +69,8 @@ describe('CompileController', function () {
         '@overleaf/settings': (this.Settings = {
           apis: {
             clsi: {
-              outputUrl: 'http://clsi.example.com',
+              url: 'http://clsi.example.com',
+              outputUrlPrefix: '/zone/b',
             },
           },
         }),
@@ -155,9 +156,41 @@ describe('CompileController', function () {
               error: null,
               stats: this.stats,
               timings: this.timings,
+              outputUrlPrefix: '/zone/b',
               outputFiles: this.output_files.map(file => {
                 return {
-                  url: `${this.Settings.apis.clsi.outputUrl}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
+                  url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
+                  ...file,
+                }
+              }),
+            },
+          })
+          .should.equal(true)
+      })
+    })
+
+    describe('without a outputUrlPrefix', function () {
+      beforeEach(function () {
+        this.Settings.apis.clsi.outputUrlPrefix = ''
+        this.CompileManager.doCompileWithLock = sinon
+          .stub()
+          .yields(null, this.output_files, this.stats, this.timings)
+        this.CompileController.compile(this.req, this.res)
+      })
+
+      it('should return the JSON response with empty outputUrlPrefix', function () {
+        this.res.status.calledWith(200).should.equal(true)
+        this.res.send
+          .calledWith({
+            compile: {
+              status: 'success',
+              error: null,
+              stats: this.stats,
+              timings: this.timings,
+              outputUrlPrefix: '',
+              outputFiles: this.output_files.map(file => {
+                return {
+                  url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
                   ...file,
                 }
               }),
@@ -196,9 +229,10 @@ describe('CompileController', function () {
               error: null,
               stats: this.stats,
               timings: this.timings,
+              outputUrlPrefix: '/zone/b',
               outputFiles: this.output_files.map(file => {
                 return {
-                  url: `${this.Settings.apis.clsi.outputUrl}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
+                  url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
                   ...file,
                 }
               }),
@@ -238,9 +272,10 @@ describe('CompileController', function () {
               error: null,
               stats: this.stats,
               timings: this.timings,
+              outputUrlPrefix: '/zone/b',
               outputFiles: this.output_files.map(file => {
                 return {
-                  url: `${this.Settings.apis.clsi.outputUrl}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
+                  url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`,
                   ...file,
                 }
               }),
@@ -265,6 +300,7 @@ describe('CompileController', function () {
             compile: {
               status: 'error',
               error: this.message,
+              outputUrlPrefix: '/zone/b',
               outputFiles: [],
               // JSON.stringify will omit these
               stats: undefined,
@@ -292,6 +328,7 @@ describe('CompileController', function () {
             compile: {
               status: 'timedout',
               error: this.message,
+              outputUrlPrefix: '/zone/b',
               outputFiles: [],
               // JSON.stringify will omit these
               stats: undefined,
@@ -317,6 +354,7 @@ describe('CompileController', function () {
             compile: {
               error: null,
               status: 'failure',
+              outputUrlPrefix: '/zone/b',
               outputFiles: [],
               // JSON.stringify will omit these
               stats: undefined,

services/web/app/src/Features/Compile/ClsiManager.js

@@ -254,6 +254,7 @@ const ClsiManager = {
             const status = compile.status
             const stats = compile.stats
             const timings = compile.timings
+            const outputUrlPrefix = compile.outputUrlPrefix
             const validationProblems = undefined
             callback(
               null,
@@ -262,7 +263,8 @@ const ClsiManager = {
               clsiServerId,
               validationProblems,
               stats,
-              timings
+              timings,
+              outputUrlPrefix
             )
           }
         )

services/web/app/src/Features/Compile/CompileController.js

@@ -87,7 +87,8 @@ module.exports = CompileController = {
         limits,
         validationProblems,
         stats,
-        timings
+        timings,
+        outputUrlPrefix
       ) => {
         if (error) {
           Metrics.inc('compile-error')
@@ -100,11 +101,20 @@ module.exports = CompileController = {
           'zonal-clsi-lb-downloads',
           {},
           (_err, assignment) => {
-            if (assignment?.variant !== 'zonal' && Array.isArray(outputFiles)) {
+            if (Array.isArray(outputFiles)) {
+              // NOTE: keep this around as a safeguard for rolling back clsi.
               outputFiles.forEach(file => {
                 file.url = file.url.replace(/^\/zone\/\w/, '')
               })
             }
+            let pdfDownloadDomain = Settings.pdfDownloadDomain
+            if (
+              assignment?.variant === 'zonal' &&
+              pdfDownloadDomain &&
+              outputUrlPrefix
+            ) {
+              pdfDownloadDomain += outputUrlPrefix
+            }
             res.json({
               status,
               outputFiles,
@@ -113,7 +123,7 @@ module.exports = CompileController = {
               validationProblems,
               stats,
               timings,
-              pdfDownloadDomain: Settings.pdfDownloadDomain,
+              pdfDownloadDomain,
             })
           }
         )

services/web/app/src/Features/Compile/CompileManager.js

@@ -96,7 +96,8 @@ module.exports = CompileManager = {
                             clsiServerId,
                             validationProblems,
                             stats,
-                            timings
+                            timings,
+                            outputUrlPrefix
                           ) {
                             if (error != null) {
                               return callback(error)
@@ -109,7 +110,8 @@ module.exports = CompileManager = {
                               limits,
                               validationProblems,
                               stats,
-                              timings
+                              timings,
+                              outputUrlPrefix
                             )
                           }
                         )

services/web/test/unit/src/Compile/CompileControllerTests.js

@@ -93,7 +93,7 @@ describe('CompileController', function () {
         (this.outputFiles = [
           {
             path: 'output.pdf',
-            url: `/zone/b/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
+            url: `/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
             type: 'pdf',
           },
         ])
@@ -101,6 +101,65 @@ describe('CompileController', function () {
     })
 
     describe('zonal downloads', function () {
+      beforeEach(function () {
+        this.settings.pdfDownloadDomain = 'https://compiles.overleaf.test'
+        this.CompileManager.compile = sinon.stub().callsArgWith(
+          3,
+          null,
+          (this.status = 'success'),
+          (this.outputFiles = [
+            {
+              path: 'output.pdf',
+              url: `/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
+              type: 'pdf',
+            },
+          ]),
+          undefined, // clsiServerId
+          undefined, // limits
+          undefined, // validationProblems
+          undefined, // stats
+          undefined, // timings
+          '/zone/b'
+        )
+      })
+
+      describe('when in the default split test variant and with the old clsi deploy', function () {
+        beforeEach(function () {
+          this.getAssignment.yields(null, { variant: 'default' })
+          this.CompileManager.compile = sinon.stub().callsArgWith(
+            3,
+            null,
+            (this.status = 'success'),
+            (this.outputFiles = [
+              {
+                path: 'output.pdf',
+                // The previous clsi version sent the zone prefix in the url
+                url: `/zone/b/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
+                type: 'pdf',
+              },
+            ])
+          )
+          this.CompileController.compile(this.req, this.res, this.next)
+        })
+
+        it('should remove the zone prefix', function () {
+          this.res.statusCode.should.equal(200)
+          this.res.body.should.equal(
+            JSON.stringify({
+              status: this.status,
+              outputFiles: [
+                {
+                  path: 'output.pdf',
+                  url: `/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
+                  type: 'pdf',
+                },
+              ],
+              pdfDownloadDomain: 'https://compiles.overleaf.test',
+            })
+          )
+        })
+      })
+
       describe('when in the default split test variant and not output files were returned', function () {
         beforeEach(function () {
           this.getAssignment.yields(null, { variant: 'default' })
@@ -121,6 +180,7 @@ describe('CompileController', function () {
             JSON.stringify({
               status: this.status,
               outputFiles: null,
+              pdfDownloadDomain: 'https://compiles.overleaf.test',
             })
           )
         })
@@ -144,6 +204,7 @@ describe('CompileController', function () {
                   type: 'pdf',
                 },
               ],
+              pdfDownloadDomain: 'https://compiles.overleaf.test',
             })
           )
         })
@@ -155,7 +216,7 @@ describe('CompileController', function () {
           this.CompileController.compile(this.req, this.res, this.next)
         })
 
-        it('should keep the zone prefix', function () {
+        it('should add the zone prefix', function () {
           this.res.statusCode.should.equal(200)
           this.res.body.should.equal(
             JSON.stringify({
@@ -163,10 +224,11 @@ describe('CompileController', function () {
               outputFiles: [
                 {
                   path: 'output.pdf',
-                  url: `/zone/b/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
+                  url: `/project/${this.project_id}/user/${this.user_id}/build/id/output.pdf`,
                   type: 'pdf',
                 },
               ],
+              pdfDownloadDomain: 'https://compiles.overleaf.test/zone/b',
             })
           )
         })