Skip to content

Commit cd5d094

Browse files
ekremneyclaude
andcommitted
fix(site-ims-org-access): address review feedback on collection
- Remove orphaned JSDoc block before #fetchGrantsWithTargetOrg (copy-paste artifact from refactoring) - Move .order('id') outside pagination loop to prevent accumulating duplicate ORDER clauses on each iteration - Guard allByOrganizationIdsWithTargetOrganization against oversized arrays (>50 UUIDs) that would produce opaque 414 errors from PostgREST Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 3453d4c commit cd5d094

File tree

2 files changed

+18
-17
lines changed

2 files changed

+18
-17
lines changed

packages/spacecat-shared-data-access/src/models/site-ims-org-access/site-ims-org-access.collection.js

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -78,22 +78,6 @@ class SiteImsOrgAccessCollection extends BaseCollection {
7878
return created;
7979
}
8080

81-
/**
82-
* Returns all grants for the given delegate organization with the target organization's
83-
* id and imsOrgId embedded via PostgREST resource embedding (INNER JOIN). This avoids
84-
* a separate batch query to resolve target org IMS identifiers.
85-
*
86-
* Returns plain objects, not model instances. Access properties directly
87-
* (e.g., `entry.grant.productCode`, `entry.targetOrganization.imsOrgId`).
88-
*
89-
* @param {string} organizationId - UUID of the delegate organization.
90-
* @returns {Promise<Array<{
91-
* grant: {id: string, siteId: string, organizationId: string,
92-
* targetOrganizationId: string, productCode: string, role: string,
93-
* grantedBy: string|null, expiresAt: string|null},
94-
* targetOrganization: {id: string, imsOrgId: string}
95-
* }>>}
96-
*/
9781
/**
9882
* @param {object} query - PostgREST query builder (result of .from(...).select(...))
9983
* @returns {Promise<Array<{grant: object, targetOrganization: object}>>}
@@ -103,10 +87,11 @@ class SiteImsOrgAccessCollection extends BaseCollection {
10387
const allResults = [];
10488
let offset = 0;
10589
let keepGoing = true;
90+
const orderedQuery = query.order('id');
10691

10792
while (keepGoing) {
10893
// eslint-disable-next-line no-await-in-loop
109-
const { data, error } = await query.order('id').range(offset, offset + DEFAULT_PAGE_SIZE - 1);
94+
const { data, error } = await orderedQuery.range(offset, offset + DEFAULT_PAGE_SIZE - 1);
11095

11196
if (error) {
11297
this.log.error(`[SiteImsOrgAccess] Failed to query grants with target org - ${error.message}`, error);
@@ -188,6 +173,12 @@ class SiteImsOrgAccessCollection extends BaseCollection {
188173
if (!organizationIds || organizationIds.length === 0) {
189174
return [];
190175
}
176+
if (organizationIds.length > SiteImsOrgAccessCollection.MAX_DELEGATES_PER_SITE) {
177+
throw new DataAccessError(
178+
`allByOrganizationIdsWithTargetOrganization: organizationIds array exceeds maximum of ${SiteImsOrgAccessCollection.MAX_DELEGATES_PER_SITE}`,
179+
{ entityName: 'SiteImsOrgAccess', tableName: 'site_ims_org_accesses' },
180+
);
181+
}
191182
// eslint-disable-next-line max-len
192183
const select = 'id, site_id, organization_id, target_organization_id, product_code, role, granted_by, expires_at, organizations!site_ims_org_accesses_target_organization_id_fkey(id, ims_org_id)';
193184
return this.#fetchGrantsWithTargetOrg(

packages/spacecat-shared-data-access/test/unit/models/site-ims-org-access/site-ims-org-access.collection.test.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,5 +381,15 @@ describe('SiteImsOrgAccessCollection', () => {
381381
.to.be.rejectedWith(DataAccessError, 'Failed to query grants with target organization');
382382
expect(mockLogger.error).to.have.been.called;
383383
});
384+
385+
it('throws DataAccessError when organizationIds array exceeds the maximum size', async () => {
386+
const tooManyIds = Array.from(
387+
{ length: SiteImsOrgAccessCollection.MAX_DELEGATES_PER_SITE + 1 },
388+
(_, i) => `org-uuid-${i}`,
389+
);
390+
391+
await expect(instance.allByOrganizationIdsWithTargetOrganization(tooManyIds))
392+
.to.be.rejectedWith(DataAccessError, 'organizationIds array exceeds maximum');
393+
});
384394
});
385395
});

0 commit comments

Comments
 (0)