Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,39 @@ export interface Opportunity extends BaseModel {
getAuditId(): string;
getData(): object;
getDescription(): string;
getFixEntities(): Promise<FixEntity[]>;
getGuidance(): string;
getLastAuditedAt(): string;
getOrigin(): string;
getRunbook(): string;
getScopeId(): string | undefined;
getScopeType(): string | undefined;
getSite(): Promise<Site>;
getSiteId(): string;
getStatus(): string;
getFixEntities(): Promise<FixEntity[]>;
getSuggestions(): Promise<Suggestion[]>;
getSuggestionsByStatus(status: string): Promise<Suggestion[]>;
getSuggestionsByStatusAndRank(status: string, rank: string): Promise<Suggestion[]>;
getLastAuditedAt(): string;
getTags(): string[];
getTitle(): string;
getType(): string;
setAuditId(auditId: string): Opportunity;
setData(data: object): Opportunity;
setDescription(description: string): Opportunity;
setLastAuditedAt(lastAuditedAt: string): Opportunity;
setGuidance(guidance: string): Opportunity;
setLastAuditedAt(lastAuditedAt: string): Opportunity;
setOrigin(origin: string): Opportunity;
setRunbook(runbook: string): Opportunity;
setScopeId(scopeId: string): Opportunity;
setScopeType(scopeType: 'brand'): Opportunity;
setSiteId(siteId: string): Opportunity;
setStatus(status: string): Opportunity;
setTags(tags: string[]): Opportunity;
setTitle(title: string): Opportunity;
}

export interface OpportunityCollection extends BaseCollection<Opportunity> {
allByScope(scopeType: string, scopeId: string): Promise<Opportunity[]>;
allByAuditId(auditId: string): Promise<Opportunity[]>;
allByAuditIdAndUpdatedAt(auditId: string, updatedAt: string): Promise<Opportunity[]>;
allBySiteId(siteId: string): Promise<Opportunity[]>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
* governing permissions and limitations under the License.
*/

import { hasText } from '@adobe/spacecat-shared-utils';

import { ValidationError } from '../../errors/index.js';
import BaseCollection from '../base/base.collection.js';

/**
Expand All @@ -22,7 +25,38 @@ import BaseCollection from '../base/base.collection.js';
class OpportunityCollection extends BaseCollection {
static COLLECTION_NAME = 'OpportunityCollection';

// add custom methods here
/**
* Validates and creates a new Opportunity. Enforces that scopeType and scopeId
* must both be present or both be absent — a half-scoped record is invalid.
*
* @param {object} item - The opportunity data.
* @param {object} [options] - Optional create options (e.g. { upsert: true }).
* @returns {Promise<Opportunity>} The created opportunity instance.
*/
async create(item, options) {
const { scopeType, scopeId } = item || {};
if (Boolean(scopeType) !== Boolean(scopeId)) {
throw new ValidationError('scopeType and scopeId must both be set or both be absent');
}
return super.create(item, options);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This create() override guards a single write entry point. Three paths inherited from BaseCollection still bypass both this guard and the new Opportunity.save() override:

  • updateByKeys(keys, { scopeId: '<other-brand-uuid>' }) - patches via entity.patch(keys).set(...).go() without model instantiation. Can also re-attribute an existing opportunity to a different brand's scopeId without touching scopeType.
  • createMany([...]) - calls entity.put(item).params() per item, does not delegate to this.create().
  • saveMany(items) / _saveMany(items) - calls entity.put(updates).go() directly.

Preferred fix: mark scopeType and scopeId as readOnly: true in opportunity.schema.js. Post-create mutation becomes impossible by construction, co-presence only needs to live in create(), and all three bulk paths are closed at once - without adding more overrides.

}

/**
* Returns all opportunities matching a given scope type and scope ID.
*
* @param {string} scopeType - The scope type (e.g. 'brand').
* @param {string} scopeId - The scope entity UUID.
* @returns {Promise<Opportunity[]>} The matching opportunities.
*/
async allByScope(scopeType, scopeId) {
if (!hasText(scopeType)) {
throw new Error('allByScope: scopeType is required');
}
if (!hasText(scopeId)) {
throw new Error('allByScope: scopeId is required');
}
return this.allByIndexKeys({ scopeType, scopeId });
}
}

