From 7f5af99ee91ce596abce66b334090063ee476683 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 9 Jun 2026 08:38:32 +0200 Subject: [PATCH] [web] determine rootResourcePath if not set in compile request (#34398) * [web] determine rootResourcePath if not set in compile request This affects the following compile requests: - compile request from "Download PDF" button on project dashboard Pre-resolving the rootResourcePath for all projects is too expensive. Adding an endpoint to resolve it is too slow and doubles project auth check. - custom frontends Can't do much about these. I tried blocking them, requests still come in after weeks of returning 400s. The rootDoc maintenance methods now return {rootResourcePath,rootDocId} rather than just the rootDocId. This avoids an extra project lookup when the rootDocId was updated. GitOrigin-RevId: ab7c539b7ffd162bde0903fe1040a48834a245f5 --- .../Features/Compile/CompileController.mjs | 18 --- .../src/Features/Compile/CompileManager.mjs | 15 ++- .../src/Features/Exports/ExportsHandler.mjs | 33 ++--- .../Project/ProjectEntityUpdateHandler.mjs | 16 +-- .../Project/ProjectRootDocManager.mjs | 21 +--- .../Features/Templates/TemplatesManager.mjs | 4 +- .../Features/Uploads/ProjectUploadManager.mjs | 4 +- .../unit/src/Compile/CompileManager.test.mjs | 54 ++++++-- .../unit/src/Exports/ExportsHandler.test.mjs | 23 ++-- .../Project/ProjectRootDocManager.test.mjs | 115 +++++++----------- 10 files changed, 141 insertions(+), 162 deletions(-) diff --git a/services/web/app/src/Features/Compile/CompileController.mjs b/services/web/app/src/Features/Compile/CompileController.mjs index ef176598cf..95bcfcbefd 100644 --- a/services/web/app/src/Features/Compile/CompileController.mjs +++ b/services/web/app/src/Features/Compile/CompileController.mjs @@ -217,24 +217,6 @@ const _CompileController = { options.pdfCachingMinChunkSize = pdfCachingMinChunkSize } - if (!options.rootResourcePath) { - const agent = (req.headers['user-agent'] || '').toLowerCase() - const isKnownOtherFrontend = agent.includes('node-fetch') - logger.warn( - { isKnownOtherFrontend, req, projectId, userId, options }, - 'rootResourcePath is missing in request body' - ) - if (isKnownOtherFrontend) { - // Reject malformed compile request from "known" other frontend. - res.status(400).json({ - error: 'rootResourcePath is missing in request body', - }) - return - } - // All others: Log for now and fall back to old compile mode. - options.compileFromHistory = false - } - const { status, outputFiles, diff --git a/services/web/app/src/Features/Compile/CompileManager.mjs b/services/web/app/src/Features/Compile/CompileManager.mjs index 9a4ac41946..e68712d6c4 100644 --- a/services/web/app/src/Features/Compile/CompileManager.mjs +++ b/services/web/app/src/Features/Compile/CompileManager.mjs @@ -48,7 +48,20 @@ async function compile(projectId, userId, options = {}) { return { status: 'autocompile-backoff', outputFiles: [] } } - await ProjectRootDocManager.promises.ensureRootDocumentIsSet(projectId) + if (!options.rootResourcePath) { + const result = + await ProjectRootDocManager.promises.ensureRootDocumentIsValid(projectId) + if (result) { + options.rootDoc_id = result.rootDocId + options.rootResourcePath = result.rootResourcePath.replace(/^\//, '') + } else { + return { + status: 'validation-problems', + validationProblems: { mainFile: 'no main file specified' }, + outputFiles: [], + } + } + } const limits = await CompileManager.promises.getProjectCompileLimits(projectId) diff --git a/services/web/app/src/Features/Exports/ExportsHandler.mjs b/services/web/app/src/Features/Exports/ExportsHandler.mjs index 80b3875df6..2f12dea115 100644 --- a/services/web/app/src/Features/Exports/ExportsHandler.mjs +++ b/services/web/app/src/Features/Exports/ExportsHandler.mjs @@ -1,7 +1,6 @@ import OError from '@overleaf/o-error' import ProjectGetter from '../Project/ProjectGetter.mjs' import ProjectHistoryHandler from '../Project/ProjectHistoryHandler.mjs' -import ProjectLocator from '../Project/ProjectLocator.mjs' import ProjectRootDocManager from '../Project/ProjectRootDocManager.mjs' import UserGetter from '../User/UserGetter.mjs' import logger from '@overleaf/logger' @@ -37,18 +36,26 @@ export default ExportsHandler = { show_source: showSource, } = exportParams - let project, rootDoc, user, historyVersion + let project, rootResourcePath, user, historyVersion try { - project = await ProjectGetter.promises.getProject(projectId) - - await ProjectRootDocManager.promises.ensureRootDocumentIsValid(projectId) - - rootDoc = await ProjectLocator.promises.findRootDoc({ - project, - project_id: projectId, + project = await ProjectGetter.promises.getProject(projectId, { + overleaf: 1, + compiler: 1, + imageName: 1, }) + const result = + await ProjectRootDocManager.promises.ensureRootDocumentIsValid( + projectId + ) + if (!result) { + throw new OError('cannot export project without root doc', { + project_id: projectId, + }) + } + rootResourcePath = result.rootResourcePath + user = await UserGetter.promises.getUser(userId, { first_name: 1, last_name: 1, @@ -67,12 +74,6 @@ export default ExportsHandler = { }) } - if (!rootDoc || !rootDoc.path) { - throw new OError('cannot export project without root doc', { - project_id: projectId, - }) - } - if (exportParams.first_name && exportParams.last_name) { user.first_name = exportParams.first_name user.last_name = exportParams.last_name @@ -81,7 +82,7 @@ export default ExportsHandler = { return { project: { id: projectId, - rootDocPath: rootDoc.path ? rootDoc.path.fileSystem : undefined, + rootDocPath: rootResourcePath, historyId: project.overleaf?.history?.id, historyVersion, v1ProjectId: project.overleaf != null ? project.overleaf.id : undefined, diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs index 211070c639..2f53f2dd1d 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs @@ -201,27 +201,27 @@ async function updateDocLines( return { rev, modified } } -async function setRootDoc(projectId, newRootDocID) { - logger.debug({ projectId, rootDocId: newRootDocID }, 'setting root doc') - if (projectId == null || newRootDocID == null) { +async function setRootDoc(projectId, rootDocId) { + logger.debug({ projectId, rootDocId }, 'setting root doc') + if (projectId == null || rootDocId == null) { throw new Errors.InvalidError('missing arguments (project or doc)') } - const docPath = + const rootResourcePath = await ProjectEntityHandler.promises.getDocPathByProjectIdAndDocId( projectId, - newRootDocID + rootDocId ) - if (ProjectEntityUpdateHandler.isPathValidForRootDoc(docPath)) { + if (ProjectEntityUpdateHandler.isPathValidForRootDoc(rootResourcePath)) { await Project.updateOne( { _id: projectId }, - { rootDoc_id: newRootDocID } + { rootDoc_id: rootDocId } ).exec() } else { throw new Errors.UnsupportedFileTypeError( 'invalid file extension for root doc' ) } - return newRootDocID + return { rootDocId, rootResourcePath } } async function unsetRootDoc(projectId) { diff --git a/services/web/app/src/Features/Project/ProjectRootDocManager.mjs b/services/web/app/src/Features/Project/ProjectRootDocManager.mjs index 8b4b212d87..e1613ccaf9 100644 --- a/services/web/app/src/Features/Project/ProjectRootDocManager.mjs +++ b/services/web/app/src/Features/Project/ProjectRootDocManager.mjs @@ -129,19 +129,6 @@ async function setRootDocFromName(projectId, rootDocName) { } } -async function ensureRootDocumentIsSet(projectId) { - const project = await ProjectGetter.promises.getProject(projectId, { - rootDoc_id: 1, - }) - if (!project) { - throw new Error('project not found') - } - if (project.rootDoc_id != null) { - return - } - return await ProjectRootDocManager.promises.setRootDocAutomatically(projectId) -} - /** * @param {ObjectId | string} projectId */ @@ -154,13 +141,13 @@ async function ensureRootDocumentIsValid(projectId) { throw new Error('project not found') } if (project.rootDoc_id != null) { - const docPath = + const rootResourcePath = await ProjectEntityHandler.promises.getDocPathFromProjectByDocId( project, project.rootDoc_id ) - if (docPath) { - return + if (rootResourcePath) { + return { rootDocId: project.rootDoc_id, rootResourcePath } } await ProjectEntityUpdateHandler.promises.unsetRootDoc(projectId) } @@ -214,13 +201,11 @@ const ProjectRootDocManager = { ['path', 'content'] ), setRootDocFromName: callbackify(setRootDocFromName), - ensureRootDocumentIsSet: callbackify(ensureRootDocumentIsSet), ensureRootDocumentIsValid: callbackify(ensureRootDocumentIsValid), promises: { setRootDocAutomatically, findRootDocFileFromDirectory, setRootDocFromName, - ensureRootDocumentIsSet, ensureRootDocumentIsValid, }, } diff --git a/services/web/app/src/Features/Templates/TemplatesManager.mjs b/services/web/app/src/Features/Templates/TemplatesManager.mjs index b29ba2dd71..5a7e246fa2 100644 --- a/services/web/app/src/Features/Templates/TemplatesManager.mjs +++ b/services/web/app/src/Features/Templates/TemplatesManager.mjs @@ -116,11 +116,11 @@ const TemplatesManager = { if (mainFile == null) { return } - const rootDocId = await ProjectRootDocManager.setRootDocFromName( + const result = await ProjectRootDocManager.setRootDocFromName( project._id, mainFile ) - if (rootDocId) project.rootDoc_id = rootDocId + if (result) project.rootDoc_id = result.rootDocId }, async fetchFromV1(templateId) { diff --git a/services/web/app/src/Features/Uploads/ProjectUploadManager.mjs b/services/web/app/src/Features/Uploads/ProjectUploadManager.mjs index 35add603e5..b86e2b8496 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadManager.mjs +++ b/services/web/app/src/Features/Uploads/ProjectUploadManager.mjs @@ -91,11 +91,11 @@ async function createProjectFromZipArchiveWithName( try { const { fileEntries, docEntries } = await _initializeProjectWithZipContents(ownerId, project, contentsPath) - const rootDocId = + const result = await ProjectRootDocManager.promises.setRootDocAutomatically( project._id ) - if (rootDocId) project.rootDoc_id = rootDocId + if (result) project.rootDoc_id = result.rootDocId return { fileEntries, docEntries, project } } catch (err) { // no need to wait for the cleanup here diff --git a/services/web/test/unit/src/Compile/CompileManager.test.mjs b/services/web/test/unit/src/Compile/CompileManager.test.mjs index 556e4ae245..8a720731b8 100644 --- a/services/web/test/unit/src/Compile/CompileManager.test.mjs +++ b/services/web/test/unit/src/Compile/CompileManager.test.mjs @@ -95,9 +95,12 @@ describe('CompileManager', function () { describe('compile', function () { beforeEach(function (ctx) { ctx.CompileManager._checkIfRecentlyCompiled = sinon.stub().resolves(false) - ctx.ProjectRootDocManager.promises.ensureRootDocumentIsSet = sinon + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid = sinon .stub() - .resolves() + .resolves({ + rootDocId: 'mock-root-doc-id-123', + rootResourcePath: '/main.tex', + }) ctx.CompileManager.promises.getProjectCompileLimits = sinon .stub() .resolves(ctx.limits) @@ -108,7 +111,7 @@ describe('CompileManager', function () { }) }) - describe('succesfully', function () { + describe('successfully', function () { let result beforeEach(async function (ctx) { ctx.CompileManager._checkIfAutoCompileLimitHasBeenHit = async ( @@ -140,7 +143,7 @@ describe('CompileManager', function () { }) it('should ensure that the root document is set', function (ctx) { - ctx.ProjectRootDocManager.promises.ensureRootDocumentIsSet + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid .calledWith(ctx.project_id) .should.equal(true) }) @@ -152,13 +155,17 @@ describe('CompileManager', function () { }) it('should run the compile with the compile limits', function (ctx) { - ctx.ClsiManager.promises.sendRequest - .calledWith(ctx.project_id, ctx.user_id, { + ctx.ClsiManager.promises.sendRequest.should.have.been.calledWith( + ctx.project_id, + ctx.user_id, + { timeout: ctx.limits.timeout, compileGroup: 'standard', buildId: sinon.match(/[a-f0-9]+-[a-f0-9]+/), - }) - .should.equal(true) + rootResourcePath: 'main.tex', + rootDoc_id: 'mock-root-doc-id-123', + } + ) }) it('should resolve with the output', function (ctx) { @@ -172,6 +179,37 @@ describe('CompileManager', function () { }) }) + describe('with rootResourcePath given', function () { + it('should not check the root doc', async function (ctx) { + const { status } = await ctx.CompileManager.promises.compile( + ctx.project_id, + ctx.user_id, + { rootResourcePath: 'main.tex' } + ) + ctx.ClsiManager.promises.sendRequest.should.have.been.called + status.should.equal('mock-status') + + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid.should.not + .have.been.called + }) + }) + + describe('without a root doc', function () { + it('should return early', async function (ctx) { + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid.resolves() + const { status, validationProblems } = + await ctx.CompileManager.promises.compile( + ctx.project_id, + ctx.user_id, + {} + ) + status.should.equal('validation-problems') + validationProblems.should.deep.equal({ + mainFile: 'no main file specified', + }) + }) + }) + describe('when the project has been recently compiled', function () { it('should return', async function (ctx) { ctx.CompileManager._checkIfAutoCompileLimitHasBeenHit = async ( diff --git a/services/web/test/unit/src/Exports/ExportsHandler.test.mjs b/services/web/test/unit/src/Exports/ExportsHandler.test.mjs index 255d3805a4..1b8a5215ef 100644 --- a/services/web/test/unit/src/Exports/ExportsHandler.test.mjs +++ b/services/web/test/unit/src/Exports/ExportsHandler.test.mjs @@ -27,12 +27,6 @@ describe('ExportsHandler', function () { vi.doMock('@overleaf/fetch-utils', () => ctx.fetchUtils) - vi.doMock('../../../../app/src/Features/Project/ProjectLocator', () => ({ - default: (ctx.ProjectLocator = { - promises: {}, - }), - })) - vi.doMock( '../../../../app/src/Features/Project/ProjectRootDocManager', () => ({ @@ -174,12 +168,9 @@ describe('ExportsHandler', function () { ctx.ProjectHistoryHandler.promises.ensureHistoryExistsForProject = sinon .stub() .resolves() - ctx.ProjectLocator.promises.findRootDoc = sinon - .stub() - .resolves({ element: null, path: { fileSystem: 'main.tex' } }) ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid = sinon .stub() - .resolves() + .resolves({ rootResourcePath: 'main.tex' }) ctx.UserGetter.promises.getUser = sinon.stub().resolves(ctx.user) ctx.ExportsHandler._requestVersion = sinon .stub() @@ -305,9 +296,9 @@ describe('ExportsHandler', function () { let exportData beforeEach(async function (ctx) { ctx.project.rootDoc_id = null - ctx.ProjectLocator.promises.findRootDoc = sinon + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid = sinon .stub() - .resolves({ path: { fileSystem: 'other.tex' } }) + .resolves({ rootResourcePath: 'other.tex' }) exportData = await ctx.ExportsHandler._buildExport(ctx.export_params) }) @@ -361,9 +352,9 @@ describe('ExportsHandler', function () { beforeEach(async function (ctx) { ctx.fakeDoc_id = '1a2b3c4d5e6f' ctx.project.rootDoc_id = ctx.fakeDoc_id - ctx.ProjectLocator.promises.findRootDoc = sinon + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid = sinon .stub() - .resolves({ path: { fileSystem: 'other.tex' } }) + .resolves({ rootResourcePath: 'other.tex' }) exportData = await ctx.ExportsHandler._buildExport(ctx.export_params) }) @@ -412,9 +403,9 @@ describe('ExportsHandler', function () { describe('when no root doc can be identified', function () { beforeEach(function (ctx) { - ctx.ProjectLocator.promises.findRootDoc = sinon + ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid = sinon .stub() - .resolves({ element: null, path: null }) + .resolves() }) it('should return an error', async function (ctx) { diff --git a/services/web/test/unit/src/Project/ProjectRootDocManager.test.mjs b/services/web/test/unit/src/Project/ProjectRootDocManager.test.mjs index 00eec627ee..2286da4ce8 100644 --- a/services/web/test/unit/src/Project/ProjectRootDocManager.test.mjs +++ b/services/web/test/unit/src/Project/ProjectRootDocManager.test.mjs @@ -517,72 +517,6 @@ describe('ProjectRootDocManager', function () { }) }) - describe('ensureRootDocumentIsSet', function () { - beforeEach(function (ctx) { - ctx.project = {} - ctx.ProjectGetter.promises.getProject = sinon.stub().resolves(ctx.project) - ctx.ProjectRootDocManager.promises.setRootDocAutomatically = sinon - .stub() - .resolves() - }) - - describe('when the root doc is set', function () { - beforeEach(async function (ctx) { - ctx.project.rootDoc_id = ctx.docId2 - - await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsSet( - ctx.project_id - ) - }) - - it('should find the project fetching only the rootDoc_id field', function (ctx) { - ctx.ProjectGetter.promises.getProject - .calledWith(ctx.project_id, { rootDoc_id: 1 }) - .should.equal(true) - }) - - it('should not try to update the project rootDoc_id', function (ctx) { - ctx.ProjectRootDocManager.promises.setRootDocAutomatically.called.should.equal( - false - ) - }) - }) - - describe('when the root doc is not set', function () { - beforeEach(async function (ctx) { - await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsSet( - ctx.project_id - ) - }) - - it('should find the project with only the rootDoc_id field', function (ctx) { - ctx.ProjectGetter.promises.getProject - .calledWith(ctx.project_id, { rootDoc_id: 1 }) - .should.equal(true) - }) - - it('should update the project rootDoc_id', function (ctx) { - ctx.ProjectRootDocManager.promises.setRootDocAutomatically - .calledWith(ctx.project_id) - .should.equal(true) - }) - }) - - describe('when the project does not exist', function () { - beforeEach(function (ctx) { - ctx.ProjectGetter.promises.getProject = sinon.stub().resolves(null) - }) - - it('should reject', async function (ctx) { - await expect( - ctx.ProjectRootDocManager.promises.ensureRootDocumentIsSet( - ctx.project_id - ) - ).to.be.rejectedWith('project not found') - }) - }) - }) - describe('ensureRootDocumentIsValid', function () { beforeEach(function (ctx) { ctx.project = {} @@ -601,14 +535,23 @@ describe('ProjectRootDocManager', function () { describe('when the root doc is set', function () { describe('when the root doc is valid', function () { + let result beforeEach(async function (ctx) { ctx.project.rootDoc_id = ctx.docId2 ctx.ProjectEntityHandler.promises.getDocPathFromProjectByDocId = sinon .stub() .resolves(ctx.docPaths[ctx.docId2]) - await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid( - ctx.project_id - ) + result = + await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid( + ctx.project_id + ) + }) + + it('should return the path and id', ctx => { + result.should.deep.equal({ + rootDocId: ctx.docId2, + rootResourcePath: ctx.docPaths[ctx.docId2], + }) }) it('should find the project without doc lines', function (ctx) { @@ -625,14 +568,27 @@ describe('ProjectRootDocManager', function () { }) describe('when the root doc is not valid', function () { + let result beforeEach(async function (ctx) { ctx.project.rootDoc_id = new ObjectId() ctx.ProjectEntityHandler.promises.getDocPathFromProjectByDocId = sinon .stub() .resolves(null) - await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid( - ctx.project_id + ctx.automaticRootDoc = { + rootDocId: ctx.docId4, + rootResourcePath: ctx.docPaths[ctx.docId4], + } + ctx.ProjectRootDocManager.promises.setRootDocAutomatically.resolves( + ctx.automaticRootDoc ) + result = + await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid( + ctx.project_id + ) + }) + + it('should return the path and id', ctx => { + result.should.deep.equal(ctx.automaticRootDoc) }) it('should find the project without doc lines', function (ctx) { @@ -656,10 +612,23 @@ describe('ProjectRootDocManager', function () { }) describe('when the root doc is not set', function () { + let result beforeEach(async function (ctx) { - await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid( - ctx.project_id + ctx.automaticRootDoc = { + rootDocId: ctx.docId4, + rootResourcePath: ctx.docPaths[ctx.docId4], + } + ctx.ProjectRootDocManager.promises.setRootDocAutomatically.resolves( + ctx.automaticRootDoc ) + result = + await ctx.ProjectRootDocManager.promises.ensureRootDocumentIsValid( + ctx.project_id + ) + }) + + it('should return the path and id', ctx => { + result.should.deep.equal(ctx.automaticRootDoc) }) it('should find the project without doc lines', async function (ctx) {