feat: Add normalization after create, update, delete

packages/cli/src/modules/provisioning.ee/__tests__/role-mapping-rule.service.ee.test.ts

@@ -55,6 +55,9 @@ describe('RoleMappingRuleService', () => {
 	beforeEach(() => {
 		jest.clearAllMocks();
 		roleMappingRuleRepository.findOne.mockResolvedValue(null);
+		// normalizeOrderForType calls find after every mutation; default to empty
+		// so existing tests hit the early-exit path and require no transaction mock.
+		roleMappingRuleRepository.find.mockResolvedValue([]);
 	});
 
 	describe('create', () => {
@@ -504,4 +507,138 @@ describe('RoleMappingRuleService', () => {
 			expect(roleMappingRuleRepository.remove).toHaveBeenCalledWith(rule);
 		});
 	});
+
+	describe('normalizeOrderForType', () => {
+		const makeRule = (id: string, order: number, type = 'instance') =>
+			({ id, order, type }) as unknown as RoleMappingRule;
+
+		const updateSpy = jest.fn().mockResolvedValue(undefined);
+		const transactionSpy = jest.fn();
+
+		beforeEach(() => {
+			updateSpy.mockClear();
+			transactionSpy.mockImplementation(
+				async (cb: (tx: { update: typeof updateSpy }) => Promise<void>) => {
+					await cb({ update: updateSpy });
+				},
+			);
+			// jest-mock-extended creates a Proxy; assigning manager directly
+			// is the reliable way to inject the transaction mock.
+			(roleMappingRuleRepository as unknown as Record<string, unknown>).manager = {
+				transaction: transactionSpy,
+			};
+		});
+
+		it('should not call transaction when sequence has no gaps', async () => {
+			roleMappingRuleRepository.find.mockResolvedValue([
+				makeRule('a', 0),
+				makeRule('b', 1),
+				makeRule('c', 2),
+			]);
+
+			roleMappingRuleRepository.findOne.mockResolvedValue(makeRule('a', 0));
+			roleMappingRuleRepository.remove.mockResolvedValue(makeRule('a', 0));
+
+			await service.delete('a');
+
+			expect(transactionSpy).not.toHaveBeenCalled();
+		});
+
+		it('should renumber rules to close a gap after delete', async () => {
+			// Simulates [0, 2, 3] after deleting the rule at order 1
+			roleMappingRuleRepository.find.mockResolvedValue([
+				makeRule('a', 0),
+				makeRule('b', 2),
+				makeRule('c', 3),
+			]);
+
+			roleMappingRuleRepository.findOne.mockResolvedValue(makeRule('x', 0));
+			roleMappingRuleRepository.remove.mockResolvedValue(makeRule('x', 0));
+
+			await service.delete('x');
+
+			expect(transactionSpy).toHaveBeenCalledTimes(1);
+
+			// Phase 2 should assign contiguous orders 0, 1, 2
+			expect(updateSpy).toHaveBeenCalledWith(expect.anything(), { id: 'a' }, { order: 0 });
+			expect(updateSpy).toHaveBeenCalledWith(expect.anything(), { id: 'b' }, { order: 1 });
+			expect(updateSpy).toHaveBeenCalledWith(expect.anything(), { id: 'c' }, { order: 2 });
+		});
+
+		it('should compact a large gap after create', async () => {
+			// Simulates [0, 1, 100] after creating a rule at order 100
+			roleMappingRuleRepository.find.mockResolvedValue([
+				makeRule('a', 0),
+				makeRule('b', 1),
+				makeRule('c', 100),
+			]);
+
+			roleRepository.findOne.mockResolvedValue(globalRole);
+			roleMappingRuleRepository.save.mockResolvedValue(makeRule('c', 100));
+			roleMappingRuleRepository.findOneOrFail.mockResolvedValue({
+				...makeRule('c', 2),
+				role: globalRole,
+				projects: [],
+				createdAt: new Date('2025-01-01T00:00:00.000Z'),
+				updatedAt: new Date('2025-01-01T00:00:00.000Z'),
+			} as unknown as RoleMappingRule);
+
+			await service.create({
+				expression: 'true',
+				role: globalRole.slug,
+				type: 'instance',
+				order: 100,
+			});
+
+			expect(transactionSpy).toHaveBeenCalledTimes(1);
+			expect(updateSpy).toHaveBeenCalledWith(expect.anything(), { id: 'c' }, { order: 2 });
+		});
+
+		it('should normalize both types when type changes during patch', async () => {
+			const existingRule = {
+				id: 'rule-1',
+				expression: 'true',
+				role: globalRole,
+				type: 'instance',
+				order: 1,
+				projects: [],
+			} as unknown as RoleMappingRule;
+
+			roleMappingRuleRepository.findOne.mockImplementation(async (opts) => {
+				if (opts?.where && 'id' in opts.where) return existingRule;
+				return null;
+			});
+			roleMappingRuleRepository.save.mockResolvedValue(existingRule);
+			roleMappingRuleRepository.findOneOrFail.mockResolvedValue({
+				...existingRule,
+				type: 'project',
+				role: projectRole,
+				projects: [{ id: 'p1' } as Project],
+				createdAt: new Date('2025-01-01T00:00:00.000Z'),
+				updatedAt: new Date('2025-01-01T00:00:00.000Z'),
+			} as unknown as RoleMappingRule);
+			projectRepository.findBy.mockResolvedValue([{ id: 'p1' } as Project]);
+			roleRepository.findOne.mockResolvedValue(projectRole);
+
+			roleMappingRuleRepository.find
+				.mockResolvedValueOnce([makeRule('rule-1', 0, 'project')]) // new type: project — no gap
+				.mockResolvedValueOnce([
+					makeRule('rule-2', 0, 'instance'),
+					makeRule('rule-3', 2, 'instance'),
+				]); // old type: instance has gap
+
+			await service.patch('rule-1', {
+				type: 'project',
+				role: projectRole.slug,
+				projectIds: ['p1'],
+				order: 0,
+			});
+
+			// Called twice: once for new type (project), once for old type (instance)
+			expect(roleMappingRuleRepository.find).toHaveBeenCalledTimes(2);
+			// Project sequence has no gap — no transaction needed for it
+			// Instance sequence has gap [0, 2] — transaction called once
+			expect(transactionSpy).toHaveBeenCalledTimes(1);
+		});
+	});
 });