export default OpportunityCollection;
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class Opportunity extends BaseModel {
RESOLVED: 'RESOLVED',
};

static SCOPE_TYPES = {
BRAND: 'brand',
};

/**
* Adds the given suggestions to this Opportunity. Sets this opportunity as the parent
* of each suggestion, as such the opportunity ID does not need to be provided.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@

/* c8 ignore start */

import { isIsoDate, isNonEmptyObject, isValidUrl } from '@adobe/spacecat-shared-utils';
import {
isIsoDate, isNonEmptyObject, isValidUrl, isValidUUID,
} from '@adobe/spacecat-shared-utils';

import SchemaBuilder from '../base/schema.builder.js';
import Opportunity from './opportunity.model.js';
Expand Down Expand Up @@ -64,6 +66,15 @@ const schema = new SchemaBuilder(Opportunity, OpportunityCollection)
.addAttribute('lastAuditedAt', {
type: 'string',
validate: (value) => !value || isIsoDate(value),
});
})
.addAttribute('scopeType', {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important - no co-presence validation, and strict enum may reject existing NULL rows.

Two issues at scopeType and the scopeId pair:

No co-presence enforcement. Both attributes are independently optional. A record can be saved with scopeType='brand' and no scopeId, or vice versa. allByScopeId silently misses half-scoped rows; downstream auth using getScopeType() can attribute an opportunity to the wrong tenant. Add a cross-field validator enforcing both-or-neither.

Strict enum may break hydration of legacy rows. type: Object.values(Opportunity.SCOPE_TYPES) becomes ['site', 'brand']. The PR description says these columns already exist, so historical rows almost certainly have NULL for scope_type. Compare to lastAuditedAt which uses validate: (value) => !value || isIsoDate(value) to tolerate absent values. Apply the same pattern or test against a DB snapshot containing NULL-scope rows.

type: 'string',
validate: (value) => !value || Object.values(Opportunity.SCOPE_TYPES).includes(value),
})
.addAttribute('scopeId', {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical - missing composite index AND no format constraint on scopeId.

Two issues at this attribute declaration:

Missing index for allByScopeId. allByIndexKeys({ scopeType, scopeId }) requires a registered composite GSI on (scopeType, scopeId). This diff adds addAttribute only - no addIndex call. Without the index, this call throws or full-scans at runtime. The unit test stubs allByIndexKeys (opportunity.collection.test.js:90) so CI does not catch this. Add the composite index declaration to the schema and add an integration test that exercises allByScopeId against a real table.

No format constraint on scopeId. type: 'string' accepts CRLF characters (log injection), oversized payloads, and non-UUID strings. scopeId is authorization-bearing: it controls tenant attribution and the companion audit-worker PR feeds brandId from an SQS payload directly to setScopeId. Apply UUID validation consistent with how primary keys are handled in this codebase:

import { isValidUUID } from '@adobe/spacecat-shared-utils';
.addAttribute('scopeId', {
  type: 'string',
  validate: (value) => !value || isValidUUID(value),
})

type: 'string',
validate: (value) => !value || isValidUUID(value),
})
.addIndex({ composite: ['scopeId'] }, { composite: ['scopeType'] });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-presence gap: there is no cross-field validator enforcing that scopeType and scopeId are both set or both absent. A record saved with only one of the two will silently disappear from allByScope results. Add a validate function on scopeId that checks item.scopeType is consistent, and add unit tests for each half-set combination.

export default schema.build();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hot partition: scopeType has only one enum value ('brand'), so this GSI partition key concentrates all scoped opportunities in a single DynamoDB partition. DynamoDB cannot split that partition as the table grows. Invert: scopeId (UUID, high cardinality) should be the partition key and scopeType the sort key:

.addIndex({ composite: ['scopeId'] }, { composite: ['scopeType'] })

Fix while no production data exists yet.

Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,67 @@ describe('Opportunity IT', async () => {
expect(record2).to.eql(data[1]);
});

