From ca7185da170a634a50012720c29d91cd724dd27c Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 15 Jan 2025 20:11:03 +0100 Subject: [PATCH 01/21] BC-7561 - a bit of cleanup this commit changes failed deletion and counted pending requests to also be older than configured threshold --- .../deletion/api/uc/deletion-request.uc.ts | 55 +++++++++++-------- .../service/deletion-request.service.ts | 17 ++++-- .../deletion/repo/deletion-request.repo.ts | 20 ++++--- .../repo/scope/deletion-request-scope.ts | 21 ++----- 4 files changed, 62 insertions(+), 51 deletions(-) diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index 05d6c9c13fb..407c8661afc 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -76,42 +76,39 @@ export class DeletionRequestUc implements IEventHandler { async executeDeletionRequests(limit?: number): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); - const maxAmountOfDeletionRequestsDoConcurrently = this.configService.get( - 'ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS' - ); - const callsDelayMilliseconds = this.configService.get('ADMIN_API__DELETION_DELAY_MILLISECONDS'); - let tasks: DeletionRequest[] = []; + + const max = this.configService.get('ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS'); + + let deletionRequests: DeletionRequest[] = []; do { - const numberOfDeletionRequestsWithStatusPending = - // eslint-disable-next-line no-await-in-loop - await this.deletionRequestService.countPendingDeletionRequests(); - const numberOfDeletionRequestsToProccess = - maxAmountOfDeletionRequestsDoConcurrently - numberOfDeletionRequestsWithStatusPending; + // eslint-disable-next-line no-await-in-loop + const pendingCount = await this.deletionRequestService.countPendingDeletionRequests(); + limit = limit ?? max - pendingCount; + this.logger.debug({ action: 'numberItemsWithStatusPending, amountWillingToTake', - numberOfDeletionRequestsWithStatusPending, - numberOfDeletionRequestsToProccess, + numberOfDeletionRequestsWithStatusPending: pendingCount, + numberOfDeletionRequestsToProccess: limit, }); // eslint-disable-next-line no-await-in-loop - if (numberOfDeletionRequestsToProccess > 0) { + if (limit > 0) { // eslint-disable-next-line no-await-in-loop - tasks = await this.deletionRequestService.findAllItemsToExecute(numberOfDeletionRequestsToProccess); + deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit); + + this.logger.debug({ action: 'processing deletion request', deletionRequests }); // eslint-disable-next-line no-await-in-loop await Promise.all( - tasks.map(async (req) => { + deletionRequests.map(async (req) => { await this.executeDeletionRequest(req); }) ); } - // short sleep mode to give time for deletion process to do their work - if (callsDelayMilliseconds && callsDelayMilliseconds > 0) { - // eslint-disable-next-line no-await-in-loop - await new Promise((resolve) => { - setTimeout(resolve, callsDelayMilliseconds); - }); - } - } while (tasks.length > 0); + + // eslint-disable-next-line no-await-in-loop + await this.sleep(); + } while (deletionRequests.length > 0); + this.logger.debug({ action: 'deletion process completed' }); } @@ -150,4 +147,16 @@ export class DeletionRequestUc implements IEventHandler { await this.deletionRequestService.markDeletionRequestAsFailed(deletionRequest.id); } } + + // short sleep mode to give time for deletion process to do its work + private async sleep() { + const delay = this.configService.get('ADMIN_API__DELETION_DELAY_MILLISECONDS'); + if (delay > 0) { + return new Promise((resolve) => { + setTimeout(resolve, delay); + }); + } + + return Promise.resolve(); + } } diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index 7e72b076bcc..716696e5303 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -9,10 +9,16 @@ import { DeletionConfig } from '../../deletion.config'; @Injectable() export class DeletionRequestService { + private modificationThreshold: Date; + constructor( private readonly deletionRequestRepo: DeletionRequestRepo, private readonly configService: ConfigService - ) {} + ) { + const threshold = this.configService.get('ADMIN_API__MODIFICATION_THRESHOLD_MS'); + const modificationThreshold = new Date(Date.now() - threshold); + this.modificationThreshold = modificationThreshold; + } async createDeletionRequest( targetRefId: EntityId, @@ -41,15 +47,14 @@ export class DeletionRequestService { return deletionRequest; } - async findAllItemsToExecute(limit?: number): Promise { - const threshold = this.configService.get('ADMIN_API__MODIFICATION_THRESHOLD_MS'); - const itemsToDelete: DeletionRequest[] = await this.deletionRequestRepo.findAllItemsToExecution(threshold, limit); + async findAllItemsToExecute(limit: number): Promise { + const deletionRequests = await this.deletionRequestRepo.findAllItemsToExecution(this.modificationThreshold, limit); - return itemsToDelete; + return deletionRequests; } async countPendingDeletionRequests(): Promise { - const numberItemsWithStatusPending: number = await this.deletionRequestRepo.countPendingDeletionRequests(); + const numberItemsWithStatusPending: number = await this.deletionRequestRepo.countPendingDeletionRequests(this.modificationThreshold); return numberItemsWithStatusPending; } diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index ed608270bd0..51807a4b538 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -6,6 +6,7 @@ import { DeletionRequest } from '../domain/do'; import { DeletionRequestEntity } from './entity'; import { DeletionRequestMapper } from './mapper'; import { DeletionRequestScope } from './scope'; +import { StatusModel } from '../domain/types'; @Injectable() export class DeletionRequestRepo { @@ -31,11 +32,15 @@ export class DeletionRequestRepo { await this.em.flush(); } - async findAllItemsToExecution(threshold: number, limit?: number): Promise { - const currentDate = new Date(); - const modificationThreshold = new Date(Date.now() - threshold); - const scope = new DeletionRequestScope().byDeleteAfter(currentDate).byStatus(modificationThreshold); - const order = { createdAt: SortOrder.desc }; + async findAllItemsToExecution(olderThan: Date, limit: number): Promise { + const scope = new DeletionRequestScope(); + scope.byDeleteAfter(new Date()); + scope.byStatus(StatusModel.REGISTERED, olderThan); + // TODO failed and hanging pending should be handled differently + scope.byStatus(StatusModel.FAILED, olderThan); + scope.byStatus(StatusModel.PENDING, olderThan); + + const order = { createdAt: SortOrder.desc }; // TODO why decending order? Should it not be ascending? const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { limit, @@ -47,8 +52,9 @@ export class DeletionRequestRepo { return mapped; } - async countPendingDeletionRequests(): Promise { - const scope = new DeletionRequestScope().byStatusPending(); + async countPendingDeletionRequests(olderThan: Date): Promise { + const scope = new DeletionRequestScope(); + scope.byStatus(StatusModel.PENDING, olderThan); const numberItemsWithStatusPending: number = await this.em.count(DeletionRequestEntity, scope.query); diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts index 6e602f0690c..4f38a9f78cb 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts @@ -9,21 +9,12 @@ export class DeletionRequestScope extends Scope { return this; } - byStatus(fifteenMinutesAgo: Date): this { - this.addQuery({ - $or: [ - { status: StatusModel.FAILED }, - { - $and: [{ status: [StatusModel.REGISTERED, StatusModel.PENDING] }, { updatedAt: { $lt: fifteenMinutesAgo } }], - }, - ], - }); - - return this; - } - - byStatusPending(): this { - this.addQuery({ status: [StatusModel.PENDING] }); + byStatus(status: StatusModel, olderThan: Date): this { + if (olderThan) { + this.addQuery({ $and: [{ status }, { updatedAt: { $lt: olderThan } }] }); + } else { + this.addQuery({ status }); + } return this; } From dba8d833065cf19a4671da3840baf1c43a30376d Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Mon, 20 Jan 2025 11:01:50 +0100 Subject: [PATCH 02/21] BC-7561 - improve logic for bulk deletion add new config var for when pending or failed should be no longer processed --- ...eletion-request-body-props.builder.spec.ts | 10 +++---- .../deletion-request-body-props.builder.ts | 4 +-- .../deletion-request-create.api.spec.ts | 10 +++---- .../dto/deletion-request.body.props.spec.ts | 21 +++++++------ .../dto/deletion-request.body.props.ts | 3 +- .../api/uc/deletion-request.uc.spec.ts | 4 +-- .../deletion/api/uc/deletion-request.uc.ts | 14 +++++---- .../src/modules/deletion/deletion.config.ts | 6 ++-- .../service/deletion-request.service.ts | 30 ++++++++++++------- .../deletion/domain/testing/test-config.ts | 12 ++++---- .../deletion/repo/deletion-request.repo.ts | 24 ++++++++++----- .../repo/entity/deletion-log.entity.ts | 3 +- .../repo/scope/deletion-request-scope.ts | 15 +++++++--- .../modules/server/admin-api-server.config.ts | 12 ++++++-- .../modules/server/admin-api.server.module.ts | 2 +- .../modules/user/interfaces/user-config.ts | 1 + .../src/modules/user/service/user.service.ts | 9 ++++-- config/default.schema.json | 24 ++++++++++----- package.json | 3 +- 19 files changed, 131 insertions(+), 76 deletions(-) diff --git a/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.spec.ts b/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.spec.ts index b86277aca15..1078ec41c34 100644 --- a/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.spec.ts +++ b/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.spec.ts @@ -10,18 +10,18 @@ describe(DeletionRequestBodyPropsBuilder.name, () => { const setup = () => { const domain = DomainName.PSEUDONYMS; const refId = new ObjectId().toHexString(); - const deleteInMinutes = 1000; - return { domain, refId, deleteInMinutes }; + const deleteAfterMinutes = 1000; + return { domain, refId, deleteAfterMinutes }; }; it('should build deletionRequestBodyParams with all attributes', () => { - const { domain, refId, deleteInMinutes } = setup(); + const { domain, refId, deleteAfterMinutes } = setup(); - const result = DeletionRequestBodyPropsBuilder.build(domain, refId, deleteInMinutes); + const result = DeletionRequestBodyPropsBuilder.build(domain, refId, deleteAfterMinutes); // Assert expect(result.targetRef.domain).toEqual(domain); expect(result.targetRef.id).toEqual(refId); - expect(result.deleteInMinutes).toEqual(deleteInMinutes); + expect(result.deleteAfterMinutes).toEqual(deleteAfterMinutes); }); }); }); diff --git a/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.ts b/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.ts index 7295d8c657f..5f6bfb9be7d 100644 --- a/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.ts +++ b/apps/server/src/modules/deletion/api/builder/deletion-request-body-props.builder.ts @@ -3,10 +3,10 @@ import { DeletionRequestBodyProps } from '../controller/dto'; import { DomainName } from '../../domain/types'; export class DeletionRequestBodyPropsBuilder { - static build(domain: DomainName, id: EntityId, deleteInMinutes?: number): DeletionRequestBodyProps { + static build(domain: DomainName, id: EntityId, deleteAfterMinutes?: number): DeletionRequestBodyProps { const deletionRequestItem = { targetRef: { domain, id }, - deleteInMinutes, + deleteAfterMinutes, }; return deletionRequestItem; diff --git a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts index 7eb27da8ebb..49b9e0c141c 100644 --- a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts +++ b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts @@ -85,17 +85,17 @@ describe(`deletionRequest create (api)`, () => { domain: DomainName.USER, id: '653e4833cc39e5907a1e18d2', }, - deleteInMinutes: 0, + deleteAfterMinutes: 0, }; - const defaultDeleteInMinutes = 43200; + const defaultdeleteAfterMinutes = 43200; const operationalTimeToleranceInSeconds = 30; return { deletionRequestToCreate, deletionRequestToImmediateRemoval, - defaultDeleteInMinutes, + defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds, }; }; @@ -122,7 +122,7 @@ describe(`deletionRequest create (api)`, () => { 'should set the "deletion planned at" date to the date after the default "delete in minutes" value ' + '(43200 minutes which is 30 days), with some operational time tolerance', async () => { - const { deletionRequestToCreate, defaultDeleteInMinutes, operationalTimeToleranceInSeconds } = setup(); + const { deletionRequestToCreate, defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); const response = await testApiClient.post('', deletionRequestToCreate); @@ -134,7 +134,7 @@ describe(`deletionRequest create (api)`, () => { const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange( createdItem.createdAt, createdItem.deleteAfter, - defaultDeleteInMinutes, + defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds ); diff --git a/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts b/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts index 1c4b5b80460..7714714e194 100644 --- a/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts +++ b/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts @@ -14,24 +14,23 @@ describe(DeletionRequestBodyProps.name, () => { expect(deletionRequestBodyProps).toBeDefined(); }); - it('deleteInMinutes should be a number with default value 43200', () => { + it('deleteAfterMinutes should be a number', () => { const { deletionRequestBodyProps } = setup(); - expect(deletionRequestBodyProps.deleteInMinutes).toBeDefined(); - expect(typeof deletionRequestBodyProps.deleteInMinutes).toBe('number'); - expect(deletionRequestBodyProps.deleteInMinutes).toBe(43200); + expect(deletionRequestBodyProps.deleteAfterMinutes).toBeDefined(); + expect(typeof deletionRequestBodyProps.deleteAfterMinutes).toBe('number'); }); - it('deleteInMinutes should be optional', () => { + it('deleteAfterMinutes should be optional', () => { const { deletionRequestBodyProps } = setup(); - deletionRequestBodyProps.deleteInMinutes = undefined; - expect(deletionRequestBodyProps.deleteInMinutes).toBeUndefined(); + deletionRequestBodyProps.deleteAfterMinutes = undefined; + expect(deletionRequestBodyProps.deleteAfterMinutes).toBeUndefined(); }); - it('deleteInMinutes should be a number when provided', () => { + it('deleteAfterMinutes should be a number when provided', () => { const { deletionRequestBodyProps } = setup(); - const customDeleteInMinutes = 60; - deletionRequestBodyProps.deleteInMinutes = customDeleteInMinutes; - expect(deletionRequestBodyProps.deleteInMinutes).toBe(customDeleteInMinutes); + const customdeleteAfterMinutes = 60; + deletionRequestBodyProps.deleteAfterMinutes = customdeleteAfterMinutes; + expect(deletionRequestBodyProps.deleteAfterMinutes).toBe(customdeleteAfterMinutes); }); }); }); diff --git a/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.ts b/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.ts index 5dc07f31d9b..99c0634c68b 100644 --- a/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.ts +++ b/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.ts @@ -2,7 +2,6 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { IsNumber, IsOptional, Min } from 'class-validator'; import { DeletionTargetRef } from '../../../domain/interface'; -const MINUTES_OF_30_DAYS = 30 * 24 * 60; export class DeletionRequestBodyProps { @ApiProperty({ required: true, @@ -17,5 +16,5 @@ export class DeletionRequestBodyProps { required: true, nullable: false, }) - deleteInMinutes?: number = MINUTES_OF_30_DAYS; + deleteAfterMinutes?: number; } diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 90fb8ee2b64..6b93618628f 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -75,7 +75,7 @@ describe(DeletionRequestUc.name, () => { domain: DomainName.USER, id: new ObjectId().toHexString(), }, - deleteInMinutes: 1440, + deleteAfterMinutes: 1440, }; const deletionRequest = deletionRequestFactory.build(); @@ -93,7 +93,7 @@ describe(DeletionRequestUc.name, () => { expect(deletionRequestService.createDeletionRequest).toHaveBeenCalledWith( deletionRequestToCreate.targetRef.id, deletionRequestToCreate.targetRef.domain, - deletionRequestToCreate.deleteInMinutes + deletionRequestToCreate.deleteAfterMinutes ); }); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index 407c8661afc..12b33ff1908 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -36,15 +36,15 @@ export class DeletionRequestUc implements IEventHandler { 'course', 'dashboard', 'file', - 'fileRecords', + //'fileRecords', 'lessons', + 'news', 'pseudonyms', 'rocketChatUser', + 'submissions', 'task', 'teams', 'user', - 'submissions', - 'news', ]; } @@ -65,10 +65,14 @@ export class DeletionRequestUc implements IEventHandler { async createDeletionRequest(deletionRequest: DeletionRequestBodyProps): Promise { this.logger.debug({ action: 'createDeletionRequest', deletionRequest }); + const hours = + deletionRequest.deleteAfterMinutes ?? this.configService.get('ADMIN_API__DELETION_DELETE_AFTER_MINUTES'); + const deleteAfter = new Date(); + deleteAfter.setMinutes(deleteAfter.getHours() + hours); const result = await this.deletionRequestService.createDeletionRequest( deletionRequest.targetRef.id, deletionRequest.targetRef.domain, - deletionRequest.deleteInMinutes + deleteAfter ); return result; @@ -77,7 +81,7 @@ export class DeletionRequestUc implements IEventHandler { async executeDeletionRequests(limit?: number): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); - const max = this.configService.get('ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS'); + const max = this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); let deletionRequests: DeletionRequest[] = []; diff --git a/apps/server/src/modules/deletion/deletion.config.ts b/apps/server/src/modules/deletion/deletion.config.ts index a8c78307587..eb9460dbd1f 100644 --- a/apps/server/src/modules/deletion/deletion.config.ts +++ b/apps/server/src/modules/deletion/deletion.config.ts @@ -18,7 +18,9 @@ export interface DeletionConfig FilesConfig, RocketChatUserConfig, XApiKeyAuthGuardConfig { - ADMIN_API__MODIFICATION_THRESHOLD_MS: number; - ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS: number; + ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS: number; + ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS: number; ADMIN_API__DELETION_DELAY_MILLISECONDS: number; + ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS: number; + ADMIN_API__DELETION_DELETE_AFTER_MINUTES: number; } diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index 716696e5303..3635363db98 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -9,29 +9,30 @@ import { DeletionConfig } from '../../deletion.config'; @Injectable() export class DeletionRequestService { - private modificationThreshold: Date; + private olderThan: Date; + + private newerThan: Date; constructor( private readonly deletionRequestRepo: DeletionRequestRepo, private readonly configService: ConfigService ) { - const threshold = this.configService.get('ADMIN_API__MODIFICATION_THRESHOLD_MS'); - const modificationThreshold = new Date(Date.now() - threshold); - this.modificationThreshold = modificationThreshold; + const thresholdOlder = this.configService.get('ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS'); + this.olderThan = new Date(Date.now() - thresholdOlder); + + const thresholdNewer = this.configService.get('ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS'); + this.newerThan = new Date(Date.now() - thresholdNewer); } async createDeletionRequest( targetRefId: EntityId, targetRefDomain: DomainName, - deleteInMinutes = 43200 + deleteAfter: Date ): Promise<{ requestId: EntityId; deletionPlannedAt: Date }> { - const dateOfDeletion = new Date(); - dateOfDeletion.setMinutes(dateOfDeletion.getMinutes() + deleteInMinutes); - const newDeletionRequest = new DeletionRequest({ id: new ObjectId().toHexString(), targetRefDomain, - deleteAfter: dateOfDeletion, + deleteAfter, targetRefId, status: StatusModel.REGISTERED, }); @@ -48,13 +49,20 @@ export class DeletionRequestService { } async findAllItemsToExecute(limit: number): Promise { - const deletionRequests = await this.deletionRequestRepo.findAllItemsToExecution(this.modificationThreshold, limit); + const deletionRequests = await this.deletionRequestRepo.findAllItemsToExecution( + this.olderThan, + this.newerThan, + limit + ); return deletionRequests; } async countPendingDeletionRequests(): Promise { - const numberItemsWithStatusPending: number = await this.deletionRequestRepo.countPendingDeletionRequests(this.modificationThreshold); + const numberItemsWithStatusPending: number = await this.deletionRequestRepo.countPendingDeletionRequests( + this.olderThan, + this.newerThan + ); return numberItemsWithStatusPending; } diff --git a/apps/server/src/modules/deletion/domain/testing/test-config.ts b/apps/server/src/modules/deletion/domain/testing/test-config.ts index 86e6c86798b..df76d882d2f 100644 --- a/apps/server/src/modules/deletion/domain/testing/test-config.ts +++ b/apps/server/src/modules/deletion/domain/testing/test-config.ts @@ -1,9 +1,11 @@ import { Configuration } from '@hpi-schul-cloud/commons'; const deletionConfig = { - ADMIN_API__MODIFICATION_THRESHOLD_MS: Configuration.get('ADMIN_API__MODIFICATION_THRESHOLD_MS') as number, - ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS: Configuration.get( - 'ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS' + ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS: Configuration.get( + 'ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS' + ) as number, + ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS: Configuration.get( + 'ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS' ) as number, ADMIN_API__DELETION_DELAY_MILLISECONDS: Configuration.get('ADMIN_API__DELETION_DELAY_MILLISECONDS') as number, }; @@ -12,8 +14,8 @@ const config = () => deletionConfig; export const deletionTestConfig = () => { const conf = config(); - conf.ADMIN_API__MODIFICATION_THRESHOLD_MS = 1000; - conf.ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS = 2; + conf.ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS = 1000; + conf.ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS = 2; conf.ADMIN_API__DELETION_DELAY_MILLISECONDS = 100; return conf; }; diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index 51807a4b538..653534d9e12 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -32,13 +32,18 @@ export class DeletionRequestRepo { await this.em.flush(); } - async findAllItemsToExecution(olderThan: Date, limit: number): Promise { + async findAllItemsToExecution(olderThan: Date, newerThan: Date, limit: number): Promise { + if (olderThan < newerThan) { + throw new Error('olderThan must be greater than newerThan'); + } const scope = new DeletionRequestScope(); scope.byDeleteAfter(new Date()); - scope.byStatus(StatusModel.REGISTERED, olderThan); - // TODO failed and hanging pending should be handled differently - scope.byStatus(StatusModel.FAILED, olderThan); - scope.byStatus(StatusModel.PENDING, olderThan); + + const statusScope = new DeletionRequestScope('$or'); + statusScope.byStatusAndDate(StatusModel.REGISTERED); + this.addScopeForFailedRequests(statusScope, olderThan, newerThan); + + scope.addQuery(statusScope.query); const order = { createdAt: SortOrder.desc }; // TODO why decending order? Should it not be ascending? @@ -52,9 +57,14 @@ export class DeletionRequestRepo { return mapped; } - async countPendingDeletionRequests(olderThan: Date): Promise { + private addScopeForFailedRequests(scope: DeletionRequestScope, olderThan: Date, newerThan: Date): void { + scope.byStatusAndDate(StatusModel.FAILED, olderThan, newerThan); + scope.byStatusAndDate(StatusModel.PENDING, olderThan, newerThan); + } + + async countPendingDeletionRequests(olderThan: Date, newerThan: Date): Promise { const scope = new DeletionRequestScope(); - scope.byStatus(StatusModel.PENDING, olderThan); + this.addScopeForFailedRequests(scope, olderThan, newerThan); const numberItemsWithStatusPending: number = await this.em.count(DeletionRequestEntity, scope.query); diff --git a/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts b/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts index 971b712cab7..9abb397e99b 100644 --- a/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts +++ b/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts @@ -1,7 +1,7 @@ import { BaseEntityWithTimestamps } from '@shared/domain/entity/base.entity'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; -import { Entity, Property, Index } from '@mikro-orm/core'; +import {Entity, Property, Index, Unique} from '@mikro-orm/core'; import { DomainOperationReport, DomainDeletionReport } from '../../domain/interface'; import { DomainName } from '../../domain/types'; @@ -17,6 +17,7 @@ export interface DeletionLogEntityProps { } @Entity({ tableName: 'deletionlogs' }) +@Unique({ properties: ['domain', 'deletionRequestId'] }) // TODO check if this is correct, if we want to try to delete multiple times, it should not create a new log entry, but update it. Or first remove it export class DeletionLogEntity extends BaseEntityWithTimestamps { @Property() @Index() diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts index 4f38a9f78cb..a55594879ea 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts @@ -9,13 +9,20 @@ export class DeletionRequestScope extends Scope { return this; } - byStatus(status: StatusModel, olderThan: Date): this { + byStatusAndDate(status: StatusModel, olderThan?: Date, newerThan?: Date): this { + let query = { status }; if (olderThan) { - this.addQuery({ $and: [{ status }, { updatedAt: { $lt: olderThan } }] }); - } else { - this.addQuery({ status }); + const olderThanQuery = { updatedAt: { $lt: olderThan } }; + query = { ...query, ...olderThanQuery }; } + if (newerThan) { + const newerThanQuery = { updatedAt: { $gte: newerThan } }; + query = { ...query, ...newerThanQuery }; + } + + this.addQuery(query); + return this; } } diff --git a/apps/server/src/modules/server/admin-api-server.config.ts b/apps/server/src/modules/server/admin-api-server.config.ts index 4e889b24015..0c1dcdfd122 100644 --- a/apps/server/src/modules/server/admin-api-server.config.ts +++ b/apps/server/src/modules/server/admin-api-server.config.ts @@ -19,11 +19,17 @@ export interface AdminApiServerConfig } const config: AdminApiServerConfig = { - ADMIN_API__MODIFICATION_THRESHOLD_MS: Configuration.get('ADMIN_API__MODIFICATION_THRESHOLD_MS') as number, - ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS: Configuration.get( - 'ADMIN_API__MAX_CONCURRENT_DELETION_REQUESTS' + ADMIN_API__DELETION_DELETE_AFTER_MINUTES: Configuration.get('ADMIN_API__DELETION_DELETE_AFTER_MINUTES') as number, + ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS: Configuration.get( + 'ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS' + ) as number, + ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS: Configuration.get( + 'ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS' ) as number, ADMIN_API__DELETION_DELAY_MILLISECONDS: Configuration.get('ADMIN_API__DELETION_DELAY_MILLISECONDS') as number, + ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS: Configuration.get( + 'ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS' + ) as number, NEST_LOG_LEVEL: Configuration.get('NEST_LOG_LEVEL') as string, EXIT_ON_ERROR: Configuration.get('EXIT_ON_ERROR') as boolean, AVAILABLE_LANGUAGES: (Configuration.get('I18N__AVAILABLE_LANGUAGES') as string).split(',') as LanguageType[], diff --git a/apps/server/src/modules/server/admin-api.server.module.ts b/apps/server/src/modules/server/admin-api.server.module.ts index e8ab72ea612..a3a734eb87a 100644 --- a/apps/server/src/modules/server/admin-api.server.module.ts +++ b/apps/server/src/modules/server/admin-api.server.module.ts @@ -44,7 +44,7 @@ const serverModules = [ password: DB_PASSWORD, user: DB_USERNAME, entities: [...ALL_ENTITIES, FileEntity], - // debug: true, // use it for locally debugging of queries + debug: true, // use it for locally debugging of queries }), CqrsModule, LoggerModule, diff --git a/apps/server/src/modules/user/interfaces/user-config.ts b/apps/server/src/modules/user/interfaces/user-config.ts index 8923e95fb1d..769f2e20af0 100644 --- a/apps/server/src/modules/user/interfaces/user-config.ts +++ b/apps/server/src/modules/user/interfaces/user-config.ts @@ -1,4 +1,5 @@ export interface UserConfig { AVAILABLE_LANGUAGES: string[]; TEACHER_VISIBILITY_FOR_EXTERNAL_TEAM_INVITATION: string; + CALENDAR_SERVICE_ENABLED: boolean; } diff --git a/apps/server/src/modules/user/service/user.service.ts b/apps/server/src/modules/user/service/user.service.ts index 920ec2d37da..3667aba6661 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -250,9 +250,14 @@ export class UserService implements DeletionService, IEventHandler('CALENDAR_SERVICE_ENABLED')) { + const calendarEventsDeleted = await this.removeCalendarEvents(userId); + subdomainOperation.push(calendarEventsDeleted); + } const numberOfDeletedUsers = await this.userRepo.deleteUser(userId); @@ -263,7 +268,7 @@ export class UserService implements DeletionService, IEventHandler:],[:]" }, - "MODIFICATION_THRESHOLD_MS": { + "DELETION_DELETE_AFTER_MINUTES": { "type": "number", - "default": 60000, - "description": "threshold in milliseconds to get deletionRequest to delete" + "default": 43200, + "description": "threshold in minutes, after which a deletion requests can be processed" }, - "MAX_CONCURRENT_DELETION_REQUESTS": { + "DELETION_MODIFICATION_THRESHOLD_MS": { "type": "number", - "default": 100, - "description": "max amount of deletion requests proccessed concurently" + "default": 300000, + "description": "threshold in milliseconds to try again to process a hanging Pending or Failed deletion requests" + }, + "DELETION_CONSIDER_FAILED_AFTER_MS" : { + "type": "number", + "default": 3600000, + "description": "threshold in milliseconds to stop trying to process Pending or Failed deletion requests" + }, + "DELETION_MAX_CONCURRENT_DELETION_REQUESTS": { + "type": "number", + "default": 20, + "description": "max amount of deletion requests to be processed simultaneously" }, "DELETION_DELAY_MILLISECONDS": { "type": "number", "default": 5000, - "description": "deley in milliseconds to give time modules to perform deletion proccess" + "description": "delay in milliseconds to sleep before processing another batch of of deletion requests" } }, "default": {} diff --git a/package.json b/package.json index df8c267b310..08014c9b30b 100644 --- a/package.json +++ b/package.json @@ -59,14 +59,15 @@ "nest:start:dev": "nest start server --watch", "nest:start:debug": "nest start server --debug --watch", "nest:start:prod": "node dist/apps/server/apps/server.app", - "nest:start:management": "nest start management", "nest:start:admin-api-server": "nest start admin-api-server", + "nest:start:admin-api-server:dev": "nest start admin-api-server --watch", "nest:start:admin-api-server:debug": "nest start admin-api-server --debug --watch", "nest:start:admin-api-server:prod": "node dist/apps/server/apps/admin-api-server.app", "nest:start:board-collaboration": "nest start board-collaboration", "nest:start:board-collaboration:dev": "nest start board-collaboration --watch --", "nest:start:board-collaboration:debug": "nest start board-collaboration --debug --watch", "nest:start:board-collaboration:prod": "node dist/apps/server/apps/board-collaboration.app", + "nest:start:management": "nest start management", "nest:start:management:dev": "nest start management --watch", "nest:start:management:debug": "nest start management --debug --watch", "nest:start:management:prod": "node dist/apps/server/apps/management.app", From ee088b53381853d3cac33a4a74aea962b93b98ba Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Mon, 20 Jan 2025 11:29:10 +0100 Subject: [PATCH 03/21] fix api tets and change sorting --- .../deletion-request-create.api.spec.ts | 74 ++++++++++------ .../dto/deletion-request.body.props.spec.ts | 7 +- .../deletion/api/uc/deletion-request.uc.ts | 6 +- .../service/deletion-request.service.ts | 4 +- .../repo/deletion-request.repo.spec.ts | 85 ++++++++++++------- .../deletion/repo/deletion-request.repo.ts | 9 +- .../repo/entity/deletion-log.entity.ts | 2 +- .../repo/scope/deletion-request-scope.spec.ts | 8 +- .../repo/scope/deletion-request-scope.ts | 4 +- 9 files changed, 120 insertions(+), 79 deletions(-) diff --git a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts index 894baab6f3e..06de3203e64 100644 --- a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts +++ b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts @@ -117,34 +117,30 @@ describe(`deletionRequest create (api)`, () => { expect(result.requestId).toBeDefined(); }); - describe('when the "delete in minutes" param has not been provided', () => { - it( - 'should set the "deletion planned at" date to the date after the default "delete in minutes" value ' + - '(43200 minutes which is 30 days), with some operational time tolerance', - async () => { - const { deletionRequestToCreate, defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); - - const response = await testApiClient.post('', deletionRequestToCreate); - - const result = response.body as DeletionRequestResponse; - const createdDeletionRequestId = result.requestId; - - const createdItem = await em.findOneOrFail(DeletionRequestEntity, createdDeletionRequestId); - - const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange( - createdItem.createdAt, - createdItem.deleteAfter, - defaultdeleteAfterMinutes, - operationalTimeToleranceInSeconds - ); - - expect(isDeletionPlannedAtDateCorrect).toEqual(true); - } - ); + describe('when the "delete after minutes" param has not been provided', () => { + it('should set the "deleteAfter" date to the date after the default DELETION_DELETE_AFTER_MINUTES ', async () => { + const { deletionRequestToCreate, defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); + + const response = await testApiClient.post('', deletionRequestToCreate); + + const result = response.body as DeletionRequestResponse; + const createdDeletionRequestId = result.requestId; + + const createdItem = await em.findOneOrFail(DeletionRequestEntity, createdDeletionRequestId); + + const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange( + createdItem.createdAt, + createdItem.deleteAfter, + defaultdeleteAfterMinutes, + operationalTimeToleranceInSeconds + ); + + expect(isDeletionPlannedAtDateCorrect).toEqual(true); + }); }); - describe('when the "delete in minutes" param has been set to 0', () => { - it('should set the "deletion planned at" date to now, with some operational time tolerance', async () => { + describe('when the "delete after minutes" param has been set to 0', () => { + it('should set the "deleteAfter" date to now', async () => { const { deletionRequestToImmediateRemoval, operationalTimeToleranceInSeconds } = setup(); const response = await testApiClient.post('', deletionRequestToImmediateRemoval); @@ -164,6 +160,32 @@ describe(`deletionRequest create (api)`, () => { expect(isDeletionPlannedAtDateCorrect).toEqual(true); }); }); + + describe('when the "delete after minutes" param has been set to > 0', () => { + it('should set the "deleteAfter" date to now plus "delete after minutes" ', async () => { + const { deletionRequestToCreate, operationalTimeToleranceInSeconds } = setup(); + + const deleteAfterMinutes = 120; + + deletionRequestToCreate.deleteAfterMinutes = deleteAfterMinutes; + + const response = await testApiClient.post('', deletionRequestToCreate); + + const result = response.body as DeletionRequestResponse; + const createdDeletionRequestId = result.requestId; + + const createdItem = await em.findOneOrFail(DeletionRequestEntity, createdDeletionRequestId); + + const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange( + createdItem.createdAt, + createdItem.deleteAfter, + deleteAfterMinutes, + operationalTimeToleranceInSeconds + ); + + expect(isDeletionPlannedAtDateCorrect).toEqual(true); + }); + }); }); }); }); diff --git a/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts b/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts index 7714714e194..418fe5400a6 100644 --- a/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts +++ b/apps/server/src/modules/deletion/api/controller/dto/deletion-request.body.props.spec.ts @@ -14,12 +14,6 @@ describe(DeletionRequestBodyProps.name, () => { expect(deletionRequestBodyProps).toBeDefined(); }); - it('deleteAfterMinutes should be a number', () => { - const { deletionRequestBodyProps } = setup(); - expect(deletionRequestBodyProps.deleteAfterMinutes).toBeDefined(); - expect(typeof deletionRequestBodyProps.deleteAfterMinutes).toBe('number'); - }); - it('deleteAfterMinutes should be optional', () => { const { deletionRequestBodyProps } = setup(); deletionRequestBodyProps.deleteAfterMinutes = undefined; @@ -31,6 +25,7 @@ describe(DeletionRequestBodyProps.name, () => { const customdeleteAfterMinutes = 60; deletionRequestBodyProps.deleteAfterMinutes = customdeleteAfterMinutes; expect(deletionRequestBodyProps.deleteAfterMinutes).toBe(customdeleteAfterMinutes); + expect(typeof deletionRequestBodyProps.deleteAfterMinutes).toBe('number'); }); }); }); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index 12b33ff1908..ebf6c51b795 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -36,7 +36,7 @@ export class DeletionRequestUc implements IEventHandler { 'course', 'dashboard', 'file', - //'fileRecords', + 'fileRecords', 'lessons', 'news', 'pseudonyms', @@ -65,10 +65,10 @@ export class DeletionRequestUc implements IEventHandler { async createDeletionRequest(deletionRequest: DeletionRequestBodyProps): Promise { this.logger.debug({ action: 'createDeletionRequest', deletionRequest }); - const hours = + const minutes = deletionRequest.deleteAfterMinutes ?? this.configService.get('ADMIN_API__DELETION_DELETE_AFTER_MINUTES'); const deleteAfter = new Date(); - deleteAfter.setMinutes(deleteAfter.getHours() + hours); + deleteAfter.setMinutes(deleteAfter.getMinutes() + minutes); const result = await this.deletionRequestService.createDeletionRequest( deletionRequest.targetRef.id, deletionRequest.targetRef.domain, diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index 3635363db98..d73d5c379f3 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -50,9 +50,9 @@ export class DeletionRequestService { async findAllItemsToExecute(limit: number): Promise { const deletionRequests = await this.deletionRequestRepo.findAllItemsToExecution( + limit, this.olderThan, - this.newerThan, - limit + this.newerThan ); return deletionRequests; diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts index aa6b7755532..1e6a692238c 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts @@ -105,15 +105,19 @@ describe(DeletionRequestRepo.name, () => { describe('findAllItemsToExecution', () => { describe('when there is no deletionRequest for execution', () => { const setup = () => { - const threshold = 1000; + const limit = 1000; + const olderThan = new Date(); + const newerThan = new Date(); return { - threshold, + limit, + olderThan, + newerThan, }; }; it('should return empty array', async () => { - const { threshold } = setup(); - const result = await repo.findAllItemsToExecution(threshold); + const { limit, olderThan, newerThan } = setup(); + const result = await repo.findAllItemsToExecution(limit, olderThan, newerThan); expect(result).toEqual([]); }); @@ -121,9 +125,15 @@ describe(DeletionRequestRepo.name, () => { describe('when there are deletionRequests for execution', () => { const setup = async () => { - const threshold = 1000; + const limit = 1000; + const olderThan = new Date(); + olderThan.setDate(olderThan.getDate() + 1); + const newerThan = new Date(); + newerThan.setDate(newerThan.getDate() - 1); + const dateInFuture = new Date(); dateInFuture.setDate(dateInFuture.getDate() + 30); + const deletionRequestEntity1: DeletionRequestEntity = deletionRequestEntityFactory.build({ createdAt: new Date(2023, 7, 1), updatedAt: new Date(2023, 8, 2), @@ -140,14 +150,17 @@ describe(DeletionRequestRepo.name, () => { createdAt: new Date(2023, 8, 1), updatedAt: new Date(2023, 8, 1), deleteAfter: new Date(2023, 9, 1), + status: StatusModel.REGISTERED, }); const deletionRequestEntity4: DeletionRequestEntity = deletionRequestEntityFactory.build({ createdAt: new Date(2023, 9, 1), updatedAt: new Date(2023, 9, 1), deleteAfter: new Date(2023, 10, 1), + status: StatusModel.REGISTERED, }); const deletionRequestEntity5: DeletionRequestEntity = deletionRequestEntityFactory.build({ deleteAfter: dateInFuture, + status: StatusModel.REGISTERED, }); await em.persistAndFlush([ @@ -178,34 +191,22 @@ describe(DeletionRequestRepo.name, () => { createdAt: deletionRequestEntity3.createdAt, updatedAt: deletionRequestEntity3.updatedAt, }, - { - id: deletionRequestEntity2.id, - targetRefDomain: deletionRequestEntity2.targetRefDomain, - deleteAfter: deletionRequestEntity2.deleteAfter, - targetRefId: deletionRequestEntity2.targetRefId, - status: deletionRequestEntity2.status, - createdAt: deletionRequestEntity2.createdAt, - updatedAt: deletionRequestEntity2.updatedAt, - }, ]; - return { deletionRequestEntity1, deletionRequestEntity5, expectedArray, threshold }; + return { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit, olderThan, newerThan }; }; - it('should find deletionRequests with deleteAfter smaller then today and status with value registered or failed', async () => { - const { deletionRequestEntity1, deletionRequestEntity5, expectedArray, threshold } = await setup(); + it('should find deletionRequests with deleteAfter smaller then today and status with value registered', async () => { + const { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit, olderThan, newerThan } = + await setup(); - const results = await repo.findAllItemsToExecution(threshold); + const results = await repo.findAllItemsToExecution(limit, olderThan, newerThan); - expect(results.length).toEqual(3); + expect(results.length).toEqual(2); // Verify explicit fields. expect(results).toEqual( - expect.arrayContaining([ - expect.objectContaining(expectedArray[0]), - expect.objectContaining(expectedArray[1]), - expect.objectContaining(expectedArray[2]), - ]) + expect.arrayContaining([expect.objectContaining(expectedArray[0]), expect.objectContaining(expectedArray[1])]) ); const result1: DeletionRequest = await repo.findById(deletionRequestEntity1.id); @@ -218,9 +219,9 @@ describe(DeletionRequestRepo.name, () => { }); it('should find deletionRequests to execute with limit = 2', async () => { - const { expectedArray, threshold } = await setup(); + const { expectedArray, olderThan, newerThan } = await setup(); - const results = await repo.findAllItemsToExecution(threshold, 2); + const results = await repo.findAllItemsToExecution(2, olderThan, newerThan); expect(results.length).toEqual(2); @@ -234,8 +235,27 @@ describe(DeletionRequestRepo.name, () => { describe('countPendingDeletionRequests', () => { describe('when there is no deletionRequest with status pending', () => { + const setup = async () => { + const olderThan = new Date(); + olderThan.setDate(olderThan.getDate() + 1); + const newerThan = new Date(); + newerThan.setDate(newerThan.getDate() - 1); + const deletionRequestWithStatusRegistered: DeletionRequestEntity[] = + deletionRequestEntityFactory.buildListWithId(5, { + status: StatusModel.REGISTERED, + }); + + await em.persistAndFlush(deletionRequestWithStatusRegistered); + em.clear(); + + return { + olderThan, + newerThan, + }; + }; it('should return zero', async () => { - const result = await repo.countPendingDeletionRequests(); + const { olderThan, newerThan } = await setup(); + const result = await repo.countPendingDeletionRequests(olderThan, newerThan); expect(result).toEqual(0); }); @@ -243,6 +263,11 @@ describe(DeletionRequestRepo.name, () => { describe('when there are deletionRequests with status pending', () => { const setup = async () => { + const olderThan = new Date(); + olderThan.setDate(olderThan.getDate() + 1); + const newerThan = new Date(); + newerThan.setDate(newerThan.getDate() - 1); + const deletionRequestWithStatusPending: DeletionRequestEntity[] = deletionRequestEntityFactory.buildListWithId( 5, { @@ -260,13 +285,13 @@ describe(DeletionRequestRepo.name, () => { await em.persistAndFlush([...deletionRequestWithStatusPending, ...deletionRequestWithStatusRegistered]); em.clear(); - return { expectedNumberDeletionRequestWithStatusPending }; + return { expectedNumberDeletionRequestWithStatusPending, olderThan, newerThan }; }; it('should count deletionRequests with status pending and return proper number', async () => { - const { expectedNumberDeletionRequestWithStatusPending } = await setup(); + const { expectedNumberDeletionRequestWithStatusPending, olderThan, newerThan } = await setup(); - const result = await repo.countPendingDeletionRequests(); + const result = await repo.countPendingDeletionRequests(olderThan, newerThan); expect(result).toEqual(expectedNumberDeletionRequestWithStatusPending); }); diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index 653534d9e12..68c6b96f97d 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -32,7 +32,7 @@ export class DeletionRequestRepo { await this.em.flush(); } - async findAllItemsToExecution(olderThan: Date, newerThan: Date, limit: number): Promise { + async findAllItemsToExecution(limit: number, olderThan: Date, newerThan: Date): Promise { if (olderThan < newerThan) { throw new Error('olderThan must be greater than newerThan'); } @@ -40,12 +40,12 @@ export class DeletionRequestRepo { scope.byDeleteAfter(new Date()); const statusScope = new DeletionRequestScope('$or'); - statusScope.byStatusAndDate(StatusModel.REGISTERED); + statusScope.byStatusAndDate([StatusModel.REGISTERED]); this.addScopeForFailedRequests(statusScope, olderThan, newerThan); scope.addQuery(statusScope.query); - const order = { createdAt: SortOrder.desc }; // TODO why decending order? Should it not be ascending? + const order = { createdAt: SortOrder.asc }; const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { limit, @@ -58,8 +58,7 @@ export class DeletionRequestRepo { } private addScopeForFailedRequests(scope: DeletionRequestScope, olderThan: Date, newerThan: Date): void { - scope.byStatusAndDate(StatusModel.FAILED, olderThan, newerThan); - scope.byStatusAndDate(StatusModel.PENDING, olderThan, newerThan); + scope.byStatusAndDate([StatusModel.FAILED, StatusModel.PENDING], olderThan, newerThan); } async countPendingDeletionRequests(olderThan: Date, newerThan: Date): Promise { diff --git a/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts b/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts index 9abb397e99b..27801cbbe89 100644 --- a/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts +++ b/apps/server/src/modules/deletion/repo/entity/deletion-log.entity.ts @@ -1,7 +1,7 @@ import { BaseEntityWithTimestamps } from '@shared/domain/entity/base.entity'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; -import {Entity, Property, Index, Unique} from '@mikro-orm/core'; +import { Entity, Property, Index, Unique } from '@mikro-orm/core'; import { DomainOperationReport, DomainDeletionReport } from '../../domain/interface'; import { DomainName } from '../../domain/types'; diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts index ae043621731..9b779e6a299 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts @@ -32,7 +32,7 @@ describe(DeletionRequestScope.name, () => { }); }); - describe('byStatus', () => { + describe('byStatusAndDate', () => { const setup = () => { const fifteenMinutesAgo = new Date(Date.now() - 15 * 60 * 1000); // 15 minutes ago const expectedQuery = { @@ -55,7 +55,7 @@ describe(DeletionRequestScope.name, () => { it('should add query', () => { const { expectedQuery, fifteenMinutesAgo } = setup(); - const result = scope.byStatus(fifteenMinutesAgo); + const result = scope.byStatusAndDate(StatusModel.FAILED, fifteenMinutesAgo); expect(result).toBeInstanceOf(DeletionRequestScope); expect(scope.query).toEqual(expectedQuery); @@ -63,7 +63,7 @@ describe(DeletionRequestScope.name, () => { }); }); - describe('byStatusPending', () => { + describe('byStatusAndDate for Pending', () => { const setup = () => { const expectedQuery = { status: [StatusModel.PENDING] }; @@ -74,7 +74,7 @@ describe(DeletionRequestScope.name, () => { it('should add query', () => { const { expectedQuery } = setup(); - const result = scope.byStatusPending(); + const result = scope.byStatusAndDate(StatusModel.PENDING); expect(result).toBeInstanceOf(DeletionRequestScope); expect(scope.query).toEqual(expectedQuery); diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts index a55594879ea..116c8aca393 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts @@ -9,8 +9,8 @@ export class DeletionRequestScope extends Scope { return this; } - byStatusAndDate(status: StatusModel, olderThan?: Date, newerThan?: Date): this { - let query = { status }; + byStatusAndDate(status: StatusModel[], olderThan?: Date, newerThan?: Date): this { + let query = { status: { $in: status } }; if (olderThan) { const olderThanQuery = { updatedAt: { $lt: olderThan } }; query = { ...query, ...olderThanQuery }; From fc5ce93b3b0bebbf583b4780e981d2438114ec77 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Tue, 21 Jan 2025 17:17:03 +0100 Subject: [PATCH 04/21] Fix some tests --- .../dto/deletion-execution.params.ts | 6 ++- .../api/uc/deletion-request.uc.spec.ts | 15 +++++-- .../service/deletion-request.service.spec.ts | 45 +++++++++++++------ .../deletion/domain/testing/test-config.ts | 8 +++- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/apps/server/src/modules/deletion/api/controller/dto/deletion-execution.params.ts b/apps/server/src/modules/deletion/api/controller/dto/deletion-execution.params.ts index 9ca4a18bd71..79e54651988 100644 --- a/apps/server/src/modules/deletion/api/controller/dto/deletion-execution.params.ts +++ b/apps/server/src/modules/deletion/api/controller/dto/deletion-execution.params.ts @@ -1,5 +1,5 @@ import { ApiPropertyOptional } from '@nestjs/swagger'; -import { IsInt, IsOptional, Min } from 'class-validator'; +import { IsBoolean, IsInt, IsOptional, Min } from 'class-validator'; export class DeletionExecutionParams { @IsInt() @@ -7,4 +7,8 @@ export class DeletionExecutionParams { @IsOptional() @ApiPropertyOptional({ description: 'Page limit, defaults to 100.', minimum: 1 }) limit?: number = 100; + + @IsBoolean() + @IsOptional() + runFailed?: boolean = false; } diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 14aeb75b1de..abb7c3711b3 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -17,6 +17,7 @@ import { DeletionRequestLogResponseBuilder } from '../builder'; import { DeletionRequestBodyProps } from '../controller/dto'; import { DeletionLogStatisticBuilder, DeletionTargetRefBuilder } from '../controller/dto/builder'; import { DeletionRequestUc } from './deletion-request.uc'; +import objectContaining = jasmine.objectContaining; describe(DeletionRequestUc.name, () => { let module: TestingModule; @@ -61,6 +62,9 @@ describe(DeletionRequestUc.name, () => { deletionRequestService = module.get(DeletionRequestService); deletionLogService = module.get(DeletionLogService); eventBus = module.get(EventBus); + + jest.useFakeTimers(); + jest.setSystemTime(new Date(Date.now())); }); beforeEach(() => { @@ -70,30 +74,35 @@ describe(DeletionRequestUc.name, () => { describe('createDeletionRequest', () => { describe('when creating a deletionRequest', () => { const setup = () => { + const deleteAfterMinutes = 1; const deletionRequestToCreate: DeletionRequestBodyProps = { targetRef: { domain: DomainName.USER, id: new ObjectId().toHexString(), }, - deleteAfterMinutes: 1440, + deleteAfterMinutes, }; const deletionRequest = deletionRequestFactory.build(); + const deleteAfter = new Date(); + deleteAfter.setMinutes(deleteAfter.getMinutes() + deleteAfterMinutes); + return { deletionRequestToCreate, deletionRequest, + deleteAfter, }; }; it('should call the service to create the deletionRequest', async () => { - const { deletionRequestToCreate } = setup(); + const { deletionRequestToCreate, deleteAfter } = setup(); await uc.createDeletionRequest(deletionRequestToCreate); expect(deletionRequestService.createDeletionRequest).toHaveBeenCalledWith( deletionRequestToCreate.targetRef.id, deletionRequestToCreate.targetRef.domain, - deletionRequestToCreate.deleteAfterMinutes + deleteAfter ); }); diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts index b56e09183b0..ec1c91e9d91 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts @@ -1,5 +1,5 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { ConfigModule } from '@nestjs/config'; +import { ConfigModule, ConfigService } from '@nestjs/config'; import { Test, TestingModule } from '@nestjs/testing'; import { createConfigModuleOptions } from '@shared/common'; import { setupEntities } from '@testing/setup-entities'; @@ -13,6 +13,7 @@ describe(DeletionRequestService.name, () => { let module: TestingModule; let service: DeletionRequestService; let deletionRequestRepo: DeepMocked; + let configService: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -28,8 +29,12 @@ describe(DeletionRequestService.name, () => { service = module.get(DeletionRequestService); deletionRequestRepo = module.get(DeletionRequestRepo); + configService = module.get(ConfigService); await setupEntities(); + + jest.useFakeTimers(); + jest.setSystemTime(new Date()); }); beforeEach(() => { @@ -51,20 +56,23 @@ describe(DeletionRequestService.name, () => { const setup = () => { const targetRefId = '653e4833cc39e5907a1e18d2'; const targetRefDomain = DomainName.USER; + const deleteAfter = new Date(); + const minutes = 60; + deleteAfter.setMinutes(deleteAfter.getMinutes() + minutes); - return { targetRefId, targetRefDomain }; + return { targetRefId, targetRefDomain, deleteAfter }; }; it('should call deletionRequestRepo.create', async () => { - const { targetRefId, targetRefDomain } = setup(); + const { targetRefId, targetRefDomain, deleteAfter } = setup(); - await service.createDeletionRequest(targetRefId, targetRefDomain); + await service.createDeletionRequest(targetRefId, targetRefDomain, deleteAfter); expect(deletionRequestRepo.create).toHaveBeenCalledWith( expect.objectContaining({ id: expect.any(String), targetRefDomain, - deleteAfter: expect.any(Date), + deleteAfter, targetRefId, status: StatusModel.REGISTERED, }) @@ -105,30 +113,39 @@ describe(DeletionRequestService.name, () => { describe('findAllItemsToExecute', () => { describe('when finding all deletionRequests for execution', () => { const setup = () => { + const limit = 100; + const dateInPast = new Date(); - const threshold = 1000; - const limit = undefined; dateInPast.setDate(dateInPast.getDate() - 1); + + const thresholdOlderMs = configService.get('ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS') ?? 100; + const thresholdNewerMs = configService.get('ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS') ?? 1000; + + const olderThan = new Date(Date.now() - thresholdOlderMs); + const newerThan = new Date(Date.now() - thresholdNewerMs); + const deletionRequest1 = deletionRequestFactory.build({ deleteAfter: dateInPast }); const deletionRequest2 = deletionRequestFactory.build({ deleteAfter: dateInPast }); deletionRequestRepo.findAllItemsToExecution.mockResolvedValue([deletionRequest1, deletionRequest2]); const deletionRequests = [deletionRequest1, deletionRequest2]; - return { deletionRequests, limit, threshold }; + + return { deletionRequests, limit, olderThan, newerThan }; }; - it('should call deletionRequestRepo.findAllItemsByDeletionDate with required parameter', async () => { - const { limit, threshold } = setup(); + it('should call deletionRequestRepo.findAllItemsToExecution with required parameter', async () => { + const { limit } = setup(); - await service.findAllItemsToExecute(); + await service.findAllItemsToExecute(limit); - expect(deletionRequestRepo.findAllItemsToExecution).toBeCalledWith(threshold, limit); + // TODO fix date comparison + expect(deletionRequestRepo.findAllItemsToExecution).toBeCalledWith(limit, expect.any(Date), expect.any(Date)); }); it('should return array of two deletionRequests to execute', async () => { - const { deletionRequests } = setup(); - const result = await service.findAllItemsToExecute(); + const { deletionRequests, limit } = setup(); + const result = await service.findAllItemsToExecute(limit); expect(result).toHaveLength(2); expect(result).toEqual(deletionRequests); diff --git a/apps/server/src/modules/deletion/domain/testing/test-config.ts b/apps/server/src/modules/deletion/domain/testing/test-config.ts index df76d882d2f..f0ca43c7b16 100644 --- a/apps/server/src/modules/deletion/domain/testing/test-config.ts +++ b/apps/server/src/modules/deletion/domain/testing/test-config.ts @@ -1,9 +1,13 @@ import { Configuration } from '@hpi-schul-cloud/commons'; const deletionConfig = { + ADMIN_API__DELETION_DELETE_AFTER_MINUTES: Configuration.get('ADMIN_API__DELETION_DELETE_AFTER_MINUTES') as number, ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS: Configuration.get( 'ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS' ) as number, + ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS: Configuration.get( + 'ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS' + ) as number, ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS: Configuration.get( 'ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS' ) as number, @@ -14,7 +18,9 @@ const config = () => deletionConfig; export const deletionTestConfig = () => { const conf = config(); - conf.ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS = 1000; + conf.ADMIN_API__DELETION_DELETE_AFTER_MINUTES = 1; + conf.ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS = 100; + conf.ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS = 1000; conf.ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS = 2; conf.ADMIN_API__DELETION_DELAY_MILLISECONDS = 100; return conf; From 3772528cdef6cdb60afb308c1f502e9bb028e980 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 22 Jan 2025 09:38:30 +0100 Subject: [PATCH 05/21] split pending and failed into a separate job --- ...gger-deletion-execution-options.builder.ts | 4 +-- .../deletion-client/deletion.client.ts | 24 ++++++++++---- .../deletion-execution.console.ts | 9 ++++- ...er-deletion-execution-options.interface.ts | 1 + .../uc/deletion-execution.uc.ts | 4 +-- .../deletion-executions.controller.ts | 5 ++- .../deletion/api/uc/deletion-request.uc.ts | 15 ++------- .../service/deletion-request.service.spec.ts | 4 +-- .../service/deletion-request.service.ts | 19 +++-------- .../repo/deletion-request.repo.spec.ts | 6 ++-- .../deletion/repo/deletion-request.repo.ts | 33 +++++++++---------- config/default.schema.json | 2 +- package.json | 2 +- 13 files changed, 65 insertions(+), 63 deletions(-) diff --git a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.ts b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.ts index ed652006e9d..89644fb4887 100644 --- a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.ts +++ b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.ts @@ -1,7 +1,7 @@ import { TriggerDeletionExecutionOptions } from '../interface'; export class TriggerDeletionExecutionOptionsBuilder { - static build(limit: number): TriggerDeletionExecutionOptions { - return { limit }; + static build(limit: number, runFailed: boolean): TriggerDeletionExecutionOptions { + return { limit, runFailed }; } } diff --git a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts index f18fd181ade..bc4f36abe56 100644 --- a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts +++ b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts @@ -26,9 +26,9 @@ export class DeletionClient { } } - public async executeDeletions(limit?: number): Promise { + public async executeDeletions(limit?: number, runFailed?: boolean): Promise { try { - const response = await this.postDeletionExecutionRequest(limit); + const response = await this.postDeletionExecutionRequest(limit, runFailed); this.checkResponseStatusCode(response, HttpStatusCode.NoContent); } catch (err) { throw this.createError(err, 'executeDeletions'); @@ -46,13 +46,25 @@ export class DeletionClient { return response; } - private async postDeletionExecutionRequest(limit?: number): Promise> { + private async postDeletionExecutionRequest(limit?: number, runFailed?: boolean): Promise> { const defaultHeaders = this.createDefaultHeaders(); - const headers = this.isLimitGeaterZero(limit) ? { ...defaultHeaders, params: { limit } } : defaultHeaders; + const baseUrl = this.configService.get('ADMIN_API_CLIENT_BASE_URL', { infer: true }); - const postDeletionExecutionsEndpoint = new URL('/admin/api/v1/deletionExecutions', baseUrl).toString(); + const endpoint = '/admin/api/v1/deletionExecutions'; + + const postDeletionExecutionsEndpoint = new URL(endpoint, baseUrl); + + const params = new URLSearchParams(postDeletionExecutionsEndpoint.search); + if (limit && this.isLimitGeaterZero(limit)) { + params.append('limit', limit.toString()); + } + if (runFailed) { + params.append('runFailed', runFailed.toString()); + } + + const fullUrl = `${postDeletionExecutionsEndpoint.toString()}?${params.toString()}`; - const request = this.httpService.post(postDeletionExecutionsEndpoint, null, headers); + const request = this.httpService.post(fullUrl, null, defaultHeaders); const response = await firstValueFrom(request); return response; diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.ts index fe704c9058e..b58a3c9ce20 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.ts @@ -17,6 +17,11 @@ export class DeletionExecutionConsole { description: 'Limit of the requested deletion executions that should be performed.', required: false, }, + { + flags: '-f, --runFailed ', + description: 'Limit of the requested deletion executions that should be performed.', + required: false, + }, ], }) async triggerDeletionExecution(options: TriggerDeletionExecutionOptions): Promise { @@ -26,7 +31,9 @@ export class DeletionExecutionConsole { let result: DeletionExecutionTriggerResult; try { - await this.deletionExecutionUc.triggerDeletionExecution(options.limit ? Number(options.limit) : undefined); + const limit = options.limit ? Number(options.limit) : undefined; + const runFailed = options.runFailed ?? false; + await this.deletionExecutionUc.triggerDeletionExecution(limit, runFailed); result = DeletionExecutionTriggerResultBuilder.buildSuccess(); } catch (err) { diff --git a/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts b/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts index b17aafa1112..9b91cbbfdd0 100644 --- a/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts +++ b/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts @@ -1,3 +1,4 @@ export interface TriggerDeletionExecutionOptions { limit: number; + runFailed: boolean; } diff --git a/apps/server/src/modules/deletion-console/uc/deletion-execution.uc.ts b/apps/server/src/modules/deletion-console/uc/deletion-execution.uc.ts index 66bcd597d16..4dcc0ed17ea 100644 --- a/apps/server/src/modules/deletion-console/uc/deletion-execution.uc.ts +++ b/apps/server/src/modules/deletion-console/uc/deletion-execution.uc.ts @@ -5,7 +5,7 @@ import { DeletionClient } from '../deletion-client'; export class DeletionExecutionUc { constructor(private readonly deletionClient: DeletionClient) {} - async triggerDeletionExecution(limit?: number): Promise { - await this.deletionClient.executeDeletions(limit); + async triggerDeletionExecution(limit?: number, runFailed?: boolean): Promise { + await this.deletionClient.executeDeletions(limit, runFailed); } } diff --git a/apps/server/src/modules/deletion/api/controller/deletion-executions.controller.ts b/apps/server/src/modules/deletion/api/controller/deletion-executions.controller.ts index 4fad5858ccf..4229edd76e8 100644 --- a/apps/server/src/modules/deletion/api/controller/deletion-executions.controller.ts +++ b/apps/server/src/modules/deletion/api/controller/deletion-executions.controller.ts @@ -19,6 +19,9 @@ export class DeletionExecutionsController { status: 204, }) async executeDeletions(@Query() deletionExecutionQuery: DeletionExecutionParams) { - return this.deletionRequestUc.executeDeletionRequests(deletionExecutionQuery.limit); + return this.deletionRequestUc.executeDeletionRequests( + deletionExecutionQuery.limit, + deletionExecutionQuery.runFailed + ); } } diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index a8ec1619b5d..fdab1ed224b 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -78,27 +78,18 @@ export class DeletionRequestUc implements IEventHandler { return result; } - async executeDeletionRequests(limit?: number): Promise { + async executeDeletionRequests(limit?: number, failed?: boolean): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); - const max = this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); + limit = limit ?? this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); let deletionRequests: DeletionRequest[] = []; do { - // eslint-disable-next-line no-await-in-loop - const pendingCount = await this.deletionRequestService.countPendingDeletionRequests(); - limit = limit ?? max - pendingCount; - - this.logger.debug({ - action: 'numberItemsWithStatusPending, amountWillingToTake', - numberOfDeletionRequestsWithStatusPending: pendingCount, - numberOfDeletionRequestsToProccess: limit, - }); // eslint-disable-next-line no-await-in-loop if (limit > 0) { // eslint-disable-next-line no-await-in-loop - deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit); + deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit, failed); this.logger.debug({ action: 'processing deletion request', deletionRequests }); // eslint-disable-next-line no-await-in-loop diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts index ec1c91e9d91..b33ce212fe1 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts @@ -127,7 +127,7 @@ describe(DeletionRequestService.name, () => { const deletionRequest1 = deletionRequestFactory.build({ deleteAfter: dateInPast }); const deletionRequest2 = deletionRequestFactory.build({ deleteAfter: dateInPast }); - deletionRequestRepo.findAllItemsToExecution.mockResolvedValue([deletionRequest1, deletionRequest2]); + deletionRequestRepo.findAllItems.mockResolvedValue([deletionRequest1, deletionRequest2]); const deletionRequests = [deletionRequest1, deletionRequest2]; @@ -140,7 +140,7 @@ describe(DeletionRequestService.name, () => { await service.findAllItemsToExecute(limit); // TODO fix date comparison - expect(deletionRequestRepo.findAllItemsToExecution).toBeCalledWith(limit, expect.any(Date), expect.any(Date)); + expect(deletionRequestRepo.findAllItems).toBeCalledWith(limit, expect.any(Date), expect.any(Date)); }); it('should return array of two deletionRequests to execute', async () => { diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index d73d5c379f3..a04edd17d98 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -48,25 +48,14 @@ export class DeletionRequestService { return deletionRequest; } - async findAllItemsToExecute(limit: number): Promise { - const deletionRequests = await this.deletionRequestRepo.findAllItemsToExecution( - limit, - this.olderThan, - this.newerThan - ); + async findAllItemsToExecute(limit: number, failed = false): Promise { + const deletionRequests = failed + ? await this.deletionRequestRepo.findAllFailedItems(limit, this.olderThan, this.newerThan) + : await this.deletionRequestRepo.findAllItems(limit); return deletionRequests; } - async countPendingDeletionRequests(): Promise { - const numberItemsWithStatusPending: number = await this.deletionRequestRepo.countPendingDeletionRequests( - this.olderThan, - this.newerThan - ); - - return numberItemsWithStatusPending; - } - async update(deletionRequestToUpdate: DeletionRequest): Promise { await this.deletionRequestRepo.update(deletionRequestToUpdate); } diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts index 1e6a692238c..0cae219b705 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts @@ -117,7 +117,7 @@ describe(DeletionRequestRepo.name, () => { }; it('should return empty array', async () => { const { limit, olderThan, newerThan } = setup(); - const result = await repo.findAllItemsToExecution(limit, olderThan, newerThan); + const result = await repo.findAllItems(limit, olderThan, newerThan); expect(result).toEqual([]); }); @@ -200,7 +200,7 @@ describe(DeletionRequestRepo.name, () => { const { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit, olderThan, newerThan } = await setup(); - const results = await repo.findAllItemsToExecution(limit, olderThan, newerThan); + const results = await repo.findAllItems(limit, olderThan, newerThan); expect(results.length).toEqual(2); @@ -221,7 +221,7 @@ describe(DeletionRequestRepo.name, () => { it('should find deletionRequests to execute with limit = 2', async () => { const { expectedArray, olderThan, newerThan } = await setup(); - const results = await repo.findAllItemsToExecution(2, olderThan, newerThan); + const results = await repo.findAllItems(2, olderThan, newerThan); expect(results.length).toEqual(2); diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index 68c6b96f97d..868a3d90a18 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -32,18 +32,10 @@ export class DeletionRequestRepo { await this.em.flush(); } - async findAllItemsToExecution(limit: number, olderThan: Date, newerThan: Date): Promise { - if (olderThan < newerThan) { - throw new Error('olderThan must be greater than newerThan'); - } + async findAllItems(limit: number): Promise { const scope = new DeletionRequestScope(); scope.byDeleteAfter(new Date()); - - const statusScope = new DeletionRequestScope('$or'); - statusScope.byStatusAndDate([StatusModel.REGISTERED]); - this.addScopeForFailedRequests(statusScope, olderThan, newerThan); - - scope.addQuery(statusScope.query); + scope.byStatusAndDate([StatusModel.REGISTERED]); const order = { createdAt: SortOrder.asc }; @@ -57,17 +49,24 @@ export class DeletionRequestRepo { return mapped; } - private addScopeForFailedRequests(scope: DeletionRequestScope, olderThan: Date, newerThan: Date): void { + async findAllFailedItems(limit: number, olderThan: Date, newerThan: Date): Promise { + if (olderThan < newerThan) { + throw new Error('olderThan must be greater than newerThan'); + } + const scope = new DeletionRequestScope(); + scope.byDeleteAfter(new Date()); scope.byStatusAndDate([StatusModel.FAILED, StatusModel.PENDING], olderThan, newerThan); - } - async countPendingDeletionRequests(olderThan: Date, newerThan: Date): Promise { - const scope = new DeletionRequestScope(); - this.addScopeForFailedRequests(scope, olderThan, newerThan); + const order = { createdAt: SortOrder.asc }; + + const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { + limit, + orderBy: order, + }); - const numberItemsWithStatusPending: number = await this.em.count(DeletionRequestEntity, scope.query); + const mapped: DeletionRequest[] = deletionRequestEntities.map((entity) => DeletionRequestMapper.mapToDO(entity)); - return numberItemsWithStatusPending; + return mapped; } async update(deletionRequest: DeletionRequest): Promise { diff --git a/config/default.schema.json b/config/default.schema.json index cf4757fcd8a..340a5d80456 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -1399,7 +1399,7 @@ }, "DELETION_CONSIDER_FAILED_AFTER_MS" : { "type": "number", - "default": 3600000, + "default": 360000000, "description": "threshold in milliseconds to stop trying to process Pending or Failed deletion requests" }, "DELETION_MAX_CONCURRENT_DELETION_REQUESTS": { diff --git a/package.json b/package.json index 08014c9b30b..78e429117ed 100644 --- a/package.json +++ b/package.json @@ -91,7 +91,7 @@ "nest:start:console:debug": "nest start console --debug --watch --", "nest:start:deletion-console": "nest start deletion-console --", "nest:start:deletion-console:dev": "nest start deletion-console --watch --", - "nest:start:deletion-console:debug": "nest start deletion-console --debug --watch --", + "nest:start:deletion-console:debug": "nest start deletion-console --debug --watch -- execution trigger -f true", "nest:start:idp-console": "nest start idp-console --", "nest:start:idp-console:dev": "nest start idp-console --watch --", "nest:start:idp-console:debug": "nest start idp-console --debug --watch --", From 7b8eccfd7a6b58cfc9df884c29ec99ac8cd425fe Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 22 Jan 2025 17:19:44 +0100 Subject: [PATCH 06/21] implement type casting for command flags --- .../modules/deletion-console/deletion-execution.console.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.ts index b58a3c9ce20..1af6c4f3215 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.ts @@ -14,11 +14,13 @@ export class DeletionExecutionConsole { options: [ { flags: '-l, --limit ', + fn: (value: string) => (value ? Number(value) : undefined), description: 'Limit of the requested deletion executions that should be performed.', required: false, }, { flags: '-f, --runFailed ', + fn: (value: string) => /^(true|yes|1)$/i.test(value), description: 'Limit of the requested deletion executions that should be performed.', required: false, }, @@ -31,10 +33,7 @@ export class DeletionExecutionConsole { let result: DeletionExecutionTriggerResult; try { - const limit = options.limit ? Number(options.limit) : undefined; - const runFailed = options.runFailed ?? false; - await this.deletionExecutionUc.triggerDeletionExecution(limit, runFailed); - + await this.deletionExecutionUc.triggerDeletionExecution(options.limit, options.runFailed); result = DeletionExecutionTriggerResultBuilder.buildSuccess(); } catch (err) { result = DeletionExecutionTriggerResultBuilder.buildFailure(err as Error); From 974567124c81d4c829338f3657bd5d31170be767 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 22 Jan 2025 18:41:25 +0100 Subject: [PATCH 07/21] fix linter / remove tests --- .../api/uc/deletion-request.uc.spec.ts | 16 +---- .../service/deletion-request.service.spec.ts | 27 -------- .../repo/deletion-request.repo.spec.ts | 65 ------------------- 3 files changed, 2 insertions(+), 106 deletions(-) diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 1edbcc12f50..8ee6dbbd5eb 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -17,7 +17,6 @@ import { DeletionRequestLogResponseBuilder } from '../builder'; import { DeletionRequestBodyProps } from '../controller/dto'; import { DeletionLogStatisticBuilder, DeletionTargetRefBuilder } from '../controller/dto/builder'; import { DeletionRequestUc } from './deletion-request.uc'; -import objectContaining = jasmine.objectContaining; describe(DeletionRequestUc.name, () => { let module: TestingModule; @@ -160,17 +159,7 @@ describe(DeletionRequestUc.name, () => { return { deletionRequest }; }; - it('should call deletionRequestService.countPendingDeletionRequests', async () => { - deletionRequestService.countPendingDeletionRequests.mockResolvedValue(0); - - await uc.executeDeletionRequests(); - - expect(deletionRequestService.countPendingDeletionRequests).toHaveBeenCalled(); - }); - it('should call deletionRequestService.findAllItemsToExecute', async () => { - deletionRequestService.countPendingDeletionRequests.mockResolvedValue(0); - await uc.executeDeletionRequests(); expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalled(); @@ -178,7 +167,7 @@ describe(DeletionRequestUc.name, () => { it('should call deletionRequestService.markDeletionRequestAsPending to update status of deletionRequests', async () => { const { deletionRequest } = setup(); - deletionRequestService.countPendingDeletionRequests.mockResolvedValue(0); + deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); await uc.executeDeletionRequests(); @@ -188,7 +177,7 @@ describe(DeletionRequestUc.name, () => { it('should call eventBus.publish', async () => { const { deletionRequest } = setup(); - deletionRequestService.countPendingDeletionRequests.mockResolvedValue(0); + deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); await uc.executeDeletionRequests(); @@ -203,7 +192,6 @@ describe(DeletionRequestUc.name, () => { const setup = () => { const deletionRequestToExecute = deletionRequestFactory.build({ deleteAfter: new Date('2023-01-01') }); - deletionRequestService.countPendingDeletionRequests.mockResolvedValue(0); deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequestToExecute]); eventBus.publish.mockRejectedValueOnce(new Error()); diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts index b33ce212fe1..47e82de4e8f 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts @@ -153,33 +153,6 @@ describe(DeletionRequestService.name, () => { }); }); - describe('countPendingDeletionRequests', () => { - describe('when counting all deletionRequests with status pending', () => { - const setup = () => { - const deletionRequestWithStatusPending = deletionRequestFactory.buildListWithId(5, { - status: StatusModel.PENDING, - }); - - deletionRequestRepo.countPendingDeletionRequests.mockResolvedValue(deletionRequestWithStatusPending.length); - - const numberDeletionRequestsWithStatusPending = deletionRequestWithStatusPending.length; - return { numberDeletionRequestsWithStatusPending }; - }; - - it('should call deletionRequestRepo.countPendingDeletionRequests', async () => { - await service.countPendingDeletionRequests(); - - expect(deletionRequestRepo.countPendingDeletionRequests).toBeCalled(); - }); - - it('should return number of deletionRequests with status pending', async () => { - const { numberDeletionRequestsWithStatusPending } = setup(); - const result = await service.countPendingDeletionRequests(); - - expect(result).toEqual(numberDeletionRequestsWithStatusPending); - }); - }); - }); describe('update', () => { describe('when updating deletionRequest', () => { const setup = () => { diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts index 0cae219b705..f153821aa04 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts @@ -233,71 +233,6 @@ describe(DeletionRequestRepo.name, () => { }); }); - describe('countPendingDeletionRequests', () => { - describe('when there is no deletionRequest with status pending', () => { - const setup = async () => { - const olderThan = new Date(); - olderThan.setDate(olderThan.getDate() + 1); - const newerThan = new Date(); - newerThan.setDate(newerThan.getDate() - 1); - const deletionRequestWithStatusRegistered: DeletionRequestEntity[] = - deletionRequestEntityFactory.buildListWithId(5, { - status: StatusModel.REGISTERED, - }); - - await em.persistAndFlush(deletionRequestWithStatusRegistered); - em.clear(); - - return { - olderThan, - newerThan, - }; - }; - it('should return zero', async () => { - const { olderThan, newerThan } = await setup(); - const result = await repo.countPendingDeletionRequests(olderThan, newerThan); - - expect(result).toEqual(0); - }); - }); - - describe('when there are deletionRequests with status pending', () => { - const setup = async () => { - const olderThan = new Date(); - olderThan.setDate(olderThan.getDate() + 1); - const newerThan = new Date(); - newerThan.setDate(newerThan.getDate() - 1); - - const deletionRequestWithStatusPending: DeletionRequestEntity[] = deletionRequestEntityFactory.buildListWithId( - 5, - { - status: StatusModel.PENDING, - } - ); - - const deletionRequestWithStatusRegistered: DeletionRequestEntity[] = - deletionRequestEntityFactory.buildListWithId(2, { - status: StatusModel.REGISTERED, - }); - - const expectedNumberDeletionRequestWithStatusPending = deletionRequestWithStatusPending.length; - - await em.persistAndFlush([...deletionRequestWithStatusPending, ...deletionRequestWithStatusRegistered]); - em.clear(); - - return { expectedNumberDeletionRequestWithStatusPending, olderThan, newerThan }; - }; - - it('should count deletionRequests with status pending and return proper number', async () => { - const { expectedNumberDeletionRequestWithStatusPending, olderThan, newerThan } = await setup(); - - const result = await repo.countPendingDeletionRequests(olderThan, newerThan); - - expect(result).toEqual(expectedNumberDeletionRequestWithStatusPending); - }); - }); - }); - describe('update', () => { describe('when updating deletionRequest', () => { const setup = async () => { From 26444f0f8b8cb6a9a0293ed01bbcc0c0598af3b1 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 22 Jan 2025 18:42:50 +0100 Subject: [PATCH 08/21] unset db debug mode --- apps/server/src/modules/server/admin-api.server.app.module.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/server/admin-api.server.app.module.ts b/apps/server/src/modules/server/admin-api.server.app.module.ts index 8e8ce19baa1..426b64b8a4a 100644 --- a/apps/server/src/modules/server/admin-api.server.app.module.ts +++ b/apps/server/src/modules/server/admin-api.server.app.module.ts @@ -45,7 +45,7 @@ const serverModules = [ password: DB_PASSWORD, user: DB_USERNAME, entities: [...ALL_ENTITIES, FileEntity], - debug: true, // use it for locally debugging of queries + // debug: true, // use it for locally debugging of queries }), CqrsModule, LoggerModule, From d10659a68698db8ca7d1acf666ae474300784e70 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 22 Jan 2025 18:45:30 +0100 Subject: [PATCH 09/21] undo flag in console --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index bf6e2e2fa3b..e1811b316e7 100644 --- a/package.json +++ b/package.json @@ -91,7 +91,7 @@ "nest:start:console:debug": "nest start console --debug --watch --", "nest:start:deletion-console": "nest start deletion-console --", "nest:start:deletion-console:dev": "nest start deletion-console --watch --", - "nest:start:deletion-console:debug": "nest start deletion-console --debug --watch -- execution trigger -f true", + "nest:start:deletion-console:debug": "nest start deletion-console --debug --watch -- execution trigger", "nest:start:idp-console": "nest start idp-console --", "nest:start:idp-console:dev": "nest start idp-console --watch --", "nest:start:idp-console:debug": "nest start idp-console --debug --watch --", From e8ab58297bac350973c248cf817e7aad1b7ff242 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Thu, 23 Jan 2025 07:24:08 +0100 Subject: [PATCH 10/21] fix test --- .../deletion-console/deletion-execution.console.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts index 833831c5b77..2359007788f 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts @@ -38,7 +38,7 @@ describe(DeletionExecutionConsole.name, () => { const setup = () => { const limit = 1000; - const options = TriggerDeletionExecutionOptionsBuilder.build(1000); + const options = TriggerDeletionExecutionOptionsBuilder.build(1000, false); return { limit, options }; }; @@ -50,13 +50,13 @@ describe(DeletionExecutionConsole.name, () => { await console.triggerDeletionExecution(options); - expect(spy).toBeCalledWith(limit); + expect(spy).toBeCalledWith(limit, false); }); }); describe(`when ${DeletionExecutionUc.name}'s triggerDeletionExecution() method doesn't throw an exception`, () => { const setup = () => { - const options = TriggerDeletionExecutionOptionsBuilder.build(1000); + const options = TriggerDeletionExecutionOptionsBuilder.build(1000, false); deletionExecutionUc.triggerDeletionExecution.mockResolvedValueOnce(undefined); const spy = jest.spyOn(DeletionExecutionTriggerResultBuilder, 'buildSuccess'); @@ -75,7 +75,7 @@ describe(DeletionExecutionConsole.name, () => { describe(`when ${DeletionExecutionUc.name}'s triggerDeletionExecution() method throws an exception`, () => { const setup = () => { - const options = TriggerDeletionExecutionOptionsBuilder.build(1000); + const options = TriggerDeletionExecutionOptionsBuilder.build(1000, false); const err = new Error('some error occurred...'); deletionExecutionUc.triggerDeletionExecution.mockRejectedValueOnce(err); From 8b8e624f3d32983f1afde75e81d86e5cc5c8074e Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Thu, 23 Jan 2025 09:12:15 +0100 Subject: [PATCH 11/21] fix tests --- ...deletion-execution-options.builder.spec.ts | 9 +-- .../api/uc/deletion-request.uc.spec.ts | 3 - .../service/deletion-request.service.spec.ts | 3 +- .../repo/deletion-request.repo.spec.ts | 25 +++----- .../repo/scope/deletion-request-scope.spec.ts | 57 ++++++++++++------- 5 files changed, 52 insertions(+), 45 deletions(-) diff --git a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts index 21171adb405..87ba8d4ec1f 100644 --- a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts +++ b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts @@ -6,16 +6,17 @@ describe(TriggerDeletionExecutionOptionsBuilder.name, () => { describe('when called with proper arguments', () => { const setup = () => { const limit = 1000; + const runFailed = false; - const expectedOutput: TriggerDeletionExecutionOptions = { limit }; + const expectedOutput: TriggerDeletionExecutionOptions = { limit, runFailed }; - return { limit, expectedOutput }; + return { expectedOutput, limit, runFailed }; }; it('should return valid object with expected values', () => { - const { limit, expectedOutput } = setup(); + const { expectedOutput, limit, runFailed } = setup(); - const output = TriggerDeletionExecutionOptionsBuilder.build(limit); + const output = TriggerDeletionExecutionOptionsBuilder.build(limit, runFailed); expect(output).toStrictEqual(expectedOutput); }); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 8ee6dbbd5eb..3baf2a688da 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -61,9 +61,6 @@ describe(DeletionRequestUc.name, () => { deletionRequestService = module.get(DeletionRequestService); deletionLogService = module.get(DeletionLogService); eventBus = module.get(EventBus); - - jest.useFakeTimers(); - jest.setSystemTime(new Date(Date.now())); }); beforeEach(() => { diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts index 47e82de4e8f..5e4676bcc67 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts @@ -139,8 +139,7 @@ describe(DeletionRequestService.name, () => { await service.findAllItemsToExecute(limit); - // TODO fix date comparison - expect(deletionRequestRepo.findAllItems).toBeCalledWith(limit, expect.any(Date), expect.any(Date)); + expect(deletionRequestRepo.findAllItems).toBeCalledWith(limit); }); it('should return array of two deletionRequests to execute', async () => { diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts index f153821aa04..7f71a5af7c3 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts @@ -102,22 +102,18 @@ describe(DeletionRequestRepo.name, () => { }); }); - describe('findAllItemsToExecution', () => { + describe('findAllItems', () => { describe('when there is no deletionRequest for execution', () => { const setup = () => { const limit = 1000; - const olderThan = new Date(); - const newerThan = new Date(); return { limit, - olderThan, - newerThan, }; }; it('should return empty array', async () => { - const { limit, olderThan, newerThan } = setup(); - const result = await repo.findAllItems(limit, olderThan, newerThan); + const { limit } = setup(); + const result = await repo.findAllItems(limit); expect(result).toEqual([]); }); @@ -126,10 +122,6 @@ describe(DeletionRequestRepo.name, () => { describe('when there are deletionRequests for execution', () => { const setup = async () => { const limit = 1000; - const olderThan = new Date(); - olderThan.setDate(olderThan.getDate() + 1); - const newerThan = new Date(); - newerThan.setDate(newerThan.getDate() - 1); const dateInFuture = new Date(); dateInFuture.setDate(dateInFuture.getDate() + 30); @@ -193,14 +185,13 @@ describe(DeletionRequestRepo.name, () => { }, ]; - return { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit, olderThan, newerThan }; + return { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit }; }; it('should find deletionRequests with deleteAfter smaller then today and status with value registered', async () => { - const { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit, olderThan, newerThan } = - await setup(); + const { deletionRequestEntity1, deletionRequestEntity5, expectedArray, limit } = await setup(); - const results = await repo.findAllItems(limit, olderThan, newerThan); + const results = await repo.findAllItems(limit); expect(results.length).toEqual(2); @@ -219,9 +210,9 @@ describe(DeletionRequestRepo.name, () => { }); it('should find deletionRequests to execute with limit = 2', async () => { - const { expectedArray, olderThan, newerThan } = await setup(); + const { expectedArray } = await setup(); - const results = await repo.findAllItems(2, olderThan, newerThan); + const results = await repo.findAllItems(2); expect(results.length).toEqual(2); diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts index 9b779e6a299..7149eebeee7 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts @@ -34,38 +34,57 @@ describe(DeletionRequestScope.name, () => { describe('byStatusAndDate', () => { const setup = () => { - const fifteenMinutesAgo = new Date(Date.now() - 15 * 60 * 1000); // 15 minutes ago - const expectedQuery = { - $or: [ - { status: StatusModel.FAILED }, - { - $and: [ - { status: [StatusModel.REGISTERED, StatusModel.PENDING] }, - { updatedAt: { $lt: fifteenMinutesAgo } }, - ], - }, - ], + const newerThan = new Date(Date.now() - 15 * 60 * 1000); // 15 minutes ago + const olderThan = new Date(Date.now() - 5 * 60 * 1000); // 5 minutes ago + + const expectedQueryOlder = { + status: { + $in: [StatusModel.FAILED, StatusModel.PENDING], + }, + updatedAt: { + $lt: olderThan, + }, + }; + const expectedQueryNewer = { + status: { + $in: [StatusModel.FAILED, StatusModel.PENDING], + }, + updatedAt: { + $gte: newerThan, + }, }; return { - expectedQuery, - fifteenMinutesAgo, + expectedQueryOlder, + expectedQueryNewer, + olderThan, + newerThan, }; }; - describe('when fifteenMinutesAgo is set', () => { + describe('when olderThan is set', () => { it('should add query', () => { - const { expectedQuery, fifteenMinutesAgo } = setup(); + const { expectedQueryOlder, olderThan } = setup(); - const result = scope.byStatusAndDate(StatusModel.FAILED, fifteenMinutesAgo); + const result = scope.byStatusAndDate([StatusModel.FAILED, StatusModel.PENDING], olderThan); expect(result).toBeInstanceOf(DeletionRequestScope); - expect(scope.query).toEqual(expectedQuery); + expect(scope.query).toEqual(expectedQueryOlder); + }); + }); + describe('when newerThan is set', () => { + it('should add query', () => { + const { expectedQueryNewer, newerThan } = setup(); + + const result = scope.byStatusAndDate([StatusModel.FAILED, StatusModel.PENDING], undefined, newerThan); + + expect(result).toBeInstanceOf(DeletionRequestScope); + expect(scope.query).toEqual(expectedQueryNewer); }); }); }); describe('byStatusAndDate for Pending', () => { const setup = () => { - const expectedQuery = { status: [StatusModel.PENDING] }; + const expectedQuery = { status: { $in: [StatusModel.PENDING] } }; return { expectedQuery }; }; @@ -74,7 +93,7 @@ describe(DeletionRequestScope.name, () => { it('should add query', () => { const { expectedQuery } = setup(); - const result = scope.byStatusAndDate(StatusModel.PENDING); + const result = scope.byStatusAndDate([StatusModel.PENDING]); expect(result).toBeInstanceOf(DeletionRequestScope); expect(scope.query).toEqual(expectedQuery); From 86bfdcf3191588573888b116d51f3b23e7d4e4b4 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Thu, 23 Jan 2025 13:31:59 +0100 Subject: [PATCH 12/21] add cronjob --- .../schulcloud-server-core/tasks/main.yml | 18 +++ ...on-trigger-failed-cronjob-configmap.yml.j2 | 15 +++ ...ata-deletion-trigger-failed-cronjob.yml.j2 | 111 ++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob-configmap.yml.j2 create mode 100644 ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob.yml.j2 diff --git a/ansible/roles/schulcloud-server-core/tasks/main.yml b/ansible/roles/schulcloud-server-core/tasks/main.yml index d6d723cafa3..84b0c8fa9cf 100644 --- a/ansible/roles/schulcloud-server-core/tasks/main.yml +++ b/ansible/roles/schulcloud-server-core/tasks/main.yml @@ -240,6 +240,24 @@ tags: - configmap + - name: Data deletion trigger failed CronJob + kubernetes.core.k8s: + kubeconfig: ~/.kube/config + namespace: "{{ NAMESPACE }}" + template: data-deletion-trigger-failed-cronjob.yml.j2 + when: WITH_API_ADMIN is defined and WITH_API_ADMIN|bool + tags: + - cronjob + + - name: Data deletion trigger failed CronJob ConfigMap + kubernetes.core.k8s: + kubeconfig: ~/.kube/config + namespace: "{{ NAMESPACE }}" + template: data-deletion-trigger-failed-cronjob-configmap.yml.j2 + when: WITH_API_ADMIN is defined and WITH_API_ADMIN|bool + tags: + - configmap + - name: amqp files storage Deployment kubernetes.core.k8s: kubeconfig: ~/.kube/config diff --git a/ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob-configmap.yml.j2 b/ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob-configmap.yml.j2 new file mode 100644 index 00000000000..c20477519ed --- /dev/null +++ b/ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob-configmap.yml.j2 @@ -0,0 +1,15 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + namespace: {{ NAMESPACE }} + name: data-deletion-trigger-failed-cronjob-configmap + labels: + app: data-deletion-trigger-failed-cronjob +data: + NODE_OPTIONS: "--max-old-space-size=3072" + NEST_LOG_LEVEL: "error" + EXIT_ON_ERROR: "true" + SC_DOMAIN: "{{ DOMAIN }}" + FEATURE_PROMETHEUS_METRICS_ENABLED: "true" + ETHERPAD__PAD_URI: "https://{{ DOMAIN }}/etherpad/p" + diff --git a/ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob.yml.j2 b/ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob.yml.j2 new file mode 100644 index 00000000000..fcf52dc09da --- /dev/null +++ b/ansible/roles/schulcloud-server-core/templates/data-deletion-trigger-failed-cronjob.yml.j2 @@ -0,0 +1,111 @@ +apiVersion: batch/v1 +kind: CronJob +metadata: + namespace: {{ NAMESPACE }} + labels: + app: data-deletion-trigger-failed + app.kubernetes.io/part-of: schulcloud-verbund + app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }} + app.kubernetes.io/name: data-deletion-trigger-failed + app.kubernetes.io/component: data-deletion + app.kubernetes.io/managed-by: ansible + git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }} + git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }} + name: data-deletion-trigger-failed-cronjob +spec: + concurrencyPolicy: Forbid + schedule: "{{ SERVER_DATA_DELETION_TRIGGER_FAILED_CRONJOB_SCHEDULE|default("@daily", true) }}" + jobTemplate: + metadata: + labels: + app: data-deletion-trigger-failed + app.kubernetes.io/part-of: schulcloud-verbund + app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }} + app.kubernetes.io/name: data-deletion-trigger-failed + app.kubernetes.io/component: data-deletion + app.kubernetes.io/managed-by: ansible + git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }} + git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }} + spec: + template: + spec: + securityContext: + runAsUser: 1000 + runAsGroup: 1000 + fsGroup: 1000 + runAsNonRoot: true + containers: + - name: data-deletion-trigger-failed-cronjob + image: {{ SCHULCLOUD_SERVER_IMAGE }}:{{ SCHULCLOUD_SERVER_IMAGE_TAG }} + envFrom: + - configMapRef: + name: data-deletion-trigger-failed-cronjob-configmap + - secretRef: + name: admin-api-client-secret + - secretRef: + name: api-files-secret + command: ['/bin/sh', '-c'] + args: ['npm run nest:start:deletion-console -- execution trigger -f true'] + resources: + limits: + cpu: {{ API_CPU_LIMITS|default("2000m", true) }} + memory: {{ API_MEMORY_LIMITS|default("2Gi", true) }} + requests: + cpu: {{ API_CPU_REQUESTS|default("100m", true) }} + memory: {{ API_MEMORY_REQUESTS|default("150Mi", true) }} + restartPolicy: OnFailure +{% if AFFINITY_ENABLE is defined and AFFINITY_ENABLE|bool %} + affinity: + podAffinity: + preferredDuringSchedulingIgnoredDuringExecution: + - weight: 20 + podAffinityTerm: + labelSelector: + matchExpressions: + - key: app.kubernetes.io/part-of + operator: In + values: + - schulcloud-verbund + topologyKey: "kubernetes.io/hostname" + namespaceSelector: {} + - weight: 10 + podAffinityTerm: + labelSelector: + matchExpressions: + - key: git.repo + operator: In + values: + - {{ SCHULCLOUD_SERVER_REPO_NAME }} + topologyKey: "kubernetes.io/hostname" + namespaceSelector: {} + - weight: 10 + podAffinityTerm: + labelSelector: + matchExpressions: + - key: git.branch + operator: In + values: + - {{ SCHULCLOUD_SERVER_BRANCH_NAME }} + topologyKey: "kubernetes.io/hostname" + namespaceSelector: {} + - weight: 10 + podAffinityTerm: + labelSelector: + matchExpressions: + - key: app.kubernetes.io/version + operator: In + values: + - {{ SCHULCLOUD_SERVER_IMAGE_TAG }} + topologyKey: "kubernetes.io/hostname" + namespaceSelector: {} +{% endif %} + metadata: + labels: + app: data-deletion-trigger-failed + app.kubernetes.io/part-of: schulcloud-verbund + app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }} + app.kubernetes.io/name: data-deletion-trigger-failed + app.kubernetes.io/component: data-deletion + app.kubernetes.io/managed-by: ansible + git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }} + git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }} \ No newline at end of file From 51ea68e91c9e340fba4b8d949b208a420ec89357 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Thu, 23 Jan 2025 13:57:53 +0100 Subject: [PATCH 13/21] fix test --- apps/server/src/modules/user/service/user.service.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/server/src/modules/user/service/user.service.spec.ts b/apps/server/src/modules/user/service/user.service.spec.ts index ec59a7dc5fd..acbb232e94b 100644 --- a/apps/server/src/modules/user/service/user.service.spec.ts +++ b/apps/server/src/modules/user/service/user.service.spec.ts @@ -851,6 +851,8 @@ describe('UserService', () => { userRepo.findByIdOrNull.mockResolvedValueOnce(user); userRepo.deleteUser.mockResolvedValue(1); + config.get.mockReturnValue(true); + return { expectedResult, user, From e80920a72aa9f8a9c26b5427c931e94db58f6415 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Fri, 24 Jan 2025 14:15:03 +0100 Subject: [PATCH 14/21] improve test cov --- .../deletion-client/deletion.client.spec.ts | 23 +++++++ .../deletion-execution.console.ts | 2 + .../api/uc/deletion-request.uc.spec.ts | 20 +++++- .../repo/deletion-request.repo.spec.ts | 62 +++++++++++++++++++ 4 files changed, 106 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.spec.ts b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.spec.ts index 2a445e9bc4e..44136994f35 100644 --- a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.spec.ts +++ b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.spec.ts @@ -249,5 +249,28 @@ describe(DeletionClient.name, () => { await expect(client.executeDeletions()).rejects.toThrow(Error); }); }); + + describe('when runFailed is true', () => { + const setup = () => { + setupConfig(); + + const response: AxiosResponse = axiosResponseFactory.build({ + status: 204, + }); + + httpService.post.mockReturnValueOnce(of(response)); + + const fullUrl = 'http://api-admin:4030/admin/api/v1/deletionExecutions?runFailed=true'; + + return { fullUrl }; + }; + it('should call endpoint with runFailed param', async () => { + const { fullUrl } = setup(); + + await client.executeDeletions(undefined, true); + + expect(httpService.post).toHaveBeenCalledWith(fullUrl, null, expect.any(Object)); + }); + }); }); }); diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.ts index 1af6c4f3215..6915420b2fd 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.ts @@ -14,12 +14,14 @@ export class DeletionExecutionConsole { options: [ { flags: '-l, --limit ', + /* istanbul ignore next */ fn: (value: string) => (value ? Number(value) : undefined), description: 'Limit of the requested deletion executions that should be performed.', required: false, }, { flags: '-f, --runFailed ', + /* istanbul ignore next */ fn: (value: string) => /^(true|yes|1)$/i.test(value), description: 'Limit of the requested deletion executions that should be performed.', required: false, diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index c61bcba60e6..040f2180fb2 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -1,7 +1,7 @@ import { LegacyLogger } from '@core/logger'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { MikroORM } from '@mikro-orm/core'; -import { ConfigModule } from '@nestjs/config'; +import { ConfigModule, ConfigService } from '@nestjs/config'; import { EventBus } from '@nestjs/cqrs'; import { Test, TestingModule } from '@nestjs/testing'; import { createConfigModuleOptions } from '@shared/common/config-module-options'; @@ -17,6 +17,7 @@ import { DeletionRequestLogResponseBuilder } from '../builder'; import { DeletionRequestBodyProps } from '../controller/dto'; import { DeletionLogStatisticBuilder, DeletionTargetRefBuilder } from '../controller/dto/builder'; import { DeletionRequestUc } from './deletion-request.uc'; +import { DeletionConfig } from '../../deletion.config'; describe(DeletionRequestUc.name, () => { let module: TestingModule; @@ -24,6 +25,7 @@ describe(DeletionRequestUc.name, () => { let deletionRequestService: DeepMocked; let deletionLogService: DeepMocked; let eventBus: DeepMocked; + let configService: DeepMocked>; beforeAll(async () => { const orm = await setupEntities(); @@ -54,6 +56,10 @@ describe(DeletionRequestUc.name, () => { provide: MikroORM, useValue: orm, }, + { + provide: ConfigService, + useValue: createMock(), + }, ], }).compile(); @@ -61,6 +67,7 @@ describe(DeletionRequestUc.name, () => { deletionRequestService = module.get(DeletionRequestService); deletionLogService = module.get(DeletionLogService); eventBus = module.get(EventBus); + configService = module.get(ConfigService); }); beforeEach(() => { @@ -183,6 +190,17 @@ describe(DeletionRequestUc.name, () => { new UserDeletedEvent(deletionRequest.id, deletionRequest.targetRefId) ); }); + + it('should work with a delay of 0', async () => { + const { deletionRequest } = setup(); + configService.get.mockReturnValueOnce(0); + + deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); + + await uc.executeDeletionRequests(); + + expect(deletionRequestService.markDeletionRequestAsFailed).not.toHaveBeenCalled(); + }); }); describe('when an error occurred', () => { diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts index 7f71a5af7c3..71043a9597b 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts @@ -224,6 +224,68 @@ describe(DeletionRequestRepo.name, () => { }); }); + describe('findAllFailedItems', () => { + const setup = async () => { + const limit = 10; + const olderThan = new Date(2023, 11, 1); + const newerThan = new Date(2023, 8, 1); + + const deletionRequestEntity1: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 7, 1), + updatedAt: new Date(2023, 8, 2), + deleteAfter: new Date(2023, 8, 1), + status: StatusModel.PENDING, + }); + const deletionRequestEntity2: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 7, 1), + updatedAt: new Date(2023, 8, 2), + deleteAfter: new Date(2023, 8, 1), + status: StatusModel.FAILED, + }); + const deletionRequestEntity3: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 8, 1), + updatedAt: new Date(2023, 7, 1), + deleteAfter: new Date(2023, 9, 1), + status: StatusModel.FAILED, + }); + const deletionRequestEntity4: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 8, 1), + updatedAt: new Date(2023, 8, 1), + deleteAfter: new Date(2023, 9, 1), + status: StatusModel.REGISTERED, + }); + + await em.persistAndFlush([ + deletionRequestEntity1, + deletionRequestEntity2, + deletionRequestEntity3, + deletionRequestEntity4, + ]); + em.clear(); + + return { limit, olderThan, newerThan, deletionRequestEntity1, deletionRequestEntity2 }; + }; + + it('should throw an error if olderThan is less than newerThan', async () => { + const { limit } = await setup(); + + const olderThan = new Date(2023, 10, 1); + const newerThan = new Date(2023, 11, 1); + + await expect(repo.findAllFailedItems(limit, olderThan, newerThan)).rejects.toThrow(); + }); + + it('should return failed deletion requests within the specified date range', async () => { + const { limit, olderThan, newerThan, deletionRequestEntity1, deletionRequestEntity2 } = await setup(); + + const results = await repo.findAllFailedItems(limit, olderThan, newerThan); + + expect(results.length).toBe(2); + expect(results[0].id).toBe(deletionRequestEntity1.id); + expect(results[1].id).toBe(deletionRequestEntity2.id); + }); + }); + describe('update', () => { describe('when updating deletionRequest', () => { const setup = async () => { From a42c73b3770c3623cd34007d60f24d6f8c7a97ea Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Fri, 24 Jan 2025 17:17:01 +0100 Subject: [PATCH 15/21] fixes from revviews --- ...deletion-execution-options.builder.spec.ts | 9 +++----- .../deletion-client/deletion.client.ts | 4 ++-- .../deletion-request-create.api.spec.ts | 8 +++---- .../api/uc/deletion-request.uc.spec.ts | 17 ++++++++++---- .../deletion/api/uc/deletion-request.uc.ts | 23 +++++++++---------- .../service/deletion-request.service.ts | 4 ++-- .../deletion/repo/deletion-request.repo.ts | 4 ++-- config/default.schema.json | 4 ++-- 8 files changed, 38 insertions(+), 35 deletions(-) diff --git a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts index 87ba8d4ec1f..0b4eb6cd53c 100644 --- a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts +++ b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts @@ -1,4 +1,3 @@ -import { TriggerDeletionExecutionOptions } from '../interface'; import { TriggerDeletionExecutionOptionsBuilder } from './trigger-deletion-execution-options.builder'; describe(TriggerDeletionExecutionOptionsBuilder.name, () => { @@ -8,17 +7,15 @@ describe(TriggerDeletionExecutionOptionsBuilder.name, () => { const limit = 1000; const runFailed = false; - const expectedOutput: TriggerDeletionExecutionOptions = { limit, runFailed }; - - return { expectedOutput, limit, runFailed }; + return { limit, runFailed }; }; it('should return valid object with expected values', () => { - const { expectedOutput, limit, runFailed } = setup(); + const { limit, runFailed } = setup(); const output = TriggerDeletionExecutionOptionsBuilder.build(limit, runFailed); - expect(output).toStrictEqual(expectedOutput); + expect(output).toStrictEqual({ limit, runFailed }); }); }); }); diff --git a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts index bc4f36abe56..c37fd77062d 100644 --- a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts +++ b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts @@ -55,7 +55,7 @@ export class DeletionClient { const postDeletionExecutionsEndpoint = new URL(endpoint, baseUrl); const params = new URLSearchParams(postDeletionExecutionsEndpoint.search); - if (limit && this.isLimitGeaterZero(limit)) { + if (limit && this.isLimitGreaterZero(limit)) { params.append('limit', limit.toString()); } if (runFailed) { @@ -99,7 +99,7 @@ export class DeletionClient { } } - private isLimitGeaterZero(limit?: number): boolean { + private isLimitGreaterZero(limit?: number): boolean { return typeof limit === 'number' && limit > 0; } diff --git a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts index e2d09c9be53..6b7324a95f4 100644 --- a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts +++ b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts @@ -88,14 +88,14 @@ describe(`deletionRequest create (api)`, () => { deleteAfterMinutes: 0, }; - const defaultdeleteAfterMinutes = 43200; + const defaultDeleteAfterMinutes = 43200; const operationalTimeToleranceInSeconds = 30; return { deletionRequestToCreate, deletionRequestToImmediateRemoval, - defaultdeleteAfterMinutes, + defaultDeleteAfterMinutes, operationalTimeToleranceInSeconds, }; }; @@ -119,7 +119,7 @@ describe(`deletionRequest create (api)`, () => { describe('when the "delete after minutes" param has not been provided', () => { it('should set the "deleteAfter" date to the date after the default DELETION_DELETE_AFTER_MINUTES ', async () => { - const { deletionRequestToCreate, defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); + const { deletionRequestToCreate, defaultDeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); const response = await testApiClient.post('', deletionRequestToCreate); @@ -131,7 +131,7 @@ describe(`deletionRequest create (api)`, () => { const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange( createdItem.createdAt, createdItem.deleteAfter, - defaultdeleteAfterMinutes, + defaultDeleteAfterMinutes, operationalTimeToleranceInSeconds ); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 040f2180fb2..f2235990d77 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -1,10 +1,9 @@ import { LegacyLogger } from '@core/logger'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { MikroORM } from '@mikro-orm/core'; -import { ConfigModule, ConfigService } from '@nestjs/config'; +import { ConfigService } from '@nestjs/config'; import { EventBus } from '@nestjs/cqrs'; import { Test, TestingModule } from '@nestjs/testing'; -import { createConfigModuleOptions } from '@shared/common/config-module-options'; import { setupEntities } from '@testing/setup-entities'; import { ObjectId } from 'bson'; import { DomainDeletionReportBuilder, DomainOperationReportBuilder } from '../../domain/builder'; @@ -31,7 +30,6 @@ describe(DeletionRequestUc.name, () => { const orm = await setupEntities(); module = await Test.createTestingModule({ - imports: [ConfigModule.forRoot(createConfigModuleOptions(deletionTestConfig))], providers: [ DeletionRequestUc, { @@ -161,9 +159,13 @@ describe(DeletionRequestUc.name, () => { const setup = () => { const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); + configService.get.mockImplementation((key) => deletionTestConfig()[key]); + return { deletionRequest }; }; it('should call deletionRequestService.findAllItemsToExecute', async () => { + setup(); + await uc.executeDeletionRequests(); expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalled(); @@ -192,8 +194,13 @@ describe(DeletionRequestUc.name, () => { }); it('should work with a delay of 0', async () => { - const { deletionRequest } = setup(); - configService.get.mockReturnValueOnce(0); + const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); + configService.get.mockImplementation((key) => { + if (key === 'ADMIN_API__DELETION_DELAY_MILLISECONDS') { + return 0; + } + return deletionTestConfig()[key]; + }); deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index fdab1ed224b..7c565cecc78 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -17,7 +17,7 @@ import { DeletionConfig } from '../../deletion.config'; @Injectable() @EventsHandler(DataDeletedEvent) export class DeletionRequestUc implements IEventHandler { - config: string[]; + private config: string[]; constructor( private readonly deletionRequestService: DeletionRequestService, @@ -78,31 +78,31 @@ export class DeletionRequestUc implements IEventHandler { return result; } - async executeDeletionRequests(limit?: number, failed?: boolean): Promise { + async executeDeletionRequests(limit?: number, getFailed?: boolean): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); limit = limit ?? this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); let deletionRequests: DeletionRequest[] = []; - do { - // eslint-disable-next-line no-await-in-loop - if (limit > 0) { + if (limit > 0) { + do { // eslint-disable-next-line no-await-in-loop - deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit, failed); + deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit, getFailed); this.logger.debug({ action: 'processing deletion request', deletionRequests }); + // eslint-disable-next-line no-await-in-loop await Promise.all( deletionRequests.map(async (req) => { await this.executeDeletionRequest(req); }) ); - } - // eslint-disable-next-line no-await-in-loop - await this.sleep(); - } while (deletionRequests.length > 0); + // eslint-disable-next-line no-await-in-loop + await this.delayForDeletion(); + } while (deletionRequests.length > 0); + } this.logger.debug({ action: 'deletion process completed' }); } @@ -143,8 +143,7 @@ export class DeletionRequestUc implements IEventHandler { } } - // short sleep mode to give time for deletion process to do its work - private async sleep() { + private async delayForDeletion() { const delay = this.configService.get('ADMIN_API__DELETION_DELAY_MILLISECONDS'); if (delay > 0) { return new Promise((resolve) => { diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index a04edd17d98..ba1e336df61 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -48,8 +48,8 @@ export class DeletionRequestService { return deletionRequest; } - async findAllItemsToExecute(limit: number, failed = false): Promise { - const deletionRequests = failed + async findAllItemsToExecute(limit: number, getFailed = false): Promise { + const deletionRequests = getFailed ? await this.deletionRequestRepo.findAllFailedItems(limit, this.olderThan, this.newerThan) : await this.deletionRequestRepo.findAllItems(limit); diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index 868a3d90a18..3bf50d79e4c 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -39,7 +39,7 @@ export class DeletionRequestRepo { const order = { createdAt: SortOrder.asc }; - const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { + const deletionRequestEntities = await this.em.find(DeletionRequestEntity, scope.query, { limit, orderBy: order, }); @@ -59,7 +59,7 @@ export class DeletionRequestRepo { const order = { createdAt: SortOrder.asc }; - const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { + const deletionRequestEntities = await this.em.find(DeletionRequestEntity, scope.query, { limit, orderBy: order, }); diff --git a/config/default.schema.json b/config/default.schema.json index 50fd5715fa6..71bae7e907c 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -1395,7 +1395,7 @@ "DELETION_DELETE_AFTER_MINUTES": { "type": "number", "default": 43200, - "description": "threshold in minutes, after which a deletion requests can be processed" + "description": "threshold in minutes, after which a deletion request can be processed" }, "DELETION_MODIFICATION_THRESHOLD_MS": { "type": "number", @@ -1415,7 +1415,7 @@ "DELETION_DELAY_MILLISECONDS": { "type": "number", "default": 5000, - "description": "delay in milliseconds to sleep before processing another batch of of deletion requests" + "description": "delay in milliseconds to sleep before processing another batch of deletion requests" } }, "default": {} From e216fc9399e955d5f542a3cdc122a6a0599a090f Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Fri, 24 Jan 2025 19:26:05 +0100 Subject: [PATCH 16/21] fix query --- .../deletion-execution.console.ts | 4 +- .../repo/scope/deletion-request-scope.spec.ts | 63 +++++++++++++++++-- .../repo/scope/deletion-request-scope.ts | 17 ++--- 3 files changed, 70 insertions(+), 14 deletions(-) diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.ts index 6915420b2fd..4d97390130e 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.ts @@ -15,14 +15,14 @@ export class DeletionExecutionConsole { { flags: '-l, --limit ', /* istanbul ignore next */ - fn: (value: string) => (value ? Number(value) : undefined), + fn: (value: string) => (value ? Number(value) : undefined), // NOSONAR description: 'Limit of the requested deletion executions that should be performed.', required: false, }, { flags: '-f, --runFailed ', /* istanbul ignore next */ - fn: (value: string) => /^(true|yes|1)$/i.test(value), + fn: (value: string) => /^(true|yes|1)$/i.test(value), // NOSONAR description: 'Limit of the requested deletion executions that should be performed.', required: false, }, diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts index 7149eebeee7..2966cbd62e8 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.spec.ts @@ -41,21 +41,54 @@ describe(DeletionRequestScope.name, () => { status: { $in: [StatusModel.FAILED, StatusModel.PENDING], }, - updatedAt: { - $lt: olderThan, - }, + $and: [ + { + updatedAt: { + $lt: olderThan, + }, + }, + ], }; const expectedQueryNewer = { status: { $in: [StatusModel.FAILED, StatusModel.PENDING], }, - updatedAt: { - $gte: newerThan, + $and: [ + { + updatedAt: { + $gte: newerThan, + }, + }, + ], + }; + const expectedQueryOlderAndNewer = { + status: { + $in: [StatusModel.FAILED, StatusModel.PENDING], }, + $and: [ + { + updatedAt: { + $lt: olderThan, + }, + }, + { + updatedAt: { + $gte: newerThan, + }, + }, + ], }; + const expectedQueryNoDates = { + status: { + $in: [StatusModel.FAILED, StatusModel.PENDING], + }, + }; + return { expectedQueryOlder, expectedQueryNewer, + expectedQueryOlderAndNewer, + expectedQueryNoDates, olderThan, newerThan, }; @@ -80,6 +113,26 @@ describe(DeletionRequestScope.name, () => { expect(scope.query).toEqual(expectedQueryNewer); }); }); + describe('when olderThan and newerThan are set', () => { + it('should add query', () => { + const { expectedQueryOlderAndNewer, olderThan, newerThan } = setup(); + + const result = scope.byStatusAndDate([StatusModel.FAILED, StatusModel.PENDING], olderThan, newerThan); + + expect(result).toBeInstanceOf(DeletionRequestScope); + expect(scope.query).toEqual(expectedQueryOlderAndNewer); + }); + }); + describe('when olderThan and newerThan are not set', () => { + it('should add query', () => { + const { expectedQueryNoDates } = setup(); + + const result = scope.byStatusAndDate([StatusModel.FAILED, StatusModel.PENDING]); + + expect(result).toBeInstanceOf(DeletionRequestScope); + expect(scope.query).toEqual(expectedQueryNoDates); + }); + }); }); describe('byStatusAndDate for Pending', () => { diff --git a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts index 116c8aca393..5c0d00f5415 100644 --- a/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts +++ b/apps/server/src/modules/deletion/repo/scope/deletion-request-scope.ts @@ -1,4 +1,5 @@ import { Scope } from '@shared/repo/scope'; +import { FilterQuery } from '@mikro-orm/core'; import { DeletionRequestEntity } from '../entity'; import { StatusModel } from '../../domain/types'; @@ -10,19 +11,21 @@ export class DeletionRequestScope extends Scope { } byStatusAndDate(status: StatusModel[], olderThan?: Date, newerThan?: Date): this { - let query = { status: { $in: status } }; + const query: FilterQuery = { status: { $in: status } }; + + const dateConditions: FilterQuery[] = []; if (olderThan) { - const olderThanQuery = { updatedAt: { $lt: olderThan } }; - query = { ...query, ...olderThanQuery }; + dateConditions.push({ updatedAt: { $lt: olderThan } }); } - if (newerThan) { - const newerThanQuery = { updatedAt: { $gte: newerThan } }; - query = { ...query, ...newerThanQuery }; + dateConditions.push({ updatedAt: { $gte: newerThan } }); } - this.addQuery(query); + if (dateConditions.length > 0) { + query.$and = dateConditions; + } + this.addQuery(query); return this; } } From 92dc292ab903fc0812c5414c3a65249fb711c570 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Mon, 27 Jan 2025 11:46:03 +0100 Subject: [PATCH 17/21] hacky type casting for console args --- .../deletion-execution.console.spec.ts | 25 +++++++++++++++++++ .../deletion-execution.console.ts | 18 +++++++++---- ...er-deletion-execution-options.interface.ts | 4 +-- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts index 2359007788f..27eb1850c5e 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.spec.ts @@ -6,6 +6,7 @@ import { DeletionExecutionConsole } from './deletion-execution.console'; import { DeletionExecutionTriggerResultBuilder, TriggerDeletionExecutionOptionsBuilder } from './builder'; import { DeletionExecutionUc } from './uc'; import { DeletionConsoleModule } from './deletion-console.app.module'; +import { TriggerDeletionExecutionOptions } from './interface'; describe(DeletionExecutionConsole.name, () => { let module: TestingModule; @@ -92,5 +93,29 @@ describe(DeletionExecutionConsole.name, () => { expect(spy).toHaveBeenCalledWith(err); }); }); + + describe('when runFailed is given as string "true"', () => { + it('should convert runFailed to boolean', async () => { + const options = { limit: 1000, runFailed: 'true' } as unknown as TriggerDeletionExecutionOptions; + + const spy = jest.spyOn(deletionExecutionUc, 'triggerDeletionExecution').mockResolvedValueOnce(undefined); + + await console.triggerDeletionExecution(options); + + expect(spy).toHaveBeenCalledWith(1000, true); + }); + }); + + describe('when limit is a string "5"', () => { + it('should convert limit to number', async () => { + const options = { limit: '5', runFailed: false } as unknown as TriggerDeletionExecutionOptions; + + const spy = jest.spyOn(deletionExecutionUc, 'triggerDeletionExecution').mockResolvedValueOnce(undefined); + + await console.triggerDeletionExecution(options); + + expect(spy).toHaveBeenCalledWith(5, false); + }); + }); }); }); diff --git a/apps/server/src/modules/deletion-console/deletion-execution.console.ts b/apps/server/src/modules/deletion-console/deletion-execution.console.ts index 4d97390130e..fade3e3b872 100644 --- a/apps/server/src/modules/deletion-console/deletion-execution.console.ts +++ b/apps/server/src/modules/deletion-console/deletion-execution.console.ts @@ -14,26 +14,34 @@ export class DeletionExecutionConsole { options: [ { flags: '-l, --limit ', - /* istanbul ignore next */ - fn: (value: string) => (value ? Number(value) : undefined), // NOSONAR + // TODO implemnt fn with proper command testing + // fn: (value: string) => (value ? Number(value) : undefined), description: 'Limit of the requested deletion executions that should be performed.', required: false, }, { flags: '-f, --runFailed ', - /* istanbul ignore next */ - fn: (value: string) => /^(true|yes|1)$/i.test(value), // NOSONAR + // TODO implemnt fn with proper command testing + // fn: (value: string) => /^(true|yes|1)$/i.test(value), description: 'Limit of the requested deletion executions that should be performed.', required: false, }, ], }) - async triggerDeletionExecution(options: TriggerDeletionExecutionOptions): Promise { + public async triggerDeletionExecution(options: TriggerDeletionExecutionOptions): Promise { // Try to trigger the deletion execution(s) via Deletion API client, // return successful status in case of a success, otherwise return // a result with a failure status and a proper error message. let result: DeletionExecutionTriggerResult; + if (typeof options.limit === 'string') { + options.limit = Number(options.limit); + } + + if (typeof options.runFailed === 'string') { + options.runFailed = /^(true|yes|1)$/i.test(options.runFailed); + } + try { await this.deletionExecutionUc.triggerDeletionExecution(options.limit, options.runFailed); result = DeletionExecutionTriggerResultBuilder.buildSuccess(); diff --git a/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts b/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts index 9b91cbbfdd0..14d69b46c1f 100644 --- a/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts +++ b/apps/server/src/modules/deletion-console/interface/trigger-deletion-execution-options.interface.ts @@ -1,4 +1,4 @@ export interface TriggerDeletionExecutionOptions { - limit: number; - runFailed: boolean; + limit: string | number; + runFailed: string | boolean; } From a7e13c7b96996b804dbec01fc88b9cdf1b96582a Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Mon, 27 Jan 2025 15:03:42 +0100 Subject: [PATCH 18/21] undo package.json command --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 379326ef77c..42b8b2e2966 100644 --- a/package.json +++ b/package.json @@ -91,7 +91,7 @@ "nest:start:console:debug": "nest start console --debug --watch --", "nest:start:deletion-console": "nest start deletion-console --", "nest:start:deletion-console:dev": "nest start deletion-console --watch --", - "nest:start:deletion-console:debug": "nest start deletion-console --debug --watch -- execution trigger", + "nest:start:deletion-console:debug": "nest start deletion-console --debug --watch --", "nest:start:idp-console": "nest start idp-console --", "nest:start:idp-console:dev": "nest start idp-console --watch --", "nest:start:idp-console:debug": "nest start idp-console --debug --watch --", From c40c290e24eb37cc625ec25d72b0b53a43952511 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Tue, 28 Jan 2025 15:59:08 +0100 Subject: [PATCH 19/21] throttle down max number of processing requests in batch avoid loading node by checking how many requests are already currently in progress --- .../api/uc/deletion-request.uc.spec.ts | 12 ++++- .../deletion/api/uc/deletion-request.uc.ts | 10 ++-- .../service/deletion-request.service.spec.ts | 30 ++++++++++++ .../service/deletion-request.service.ts | 7 ++- .../repo/deletion-request.repo.spec.ts | 49 +++++++++++++++++++ .../deletion/repo/deletion-request.repo.ts | 9 ++++ 6 files changed, 112 insertions(+), 5 deletions(-) diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index f2235990d77..7e71320212a 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -160,15 +160,23 @@ describe(DeletionRequestUc.name, () => { const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); configService.get.mockImplementation((key) => deletionTestConfig()[key]); + deletionRequestService.findInProgressCount.mockResolvedValueOnce(1); return { deletionRequest }; }; + it('should call deletionRequestService.findInProgressCount', async () => { + setup(); + + await uc.executeDeletionRequests(); + + expect(deletionRequestService.findInProgressCount).toHaveBeenCalled(); + }); it('should call deletionRequestService.findAllItemsToExecute', async () => { setup(); await uc.executeDeletionRequests(); - expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalled(); + expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalledWith(1, undefined); }); it('should call deletionRequestService.markDeletionRequestAsPending to update status of deletionRequests', async () => { @@ -203,6 +211,7 @@ describe(DeletionRequestUc.name, () => { }); deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); + deletionRequestService.findInProgressCount.mockResolvedValueOnce(0); await uc.executeDeletionRequests(); @@ -215,6 +224,7 @@ describe(DeletionRequestUc.name, () => { const deletionRequestToExecute = deletionRequestFactory.build({ deleteAfter: new Date('2023-01-01') }); deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequestToExecute]); + deletionRequestService.findInProgressCount.mockResolvedValueOnce(1); eventBus.publish.mockRejectedValueOnce(new Error()); return { diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index 7c565cecc78..dbd0edc76f5 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -81,14 +81,18 @@ export class DeletionRequestUc implements IEventHandler { async executeDeletionRequests(limit?: number, getFailed?: boolean): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); - limit = limit ?? this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); + const inProgress = await this.deletionRequestService.findInProgressCount(); + + const max = limit + ? limit - inProgress + : this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS') - inProgress; let deletionRequests: DeletionRequest[] = []; - if (limit > 0) { + if (max > 0) { do { // eslint-disable-next-line no-await-in-loop - deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit, getFailed); + deletionRequests = await this.deletionRequestService.findAllItemsToExecute(max, getFailed); this.logger.debug({ action: 'processing deletion request', deletionRequests }); diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts index 2e9e209e92c..bae7a31adbe 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.spec.ts @@ -152,6 +152,36 @@ describe(DeletionRequestService.name, () => { }); }); + describe('findInProgressCount', () => { + describe('when finding in progress deletionRequests', () => { + const setup = () => { + const thresholdNewerMs = configService.get('ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS') ?? 1000; + const newerThan = new Date(Date.now() - thresholdNewerMs); + + const count = 2; + + deletionRequestRepo.findInProgressCount.mockResolvedValue(count); + + return { count, newerThan }; + }; + + it('should call deletionRequestRepo.findInProgressCount', async () => { + const { newerThan } = setup(); + + await service.findInProgressCount(); + + expect(deletionRequestRepo.findInProgressCount).toBeCalledWith(newerThan); + }); + + it('should return count of in progress deletionRequests', async () => { + const { count } = setup(); + const result = await service.findInProgressCount(); + + expect(result).toEqual(count); + }); + }); + }); + describe('update', () => { describe('when updating deletionRequest', () => { const setup = () => { diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index ba1e336df61..abea479f654 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -48,7 +48,7 @@ export class DeletionRequestService { return deletionRequest; } - async findAllItemsToExecute(limit: number, getFailed = false): Promise { + public async findAllItemsToExecute(limit: number, getFailed = false): Promise { const deletionRequests = getFailed ? await this.deletionRequestRepo.findAllFailedItems(limit, this.olderThan, this.newerThan) : await this.deletionRequestRepo.findAllItems(limit); @@ -56,6 +56,11 @@ export class DeletionRequestService { return deletionRequests; } + public async findInProgressCount(): Promise { + const count = await this.deletionRequestRepo.findInProgressCount(this.newerThan); + return count; + } + async update(deletionRequestToUpdate: DeletionRequest): Promise { await this.deletionRequestRepo.update(deletionRequestToUpdate); } diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts index 71043a9597b..4021cc91a3e 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.spec.ts @@ -286,6 +286,55 @@ describe(DeletionRequestRepo.name, () => { }); }); + describe('findInProgressCount', () => { + const setup = async () => { + const newerThan = new Date(2023, 8, 1); + + const deletionRequestEntity1: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 7, 1), + updatedAt: new Date(2023, 8, 2), + deleteAfter: new Date(2023, 8, 1), + status: StatusModel.PENDING, + }); + const deletionRequestEntity2: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 7, 1), + updatedAt: new Date(2023, 8, 2), + deleteAfter: new Date(2023, 8, 1), + status: StatusModel.FAILED, + }); + const deletionRequestEntity3: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 8, 1), + updatedAt: new Date(2023, 7, 1), + deleteAfter: new Date(2023, 9, 1), + status: StatusModel.FAILED, + }); + const deletionRequestEntity4: DeletionRequestEntity = deletionRequestEntityFactory.build({ + createdAt: new Date(2023, 8, 1), + updatedAt: new Date(2023, 8, 1), + deleteAfter: new Date(2023, 9, 1), + status: StatusModel.REGISTERED, + }); + + await em.persistAndFlush([ + deletionRequestEntity1, + deletionRequestEntity2, + deletionRequestEntity3, + deletionRequestEntity4, + ]); + em.clear(); + + return { newerThan }; + }; + + it('should return the count of deletionRequests with status pending or failed', async () => { + const { newerThan } = await setup(); + + const result = await repo.findInProgressCount(newerThan); + + expect(result).toBe(2); + }); + }); + describe('update', () => { describe('when updating deletionRequest', () => { const setup = async () => { diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index 3bf50d79e4c..986129ecd1f 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -69,6 +69,15 @@ export class DeletionRequestRepo { return mapped; } + public async findInProgressCount(newerThan: Date): Promise { + const scope = new DeletionRequestScope(); + scope.byStatusAndDate([StatusModel.PENDING, StatusModel.FAILED], undefined, newerThan); + + const count = await this.em.count(DeletionRequestEntity, scope.query); + + return count; + } + async update(deletionRequest: DeletionRequest): Promise { const deletionRequestEntity = DeletionRequestMapper.mapToEntity(deletionRequest); const referencedEntity = this.em.getReference(DeletionRequestEntity, deletionRequestEntity.id); From 6adc0161de603ca09202924317472ad834e61b33 Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 29 Jan 2025 14:44:29 +0100 Subject: [PATCH 20/21] fix max concurency / limit and some tests --- .../api/uc/deletion-request.uc.spec.ts | 68 ++++++++++++++++--- .../deletion/api/uc/deletion-request.uc.ts | 23 ++++--- 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 7e71320212a..40c273c7389 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -66,6 +66,8 @@ describe(DeletionRequestUc.name, () => { deletionLogService = module.get(DeletionLogService); eventBus = module.get(EventBus); configService = module.get(ConfigService); + + jest.useFakeTimers(); }); beforeEach(() => { @@ -157,10 +159,29 @@ describe(DeletionRequestUc.name, () => { describe('executeDeletionRequests', () => { describe('when deletionRequests to execute exists', () => { const setup = () => { - const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); + deletionRequestService.findInProgressCount.mockResolvedValue(1); - configService.get.mockImplementation((key) => deletionTestConfig()[key]); - deletionRequestService.findInProgressCount.mockResolvedValueOnce(1); + configService.get.mockImplementation((key) => { + if (key === 'ADMIN_API__DELETION_DELETE_AFTER_MINUTES') { + return 1; + } + if (key === 'ADMIN_API__DELETION_MODIFICATION_THRESHOLD_MS') { + return 100; + } + if (key === 'ADMIN_API__DELETION_CONSIDER_FAILED_AFTER_MS') { + return 1000; + } + if (key === 'ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS') { + return 2; + } + if (key === 'ADMIN_API__DELETION_DELAY_MILLISECONDS') { + return 100; + } + }); + + const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); + deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); + deletionRequestService.markDeletionRequestAsPending.mockResolvedValueOnce(true); return { deletionRequest }; }; @@ -171,7 +192,8 @@ describe(DeletionRequestUc.name, () => { expect(deletionRequestService.findInProgressCount).toHaveBeenCalled(); }); - it('should call deletionRequestService.findAllItemsToExecute', async () => { + + it('should call deletionRequestService.findAllItemsToExecute with correct limit', async () => { setup(); await uc.executeDeletionRequests(); @@ -179,11 +201,39 @@ describe(DeletionRequestUc.name, () => { expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalledWith(1, undefined); }); + describe('when limit is given', () => { + it('should call deletionRequestService.findAllItemsToExecute with correct limit', async () => { + setup(); + + await uc.executeDeletionRequests(10); + + expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalledWith(9, undefined); + }); + }); + + describe('when limit is not given', () => { + it('should call deletionRequestService.findAllItemsToExecute with correct limit', async () => { + setup(); + + await uc.executeDeletionRequests(); + + expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalledWith(1, undefined); + }); + }); + + describe('when getFailed is true', () => { + it('should call deletionRequestService.findAllItemsToExecute with getFailed true', async () => { + setup(); + + await uc.executeDeletionRequests(undefined, true); + + expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalledWith(1, true); + }); + }); + it('should call deletionRequestService.markDeletionRequestAsPending to update status of deletionRequests', async () => { const { deletionRequest } = setup(); - deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); - await uc.executeDeletionRequests(); expect(deletionRequestService.markDeletionRequestAsPending).toHaveBeenCalledWith(deletionRequest.id); @@ -192,8 +242,6 @@ describe(DeletionRequestUc.name, () => { it('should call eventBus.publish', async () => { const { deletionRequest } = setup(); - deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); - await uc.executeDeletionRequests(); expect(eventBus.publish).toHaveBeenCalledWith( @@ -201,6 +249,7 @@ describe(DeletionRequestUc.name, () => { ); }); + /* it('should work with a delay of 0', async () => { const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); configService.get.mockImplementation((key) => { @@ -217,12 +266,15 @@ describe(DeletionRequestUc.name, () => { expect(deletionRequestService.markDeletionRequestAsFailed).not.toHaveBeenCalled(); }); + */ }); describe('when an error occurred', () => { const setup = () => { const deletionRequestToExecute = deletionRequestFactory.build({ deleteAfter: new Date('2023-01-01') }); + configService.get.mockImplementation((key) => deletionTestConfig()[key]); + deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequestToExecute]); deletionRequestService.findInProgressCount.mockResolvedValueOnce(1); eventBus.publish.mockRejectedValueOnce(new Error()); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index dbd0edc76f5..775bab43dfb 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -81,19 +81,20 @@ export class DeletionRequestUc implements IEventHandler { async executeDeletionRequests(limit?: number, getFailed?: boolean): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); - const inProgress = await this.deletionRequestService.findInProgressCount(); + let deletionRequests: DeletionRequest[] = []; - const max = limit - ? limit - inProgress - : this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS') - inProgress; + do { + // eslint-disable-next-line no-await-in-loop + const inProgress = await this.deletionRequestService.findInProgressCount(); - let deletionRequests: DeletionRequest[] = []; + const max = limit + ? limit - inProgress + : this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS') - inProgress; - if (max > 0) { - do { - // eslint-disable-next-line no-await-in-loop - deletionRequests = await this.deletionRequestService.findAllItemsToExecute(max, getFailed); + // eslint-disable-next-line no-await-in-loop + deletionRequests = await this.deletionRequestService.findAllItemsToExecute(max, getFailed); + if (max > 0) { this.logger.debug({ action: 'processing deletion request', deletionRequests }); // eslint-disable-next-line no-await-in-loop @@ -105,8 +106,8 @@ export class DeletionRequestUc implements IEventHandler { // eslint-disable-next-line no-await-in-loop await this.delayForDeletion(); - } while (deletionRequests.length > 0); - } + } + } while (deletionRequests.length > 0); this.logger.debug({ action: 'deletion process completed' }); } From b2a607b93a989b180c79fe5c6bc5ccfab6d2846a Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Wed, 29 Jan 2025 14:55:00 +0100 Subject: [PATCH 21/21] optimization --- .../src/modules/deletion/api/uc/deletion-request.uc.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index 775bab43dfb..92444e035c4 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -82,14 +82,12 @@ export class DeletionRequestUc implements IEventHandler { this.logger.debug({ action: 'executeDeletionRequests', limit }); let deletionRequests: DeletionRequest[] = []; - + const configLimit = this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); do { // eslint-disable-next-line no-await-in-loop const inProgress = await this.deletionRequestService.findInProgressCount(); - const max = limit - ? limit - inProgress - : this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS') - inProgress; + const max = limit ? limit - inProgress : configLimit - inProgress; // eslint-disable-next-line no-await-in-loop deletionRequests = await this.deletionRequestService.findAllItemsToExecute(max, getFailed);