From 5e33b68753ea53a5207dc08e3fd04d39697328d8 Mon Sep 17 00:00:00 2001 From: Florent Date: Thu, 20 Jul 2023 12:16:58 +0200 Subject: [PATCH] Issue 359 support scaleway (#577) * Fix support of Scaleway S3 bucket #359 While MinIO and AWS return versionId as strings, other S3 API implementations return versionId as integers. We must carefully convert the versionId as string in order to cover these various behaviors. Also ensure that docStorage is initialized before attempting to calculate the data size in order to avoid an exception. * Add unit tests for MinIOExternalStorage#versions() #359 Introduced some unit tests to : - ensure listObjects is called with the right arguments; - cover the case when a S3 bucket implementation does not return the versionId as a string but rather as an integer (like Scaleway): in such a case, ensure that the returned snapshotId is a string; - cover the case when the listObjects function emits an error, ensure the versions() call rejets with the error emitted; - that the deleteMarkers are only returned when the includeDeleteMarkers is passed; --------- Co-authored-by: Florent FAYOLLE --- app/server/lib/ActiveDoc.ts | 3 + app/server/lib/MinIOExternalStorage.ts | 29 +++-- test/server/lib/MinIOExternalStorage.ts | 157 ++++++++++++++++++++++++ 3 files changed, 177 insertions(+), 12 deletions(-) create mode 100644 test/server/lib/MinIOExternalStorage.ts diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index c37f405e..929ff7a9 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -2134,6 +2134,9 @@ export class ActiveDoc extends EventEmitter { private async _checkDataSizeLimitRatio(docSession: OptDocSession | null) { const start = Date.now(); + if (!this.docStorage.isInitialized()) { + return; + } const dataSizeBytes = await this._updateDataSize(); const timeToMeasure = Date.now() - start; log.rawInfo('Data size from dbstat...', { diff --git a/app/server/lib/MinIOExternalStorage.ts b/app/server/lib/MinIOExternalStorage.ts index fe70ea4b..73258887 100644 --- a/app/server/lib/MinIOExternalStorage.ts +++ b/app/server/lib/MinIOExternalStorage.ts @@ -25,19 +25,21 @@ type MinIOBucketItemStat = minio.BucketItemStat & { * will work with MinIO and other S3-compatible storage. */ export class MinIOExternalStorage implements ExternalStorage { - private _s3: MinIOClient; - // Specify bucket to use, and optionally the max number of keys to request // in any call to listObjectVersions (used for testing) - constructor(public bucket: string, public options: { - endPoint: string, - port?: number, - useSSL?: boolean, - accessKey: string, - secretKey: string, - region: string - }, private _batchSize?: number) { - this._s3 = new minio.Client(options) as MinIOClient; + constructor( + public bucket: string, + public options: { + endPoint: string, + port?: number, + useSSL?: boolean, + accessKey: string, + secretKey: string, + region: string + }, + private _batchSize?: number, + private _s3 = new minio.Client(options) as MinIOClient + ) { } public async exists(key: string, snapshotId?: string) { @@ -131,7 +133,10 @@ export class MinIOExternalStorage implements ExternalStorage { (options?.includeDeleteMarkers || !(v as any).isDeleteMarker)) .map(v => ({ lastModified: v.lastModified.toISOString(), - snapshotId: (v as any).versionId!, + // Circumvent inconsistency of MinIO API with versionId by casting it to string + // PR to MinIO so we don't have to do that anymore: + // https://github.com/minio/minio-js/pull/1193 + snapshotId: String((v as any).versionId!), })); } diff --git a/test/server/lib/MinIOExternalStorage.ts b/test/server/lib/MinIOExternalStorage.ts new file mode 100644 index 00000000..915484a2 --- /dev/null +++ b/test/server/lib/MinIOExternalStorage.ts @@ -0,0 +1,157 @@ +import * as minio from "minio"; +import sinon from "sinon"; +import * as stream from "node:stream"; + +import {MinIOExternalStorage} from "app/server/lib/MinIOExternalStorage"; +import {assert} from "chai"; + +describe("MinIOExternalStorage", function () { + const sandbox = sinon.createSandbox(); + const FakeClientClass = class extends minio.Client { + public listObjects( + bucket: string, + key: string, + recursive: boolean, + options?: {IncludeVersion?: boolean} + ): minio.BucketStream { + return new stream.Readable(); + } + }; + const dummyBucket = 'some-bucket'; + const dummyOptions = { + endPoint: 'some-endpoint', + accessKey: 'some-accessKey', + secretKey: 'some-secretKey', + region: 'some-region', + }; + afterEach(function () { + sandbox.restore(); + }); + + describe('versions()', function () { + function makeFakeStream(listedObjects: object[]) { + const fakeStream = new stream.Readable({objectMode: true}); + const readSpy = sandbox.stub(fakeStream, "_read"); + for (const [index, obj] of listedObjects.entries()) { + readSpy.onCall(index).callsFake(() => fakeStream.push(obj)); + } + readSpy.onCall(listedObjects.length).callsFake(() => fakeStream.push(null)); + return {fakeStream, readSpy}; + } + + it("should call listObjects with the right arguments", async function () { + const s3 = sandbox.createStubInstance(FakeClientClass); + const key = "some-key"; + const expectedRecursive = false; + const expectedOptions = {IncludeVersion: true}; + const {fakeStream} = makeFakeStream([]); + + s3.listObjects.returns(fakeStream); + + const extStorage = new MinIOExternalStorage(dummyBucket, dummyOptions, 42, s3); + const result = await extStorage.versions(key); + + assert.deepEqual(result, []); + assert.isTrue(s3.listObjects.calledWith(dummyBucket, key, expectedRecursive, expectedOptions)); + }); + + // This test can be removed once this PR is merged: https://github.com/minio/minio-js/pull/1193 + // and when the minio-js version used as a dependency includes that patch. + // + // For more context: https://github.com/gristlabs/grist-core/pull/577 + it("should return versionId's as string when return snapshotId is an integer", async function () { + // given + const s3 = sandbox.createStubInstance(FakeClientClass); + const key = "some-key"; + const versionId = 123; + const lastModified = new Date(); + const {fakeStream, readSpy} = makeFakeStream([ + { + name: key, + lastModified, + versionId, + } + ]); + + s3.listObjects.returns(fakeStream); + const extStorage = new MinIOExternalStorage(dummyBucket, dummyOptions, 42, s3); + // when + const result = await extStorage.versions(key); + // then + assert.equal(readSpy.callCount, 2); + assert.deepEqual(result, [{ + lastModified: lastModified.toISOString(), + snapshotId: String(versionId) + }]); + }); + + it("should include markers only when asked through options", async function () { + // given + const s3 = sandbox.createStubInstance(FakeClientClass); + const key = "some-key"; + const lastModified = new Date(); + const objectsFromS3 = [ + { + name: key, + lastModified, + versionId: 'regular-version-uuid', + isDeleteMarker: false + }, + { + name: key, + lastModified, + versionId: 'delete-marker-version-uuid', + isDeleteMarker: true + } + ]; + let {fakeStream} = makeFakeStream(objectsFromS3); + + s3.listObjects.returns(fakeStream); + const extStorage = new MinIOExternalStorage(dummyBucket, dummyOptions, 42, s3); + + // when + const result = await extStorage.versions(key); + + // then + assert.deepEqual(result, [{ + lastModified: lastModified.toISOString(), + snapshotId: objectsFromS3[0].versionId + }]); + + // given + fakeStream = makeFakeStream(objectsFromS3).fakeStream; + s3.listObjects.returns(fakeStream); + + // when + const resultWithDeleteMarkers = await extStorage.versions(key, {includeDeleteMarkers: true}); + + // then + assert.deepEqual(resultWithDeleteMarkers, [{ + lastModified: lastModified.toISOString(), + snapshotId: objectsFromS3[0].versionId + }, { + lastModified: lastModified.toISOString(), + snapshotId: objectsFromS3[1].versionId + }]); + }); + + it("should reject when an error occurs while listing objects", function () { + // given + const s3 = sandbox.createStubInstance(FakeClientClass); + const key = "some-key"; + const fakeStream = new stream.Readable({objectMode: true}); + const error = new Error("dummy-error"); + sandbox.stub(fakeStream, "_read") + .returns(fakeStream) + .callsFake(() => fakeStream.emit('error', error)); + s3.listObjects.returns(fakeStream); + const extStorage = new MinIOExternalStorage(dummyBucket, dummyOptions, 42, s3); + + // when + const result = extStorage.versions(key); + + // then + return assert.isRejected(result, error); + }); + }); +}); \ No newline at end of file