From 2e53ac8c91d6d13cdd4492d6341ff2206fe8dc12 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Tue, 28 Jul 2026 16:37:42 -0400 Subject: [PATCH 1/5] update segment service methods for typeorm 1.0 cascading behavior --- .../src/api/services/SegmentService.ts | 142 +++++++++++------- .../test/unit/services/SegmentService.test.ts | 114 ++++---------- 2 files changed, 121 insertions(+), 135 deletions(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index bebcb74e0..cd474a15b 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -17,8 +17,7 @@ import { EXPERIMENT_STATE_DISPLAY_NAME_OVERRIDES, EXPERIMENT_STATE, } from 'upgrade_types'; -import { Not } from 'typeorm'; -import { EntityManager, DataSource } from 'typeorm'; +import { EntityManager, DataSource, Not, In } from 'typeorm'; import Papa from 'papaparse'; import { env } from '../../env'; @@ -432,13 +431,27 @@ export class SegmentService { const newList: SegmentInputValidator = { ...segmentInput, type: SEGMENT_TYPE.PRIVATE }; const createdSegment = await manager.transaction(async (transactionalEntityManager) => { const createdSegment = await this.upsertSegmentInPipeline(newList, logger, transactionalEntityManager); - const parentSegment = await this.getSegmentById(parentSegmentId, logger); + const segmentRepo = transactionalEntityManager.getRepository(Segment); + + // Load only ID to avoid loading full relationships (which causes cascade issues) + const parentSegment = await segmentRepo.findOne({ + where: { id: parentSegmentId }, + select: { id: true, tags: true }, + }); + if (!parentSegment) { throw new Error('Parent Segment not found'); } - parentSegment.tags = parentSegment.tags || []; - parentSegment.subSegments = [...parentSegment.subSegments, createdSegment]; - await transactionalEntityManager.getRepository(Segment).save(parentSegment); + + // Update tags if needed + if (!parentSegment.tags) { + parentSegment.tags = []; + await segmentRepo.save(parentSegment); + } + + // Use relation API to avoid cascade behavior from loaded entities + await segmentRepo.createQueryBuilder().relation('subSegments').of(parentSegment).add(createdSegment); + return createdSegment; }); @@ -449,24 +462,20 @@ export class SegmentService { public async deleteList(segmentId: string, parentSegmentId: string, logger: UpgradeLogger): Promise { logger.info({ message: `Deleting list => ${segmentId} from segment ${parentSegmentId}` }); + + const parentSegment = await this.getSegmentById(parentSegmentId, logger); + if (!parentSegment || !parentSegment.subSegments.some((subSegment) => subSegment.id === segmentId)) { + throw new Error(`List ${segmentId} not found in parent segment ${parentSegmentId}`); + } + const manager = this.dataSource; const deletedSegmentResponse = await manager.transaction(async (transactionalEntityManager) => { - const parentSegment = await this.getSegmentById(parentSegmentId, logger); - if (!parentSegment) { - throw new Error('Parent Segment not found'); - } - if (!parentSegment.subSegments.map((subSegment) => subSegment.id).includes(segmentId)) { - throw new Error(`List ${segmentId} not found in parent segment ${parentSegmentId}`); - } const deletedSegmentResponse = await this.segmentRepository.deleteSegments( [segmentId], logger, transactionalEntityManager ); - parentSegment.subSegments = parentSegment.subSegments.filter((subSegment) => subSegment.id !== segmentId); - - await transactionalEntityManager.getRepository(Segment).save(parentSegment); return deletedSegmentResponse; }); @@ -919,13 +928,21 @@ export class SegmentService { skipScheduleRecompute = false ): Promise { let segmentDoc: Segment; + const segmentRepo = transactionalEntityManager.getRepository(Segment); if (segment.id) { try { - // Full replace: clear members with a single delete-by-segmentId per member table. A per-row - // criteria array (the previous approach) expands into a giant OR predicate that is very slow - // for large lists. The delete is cheap even when there are no members, so we skip the - // pre-SELECT that used to load the full member arrays just to decide whether to delete. + // Full replace: clear members and relationships with a single delete-by-segmentId per table. + // A per-row criteria array (the previous approach) expands into a giant OR predicate that + // is very slow for large lists. The delete is cheap even when there are no members, so we + // skip the pre-SELECT that used to load the full member arrays just to decide whether to delete. + + // Load old subsegments to remove them + const oldSegment = await segmentRepo.findOne({ + where: { id: segment.id }, + relations: { subSegments: true }, + }); + await Promise.all([ this.individualForSegmentRepository.deleteIndividualForSegmentById( segment.id, @@ -933,6 +950,14 @@ export class SegmentService { logger ), this.groupForSegmentRepository.deleteGroupForSegmentById(segment.id, transactionalEntityManager, logger), + // Remove old subsegment relationships + oldSegment && oldSegment.subSegments.length > 0 + ? segmentRepo + .createQueryBuilder() + .relation('subSegments') + .of({ id: segment.id }) + .remove(oldSegment.subSegments) + : Promise.resolve(), ]); } catch (err) { const error = err as ErrorWithType; @@ -946,46 +971,46 @@ export class SegmentService { // create/update segment document segment.id = segment.id || crypto.randomUUID(); const { id, name, description, context, type, listType, tags } = segment; - const segmentsById = await this.getSegmentByIds(segment.subSegmentIds || []); - const allSegments = [...segmentsById, ...(segment.subSegments || [])]; - // If the segment is public and there are private subsegments, they are lists - so we need to clone the data - const isListData = - type === SEGMENT_TYPE.PUBLIC && allSegments.some((subSegment) => subSegment.type === SEGMENT_TYPE.PRIVATE); - let subSegmentData; - if (isListData) { + + let subSegmentData: any[] = []; + + // For non-list segments with subSegmentIds, just verify they exist without loading full relations + if (type === SEGMENT_TYPE.PUBLIC && segment.subSegments?.some((sub) => sub.type === SEGMENT_TYPE.PRIVATE)) { + // Public segment with embedded private subsegments (lists) - recursively create them subSegmentData = await Promise.all( - allSegments.map(async (subSegment) => { + segment.subSegments.map(async (subSegment) => { // Create a new segment input object for the list const segmentInput = subSegment as unknown as SegmentInputValidator; - segmentInput.userIds = subSegment.individualForSegment.map((user) => user.userId); - segmentInput.groups = subSegment.groupForSegment.map((group) => { - return { type: group.type, groupId: group.groupId }; - }); - segmentInput.subSegmentIds = subSegment.subSegments.map((subSegment) => subSegment.id); - subSegment.id = undefined; + segmentInput.userIds = subSegment.individualForSegment?.map((user) => user.userId) || []; + segmentInput.groups = + subSegment.groupForSegment?.map((group) => { + return { type: group.type, groupId: group.groupId }; + }) || []; + segmentInput.subSegmentIds = subSegment.subSegments?.map((subSegment) => subSegment.id) || []; return await this.addSegmentDataWithPipeline(segmentInput, logger, transactionalEntityManager); }) ); - } else { - subSegmentData = - segment.subSegmentIds - ?.map((subSegmentId) => { - const subSegment = allSegments.find((segment) => subSegmentId === segment.id); - if (subSegment) { - return subSegment; - } else { - const error = new Error( - 'SubSegment: ' + subSegmentId + ' not found. Please import subSegment and link in experiment.' - ); - (error as any).type = SERVER_ERROR.QUERY_FAILED; - logger.error(error); - return null; - } - }) - ?.filter((subSegment) => subSegment !== null) || []; // filter out null values + } else if (segment.subSegmentIds && segment.subSegmentIds.length > 0) { + // Just adding references to existing segments - verify existence without loading full relations + const existingIds = await segmentRepo.find({ + where: { id: In(segment.subSegmentIds) }, + select: { id: true }, + }); + + const existingIdSet = new Set(existingIds.map((s) => s.id)); + + for (const subSegmentId of segment.subSegmentIds) { + if (!existingIdSet.has(subSegmentId)) { + // Skip unknown subsegment references silently — validation already warned about these + logger.warn({ message: `SubSegment: ${subSegmentId} not found, skipping reference.` }); + continue; + } + subSegmentData.push({ id: subSegmentId }); + } } + try { - segmentDoc = await transactionalEntityManager.getRepository(Segment).save({ + segmentDoc = await segmentRepo.save({ id, name, description, @@ -993,8 +1018,17 @@ export class SegmentService { type, listType, tags, - subSegments: subSegmentData, + // Don't include subSegments in save to avoid cascade updates }); + + // Manually set the relationship without cascading updates to child segments + // This prevents TypeORM from overwriting the child segments' own subSegments relationships + if (subSegmentData && subSegmentData.length > 0) { + // Only reference by ID, don't pass full objects to avoid cascade behavior + for (const subSegment of subSegmentData) { + await segmentRepo.createQueryBuilder().relation('subSegments').of(segmentDoc).add({ id: subSegment.id }); + } + } } catch (err) { const error = err as ErrorWithType; error.details = 'Error in saving segment in DB'; @@ -1054,7 +1088,7 @@ export class SegmentService { this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); } - return transactionalEntityManager.getRepository(Segment).findOne({ + return segmentRepo.findOne({ where: { id: segmentDoc.id }, relations: { subSegments: true, individualForSegment: true, groupForSegment: true }, }); diff --git a/packages/backend/test/unit/services/SegmentService.test.ts b/packages/backend/test/unit/services/SegmentService.test.ts index d40e0a1da..3754b7b1f 100644 --- a/packages/backend/test/unit/services/SegmentService.test.ts +++ b/packages/backend/test/unit/services/SegmentService.test.ts @@ -235,6 +235,10 @@ describe('Segment Service Testing', () => { }), createQueryBuilder: jest.fn(() => ({ insert: jest.fn().mockReturnThis(), + relation: jest.fn().mockReturnThis(), + of: jest.fn().mockReturnThis(), + remove: jest.fn().mockReturnThis(), + add: jest.fn().mockReturnThis(), leftJoinAndSelect: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), andWhere: jest.fn().mockReturnThis(), @@ -1011,53 +1015,31 @@ describe('Segment Service Testing', () => { }); it('should ignore private subsegments that are not in subSegmentIds for private segments', async () => { - // Create a private segment (like an exclusion/inclusion list) + // Create a private segment with subSegmentIds const privateSegment = new SegmentInputValidator(); privateSegment.id = 'private-segment-id'; privateSegment.name = 'private-segment'; privateSegment.type = SEGMENT_TYPE.PRIVATE; privateSegment.context = 'add'; - privateSegment.subSegmentIds = ['allowed-subsegment-id']; // Only this one should be processed + privateSegment.subSegmentIds = ['allowed-subsegment-id']; privateSegment.userIds = []; privateSegment.groups = []; + privateSegment.subSegments = []; - // Create subsegments - some in subSegmentIds, some not - const allowedSubsegment = new Segment(); - allowedSubsegment.id = 'allowed-subsegment-id'; - allowedSubsegment.name = 'allowed-subsegment'; - allowedSubsegment.type = SEGMENT_TYPE.PUBLIC; - - const ignoredSubsegment = new Segment(); - ignoredSubsegment.id = 'ignored-subsegment-id'; - ignoredSubsegment.name = 'ignored-subsegment'; - ignoredSubsegment.type = SEGMENT_TYPE.PRIVATE; - - // Add both to subSegments array, but only the allowed one to subSegmentIds - privateSegment.subSegments = [allowedSubsegment, ignoredSubsegment]; - - // Mock service methods - getSegmentByIds should return what it finds based on subSegmentIds service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); - service.getSegmentByIds = jest.fn().mockImplementation((ids) => { - // Simulate finding segments by the requested IDs - const allAvailableSegments = [allowedSubsegment, ignoredSubsegment]; - return Promise.resolve(allAvailableSegments.filter((seg) => ids.includes(seg.id))); - }); - // Mock repository save to capture what gets passed to it - repo.save = jest.fn().mockResolvedValue({ - id: privateSegment.id, - name: privateSegment.name, - type: privateSegment.type, - context: privateSegment.context, - subSegments: [], - }); + // repo.find returns only the allowed subsegment + repo.find = jest.fn().mockResolvedValue([{ id: 'allowed-subsegment-id' }]); + repo.save = jest.fn().mockResolvedValue({ id: privateSegment.id }); await service.upsertSegment(privateSegment, logger); - // Verify that getSegmentByIds was called with only the IDs from subSegmentIds - expect(service.getSegmentByIds).toHaveBeenCalledWith(['allowed-subsegment-id']); + // Verify repo.find was called to check subsegment existence + expect(repo.find).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ id: expect.anything() }) }) + ); - // Verify that repo.save was called with only the subsegment found via subSegmentIds + // Verify repo.save was called WITHOUT subSegments (relations are added via QueryBuilder) expect(repo.save).toHaveBeenCalledWith({ id: privateSegment.id, name: privateSegment.name, @@ -1066,7 +1048,6 @@ describe('Segment Service Testing', () => { type: privateSegment.type, listType: undefined, tags: undefined, - subSegments: [allowedSubsegment], // Only the one from subSegmentIds, not the ignored one }); }); @@ -1082,37 +1063,22 @@ describe('Segment Service Testing', () => { privateSegment.groups = []; privateSegment.subSegments = []; - // Create valid subsegments - const validSubsegment1 = new Segment(); - validSubsegment1.id = 'valid-subsegment-1'; - validSubsegment1.name = 'valid-subsegment-1'; - validSubsegment1.type = SEGMENT_TYPE.PUBLIC; - - const validSubsegment2 = new Segment(); - validSubsegment2.id = 'valid-subsegment-2'; - validSubsegment2.name = 'valid-subsegment-2'; - validSubsegment2.type = SEGMENT_TYPE.PUBLIC; - // Mock service methods service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); - service.getSegmentByIds = jest.fn().mockResolvedValue([validSubsegment1, validSubsegment2]); - // Mock repository save - const savedSegment = { - id: privateSegment.id, - name: privateSegment.name, - type: privateSegment.type, - context: privateSegment.context, - subSegments: [validSubsegment1, validSubsegment2], - }; - repo.save = jest.fn().mockResolvedValue(savedSegment); + // repo.find is called via segmentRepo.find in the transactional entity manager + repo.find = jest.fn().mockResolvedValue([{ id: 'valid-subsegment-1' }, { id: 'valid-subsegment-2' }]); + + repo.save = jest.fn().mockResolvedValue({ id: privateSegment.id }); await service.upsertSegment(privateSegment, logger); - // Verify that getSegmentByIds was called with the correct IDs - expect(service.getSegmentByIds).toHaveBeenCalledWith(['valid-subsegment-1', 'valid-subsegment-2']); + // Verify that repo.find was called to check subsegment existence + expect(repo.find).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ id: expect.anything() }) }) + ); - // Verify that repo.save was called with both valid subsegments + // Verify that repo.save was called WITHOUT subSegments (relations are added via QueryBuilder) expect(repo.save).toHaveBeenCalledWith({ id: privateSegment.id, name: privateSegment.name, @@ -1121,12 +1087,11 @@ describe('Segment Service Testing', () => { type: privateSegment.type, listType: undefined, tags: undefined, - subSegments: [validSubsegment1, validSubsegment2], }); }); it('should handle missing subsegments in subSegmentIds gracefully', async () => { - // Create a private segment with subSegmentIds that don't exist + // Create a private segment with subSegmentIds where one doesn't exist const privateSegment = new SegmentInputValidator(); privateSegment.id = 'private-segment-id'; privateSegment.name = 'private-segment'; @@ -1137,32 +1102,20 @@ describe('Segment Service Testing', () => { privateSegment.groups = []; privateSegment.subSegments = []; - // Only one subsegment exists - const existingSubsegment = new Segment(); - existingSubsegment.id = 'existing-subsegment'; - existingSubsegment.name = 'existing-subsegment'; - existingSubsegment.type = SEGMENT_TYPE.PUBLIC; - - // Mock service methods - getSegmentByIds returns only the existing one service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); - service.getSegmentByIds = jest.fn().mockResolvedValue([existingSubsegment]); - // Mock repository save - const savedSegment = { - id: privateSegment.id, - name: privateSegment.name, - type: privateSegment.type, - context: privateSegment.context, - subSegments: [existingSubsegment], - }; - repo.save = jest.fn().mockResolvedValue(savedSegment); + // repo.find returns only the existing subsegment — missing one is silently skipped + repo.find = jest.fn().mockResolvedValue([{ id: 'existing-subsegment' }]); + repo.save = jest.fn().mockResolvedValue({ id: privateSegment.id }); await service.upsertSegment(privateSegment, logger); - // Verify that getSegmentByIds was called with all requested IDs - expect(service.getSegmentByIds).toHaveBeenCalledWith(['missing-subsegment-1', 'existing-subsegment']); + // Verify repo.find was called to check subsegment existence + expect(repo.find).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ id: expect.anything() }) }) + ); - // Verify that repo.save was called with only the existing subsegment + // Verify repo.save was called WITHOUT subSegments (relations are added via QueryBuilder) expect(repo.save).toHaveBeenCalledWith({ id: privateSegment.id, name: privateSegment.name, @@ -1171,7 +1124,6 @@ describe('Segment Service Testing', () => { type: privateSegment.type, listType: undefined, tags: undefined, - subSegments: [existingSubsegment], }); }); }); From 1569654275386f8f71be67433fc96a2c0bb3818d Mon Sep 17 00:00:00 2001 From: Ben Blanchard Date: Tue, 28 Jul 2026 16:53:28 -0400 Subject: [PATCH 2/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/backend/src/api/services/SegmentService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index cd474a15b..5e056c34b 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -433,7 +433,7 @@ export class SegmentService { const createdSegment = await this.upsertSegmentInPipeline(newList, logger, transactionalEntityManager); const segmentRepo = transactionalEntityManager.getRepository(Segment); - // Load only ID to avoid loading full relationships (which causes cascade issues) + // Load only the ID (+ tags) and avoid loading relationships (which can trigger cascade issues) const parentSegment = await segmentRepo.findOne({ where: { id: parentSegmentId }, select: { id: true, tags: true }, From 13153ba445979c0130c33b6d153e47f44566fff4 Mon Sep 17 00:00:00 2001 From: Ben Blanchard Date: Tue, 28 Jul 2026 16:54:27 -0400 Subject: [PATCH 3/5] more precise type Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/backend/src/api/services/SegmentService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 5e056c34b..cce85d229 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -972,7 +972,7 @@ export class SegmentService { segment.id = segment.id || crypto.randomUUID(); const { id, name, description, context, type, listType, tags } = segment; - let subSegmentData: any[] = []; + let subSegmentData: Array> = []; // For non-list segments with subSegmentIds, just verify they exist without loading full relations if (type === SEGMENT_TYPE.PUBLIC && segment.subSegments?.some((sub) => sub.type === SEGMENT_TYPE.PRIVATE)) { From 6d0e591a4540b74a65987aa4beb2da8ec7acd89d Mon Sep 17 00:00:00 2001 From: Ben Blanchard Date: Tue, 28 Jul 2026 16:54:55 -0400 Subject: [PATCH 4/5] add relations in one call Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/backend/src/api/services/SegmentService.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index cce85d229..cf06066ca 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -1023,11 +1023,12 @@ export class SegmentService { // Manually set the relationship without cascading updates to child segments // This prevents TypeORM from overwriting the child segments' own subSegments relationships - if (subSegmentData && subSegmentData.length > 0) { - // Only reference by ID, don't pass full objects to avoid cascade behavior - for (const subSegment of subSegmentData) { - await segmentRepo.createQueryBuilder().relation('subSegments').of(segmentDoc).add({ id: subSegment.id }); - } + if (subSegmentData.length > 0) { + await segmentRepo + .createQueryBuilder() + .relation('subSegments') + .of(segmentDoc) + .add(subSegmentData.map((s) => ({ id: s.id }))); } } catch (err) { const error = err as ErrorWithType; From da3f8989ba31d617190215dd465b8a7c019584e9 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Tue, 28 Jul 2026 17:34:23 -0400 Subject: [PATCH 5/5] create new ids for cloned private segments --- packages/backend/src/api/services/SegmentService.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index cf06066ca..87b206510 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -979,8 +979,9 @@ export class SegmentService { // Public segment with embedded private subsegments (lists) - recursively create them subSegmentData = await Promise.all( segment.subSegments.map(async (subSegment) => { - // Create a new segment input object for the list + // Create a new segment input object for the list (cloning, not updating) const segmentInput = subSegment as unknown as SegmentInputValidator; + segmentInput.id = undefined; // Clear id to create new clone instead of updating existing segment segmentInput.userIds = subSegment.individualForSegment?.map((user) => user.userId) || []; segmentInput.groups = subSegment.groupForSegment?.map((group) => {