From c524127efcf4706234a4332a8d50ea485e6e9b0d Mon Sep 17 00:00:00 2001 From: Norris Ng <103449568+norrisng-bc@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:41:17 -0800 Subject: [PATCH] syncObject: delete 'coms-id' tag on new file if id already in use --- app/src/services/sync.js | 38 ++++++++++++- app/tests/unit/services/sync.spec.js | 83 ++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/app/src/services/sync.js b/app/src/services/sync.js index 1477e3d2..8cb6db1e 100644 --- a/app/src/services/sync.js +++ b/app/src/services/sync.js @@ -175,7 +175,43 @@ const service = { // Case: not in COMS - insert new COMS object else { - const objId = await service._deriveObjectId(s3Object, path, bucketId); + let objId = await service._deriveObjectId(s3Object, path, bucketId); + let existingComsId; + try { + existingComsId = await objectService.read(objId); + } catch { + // Derived object id not currently in use by COMS + } + + // Object id in 'coms-id' S3 tag already in use in COMS: + // Delete the 'coms-id' tag on the new object, + // so that we don't attempt to re-use an existing COMS object id. + if (existingComsId) { + const sourceObject = await storageService.getObjectTagging({ + filePath: path, + bucketId: bucketId + }); + + // Workaround for deleting just the 'coms-id' tag (S3 can only delete ALL tags on an object): + // Retrieve all tags, delete all tags on object, then re-add all tags except 'coms-id'. + const objectTags = sourceObject.TagSet.filter( + x => !sourceObject.TagSet.includes(x.Key) && x.Key != 'coms-id' + ); + await storageService.deleteObjectTagging({ + filePath: path, + bucketId: bucketId + }); + const data = { + bucketId, + filePath: path, + tags: objectTags + }; + await storageService.putObjectTagging(data); + + // _deriveObjectId() doesn't care if it conflicts with existing coms-id's, + // so generate a new one after deleting the S3 tag + objId = uuidv4(); + } response = await objectService.create({ id: objId, diff --git a/app/tests/unit/services/sync.spec.js b/app/tests/unit/services/sync.spec.js index c0d24a64..d038ef5f 100644 --- a/app/tests/unit/services/sync.spec.js +++ b/app/tests/unit/services/sync.spec.js @@ -348,11 +348,14 @@ describe('syncObject', () => { const _deriveObjectIdSpy = jest.spyOn(service, '_deriveObjectId'); const createSpy = jest.spyOn(objectService, 'create'); const deleteSpy = jest.spyOn(objectService, 'delete'); + const deleteObjectTaggingSpy = jest.spyOn(storageService, 'deleteObjectTagging'); const getObjectPublicSpy = jest.spyOn(storageService, 'getObjectPublic'); const pruneOrphanedMetadataSpy = jest.spyOn(metadataService, 'pruneOrphanedMetadata'); const pruneOrphanedTagsSpy = jest.spyOn(tagService, 'pruneOrphanedTags'); + const readSpy = jest.spyOn(objectService, 'read'); const searchObjectsSpy = jest.spyOn(objectService, 'searchObjects'); const updateSpy = jest.spyOn(objectService, 'update'); + // const uuidv4Spy = jest.spyOn(require('uuid'), uuidv4); beforeEach(() => { _deriveObjectIdSpy.mockReset(); @@ -391,6 +394,10 @@ describe('syncObject', () => { expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(0); + expect(readSpy).toHaveBeenCalledTimes(0); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(0); expect(deleteSpy).toHaveBeenCalledTimes(0); expect(getObjectPublicSpy).toHaveBeenCalledTimes(1); @@ -429,6 +436,10 @@ describe('syncObject', () => { expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(0); + expect(readSpy).toHaveBeenCalledTimes(0); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(0); expect(deleteSpy).toHaveBeenCalledTimes(0); expect(getObjectPublicSpy).toHaveBeenCalledTimes(1); @@ -467,6 +478,10 @@ describe('syncObject', () => { expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(0); + expect(readSpy).toHaveBeenCalledTimes(0); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(0); expect(deleteSpy).toHaveBeenCalledTimes(0); expect(getObjectPublicSpy).toHaveBeenCalledTimes(1); @@ -494,6 +509,7 @@ describe('syncObject', () => { headObjectSpy.mockResolvedValue({}); searchObjectsSpy.mockResolvedValue({ total: 0, data: [] }); getObjectPublicSpy.mockResolvedValue(true); + readSpy.mockImplementation(() => { throw new Error(); }); const result = await service.syncObject(path, bucketId); @@ -504,6 +520,10 @@ describe('syncObject', () => { expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledWith(expect.any(Object), path, bucketId); + expect(readSpy).toHaveBeenCalledTimes(1); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(1); expect(createSpy).toHaveBeenCalledWith(expect.objectContaining({ id: validUuidv4, @@ -538,6 +558,7 @@ describe('syncObject', () => { headObjectSpy.mockResolvedValue({}); searchObjectsSpy.mockResolvedValue({ total: 0, data: [] }); getObjectPublicSpy.mockImplementation(() => { throw new Error(); }); + readSpy.mockImplementation(() => { throw new Error(); }); const result = await service.syncObject(path, bucketId); @@ -548,6 +569,10 @@ describe('syncObject', () => { expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledWith(expect.any(Object), path, bucketId); + expect(readSpy).toHaveBeenCalledTimes(1); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(1); expect(createSpy).toHaveBeenCalledWith(expect.objectContaining({ id: validUuidv4, @@ -575,6 +600,60 @@ describe('syncObject', () => { expect(objectModelTrx.commit).toHaveBeenCalledTimes(1); }); + it('should insert new object when not in COMS when it has a conflicting coms-id S3 tag', async () => { + const comsObject = {}; + _deriveObjectIdSpy.mockResolvedValue(validUuidv4); + createSpy.mockResolvedValue(comsObject); + headObjectSpy.mockResolvedValue({}); + searchObjectsSpy.mockResolvedValue({ total: 0, data: [] }); + getObjectPublicSpy.mockResolvedValue(true); + readSpy.mockResolvedValue(comsObject); + getObjectTaggingSpy.mockResolvedValue({ + TagSet: [{}, { Key: 'coms-id', Value: validUuidv4 }] + }); + deleteObjectTaggingSpy.mockImplementation(jest.fn()); + + const result = await service.syncObject(path, bucketId); + + expect(result).toBeTruthy(); + expect(result.modified).toBeTruthy(); + expect(result.object).toEqual(comsObject); + + expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); + expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(1); + expect(_deriveObjectIdSpy).toHaveBeenCalledWith(expect.any(Object), path, bucketId); + expect(readSpy).toHaveBeenCalledTimes(1); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(1); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(1); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(1); + expect(createSpy).toHaveBeenCalledTimes(1); + expect(createSpy).toHaveBeenCalledWith(expect.objectContaining({ + id: expect.stringMatching(uuidv4Regex), + name: path.match(/(?!.*\/)(.*)$/)[0], + path: path, + public: true, + bucketId: bucketId, + userId: expect.any(String), + }), expect.any(Object)); + expect(createSpy.mock.calls[0]['id']).not.toEqual(validUuidv4); + expect(deleteSpy).toHaveBeenCalledTimes(0); + expect(getObjectPublicSpy).toHaveBeenCalledTimes(1); + expect(getObjectPublicSpy).toHaveBeenCalledWith(expect.objectContaining({ + filePath: path, bucketId: bucketId + })); + expect(headObjectSpy).toHaveBeenCalledTimes(1); + expect(headObjectSpy).toHaveBeenCalledWith(expect.objectContaining({ + filePath: path, bucketId: bucketId + })); + expect(pruneOrphanedMetadataSpy).toHaveBeenCalledTimes(0); + expect(pruneOrphanedTagsSpy).toHaveBeenCalledTimes(0); + expect(searchObjectsSpy).toHaveBeenCalledTimes(1); + expect(searchObjectsSpy).toHaveBeenCalledWith(expect.objectContaining({ + path: path, bucketId: bucketId + }), expect.any(Object)); + expect(objectModelTrx.commit).toHaveBeenCalledTimes(1); + }); + it('should drop COMS object when not in S3', async () => { const comsObject = { id: validUuidv4 }; deleteSpy.mockResolvedValue(comsObject); @@ -591,6 +670,10 @@ describe('syncObject', () => { expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1); expect(_deriveObjectIdSpy).toHaveBeenCalledTimes(0); + expect(readSpy).toHaveBeenCalledTimes(0); + expect(getObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(deleteObjectTaggingSpy).toHaveBeenCalledTimes(0); + expect(putObjectTaggingSpy).toHaveBeenCalledTimes(0); expect(createSpy).toHaveBeenCalledTimes(0); expect(deleteSpy).toHaveBeenCalledTimes(1); expect(deleteSpy).toHaveBeenCalledWith(validUuidv4, expect.any(Object));