From a78bcee15f1b4afb34035988dbd9152be2b47c95 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 25 May 2022 11:56:10 +0100 Subject: [PATCH] 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 | 3 +- services/clsi/config/settings.defaults.js | 4 +- .../test/unit/js/CompileControllerTests.js | 46 +++++++++++-- .../app/src/Features/Compile/ClsiManager.js | 4 +- .../src/Features/Compile/CompileController.js | 16 ++++- .../src/Features/Compile/CompileManager.js | 6 +- .../src/Compile/CompileControllerTests.js | 68 ++++++++++++++++++- 7 files changed, 130 insertions(+), 17 deletions(-) diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index a5dc885da0..596158f7a7 100644 --- a/services/clsi/app/js/CompileController.js +++ b/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}` : '') + diff --git a/services/clsi/config/settings.defaults.js b/services/clsi/config/settings.defaults.js index 9a86dd36dd..9a7d83539b 100644 --- a/services/clsi/config/settings.defaults.js +++ b/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'}:${ diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index 8d66948b1f..71f159e069 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/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, diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 310900c9be..5facba5c43 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/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 ) } ) diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 1f4b512598..39263d4496 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/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, }) } ) diff --git a/services/web/app/src/Features/Compile/CompileManager.js b/services/web/app/src/Features/Compile/CompileManager.js index fcb7ddaaae..2c38207fd1 100644 --- a/services/web/app/src/Features/Compile/CompileManager.js +++ b/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 ) } ) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index 9c8ad93d0e..6d2db992a3 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/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', }) ) })