packages/cli/src/modules/provisioning.ee/role-mapping-rule.service.ee.ts

@@ -110,6 +110,8 @@ export class RoleMappingRuleService {
 
 		const saved = await this.roleMappingRuleRepository.save(rule);
 
+		await this.normalizeOrderForType(dto.type);
+
 		const loaded = await this.roleMappingRuleRepository.findOneOrFail({
 			where: { id: saved.id },
 			relations: ['projects', 'role'],
@@ -136,7 +138,8 @@ export class RoleMappingRuleService {
 			throw new NotFoundError('Could not find role mapping rule');
 		}
 
-		const mergedType = dto.type ?? (rule.type as 'instance' | 'project');
+		const originalType = rule.type as 'instance' | 'project';
+		const mergedType = dto.type ?? originalType;
 		const mergedOrder = dto.order ?? rule.order;
 		const mergedExpression = dto.expression ?? rule.expression;
 		const mergedRoleSlug = dto.role ?? rule.role.slug;
@@ -178,6 +181,11 @@ export class RoleMappingRuleService {
 
 		await this.roleMappingRuleRepository.save(rule);
 
+		await this.normalizeOrderForType(mergedType);
+		if (originalType !== mergedType) {
+			await this.normalizeOrderForType(originalType);
+		}
+
 		const loaded = await this.roleMappingRuleRepository.findOneOrFail({
 			where: { id: rule.id },
 			relations: ['projects', 'role'],
@@ -197,7 +205,36 @@ export class RoleMappingRuleService {
 			throw new NotFoundError('Could not find role mapping rule');
 		}
 
+		const ruleType = rule.type as 'instance' | 'project';
 		await this.roleMappingRuleRepository.remove(rule);
+		await this.normalizeOrderForType(ruleType);
+	}
+
+	private async normalizeOrderForType(type: 'instance' | 'project'): Promise<void> {
+		const rules = await this.roleMappingRuleRepository.find({
+			where: { type },
+			select: ['id', 'order'],
+			order: { order: 'ASC' },
+		});
+
+		if (rules.length === 0) return;
+
+		// Early exit: already a contiguous sequence starting at 0
+		if (rules.every((r, i) => r.order === i)) return;
+
+		await this.roleMappingRuleRepository.manager.transaction(async (tx) => {
+			// Phase 1 — move all to a safe high offset to avoid unique constraint
+			// conflicts during resequencing (checked per-statement in SQLite/Postgres)
+			const offset = rules.length + 1000;
+			for (let i = 0; i < rules.length; i++) {
+				await tx.update(RoleMappingRule, { id: rules[i].id }, { order: offset + i });
+			}
+
+			// Phase 2 — assign final 0-based contiguous orders
+			for (let i = 0; i < rules.length; i++) {
+				await tx.update(RoleMappingRule, { id: rules[i].id }, { order: i });
+			}
+		});
 	}
 
 	private async assertOrderAvailable(

packages/cli/test/integration/role-mapping-rule.api.test.ts

@@ -137,6 +137,28 @@ describe('POST /role-mapping-rule', () => {
 		expect(response.body.message).toContain('order');
 	});
 
+	it('should normalize order when created with an abnormally high order value', async () => {
+		await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, order: 0 })
+			.expect(200);
+		await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, expression: 'claims.b', order: 1 })
+			.expect(200);
+
+		const response = await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, expression: 'claims.c', order: 100 })
+			.expect(200);
+
+		expect(response.body.data.order).toBe(2);
+
+		const repo = Container.get(RoleMappingRuleRepository);
+		const all = await repo.find({ where: { type: 'instance' }, order: { order: 'ASC' } });
+		expect(all.map((r) => r.order)).toEqual([0, 1, 2]);
+	});
+
 	it('should create a project mapping rule linked to team projects', async () => {
 		const teamProject = await createTeamProject(undefined, owner);
 
@@ -153,7 +175,7 @@ describe('POST /role-mapping-rule', () => {
 
 		expect(response.body.data).toMatchObject({
 			type: 'project',
-			order: 3,
+			order: 0,
 		});
 		expect(response.body.data.projectIds).toContain(teamProject.id);
 
@@ -204,19 +226,19 @@ describe('GET /role-mapping-rule', () => {
 		await ownerAgent
 			.post('/role-mapping-rule')
 			.send({
-				expression: 'claims.second',
+				expression: 'claims.first',
 				role: 'global:member',
 				type: 'instance',
-				order: 1,
+				order: 0,
 			})
 			.expect(200);
 		await ownerAgent
 			.post('/role-mapping-rule')
 			.send({
-				expression: 'claims.first',
+				expression: 'claims.second',
 				role: 'global:member',
 				type: 'instance',
-				order: 0,
+				order: 1,
 			})
 			.expect(200);
 
@@ -413,6 +435,32 @@ describe('PATCH /role-mapping-rule/:id', () => {
 		expect(stored?.expression).toBe('claims.patched === true');
 	});
 
+	it('should normalize order when patched to an abnormally high order value', async () => {
+		await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, order: 0 })
+			.expect(200);
+		await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, expression: 'claims.b', order: 1 })
+			.expect(200);
+		const third = await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, expression: 'claims.c', order: 2 })
+			.expect(200);
+
+		const response = await ownerAgent
+			.patch(`/role-mapping-rule/${third.body.data.id}`)
+			.send({ order: 100 })
+			.expect(200);
+
+		expect(response.body.data.order).toBe(2);
+
+		const repo = Container.get(RoleMappingRuleRepository);
+		const all = await repo.find({ where: { type: 'instance' }, order: { order: 'ASC' } });
+		expect(all.map((r) => r.order)).toEqual([0, 1, 2]);
+	});
+
 	it('should return 409 when patch sets order used by another rule', async () => {
 		await ownerAgent
 			.post('/role-mapping-rule')
@@ -483,6 +531,28 @@ describe('DELETE /role-mapping-rule/:id', () => {
 		await ownerAgent.delete('/role-mapping-rule/0000000000000099').expect(404);
 	});
 
+	it('should compact remaining rules after deleting a rule from the middle', async () => {
+		await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, order: 0 })
+			.expect(200);
+		const second = await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, expression: 'claims.b', order: 1 })
+			.expect(200);
+		await ownerAgent
+			.post('/role-mapping-rule')
+			.send({ ...validInstancePayload, expression: 'claims.c', order: 2 })
+			.expect(200);
+
+		await ownerAgent.delete(`/role-mapping-rule/${second.body.data.id}`).expect(200);
+
+		const repo = Container.get(RoleMappingRuleRepository);
+		const remaining = await repo.find({ where: { type: 'instance' }, order: { order: 'ASC' } });
+		expect(remaining).toHaveLength(2);
+		expect(remaining.map((r) => r.order)).toEqual([0, 1]);
+	});
+
 	it('should delete a rule and remove it from the database', async () => {
 		const createRes = await ownerAgent
 			.post('/role-mapping-rule')