Skip to content

Commit

Permalink
fix: dependent remove errors, more tests (#532)
Browse files Browse the repository at this point in the history
- make sure parent record is not removed if any of the child records
fails to remove
- add more IT tests
  • Loading branch information
solaris007 authored Jan 10, 2025
1 parent 2cd886f commit cdec563
Showing 5 changed files with 116 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -252,14 +252,27 @@ class BaseModel {
async _remove() {
try {
const dependents = await this.#fetchDependents();
// eslint-disable-next-line no-underscore-dangle
const removePromises = dependents.map((dependent) => dependent._remove());
removePromises.push(this.entity.remove({ [this.idName]: this.getId() }).go());

const removePromises = dependents.map(async (dependent) => {
try {
// eslint-disable-next-line no-underscore-dangle
await dependent._remove();
} catch (e) {
this.log.error(`Failed to remove dependent entity ${dependent.entityName} with ID ${dependent.getId()}`, e);
throw new DataAccessError(
`Failed to remove dependent entity ${dependent.entityName} with ID ${dependent.getId()}`,
dependent,
e,
);
}
});

this.log.info(`Removing entity ${this.entityName} with ID ${this.getId()} and ${dependents.length} dependents`);

await Promise.all(removePromises);

await this.entity.remove({ [this.idName]: this.getId() }).go();

this.#invalidateCache();

return this;
Original file line number Diff line number Diff line change
@@ -16,6 +16,8 @@ import { isIsoDate } from '@adobe/spacecat-shared-utils';

import { expect, use } from 'chai';
import chaiAsPromised from 'chai-as-promised';
import sinon from 'sinon';
import sinonChai from 'sinon-chai';
import { v4 as uuid, validate as uuidValidate } from 'uuid';

import { ValidationError } from '../../../src/index.js';
@@ -26,23 +28,38 @@ import { seedDatabase } from '../util/seed.js';
import { sanitizeIdAndAuditFields, sanitizeTimestamps } from '../../../src/v2/util/util.js';

use(chaiAsPromised);
use(sinonChai);

describe('Opportunity IT', async () => {
const { siteId } = fixtures.sites[0];

let sampleData;
let mockLogger;

let Opportunity;
let Suggestion;

before(async () => {
sampleData = await seedDatabase();
});

beforeEach(() => {
mockLogger = {
debug: sinon.stub(),
error: sinon.stub(),
info: sinon.stub(),
warn: sinon.stub(),
};

const dataAccess = getDataAccess();
const dataAccess = getDataAccess({}, mockLogger);
Opportunity = dataAccess.Opportunity;
Suggestion = dataAccess.Suggestion;
});

afterEach(() => {
sinon.restore();
});

it('finds one opportunity by id', async () => {
const opportunity = await Opportunity.findById(sampleData.opportunities[0].getId());

@@ -204,6 +221,31 @@ describe('Opportunity IT', async () => {
}));
});

it('throws when removing a dependent fails', async () => { /* eslint-disable no-underscore-dangle */
const opportunity = await Opportunity.findById(sampleData.opportunities[1].getId());
const suggestions = await opportunity.getSuggestions();

expect(suggestions).to.be.an('array').with.length(3);

// make one suggestion fail to remove
suggestions[0]._remove = sinon.stub().rejects(new Error('Failed to remove suggestion'));

opportunity.getSuggestions = sinon.stub().resolves(suggestions);

await expect(opportunity.remove()).to.be.rejectedWith(`Failed to remove entity opportunity with ID ${opportunity.getId()}`);
expect(suggestions[0]._remove).to.have.been.calledOnce;
expect(mockLogger.error).to.have.been.calledWith(`Failed to remove dependent entity suggestion with ID ${suggestions[0].getId()}`);

// make sure the opportunity is still there
const stillThere = await Opportunity.findById(sampleData.opportunities[1].getId());
expect(stillThere).to.be.an('object');

// make sure the other suggestions are removed
const remainingSuggestions = await Suggestion.allByOpportunityId(opportunity.getId());
expect(remainingSuggestions).to.be.an('array').with.length(1);
expect(remainingSuggestions[0].getId()).to.equal(suggestions[0].getId());
});

it('creates many opportunities', async () => {
const data = [
{
Original file line number Diff line number Diff line change
@@ -150,6 +150,42 @@ describe('SiteTopPage IT', async () => {
expect(updatedSiteTopPage.getImportedAt()).to.equal(updates.importedAt);
});

it('stores and returns multiple top pages with identical source, geo and traffic', async () => {
const site = sampleData.sites[0];
const source = 'some-source';
const geo = 'APAC';
const traffic = 1000;
const createdPages = [];

for (let i = 0; i < 2; i += 1) {
const data = {
siteId: site.getId(),
url: `https://www.example.com/page${i}`,
traffic,
source,
topKeyword: 'example',
geo,
};

// eslint-disable-next-line no-await-in-loop
createdPages.push(await SiteTopPage.create(data));
}

const siteTopPages = await SiteTopPage.allBySiteIdAndSourceAndGeo(
site.getId(),
source,
geo,
{ order: 'desc' },
);

expect(siteTopPages).to.be.an('array');
expect(siteTopPages.length).to.equal(2);

// results ordered by updatedAt desc
expect(siteTopPages[1].getId()).to.equal(createdPages[0].getId());
expect(siteTopPages[0].getId()).to.equal(createdPages[1].getId());
});

it('removes a site top page', async () => {
const siteTopPage = await SiteTopPage.findById(sampleData.siteTopPages[0].getId());

4 changes: 2 additions & 2 deletions packages/spacecat-shared-data-access/test/it/util/db.js
Original file line number Diff line number Diff line change
@@ -70,9 +70,9 @@ const getDynamoClients = (config = {}) => {
return { dbClient, docClient };
};

export const getDataAccess = (config) => {
export const getDataAccess = (config, logger = console) => {
const { dbClient } = getDynamoClients(config);
return createDataAccess(TEST_DA_CONFIG, console, dbClient);
return createDataAccess(TEST_DA_CONFIG, logger, dbClient);
};

export { getDynamoClients };
Original file line number Diff line number Diff line change
@@ -254,6 +254,25 @@ describe('BaseModel', () => { /* eslint-disable no-underscore-dangle */
expect(dependent._remove.notCalled).to.be.true;
});

it('logs an error and throws if removal of a dependent fails', async () => {
const reference = Reference.fromJSON({
type: Reference.TYPES.HAS_ONE,
target: 'SomeModel',
options: { removeDependents: true },
});

baseModelInstance.getSomeModel = stub().resolves(dependent);
baseModelInstance.getSuggestions = stub().resolves(dependents);

schema.references.push(reference);

const error = new Error('Remove failed');
dependent._remove = stub().returns(Promise.reject(error));

await expect(baseModelInstance.remove()).to.be.rejectedWith('Failed to remove entity opportunity with ID 12345');
expect(mockLogger.error.calledOnce).to.be.true;
});

it('logs an error and throws when remove fails', async () => {
const error = new Error('Remove failed');
mockElectroService.entities.opportunity.remove.returns({ go: () => Promise.reject(error) });

0 comments on commit cdec563

Please sign in to comment.