diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml index 806eb703..47a306e0 100644 --- a/.github/workflows/workflow.yml +++ b/.github/workflows/workflow.yml @@ -59,7 +59,7 @@ jobs: run: docker compose -f docker-compose.yml -f docker-compose.ldap.yml down - name: SonarCloud Scan - uses: SonarSource/sonarqube-scan-action@v5 + uses: SonarSource/sonarqube-scan-action@v6 env: SONAR_TOKEN: ${{ secrets.SONARCLOUD_TOKEN }} diff --git a/src/builds/builds.service.spec.ts b/src/builds/builds.service.spec.ts index 2cbe0f22..6a1ee5ee 100644 --- a/src/builds/builds.service.spec.ts +++ b/src/builds/builds.service.spec.ts @@ -151,32 +151,128 @@ describe('BuildsService', () => { }); }); - it('delete', async () => { - const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build); - const buildFindManyMock = jest.fn().mockImplementation(() => Promise.resolve(build)); - const buildDeleteMock = jest.fn().mockImplementation(() => Promise.resolve(build)); - const testRunDeleteMock = jest.fn(); - const eventBuildDeletedMock = jest.fn(); - service = await initService({ - buildFindUniqueMock, - buildDeleteMock, - testRunDeleteMock, - eventBuildDeletedMock, - buildFindManyMock, + describe('remove', () => { + it('should remove a build successfully', async () => { + const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build); + const buildFindManyMock = jest.fn().mockImplementation(() => Promise.resolve(build)); + const buildDeleteMock = jest.fn().mockImplementation(() => Promise.resolve(build)); + const testRunDeleteMock = jest.fn(); + const eventBuildDeletedMock = jest.fn(); + service = await initService({ + buildFindUniqueMock, + buildDeleteMock, + testRunDeleteMock, + eventBuildDeletedMock, + buildFindManyMock, + }); + + await service.remove(build.id); + + expect(buildFindUniqueMock).toHaveBeenCalledWith({ + where: { id: build.id }, + include: { + testRuns: true, + }, + }); + expect(testRunDeleteMock).toHaveBeenCalledWith(build.testRuns[0].id); + expect(eventBuildDeletedMock).toHaveBeenCalledWith(new BuildDto(build)); + expect(buildDeleteMock).toHaveBeenCalledWith({ + where: { id: build.id }, + }); }); - await service.remove(build.id); + it('should handle undefined ID gracefully', async () => { + const buildFindUniqueMock = jest.fn(); + const loggerWarnMock = jest.fn(); + service = await initService({ buildFindUniqueMock }); + (service as any).logger = { warn: loggerWarnMock }; // Mock the logger - expect(buildFindUniqueMock).toHaveBeenCalledWith({ - where: { id: build.id }, - include: { - testRuns: true, - }, + await service.remove(undefined); + + expect(loggerWarnMock).toHaveBeenCalledWith('Attempted to remove build with undefined ID.'); + expect(buildFindUniqueMock).not.toHaveBeenCalled(); }); - expect(testRunDeleteMock).toHaveBeenCalledWith(build.testRuns[0].id); - expect(eventBuildDeletedMock).toHaveBeenCalledWith(new BuildDto(build)); - expect(buildDeleteMock).toHaveBeenCalledWith({ - where: { id: build.id }, + }); + + describe('deleteOldBuilds', () => { + const projectId = 'someProjectId'; + const build1 = { ...buildDto, id: 'build1', createdAt: new Date(Date.now() - 10000) }; + const build2 = { ...buildDto, id: 'build2', createdAt: new Date(Date.now() - 5000) }; + + it('should delete old builds and keep the specified number', async () => { + const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1, build2]); + const buildCountMock = jest.fn().mockResolvedValueOnce(2); + const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build1).mockResolvedValueOnce(build2); + mocked(BuildDto) + .mockReturnValueOnce(build1 as MockedObject) + .mockReturnValueOnce(build2 as MockedObject); + const testRunFindManyMock = jest.fn().mockResolvedValue([]); + const removeMock = jest.fn().mockResolvedValue(undefined); + const loggerDebugMock = jest.fn(); + + service = await initService({ buildFindManyMock, buildCountMock, buildFindUniqueMock, testRunFindManyMock }); + service.remove = removeMock; + (service as any).logger = { debug: loggerDebugMock }; + + await service.deleteOldBuilds(projectId, 2); + + expect(buildFindManyMock).toHaveBeenCalledWith({ + where: { projectId }, + take: undefined, + skip: 1, + orderBy: { createdAt: 'desc' }, + }); + expect(removeMock).toHaveBeenCalledTimes(2); + expect(removeMock).toHaveBeenCalledWith(build1.id); + expect(removeMock).toHaveBeenCalledWith(build2.id); + expect(loggerDebugMock).toHaveBeenCalledWith( + `Starting to delete old builds for project ${projectId}, keeping 2 builds.` + ); + expect(loggerDebugMock).toHaveBeenCalledWith(`Finished deleting old builds for project ${projectId}.`); + }); + + it('should handle concurrent calls for the same project by returning the existing promise', async () => { + const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1, build2]); + const buildCountMock = jest.fn().mockResolvedValueOnce(2); + const removeMock = jest.fn().mockResolvedValue(undefined); + const testRunFindManyMock = jest.fn().mockResolvedValue([]); + const buildFindUniqueMock = jest.fn().mockResolvedValueOnce(build1).mockResolvedValueOnce(build2); + mocked(BuildDto) + .mockReturnValueOnce(build1 as MockedObject) + .mockReturnValueOnce(build2 as MockedObject); + const loggerDebugMock = jest.fn(); + + service = await initService({ buildFindManyMock, buildCountMock, testRunFindManyMock, buildFindUniqueMock }); + service.remove = removeMock; + (service as any).logger = { debug: loggerDebugMock }; + + const promise1 = service.deleteOldBuilds(projectId, 2); + const promise2 = service.deleteOldBuilds(projectId, 2); + + expect(promise1).toStrictEqual(promise2); + expect(loggerDebugMock).toHaveBeenCalledWith( + `Deletion for project ${projectId} is already in progress. Returning existing promise.` + ); + + await promise1; // Wait for the deletion to complete + + expect(buildFindManyMock).toHaveBeenCalledTimes(1); // Only called once + expect(removeMock).toHaveBeenCalledTimes(2); + expect(loggerDebugMock).toHaveBeenCalledWith(`Finished deleting old builds for project ${projectId}.`); + }); + + it('should remove the promise from the map after completion (success)', async () => { + const buildFindManyMock = jest.fn().mockResolvedValueOnce([build1]); + const buildCountMock = jest.fn().mockResolvedValueOnce(1); + const removeMock = jest.fn().mockResolvedValue(undefined); + + service = await initService({ buildFindManyMock, buildCountMock }); + service.remove = removeMock; + + const projectId = 'testProject'; + await service.deleteOldBuilds(projectId, 0); + + expect(service['ongoingDeletions'].has(projectId)).toBe(false); }); }); diff --git a/src/builds/builds.service.ts b/src/builds/builds.service.ts index 90414c0c..c9aac1ee 100644 --- a/src/builds/builds.service.ts +++ b/src/builds/builds.service.ts @@ -10,6 +10,7 @@ import { ModifyBuildDto } from './dto/build-modify.dto'; @Injectable() export class BuildsService { private readonly logger: Logger = new Logger(BuildsService.name); + private readonly ongoingDeletions = new Map>(); constructor( private prismaService: PrismaService, @@ -60,6 +61,10 @@ export class BuildsService { } async remove(id: string): Promise { + if (!id) { + this.logger.warn(`Attempted to remove build with undefined ID.`); + return; + } this.logger.debug(`Going to remove Build ${id}`); const build = await this.prismaService.build.findUnique({ @@ -93,13 +98,30 @@ export class BuildsService { return build; } - async deleteOldBuilds(projectId: string, keepBuilds: number) { - keepBuilds = keepBuilds < 2 ? keepBuilds : keepBuilds - 1; - this.findMany(projectId, undefined, keepBuilds).then((buildList) => { - buildList.data.forEach((eachBuild) => { - this.remove(eachBuild.id); - }); + async deleteOldBuilds(projectId: string, keepBuilds: number): Promise { + if (this.ongoingDeletions.has(projectId)) { + this.logger.debug(`Deletion for project ${projectId} is already in progress. Returning existing promise.`); + return this.ongoingDeletions.get(projectId); + } + + const deletionPromise = (async () => { + this.logger.debug(`Starting to delete old builds for project ${projectId}, keeping ${keepBuilds} builds.`); + keepBuilds = keepBuilds < 2 ? keepBuilds : keepBuilds - 1; + const buildList = await this.findMany(projectId, undefined, keepBuilds); + for (const eachBuild of buildList.data) { + await this.remove(eachBuild.id); + } + this.logger.debug(`Finished deleting old builds for project ${projectId}.`); + })(); + + this.ongoingDeletions.set(projectId, deletionPromise); + + deletionPromise.finally(() => { + this.ongoingDeletions.delete(projectId); + this.logger.debug(`Deletion promise for project ${projectId} resolved/rejected and removed from map.`); }); + + return deletionPromise; } async approve(id: string, merge: boolean): Promise {