From 980a29e23e9ad32a3dfcd8eaa540c3c22a65ae63 Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Fri, 12 Jan 2024 12:31:37 -0800 Subject: [PATCH] Bugfix searchObject utilization in affected methods The searchObjects service call had a structural change that was not propagating correctly to the synchronization methods. This commit fixes that by ensuring that the values are extracted out correctly from the new structure. Also improved test branch coverage and JSDocs. Signed-off-by: Jeremy Ho --- app/src/controllers/sync.js | 2 +- app/src/services/object.js | 19 ++++--- app/src/services/sync.js | 2 +- app/tests/unit/controllers/sync.spec.js | 2 +- app/tests/unit/services/object.spec.js | 71 ++++++++++++++++++++++-- app/tests/unit/services/sync.spec.js | 12 ++-- app/tests/unit/validators/bucket.spec.js | 2 +- 7 files changed, 88 insertions(+), 22 deletions(-) diff --git a/app/src/controllers/sync.js b/app/src/controllers/sync.js index 6ce4d7ca..7e903c2b 100644 --- a/app/src/controllers/sync.js +++ b/app/src/controllers/sync.js @@ -29,7 +29,7 @@ const controller = { // Aggregate and dedupe all file paths to consider const jobs = [...new Set([ - ...dbResponse.map(object => object.path), + ...dbResponse.data.map(object => object.path), ...s3Response.DeleteMarkers.map(object => object.Key), ...s3Response.Versions.map(object => object.Key) ])].map(path => ({ path: path, bucketId: bucketId })); diff --git a/app/src/services/object.js b/app/src/services/object.js index a32b6c5a..e7398ff0 100644 --- a/app/src/services/object.js +++ b/app/src/services/object.js @@ -112,18 +112,23 @@ const service = { * @param {string} [params.name] Optional metadata name string to match on * @param {object} [params.metadata] Optional object of metadata key/value pairs * @param {object} [params.tag] Optional object of tag key/value pairs + * @param {object} [params.limit] Optional number of records to limit by + * @param {object} [params.order] Optional column attribute to order by + * @param {object} [params.page] Optional page set to return + * @param {object} [params.sort] Optional `asc` or `desc` sort ordering * @param {object} [etrx=undefined] An optional Objection Transaction object - * @returns {Promise} The result of running the find operation + * @returns {Promise<{total: number, data: object[]}>} The find operation result containing the `total` length of + * the search query, and the relevant array of COMS objects. */ searchObjects: async (params, etrx = undefined) => { let trx; - let response = []; + let response = {}; try { trx = etrx ? etrx : await ObjectModel.startTransaction(); // GroupBy() seems to be working faster with ObjectionJS Graphs // when comparing with distinct() response.data = await ObjectModel.query(trx) - .allowGraph('[objectPermission, version, bucketPermission]') + .allowGraph('[bucketPermission, objectPermission, version]') .groupBy('object.id') .modify('filterIds', params.id) .modify('filterBucketIds', params.bucketId) @@ -142,16 +147,16 @@ const service = { .modify('pagination', params.page, params.limit) .modify('sortOrder', params.sort, params.order) .then(result => { - let resultObject = []; + let results = []; if (Object.hasOwn(result, 'results')) { - resultObject = result.results; + results = result.results; response.total = result.total; } else { - resultObject = result; + results = result; response.total = result.length; } return Promise.all( - resultObject.map(row => { + results.map(row => { // eslint-disable-next-line no-unused-vars const { objectPermission, bucketPermission, version, ...object } = row; if (params.permissions) { diff --git a/app/src/services/sync.js b/app/src/services/sync.js index 63d708d2..63012c38 100644 --- a/app/src/services/sync.js +++ b/app/src/services/sync.js @@ -143,7 +143,7 @@ const service = { .catch((e) => { // return boolean true if object is soft-deleted in S3 return !!e.$response.headers['x-amz-delete-marker']; }) - ]).then(settled => settled.map(promise => Array.isArray(promise.value) ? promise.value[0] : promise.value)); + ]).then(([comsPromise, s3Promise]) => ([comsPromise.value?.data[0], s3Promise.value])); if (s3Object) { // Determine S3 Public status diff --git a/app/tests/unit/controllers/sync.spec.js b/app/tests/unit/controllers/sync.spec.js index edbaa08e..29bcbb5c 100644 --- a/app/tests/unit/controllers/sync.spec.js +++ b/app/tests/unit/controllers/sync.spec.js @@ -37,7 +37,7 @@ describe('syncBucket', () => { DeleteMarkers: [{ Key: path }], Versions: [{ Key: path }] }); - searchObjectsSpy.mockResolvedValue([{ path: path }]); + searchObjectsSpy.mockResolvedValue({ total: 1, data: [{ path: path }] }); await controller.syncBucket(req, res, next); diff --git a/app/tests/unit/services/object.spec.js b/app/tests/unit/services/object.spec.js index 0c760788..3aaa604a 100644 --- a/app/tests/unit/services/object.spec.js +++ b/app/tests/unit/services/object.spec.js @@ -104,22 +104,84 @@ describe('getBucketKey', () => { }); describe('searchObjects', () => { - it('Search and filter for specific object records', async () => { - ObjectModel.then.mockImplementation(() => { }); + // TODO: Add in other untested multiplicity cases + it('Search and filter for specific object records with permissions and without pagination', async () => { const params = { bucketId: BUCKET_ID, bucketName: 'bucketName', active: 'true', key: 'key', - userId: SYSTEM_USER + userId: 'ae8a58f6-62bc-4dd1-acef-79f123609d48', + permissions: true }; - await service.searchObjects(params); + // We only care about mocking the final 13th chained modify result + for (let i = 0; i < 12; i++) ObjectModel.modify.mockReturnValueOnce(ObjectModel); + ObjectModel.modify.mockResolvedValueOnce([{ + objectPermission: [{ permCode: 'READ' }], ...params + }]); + + const result = await service.searchObjects(params); + + expect(result).toBeTruthy(); + expect(result).toHaveProperty('total'); + expect(result).toHaveProperty('data'); + expect(Array.isArray(result.data)).toBeTruthy(); + expect(result.data[0]).toHaveProperty('permissions'); + expect(result.data[0].permissions).toEqual(expect.arrayContaining(['READ'])); + + expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); + expect(ObjectModel.query).toHaveBeenCalledTimes(1); + expect(ObjectModel.query).toHaveBeenCalledWith(expect.anything()); + expect(ObjectModel.allowGraph).toHaveBeenCalledTimes(1); + expect(ObjectModel.allowGraph).toHaveBeenCalledWith('[bucketPermission, objectPermission, version]'); + expect(ObjectModel.groupBy).toHaveBeenCalledTimes(1); + expect(ObjectModel.modify).toHaveBeenCalledTimes(13); + 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.modify).toHaveBeenNthCalledWith(12, 'pagination', params.page, params.limit); + expect(ObjectModel.modify).toHaveBeenNthCalledWith(13, 'sortOrder', params.sort, params.order); + expect(objectModelTrx.commit).toHaveBeenCalledTimes(1); + }); + + it('Search and filter for specific object records with pagination', async () => { + // We only care about mocking the final 13th chained modify result + for (let i = 0; i < 12; i++) ObjectModel.modify.mockReturnValueOnce(ObjectModel); + ObjectModel.modify.mockResolvedValueOnce({ total: 0, results: [] }); + + const params = { + bucketId: BUCKET_ID, + bucketName: 'bucketName', + active: 'true', + key: 'key', + userId: SYSTEM_USER, + permissions: false + }; + + const result = await service.searchObjects(params); + + expect(result).toBeTruthy(); + expect(result).toHaveProperty('total'); + expect(result).toHaveProperty('data'); + expect(Array.isArray(result.data)).toBeTruthy(); expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(ObjectModel.query).toHaveBeenCalledTimes(1); expect(ObjectModel.query).toHaveBeenCalledWith(expect.anything()); expect(ObjectModel.allowGraph).toHaveBeenCalledTimes(1); + expect(ObjectModel.allowGraph).toHaveBeenCalledWith('[bucketPermission, objectPermission, version]'); expect(ObjectModel.groupBy).toHaveBeenCalledTimes(1); expect(ObjectModel.modify).toHaveBeenCalledTimes(13); expect(ObjectModel.modify).toHaveBeenNthCalledWith(1, 'filterIds', params.id); @@ -138,7 +200,6 @@ describe('searchObjects', () => { expect(ObjectModel.modify).toHaveBeenNthCalledWith(11, 'hasPermission', params.userId, 'READ'); expect(ObjectModel.modify).toHaveBeenNthCalledWith(12, 'pagination', params.page, params.limit); expect(ObjectModel.modify).toHaveBeenNthCalledWith(13, 'sortOrder', params.sort, params.order); - expect(ObjectModel.then).toHaveBeenCalledTimes(1); expect(objectModelTrx.commit).toHaveBeenCalledTimes(1); }); }); diff --git a/app/tests/unit/services/sync.spec.js b/app/tests/unit/services/sync.spec.js index b3aea5c5..80b62d5b 100644 --- a/app/tests/unit/services/sync.spec.js +++ b/app/tests/unit/services/sync.spec.js @@ -377,7 +377,7 @@ describe('syncObject', () => { it('should return object when already synced', async () => { const comsObject = { id: validUuidv4, path: path, public: true }; headObjectSpy.mockResolvedValue({}); - searchObjectsSpy.mockResolvedValue([comsObject]); + searchObjectsSpy.mockResolvedValue({ total: 1, data: [comsObject] }); getObjectPublicSpy.mockResolvedValue(true); const result = await service.syncObject(path, bucketId); @@ -411,7 +411,7 @@ describe('syncObject', () => { it('should return object when already synced but public mismatch', async () => { const comsObject = { id: validUuidv4, path: path, public: true }; headObjectSpy.mockResolvedValue({}); - searchObjectsSpy.mockResolvedValue([comsObject]); + searchObjectsSpy.mockResolvedValue({ total: 1, data: [comsObject] }); getObjectPublicSpy.mockResolvedValue(false); updateSpy.mockResolvedValue(comsObject); @@ -449,7 +449,7 @@ describe('syncObject', () => { it('should return object when already synced but S3 ACL errors out', async () => { const comsObject = { id: validUuidv4, path: path, public: true }; headObjectSpy.mockResolvedValue({}); - searchObjectsSpy.mockResolvedValue([comsObject]); + searchObjectsSpy.mockResolvedValue({ total: 1, data: [comsObject] }); getObjectPublicSpy.mockImplementation(() => { throw new Error(); }); const result = await service.syncObject(path, bucketId); @@ -485,7 +485,7 @@ describe('syncObject', () => { _deriveObjectIdSpy.mockResolvedValue(validUuidv4); createSpy.mockResolvedValue(comsObject); headObjectSpy.mockResolvedValue({}); - searchObjectsSpy.mockResolvedValue(undefined); + searchObjectsSpy.mockResolvedValue({ total: 0, data: [] }); getObjectPublicSpy.mockResolvedValue(true); const result = await service.syncObject(path, bucketId); @@ -529,7 +529,7 @@ describe('syncObject', () => { _deriveObjectIdSpy.mockResolvedValue(validUuidv4); createSpy.mockResolvedValue(comsObject); headObjectSpy.mockResolvedValue({}); - searchObjectsSpy.mockResolvedValue(undefined); + searchObjectsSpy.mockResolvedValue({ total: 0, data: [] }); getObjectPublicSpy.mockImplementation(() => { throw new Error(); }); const result = await service.syncObject(path, bucketId); @@ -574,7 +574,7 @@ describe('syncObject', () => { headObjectSpy.mockRejectedValue({}); pruneOrphanedMetadataSpy.mockResolvedValue(0); pruneOrphanedTagsSpy.mockResolvedValue(0); - searchObjectsSpy.mockResolvedValue([comsObject]); + searchObjectsSpy.mockResolvedValue({ total: 1, data: [comsObject] }); const result = await service.syncObject(path, bucketId); diff --git a/app/tests/unit/validators/bucket.spec.js b/app/tests/unit/validators/bucket.spec.js index 33c5eb6c..c446c6ee 100644 --- a/app/tests/unit/validators/bucket.spec.js +++ b/app/tests/unit/validators/bucket.spec.js @@ -147,7 +147,7 @@ describe('createBucketChild', () => { }); }); - describe.only('key', () => { + describe('key', () => { const subKey = body.keys.subKey; it('is a string', () => {