From c387e60a289aeb3f584b69ec6fb2e8891e5f89ea Mon Sep 17 00:00:00 2001 From: andrew rumble Date: Thu, 13 Jun 2024 10:13:22 +0100 Subject: [PATCH] Remove unnecessary symlink check GitOrigin-RevId: 08d7295403a258818276b9fbd7666a20fbc2e00f --- .../clsi/app/js/OutputFileArchiveManager.js | 27 +----- .../unit/js/OutputFileArchiveManagerTests.js | 85 +++++++++---------- 2 files changed, 43 insertions(+), 69 deletions(-) diff --git a/services/clsi/app/js/OutputFileArchiveManager.js b/services/clsi/app/js/OutputFileArchiveManager.js index 51b4991838..45e9f33efa 100644 --- a/services/clsi/app/js/OutputFileArchiveManager.js +++ b/services/clsi/app/js/OutputFileArchiveManager.js @@ -2,7 +2,7 @@ const archiver = require('archiver') const OutputCacheManager = require('./OutputCacheManager') const OutputFileFinder = require('./OutputFileFinder') const Settings = require('@overleaf/settings') -const { open, realpath } = require('node:fs/promises') +const { open } = require('node:fs/promises') const path = require('path') const { NotFoundError } = require('./Errors') @@ -20,23 +20,6 @@ function getContentDir(projectId, userId) { return `${Settings.path.outputDir}/${subDir}/` } -/** - * Is the provided path a symlink? - * @param {string} path - * @return {Promise} - */ -async function isSymlink(path) { - try { - const realPath = await realpath(path) - return realPath !== path - } catch (error) { - if (error.code === 'ELOOP') { - return true - } - throw error - } -} - module.exports = { async archiveFilesForBuild(projectId, userId, build) { const validFiles = await this._getAllOutputFiles(projectId, userId, build) @@ -47,11 +30,9 @@ module.exports = { for (const file of validFiles) { try { - if (!(await isSymlink(file))) { - const fileHandle = await open(file, 'r') - const fileStream = fileHandle.createReadStream() - archive.append(fileStream, { name: path.basename(file) }) - } + const fileHandle = await open(file, 'r') + const fileStream = fileHandle.createReadStream() + archive.append(fileStream, { name: path.basename(file) }) } catch (error) { missingFiles.push(file) } diff --git a/services/clsi/test/unit/js/OutputFileArchiveManagerTests.js b/services/clsi/test/unit/js/OutputFileArchiveManagerTests.js index 6a6c3190ad..c2a7047a76 100644 --- a/services/clsi/test/unit/js/OutputFileArchiveManagerTests.js +++ b/services/clsi/test/unit/js/OutputFileArchiveManagerTests.js @@ -59,7 +59,7 @@ describe('OutputFileArchiveManager', function () { }) }) - describe('when called with no files', function () { + describe('when the output cache directory contains only exportable files', function () { beforeEach(async function () { this.OutputFileFinder.promises.findOutputFiles.resolves({ outputFiles: [ @@ -111,7 +111,7 @@ describe('OutputFileArchiveManager', function () { }) }) - describe('when called with a list of files that all are in the output directory', function () { + describe('when the directory includes files ignored by web', function () { beforeEach(async function () { this.OutputFileFinder.promises.findOutputFiles.resolves({ outputFiles: [ @@ -119,88 +119,81 @@ describe('OutputFileArchiveManager', function () { { path: 'file_2' }, { path: 'file_3' }, { path: 'file_4' }, + { path: 'output.pdf' }, ], }) await this.OutputFileArchiveManager.archiveFilesForBuild( projectId, userId, - buildId, - ['file_1', 'file_4'] + buildId ) }) - it('creates a zip archive', function () { - sinon.assert.calledWith(this.archiver, 'zip') - }) - - it('adds only output files from the list of files to the archive', function () { - expect(this.archive.append.callCount).to.equal(2) + it('only includes the non-ignored files in the archive', function () { + expect(this.archive.append.callCount).to.equal(4) sinon.assert.calledWith( this.archive.append, - `handle: file_1`, - sinon.match({ - name: 'file_1', - }) + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_1`, + sinon.match({ name: 'file_1' }) ) sinon.assert.calledWith( this.archive.append, - `handle: file_4`, - sinon.match({ - name: 'file_4', - }) + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_2`, + sinon.match({ name: 'file_2' }) + ) + sinon.assert.calledWith( + this.archive.append, + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_3`, + sinon.match({ name: 'file_3' }) + ) + sinon.assert.calledWith( + this.archive.append, + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_4`, + sinon.match({ name: 'file_4' }) ) - }) - - it('finalizes the archive after all files are appended', function () { - sinon.assert.called(this.archive.finalize) - expect(this.archive.finalize.calledBefore(this.archive.append)).to.be - .false }) }) - describe('when called with a list of files and one of the files is missing from the output directory', function () { + describe('when one of the files is called output.pdf', function () { beforeEach(async function () { this.OutputFileFinder.promises.findOutputFiles.resolves({ outputFiles: [ { path: 'file_1' }, { path: 'file_2' }, { path: 'file_3' }, + { path: 'file_4' }, + { path: 'output.pdf' }, ], }) await this.OutputFileArchiveManager.archiveFilesForBuild( projectId, userId, - buildId, - ['file_1', 'file_4'] + buildId ) }) - it('creates a zip archive', function () { - sinon.assert.calledWith(this.archiver, 'zip') - }) - - it('adds the files that were found to the archive', function () { + it('does not include that file in the archive', function () { + expect(this.archive.append.callCount).to.equal(4) sinon.assert.calledWith( this.archive.append, - `handle: file_1`, + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_1`, sinon.match({ name: 'file_1' }) ) - }) - - it('adds a file listing any missing files', function () { sinon.assert.calledWith( this.archive.append, - 'file_4', - sinon.match({ - name: 'missing_files.txt', - }) + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_2`, + sinon.match({ name: 'file_2' }) + ) + sinon.assert.calledWith( + this.archive.append, + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_3`, + sinon.match({ name: 'file_3' }) + ) + sinon.assert.calledWith( + this.archive.append, + `handle: ${this.outputDir}/${projectId}-${userId}/${buildId}/file_4`, + sinon.match({ name: 'file_4' }) ) - }) - - it('finalizes the archive after all files are appended', function () { - sinon.assert.called(this.archive.finalize) - expect(this.archive.finalize.calledBefore(this.archive.append)).to.be - .false }) })