Merge pull request #241 from bcgov/bugfix/searchObject

Bugfix synchronization collisions with database when the object entry already exists

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/metadata.js

@@ -182,6 +182,7 @@ const service = {
   fetchMetadataForObject: (params) => {
     return ObjectModel.query()
       .select('object.id AS objectId', 'object.bucketId as bucketId')
+      .allowGraph('[bucketPermission, objectPermission, version.metadata]')
       .withGraphJoined('version.metadata')
       // get latest version that isn't a delete marker by default
       .modifyGraph('version', builder => {

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/src/services/tag.js

@@ -233,6 +233,7 @@ const service = {
   fetchTagsForObject: (params) => {
     return ObjectModel.query()
       .select('object.id AS objectId', 'object.bucketId as bucketId')
+      .allowGraph('[bucketPermission, objectPermission, version.tag]')
       .withGraphJoined('version.tag')
       // get latest version that isn't a delete marker by default
       .modifyGraph('version', builder => {

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

charts/coms/Chart.yaml

@@ -3,7 +3,7 @@ name: common-object-management-service
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
 # Versions are expected to follow Semantic Versioning (https://semver.org/)
-version: 0.0.20
+version: 0.0.21
 kubeVersion: ">= 1.13.0"
 description: A microservice for managing access control to S3 Objects
 # A chart can be either an 'application' or a 'library' chart.

charts/coms/README.md

@@ -1,6 +1,6 @@
 # common-object-management-service
 
-![Version: 0.0.20](https://img.shields.io/badge/Version-0.0.20-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 0.7.0](https://img.shields.io/badge/AppVersion-0.7.0-informational?style=flat-square)
+![Version: 0.0.21](https://img.shields.io/badge/Version-0.0.21-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 0.7.0](https://img.shields.io/badge/AppVersion-0.7.0-informational?style=flat-square)
 
 A microservice for managing access control to S3 Objects
 
@@ -78,4 +78,4 @@ Kubernetes: `>= 1.13.0`
 | serviceAccount.name | string | `nil` | The name of the service account to use. If not set and create is true, a name is generated using the fullname template |
 
 ----------------------------------------------
-Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0)
+Autogenerated from chart metadata using [helm-docs v1.11.3](https://github.com/norwoodj/helm-docs/releases/v1.11.3)

charts/coms/templates/networkpolicy.yaml

@@ -12,8 +12,6 @@ spec:
         - namespaceSelector:
             matchLabels:
               network.openshift.io/policy-group: ingress
-        - podSelector:
-            matchLabels: {{ include "coms.selectorLabels" . | nindent 14 }}
       ports:
         - port: {{ default "8080" .Values.config.configMap.SERVER_PORT | atoi }}
           protocol: TCP