Skip to content

Commit

Permalink
fix: pr feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@solaris007
solaris007 committed Dec 12, 2024
1 parent 5853858 commit 20f275f
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 48 deletions.
27 changes: 24 additions & 3 deletions packages/spacecat-shared-data-access/src/v2/models/base/base.collection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ function keyNamesToMethodName(keyNames, prefix) {
return prefix + keyNames.map(capitalize).join('And');
}

function isValidParent(parent, child) {
if (!hasText(parent.entityName)) {
return false;
}

const foreignKey = `${parent.entityName}Id`;

return child.record?.[foreignKey] === parent.record?.[foreignKey];
}

function validateValue(context, keyName, value) {
const { type } = context.entity.model.schema.attributes[keyName];
const validator = type === 'number' ? isNumber : hasText;
Expand Down Expand Up @@ -298,7 +308,7 @@ class BaseCollection {
* Finds a single entity from the "all" index. Requires an index named "all" with a partition key
* named "pk" with a static value of "ALL_<ENTITYNAME>".
* @param {Object} [sortKeys] - The sort keys to use for the query.
* @param {Object} [options] - Additional options for the query.
* @param {{index?: string, attributes?: string[]}} [options] - Additional options for the query.
* @return {Promise<BaseModel|Array<BaseModel>|null>}
*/
async findByAll(sortKeys = {}, options = {}) {
Expand Down Expand Up @@ -331,7 +341,7 @@ class BaseCollection {
/**
* Finds a single entity by index keys.
* @param {Object} keys - The index keys to use for the query.
* @param {Object} options - Additional options for the query.
* @param {{index?: string, attributes?: string[]}} [options] - Additional options for the query.
* @returns {Promise<BaseModel|null>} - A promise that resolves to the model instance or null.
* @async
*/
Expand Down Expand Up @@ -421,8 +431,12 @@ class BaseCollection {

const createdItems = this.#createInstances(validatedItems);

if (parent) {
if (isNonEmptyObject(parent)) {
createdItems.forEach((record) => {
if (!isValidParent(parent, record)) {
this.log.warn(`Failed to associate parent with child [${this.entityName}]: parent is invalid`);
return;
}
// eslint-disable-next-line no-underscore-dangle
record._cacheReference(parent.entity.model.name, parent);
});
Expand Down Expand Up @@ -464,6 +478,13 @@ class BaseCollection {
}
}

/**
* Removes all records of this entity based on the provided IDs.
* @param {Array<string>} ids - An array of IDs to remove.
* @return {Promise<void>} - A promise that resolves when the removal operation is complete.
* @throws {Error} - Throws an error if the IDs are not provided or if the
* removal operation fails.
*/
async removeByIds(ids) {
if (!Array.isArray(ids) || ids.length === 0) {
const message = `Failed to remove [${this.entityName}]: ids must be a non-empty array`;
Expand Down
7 changes: 2 additions & 5 deletions packages/spacecat-shared-data-access/test/it/suggestion/suggestion.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import chaiAsPromised from 'chai-as-promised';
import { validate as uuidValidate } from 'uuid';

import { ValidationError } from '../../../src/index.js';
import { sanitizeTimestamps } from '../../../src/v2/util/util.js';
import { sanitizeIdAndAuditFields, sanitizeTimestamps } from '../../../src/v2/util/util.js';

import { getDataAccess } from '../util/db.js';
import { seedDatabase } from '../util/seed.js';
Expand Down Expand Up @@ -176,11 +176,8 @@ describe('Suggestion IT', async () => {
expect(isIsoDate(suggestion.getCreatedAt())).to.be.true;
expect(isIsoDate(suggestion.getUpdatedAt())).to.be.true;

const record = suggestion.toJSON();
const record = sanitizeIdAndAuditFields('Suggestion', suggestion.toJSON());
delete record.opportunityId;
delete record.suggestionId;
delete record.createdAt;
delete record.updatedAt;

expect(record).to.eql(data[index]);
});
Expand Down
139 changes: 99 additions & 40 deletions packages/spacecat-shared-data-access/test/unit/v2/models/base/base.collection.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,29 @@ import { spy, stub } from 'sinon';
import sinonChai from 'sinon-chai';

import BaseCollection from '../../../../../src/v2/models/base/base.collection.js';
import BaseModel from '../../../../../src/v2/models/base/base.model.js';

chaiUse(chaiAsPromised);
chaiUse(sinonChai);

describe('BaseCollection', () => {
const MockModel = class MockEntityModel extends BaseModel {
// eslint-disable-next-line class-methods-use-this
_cacheReference() {}
};

let baseCollectionInstance;
let mockElectroService;
let mockEntityRegistry;
let mockModel;
let mockLogger;

const mockRecord = {
id: 'ef39921f-9a02-41db-b491-02c98987d956',
mockEntityModelId: 'ef39921f-9a02-41db-b491-02c98987d956',
mockParentEntityModelId: 'some-parent-id',
data: {
someKey: 'someValue',
},
};
const mockEntityModel = {
data: { ...mockRecord },
};

beforeEach(() => {
mockEntityRegistry = {
Expand All @@ -50,15 +53,6 @@ describe('BaseCollection', () => {
warn: spy(),
};

mockModel = class MockEntityModel {
constructor(service, factory, data) {
this.data = data;
}

// eslint-disable-next-line class-methods-use-this
_cacheReference() {}
};

mockElectroService = {
entities: {
mockEntityModel: {
Expand All @@ -77,9 +71,13 @@ describe('BaseCollection', () => {
primary: stub(),
},
model: {
name: 'mockentitymodel',
name: 'mockEntityModel',
indexes: {},
table: 'mockentitymodel',
table: 'data',
original: {},
schema: {
attributes: {},
},
},
},
},
Expand All @@ -88,23 +86,23 @@ describe('BaseCollection', () => {
baseCollectionInstance = new BaseCollection(
mockElectroService,
mockEntityRegistry,
mockModel,
MockModel,
mockLogger,
);
});

describe('collection methods', () => {
const createInstancew = () => new BaseCollection(
const createInstance = () => new BaseCollection(
mockElectroService,
mockEntityRegistry,
mockModel,
MockModel,
mockLogger,
);

it('does not create accessors for the primary index', () => {
mockElectroService.entities.mockEntityModel.model.indexes = { primary: {} };

const instance = createInstancew();
const instance = createInstance();

expect(instance).to.not.have.property('allBy');
expect(instance).to.not.have.property('findBy');
Expand All @@ -115,7 +113,7 @@ describe('BaseCollection', () => {
bySomeKey: { pk: { facets: ['someKey'] } },
};

const instance = createInstancew();
const instance = createInstance();

expect(instance).to.have.property('allBySomeKey');
expect(instance).to.have.property('findBySomeKey');
Expand All @@ -126,7 +124,7 @@ describe('BaseCollection', () => {
bySomeKey: { sk: { facets: ['someKey'] } },
};

const instance = createInstancew();
const instance = createInstance();

expect(instance).to.have.property('allBySomeKey');
expect(instance).to.have.property('findBySomeKey');
Expand All @@ -137,7 +135,7 @@ describe('BaseCollection', () => {
bySomeKey: { pk: { facets: ['someKey'] }, sk: { facets: ['someOtherKey'] } },
};

const instance = createInstancew();
const instance = createInstance();

expect(instance).to.have.property('allBySomeKey');
expect(instance).to.have.property('allBySomeKeyAndSomeOtherKey');
Expand All @@ -160,7 +158,7 @@ describe('BaseCollection', () => {
},
};

const instance = createInstancew();
const instance = createInstance();
const someKey = 'someValue';
const someOtherKey = 1;
const options = { order: 'desc' };
Expand All @@ -187,7 +185,8 @@ describe('BaseCollection', () => {
);

const result = await baseCollectionInstance.findById('ef39921f-9a02-41db-b491-02c98987d956');
expect(result).to.deep.include(mockEntityModel);

expect(result.record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.get.calledOnce).to.be.true;
});

Expand All @@ -197,6 +196,7 @@ describe('BaseCollection', () => {
);

const result = await baseCollectionInstance.findById('ef39921f-9a02-41db-b491-02c98987d956');

expect(result).to.be.null;
expect(mockElectroService.entities.mockEntityModel.get.calledOnce).to.be.true;
});
Expand Down Expand Up @@ -224,11 +224,11 @@ describe('BaseCollection', () => {

it('creates a new entity successfully', async () => {
mockElectroService.entities.mockEntityModel.create.returns(
{ go: () => Promise.resolve(mockEntityModel) },
{ go: () => Promise.resolve({ data: mockRecord }) },
);

const result = await baseCollectionInstance.create(mockRecord);
expect(result).to.deep.include(mockEntityModel);
expect(result.record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.create.calledOnce).to.be.true;
});

Expand Down Expand Up @@ -273,7 +273,8 @@ describe('BaseCollection', () => {

const result = await baseCollectionInstance.createMany(mockRecords);
expect(result.createdItems).to.be.an('array').that.has.length(2);
expect(result.createdItems).to.deep.include(mockEntityModel);
expect(result.createdItems[0].record).to.deep.include(mockRecord);
expect(result.createdItems[1].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.put.calledThrice).to.be.true;
});

Expand All @@ -298,13 +299,66 @@ describe('BaseCollection', () => {
},
);

const result = await baseCollectionInstance.createMany(
mockRecords,
{ entity: { model: { name: 'mockEntityModel' } } },
);
const parent = {
record: { mockParentEntityModelId: mockRecord.mockParentEntityModelId },
entityName: 'mockParentEntityModel',
entity: { model: { name: 'mockParentEntityModel' } },
};

const result = await baseCollectionInstance.createMany(mockRecords, parent);

expect(result.createdItems).to.be.an('array').that.has.length(2);
expect(result.createdItems).to.deep.include(mockEntityModel);
expect(result.createdItems[0].record).to.deep.include(mockRecord);
expect(result.createdItems[1].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.put.calledThrice).to.be.true;
expect(mockLogger.warn).to.not.have.been.called;
});

it('logs warning if parent is invalid', async () => {
const mockRecords = [mockRecord, mockRecord];
const mockPutResults = {
type: 'query',
method: 'batchWrite',
params: {
RequestItems: {
mockEntityModel: [
{ PutRequest: { Item: mockRecord } },
{ PutRequest: { Item: mockRecord } },
],
},
},
};
mockElectroService.entities.mockEntityModel.put.returns(
{
go: () => Promise.resolve(mockPutResults),
params: () => ({ Item: { ...mockRecord } }),
},
);

const idNotMatchingParent = {
record: { mockParentEntityModelId: 'invalid-id' },
entityName: 'mockParentEntityModel',
entity: { model: { name: 'mockParentEntityModel' } },
};

const noEntityParent = {
record: { mockParentEntityModelId: 'invalid-id' },
entity: { model: { name: 'mockParentEntityModel' } },
};

const r1 = await baseCollectionInstance.createMany(mockRecords, idNotMatchingParent);
const r2 = await baseCollectionInstance.createMany(mockRecords, noEntityParent);

expect(r1.createdItems).to.be.an('array').that.has.length(2);
expect(r1.createdItems[0].record).to.deep.include(mockRecord);
expect(r1.createdItems[1].record).to.deep.include(mockRecord);

expect(r2.createdItems).to.be.an('array').that.has.length(2);
expect(r2.createdItems[0].record).to.deep.include(mockRecord);
expect(r2.createdItems[1].record).to.deep.include(mockRecord);

expect(mockElectroService.entities.mockEntityModel.put).to.have.callCount(6);
expect(mockLogger.warn).to.have.callCount(4);
});

it('creates some entities successfully with unprocessed items', async () => {
Expand All @@ -327,7 +381,7 @@ describe('BaseCollection', () => {

const result = await baseCollectionInstance.createMany(mockRecords);
expect(result.createdItems).to.be.an('array').that.has.length(1);
expect(result.createdItems).to.deep.include(mockEntityModel);
expect(result.createdItems[0].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.put.calledThrice).to.be.true;
expect(mockLogger.error.calledOnceWith(`Failed to process all items in batch write for [mockEntityModel]: ${JSON.stringify([mockRecord])}`)).to.be.true;
});
Expand Down Expand Up @@ -409,7 +463,8 @@ describe('BaseCollection', () => {
);

const result = await baseCollectionInstance.all();
expect(result).to.deep.include(mockEntityModel);
expect(result).to.be.an('array').that.has.length(1);
expect(result[0].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.query.all)
.to.have.been.calledOnceWithExactly({ pk: 'ALL_MOCKENTITYMODELS' });
});
Expand All @@ -425,7 +480,8 @@ describe('BaseCollection', () => {
{ between: { attribute: 'test', start: 'a', end: 'b' } },
);

expect(result).to.deep.include(mockEntityModel);
expect(result).to.be.an('array').that.has.length(1);
expect(result[0].record).to.deep.include(mockRecord);
expect(mockBetween).to.have.been.calledOnceWithExactly({ test: 'a' }, { test: 'b' });
expect(mockGo).to.have.been.calledOnceWithExactly({ order: 'desc' });
});
Expand All @@ -439,7 +495,8 @@ describe('BaseCollection', () => {

const result = await baseCollectionInstance.all({}, { attributes: ['test'] });

expect(result).to.deep.include(mockEntityModel);
expect(result).to.be.an('array').that.has.length(1);
expect(result[0].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.query.all)
.to.have.been.calledOnceWithExactly({ pk: 'ALL_MOCKENTITYMODELS' });
expect(mockGo).to.have.been.calledOnceWithExactly({ order: 'desc', attributes: ['test'] });
Expand All @@ -466,7 +523,8 @@ describe('BaseCollection', () => {
);

const result = await baseCollectionInstance.allByIndexKeys({ someKey: 'someValue' });
expect(result).to.deep.include(mockEntityModel);
expect(result).to.be.an('array').that.has.length(1);
expect(result[0].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.query.bySomeKey)
.to.have.been.calledOnceWithExactly({ someKey: 'someValue' });
});
Expand All @@ -481,7 +539,8 @@ describe('BaseCollection', () => {
);

const result = await baseCollectionInstance.allByIndexKeys({ someKey: 'someValue' });
expect(result).to.deep.include(mockEntityModel);
expect(result).to.be.an('array').that.has.length(1);
expect(result[0].record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.query.primary)
.to.have.been.calledOnceWithExactly({ someKey: 'someValue' });
});
Expand All @@ -501,7 +560,7 @@ describe('BaseCollection', () => {
);

const result = await baseCollectionInstance.findByAll({ someKey: 'someValue' });
expect(result).to.deep.include(mockEntityModel);
expect(result.record).to.deep.include(mockRecord);
expect(mockElectroService.entities.mockEntityModel.query.all)
.to.have.been.calledOnceWithExactly(
{ pk: 'ALL_MOCKENTITYMODELS', someKey: 'someValue' },
Expand Down

0 comments on commit 20f275f

Please sign in to comment.