From ea855df06017080fab84459ba98a7b47a8866b8b Mon Sep 17 00:00:00 2001 From: Merlijn Vos Date: Wed, 18 Jan 2023 16:12:03 +0100 Subject: [PATCH] Fixes for new metadata structure (#379) --- packages/file-store/test.ts | 2 +- packages/gcs-store/index.ts | 5 +- packages/s3-store/index.ts | 18 ++++--- packages/server/src/constants.ts | 4 ++ packages/server/src/handlers/HeadHandler.ts | 2 +- packages/server/src/handlers/PostHandler.ts | 10 +++- packages/server/src/models/Metadata.ts | 12 ++--- packages/server/src/models/Upload.ts | 2 +- packages/server/test/HeadHandler.test.ts | 2 +- packages/server/test/Metadata.test.ts | 24 +++++----- test/e2e.test.ts | 52 +++++++++++++++++---- test/stores.test.ts | 12 ++--- 12 files changed, 94 insertions(+), 51 deletions(-) diff --git a/packages/file-store/test.ts b/packages/file-store/test.ts index 9a93afa2..0d1216bd 100644 --- a/packages/file-store/test.ts +++ b/packages/file-store/test.ts @@ -71,7 +71,7 @@ describe('FileStore', function () { // @ts-expect-error todo size: this.testFileSize, offset: 0, - metadata: 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential', + metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) it("created file's size should match 'upload_length'", async function () { diff --git a/packages/gcs-store/index.ts b/packages/gcs-store/index.ts index e839844f..7ac603a6 100644 --- a/packages/gcs-store/index.ts +++ b/packages/gcs-store/index.ts @@ -43,6 +43,7 @@ export class GCSStore extends DataStore { } const gcs_file = this.bucket.file(file.id) + const options = { metadata: { metadata: { @@ -50,7 +51,7 @@ export class GCSStore extends DataStore { size: file.size, sizeIsDeferred: `${file.sizeIsDeferred}`, offset: file.offset, - metadata: file.metadata, + metadata: JSON.stringify(file.metadata), }, }, } @@ -160,7 +161,7 @@ export class GCSStore extends DataStore { id, size: size ? Number.parseInt(size, 10) : size, offset: Number.parseInt(metadata.size, 10), // `size` is set by GCS - metadata: meta, + metadata: meta ? JSON.parse(meta) : undefined, }) ) }) diff --git a/packages/s3-store/index.ts b/packages/s3-store/index.ts index 334ada67..46d45cc3 100644 --- a/packages/s3-store/index.ts +++ b/packages/s3-store/index.ts @@ -84,22 +84,21 @@ export class S3Store extends DataStore { * on the S3 object's `Metadata` field, so that only a `headObject` * is necessary to retrieve the data. */ - private async saveMetadata(file: Upload, upload_id: string) { - log(`[${file.id}] saving metadata`) + private async saveMetadata(upload: Upload, upload_id: string) { + log(`[${upload.id}] saving metadata`) await this.client .putObject({ Bucket: this.bucket, - Key: `${file.id}.info`, + Key: `${upload.id}.info`, Body: '', Metadata: { - file: JSON.stringify(file), + file: JSON.stringify(upload), upload_id, tus_version: TUS_RESUMABLE, }, }) .promise() - log(`[${file.id}] metadata file saved`) - return {file, upload_id} + log(`[${upload.id}] metadata file saved`) } /** @@ -399,7 +398,11 @@ export class S3Store extends DataStore { Key: upload.id, Metadata: {tus_version: TUS_RESUMABLE}, } - const file: Record = {id: upload.id, offset: upload.offset} + const file: Record = { + id: upload.id, + offset: upload.offset, + metadata: upload.metadata, + } if (upload.size) { file.size = upload.size.toString() @@ -486,6 +489,7 @@ export class S3Store extends DataStore { ...this.cache.get(id)?.file, offset: metadata.file.size as number, size: metadata.file.size, + metadata: metadata.file.metadata, }) } diff --git a/packages/server/src/constants.ts b/packages/server/src/constants.ts index 6c3413d1..c0ab7b2a 100644 --- a/packages/server/src/constants.ts +++ b/packages/server/src/constants.ts @@ -53,6 +53,10 @@ export const ERRORS = { status_code: 400, body: 'Upload-Length or Upload-Defer-Length header required\n', }, + INVALID_METADATA: { + status_code: 400, + body: 'Upload-Metadata is invalid. It MUST consist of one or more comma-separated key-value pairs. The key and value MUST be separated by a space. The key MUST NOT contain spaces and commas and MUST NOT be empty. The key SHOULD be ASCII encoded and the value MUST be Base64 encoded. All keys MUST be unique', + }, UNKNOWN_ERROR: { status_code: 500, body: 'Something went wrong with that request\n', diff --git a/packages/server/src/handlers/HeadHandler.ts b/packages/server/src/handlers/HeadHandler.ts index ce34bff1..5f396799 100644 --- a/packages/server/src/handlers/HeadHandler.ts +++ b/packages/server/src/handlers/HeadHandler.ts @@ -49,7 +49,7 @@ export class HeadHandler extends BaseHandler { if (file.metadata !== undefined) { // If the size of the upload is known, the Server MUST include // the Upload-Length header in the response. - res.setHeader('Upload-Metadata', Metadata.stringify(file.metadata)) + res.setHeader('Upload-Metadata', Metadata.stringify(file.metadata) as string) } return res.end() diff --git a/packages/server/src/handlers/PostHandler.ts b/packages/server/src/handlers/PostHandler.ts index 7d815338..afd12cf7 100644 --- a/packages/server/src/handlers/PostHandler.ts +++ b/packages/server/src/handlers/PostHandler.ts @@ -52,7 +52,6 @@ export class PostHandler extends BaseHandler { } let id - try { id = this.options.namingFunction(req) } catch (error) { @@ -60,11 +59,18 @@ export class PostHandler extends BaseHandler { throw ERRORS.FILE_WRITE_ERROR } + let metadata + try { + metadata = Metadata.parse(upload_metadata) + } catch (error) { + throw ERRORS.INVALID_METADATA + } + const upload = new Upload({ id, size: upload_length ? Number.parseInt(upload_length, 10) : undefined, offset: 0, - metadata: Metadata.parse(upload_metadata), + metadata, }) if (this.options.onUploadCreate) { diff --git a/packages/server/src/models/Metadata.ts b/packages/server/src/models/Metadata.ts index 980d53bb..130beed9 100644 --- a/packages/server/src/models/Metadata.ts +++ b/packages/server/src/models/Metadata.ts @@ -32,10 +32,10 @@ export function validateValue(value: string) { } export function parse(str?: string) { - const meta: Record = {} + const meta: Record = {} if (!str) { - return {} + return undefined } for (const pair of str.split(',')) { @@ -46,9 +46,7 @@ export function parse(str?: string) { (tokens.length === 2 && validateKey(key) && validateValue(value))) && !(key in meta) ) { - const decodedValue = value - ? Buffer.from(value, 'base64').toString('utf8') - : undefined + const decodedValue = value ? Buffer.from(value, 'base64').toString('utf8') : null meta[key] = decodedValue } else { throw new Error('Metadata string is not valid') @@ -58,10 +56,10 @@ export function parse(str?: string) { return meta } -export function stringify(metadata: NonNullable) { +export function stringify(metadata: NonNullable): string { return Object.entries(metadata) .map(([key, value]) => { - if (value === undefined) { + if (value === null) { return key } diff --git a/packages/server/src/models/Upload.ts b/packages/server/src/models/Upload.ts index f62f3472..1b4706b6 100644 --- a/packages/server/src/models/Upload.ts +++ b/packages/server/src/models/Upload.ts @@ -2,7 +2,7 @@ type TUpload = { id: string size?: number offset: number - metadata?: Record + metadata?: Record creation_date?: string } diff --git a/packages/server/test/HeadHandler.test.ts b/packages/server/test/HeadHandler.test.ts index 3bda5ce9..e0414207 100644 --- a/packages/server/test/HeadHandler.test.ts +++ b/packages/server/test/HeadHandler.test.ts @@ -65,7 +65,7 @@ describe('HeadHandler', () => { const file = new Upload({ id: '1234', offset: 0, - metadata: {is_confidential: undefined, foo: 'bar'}, + metadata: {is_confidential: null, foo: 'bar'}, }) fake_store.getUpload.resolves(file) await handler.send(req, res) diff --git a/packages/server/test/Metadata.test.ts b/packages/server/test/Metadata.test.ts index b611f953..d10bc8a5 100644 --- a/packages/server/test/Metadata.test.ts +++ b/packages/server/test/Metadata.test.ts @@ -9,8 +9,8 @@ describe('Metadata', () => { 'file/name': 'test.mp4', size: '960244', 'type!': 'video/mp4', - video: undefined, - withWhitespace: undefined, + video: null, + withWhitespace: null, } const decoded = parse(str) assert.deepStrictEqual(decoded, obj) @@ -21,8 +21,8 @@ describe('Metadata', () => { filename: 'test.mp4', size: '960244', type: 'video/mp4', - video: undefined, - withWhitespace: undefined, + video: null, + withWhitespace: null, } const encoded = stringify(obj) @@ -34,11 +34,9 @@ describe('Metadata', () => { assert.strictEqual(stringify({size: '960244'}), 'size OTYwMjQ0') assert.strictEqual(stringify({type: 'video/mp4'}), 'type dmlkZW8vbXA0') // Multiple valid options - assert.notStrictEqual(['video', 'video '].indexOf(stringify({video: undefined})), -1) + assert.notStrictEqual(['video', 'video '].indexOf(stringify({video: null})), -1) assert.notStrictEqual( - ['withWhitespace', 'withWhitespace '].indexOf( - stringify({withWhitespace: undefined}) - ), + ['withWhitespace', 'withWhitespace '].indexOf(stringify({withWhitespace: null})), -1 ) }) @@ -51,20 +49,20 @@ describe('Metadata', () => { assert.deepStrictEqual(parse('type dmlkZW8vbXA0'), { type: 'video/mp4', }) - assert.deepStrictEqual(parse('video'), {video: undefined}) - assert.deepStrictEqual(parse('video '), {video: undefined}) + assert.deepStrictEqual(parse('video'), {video: null}) + assert.deepStrictEqual(parse('video '), {video: null}) assert.deepStrictEqual(parse('withWhitespace'), { - withWhitespace: undefined, + withWhitespace: null, }) assert.deepStrictEqual(parse('withWhitespace '), { - withWhitespace: undefined, + withWhitespace: null, }) }) it('cyclic test', () => { const obj = { filename: 'world_domination_plan.pdf', - is_confidential: undefined, + is_confidential: null, } // Object -> string -> object assert.deepStrictEqual(parse(stringify(obj)), obj) diff --git a/test/e2e.test.ts b/test/e2e.test.ts index 4e444937..b3fb4b76 100644 --- a/test/e2e.test.ts +++ b/test/e2e.test.ts @@ -7,7 +7,7 @@ import rimraf from 'rimraf' import request from 'supertest' import {Storage} from '@google-cloud/storage' -import {Server, TUS_RESUMABLE, MemoryConfigstore} from '@tus/server' +import {Server, TUS_RESUMABLE, MemoryConfigstore, ERRORS} from '@tus/server' import {FileStore} from '@tus/file-store' import {GCSStore} from '@tus/gcs-store' @@ -20,7 +20,7 @@ const BUCKET = 'tus-node-server-ci' const FILES_DIRECTORY = path.resolve(STORE_PATH) const TEST_FILE_SIZE = '960244' const TEST_FILE_PATH = path.resolve('fixtures', 'test.mp4') -const TEST_METADATA = 'some data, for you' +const TEST_METADATA = 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential' const gcs = new Storage({ projectId: PROJECT_ID, keyFilename: KEYFILE, @@ -115,6 +115,19 @@ describe('EndToEnd', () => { }) }) + it('should throw informative error on invalid metadata', (done) => { + agent + .post(STORE_PATH) + .set('Tus-Resumable', TUS_RESUMABLE) + .set('Upload-Length', TEST_FILE_SIZE) + .set('Upload-Metadata', 'no sir') + .expect(ERRORS.INVALID_METADATA.status_code) + .end((_, res) => { + assert.equal(res.text, ERRORS.INVALID_METADATA.body) + done() + }) + }) + it('should create a file with a deferred length', (done) => { agent .post(STORE_PATH) @@ -649,15 +662,34 @@ describe('EndToEnd', () => { describe('HEAD', () => { it('should return the ending offset of the uploaded file', (done) => { - agent - .head(`${STORE_PATH}/${file_id}`) + const read_stream = fs.createReadStream(TEST_FILE_PATH) + const write_stream = agent + .post(STORE_PATH) .set('Tus-Resumable', TUS_RESUMABLE) - .expect(200) - .expect('Upload-Metadata', TEST_METADATA) - .expect('Upload-Offset', `${TEST_FILE_SIZE}`) - .expect('Upload-Length', `${TEST_FILE_SIZE}`) - .expect('Tus-Resumable', TUS_RESUMABLE) - .end(done) + .set('Upload-Length', TEST_FILE_SIZE) + .set('Content-Type', 'application/offset+octet-stream') + write_stream.on('response', (res) => { + assert.equal(res.statusCode, 201) + assert.equal(res.header['tus-resumable'], TUS_RESUMABLE) + assert.equal(res.header['upload-offset'], `${TEST_FILE_SIZE}`) + const id = res.headers.location.split('/').pop() + agent + .head(`${STORE_PATH}/${id}`) + .set('Tus-Resumable', TUS_RESUMABLE) + .expect(200) + .expect('Upload-Offset', `${TEST_FILE_SIZE}`) + .expect('Upload-Length', `${TEST_FILE_SIZE}`) + .expect('Tus-Resumable', TUS_RESUMABLE) + .end(done) + }) + // Using .pipe() broke when upgrading to Superagent 3.0+, + // so now we use data events to read the file to the agent. + read_stream.on('data', (chunk) => { + write_stream.write(chunk) + }) + read_stream.on('end', () => { + write_stream.end(() => {}) + }) }) }) }) diff --git a/test/stores.test.ts b/test/stores.test.ts index 4db1949a..a6c340f4 100644 --- a/test/stores.test.ts +++ b/test/stores.test.ts @@ -25,7 +25,7 @@ export const shouldCreateUploads = function () { id: 'create-test', size: 1000, offset: 0, - metadata: 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential', + metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) const file_defered = new Upload({ id: 'create-test-deferred', @@ -62,7 +62,7 @@ export const shouldCreateUploads = function () { it('should store `upload_metadata` when creating new resource', async function () { await this.datastore.create(file) const upload = await this.datastore.getUpload(file.id) - assert.strictEqual(upload.metadata, file.metadata) + assert.deepStrictEqual(upload.metadata, file.metadata) }) }) } @@ -103,7 +103,7 @@ export const shouldWriteUploads = function () { id: 'write-test', size: this.testFileSize, offset: 0, - metadata: 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential', + metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) await this.datastore.create(file) const readable = fs.createReadStream(this.testFilePath) @@ -116,7 +116,7 @@ export const shouldWriteUploads = function () { id: 'write-test-reject', size: this.testFileSize, offset: 0, - metadata: 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential', + metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) await this.datastore.create(file) const readable = new stream.Readable({ @@ -142,7 +142,7 @@ export const shouldHandleOffset = function () { id: 'offset-test', size: this.testFileSize, offset: 0, - metadata: 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential', + metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) await this.datastore.create(file) @@ -167,7 +167,7 @@ export const shouldDeclareUploadLength = function () { const file = new Upload({ id: 'declare-length-test', offset: 0, - metadata: 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential', + metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) await this.datastore.create(file)