diff --git a/src/modules/file/actions/undo-file-versioning.action.spec.ts b/src/modules/file/actions/undo-file-versioning.action.spec.ts index 7135d9f25..32afef07d 100644 --- a/src/modules/file/actions/undo-file-versioning.action.spec.ts +++ b/src/modules/file/actions/undo-file-versioning.action.spec.ts @@ -42,16 +42,16 @@ describe('UndoFileVersioningAction', () => { it('When versioning is disabled, then should delete all user versions', async () => { jest .spyOn(fileVersionRepository, 'deleteUserVersionsBatch') - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(50); + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(500); const result = await action.execute(userUuid); - expect(result).toEqual({ deletedCount: 250 }); + expect(result).toEqual({ deletedCount: 2500 }); expect( fileVersionRepository.deleteUserVersionsBatch, - ).toHaveBeenCalledWith(userUuid, 100); + ).toHaveBeenCalledWith(userUuid, 1000); expect( fileVersionRepository.deleteUserVersionsBatch, ).toHaveBeenCalledTimes(3); @@ -92,14 +92,14 @@ describe('UndoFileVersioningAction', () => { it('When deleting in batches, then should continue until batch returns less than batch size', async () => { jest .spyOn(fileVersionRepository, 'deleteUserVersionsBatch') - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(30); + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(300); const result = await action.execute(userUuid); - expect(result).toEqual({ deletedCount: 330 }); + expect(result).toEqual({ deletedCount: 3300 }); expect( fileVersionRepository.deleteUserVersionsBatch, ).toHaveBeenCalledTimes(4); @@ -113,12 +113,12 @@ describe('UndoFileVersioningAction', () => { jest .spyOn(fileVersionRepository, 'deleteUserVersionsBatch') .mockRejectedValueOnce(new Error('Timeout')) - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(50); + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(500); const result = await action.execute(userUuid); - expect(result).toEqual({ deletedCount: 150 }); + expect(result).toEqual({ deletedCount: 1500 }); expect(delaySpy).toHaveBeenCalledWith(1000); expect( fileVersionRepository.deleteUserVersionsBatch, @@ -134,12 +134,12 @@ describe('UndoFileVersioningAction', () => { .spyOn(fileVersionRepository, 'deleteUserVersionsBatch') .mockRejectedValueOnce(new Error('Lock timeout')) .mockRejectedValueOnce(new Error('Lock timeout')) - .mockResolvedValueOnce(100) - .mockResolvedValueOnce(0); + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(500); const result = await action.execute(userUuid); - expect(result).toEqual({ deletedCount: 100 }); + expect(result).toEqual({ deletedCount: 1500 }); expect(delaySpy).toHaveBeenCalledWith(1000); expect(delaySpy).toHaveBeenCalledWith(2000); expect( @@ -154,7 +154,7 @@ describe('UndoFileVersioningAction', () => { jest .spyOn(fileVersionRepository, 'deleteUserVersionsBatch') - .mockResolvedValueOnce(100) + .mockResolvedValueOnce(1000) .mockRejectedValueOnce(new Error('Corrupted data')) .mockRejectedValueOnce(new Error('Corrupted data')) .mockRejectedValueOnce(new Error('Corrupted data')); @@ -190,5 +190,94 @@ describe('UndoFileVersioningAction', () => { expect(delaySpy).toHaveBeenNthCalledWith(1, 1000); expect(delaySpy).toHaveBeenNthCalledWith(2, 2000); }); + + it('When limits are provided, then should delete versions exceeding limits', async () => { + const limits = { + retentionDays: 30, + maxVersions: 5, + }; + + jest + .spyOn(fileVersionRepository, 'deleteUserVersionsByLimits') + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(500); + + const result = await action.execute(userUuid, { limits }); + + expect(result).toEqual({ deletedCount: 2500 }); + expect( + fileVersionRepository.deleteUserVersionsByLimits, + ).toHaveBeenCalledWith(userUuid, 30, 5, 1000); + expect( + fileVersionRepository.deleteUserVersionsByLimits, + ).toHaveBeenCalledTimes(3); + }); + + it('When limits are provided with custom batch size, then should use custom batch size', async () => { + const limits = { + retentionDays: 60, + maxVersions: 10, + }; + const customBatchSize = 500; + + jest + .spyOn(fileVersionRepository, 'deleteUserVersionsByLimits') + .mockResolvedValueOnce(500) + .mockResolvedValueOnce(200); + + const result = await action.execute(userUuid, { + limits, + batchSize: customBatchSize, + }); + + expect(result).toEqual({ deletedCount: 700 }); + expect( + fileVersionRepository.deleteUserVersionsByLimits, + ).toHaveBeenCalledWith(userUuid, 60, 10, customBatchSize); + }); + + it('When limits are provided and batch fails, then should retry', async () => { + const limits = { + retentionDays: 30, + maxVersions: 5, + }; + + const delaySpy = jest + .spyOn(action as any, 'delay') + .mockResolvedValue(undefined); + + jest + .spyOn(fileVersionRepository, 'deleteUserVersionsByLimits') + .mockRejectedValueOnce(new Error('Timeout')) + .mockResolvedValueOnce(1000) + .mockResolvedValueOnce(500); + + const result = await action.execute(userUuid, { limits }); + + expect(result).toEqual({ deletedCount: 1500 }); + expect(delaySpy).toHaveBeenCalledWith(1000); + expect( + fileVersionRepository.deleteUserVersionsByLimits, + ).toHaveBeenCalledTimes(3); + }); + + it('When limits are provided and user has no versions to delete, then should return zero', async () => { + const limits = { + retentionDays: 30, + maxVersions: 5, + }; + + jest + .spyOn(fileVersionRepository, 'deleteUserVersionsByLimits') + .mockResolvedValueOnce(0); + + const result = await action.execute(userUuid, { limits }); + + expect(result).toEqual({ deletedCount: 0 }); + expect( + fileVersionRepository.deleteUserVersionsByLimits, + ).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/src/modules/file/actions/undo-file-versioning.action.ts b/src/modules/file/actions/undo-file-versioning.action.ts index 3402a9283..04840f155 100644 --- a/src/modules/file/actions/undo-file-versioning.action.ts +++ b/src/modules/file/actions/undo-file-versioning.action.ts @@ -5,6 +5,14 @@ import { } from '@nestjs/common'; import { SequelizeFileVersionRepository } from '../file-version.repository'; +export interface UndoOptions { + batchSize?: number; + limits?: { + retentionDays: number; + maxVersions: number; + }; +} + @Injectable() export class UndoFileVersioningAction { constructor( @@ -13,9 +21,52 @@ export class UndoFileVersioningAction { async execute( userUuid: string, - options?: { batchSize?: number }, + options?: UndoOptions, + ): Promise<{ deletedCount: number }> { + const batchSize = options?.batchSize ?? 1000; + + if (!options?.limits) { + return this.executeUndo(userUuid, batchSize); + } + + return this.executePartialUndo( + userUuid, + batchSize, + options.limits.retentionDays, + options.limits.maxVersions, + ); + } + + private async executeUndo( + userUuid: string, + batchSize: number, + ): Promise<{ deletedCount: number }> { + return this.processBatchesWithRetry(userUuid, batchSize, () => + this.fileVersionRepository.deleteUserVersionsBatch(userUuid, batchSize), + ); + } + + private async executePartialUndo( + userUuid: string, + batchSize: number, + retentionDays: number, + maxVersions: number, + ): Promise<{ deletedCount: number }> { + return this.processBatchesWithRetry(userUuid, batchSize, () => + this.fileVersionRepository.deleteUserVersionsByLimits( + userUuid, + retentionDays, + maxVersions, + batchSize, + ), + ); + } + + private async processBatchesWithRetry( + userUuid: string, + batchSize: number, + deleteFn: () => Promise, ): Promise<{ deletedCount: number }> { - const batchSize = options?.batchSize ?? 100; const maxRetries = 3; let totalDeleted = 0; let processedCount: number; @@ -26,11 +77,7 @@ export class UndoFileVersioningAction { while (!success && retries < maxRetries) { try { - processedCount = - await this.fileVersionRepository.deleteUserVersionsBatch( - userUuid, - batchSize, - ); + processedCount = await deleteFn(); totalDeleted += processedCount; success = true; } catch (error) { diff --git a/src/modules/file/file-version.repository.ts b/src/modules/file/file-version.repository.ts index 4dfabfc1d..9eee80d1a 100644 --- a/src/modules/file/file-version.repository.ts +++ b/src/modules/file/file-version.repository.ts @@ -23,6 +23,12 @@ export interface FileVersionRepository { delete(id: string): Promise; deleteAllByFileId(fileId: string): Promise; deleteUserVersionsBatch(userId: string, limit: number): Promise; + deleteUserVersionsByLimits( + userId: string, + retentionDays: number, + maxVersions: number, + limit: number, + ): Promise; sumExistingSizesByUser(userId: string): Promise; findExpiredVersionIdsByTierLimits(limit: number): Promise; } @@ -152,6 +158,58 @@ export class SequelizeFileVersionRepository implements FileVersionRepository { return result[1]; } + async deleteUserVersionsByLimits( + userId: string, + retentionDays: number, + maxVersions: number, + limit: number, + ): Promise { + const query = ` + WITH ranked_versions AS ( + SELECT + fv.id as version_id, + fv.file_id, + fv.created_at, + ROW_NUMBER() OVER ( + PARTITION BY fv.file_id + ORDER BY fv.created_at DESC + ) as version_rank + FROM file_versions fv + WHERE fv.user_id = :userId + AND fv.status = :existsStatus + ) + UPDATE file_versions + SET status = :deletedStatus, updated_at = NOW() + WHERE id IN ( + SELECT version_id + FROM ranked_versions + WHERE + (:maxVersions > 0 AND version_rank > :maxVersions) + OR + (:retentionDays > 0 AND created_at < NOW() - (:retentionDays || ' days')::INTERVAL) + ORDER BY version_id ASC + LIMIT :limit + ) + `; + + const result = await this.model.sequelize.query( + query, + { + replacements: { + userId, + retentionDays, + maxVersions, + limit, + deletedStatus: FileVersionStatus.DELETED, + existsStatus: FileVersionStatus.EXISTS, + }, + type: QueryTypes.UPDATE, + }, + ); + + return result[1]; + } + async sumExistingSizesByUser(userId: string): Promise { const result = await this.model.findAll({ attributes: [[Sequelize.fn('SUM', Sequelize.col('size')), 'total']], diff --git a/src/modules/file/file.usecase.spec.ts b/src/modules/file/file.usecase.spec.ts index 7a6439273..a280c2fcd 100644 --- a/src/modules/file/file.usecase.spec.ts +++ b/src/modules/file/file.usecase.spec.ts @@ -2996,36 +2996,39 @@ describe('FileUseCases', () => { describe('undoFileVersioning', () => { const userUuid = v4(); - it('When versioning is enabled, then should not delete any versions', async () => { + it('When called, then should delete all user versions with default batch size', async () => { jest .spyOn(undoFileVersioningAction, 'execute') - .mockResolvedValue({ deletedCount: 0 }); + .mockResolvedValue({ deletedCount: 250 }); const result = await service.undoFileVersioning(userUuid); - expect(undoFileVersioningAction.execute).toHaveBeenCalledWith( - userUuid, - undefined, - ); - expect(result).toEqual({ deletedCount: 0 }); + expect(undoFileVersioningAction.execute).toHaveBeenCalledWith(userUuid, { + batchSize: 1000, + }); + expect(result).toEqual({ deletedCount: 250 }); }); + }); - it('When custom batch size is provided, then should use that batch size', async () => { - const customBatchSize = 50; + describe('partialUndoFileVersioning', () => { + const userUuid = v4(); + const limits = { + retentionDays: 30, + maxVersions: 5, + }; + it('When called with limits, then should delete versions exceeding limits', async () => { jest .spyOn(undoFileVersioningAction, 'execute') - .mockResolvedValue({ deletedCount: 75 }); + .mockResolvedValue({ deletedCount: 150 }); - const result = await service.undoFileVersioning(userUuid, { - batchSize: customBatchSize, - }); + const result = await service.partialUndoFileVersioning(userUuid, limits); - expect(undoFileVersioningAction.execute).toHaveBeenCalledWith( - userUuid, - { batchSize: customBatchSize }, - ); - expect(result).toEqual({ deletedCount: 75 }); + expect(undoFileVersioningAction.execute).toHaveBeenCalledWith(userUuid, { + batchSize: 1000, + limits, + }); + expect(result).toEqual({ deletedCount: 150 }); }); }); }); diff --git a/src/modules/file/file.usecase.ts b/src/modules/file/file.usecase.ts index fe0b5d9f6..c37fb3b61 100644 --- a/src/modules/file/file.usecase.ts +++ b/src/modules/file/file.usecase.ts @@ -1149,8 +1149,19 @@ export class FileUseCases { async undoFileVersioning( userUuid: string, - options?: { batchSize?: number }, ): Promise<{ deletedCount: number }> { - return this.undoFileVersioningAction.execute(userUuid, options); + return this.undoFileVersioningAction.execute(userUuid, { + batchSize: 1000, + }); + } + + async partialUndoFileVersioning( + userUuid: string, + limits: { retentionDays: number; maxVersions: number }, + ): Promise<{ deletedCount: number }> { + return this.undoFileVersioningAction.execute(userUuid, { + batchSize: 1000, + limits, + }); } } diff --git a/src/modules/gateway/gateway.usecase.spec.ts b/src/modules/gateway/gateway.usecase.spec.ts index 829ef21e9..fbd66fc18 100644 --- a/src/modules/gateway/gateway.usecase.spec.ts +++ b/src/modules/gateway/gateway.usecase.spec.ts @@ -731,12 +731,15 @@ describe('GatewayUseCases', () => { expect(user.tierId).not.toBe(originalTierId); }); - it('When user has versioning enabled, then should not delete file versions', async () => { + it('When user has versioning enabled, then should call partial undo', async () => { const enabledLimits = newVersioningLimits({ enabled: true }); const getLimitsSpy = jest .spyOn(featureLimitService, 'getFileVersioningLimits') .mockResolvedValue(enabledLimits); const undoSpy = jest.spyOn(fileUseCases, 'undoFileVersioning'); + const partialUndoSpy = jest + .spyOn(fileUseCases, 'partialUndoFileVersioning') + .mockResolvedValue({ deletedCount: 0 }); jest.spyOn(userUseCases, 'updateUserStorage').mockResolvedValue(); jest.spyOn(cacheManagerService, 'expireLimit').mockResolvedValue(); @@ -748,6 +751,10 @@ describe('GatewayUseCases', () => { expect(getLimitsSpy).toHaveBeenCalledWith(user.uuid); expect(undoSpy).not.toHaveBeenCalled(); + expect(partialUndoSpy).toHaveBeenCalledWith(user.uuid, { + retentionDays: enabledLimits.retentionDays, + maxVersions: enabledLimits.maxVersions, + }); }); it('When user has versioning disabled, then should delete all file versions', async () => { diff --git a/src/modules/gateway/gateway.usecase.ts b/src/modules/gateway/gateway.usecase.ts index a3e03e9c1..9b392df93 100644 --- a/src/modules/gateway/gateway.usecase.ts +++ b/src/modules/gateway/gateway.usecase.ts @@ -342,12 +342,23 @@ export class GatewayUseCases { const limits = await this.featureLimitService.getFileVersioningLimits(user.uuid); - if (!limits.enabled) { + if (limits.enabled) { + const { deletedCount } = + await this.fileUseCases.partialUndoFileVersioning(user.uuid, { + retentionDays: limits.retentionDays, + maxVersions: limits.maxVersions, + }); + if (deletedCount > 0) { + Logger.log( + `[GATEWAY/UPDATE_TIER] Adjusted ${deletedCount} file versions to tier limits for user ${user.uuid} due to tier change`, + ); + } + } else { const { deletedCount } = await this.fileUseCases.undoFileVersioning(user.uuid); if (deletedCount > 0) { Logger.log( - `[GATEWAY/UPDATE_TIER] Deleted ${deletedCount} file versions for user ${user.uuid} due to tier change`, + `[GATEWAY/UPDATE_TIER] Deleted ${deletedCount} file versions (full cleanup) for user ${user.uuid} due to tier change`, ); } }