From 6c4aa88b9ea33a7a39abf351f213cbc12af56754 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Tue, 31 May 2022 10:30:21 -0400 Subject: [PATCH] Merge pull request #8132 from overleaf/em-node-fetch-filestore Replace request with node-fetch in filestore GitOrigin-RevId: 49487a941b63655920de04fe50fd197f67498e58 --- package-lock.json | 6 +- services/filestore/package.json | 3 +- .../test/acceptance/js/FilestoreTests.js | 346 ++++++++---------- .../test/acceptance/js/TestHelper.js | 11 +- 4 files changed, 155 insertions(+), 211 deletions(-) diff --git a/package-lock.json b/package-lock.json index 83e1e8a1dd..56d4c25339 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32772,10 +32772,9 @@ "express": "^4.17.1", "glob": "^7.1.6", "lodash.once": "^4.1.1", + "node-fetch": "^2.6.7", "node-uuid": "~1.4.8", "range-parser": "^1.2.1", - "request": "^2.88.2", - "request-promise-native": "^1.0.8", "stream-buffers": "~0.2.6", "tiny-async-pool": "^1.1.0" }, @@ -40734,10 +40733,9 @@ "lodash.once": "^4.1.1", "mocha": "^8.4.0", "mongodb": "^3.5.9", + "node-fetch": "^2.6.7", "node-uuid": "~1.4.8", "range-parser": "^1.2.1", - "request": "^2.88.2", - "request-promise-native": "^1.0.8", "sandboxed-module": "2.0.4", "sinon": "9.0.2", "sinon-chai": "^3.7.0", diff --git a/services/filestore/package.json b/services/filestore/package.json index b7c237cdac..b7026232fa 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -28,10 +28,9 @@ "express": "^4.17.1", "glob": "^7.1.6", "lodash.once": "^4.1.1", + "node-fetch": "^2.6.7", "node-uuid": "~1.4.8", "range-parser": "^1.2.1", - "request": "^2.88.2", - "request-promise-native": "^1.0.8", "stream-buffers": "~0.2.6", "tiny-async-pool": "^1.1.0" }, diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index 2777e79728..ab9261901c 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -5,12 +5,8 @@ const Settings = require('@overleaf/settings') const Path = require('path') const FilestoreApp = require('./FilestoreApp') const TestHelper = require('./TestHelper') -const rp = require('request-promise-native').defaults({ - resolveWithFullResponse: true, -}) +const fetch = require('node-fetch') const S3 = require('aws-sdk/clients/s3') -const Stream = require('stream') -const request = require('request') const { promisify } = require('util') const { Storage } = require('@google-cloud/storage') const streamifier = require('streamifier') @@ -21,7 +17,6 @@ const ChildProcess = require('child_process') const fsWriteFile = promisify(fs.writeFile) const fsStat = promisify(fs.stat) -const pipeline = promisify(Stream.pipeline) const exec = promisify(ChildProcess.exec) const msleep = promisify(setTimeout) @@ -132,16 +127,18 @@ describe('Filestore', function () { }) it('should send a 200 for the status endpoint', async function () { - const response = await rp(`${filestoreUrl}/status`) - expect(response.statusCode).to.equal(200) - expect(response.body).to.contain('filestore') - expect(response.body).to.contain('up') + const response = await fetch(`${filestoreUrl}/status`) + expect(response.status).to.equal(200) + const body = await response.text() + expect(body).to.contain('filestore') + expect(body).to.contain('up') }) it('should send a 200 for the health-check endpoint', async function () { - const response = await rp(`${filestoreUrl}/health_check`) - expect(response.statusCode).to.equal(200) - expect(response.body).to.equal('OK') + const response = await fetch(`${filestoreUrl}/health_check`) + expect(response.status).to.equal(200) + const body = await response.text() + expect(body).to.equal('OK') }) describe('with a file on the server', function () { @@ -161,11 +158,8 @@ describe('Filestore', function () { await fsWriteFile(localFileReadPath, constantFileContent) - const writeStream = request.post(fileUrl) const readStream = fs.createReadStream(localFileReadPath) - // hack to consume the result to ensure the http request has been fully processed - const resultStream = fs.createWriteStream('/dev/null') - await pipeline(readStream, writeStream, resultStream) + await fetch(fileUrl, { method: 'POST', body: readStream }) }) beforeEach(async function retrievePreviousIngressMetrics() { @@ -181,93 +175,82 @@ describe('Filestore', function () { }) it('should return 404 for a non-existant id', async function () { - const options = { uri: fileUrl + '___this_is_clearly_wrong___' } - await expect( - rp.get(options) - ).to.eventually.be.rejected.and.have.property('statusCode', 404) + const url = fileUrl + '___this_is_clearly_wrong___' + const response = await fetch(url) + expect(response.status).to.equal(404) }) it('should return the file size on a HEAD request', async function () { const expectedLength = Buffer.byteLength(constantFileContent) - const res = await rp.head(fileUrl) - expect(res.statusCode).to.equal(200) - expect(res.headers['content-length']).to.equal( + const res = await fetch(fileUrl, { method: 'HEAD' }) + expect(res.status).to.equal(200) + expect(res.headers.get('Content-Length')).to.equal( expectedLength.toString() ) }) it('should be able get the file back', async function () { - const res = await rp.get(fileUrl) - expect(res.body).to.equal(constantFileContent) + const res = await fetch(fileUrl) + const body = await res.text() + expect(body).to.equal(constantFileContent) }) it('should not leak a socket', async function () { - await rp.get(fileUrl) + await fetch(fileUrl) await expectNoSockets() }) it('should be able to get back the first 9 bytes of the file', async function () { - const options = { - uri: fileUrl, - headers: { - Range: 'bytes=0-8', - }, - } - const res = await rp.get(options) - expect(res.body).to.equal('hello wor') + const res = await fetch(fileUrl, { headers: { Range: 'bytes=0-8' } }) + const body = await res.text() + expect(body).to.equal('hello wor') }) it('should be able to get back bytes 4 through 10 of the file', async function () { - const options = { - uri: fileUrl, - headers: { - Range: 'bytes=4-10', - }, - } - const res = await rp.get(options) - expect(res.body).to.equal('o world') + const res = await fetch(fileUrl, { headers: { Range: 'bytes=4-10' } }) + const body = await res.text() + expect(body).to.equal('o world') }) it('should be able to delete the file', async function () { - const response = await rp.del(fileUrl) - expect(response.statusCode).to.equal(204) - await expect( - rp.get(fileUrl) - ).to.eventually.be.rejected.and.have.property('statusCode', 404) + const response = await fetch(fileUrl, { method: 'DELETE' }) + expect(response.status).to.equal(204) + const response2 = await fetch(fileUrl) + expect(response2.status).to.equal(404) }) it('should be able to copy files', async function () { const newProjectID = ObjectId().toString() const newFileId = ObjectId().toString() const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${newFileId}` - const opts = { - method: 'put', - uri: newFileUrl, - json: { + let response = await fetch(newFileUrl, { + method: 'PUT', + body: JSON.stringify({ source: { project_id: projectId, file_id: fileId, }, + }), + headers: { + 'Content-Type': 'application/json', }, - } - let response = await rp(opts) - expect(response.statusCode).to.equal(200) - response = await rp.del(fileUrl) - expect(response.statusCode).to.equal(204) - response = await rp.get(newFileUrl) - expect(response.body).to.equal(constantFileContent) + }) + expect(response.status).to.equal(200) + response = await fetch(fileUrl, { method: 'DELETE' }) + expect(response.status).to.equal(204) + response = await fetch(newFileUrl) + const body = await response.text() + expect(body).to.equal(constantFileContent) }) it('should be able to overwrite the file', async function () { const newContent = `here is some different content, ${Math.random()}` - const writeStream = request.post(fileUrl) const readStream = streamifier.createReadStream(newContent) - // hack to consume the result to ensure the http request has been fully processed - const resultStream = fs.createWriteStream('/dev/null') - await pipeline(readStream, writeStream, resultStream) + await fetch(fileUrl, { method: 'POST', body: readStream }) - const response = await rp.get(fileUrl) - expect(response.body).to.equal(newContent) + const response = await fetch(fileUrl) + const body = await response.text() + expect(body).to.equal(newContent) }) if (['S3Persistor', 'GcsPersistor'].includes(backend)) { @@ -280,7 +263,8 @@ describe('Filestore', function () { }) it('should record an ingress metric when downloading the file', async function () { - await rp.get(fileUrl) + const response = await fetch(fileUrl) + expect(response.ok).to.be.true const metric = await TestHelper.getMetric( filestoreUrl, `${metricPrefix}_ingress` @@ -291,13 +275,10 @@ describe('Filestore', function () { }) it('should record an ingress metric for a partial download', async function () { - const options = { - uri: fileUrl, - headers: { - Range: 'bytes=0-8', - }, - } - await rp.get(options) + const response = await fetch(fileUrl, { + headers: { Range: 'bytes=0-8' }, + }) + expect(response.ok).to.be.true const metric = await TestHelper.getMetric( filestoreUrl, `${metricPrefix}_ingress` @@ -354,72 +335,62 @@ describe('Filestore', function () { ] otherFileUrls = [`${otherProjectUrl}/file/${fileIds[2]}`] - const writeStreams = [ - request.post(fileUrls[0]), - request.post(fileUrls[1]), - request.post(otherFileUrls[0]), - ] - const readStreams = [ - fs.createReadStream(localFileReadPaths[0]), - fs.createReadStream(localFileReadPaths[1]), - fs.createReadStream(localFileReadPaths[2]), - ] - // hack to consume the result to ensure the http request has been fully processed - const resultStreams = [ - fs.createWriteStream('/dev/null'), - fs.createWriteStream('/dev/null'), - fs.createWriteStream('/dev/null'), - ] - return Promise.all([ - pipeline(readStreams[0], writeStreams[0], resultStreams[0]), - pipeline(readStreams[1], writeStreams[1], resultStreams[1]), - pipeline(readStreams[2], writeStreams[2], resultStreams[2]), + await Promise.all([ + fetch(fileUrls[0], { + method: 'POST', + body: fs.createReadStream(localFileReadPaths[0]), + }), + fetch(fileUrls[1], { + method: 'POST', + body: fs.createReadStream(localFileReadPaths[1]), + }), + fetch(otherFileUrls[0], { + method: 'POST', + body: fs.createReadStream(localFileReadPaths[2]), + }), ]) }) it('should get the directory size', async function () { - const response = await rp.get( + const response = await fetch( `${filestoreUrl}/project/${projectId}/size` ) - expect(parseInt(JSON.parse(response.body)['total bytes'])).to.equal( + const body = await response.text() + expect(parseInt(JSON.parse(body)['total bytes'])).to.equal( constantFileContents[0].length + constantFileContents[1].length ) }) it('should store the files', async function () { for (const index in fileUrls) { - await expect(rp.get(fileUrls[index])).to.eventually.have.property( - 'body', - constantFileContents[index] - ) + const response = await fetch(fileUrls[index]) + const body = await response.text() + expect(body).to.equal(constantFileContents[index]) } }) it('should be able to delete the project', async function () { - await expect(rp.delete(projectUrl)).to.eventually.have.property( - 'statusCode', - 204 - ) + let response = await fetch(projectUrl, { method: 'DELETE' }) + expect(response.status).to.equal(204) for (const index in fileUrls) { - await expect( - rp.get(fileUrls[index]) - ).to.eventually.be.rejected.and.have.property('statusCode', 404) + response = await fetch(fileUrls[index]) + expect(response.status).to.equal(404) } }) it('should not delete files in other projects', async function () { for (const index in otherFileUrls) { - await expect( - rp.get(otherFileUrls[index]) - ).to.eventually.have.property('statusCode', 200) + const response = await fetch(otherFileUrls[index]) + expect(response.status).to.equal(200) } }) it('should not delete a partial project id', async function () { - await expect( - rp.delete(`${filestoreUrl}/project/5`) - ).to.eventually.be.rejected.and.have.property('statusCode', 400) + const response = await fetch(`${filestoreUrl}/project/5`, { + method: 'DELETE', + }) + expect(response.status).to.equal(400) }) }) @@ -433,21 +404,14 @@ describe('Filestore', function () { largeFileContent = '_wombat_'.repeat(1024 * 1024) // 8 megabytes largeFileContent += Math.random() - const writeStream = request.post(fileUrl) const readStream = streamifier.createReadStream(largeFileContent) - // hack to consume the result to ensure the http request has been fully processed - const resultStream = fs.createWriteStream('/dev/null') - - try { - await pipeline(readStream, writeStream, resultStream) - } catch (err) { - error = err - } + await fetch(fileUrl, { method: 'POST', body: readStream }) }) it('should be able to get the file back', async function () { - const response = await rp.get(fileUrl) - expect(response.body).to.equal(largeFileContent) + const response = await fetch(fileUrl) + const body = await response.text() + expect(body).to.equal(largeFileContent) }) it('should not throw an error', function () { @@ -455,27 +419,17 @@ describe('Filestore', function () { }) it('should not leak a socket', async function () { - await rp.get(fileUrl) + const response = await fetch(fileUrl) + await response.text() await expectNoSockets() }) it('should not leak a socket if the connection is aborted', async function () { - this.timeout(20000) - for (let i = 0; i < 5; i++) { - // test is not 100% reliable, so repeat - // create a new connection and have it time out before reading any data - await new Promise(resolve => { - const streamThatHangs = new Stream.PassThrough() - const stream = request({ url: fileUrl, timeout: 1000 }) - stream.pipe(streamThatHangs) - stream.on('error', () => { - stream.destroy() - streamThatHangs.destroy() - resolve() - }) - }) - await expectNoSockets() - } + const controller = new AbortController() + const response = await fetch(fileUrl, { signal: controller.signal }) + expect(response.ok).to.be.true + controller.abort() + await expectNoSockets() }) }) @@ -515,8 +469,9 @@ describe('Filestore', function () { }) it('should get the file from the specified bucket', async function () { - const response = await rp.get(fileUrl) - expect(response.body).to.equal(constantFileContent) + const response = await fetch(fileUrl) + const body = await response.text() + expect(body).to.equal(constantFileContent) }) }) } @@ -533,17 +488,9 @@ describe('Filestore', function () { content = '_wombat_' + Math.random() - const writeStream = request.post(fileUrl) const readStream = streamifier.createReadStream(content) - // hack to consume the result to ensure the http request has been fully processed - const resultStream = fs.createWriteStream('/dev/null') - - try { - await pipeline(readStream, writeStream, resultStream) - await rp.delete(fileUrl) - } catch (err) { - error = err - } + await fetch(fileUrl, { method: 'POST', body: readStream }) + await fetch(fileUrl, { method: 'DELETE' }) }) afterEach(function () { @@ -625,12 +572,14 @@ describe('Filestore', function () { }) it('should fetch the file', async function () { - const res = await rp.get(fileUrl) - expect(res.body).to.equal(constantFileContent) + const res = await fetch(fileUrl) + const body = await res.text() + expect(body).to.equal(constantFileContent) }) it('should not copy the file to the primary', async function () { - await rp.get(fileUrl) + const response = await fetch(fileUrl) + expect(response.ok).to.be.true await TestHelper.expectPersistorNotToHaveFile( app.persistor.primaryPersistor, @@ -646,12 +595,14 @@ describe('Filestore', function () { }) it('should fetch the file', async function () { - const res = await rp.get(fileUrl) - expect(res.body).to.equal(constantFileContent) + const res = await fetch(fileUrl) + const body = await res.text() + expect(body).to.equal(constantFileContent) }) it('copies the file to the primary', async function () { - await rp.get(fileUrl) + const response = await fetch(fileUrl) + expect(response.ok).to.be.true // wait for the file to copy in the background await msleep(1000) @@ -675,12 +626,14 @@ describe('Filestore', function () { opts = { method: 'put', - uri: newFileUrl, - json: { + body: JSON.stringify({ source: { project_id: projectId, file_id: fileId, }, + }), + headers: { + 'Content-Type': 'application/json', }, } }) @@ -689,8 +642,8 @@ describe('Filestore', function () { beforeEach(async function () { app.persistor.settings.copyOnMiss = false - const response = await rp(opts) - expect(response.statusCode).to.equal(200) + const response = await fetch(newFileUrl, opts) + expect(response.status).to.equal(200) }) it('should leave the old file in the old bucket', async function () { @@ -735,8 +688,8 @@ describe('Filestore', function () { beforeEach(async function () { app.persistor.settings.copyOnMiss = true - const response = await rp(opts) - expect(response.statusCode).to.equal(200) + const response = await fetch(newFileUrl, opts) + expect(response.status).to.equal(200) }) it('should leave the old file in the old bucket', async function () { @@ -782,12 +735,9 @@ describe('Filestore', function () { describe('when sending a file', function () { beforeEach(async function () { - const writeStream = request.post(fileUrl) const readStream = streamifier.createReadStream(constantFileContent) - // hack to consume the result to ensure the http request has been fully processed - const resultStream = fs.createWriteStream('/dev/null') - await pipeline(readStream, writeStream, resultStream) + await fetch(fileUrl, { method: 'POST', body: readStream }) }) it('should store the file on the primary', async function () { @@ -820,11 +770,10 @@ describe('Filestore', function () { }) it('should delete the file', async function () { - const response = await rp.del(fileUrl) - expect(response.statusCode).to.equal(204) - await expect( - rp.get(fileUrl) - ).to.eventually.be.rejected.and.have.property('statusCode', 404) + const response1 = await fetch(fileUrl, { method: 'DELETE' }) + expect(response1.status).to.equal(204) + const response2 = await fetch(fileUrl) + expect(response2.status).to.equal(404) }) }) @@ -839,11 +788,10 @@ describe('Filestore', function () { }) it('should delete the file', async function () { - const response = await rp.del(fileUrl) - expect(response.statusCode).to.equal(204) - await expect( - rp.get(fileUrl) - ).to.eventually.be.rejected.and.have.property('statusCode', 404) + const response1 = await fetch(fileUrl, { method: 'DELETE' }) + expect(response1.status).to.equal(204) + const response2 = await fetch(fileUrl) + expect(response2.status).to.equal(404) }) }) @@ -864,11 +812,10 @@ describe('Filestore', function () { }) it('should delete the files', async function () { - const response = await rp.del(fileUrl) - expect(response.statusCode).to.equal(204) - await expect( - rp.get(fileUrl) - ).to.eventually.be.rejected.and.have.property('statusCode', 404) + const response1 = await fetch(fileUrl, { method: 'DELETE' }) + expect(response1.status).to.equal(204) + const response2 = await fetch(fileUrl) + expect(response2.status).to.equal(404) }) }) @@ -876,8 +823,8 @@ describe('Filestore', function () { it('should return return 204', async function () { // S3 doesn't give us a 404 when the object doesn't exist, so to stay // consistent we merrily return 204 ourselves here as well - const response = await rp.del(fileUrl) - expect(response.statusCode).to.equal(204) + const response = await fetch(fileUrl, { method: 'DELETE' }) + expect(response.status).to.equal(204) }) }) }) @@ -896,15 +843,14 @@ describe('Filestore', function () { fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}` const stat = await fsStat(localFileReadPath) localFileSize = stat.size - const writeStream = request.post(fileUrl) - const endStream = fs.createWriteStream('/dev/null') const readStream = fs.createReadStream(localFileReadPath) - await pipeline(readStream, writeStream, endStream) + await fetch(fileUrl, { method: 'POST', body: readStream }) }) it('should be able get the file back', async function () { - const response = await rp.get(fileUrl) - expect(response.body.substring(0, 8)).to.equal('%PDF-1.5') + const response = await fetch(fileUrl) + const body = await response.text() + expect(body.substring(0, 8)).to.equal('%PDF-1.5') }) if (['S3Persistor', 'GcsPersistor'].includes(backend)) { @@ -926,15 +872,16 @@ describe('Filestore', function () { }) it('should not time out', async function () { - const response = await rp.get(previewFileUrl) - expect(response.statusCode).to.equal(200) + const response = await fetch(previewFileUrl) + expect(response.status).to.equal(200) }) it('should respond with image data', async function () { // note: this test relies of the imagemagick conversion working - const response = await rp.get(previewFileUrl) - expect(response.body.length).to.be.greaterThan(400) - expect(response.body.substr(1, 3)).to.equal('PNG') + const response = await fetch(previewFileUrl) + const body = await response.text() + expect(body.length).to.be.greaterThan(400) + expect(body.substr(1, 3)).to.equal('PNG') }) }) @@ -947,14 +894,15 @@ describe('Filestore', function () { }) it('should not time out', async function () { - const response = await rp.get(previewFileUrl) - expect(response.statusCode).to.equal(200) + const response = await fetch(previewFileUrl) + expect(response.status).to.equal(200) }) it("should respond with only an 'OK'", async function () { // note: this test relies of the imagemagick conversion working - const response = await rp.get(previewFileUrl) - expect(response.body).to.equal('OK') + const response = await fetch(previewFileUrl) + const body = await response.text() + expect(body).to.equal('OK') }) }) }) diff --git a/services/filestore/test/acceptance/js/TestHelper.js b/services/filestore/test/acceptance/js/TestHelper.js index 6773a2c400..eb4e33c0d1 100644 --- a/services/filestore/test/acceptance/js/TestHelper.js +++ b/services/filestore/test/acceptance/js/TestHelper.js @@ -1,7 +1,5 @@ const streamifier = require('streamifier') -const rp = require('request-promise-native').defaults({ - resolveWithFullResponse: true, -}) +const fetch = require('node-fetch') const { expect } = require('chai') @@ -15,10 +13,11 @@ module.exports = { } async function getMetric(filestoreUrl, metric) { - const res = await rp.get(`${filestoreUrl}/metrics`) - expect(res.statusCode).to.equal(200) + const res = await fetch(`${filestoreUrl}/metrics`) + expect(res.status).to.equal(200) const metricRegex = new RegExp(`^${metric}{[^}]+} ([0-9]+)$`, 'm') - const found = metricRegex.exec(res.body) + const body = await res.text() + const found = metricRegex.exec(body) return parseInt(found ? found[1] : 0) || 0 }