re-factor version delete process

app/src/controllers/object.js

@@ -463,11 +463,11 @@ const controller = {
       // if request is to delete a version
       if (data.s3VersionId) {
         // delete version in DB
-        await versionService.delete(objId, s3Response.VersionId, userId);
+        await versionService.delete(objId, s3Response.VersionId);
         // prune tags amd metadata
         await metadataService.pruneOrphanedMetadata();
         await tagService.pruneOrphanedTags();
-        // if other versions in DB, delete object record
+        // if no other versions in DB, delete object record
         const remainingVersions = await versionService.list(objId);
         if (remainingVersions.length === 0) await objectService.delete(objId);
       } else { // else deleting the object

app/src/services/sync.js

@@ -207,12 +207,25 @@ const service = {
         .map(dm => ({ DeleteMarker: true, ...dm }))
         .concat(s3VersionsRaw.Versions);
 
-      // Drop versions in COMS that are no longer in S3
-      await Promise.all(comsVersions.map(async cv => {
-        if (cv.s3VersionId && !s3Versions.some(s3v => (s3v.VersionId === cv.s3VersionId))) {
-          await versionService.delete(comsObject.id, (cv.s3VersionId ?? null), userId, trx);
-        }
-      }));
+      // delete versions from COMS that are not in S3
+      // if array length of coms versions is longer than length of s3 versions
+      if(comsVersions.length > s3Versions.length){
+        // only keep first of duplicates
+        const uniqueComsVersions = comsVersions.filter((obj, index) =>
+          comsVersions.findIndex((item) => item.s3VersionId === obj.s3VersionId) === index
+        );
+
+        await Version.query(trx)
+          .delete()
+          .where('objectId', comsObject.id)
+          .whereNotNull('s3VersionId',)
+          .where(q =>
+            q.whereNotIn('id', uniqueComsVersions.map(v => v.id))
+              .orWhereNotIn('s3VersionId', s3Versions.map(v => v.VersionId)
+              ))
+          .returning('*')
+          .throwIfNotFound();
+      }
 
       // Add and Update versions in COMS
       const response = await Promise.all(s3Versions.map(async s3Version => {
@@ -252,18 +265,14 @@ const service = {
           // Version record not modified
           else return { version: existingVersion };
         }
-
         // S3 Object is in versioned bucket (ie: if VersionId is not 'null')
         else {
           const comsVersion = comsVersions.find(cv => cv.s3VersionId === s3Version.VersionId);
-
           if (comsVersion) { // Version is in COMS
-            if (s3Version.IsLatest) { // Patch isLatest flags if changed
-              const updated = await versionService.updateIsLatest(comsVersion.id, trx);
-              return { modified: true, version: updated };
-            } else { // Version record not modified
-              return { version: comsVersion };
-            }
+            // set isLatest in COMS db
+            return s3Version.IsLatest ?
+              { modified: true, version: await versionService.updateIsLatest(comsObject.id, trx) } :
+              { version: comsVersion };
           } else { // Version is not in COMS
             const mimeType = s3Version.DeleteMarker
               ? undefined // Will default to 'application/octet-stream'

app/src/services/version.js

@@ -1,6 +1,9 @@
 const { v4: uuidv4, NIL: SYSTEM_USER } = require('uuid');
 const { Version } = require('../db/models');
 
+const objectService = require('./object');
+const storageService = require('./storage');
+
 /**
  * The Version DB Service
  */
@@ -105,14 +108,16 @@ const service = {
   /**
    * @function delete
    * Delete a version record of an object
+   * Note: For consistency, it is recommended to sync isLatest with S3 by calling
+   * `service.updateIsLatest()` after invoking this function
    * @param {string} objId The object uuid
    * @param {string} s3VersionId The version ID or null if deleting an object
    * @param {string} [userId=undefined] An optional uuid of a user
    * @param {object} [etrx=undefined] An optional Objection Transaction object
    * @returns {Promise<integer>} The number of remaining versions in db after the delete
    * @throws The error encountered upon db transaction failure
    */
-  delete: async (objId, s3VersionId, userId = undefined, etrx = undefined) => {
+  delete: async (objId, s3VersionId, etrx = undefined) => {
     let trx;
     try {
       trx = etrx ? etrx : await Version.startTransaction();
@@ -125,9 +130,7 @@ const service = {
         .returning('*')
         .throwIfNotFound();
 
-      // sync other versions with isLatest
-      const { syncVersions } = require('./sync');
-      await syncVersions(objId, userId, trx);
+      await service.updateIsLatest(objId, trx);
 
       if (!etrx) await trx.commit();
       return Promise.resolve(response);
@@ -277,30 +280,36 @@ const service = {
 
   /**
    * @function updateIsLatest
-   * Set specified version as latest in COMS db
-   * and ensures only one version has isLatest: true
-   * @param {string} versionId COMS version uuid
+   * Set version as latest in COMS db.
+   * Determines latest by checking S3 and ensures only one version has isLatest: true
+   * @param {string} objectId COMS object uuid
    * @param {object} [etrx=undefined] An optional Objection Transaction object
    * @returns {object} Version model of provided version in db
    */
-  updateIsLatest: async (versionId, etrx = undefined) => {
+  updateIsLatest: async (objectId, etrx = undefined) => {
     // TODO: consider having accepting a `userId` argument for version.updatedBy when a version becomes 'latest'
     let trx;
     try {
       trx = etrx ? etrx : await Version.startTransaction();
 
+      // get VersionId of latest version in S3
+      const object = await objectService.read(objectId, trx);
+      const s3Versions = await storageService.listAllObjectVersions({ filePath: object.path, bucketId: object.bucketId });
+      const latestS3VersionId = s3Versions.DeleteMarkers.concat(s3Versions.Versions).filter((v) => v.IsLatest)[0].VersionId;
+
+      // get same version from COMS db
       const current = await Version.query(trx)
-        .findById(versionId)
+        .first()
+        .where({ objectId: objectId, s3VersionId: latestS3VersionId })
         .throwIfNotFound();
-
       let updated;
-      // if the version is not already marked as isLatest
+      // update as latest if not already and fetch
       if (!current.isLatest) {
-        // update this version as latest and fetch
         updated = await Version.query(trx)
-          .updateAndFetchById(versionId, { isLatest: true });
+          .updateAndFetchById(current.id, { isLatest: true });
       }
-      await service.removeDuplicateLatest(versionId, current.objectId, trx);
+      // set other versions in COMS db to isLatest=false
+      await service.removeDuplicateLatest(current.id, current.objectId, trx);
 
       if (!etrx) await trx.commit();
       return Promise.resolve(updated ?? current);

app/tests/unit/controllers/object.spec.js

@@ -372,7 +372,7 @@ describe('deleteObject', () => {
 
     await controller.deleteObject(req, res, next);
     expect(versionDeleteSpy).toHaveBeenCalledTimes(1);
-    expect(versionDeleteSpy).toHaveBeenCalledWith('xyz-789', '123', '456');
+    expect(versionDeleteSpy).toHaveBeenCalledWith('xyz-789', '123');
   });
 
   it('should delete object if object has no other remaining versions', async () => {

app/tests/unit/services/sync.spec.js

@@ -11,8 +11,15 @@ jest.mock('../../../src/db/models/tables/objectModel', () => ({
 
 const versionTrx = trxBuilder();
 jest.mock('../../../src/db/models/tables/version', () => ({
+  delete: jest.fn(),
+  query: jest.fn(),
   startTransaction: jest.fn(),
-  then: jest.fn()
+  then: jest.fn(),
+  returning: jest.fn(),
+  throwIfNotFound: jest.fn(),
+  where: jest.fn(),
+  whereNotNull: jest.fn(),
+  whereNotIn: jest.fn(),
 }));
 
 const {
@@ -430,7 +437,6 @@ describe('syncObject', () => {
 
 describe('syncVersions', () => {
   const createSpy = jest.spyOn(versionService, 'create');
-  const deleteSpy = jest.spyOn(versionService, 'delete');
   const listSpy = jest.spyOn(versionService, 'list');
   const listAllObjectVersionsSpy = jest.spyOn(storageService, 'listAllObjectVersions');
   const readSpy = jest.spyOn(objectService, 'read');
@@ -445,7 +451,6 @@ describe('syncVersions', () => {
 
   beforeEach(() => {
     createSpy.mockReset();
-    deleteSpy.mockReset();
     headObjectSpy.mockReset();
     listSpy.mockReset();
     listAllObjectVersionsSpy.mockReset();
@@ -456,7 +461,6 @@ describe('syncVersions', () => {
 
   afterAll(() => {
     createSpy.mockRestore();
-    deleteSpy.mockRestore();
     listSpy.mockRestore();
     listAllObjectVersionsSpy.mockRestore();
     readSpy.mockRestore();
@@ -484,7 +488,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(2);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -522,7 +526,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(2);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -564,7 +568,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(1);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -604,7 +608,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(0);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -644,7 +648,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(0);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -683,7 +687,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(0);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -707,7 +711,12 @@ describe('syncVersions', () => {
     it('should drop COMS versions that are not in S3', async () => {
       createSpy.mockResolvedValue({});
       headObjectSpy.mockResolvedValue({});
-      listSpy.mockResolvedValue([{ s3VersionId: validUuidv4 }]);
+      // extra versions in coms to delete
+      listSpy.mockResolvedValue([
+        { s3VersionId: validUuidv4 },
+        { s3VersionId: validUuidv4 },
+        { s3VersionId: validUuidv4 }
+      ]);
       listAllObjectVersionsSpy.mockResolvedValue({ DeleteMarkers: [{}], Versions: [{}] });
 
       const result = await service.syncVersions(comsObject);
@@ -722,8 +731,8 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(2);
-      expect(deleteSpy).toHaveBeenCalledTimes(1);
-      expect(deleteSpy).toHaveBeenCalledWith(comsObject.id, validUuidv4, expect.any(String), expect.any(Object));
+      expect(Version.delete).toHaveBeenCalledTimes(1);
+
       expect(headObjectSpy).toHaveBeenCalledTimes(1);
       expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({
         filePath: comsObject.path,
@@ -764,7 +773,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(1);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(0);
       expect(listSpy).toHaveBeenCalledTimes(1);
       expect(listSpy).toHaveBeenCalledWith(validUuidv4, expect.any(Object));
@@ -800,7 +809,7 @@ describe('syncVersions', () => {
 
       expect(Version.startTransaction).toHaveBeenCalledTimes(1);
       expect(createSpy).toHaveBeenCalledTimes(1);
-      expect(deleteSpy).toHaveBeenCalledTimes(0);
+      expect(Version.delete).toHaveBeenCalledTimes(0);
       expect(headObjectSpy).toHaveBeenCalledTimes(0);
       expect(listSpy).toHaveBeenCalledTimes(1);
       expect(listSpy).toHaveBeenCalledWith(validUuidv4, expect.any(Object));