From 7b3e39f63f4409b5a818b6ef899a68d84cdc3344 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 31 Oct 2024 13:08:41 +0100 Subject: [PATCH] Merge pull request #21375 from overleaf/jpa-if-none-match [object-persistor] add support for ifNoneMatch=* in sendStream GitOrigin-RevId: 268f054ac1b6452105b02757cdec32bad00702fd --- libraries/object-persistor/package.json | 2 +- libraries/object-persistor/src/Errors.js | 2 + libraries/object-persistor/src/FSPersistor.js | 12 +++- .../object-persistor/src/GcsPersistor.js | 8 ++- .../object-persistor/src/PersistorHelper.js | 9 ++- libraries/object-persistor/src/S3Persistor.js | 5 +- package-lock.json | 55 ++++++++++++++++++- .../test/acceptance/js/FilestoreTests.js | 38 +++++++++++++ 8 files changed, 122 insertions(+), 9 deletions(-) diff --git a/libraries/object-persistor/package.json b/libraries/object-persistor/package.json index 3aed927437..77b88d75ed 100644 --- a/libraries/object-persistor/package.json +++ b/libraries/object-persistor/package.json @@ -24,7 +24,7 @@ "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", - "aws-sdk": "^2.718.0", + "aws-sdk": "^2.1691.0", "fast-crc32c": "overleaf/node-fast-crc32c#aae6b2a4c7a7a159395df9cc6c38dfde702d6f51", "glob": "^7.1.6", "range-parser": "^1.2.1", diff --git a/libraries/object-persistor/src/Errors.js b/libraries/object-persistor/src/Errors.js index 0e32a3efe0..330ffe42b5 100644 --- a/libraries/object-persistor/src/Errors.js +++ b/libraries/object-persistor/src/Errors.js @@ -5,6 +5,7 @@ class WriteError extends OError {} class ReadError extends OError {} class SettingsError extends OError {} class NotImplementedError extends OError {} +class AlreadyWrittenError extends OError {} module.exports = { NotFoundError, @@ -12,4 +13,5 @@ module.exports = { ReadError, SettingsError, NotImplementedError, + AlreadyWrittenError, } diff --git a/libraries/object-persistor/src/FSPersistor.js b/libraries/object-persistor/src/FSPersistor.js index 3752ad3756..df2c82b1bc 100644 --- a/libraries/object-persistor/src/FSPersistor.js +++ b/libraries/object-persistor/src/FSPersistor.js @@ -8,7 +8,7 @@ const { pipeline } = require('stream/promises') const { promisify } = require('util') const AbstractPersistor = require('./AbstractPersistor') -const { ReadError, WriteError } = require('./Errors') +const { ReadError, WriteError, NotImplementedError } = require('./Errors') const PersistorHelper = require('./PersistorHelper') const glob = promisify(globCallbacks) @@ -36,6 +36,14 @@ module.exports = class FSPersistor extends AbstractPersistor { } async sendStream(location, target, sourceStream, opts = {}) { + if (opts.ifNoneMatch === '*') { + // The standard library only has fs.rename(), which does not support exclusive flags. + // Refuse to act on this write operation. + throw new NotImplementedError( + 'Overwrite protection required by caller, but it is not available is FS backend. Configure GCS or S3 backend instead, get in touch with support for further information.' + ) + } + const targetPath = this._getFsPath(location, target) try { @@ -55,7 +63,7 @@ module.exports = class FSPersistor extends AbstractPersistor { throw PersistorHelper.wrapError( err, 'failed to write stream', - { location, target }, + { location, target, ifNoneMatch: opts.ifNoneMatch }, WriteError ) } diff --git a/libraries/object-persistor/src/GcsPersistor.js b/libraries/object-persistor/src/GcsPersistor.js index b8c542bb62..ebe9f92ebc 100644 --- a/libraries/object-persistor/src/GcsPersistor.js +++ b/libraries/object-persistor/src/GcsPersistor.js @@ -78,10 +78,14 @@ module.exports = class GcsPersistor extends AbstractPersistor { writeOptions.metadata = writeOptions.metadata || {} writeOptions.metadata.contentEncoding = opts.contentEncoding } + const fileOptions = {} + if (opts.ifNoneMatch === '*') { + fileOptions.generation = 0 + } const uploadStream = this.storage .bucket(bucketName) - .file(key) + .file(key, fileOptions) .createWriteStream(writeOptions) await pipeline(readStream, observer, uploadStream) @@ -97,7 +101,7 @@ module.exports = class GcsPersistor extends AbstractPersistor { throw PersistorHelper.wrapError( err, 'upload to GCS failed', - { bucketName, key }, + { bucketName, key, ifNoneMatch: opts.ifNoneMatch }, WriteError ) } diff --git a/libraries/object-persistor/src/PersistorHelper.js b/libraries/object-persistor/src/PersistorHelper.js index ac34b768cb..087846e476 100644 --- a/libraries/object-persistor/src/PersistorHelper.js +++ b/libraries/object-persistor/src/PersistorHelper.js @@ -3,7 +3,7 @@ const Stream = require('stream') const { pipeline } = require('stream/promises') const Logger = require('@overleaf/logger') const Metrics = require('@overleaf/metrics') -const { WriteError, NotFoundError } = require('./Errors') +const { WriteError, NotFoundError, AlreadyWrittenError } = require('./Errors') const _128KiB = 128 * 1024 const TIMING_BUCKETS = [ @@ -146,6 +146,13 @@ function wrapError(error, message, params, ErrorType) { (error.response && error.response.statusCode === 404) ) { return new NotFoundError('no such file', params, error) + } else if ( + params.ifNoneMatch === '*' && + (error.code === 'PreconditionFailed' || + error.response?.statusCode === 412 || + error instanceof AlreadyWrittenError) + ) { + return new AlreadyWrittenError(message, params, error) } else { return new ErrorType(message, params, error) } diff --git a/libraries/object-persistor/src/S3Persistor.js b/libraries/object-persistor/src/S3Persistor.js index 8df65d80f0..f0c834b185 100644 --- a/libraries/object-persistor/src/S3Persistor.js +++ b/libraries/object-persistor/src/S3Persistor.js @@ -52,6 +52,9 @@ module.exports = class S3Persistor extends AbstractPersistor { if (opts.contentEncoding) { uploadOptions.ContentEncoding = opts.contentEncoding } + if (opts.ifNoneMatch === '*') { + uploadOptions.IfNoneMatch = '*' + } // if we have an md5 hash, pass this to S3 to verify the upload - otherwise // we rely on the S3 client's checksum calculation to validate the upload @@ -69,7 +72,7 @@ module.exports = class S3Persistor extends AbstractPersistor { throw PersistorHelper.wrapError( err, 'upload to S3 failed', - { bucketName, key }, + { bucketName, key, ifNoneMatch: opts.ifNoneMatch }, WriteError ) } diff --git a/package-lock.json b/package-lock.json index 1670186422..daa771c6fb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -258,7 +258,7 @@ "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", - "aws-sdk": "^2.718.0", + "aws-sdk": "^2.1691.0", "fast-crc32c": "overleaf/node-fast-crc32c#aae6b2a4c7a7a159395df9cc6c38dfde702d6f51", "glob": "^7.1.6", "range-parser": "^1.2.1", @@ -276,6 +276,27 @@ "typescript": "^5.0.4" } }, + "libraries/object-persistor/node_modules/aws-sdk": { + "version": "2.1691.0", + "resolved": "https://registry.npmjs.org/aws-sdk/-/aws-sdk-2.1691.0.tgz", + "integrity": "sha512-/F2YC+DlsY3UBM2Bdnh5RLHOPNibS/+IcjUuhP8XuctyrN+MlL+fWDAiela32LTDk7hMy4rx8MTgvbJ+0blO5g==", + "hasInstallScript": true, + "dependencies": { + "buffer": "4.9.2", + "events": "1.1.1", + "ieee754": "1.1.13", + "jmespath": "0.16.0", + "querystring": "0.2.0", + "sax": "1.2.1", + "url": "0.10.3", + "util": "^0.12.4", + "uuid": "8.0.0", + "xml2js": "0.6.2" + }, + "engines": { + "node": ">= 10.0.0" + } + }, "libraries/object-persistor/node_modules/gcp-metadata": { "version": "5.3.0", "resolved": "https://registry.npmjs.org/gcp-metadata/-/gcp-metadata-5.3.0.tgz", @@ -337,6 +358,14 @@ } } }, + "libraries/object-persistor/node_modules/uuid": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.0.0.tgz", + "integrity": "sha512-jOXGuXZAWdsTH7eZLtyXMqUb9EcWMGZNbL9YcGBJl4MH4nrxHmZJhEHvyLFrkxo+28uLb/NYRcStH48fnD0Vzw==", + "bin": { + "uuid": "dist/bin/uuid" + } + }, "libraries/overleaf-editor-core": { "version": "1.0.0", "license": "Proprietary", @@ -49452,7 +49481,7 @@ "@overleaf/logger": "*", "@overleaf/metrics": "*", "@overleaf/o-error": "*", - "aws-sdk": "^2.718.0", + "aws-sdk": "^2.1691.0", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", "fast-crc32c": "overleaf/node-fast-crc32c#aae6b2a4c7a7a159395df9cc6c38dfde702d6f51", @@ -49468,6 +49497,23 @@ "typescript": "^5.0.4" }, "dependencies": { + "aws-sdk": { + "version": "2.1691.0", + "resolved": "https://registry.npmjs.org/aws-sdk/-/aws-sdk-2.1691.0.tgz", + "integrity": "sha512-/F2YC+DlsY3UBM2Bdnh5RLHOPNibS/+IcjUuhP8XuctyrN+MlL+fWDAiela32LTDk7hMy4rx8MTgvbJ+0blO5g==", + "requires": { + "buffer": "4.9.2", + "events": "1.1.1", + "ieee754": "1.1.13", + "jmespath": "0.16.0", + "querystring": "0.2.0", + "sax": "1.2.1", + "url": "0.10.3", + "util": "^0.12.4", + "uuid": "8.0.0", + "xml2js": "0.6.2" + } + }, "gcp-metadata": { "version": "5.3.0", "resolved": "https://registry.npmjs.org/gcp-metadata/-/gcp-metadata-5.3.0.tgz", @@ -49490,6 +49536,11 @@ "bson": "^6.1.0", "mongodb-connection-string-url": "^2.6.0" } + }, + "uuid": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.0.0.tgz", + "integrity": "sha512-jOXGuXZAWdsTH7eZLtyXMqUb9EcWMGZNbL9YcGBJl4MH4nrxHmZJhEHvyLFrkxo+28uLb/NYRcStH48fnD0Vzw==" } } }, diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index d55631fef2..eb6cf17376 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -32,6 +32,10 @@ process.on('unhandledRejection', e => { // store settings for multiple backends, so that we can test each one. // fs will always be available - add others if they are configured const { BackendSettings, s3Config } = require('./TestConfig') +const { + AlreadyWrittenError, + NotImplementedError, +} = require('@overleaf/object-persistor/src/Errors') describe('Filestore', function () { this.timeout(1000 * 10) @@ -273,6 +277,40 @@ describe('Filestore', function () { expect(body).to.equal(newContent) }) + describe('IfNoneMatch', function () { + if (backendSettings.backend === 'fs') { + it('should refuse to handle IfNoneMatch', async function () { + await expect( + app.persistor.sendStream( + Settings.filestore.stores.user_files, + `${projectId}/${fileId}`, + fs.createReadStream(localFileReadPath), + { ifNoneMatch: '*' } + ) + ).to.be.rejectedWith(NotImplementedError) + }) + } else { + it('should reject sendStream on the same key with IfNoneMatch', async function () { + await expect( + app.persistor.sendStream( + Settings.filestore.stores.user_files, + `${projectId}/${fileId}`, + fs.createReadStream(localFileReadPath), + { ifNoneMatch: '*' } + ) + ).to.be.rejectedWith(AlreadyWrittenError) + }) + it('should allow sendStream on a different key with IfNoneMatch', async function () { + await app.persistor.sendStream( + Settings.filestore.stores.user_files, + `${projectId}/${fileId}-other`, + fs.createReadStream(localFileReadPath), + { ifNoneMatch: '*' } + ) + }) + } + }) + if (backendSettings.backend !== 'fs') { it('should record an egress metric for the upload', async function () { const metric = await TestHelper.getMetric(