Merge pull request #14023 from overleaf/jpa-web-clsi-fetch

[web] migrate CompileController from request to node-fetch

GitOrigin-RevId: ffb3319319d7f986ec972b4b6c56bd5499ecd9ab
This commit is contained in:
Jakob Ackermann 2023-08-03 11:09:29 +02:00 committed by Copybot
parent 7c7a3d7a9c
commit bf2e33ec83
7 changed files with 175 additions and 263 deletions

View file

@ -33,6 +33,9 @@ module.exports = function (backendGroup) {
compileBackendClass,
callback
) {
if (!clsiCookiesEnabled) {
return callback()
}
rclient.get(this.buildKey(projectId, userId), (err, serverId) => {
if (err) {
return callback(err)

View file

@ -1,12 +1,13 @@
let CompileController
const { URL } = require('url')
const { URL, URLSearchParams } = require('url')
const { pipeline } = require('stream/promises')
const { Cookie } = require('tough-cookie')
const OError = require('@overleaf/o-error')
const Metrics = require('@overleaf/metrics')
const ProjectGetter = require('../Project/ProjectGetter')
const CompileManager = require('./CompileManager')
const ClsiManager = require('./ClsiManager')
const logger = require('@overleaf/logger')
const request = require('request')
const Settings = require('@overleaf/settings')
const SessionManager = require('../Authentication/SessionManager')
const { RateLimiter } = require('../../infrastructure/RateLimiter')
@ -17,6 +18,10 @@ const Path = require('path')
const AnalyticsManager = require('../Analytics/AnalyticsManager')
const SplitTestHandler = require('../SplitTests/SplitTestHandler')
const { callbackify } = require('../../util/promises')
const {
fetchStreamWithResponse,
RequestFailedError,
} = require('@overleaf/fetch-utils')
const COMPILE_TIMEOUT_MS = 10 * 60 * 1000
@ -274,24 +279,19 @@ module.exports = CompileController = {
downloadPdf(req, res, next) {
Metrics.inc('pdf-downloads')
const projectId = req.params.Project_id
const isPdfjsPartialDownload = req.query?.pdfng
const rateLimit = function (callback) {
if (isPdfjsPartialDownload) {
callback(null, true)
} else {
pdfDownloadRateLimiter
.consume(req.ip)
.then(() => {
callback(null, true)
})
.catch(err => {
if (err instanceof Error) {
callback(err)
} else {
callback(null, false)
}
})
}
pdfDownloadRateLimiter
.consume(req.ip)
.then(() => {
callback(null, true)
})
.catch(err => {
if (err instanceof Error) {
callback(err)
} else {
callback(null, false)
}
})
}
ProjectGetter.getProject(projectId, { name: 1 }, function (err, project) {
@ -328,7 +328,7 @@ module.exports = CompileController = {
req.params.build_id,
'output.pdf'
)
CompileController.proxyToClsi(projectId, url, req, res, next)
CompileController.proxyToClsi(projectId, url, {}, req, res, next)
})
}
})
@ -374,7 +374,7 @@ module.exports = CompileController = {
return
}
const url = `/project/${projectId}/output/output.pdf`
CompileController.proxyToClsi(projectId, url, req, res, next)
CompileController.proxyToClsi(projectId, url, {}, req, res, next)
})
},
@ -390,7 +390,7 @@ module.exports = CompileController = {
req.params.build_id,
req.params.file
)
CompileController.proxyToClsi(projectId, url, req, res, next)
CompileController.proxyToClsi(projectId, url, {}, req, res, next)
})
},
@ -412,6 +412,7 @@ module.exports = CompileController = {
CompileController.proxyToClsiWithLimits(
submissionId,
url,
{},
limits,
req,
res,
@ -464,8 +465,14 @@ module.exports = CompileController = {
if (error) return next(error)
const url = CompileController._getUrl(projectId, userId, 'sync/pdf')
const destination = { url, qs: { page, h, v, imageName } }
CompileController.proxyToClsi(projectId, destination, req, res, next)
CompileController.proxyToClsi(
projectId,
url,
{ page, h, v, imageName },
req,
res,
next
)
})
})
},
@ -499,13 +506,19 @@ module.exports = CompileController = {
if (error) return next(error)
const url = CompileController._getUrl(projectId, userId, 'sync/code')
const destination = { url, qs: { file, line, column, imageName } }
CompileController.proxyToClsi(projectId, destination, req, res, next)
CompileController.proxyToClsi(
projectId,
url,
{ file, line, column, imageName },
req,
res,
next
)
})
})
},
proxyToClsi(projectId, url, req, res, next) {
proxyToClsi(projectId, url, qs, req, res, next) {
CompileManager.getProjectCompileLimits(projectId, function (error, limits) {
if (error) {
return next(error)
@ -513,6 +526,7 @@ module.exports = CompileController = {
CompileController.proxyToClsiWithLimits(
projectId,
url,
qs,
limits,
req,
res,
@ -521,60 +535,44 @@ module.exports = CompileController = {
})
},
proxyToClsiWithLimits(projectId, url, limits, req, res, next) {
proxyToClsiWithLimits(projectId, url, qs, limits, req, res, next) {
_getPersistenceOptions(
req,
projectId,
limits.compileGroup,
limits.compileBackendClass,
(err, persistenceOptions) => {
let qs
if (err) {
OError.tag(err, 'error getting cookie jar for clsi request')
return next(err)
}
// expand any url parameter passed in as {url:..., qs:...}
if (typeof url === 'object') {
;({ url, qs } = url)
}
const compilerUrl = Settings.apis.clsi.url
url = `${compilerUrl}${url}`
const oneMinute = 60 * 1000
// the base request
const options = {
url,
url = new URL(`${Settings.apis.clsi.url}${url}`)
url.search = new URLSearchParams({
...persistenceOptions.qs,
...qs,
}).toString()
fetchStreamWithResponse(url.href, {
method: req.method,
timeout: oneMinute,
...persistenceOptions,
}
// add any provided query string
if (qs != null) {
options.qs = Object.assign(options.qs || {}, qs)
}
// if we have a build parameter, pass it through to the clsi
if (req.query?.pdfng && req.query?.build != null) {
// only for new pdf viewer
if (options.qs == null) {
options.qs = {}
}
options.qs.build = req.query.build
}
// if we are byte serving pdfs, pass through If-* and Range headers
// do not send any others, there's a proxying loop if Host: is passed!
if (req.query?.pdfng) {
const newHeaders = {}
for (const h in req.headers) {
if (/^(If-|Range)/i.test(h)) {
newHeaders[h] = req.headers[h]
signal: AbortSignal.timeout(60 * 1000),
headers: persistenceOptions.headers,
})
.then(({ stream, response }) => {
for (const key of ['Content-Length', 'Content-Type']) {
res.setHeader(key, response.headers.get(key))
}
}
options.headers = newHeaders
}
const proxy = request(options)
proxy.pipe(res)
proxy.on('error', error =>
logger.warn({ err: error, url }, 'CLSI proxy error')
)
res.writeHead(response.status)
return pipeline(stream, res)
})
.catch(err => {
if (!res.headersSent) {
if (err instanceof RequestFailedError) {
res.sendStatus(err.response.status)
} else {
res.sendStatus(500)
}
}
logger.warn({ err, projectId, url }, 'CLSI proxy error')
})
}
)
},
@ -613,15 +611,29 @@ function _getPersistenceOptions(
const { clsiserverid } = req.query
const userId = SessionManager.getLoggedInUserId(req)
if (clsiserverid && typeof clsiserverid === 'string') {
callback(null, { qs: { clsiserverid, compileGroup, compileBackendClass } })
callback(null, {
qs: { clsiserverid, compileGroup, compileBackendClass },
headers: {},
})
} else {
ClsiCookieManager.getCookieJar(
ClsiCookieManager.getServerId(
projectId,
userId,
compileGroup,
compileBackendClass,
(err, jar) => {
callback(err, { jar, qs: { compileGroup, compileBackendClass } })
(err, clsiServerId) => {
if (err) return callback(err)
callback(null, {
qs: { compileGroup, compileBackendClass },
headers: clsiServerId
? {
Cookie: new Cookie({
key: Settings.clsiCookie.key,
value: clsiServerId,
}).cookieString(),
}
: {},
})
}
)
}

View file

@ -59,9 +59,6 @@ async function createNewUser(attributes, options = {}) {
Object.assign(user, attributes)
user.ace.syntaxValidation = true
if (user.featureSwitches != null) {
user.featureSwitches.pdfng = true
}
const reversedHostname = user.email.split('@')[1].split('').reverse().join('')

View file

@ -29,7 +29,6 @@ function sentryReporter() {
'SecurityError: Permission denied to access property "pathname" on cross-origin object',
// Ignore unhandled error that is "expected" - see https://github.com/overleaf/issues/issues/3321
/^Missing PDF/,
/^pdfng error Error: MissingPDFException/,
// Ignore "expected" error from aborted fetch - see https://github.com/overleaf/issues/issues/3321
/^AbortError/,
// Ignore spurious error from Ace internals - see https://github.com/overleaf/issues/issues/3321

View file

@ -260,6 +260,7 @@
"@babel/register": "^7.21.0",
"@juggle/resize-observer": "^3.3.1",
"@lezer/generator": "^1.3.0",
"@overleaf/stream-utils": "*",
"@testing-library/cypress": "^9.0.0",
"@testing-library/dom": "^9.3.0",
"@testing-library/react": "^12.1.5",

View file

@ -5,6 +5,8 @@ const modulePath = '../../../../app/src/Features/Compile/CompileController.js'
const SandboxedModule = require('sandboxed-module')
const MockRequest = require('../helpers/MockRequest')
const MockResponse = require('../helpers/MockResponse')
const { Headers } = require('node-fetch')
const { ReadableString } = require('@overleaf/stream-utils')
describe('CompileController', function () {
beforeEach(function () {
@ -29,21 +31,23 @@ describe('CompileController', function () {
this.settings = {
apis: {
clsi: {
url: 'clsi.example.com',
url: 'http://clsi.example.com',
defaultBackendClass: 'e2',
},
clsi_priority: {
url: 'clsi-priority.example.com',
url: 'http://clsi-priority.example.com',
},
},
defaultFeatures: {
compileGroup: 'standard',
compileTimeout: 60,
},
clsiCookie: {
key: 'cookie-key',
},
}
this.jar = { cookie: 'stuff' }
this.ClsiCookieManager = {
getCookieJar: sinon.stub().yields(null, this.jar),
getServerId: sinon.stub().yields(null, 'clsi-server-id-from-redis'),
}
this.SessionManager = {
getLoggedInUser: sinon.stub().callsArgWith(1, null, this.user),
@ -51,9 +55,27 @@ describe('CompileController', function () {
getSessionUser: sinon.stub().returns(this.user),
isUserLoggedIn: sinon.stub().returns(true),
}
this.pipeline = sinon.stub().callsFake(async (stream, res) => {
if (res.callback) res.callback()
})
this.clsiStream = new ReadableString('{}')
this.clsiResponse = {
headers: new Headers({
'Content-Length': '2',
'Content-Type': 'application/json',
}),
}
this.fetchUtils = {
fetchStreamWithResponse: sinon.stub().resolves({
stream: this.clsiStream,
response: this.clsiResponse,
}),
}
this.CompileController = SandboxedModule.require(modulePath, {
requires: {
'stream/promises': { pipeline: this.pipeline },
'@overleaf/settings': this.settings,
'@overleaf/fetch-utils': this.fetchUtils,
request: (this.request = sinon.stub()),
'../Project/ProjectGetter': (this.ProjectGetter = {}),
'@overleaf/metrics': (this.Metrics = { inc: sinon.stub() }),
@ -349,7 +371,6 @@ describe('CompileController', function () {
beforeEach(function () {
this.req.params = { Project_id: this.projectId }
this.req.query = { pdfng: true }
this.project = { name: 'test namè; 1' }
this.ProjectGetter.getProject = sinon
.stub()
@ -357,8 +378,10 @@ describe('CompileController', function () {
})
describe('when downloading for embedding', function () {
beforeEach(function () {
this.CompileController.proxyToClsi = sinon.stub()
beforeEach(function (done) {
this.CompileController.proxyToClsi = sinon
.stub()
.callsFake(() => done())
this.CompileController.downloadPdf(this.req, this.res, this.next)
})
@ -387,6 +410,7 @@ describe('CompileController', function () {
.calledWith(
this.projectId,
`/project/${this.projectId}/user/${this.user_id}/output/output.pdf`,
{},
this.req,
this.res,
this.next
@ -396,9 +420,11 @@ describe('CompileController', function () {
})
describe('when the a build-id is provided', function () {
beforeEach(function () {
beforeEach(function (done) {
this.req.params.build_id = this.buildId = '1234-5678'
this.CompileController.proxyToClsi = sinon.stub()
this.CompileController.proxyToClsi = sinon
.stub()
.callsFake(() => done())
this.CompileController.downloadPdf(this.req, this.res, this.next)
})
@ -407,6 +433,7 @@ describe('CompileController', function () {
.calledWith(
this.projectId,
`/project/${this.projectId}/user/${this.user_id}/build/${this.buildId}/output/output.pdf`,
{},
this.req,
this.res,
this.next
@ -414,26 +441,6 @@ describe('CompileController', function () {
.should.equal(true)
})
})
describe('when the pdf is not going to be used in pdfjs viewer', function () {
it('should check the rate limiter when pdfng is not set', function (done) {
this.req.query = {}
this.CompileController.proxyToClsi = (projectId, url) => {
expect(this.rateLimiter.consume).to.have.been.called
done()
}
this.CompileController.downloadPdf(this.req, this.res)
})
it('should check the rate limiter when pdfng is false', function (done) {
this.req.query = { pdfng: false }
this.CompileController.proxyToClsi = (projectId, url) => {
expect(this.rateLimiter.consume).to.have.been.called
done()
}
this.CompileController.downloadPdf(this.req, this.res)
})
})
})
describe('getFileFromClsiWithoutUser', function () {
@ -464,6 +471,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith(
this.submission_id,
this.expected_url,
{},
{
compileGroup: 'standard',
compileBackendClass: 'e2',
@ -486,6 +494,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith(
this.submission_id,
this.expected_url,
{},
{
compileGroup: 'special',
compileBackendClass: 'e2',
@ -517,10 +526,8 @@ describe('CompileController', function () {
it('should proxy the request with an imageName', function () {
expect(this.CompileController.proxyToClsi).to.have.been.calledWith(
this.projectId,
{
url: `/project/${this.projectId}/user/${this.user_id}/sync/code`,
qs: { file, line, column, imageName },
},
`/project/${this.projectId}/user/${this.user_id}/sync/code`,
{ file, line, column, imageName },
this.req,
this.res,
this.next
@ -551,10 +558,8 @@ describe('CompileController', function () {
it('should proxy the request with an imageName', function () {
expect(this.CompileController.proxyToClsi).to.have.been.calledWith(
this.projectId,
{
url: `/project/${this.projectId}/user/${this.user_id}/sync/pdf`,
qs: { page, h, v, imageName },
},
`/project/${this.projectId}/user/${this.user_id}/sync/pdf`,
{ page, h, v, imageName },
this.req,
this.res,
this.next
@ -564,16 +569,6 @@ describe('CompileController', function () {
describe('proxyToClsi', function () {
beforeEach(function () {
this.request.returns(
(this.proxy = {
pipe: sinon.stub(),
on: sinon.stub(),
})
)
this.upstream = {
statusCode: 204,
headers: { mock: 'header' },
}
this.req.method = 'mock-method'
this.req.headers = {
Mock: 'Headers',
@ -585,7 +580,8 @@ describe('CompileController', function () {
describe('old pdf viewer', function () {
describe('user with standard priority', function () {
beforeEach(function () {
beforeEach(function (done) {
this.res.callback = done
this.CompileManager.getProjectCompileLimits = sinon
.stub()
.callsArgWith(1, null, {
@ -595,6 +591,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
{ query: 'foo' },
this.req,
this.res,
this.next
@ -602,43 +599,45 @@ describe('CompileController', function () {
})
it('should open a request to the CLSI', function () {
this.request
.calledWith({
jar: this.jar,
qs: { compileGroup: 'standard', compileBackendClass: 'e2' },
method: this.req.method,
url: `${this.settings.apis.clsi.url}${this.url}`,
timeout: 60 * 1000,
})
.should.equal(true)
this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith(
`${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2&query=foo`
)
})
it('should pass the request on to the client', function () {
this.proxy.pipe.calledWith(this.res).should.equal(true)
})
it('should bind an error handle to the request proxy', function () {
this.proxy.on.calledWith('error').should.equal(true)
this.pipeline.should.have.been.calledWith(this.clsiStream, this.res)
})
})
describe('user with priority compile', function () {
beforeEach(function () {
beforeEach(function (done) {
this.res.callback = done
this.CompileManager.getProjectCompileLimits = sinon
.stub()
.callsArgWith(1, null, { compileGroup: 'priority' })
.callsArgWith(1, null, {
compileGroup: 'priority',
compileBackendClass: 'c2d',
})
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
{},
this.req,
this.res,
this.next
)
})
it('should open a request to the CLSI', function () {
this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith(
`${this.settings.apis.clsi.url}${this.url}?compileGroup=priority&compileBackendClass=c2d`
)
})
})
describe('user with standard priority via query string', function () {
beforeEach(function () {
beforeEach(function (done) {
this.res.callback = done
this.req.query = { compileGroup: 'standard' }
this.CompileManager.getProjectCompileLimits = sinon
.stub()
@ -649,6 +648,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
{},
this.req,
this.res,
this.next
@ -656,28 +656,19 @@ describe('CompileController', function () {
})
it('should open a request to the CLSI', function () {
this.request
.calledWith({
jar: this.jar,
qs: { compileGroup: 'standard', compileBackendClass: 'e2' },
method: this.req.method,
url: `${this.settings.apis.clsi.url}${this.url}`,
timeout: 60 * 1000,
})
.should.equal(true)
this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith(
`${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2`
)
})
it('should pass the request on to the client', function () {
this.proxy.pipe.calledWith(this.res).should.equal(true)
})
it('should bind an error handle to the request proxy', function () {
this.proxy.on.calledWith('error').should.equal(true)
this.pipeline.should.have.been.calledWith(this.clsiStream, this.res)
})
})
describe('user with non-existent priority via query string', function () {
beforeEach(function () {
beforeEach(function (done) {
this.res.callback = done
this.req.query = { compileGroup: 'foobar' }
this.CompileManager.getProjectCompileLimits = sinon
.stub()
@ -688,6 +679,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
{},
this.req,
this.res,
this.next
@ -695,20 +687,15 @@ describe('CompileController', function () {
})
it('should proxy to the standard url', function () {
this.request
.calledWith({
jar: this.jar,
qs: { compileGroup: 'standard', compileBackendClass: 'e2' },
method: this.req.method,
url: `${this.settings.apis.clsi.url}${this.url}`,
timeout: 60 * 1000,
})
.should.equal(true)
this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith(
`${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2`
)
})
})
describe('user with build parameter via query string', function () {
beforeEach(function () {
beforeEach(function (done) {
this.res.callback = done
this.CompileManager.getProjectCompileLimits = sinon
.stub()
.callsArgWith(1, null, {
@ -719,6 +706,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
{},
this.req,
this.res,
this.next
@ -726,104 +714,10 @@ describe('CompileController', function () {
})
it('should proxy to the standard url without the build parameter', function () {
this.request
.calledWith({
jar: this.jar,
qs: { compileGroup: 'standard', compileBackendClass: 'e2' },
method: this.req.method,
url: `${this.settings.apis.clsi.url}${this.url}`,
timeout: 60 * 1000,
})
.should.equal(true)
})
})
})
describe('new pdf viewer', function () {
beforeEach(function () {
this.req.query = { pdfng: true }
})
describe('user with standard priority', function () {
beforeEach(function () {
this.CompileManager.getProjectCompileLimits = sinon
.stub()
.callsArgWith(1, null, {
compileGroup: 'standard',
compileBackendClass: 'e2',
})
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
this.req,
this.res,
this.next
this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith(
`${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2`
)
})
it('should open a request to the CLSI', function () {
this.request
.calledWith({
jar: this.jar,
qs: { compileGroup: 'standard', compileBackendClass: 'e2' },
method: this.req.method,
url: `${this.settings.apis.clsi.url}${this.url}`,
timeout: 60 * 1000,
headers: {
Range: '123-456',
'If-Range': 'abcdef',
'If-Modified-Since': 'Mon, 15 Dec 2014 15:23:56 GMT',
},
})
.should.equal(true)
})
it('should pass the request on to the client', function () {
this.proxy.pipe.calledWith(this.res).should.equal(true)
})
it('should bind an error handle to the request proxy', function () {
this.proxy.on.calledWith('error').should.equal(true)
})
})
describe('user with build parameter via query string', function () {
beforeEach(function () {
this.CompileManager.getProjectCompileLimits = sinon
.stub()
.callsArgWith(1, null, {
compileGroup: 'standard',
compileBackendClass: 'e2',
})
this.req.query = { build: 1234, pdfng: true }
this.CompileController.proxyToClsi(
this.projectId,
(this.url = '/test'),
this.req,
this.res,
this.next
)
})
it('should proxy to the standard url with the build parameter', function () {
this.request
.calledWith({
jar: this.jar,
method: this.req.method,
qs: {
build: 1234,
compileGroup: 'standard',
compileBackendClass: 'e2',
},
url: `${this.settings.apis.clsi.url}${this.url}`,
timeout: 60 * 1000,
headers: {
Range: '123-456',
'If-Range': 'abcdef',
'If-Modified-Since': 'Mon, 15 Dec 2014 15:23:56 GMT',
},
})
.should.equal(true)
})
})
})
})
@ -872,6 +766,7 @@ describe('CompileController', function () {
this.CompileController.proxyToClsi,
this.projectId,
`/project/${this.projectId}/output/output.pdf`,
{},
this.req,
this.res
)
@ -880,6 +775,7 @@ describe('CompileController', function () {
.calledWith(
this.projectId,
`/project/${this.projectId}/output/output.pdf`,
{},
this.req,
this.res
)

View file

@ -81,6 +81,10 @@ class MockResponse {
}
}
writeHead(status) {
this.statusCode = status
}
send(status, body) {
if (arguments.length < 2) {
if (typeof status !== 'number') {