Skip to content
Open
Show file tree
Hide file tree
Changes from all 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 | null | undefined): Opportunity;
setScopeType(scopeType: 'brand' | null | undefined): 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,91 @@ 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 (hasText(scopeType) !== hasText(scopeId)) {
throw new ValidationError('scopeType and scopeId must both be set or both be absent', this);
}
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.

}

/**
* Validates and bulk-creates Opportunities. Enforces the scopeType/scopeId
* co-presence invariant on every item — half-scoped records are invalid.
*
* Overrides BaseCollection.createMany() which would otherwise bypass the
* single-item create() guard and persist invalid scope tuples directly.
*
* @param {object[]} items - The opportunity items to create.
* @param {...*} rest - Additional arguments forwarded to BaseCollection.createMany.
* @returns {Promise<*>} The result from BaseCollection.createMany.
*/
async createMany(items, ...rest) {
if (Array.isArray(items)) {
for (const item of items) {
const { scopeType, scopeId } = item || {};
if (hasText(scopeType) !== hasText(scopeId)) {
throw new ValidationError('scopeType and scopeId must both be set or both be absent', this);
}
}
}
return super.createMany(items, ...rest);
}

/**
* Validates and updates an Opportunity by its keys. Enforces the scopeType/scopeId
* co-presence invariant: if either field appears in the update payload, both must
* be set or both must be absent — a half-scoped update is invalid.
*
* Note: this guard enforces co-presence but allows scope re-attribution
* (moving an opportunity from one scopeId to another within the same scopeType).
* scopeId is the tenant boundary; callers must verify authorization before mutating it.
*
* @param {object} keys - The key attributes identifying the record.
* @param {object} updates - The fields to update.
* @returns {Promise<void>}
*/
async updateByKeys(keys, updates) {
const hasScopeType = updates != null && 'scopeType' in updates;
const hasScopeId = updates != null && 'scopeId' in updates;
// If either field is included in the update, both must be included together,
// and their combined values must satisfy co-presence (both set or both absent).
if (hasScopeType !== hasScopeId) {
throw new ValidationError('scopeType and scopeId must both be set or both be absent', this);
}
if (hasScopeType && hasScopeId) {
const { scopeType, scopeId } = updates;
if (hasText(scopeType) !== hasText(scopeId)) {
throw new ValidationError('scopeType and scopeId must both be set or both be absent', this);
}
}
return super.updateByKeys(keys, updates);
}

/**
* 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 @@ -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 BaseModel from '../base/base.model.js';

/**
Expand Down Expand Up @@ -37,6 +40,27 @@ class Opportunity extends BaseModel {
RESOLVED: 'RESOLVED',
};

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

/**
* Overrides BaseModel.save() to enforce the co-presence invariant: scopeType and scopeId
* must both be set or both be absent. Guards the setter+save path in addition to create().
*
* @returns {Promise<Opportunity>} The saved opportunity.
* @throws {ValidationError} When only one of scopeType / scopeId is set.
*/
async save() {
if (hasText(this.getScopeType()) !== hasText(this.getScopeId())) {
throw new ValidationError(
'scopeType and scopeId must both be set or both be absent',
this,
);
}
return super.save();
}

/**
* 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 @@ -383,6 +383,74 @@ 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',
};

let oppA;
let oppB;

after(async () => {
await Promise.all([oppA?.remove(), oppB?.remove()].filter(Boolean));
});

it('returns only opportunities matching the given scope', async () => {
// Create one opp for brand-a and one for brand-b on the same site
oppA = await Opportunity.create({
...scopedOpportunityBase,
title: 'Brand A Opportunity',
scopeType: 'brand',
scopeId: BRAND_A_ID,
});
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,20 @@ 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:-v5.1.1}
# Default tag is bumped here whenever a new schema migration requires a matching data-service
# image. The tag is supplemented by an immutable digest so CI cannot silently consume a
# re-pushed tag — if ECR re-publishes v5.1.1 with a different binary, the digest mismatch
# causes a hard pull failure instead of silent drift.
#
# To obtain or refresh the digest:
# aws ecr describe-images \
# --profile <your-profile> --region us-east-1 \
# --repository-name mysticat-data-service \
# --image-ids imageTag=v5.1.1 \
# --query 'imageDetails[0].imageDigest' --output text
# Then export MYSTICAT_DATA_SERVICE_DIGEST=sha256:<the-digest> before npm run test:it,
# or set it in the .env file consumed by docker compose. Leave unset for local dev.
image: ${MYSTICAT_DATA_SERVICE_REPOSITORY:-682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service}:${MYSTICAT_DATA_SERVICE_TAG:-v5.1.1}${MYSTICAT_DATA_SERVICE_DIGEST:+@${MYSTICAT_DATA_SERVICE_DIGEST}}
depends_on:
db:
condition: service_healthy
Expand Down
Loading
Loading