Skip to content
Open
Show file tree
Hide file tree
Changes from 9 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,64 @@ 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 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.
*
* @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 @@ -382,6 +382,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,9 @@ 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}
# Default tag is bumped here whenever a new schema migration requires a matching data-service
# image. Override at runtime: export MYSTICAT_DATA_SERVICE_TAG=v<version> before npm run test:it.
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
Loading
Loading