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 <jujaga@gmail.com>

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 }));

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<object[]>} 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) {

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

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);
 

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);
   });
 });

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);
 

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', () => {