From cdec563c881d088feeb5ed24514f528173eec59d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominique=20J=C3=A4ggi?= <1872195+solaris007@users.noreply.github.com> Date: Fri, 10 Jan 2025 09:25:37 +0100 Subject: [PATCH] fix: dependent remove errors, more tests (#532) - make sure parent record is not removed if any of the child records fails to remove - add more IT tests --- .../src/v2/models/base/base.model.js | 19 ++++++-- .../test/it/opportunity/opportunity.test.js | 44 ++++++++++++++++++- .../it/site-top-page/site-top-page.test.js | 36 +++++++++++++++ .../test/it/util/db.js | 4 +- .../unit/v2/models/base/base.model.test.js | 19 ++++++++ 5 files changed, 116 insertions(+), 6 deletions(-) diff --git a/packages/spacecat-shared-data-access/src/v2/models/base/base.model.js b/packages/spacecat-shared-data-access/src/v2/models/base/base.model.js index 79d34de8..ad6d7d23 100755 --- a/packages/spacecat-shared-data-access/src/v2/models/base/base.model.js +++ b/packages/spacecat-shared-data-access/src/v2/models/base/base.model.js @@ -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; diff --git a/packages/spacecat-shared-data-access/test/it/opportunity/opportunity.test.js b/packages/spacecat-shared-data-access/test/it/opportunity/opportunity.test.js index 3c521a45..4d0c3e11 100644 --- a/packages/spacecat-shared-data-access/test/it/opportunity/opportunity.test.js +++ b/packages/spacecat-shared-data-access/test/it/opportunity/opportunity.test.js @@ -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 = [ { diff --git a/packages/spacecat-shared-data-access/test/it/site-top-page/site-top-page.test.js b/packages/spacecat-shared-data-access/test/it/site-top-page/site-top-page.test.js index a3141aa6..171b8938 100755 --- a/packages/spacecat-shared-data-access/test/it/site-top-page/site-top-page.test.js +++ b/packages/spacecat-shared-data-access/test/it/site-top-page/site-top-page.test.js @@ -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()); diff --git a/packages/spacecat-shared-data-access/test/it/util/db.js b/packages/spacecat-shared-data-access/test/it/util/db.js index 8d1a6450..dc7467c9 100755 --- a/packages/spacecat-shared-data-access/test/it/util/db.js +++ b/packages/spacecat-shared-data-access/test/it/util/db.js @@ -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 }; diff --git a/packages/spacecat-shared-data-access/test/unit/v2/models/base/base.model.test.js b/packages/spacecat-shared-data-access/test/unit/v2/models/base/base.model.test.js index 4e5c6bd2..0877ae97 100755 --- a/packages/spacecat-shared-data-access/test/unit/v2/models/base/base.model.test.js +++ b/packages/spacecat-shared-data-access/test/unit/v2/models/base/base.model.test.js @@ -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) });