Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BC-7561 - separate job for failed batch deletion requests #5448

Merged
merged 46 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
ca7185d
BC-7561 - a bit of cleanup
virgilchiriac Jan 15, 2025
dba8d83
BC-7561 - improve logic for bulk deletion
virgilchiriac Jan 20, 2025
a364574
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 20, 2025
ee088b5
fix api tets and change sorting
virgilchiriac Jan 20, 2025
e3b952c
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 20, 2025
fc5ce93
Fix some tests
virgilchiriac Jan 21, 2025
da3d694
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 21, 2025
3772528
split pending and failed into a separate job
virgilchiriac Jan 22, 2025
42127e2
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 22, 2025
7b8eccf
implement type casting for command flags
virgilchiriac Jan 22, 2025
9745671
fix linter / remove tests
virgilchiriac Jan 22, 2025
26444f0
unset db debug mode
virgilchiriac Jan 22, 2025
d10659a
undo flag in console
virgilchiriac Jan 22, 2025
e8ab582
fix test
virgilchiriac Jan 23, 2025
eb96b3f
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 23, 2025
8b8e624
fix tests
virgilchiriac Jan 23, 2025
3cc6551
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 23, 2025
86bfdcf
add cronjob
virgilchiriac Jan 23, 2025
51ea68e
fix test
virgilchiriac Jan 23, 2025
3114233
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 23, 2025
ec23c1d
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 24, 2025
e80920a
improve test cov
virgilchiriac Jan 24, 2025
2ec2d7d
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Jan 24, 2025
a42c73b
fixes from revviews
virgilchiriac Jan 24, 2025
e216fc9
fix query
virgilchiriac Jan 24, 2025
92dc292
hacky type casting for console args
virgilchiriac Jan 27, 2025
e73ff3a
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 27, 2025
d070847
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 27, 2025
88906bd
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 27, 2025
a7e13c7
undo package.json command
virgilchiriac Jan 27, 2025
3d25423
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Jan 27, 2025
c40c290
throttle down max number of processing requests in batch
virgilchiriac Jan 28, 2025
e6e8051
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 28, 2025
6adc016
fix max concurency / limit and some tests
virgilchiriac Jan 29, 2025
90dc237
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Jan 29, 2025
39a1d67
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 29, 2025
b2a607b
optimization
virgilchiriac Jan 29, 2025
b6d183a
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Feb 3, 2025
490e35b
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Feb 3, 2025
4ebe5f8
fix test
virgilchiriac Feb 4, 2025
240b55e
fix pending count query
virgilchiriac Feb 5, 2025
bc08140
improve tests readability
virgilchiriac Feb 5, 2025
634705f
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Feb 5, 2025
cc1dd23
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Feb 5, 2025
caba67f
fix linter
virgilchiriac Feb 5, 2025
1e62200
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Feb 5, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { TriggerDeletionExecutionOptions } from '../interface';
import { TriggerDeletionExecutionOptionsBuilder } from './trigger-deletion-execution-options.builder';

describe(TriggerDeletionExecutionOptionsBuilder.name, () => {
Expand All @@ -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 });
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<DeletionRequestOutput> = 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));
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -99,7 +99,7 @@ export class DeletionClient {
}
}

private isLimitGeaterZero(limit?: number): boolean {
private isLimitGreaterZero(limit?: number): boolean {
return typeof limit === 'number' && limit > 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,34 @@ export class DeletionExecutionConsole {
options: [
{
flags: '-l, --limit <value>',
fn: (value: string) => (value ? Number(value) : undefined),
// 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 <value>',
fn: (value: string) => /^(true|yes|1)$/i.test(value),
// 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<void> {
public async triggerDeletionExecution(options: TriggerDeletionExecutionOptions): Promise<void> {
// 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();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export interface TriggerDeletionExecutionOptions {
limit: number;
runFailed: boolean;
limit: string | number;
runFailed: string | boolean;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
};
Expand All @@ -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);

Expand All @@ -131,7 +131,7 @@ describe(`deletionRequest create (api)`, () => {
const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange(
createdItem.createdAt,
createdItem.deleteAfter,
defaultdeleteAfterMinutes,
defaultDeleteAfterMinutes,
operationalTimeToleranceInSeconds
);

Expand Down
100 changes: 89 additions & 11 deletions apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { LegacyLogger } from '@core/logger';
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { MikroORM } from '@mikro-orm/core';
import { ConfigModule } 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';
Expand All @@ -17,19 +16,20 @@ 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;
let uc: DeletionRequestUc;
let deletionRequestService: DeepMocked<DeletionRequestService>;
let deletionLogService: DeepMocked<DeletionLogService>;
let eventBus: DeepMocked<EventBus>;
let configService: DeepMocked<ConfigService<DeletionConfig, true>>;

beforeAll(async () => {
const orm = await setupEntities();

module = await Test.createTestingModule({
imports: [ConfigModule.forRoot(createConfigModuleOptions(deletionTestConfig))],
providers: [
DeletionRequestUc,
{
Expand All @@ -54,22 +54,30 @@ describe(DeletionRequestUc.name, () => {
provide: MikroORM,
useValue: orm,
},
{
provide: ConfigService,
useValue: createMock<ConfigService>(),
},
],
}).compile();

uc = module.get(DeletionRequestUc);
deletionRequestService = module.get(DeletionRequestService);
deletionLogService = module.get(DeletionLogService);
eventBus = module.get(EventBus);
configService = module.get(ConfigService);
});

beforeEach(() => {
afterEach(() => {
jest.clearAllMocks();
jest.useRealTimers();
});

describe('createDeletionRequest', () => {
describe('when creating a deletionRequest', () => {
const setup = () => {
jest.useFakeTimers().setSystemTime(new Date());

const deleteAfterMinutes = 1;
const deletionRequestToCreate: DeletionRequestBodyProps = {
targetRef: {
Expand Down Expand Up @@ -151,22 +159,83 @@ describe(DeletionRequestUc.name, () => {

describe('executeDeletionRequests', () => {
describe('when deletionRequests to execute exists', () => {
const setup = () => {
const setup = (noDelay = false) => {
deletionRequestService.findInProgressCount.mockResolvedValue(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 noDelay ? 0 : 100;
}
return deletionTestConfig()[key];
});

const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') });
deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]);
deletionRequestService.markDeletionRequestAsPending.mockResolvedValueOnce(true);

return { deletionRequest };
};
it('should call deletionRequestService.findAllItemsToExecute', async () => {
it('should call deletionRequestService.findInProgressCount', async () => {
setup();

await uc.executeDeletionRequests();

expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalled();
expect(deletionRequestService.findInProgressCount).toHaveBeenCalled();
});

it('should call deletionRequestService.findAllItemsToExecute with correct limit', async () => {
setup();

await uc.executeDeletionRequests();

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);
Expand All @@ -175,21 +244,30 @@ describe(DeletionRequestUc.name, () => {
it('should call eventBus.publish', async () => {
const { deletionRequest } = setup();

deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]);

await uc.executeDeletionRequests();

expect(eventBus.publish).toHaveBeenCalledWith(
new UserDeletedEvent(deletionRequest.id, deletionRequest.targetRefId)
);
});

it('should work with a delay of 0', async () => {
setup(true);

await uc.executeDeletionRequests();

expect(deletionRequestService.markDeletionRequestAsFailed).not.toHaveBeenCalled();
virgilchiriac marked this conversation as resolved.
Show resolved Hide resolved
});
});

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());

return {
Expand Down
Loading
Loading