describe('allByScope', () => {
const BRAND_A_ID = 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa';
const BRAND_B_ID = 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbbbb';

const scopedOpportunityBase = {
siteId,
title: 'Scoped Opportunity',
description: 'Test for allByScope',
type: 'prerender',
origin: 'AI',
status: 'NEW',
updatedBy: 'system',
};

it('returns only opportunities matching the given brandId', async () => {
// Create one opp for brand-a and one for brand-b on the same site
const oppA = await Opportunity.create({
...scopedOpportunityBase,
title: 'Brand A Opportunity',
scopeType: 'brand',
scopeId: BRAND_A_ID,
});
const oppB = await Opportunity.create({
...scopedOpportunityBase,
title: 'Brand B Opportunity',
scopeType: 'brand',
scopeId: BRAND_B_ID,
});

const resultA = await Opportunity.allByScope('brand', BRAND_A_ID);
const resultB = await Opportunity.allByScope('brand', BRAND_B_ID);

const idSetA = resultA.map((o) => o.getId());
const idSetB = resultB.map((o) => o.getId());

expect(idSetA).to.include(oppA.getId());
expect(idSetA).to.not.include(oppB.getId());

expect(idSetB).to.include(oppB.getId());
expect(idSetB).to.not.include(oppA.getId());
});

it('returns an empty array when no opportunities match the scopeId', async () => {
const UNKNOWN_BRAND_ID = 'cccccccc-cccc-4ccc-cccc-cccccccccccc';
const result = await Opportunity.allByScope('brand', UNKNOWN_BRAND_ID);
expect(result).to.be.an('array').with.length(0);
});

it('rejects co-presence violation: scopeType set without scopeId', async () => {
await expect(
Opportunity.create({ ...scopedOpportunityBase, scopeType: 'brand' }),
).to.be.rejectedWith('scopeType and scopeId must both be set or both be absent');
});

it('rejects co-presence violation: scopeId set without scopeType', async () => {
await expect(
Opportunity.create({ ...scopedOpportunityBase, scopeId: BRAND_A_ID }),
).to.be.rejectedWith('scopeType and scopeId must both be set or both be absent');
});
});

