From db9936e07c39586a01f3b31fff1199b005780fad Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Tue, 30 Nov 2021 09:02:41 +0000 Subject: [PATCH] Standardise content type handling in MSC3089 createFile() and createNewVersion() (#2014) * Provide cross platform compatible versions of createFile() and createNewVersion() The exist implementations are deprecated as they only work in a browser and support a different type of contents from MatrixClient.uploadContent() * Fix MSC3089 content upload meta data in NodeJS runtime * Break unstable createFile() and createNewVersion() instead of deprecating Test using NodeJS types instead of mocked browser Blob * chore: remove incorrect comment --- spec/MockBlob.ts | 27 ----------------------- spec/unit/models/MSC3089Branch.spec.ts | 3 +-- spec/unit/models/MSC3089TreeSpace.spec.ts | 21 +++++------------- src/models/MSC3089Branch.ts | 7 +++--- src/models/MSC3089TreeSpace.ts | 9 +++++--- 5 files changed, 17 insertions(+), 50 deletions(-) delete mode 100644 spec/MockBlob.ts diff --git a/spec/MockBlob.ts b/spec/MockBlob.ts deleted file mode 100644 index 04d01c24e..000000000 --- a/spec/MockBlob.ts +++ /dev/null @@ -1,27 +0,0 @@ -/* -Copyright 2021 The Matrix.org Foundation C.I.C. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -export class MockBlob { - private contents: number[] = []; - - public constructor(private parts: ArrayLike[]) { - parts.forEach(p => Array.from(p).forEach(e => this.contents.push(e))); - } - - public get size(): number { - return this.contents.length; - } -} diff --git a/spec/unit/models/MSC3089Branch.spec.ts b/spec/unit/models/MSC3089Branch.spec.ts index 85e1adead..d782a4276 100644 --- a/spec/unit/models/MSC3089Branch.spec.ts +++ b/spec/unit/models/MSC3089Branch.spec.ts @@ -244,8 +244,7 @@ describe("MSC3089Branch", () => { it('should create new versions of itself', async () => { const canaryName = "canary"; - const fileContents = "contents go here"; - const canaryContents = Uint8Array.from(Array.from(fileContents).map((_, i) => fileContents.charCodeAt(i))); + const canaryContents = "contents go here"; const canaryFile = {} as IEncryptedFile; const canaryAddl = { canary: true }; indexEvent.getContent = () => ({ active: true, retained: true }); diff --git a/spec/unit/models/MSC3089TreeSpace.spec.ts b/spec/unit/models/MSC3089TreeSpace.spec.ts index bf05a7914..61d1daace 100644 --- a/spec/unit/models/MSC3089TreeSpace.spec.ts +++ b/spec/unit/models/MSC3089TreeSpace.spec.ts @@ -24,7 +24,6 @@ import { TreePermissions, } from "../../../src/models/MSC3089TreeSpace"; import { DEFAULT_ALPHABET } from "../../../src/utils"; -import { MockBlob } from "../../MockBlob"; import { MatrixError } from "../../../src/http-api"; describe("MSC3089TreeSpace", () => { @@ -887,12 +886,8 @@ describe("MSC3089TreeSpace", () => { const fileName = "My File.txt"; const fileContents = "This is a test file"; - // Mock out Blob for the test environment - (global).Blob = MockBlob; - - const uploadFn = jest.fn().mockImplementation((contents: Blob, opts: any) => { - expect(contents).toBeInstanceOf(Blob); - expect(contents.size).toEqual(fileContents.length); + const uploadFn = jest.fn().mockImplementation((contents: Buffer, opts: any) => { + expect(contents.length).toEqual(fileContents.length); expect(opts).toMatchObject({ includeFilename: false, onlyContentUri: true, // because the tests rely on this - we shouldn't really be testing for this. @@ -930,7 +925,7 @@ describe("MSC3089TreeSpace", () => { }); client.sendStateEvent = sendStateFn; - const buf = Uint8Array.from(Array.from(fileContents).map((_, i) => fileContents.charCodeAt(i))); + const buf = Buffer.from(fileContents); // We clone the file info just to make sure it doesn't get mutated for the test. const result = await tree.createFile(fileName, buf, Object.assign({}, fileInfo), { metadata: true }); @@ -951,12 +946,8 @@ describe("MSC3089TreeSpace", () => { const fileName = "My File.txt"; const fileContents = "This is a test file"; - // Mock out Blob for the test environment - (global).Blob = MockBlob; - - const uploadFn = jest.fn().mockImplementation((contents: Blob, opts: any) => { - expect(contents).toBeInstanceOf(Blob); - expect(contents.size).toEqual(fileContents.length); + const uploadFn = jest.fn().mockImplementation((contents: Buffer, opts: any) => { + expect(contents.length).toEqual(fileContents.length); expect(opts).toMatchObject({ includeFilename: false, onlyContentUri: true, // because the tests rely on this - we shouldn't really be testing for this. @@ -997,7 +988,7 @@ describe("MSC3089TreeSpace", () => { }); client.sendStateEvent = sendStateFn; - const buf = Uint8Array.from(Array.from(fileContents).map((_, i) => fileContents.charCodeAt(i))); + const buf = Buffer.from(fileContents); // We clone the file info just to make sure it doesn't get mutated for the test. const result = await tree.createFile(fileName, buf, Object.assign({}, fileInfo), { "m.new_content": true }); diff --git a/src/models/MSC3089Branch.ts b/src/models/MSC3089Branch.ts index d8f62579f..133a87068 100644 --- a/src/models/MSC3089Branch.ts +++ b/src/models/MSC3089Branch.ts @@ -18,6 +18,7 @@ import { MatrixClient } from "../client"; import { IEncryptedFile, RelationType, UNSTABLE_MSC3089_BRANCH } from "../@types/event"; import { IContent, MatrixEvent } from "./event"; import { MSC3089TreeSpace } from "./MSC3089TreeSpace"; +import type { ReadStream } from "fs"; /** * Represents a [MSC3089](https://github.com/matrix-org/matrix-doc/pull/3089) branch - a reference @@ -142,16 +143,16 @@ export class MSC3089Branch { } /** - * Creates a new version of this file. + * Creates a new version of this file with contents in a type that is compatible with MatrixClient.uploadContent(). * @param {string} name The name of the file. - * @param {ArrayBuffer} encryptedContents The encrypted contents. + * @param {File | String | Buffer | ReadStream | Blob} encryptedContents The encrypted contents. * @param {Partial} info The encrypted file information. * @param {IContent} additionalContent Optional event content fields to include in the message. * @returns {Promise} Resolves when uploaded. */ public async createNewVersion( name: string, - encryptedContents: ArrayBuffer, + encryptedContents: File | String | Buffer | ReadStream | Blob, info: Partial, additionalContent?: IContent, ): Promise { diff --git a/src/models/MSC3089TreeSpace.ts b/src/models/MSC3089TreeSpace.ts index e295264e1..40a07630b 100644 --- a/src/models/MSC3089TreeSpace.ts +++ b/src/models/MSC3089TreeSpace.ts @@ -31,6 +31,7 @@ import { MSC3089Branch } from "./MSC3089Branch"; import promiseRetry from "p-retry"; import { isRoomSharedHistory } from "../crypto/algorithms/megolm"; import { ISendEventResponse } from "../@types/requests"; +import type { ReadStream } from "fs"; /** * The recommended defaults for a tree space's power levels. Note that this @@ -449,21 +450,23 @@ export class MSC3089TreeSpace { /** * Creates (uploads) a new file to this tree. The file must have already been encrypted for the room. + * The file contents are in a type that is compatible with MatrixClient.uploadContent(). * @param {string} name The name of the file. - * @param {ArrayBuffer} encryptedContents The encrypted contents. + * @param {File | String | Buffer | ReadStream | Blob} encryptedContents The encrypted contents. * @param {Partial} info The encrypted file information. * @param {IContent} additionalContent Optional event content fields to include in the message. * @returns {Promise} Resolves to the file event's sent response. */ public async createFile( name: string, - encryptedContents: ArrayBuffer, + encryptedContents: File | String | Buffer | ReadStream | Blob, info: Partial, additionalContent?: IContent, ): Promise { - const mxc = await this.client.uploadContent(new Blob([encryptedContents]), { + const mxc = await this.client.uploadContent(encryptedContents, { includeFilename: false, onlyContentUri: true, + rawResponse: false, // make this explicit otherwise behaviour is different on browser vs NodeJS }); info.url = mxc;