Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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 @@ -35,6 +35,10 @@ export interface Opportunity extends BaseModel {
getTags(): string[];
getTitle(): string;
getType(): string;
getScopeId(): string | null;
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 - TypeScript return types disagree with runtime behavior.

getScopeId(): string | null and getScopeType(): string | null declare null for the absent case, but the unit tests assert to.be.undefined. Callers checking === null get a false negative on the unset case. Auth-path code relying on this falls through silently.

Either normalize the getters to return null (preferred for JSON/DB API consistency), or change the TS declarations to string | undefined. Pick one and align code and types.

getScopeType(): string | null;
setScopeId(scopeId: string): Opportunity;
setScopeType(scopeType: string): Opportunity;
setAuditId(auditId: string): Opportunity;
setData(data: object): Opportunity;
setDescription(description: string): Opportunity;
Expand All @@ -49,6 +53,7 @@ export interface Opportunity extends BaseModel {
}

export interface OpportunityCollection extends BaseCollection<Opportunity> {
allByScopeId(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,8 @@
* governing permissions and limitations under the License.
*/

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

import BaseCollection from '../base/base.collection.js';

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

// add custom methods here
/**
* Returns all opportunities with a given scope type and scope ID.
* Used to fetch brand-scoped opportunities directly by brandId
* without going through site association.
*
* @param {string} scopeType - The scope type (e.g. 'brand', 'site').
* @param {string} scopeId - The scope entity UUID (e.g. brand UUID).
* @returns {Promise<Opportunity[]>} The matching opportunities.
*/
async allByScopeId(scopeType, 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.

Important - method name is misleading, and two issues around validation.

Rename allByScopeId. The method requires both scopeType and scopeId - it is not a lookup by ID alone. allByScopeId('brand', uuid) reads as if 'brand' is a filter on an ID-based lookup, and the positional signature makes arg-flip easy. Rename to allByScope(scopeType, scopeId) to match the existing allByAuditIdAndUpdatedAt convention. Update index.d.ts:56 in the same change.

scopeType is not validated against the enum here. hasText only checks presence. A caller passing 'BRAND' or 'admin' passes through into allByIndexKeys. Add if (!Object.values(Opportunity.SCOPE_TYPES).includes(scopeType)) throw new Error(...) as a second line of defense, consistent with how the schema handles invalid values at write time.

setScopeType setter fires enum validation only at save() time. The auto-generated setter writes unchecked to the record. Narrow the TS type to 'site' | 'brand' or add an explicit setter override that validates immediately.

if (!hasText(scopeType)) {
throw new Error('scopeType is required');
}
if (!hasText(scopeId)) {
throw new Error('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,11 @@ class Opportunity extends BaseModel {
RESOLVED: 'RESOLVED',
};

static SCOPE_TYPES = {
SITE: 'site',
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 @@ -64,6 +64,12 @@ 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: Object.values(Opportunity.SCOPE_TYPES),
})
.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',
});

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 @@ -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,35 @@ describe('OpportunityCollection', () => {
expect(model).to.be.an('object');
});
});

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

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

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

const result = await instance.allByScopeId('brand', 'brand-uuid-123');

expect(instance.allByIndexKeys).to.have.been.calledOnceWith({
scopeType: 'brand',
scopeId: 'brand-uuid-123',
});
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.allByScopeId('brand', 'brand-uuid-no-results');

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.SITE).to.equal('site');
expect(Opportunity.SCOPE_TYPES.BRAND).to.equal('brand');
});
});
});
Loading