Skip to content

Commit

Permalink
Only populate lastSyncedDate when syncing object
Browse files Browse the repository at this point in the history
In an earlier commit, lastSyncedDate would be updated/populated on object update/create (even when not syncing) - this is incorrect behaviour

Also:
* Have comsObject update within the syncObject transaction (`trx`)
* Update tests: sync service, object service
  • Loading branch information
norrisng-bc committed Feb 28, 2024
1 parent 7b8e77f commit 33696c9
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 21 deletions.
3 changes: 1 addition & 2 deletions app/src/services/object.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ const service = {
active: data.active,
bucketId: data.bucketId,
createdBy: data.userId ?? SYSTEM_USER,
lastSyncedDate: new Date()
};
const response = await ObjectModel.query(trx).insert(obj).returning('*');

Expand Down Expand Up @@ -234,7 +233,7 @@ const service = {
public: data.public,
active: data.active,
updatedBy: data.userId ?? SYSTEM_USER,
lastSyncedDate: new Date()
lastSyncedDate: data.lastSyncedDate ?? undefined
});

if (!etrx) await trx.commit();
Expand Down
13 changes: 9 additions & 4 deletions app/src/services/sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,15 @@ const service = {
// Case: already synced - record & update public status as needed
if (comsObject) {
if (s3Public === undefined || s3Public === comsObject.public) {
response = comsObject;
response = await objectService.update({ id: comsObject.id, lastSyncedDate: new Date().toISOString() }, trx);
} else {
response = await objectService.update({
id: comsObject.id, userId: userId, path: comsObject.path, public: s3Public
});
id: comsObject.id,
userId: userId,
path: comsObject.path,
public: s3Public,
lastSyncedDate: new Date().toISOString()
}, trx);
modified = true;
}
}
Expand All @@ -179,7 +183,8 @@ const service = {
path: path,
public: s3Public,
bucketId: bucketId,
userId: userId
userId: userId,
lastSyncedDate: new Date().toISOString()
}, trx);

modified = true;
Expand Down
31 changes: 21 additions & 10 deletions app/tests/unit/services/object.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ const data = {
public: 'true',
active: 'true',
createdBy: SYSTEM_USER,
userId: SYSTEM_USER
userId: SYSTEM_USER,
lastSyncedDate: undefined
};

beforeEach(() => {
Expand All @@ -43,7 +44,6 @@ beforeEach(() => {

describe('create', () => {
const addPermissionsSpy = jest.spyOn(objectPermissionService, 'addPermissions');
const dateSpy = jest.spyOn(global, 'Date');

beforeEach(() => {
addPermissionsSpy.mockReset();
Expand All @@ -59,7 +59,6 @@ describe('create', () => {
await service.create({ ...data, userId: SYSTEM_USER });

expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1);
expect(dateSpy).toHaveBeenCalledTimes(1);
expect(ObjectModel.query).toHaveBeenCalledTimes(1);
expect(ObjectModel.query).toHaveBeenCalledWith(expect.anything());
expect(ObjectModel.insert).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -230,11 +229,6 @@ describe('read', () => {
describe('update', () => {
it('Update an object DB record', async () => {

// Mocking the system time allows us to perform an assert/expect on lastSyncedDate.
// If not, then it's impossible to compare without resorting to string manipulation (ugly!)
jest.useFakeTimers();
jest.setSystemTime(new Date('2024-01-01T00:00:00'));

await service.update({ ...data });

expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1);
Expand All @@ -245,10 +239,27 @@ describe('update', () => {
public: data.public,
active: data.active,
updatedBy: data.userId,
lastSyncedDate: new Date()
lastSyncedDate: undefined
});
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);
});

it('Update an object DB record as part of a sync operation', async () => {

const testDateString = new Date('2024-01-01T00:00:00').toISOString();

jest.useRealTimers();
await service.update({ ...data, lastSyncedDate: testDateString });

expect(ObjectModel.startTransaction).toHaveBeenCalledTimes(1);
expect(ObjectModel.query).toHaveBeenCalledTimes(1);
expect(ObjectModel.patchAndFetchById).toHaveBeenCalledTimes(1);
expect(ObjectModel.patchAndFetchById).toBeCalledWith(data.id, {
path: data.path,
public: data.public,
active: data.active,
updatedBy: data.userId,
lastSyncedDate: testDateString
});
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);
});
});
17 changes: 12 additions & 5 deletions app/tests/unit/services/sync.spec.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const { NIL: SYSTEM_USER } = require('uuid');

const { resetModel, trxBuilder } = require('../../common/helper');
const utils = require('../../../src/db/models/utils');
const ObjectModel = require('../../../src/db/models/tables/objectModel');
Expand Down Expand Up @@ -371,14 +373,15 @@ describe('syncObject', () => {
pruneOrphanedMetadataSpy.mockRestore();
pruneOrphanedTagsSpy.mockRestore();
searchObjectsSpy.mockRestore();
updateSpy.mockReset();
updateSpy.mockRestore();
});

it('should return object when already synced', async () => {
const comsObject = { id: validUuidv4, path: path, public: true };
headObjectSpy.mockResolvedValue({});
searchObjectsSpy.mockResolvedValue({ total: 1, data: [comsObject] });
getObjectPublicSpy.mockResolvedValue(true);
updateSpy.mockResolvedValue(comsObject);

const result = await service.syncObject(path, bucketId);

Expand All @@ -405,7 +408,10 @@ describe('syncObject', () => {
path: path, bucketId: bucketId
}), expect.any(Object));
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);
expect(updateSpy).toHaveBeenCalledTimes(0);
expect(updateSpy).toHaveBeenCalledTimes(1);
expect(updateSpy).toHaveBeenCalledWith(expect.objectContaining({
id: comsObject.id, lastSyncedDate: expect.anything()
}), expect.any(Object));
});

it('should return object when already synced but public mismatch', async () => {
Expand Down Expand Up @@ -442,15 +448,16 @@ describe('syncObject', () => {
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);
expect(updateSpy).toHaveBeenCalledTimes(1);
expect(updateSpy).toHaveBeenCalledWith(expect.objectContaining({
id: validUuidv4, path: path, public: false
}));
id: validUuidv4, path: path, public: false, lastSyncedDate: expect.anything(), userId: SYSTEM_USER
}), expect.any(Object));
});

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({ total: 1, data: [comsObject] });
getObjectPublicSpy.mockImplementation(() => { throw new Error(); });
updateSpy.mockResolvedValue(comsObject);

const result = await service.syncObject(path, bucketId);

Expand All @@ -477,7 +484,7 @@ describe('syncObject', () => {
path: path, bucketId: bucketId
}), expect.any(Object));
expect(objectModelTrx.commit).toHaveBeenCalledTimes(1);
expect(updateSpy).toHaveBeenCalledTimes(0);
expect(updateSpy).toHaveBeenCalledTimes(1);
});

it('should insert new object when not in COMS', async () => {
Expand Down

0 comments on commit 33696c9

Please sign in to comment.