From ef23aa40c9563a492dfb322f0a851a7e339a03ad Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Fri, 26 May 2023 16:24:00 -0700 Subject: [PATCH] Fix broken unit tests and update tag regex Signed-off-by: Jeremy Ho --- app/src/controllers/object.js | 30 +++--- app/src/validators/common.js | 5 +- app/tests/unit/controllers/object.spec.js | 111 +++++++++++++++------- app/tests/unit/services/object.spec.js | 23 ++++- app/tests/unit/validators/common.spec.js | 2 +- 5 files changed, 116 insertions(+), 55 deletions(-) diff --git a/app/src/controllers/object.js b/app/src/controllers/object.js index 35a7ed68..febd7d3f 100644 --- a/app/src/controllers/object.js +++ b/app/src/controllers/object.js @@ -107,29 +107,30 @@ const controller = { // get version from S3 const source = await storageService.headObject({ filePath: objPath, - s3VersionId: sourceS3VersionId, bucketId + s3VersionId: sourceS3VersionId, + bucketId: bucketId }); if (source.ContentLength > MAXCOPYOBJECTLENGTH) { throw new Error('Cannot copy an object larger than 5GB'); } // get existing tags on source object, eg: { 'animal': 'bear', colour': 'black' } - const sourceObject = await storageService.getObjectTagging({ filePath: objPath, s3VersionId: sourceS3VersionId, bucketId }); + const sourceObject = await storageService.getObjectTagging({ filePath: objPath, s3VersionId: sourceS3VersionId, bucketId: bucketId }); const sourceTags = Object.assign({}, ...(sourceObject.TagSet.map(item => ({ [item.Key]: item.Value })))); const metadataToAppend = getMetadata(req.headers); - if (!Object.keys(metadataToAppend).length) { + if (!Object.keys({ ...metadataToAppend}).length) { // TODO: Validation level logic. To be moved. // 422 when no keys present res.status(422).end(); } else { const data = { - bucketId, + bucketId: bucketId, copySource: objPath, filePath: objPath, metadata: { - ...source.Metadata, // Take existing metadata first + ...source.Metadata, // Take existing metadata first ...metadataToAppend, // Append new metadata }, metadataDirective: MetadataDirective.REPLACE, @@ -144,8 +145,8 @@ const controller = { await utils.trxWrapper(async (trx) => { // create or update version in DB (if a non-versioned object) const version = s3Response.VersionId ? - await versionService.copy(sourceS3VersionId, s3Response.VersionId, objId, s3Response.CopyObjectResult.ETag, userId, trx) : - await versionService.update({ ...data, id: objId, etag: s3Response.CopyObjectResult.ETag }, userId, trx); + await versionService.copy(sourceS3VersionId, s3Response.VersionId, objId, s3Response.CopyObjectResult?.ETag, userId, trx) : + await versionService.update({ ...data, id: objId, etag: s3Response.CopyObjectResult?.ETag }, userId, trx); // add metadata for version in DB await metadataService.associateMetadata(version.id, getKeyValue(data.metadata), userId, trx); @@ -372,7 +373,7 @@ const controller = { } // Generate object subset by subtracting/omitting defined keys via filter/inclusion - const keysToRemove = Object.keys(getMetadata(req.headers)); + const keysToRemove = Object.keys({ ...getMetadata(req.headers) }); let metadata = undefined; if (keysToRemove.length) { metadata = Object.fromEntries( @@ -384,12 +385,13 @@ const controller = { // get existing tags on source object const sourceObject = await storageService.getObjectTagging({ filePath: objPath, - s3VersionId: sourceS3VersionId, bucketId + s3VersionId: sourceS3VersionId, + bucketId: bucketId }); const sourceTags = Object.assign({}, ...(sourceObject.TagSet.map(item => ({ [item.Key]: item.Value })))); const data = { - bucketId, + bucketId: bucketId, copySource: objPath, filePath: objPath, metadata: metadata, @@ -408,8 +410,8 @@ const controller = { await utils.trxWrapper(async (trx) => { // create or update version in DB(if a non-versioned object) const version = s3Response.VersionId ? - await versionService.copy(sourceS3VersionId, s3Response.VersionId, objId, s3Response.CopyObjectResult.ETag, userId, trx) : - await versionService.update({ ...data, id: objId, etag: s3Response.CopyObjectResult.ETag }, userId, trx); + await versionService.copy(sourceS3VersionId, s3Response.VersionId, objId, s3Response.CopyObjectResult?.ETag, userId, trx) : + await versionService.update({ ...data, id: objId, etag: s3Response.CopyObjectResult?.ETag }, userId, trx); // add metadata to version in DB await metadataService.associateMetadata(version.id, getKeyValue(data.metadata), userId, trx); @@ -768,9 +770,9 @@ const controller = { await utils.trxWrapper(async (trx) => { // create or update version (if a non-versioned object) const version = s3Response.VersionId ? - await versionService.copy(sourceS3VersionId, s3Response.VersionId, objId, s3Response.CopyObjectResult.ETag, userId, trx) : + await versionService.copy(sourceS3VersionId, s3Response.VersionId, objId, s3Response.CopyObjectResult?.ETag, userId, trx) : - await versionService.update({ ...data, id: objId, etag: s3Response.CopyObjectResult.ETag }, userId, trx); + await versionService.update({ ...data, id: objId, etag: s3Response.CopyObjectResult?.ETag }, userId, trx); // add metadata await metadataService.associateMetadata(version.id, getKeyValue(data.metadata), userId, trx); diff --git a/app/src/validators/common.js b/app/src/validators/common.js index 6d75cb8b..ef4427ff 100644 --- a/app/src/validators/common.js +++ b/app/src/validators/common.js @@ -51,10 +51,9 @@ const type = { tagset: (minKeyCount = 1, minValueStringLength = 0) => Joi.object() .pattern( - /^((?!coms-id).){1,255}$/, // don't allow key 'coms-id' + /^(?!coms-id$).{1,255}$/, // don't allow key 'coms-id' Joi.string().min(minValueStringLength).max(255), - { matches: Joi.array().min(minKeyCount) }, - + { matches: Joi.array().min(minKeyCount) } ) }; diff --git a/app/tests/unit/controllers/object.spec.js b/app/tests/unit/controllers/object.spec.js index 8d37a01d..797b655c 100644 --- a/app/tests/unit/controllers/object.spec.js +++ b/app/tests/unit/controllers/object.spec.js @@ -4,7 +4,7 @@ const { MAXCOPYOBJECTLENGTH, MetadataDirective, TaggingDirective } = require('.. const utils = require('../../../src/db/models/utils'); const controller = require('../../../src/controllers/object'); -const { storageService, objectService, metadataService, versionService, userService } = require('../../../src/services'); +const { storageService, objectService, metadataService, tagService, versionService, userService } = require('../../../src/services'); const mockResponse = () => { const res = {}; @@ -27,10 +27,12 @@ afterEach(() => { describe('addMetadata', () => { // mock service calls + const storageGetObjectTaggingSpy = jest.spyOn(storageService, 'getObjectTagging'); const storageHeadObjectSpy = jest.spyOn(storageService, 'headObject'); const storageCopyObjectSpy = jest.spyOn(storageService, 'copyObject'); const versionCopySpy = jest.spyOn(versionService, 'copy'); const metadataAssociateMetadataSpy = jest.spyOn(metadataService, 'associateMetadata'); + const tagAssociateTagsSpy = jest.spyOn(tagService, 'associateTags'); const trxWrapperSpy = jest.spyOn(utils, 'trxWrapper'); const setHeadersSpy = jest.spyOn(controller, '_processS3Headers'); @@ -39,7 +41,8 @@ describe('addMetadata', () => { // response from S3 const GoodResponse = { ContentLength: 1234, - Metadata: { 'coms-id': 1, foo: 'bar' }, + CopyObjectResultETag: { ETag: '"abcd"' }, + Metadata: { foo: 'bar' }, VersionId: '5678' }; const BadResponse = { @@ -58,12 +61,14 @@ describe('addMetadata', () => { it('responds 422 when no keys are present', async () => { // request object const req = { + currentObject: {}, headers: {}, params: { objectId: 'xyz-789' }, query: {} }; - storageHeadObjectSpy.mockReturnValue(GoodResponse); + storageHeadObjectSpy.mockResolvedValue(GoodResponse); + storageGetObjectTaggingSpy.mockResolvedValue({ TagSet: [] }); await controller.addMetadata(req, res, next); @@ -73,16 +78,20 @@ describe('addMetadata', () => { it('should add the metadata', async () => { // request object const req = { + currentObject: { + path: 'xyz-789' + }, headers: { 'x-amz-meta-baz': 'quz' }, params: { objectId: 'xyz-789' }, query: {} }; - storageHeadObjectSpy.mockReturnValue(GoodResponse); + storageHeadObjectSpy.mockResolvedValue(GoodResponse); + storageGetObjectTaggingSpy.mockResolvedValue({ TagSet: [] }); storageCopyObjectSpy.mockResolvedValue(GoodResponse); trxWrapperSpy.mockImplementation(callback => callback({})); - versionCopySpy.mockReturnValue({ id: '5dad1ec9-d3c0-4b0f-8ead-cb4d9fa98987' }); - metadataAssociateMetadataSpy.mockReturnValue({}); + versionCopySpy.mockResolvedValue({ id: '5dad1ec9-d3c0-4b0f-8ead-cb4d9fa98987' }); + metadataAssociateMetadataSpy.mockResolvedValue({}); setHeadersSpy.mockImplementation(x => x); await controller.addMetadata(req, res, next); @@ -92,17 +101,20 @@ describe('addMetadata', () => { filePath: 'xyz-789', metadata: { foo: 'bar', - baz: 'quz', - // 'coms-id': 1 + baz: 'quz' + }, + tags: { + 'coms-id': 'xyz-789' }, metadataDirective: MetadataDirective.REPLACE, - taggingDirective: TaggingDirective.COPY, + taggingDirective: TaggingDirective.REPLACE, s3VersionId: undefined }); expect(trxWrapperSpy).toHaveBeenCalledTimes(1); expect(versionCopySpy).toHaveBeenCalledTimes(1); expect(metadataAssociateMetadataSpy).toHaveBeenCalledTimes(1); + expect(tagAssociateTagsSpy).toHaveBeenCalledTimes(1); expect(res.status).toHaveBeenCalledWith(204); }); }); @@ -152,6 +164,7 @@ describe('addTags', () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, params: { objectId: 'xyz-789' }, query: { tagset: { foo: 'bar', baz: 'bam' } @@ -165,10 +178,12 @@ describe('addTags', () => { expect(res.status).toHaveBeenCalledWith(204); expect(storagePutObjectTaggingSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', filePath: 'xyz-789', tags: [ { Key: 'foo', Value: 'bar' }, { Key: 'baz', Value: 'bam' }, + { Key: 'coms-id', Value: 'xyz-789' } ], s3VersionId: undefined }); @@ -182,6 +197,7 @@ describe('addTags', () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, params: { objectId: 'xyz-789' }, query: { tagset: { foo: 'bar', baz: 'bam' } @@ -195,11 +211,13 @@ describe('addTags', () => { expect(res.status).toHaveBeenCalledWith(204); expect(storagePutObjectTaggingSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', filePath: 'xyz-789', tags: [ { Key: 'foo', Value: 'bar' }, { Key: 'baz', Value: 'bam' }, { Key: 'abc', Value: '123' }, + { Key: 'coms-id', Value: 'xyz-789' } ], s3VersionId: undefined }); @@ -210,17 +228,21 @@ describe('deleteMetadata', () => { // mock service calls const storageHeadObjectSpy = jest.spyOn(storageService, 'headObject'); const storageCopyObjectSpy = jest.spyOn(storageService, 'copyObject'); + const storageGetObjectTaggingSpy = jest.spyOn(storageService, 'getObjectTagging'); const next = jest.fn(); // response from S3 const GoodResponse = { ContentLength: 1234, - Metadata: { 'coms-id': 1, 'coms-name': 'test', foo: 'bar', baz: 'quz' } + Metadata: { foo: 'bar', baz: 'quz' } }; const BadResponse = { ContentLength: MAXCOPYOBJECTLENGTH + 1 }; + const getObjectTaggingResponse = { + TagSet: [{ Key: 'abc', Value: '123' }] + }; it('should error when Content-Length is greater than 5GB', async () => { // request object @@ -234,6 +256,7 @@ describe('deleteMetadata', () => { it('should delete the requested metadata', async () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, headers: { 'x-amz-meta-foo': 'bar' }, params: { objectId: 'xyz-789' }, query: {} @@ -241,19 +264,24 @@ describe('deleteMetadata', () => { storageHeadObjectSpy.mockReturnValue(GoodResponse); storageCopyObjectSpy.mockReturnValue({}); + storageGetObjectTaggingSpy.mockResolvedValue(getObjectTaggingResponse); await controller.deleteMetadata(req, res, next); expect(res.status).toHaveBeenCalledWith(204); expect(storageCopyObjectSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', copySource: 'xyz-789', filePath: 'xyz-789', metadata: { - baz: 'quz', - 'coms-id': 1, - 'coms-name': 'test', + baz: 'quz' }, metadataDirective: MetadataDirective.REPLACE, + taggingDirective: TaggingDirective.REPLACE, + tags: { + abc: '123', + 'coms-id': 'xyz-789' + }, s3VersionId: undefined }); }); @@ -261,6 +289,7 @@ describe('deleteMetadata', () => { it('should delete all the metadata when none provided', async () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, headers: {}, params: { objectId: 'xyz-789' }, query: {} @@ -268,18 +297,22 @@ describe('deleteMetadata', () => { storageHeadObjectSpy.mockReturnValue(GoodResponse); storageCopyObjectSpy.mockReturnValue({}); + storageGetObjectTaggingSpy.mockResolvedValue(getObjectTaggingResponse); await controller.deleteMetadata(req, res, next); expect(res.status).toHaveBeenCalledWith(204); expect(storageCopyObjectSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', copySource: 'xyz-789', filePath: 'xyz-789', - metadata: { - 'coms-id': 1, - 'coms-name': 'test' - }, + metadata: undefined, metadataDirective: MetadataDirective.REPLACE, + taggingDirective: TaggingDirective.REPLACE, + tags: { + abc: '123', + 'coms-id': 'xyz-789' + }, s3VersionId: undefined }); }); @@ -399,22 +432,25 @@ describe('deleteTags', () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, params: { objectId: 'xyz-789' }, query: { foo: 'bar', baz: 'bam' } }; storageGetObjectTaggingSpy.mockReturnValue(getObjectTaggingResponse); - storageDeleteObjectTaggingSpy.mockReturnValue({}); + storagePutObjectTaggingSpy.mockResolvedValue({}); await controller.deleteTags(req, res, next); expect(res.status).toHaveBeenCalledWith(204); - expect(storageDeleteObjectTaggingSpy).toHaveBeenCalledWith({ + expect(storageDeleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(storagePutObjectTaggingSpy).toHaveBeenCalledTimes(1); + expect(storagePutObjectTaggingSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', filePath: 'xyz-789', - tags: undefined, + tags: [{ Key: 'coms-id', Value: 'xyz-789' }], s3VersionId: undefined }); - expect(storagePutObjectTaggingSpy).toHaveBeenCalledTimes(0); }); it('should delete the requested tags', async () => { @@ -428,6 +464,7 @@ describe('deleteTags', () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, params: { objectId: 'xyz-789' }, query: { tagset: { foo: '', baz: '' } @@ -435,15 +472,17 @@ describe('deleteTags', () => { }; storageGetObjectTaggingSpy.mockReturnValue(getObjectTaggingResponse); - storagePutObjectTaggingSpy.mockReturnValue({}); + storagePutObjectTaggingSpy.mockResolvedValue({}); await controller.deleteTags(req, res, next); expect(res.status).toHaveBeenCalledWith(204); expect(storagePutObjectTaggingSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', filePath: 'xyz-789', tags: [ - { Key: 'abc', Value: '123' } + { Key: 'abc', Value: '123' }, + { Key: 'coms-id', Value: 'xyz-789' } ], s3VersionId: undefined }); @@ -472,6 +511,7 @@ describe('listObjectVersions', () => { describe('replaceMetadata', () => { // mock service calls + const storageGetObjectTaggingSpy = jest.spyOn(storageService, 'getObjectTagging'); const storageHeadObjectSpy = jest.spyOn(storageService, 'headObject'); const storageCopyObjectSpy = jest.spyOn(storageService, 'copyObject'); @@ -498,26 +538,27 @@ describe('replaceMetadata', () => { it('should replace the metadata', async () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, headers: { 'x-amz-meta-baz': 'quz' }, params: { objectId: 'xyz-789' }, query: {} }; storageHeadObjectSpy.mockReturnValue(GoodResponse); + storageGetObjectTaggingSpy.mockResolvedValue({ TagSet: [] }); storageCopyObjectSpy.mockReturnValue({}); await controller.replaceMetadata(req, res, next); expect(res.status).toHaveBeenCalledWith(204); expect(storageCopyObjectSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', copySource: 'xyz-789', filePath: 'xyz-789', - metadata: { - 'coms-id': 1, - 'coms-name': 'test', - baz: 'quz' - }, + metadata: { baz: 'quz' }, metadataDirective: MetadataDirective.REPLACE, + taggingDirective: TaggingDirective.REPLACE, + tags: { 'coms-id': 'xyz-789' }, s3VersionId: undefined }); }); @@ -525,26 +566,27 @@ describe('replaceMetadata', () => { it('should replace replace the name', async () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, headers: { 'x-amz-meta-coms-name': 'newName', 'x-amz-meta-baz': 'quz' }, params: { objectId: 'xyz-789' }, query: {} }; storageHeadObjectSpy.mockReturnValue(GoodResponse); + storageGetObjectTaggingSpy.mockResolvedValue({ TagSet: [] }); storageCopyObjectSpy.mockReturnValue({}); await controller.replaceMetadata(req, res, next); expect(res.status).toHaveBeenCalledWith(204); expect(storageCopyObjectSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', copySource: 'xyz-789', filePath: 'xyz-789', - metadata: { - 'coms-id': 1, - 'coms-name': 'newName', - baz: 'quz' - }, + metadata: { baz: 'quz', 'coms-name': 'newName' }, metadataDirective: MetadataDirective.REPLACE, + taggingDirective: TaggingDirective.REPLACE, + tags: { 'coms-id': 'xyz-789' }, s3VersionId: undefined }); }); @@ -593,6 +635,7 @@ describe('replaceTags', () => { // request object const req = { + currentObject: { bucketId: 'bid-123', path: 'xyz-789' }, params: { objectId: 'xyz-789' }, query: { tagset: { foo: 'bar', baz: 'bam' } @@ -606,10 +649,12 @@ describe('replaceTags', () => { expect(res.status).toHaveBeenCalledWith(204); expect(storagePutObjectTaggingSpy).toHaveBeenCalledWith({ + bucketId: 'bid-123', filePath: 'xyz-789', tags: [ { Key: 'foo', Value: 'bar' }, { Key: 'baz', Value: 'bam' }, + { Key: 'coms-id', Value: 'xyz-789' } ], s3VersionId: undefined }); diff --git a/app/tests/unit/services/object.spec.js b/app/tests/unit/services/object.spec.js index d492df17..32d1db7a 100644 --- a/app/tests/unit/services/object.spec.js +++ b/app/tests/unit/services/object.spec.js @@ -105,18 +105,33 @@ describe('getBucketKey', () => { describe('searchObjects', () => { it('Search and filter for specific object records', () => { ObjectModel.then.mockImplementation(() => { }); - - service.searchObjects({ + const params = { bucketId: BUCKET_ID, bucketName: 'bucketName', active: 'true', key: 'key', userId: SYSTEM_USER - }); + }; + + service.searchObjects(params); expect(ObjectModel.query).toHaveBeenCalledTimes(1); expect(ObjectModel.allowGraph).toHaveBeenCalledTimes(1); - expect(ObjectModel.modify).toHaveBeenCalledTimes(10); + expect(ObjectModel.modify).toHaveBeenCalledTimes(11); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(1, 'filterIds', params.id); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(2, 'filterBucketIds', params.bucketId); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(3, 'filterName', params.name); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(4, 'filterPath', params.path); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(5, 'filterPublic', params.public); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(6, 'filterActive', params.active); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(7, 'filterMimeType', params.mimeType); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(8, 'filterDeleteMarker', params.deleteMarker); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(9, 'filterLatest', params.latest); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(10, 'filterMetadataTag', { + metadata: params.metadata, + tag: params.tag + }); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(11, 'hasPermission', params.userId, 'READ'); expect(ObjectModel.then).toHaveBeenCalledTimes(1); }); }); diff --git a/app/tests/unit/validators/common.spec.js b/app/tests/unit/validators/common.spec.js index 22d866e2..1ec1c15a 100644 --- a/app/tests/unit/validators/common.spec.js +++ b/app/tests/unit/validators/common.spec.js @@ -182,7 +182,7 @@ describe('type', () => { it('enforces general tagset pattern', () => { expect(model.patterns).toEqual(expect.arrayContaining([ expect.objectContaining({ - regex: '/^.{1,255}$/', + regex: '/^(?!coms-id$).{1,255}$/', rule: expect.objectContaining({ type: 'string', rules: expect.arrayContaining([