describe('addFixEntities', () => {
it('creates fix entities with valid suggestions', async () => {
const opportunity = await Opportunity.findById(sampleData.opportunities[2].getId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ services:

data-service:
platform: ${MYSTICAT_DATA_SERVICE_PLATFORM:-linux/amd64}
image: ${MYSTICAT_DATA_SERVICE_REPOSITORY:-682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service}:${MYSTICAT_DATA_SERVICE_TAG:-v1.67.8}
image: ${MYSTICAT_DATA_SERVICE_REPOSITORY:-682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service}:${MYSTICAT_DATA_SERVICE_TAG:-v5.1.1}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical - unexplained 4-major-version image bump.

v1.67.8 to v5.1.1 is a four-major-version jump with no explanation in the PR description. Either:

  • This is required because v5.x introduces the scope_type/scope_id columns and/or the backing GSI. State this explicitly, confirm all test fixtures remain compatible, and document the production deployment order.
  • It is opportunistic cleanup - move it to its own PR so its blast radius is independently reviewable.

A future bisect on this commit will need to unpack which of two simultaneous changes caused a regression. Also: the image is pinned by tag (mutable) - pin by @sha256:... digest for reproducible builds.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexplained bump + mutable tag: the jump from v1.67.8 to v5.1.1 needs a comment or PR description entry explaining that v5.x carries the DynamoDB migration for scope_type/scope_id columns and the backing GSI. Without this, nobody on the release team knows the data-service must be at v5.x before this code ships.

Also, tag-pinned images are mutable - the same tag can change without notice. Pin to a digest instead:

image: 682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service@sha256:<digest>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment added in this commit explains the bump policy for developers running tests locally - that is an improvement. Two gaps remain:

  1. The tag :v5.1.1 is mutable. If it is ever re-pushed in ECR, CI will silently test against a different binary. Pin by digest: image: ...:v5.1.1@sha256:<digest> (Compose supports tag-plus-digest; the tag stays human-readable).
  2. The PR description still does not note that mysticat-data-service v5.x is a deployment prerequisite for this code change. A docker-compose comment is read by engineers running tests; it is not read by engineers sequencing a deploy. Please add one sentence to the PR body: "Requires mysticat-data-service v5.x (adds scope_type/scope_id columns and the scopeId/scopeType GSI). Confirm target environment is on v5.x before deployment."

depends_on:
db:
condition: service_healthy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import { expect, use as chaiUse } from 'chai';
import chaiAsPromised from 'chai-as-promised';
import { stub } from 'sinon';
import sinonChai from 'sinon-chai';

import Opportunity from '../../../../src/models/opportunity/opportunity.model.js';
Expand Down Expand Up @@ -70,4 +71,69 @@ describe('OpportunityCollection', () => {
expect(model).to.be.an('object');
});
});

describe('create', () => {
it('throws ValidationError when scopeType is set but scopeId is absent', async () => {
await expect(
instance.create({ type: 'content', scopeType: 'brand' }),
).to.be.rejectedWith('scopeType and scopeId must both be set or both be absent');
});

it('throws ValidationError when scopeId is set but scopeType is absent', async () => {
await expect(
instance.create({ type: 'content', scopeId: '11111111-1111-1111-1111-111111111111' }),
).to.be.rejectedWith('scopeType and scopeId must both be set or both be absent');
});

it('passes co-presence check when both scopeType and scopeId are set', async () => {
// Co-presence check passes — mock create succeeds without complaining.
await expect(
instance.create({ scopeType: 'brand', scopeId: '11111111-1111-1111-1111-111111111111' }),
).to.be.fulfilled;
});

it('passes co-presence check when neither scopeType nor scopeId is set', async () => {
// Neither present — co-presence check passes and mock create succeeds.
await expect(
instance.create({ type: 'content' }),
).to.be.fulfilled;
});

it('handles null item gracefully by delegating to super.create', async () => {
// null item: co-presence check skips (both undefined → equal), super.create handles it.
await expect(instance.create(null)).to.be.rejectedWith(/Failed to create/);
});
});

describe('allByScope', () => {
it('throws an error if scopeType is not provided', async () => {
await expect(instance.allByScope()).to.be.rejectedWith('allByScope: scopeType is required');
});

it('throws an error if scopeId is not provided', async () => {
await expect(instance.allByScope('brand')).to.be.rejectedWith('allByScope: scopeId is required');
});

it('delegates to allByIndexKeys with the correct arguments', async () => {
const mockOpportunity = { getOpportunityId: () => 'op-111' };
instance.allByIndexKeys = stub().resolves([mockOpportunity]);

const BRAND_UUID = 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa';
const result = await instance.allByScope('brand', BRAND_UUID);

expect(instance.allByIndexKeys).to.have.been.calledOnceWith({
scopeType: 'brand',
scopeId: BRAND_UUID,
});
expect(result).to.deep.equal([mockOpportunity]);
});

it('returns an empty array when no opportunities match the scope', async () => {
instance.allByIndexKeys = stub().resolves([]);

const result = await instance.allByScope('brand', 'cccccccc-cccc-4ccc-cccc-cccccccccccc');

expect(result).to.be.an('array').with.lengthOf(0);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -384,4 +384,43 @@ describe('OpportunityModel', () => {
expect(instance.record.data).to.deep.equal({ newInfo: 'updatedInfo' });
});
});

describe('getScopeType and setScopeType', () => {
it('returns undefined when scopeType is not set', () => {
expect(instance.getScopeType()).to.be.undefined;
});

it('returns the scopeType value', () => {
mockRecord.scopeType = 'brand';
expect(instance.getScopeType()).to.equal('brand');
});

it('sets the scopeType value', () => {
instance.setScopeType('brand');
expect(instance.record.scopeType).to.equal('brand');
});
});

describe('getScopeId and setScopeId', () => {
it('returns undefined when scopeId is not set', () => {
expect(instance.getScopeId()).to.be.undefined;
});

it('returns the scopeId value', () => {
mockRecord.scopeId = 'brand-uuid-123';
expect(instance.getScopeId()).to.equal('brand-uuid-123');
});

it('sets the scopeId value', () => {
instance.setScopeId('brand-uuid-456');
expect(instance.record.scopeId).to.equal('brand-uuid-456');
});
});

describe('SCOPE_TYPES', () => {
it('defines expected scope type constants', () => {
expect(Opportunity.SCOPE_TYPES.BRAND).to.equal('brand');
expect(Opportunity.SCOPE_TYPES.SITE).to.be.undefined;
});
});
});
Loading