diff --git a/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts b/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts index 70046bdb2..135a8dbd0 100644 --- a/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts +++ b/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts @@ -1,11 +1,11 @@ import type { Model } from '@douglasneuroinformatics/libnest'; -import { getModelToken, LoggingService } from '@douglasneuroinformatics/libnest'; +import { getModelToken, LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; import { ForbiddenException, NotFoundException, UnprocessableEntityException } from '@nestjs/common'; import { Test } from '@nestjs/testing'; import { DEFAULT_GROUP_NAME } from '@opendatacapture/schemas/core'; -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { AbilityFactory } from '@/auth/ability.factory'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; @@ -22,6 +22,10 @@ import { InstrumentRecordsService } from '../instrument-records.service'; import type { RecordType } from '../thread-types'; describe('InstrumentRecordsService', () => { + let loggingService: MockedInstance; + let storageService: MockedInstance; + let fileModel: MockedInstance>; + const transaction = vi.fn(); let instrumentRecordsService: InstrumentRecordsService; let instrumentRecordModel: MockedInstance>; let sessionModel: MockedInstance>; @@ -31,10 +35,27 @@ describe('InstrumentRecordsService', () => { let usersService: MockedInstance; beforeEach(async () => { + transaction.mockReset(); + const recordProvider = MockFactory.createForModelToken(getModelToken('InstrumentRecord')); + const fileProvider = MockFactory.createForModelToken(getModelToken('InstrumentRecordFile')); const moduleRef = await Test.createTestingModule({ providers: [ InstrumentRecordsService, - MockFactory.createForModelToken(getModelToken('InstrumentRecord')), + recordProvider, + fileProvider, + MockFactory.createForService(LoggingService), + { + inject: [getModelToken('InstrumentRecord'), getModelToken('InstrumentRecordFile')], + provide: PRISMA_CLIENT_TOKEN, + useFactory: ( + instrumentRecord: Model<'InstrumentRecord'>, + instrumentRecordFile: Model<'InstrumentRecordFile'> + ) => ({ + $transaction: transaction, + instrumentRecord, + instrumentRecordFile + }) + }, MockFactory.createForModelToken(getModelToken('Session')), MockFactory.createForService(GroupsService), MockFactory.createForService(UsersService), @@ -46,6 +67,9 @@ describe('InstrumentRecordsService', () => { ] }).compile(); + fileModel = moduleRef.get(getModelToken('InstrumentRecordFile')); + storageService = moduleRef.get(StorageService); + loggingService = moduleRef.get(LoggingService); instrumentRecordModel = moduleRef.get(getModelToken('InstrumentRecord')); sessionModel = moduleRef.get(getModelToken('Session')); instrumentRecordsService = moduleRef.get(InstrumentRecordsService); @@ -55,6 +79,96 @@ describe('InstrumentRecordsService', () => { usersService = moduleRef.get(UsersService); }); + describe('deleteById', () => { + const file = { basename: 'file', groupId: 'group-1', id: 'file-1', index: 0, recordId: 'record-1' }; + const storageFile = { groupId: 'group-1', location: { basename: 'file', index: 0 }, recordId: 'record-1' }; + + beforeEach(() => { + instrumentRecordModel.findFirst.mockResolvedValue({ id: 'record-1' }); + fileModel.findMany.mockResolvedValue([file]); + transaction.mockResolvedValue([{ count: 1 }, { id: 'record-1' }]); + }); + + it('should apply delete permissions before looking up files and again when deleting', async () => { + const ability = createAppAbility([ + { action: 'delete', conditions: { groupId: 'group-1' }, subject: 'InstrumentRecord' } + ]); + await instrumentRecordsService.deleteById('record-1', { ability }); + const where = { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id: 'record-1' }; + expect(instrumentRecordModel.findFirst).toHaveBeenCalledWith({ where }); + expect(instrumentRecordModel.delete).toHaveBeenCalledWith({ where }); + expect(fileModel.findMany).toHaveBeenCalledWith({ where: { recordId: 'record-1' } }); + expect(instrumentRecordModel.findFirst).toHaveBeenCalledBefore(fileModel.findMany); + }); + + it('should delete the collected file rows before the record in one transaction', async () => { + const fileOperation = Promise.resolve({ count: 1 }); + const recordOperation = Promise.resolve({ id: 'record-1' }); + fileModel.deleteMany.mockReturnValueOnce(fileOperation); + instrumentRecordModel.delete.mockReturnValueOnce(recordOperation); + await expect(instrumentRecordsService.deleteById('record-1')).resolves.toEqual({ id: 'record-1' }); + expect(transaction).toHaveBeenCalledWith([fileOperation, recordOperation]); + expect(fileModel.deleteMany).toHaveBeenCalledWith({ where: { id: { in: ['file-1'] } } }); + expect(fileModel.findMany).toHaveBeenCalledBefore(fileModel.deleteMany); + }); + + it('should wait for the transaction to commit before deleting storage objects', async () => { + const commit = Promise.withResolvers(); + transaction.mockReturnValueOnce(commit.promise); + const deletion = instrumentRecordsService.deleteById('record-1'); + await vi.waitFor(() => expect(transaction).toHaveBeenCalledOnce()); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); + commit.resolve([{ count: 1 }, { id: 'record-1' }]); + await deletion; + expect(storageService.deleteObjects).toHaveBeenCalledWith([storageFile]); + }); + + it('should leave storage untouched if the database transaction fails', async () => { + transaction.mockRejectedValueOnce(new Error('transaction failed')); + await expect(instrumentRecordsService.deleteById('record-1')).rejects.toThrow('transaction failed'); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); + }); + + it('should log orphaned objects without failing an already committed deletion', async () => { + const error = new Error('storage failed'); + storageService.deleteObjects.mockRejectedValueOnce(error); + await expect(instrumentRecordsService.deleteById('record-1')).resolves.toEqual({ id: 'record-1' }); + expect(loggingService.error).toHaveBeenCalledWith({ + error, + files: [storageFile], + message: expect.stringContaining('orphaned objects require cleanup') + }); + }); + + it('should delete records without files', async () => { + fileModel.findMany.mockResolvedValueOnce([]); + await expect(instrumentRecordsService.deleteById('record-1')).resolves.toEqual({ id: 'record-1' }); + expect(storageService.deleteObjects).toHaveBeenCalledWith([]); + }); + + it('should report a missing or inaccessible record without reading its files', async () => { + instrumentRecordModel.findFirst.mockResolvedValueOnce(null); + const ability = createAppAbility([ + { action: 'delete', conditions: { groupId: 'other-group' }, subject: 'InstrumentRecord' } + ]); + await expect(instrumentRecordsService.deleteById('record-1', { ability })).rejects.toBeInstanceOf( + NotFoundException + ); + expect(fileModel.findMany).not.toHaveBeenCalled(); + expect(transaction).not.toHaveBeenCalled(); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); + }); + + it('should refuse callers without a delete rule before querying clinical data', async () => { + await expect( + instrumentRecordsService.deleteById('record-1', { ability: createAppAbility([]) }) + ).rejects.toThrow(); + expect(instrumentRecordModel.findFirst).not.toHaveBeenCalled(); + expect(fileModel.findMany).not.toHaveBeenCalled(); + expect(transaction).not.toHaveBeenCalled(); + }); + }); + describe('findById', () => { it('should throw a NotFoundException if no record is found', async () => { instrumentRecordModel.findFirst.mockResolvedValueOnce(null); diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index 9ac0d9b89..806acb3d2 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -3,7 +3,7 @@ import { join } from 'path'; import { Worker } from 'worker_threads'; import { replacer, reviver } from '@douglasneuroinformatics/libjs'; -import { InjectModel } from '@douglasneuroinformatics/libnest'; +import { InjectModel, InjectPrismaClient, LoggingService } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; import { linearRegression } from '@douglasneuroinformatics/libstats'; import { @@ -29,6 +29,7 @@ import { isNumber, mergeWith, pickBy } from 'lodash-es'; import { accessibleQuery, forcedAppSubject } from '@/auth/ability.utils'; import type { AppAbility } from '@/auth/auth.types'; +import type { RuntimePrismaClient } from '@/core/prisma'; import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; import { InstrumentsService } from '@/instruments/instruments.service'; @@ -51,6 +52,8 @@ import type { @Injectable() export class InstrumentRecordsService { constructor( + @InjectPrismaClient() private readonly prismaClient: RuntimePrismaClient, + private readonly loggingService: LoggingService, @InjectModel('InstrumentRecord') private readonly instrumentRecordModel: Model<'InstrumentRecord'>, @InjectModel('Session') private readonly sessionModel: Model<'Session'>, private readonly groupsService: GroupsService, @@ -163,13 +166,31 @@ export class InstrumentRecordsService { } async deleteById(id: string, { ability }: EntityOperationOptions = {}) { - const isExisting = await this.instrumentRecordModel.exists({ id }); - if (!isExisting) { + const where = { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id }; + const record = await this.instrumentRecordModel.findFirst({ where }); + if (!record) { throw new NotFoundException(`Could not find record with ID '${id}'`); } - return this.instrumentRecordModel.delete({ - where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id } - }); + const files = await this.prismaClient.instrumentRecordFile.findMany({ where: { recordId: record.id } }); + const [, deletedRecord] = await this.prismaClient.$transaction([ + this.prismaClient.instrumentRecordFile.deleteMany({ where: { id: { in: files.map((file) => file.id) } } }), + this.prismaClient.instrumentRecord.delete({ where }) + ]); + const storageFiles = files.map(({ basename, groupId, index, recordId }) => ({ + groupId, + location: { basename, index }, + recordId + })); + try { + await this.storageService.deleteObjects(storageFiles); + } catch (error) { + this.loggingService.error({ + error, + files: storageFiles, + message: `Failed to delete storage objects after deleting record '${id}'; orphaned objects require cleanup` + }); + } + return deletedRecord; } async exists(where: Prisma.InstrumentRecordWhereInput): Promise { diff --git a/apps/api/src/storage/__tests__/storage.service.spec.ts b/apps/api/src/storage/__tests__/storage.service.spec.ts new file mode 100644 index 000000000..322f7edff --- /dev/null +++ b/apps/api/src/storage/__tests__/storage.service.spec.ts @@ -0,0 +1,92 @@ +import { DeleteObjectsCommand, S3Client } from '@aws-sdk/client-s3'; +import { ConfigService } from '@douglasneuroinformatics/libnest'; +import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; +import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { ServiceUnavailableException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { StorageService } from '../storage.service'; + +describe('StorageService', () => { + let storageService: StorageService; + let configService: MockedInstance; + const send = vi.fn(); + const files = [ + { groupId: 'group-1', location: { basename: 'file', index: 0 }, recordId: 'record-1' }, + { groupId: null, location: { basename: 'image', index: 2 }, recordId: 'record-2' } + ]; + + beforeEach(async () => { + send.mockReset().mockResolvedValue({}); + const configModule = await Test.createTestingModule({ + providers: [MockFactory.createForService(ConfigService)] + }).compile(); + configService = configModule.get(ConfigService); + configService.get.mockImplementation((key) => { + if (key === 'STORAGE_ENABLED') return true; + if (key === 'STORAGE_BUCKET') return 'test-bucket'; + return undefined; + }); + const moduleRef = await Test.createTestingModule({ + providers: [ + StorageService, + { provide: ConfigService, useValue: configService }, + { provide: S3Client, useValue: { send } } + ] + }).compile(); + storageService = moduleRef.get(StorageService); + }); + + it('should delete grouped and root files with their existing storage keys', async () => { + await storageService.deleteObjects(files); + expect(send).toHaveBeenCalledOnce(); + expect(send.mock.lastCall?.[0]).toBeInstanceOf(DeleteObjectsCommand); + expect(send.mock.lastCall?.[0].input).toEqual({ + Bucket: 'test-bucket', + Delete: { + Objects: [ + { Key: 'groups/group-1/records/record-1/files/file_0' }, + { Key: 'groups/__ROOT__/records/record-2/files/image_2' } + ] + } + }); + }); + + it('should skip storage entirely when there are no files', async () => { + await storageService.deleteObjects([]); + expect(send).not.toHaveBeenCalled(); + }); + + it('should split requests at the S3 limit of one thousand objects', async () => { + await storageService.deleteObjects(Array.from({ length: 1001 }, () => files[0]!)); + expect(send).toHaveBeenCalledTimes(2); + expect(send.mock.calls[0]![0].input.Delete.Objects).toHaveLength(1000); + expect(send.mock.calls[1]![0].input.Delete.Objects).toHaveLength(1); + }); + + it('should reject per-object errors even when S3 accepts the request', async () => { + send.mockResolvedValueOnce({ Errors: [{ Code: 'AccessDenied', Key: 'failed-key' }] }); + await expect(storageService.deleteObjects(files)).rejects.toThrow('AccessDenied'); + }); + + it('should propagate a failed storage request', async () => { + send.mockRejectedValueOnce(new Error('storage unavailable')); + await expect(storageService.deleteObjects(files)).rejects.toThrow('storage unavailable'); + }); + + it('should reject nonempty cleanup when storage is unavailable', async () => { + configService.get.mockReturnValue(undefined); + const moduleRef = await Test.createTestingModule({ + providers: [ + StorageService, + { provide: ConfigService, useValue: configService }, + { provide: S3Client, useValue: null } + ] + }).compile(); + const disabledStorage = moduleRef.get(StorageService); + await expect(disabledStorage.deleteObjects(files)).rejects.toBeInstanceOf(ServiceUnavailableException); + await expect(disabledStorage.deleteObjects([])).resolves.toBeUndefined(); + expect(send).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/storage/storage.service.ts b/apps/api/src/storage/storage.service.ts index a7d29e1f0..30f14cd63 100644 --- a/apps/api/src/storage/storage.service.ts +++ b/apps/api/src/storage/storage.service.ts @@ -1,5 +1,6 @@ import { CreateBucketCommand, + DeleteObjectsCommand, GetObjectCommand, HeadBucketCommand, PutObjectCommand, @@ -40,6 +41,26 @@ export class StorageService implements OnModuleInit { return this.enabled; } + async deleteObjects(files: FileSearchParams[]): Promise { + if (files.length === 0) { + return; + } + const { bucket, s3 } = this.requireStorage(); + for (let offset = 0; offset < files.length; offset += 1000) { + const result = await s3.send( + new DeleteObjectsCommand({ + Bucket: bucket, + Delete: { + Objects: files.slice(offset, offset + 1000).map((file) => ({ Key: this.getStorageKey(file) })) + } + }) + ); + if (result.Errors?.length) { + throw new Error(`Failed to delete storage objects: ${JSON.stringify(result.Errors)}`); + } + } + } + async getPresignedDownloadUrl(params: FileSearchParams): Promise<$PresignedUrlInfo> { const { bucket, s3 } = this.requireStorage(); const key = this.getStorageKey(params); diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index e83059345..eb1209648 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -11,10 +11,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { AbilityFactory } from '@/auth/ability.factory'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import type { RuntimePrismaClient } from '@/core/prisma'; +import { StorageService } from '@/storage/storage.service'; import { SubjectsService } from '../subjects.service'; describe('SubjectsService', () => { + let storageService: MockedInstance; + let loggingService: MockedInstance; let subjectsService: SubjectsService; let subjectModel: MockedInstance>; let prismaClient: MockedInstance & { @@ -26,6 +29,8 @@ describe('SubjectsService', () => { providers: [ MockFactory.createForService(CryptoService), SubjectsService, + MockFactory.createForService(StorageService), + MockFactory.createForService(LoggingService), MockFactory.createForModelToken(getModelToken('Subject')), { provide: PRISMA_CLIENT_TOKEN, @@ -33,19 +38,25 @@ describe('SubjectsService', () => { $transaction: vi.fn(), instrumentRecord: { deleteMany: vi.fn(), - findMany: vi.fn(), + findMany: vi.fn().mockResolvedValue([]), groupBy: vi.fn() }, + instrumentRecordFile: { + deleteMany: vi.fn(), + findMany: vi.fn().mockResolvedValue([]) + }, session: { deleteMany: vi.fn() }, subject: { - deleteMany: vi.fn() + delete: vi.fn() } } } ] }).compile(); + storageService = moduleRef.get(StorageService); + loggingService = moduleRef.get(LoggingService); subjectModel = moduleRef.get(getModelToken('Subject')); subjectsService = moduleRef.get(SubjectsService); prismaClient = moduleRef.get(PRISMA_CLIENT_TOKEN); @@ -232,17 +243,118 @@ describe('SubjectsService', () => { expect(subjectModel.delete).not.toHaveBeenCalled(); expect(prismaClient.$transaction).toHaveBeenCalledOnce(); }); - it('should pass operations to $transaction in order: instrumentRecord, session, subject', async () => { + it('should pass operations to $transaction in order: files, instrumentRecord, session, subject', async () => { subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); + const fileOp = 'file-op'; + prismaClient.instrumentRecordFile.deleteMany.mockReturnValueOnce(fileOp); const instrumentRecordOp = 'instrumentRecord-op'; const sessionOp = 'session-op'; const subjectOp = 'subject-op'; prismaClient.instrumentRecord.deleteMany.mockReturnValueOnce(instrumentRecordOp); prismaClient.session.deleteMany.mockReturnValueOnce(sessionOp); - prismaClient.subject.deleteMany.mockReturnValueOnce(subjectOp); + prismaClient.subject.delete.mockReturnValueOnce(subjectOp); await subjectsService.deleteById('123', { force: true }); - expect(prismaClient.$transaction).toHaveBeenCalledWith([instrumentRecordOp, sessionOp, subjectOp]); + expect(prismaClient.$transaction).toHaveBeenCalledWith([fileOp, instrumentRecordOp, sessionOp, subjectOp]); + }); + it('should scope the subject and its dependent records and sessions to delete permissions', async () => { + const ability = createAppAbility([ + { action: 'delete', conditions: { groupIds: { has: 'group-1' } }, subject: 'Subject' }, + { action: 'delete', conditions: { groupId: 'group-1' }, subject: 'InstrumentRecord' }, + { action: 'delete', conditions: { groupId: 'group-1' }, subject: 'Session' } + ]); + subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); + await subjectsService.deleteById('123', { ability, force: true }); + const where = { AND: [accessibleQuery(ability, 'delete', 'Subject')], id: '123' }; + expect(subjectModel.findFirst).toHaveBeenCalledWith({ where }); + expect(prismaClient.subject.delete).toHaveBeenCalledWith({ where }); + expect(prismaClient.instrumentRecord.findMany).toHaveBeenCalledWith({ + select: { id: true }, + where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], subjectId: '123' } + }); + expect(prismaClient.instrumentRecord.deleteMany).toHaveBeenCalledWith({ + where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id: { in: [] }, subjectId: '123' } + }); + expect(prismaClient.session.deleteMany).toHaveBeenCalledWith({ + where: { AND: [accessibleQuery(ability, 'delete', 'Session')], subjectId: '123' } + }); + }); + + it('should collect files from every authorized record and clean storage only after commit', async () => { + subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); + prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ id: 'record-1' }, { id: 'record-2' }]); + const files = [ + { basename: 'file', groupId: 'group-1', id: 'file-1', index: 0, recordId: 'record-1' }, + { basename: 'scan', groupId: null, id: 'file-2', index: 1, recordId: 'record-2' } + ]; + prismaClient.instrumentRecordFile.findMany.mockResolvedValueOnce(files); + const commit = Promise.withResolvers(); + prismaClient.$transaction.mockReturnValueOnce(commit.promise); + const deletion = subjectsService.deleteById('123', { force: true }); + await vi.waitFor(() => expect(prismaClient.$transaction).toHaveBeenCalledOnce()); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); + commit.resolve([]); + await deletion; + expect(prismaClient.instrumentRecordFile.findMany).toHaveBeenCalledWith({ + where: { recordId: { in: ['record-1', 'record-2'] } } + }); + expect(prismaClient.instrumentRecordFile.deleteMany).toHaveBeenCalledWith({ + where: { id: { in: ['file-1', 'file-2'] } } + }); + expect(subjectModel.findFirst).toHaveBeenCalledBefore(prismaClient.instrumentRecord.findMany); + expect(prismaClient.instrumentRecordFile.findMany).toHaveBeenCalledBefore( + prismaClient.instrumentRecordFile.deleteMany + ); + expect(storageService.deleteObjects).toHaveBeenCalledWith([ + { groupId: 'group-1', location: { basename: 'file', index: 0 }, recordId: 'record-1' }, + { groupId: null, location: { basename: 'scan', index: 1 }, recordId: 'record-2' } + ]); + }); + + it('should keep storage intact if any database deletion fails', async () => { + subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); + prismaClient.$transaction.mockRejectedValueOnce(new Error('transaction failed')); + await expect(subjectsService.deleteById('123', { force: true })).rejects.toThrow('transaction failed'); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); + }); + + it('should log storage cleanup failure after a successful force deletion', async () => { + subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); + prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ id: 'record-1' }]); + prismaClient.instrumentRecordFile.findMany.mockResolvedValueOnce([ + { basename: 'file', groupId: null, id: 'file-1', index: 0, recordId: 'record-1' } + ]); + const error = new Error('storage failed'); + storageService.deleteObjects.mockRejectedValueOnce(error); + await expect(subjectsService.deleteById('123', { force: true })).resolves.toEqual({ success: true }); + expect(loggingService.error).toHaveBeenCalledWith({ + error, + files: [{ groupId: null, location: { basename: 'file', index: 0 }, recordId: 'record-1' }], + message: expect.stringContaining('orphaned objects require cleanup') + }); + }); + + it('should refuse an inaccessible subject before reading or deleting dependent data', async () => { + subjectModel.findFirst.mockResolvedValueOnce(null); + const ability = createAppAbility([ + { action: 'delete', conditions: { groupIds: { has: 'other-group' } }, subject: 'Subject' } + ]); + await expect(subjectsService.deleteById('123', { ability, force: true })).rejects.toBeInstanceOf( + NotFoundException + ); + expect(prismaClient.instrumentRecord.findMany).not.toHaveBeenCalled(); + expect(prismaClient.instrumentRecordFile.findMany).not.toHaveBeenCalled(); + expect(prismaClient.$transaction).not.toHaveBeenCalled(); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); }); + + it('should leave dependent rows and storage alone when force is false', async () => { + subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); + await expect(subjectsService.deleteById('123')).resolves.toEqual({ success: true }); + expect(prismaClient.instrumentRecord.findMany).not.toHaveBeenCalled(); + expect(prismaClient.instrumentRecordFile.deleteMany).not.toHaveBeenCalled(); + expect(storageService.deleteObjects).not.toHaveBeenCalled(); + }); + it('should throw NotFoundException when subject does not exist', async () => { subjectModel.findFirst.mockResolvedValueOnce(null); await expect(subjectsService.deleteById('123')).rejects.toBeInstanceOf(NotFoundException); diff --git a/apps/api/src/subjects/subjects.module.ts b/apps/api/src/subjects/subjects.module.ts index 14f7de6ba..21894fe9b 100644 --- a/apps/api/src/subjects/subjects.module.ts +++ b/apps/api/src/subjects/subjects.module.ts @@ -1,11 +1,14 @@ import { Module } from '@nestjs/common'; +import { StorageModule } from '@/storage/storage.module'; + import { SubjectsController } from './subjects.controller'; import { SubjectsService } from './subjects.service'; @Module({ controllers: [SubjectsController], exports: [SubjectsService], + imports: [StorageModule], providers: [SubjectsService] }) export class SubjectsModule {} diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index 2345e3dca..382e64d1d 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -1,4 +1,4 @@ -import { InjectModel, InjectPrismaClient } from '@douglasneuroinformatics/libnest'; +import { InjectModel, InjectPrismaClient, LoggingService } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; import { ConflictException, Injectable, NotFoundException } from '@nestjs/common'; import { DEFAULT_GROUP_NAME } from '@opendatacapture/schemas/core'; @@ -10,6 +10,7 @@ import { accessibleQuery } from '@/auth/ability.utils'; import type { AppAbility } from '@/auth/auth.types'; import type { RuntimePrismaClient } from '@/core/prisma'; import type { EntityOperationOptions } from '@/core/types'; +import { StorageService } from '@/storage/storage.service'; const PERSONAL_INFO_FIELDS = ['dateOfBirth', 'firstName', 'lastName', 'sex'] as const; @@ -31,7 +32,9 @@ const IDENTIFIED_BY_CUSTOM_ID: Prisma.SubjectWhereInput = { export class SubjectsService { constructor( @InjectPrismaClient() private readonly prismaClient: RuntimePrismaClient, - @InjectModel('Subject') private readonly subjectModel: Model<'Subject'> + @InjectModel('Subject') private readonly subjectModel: Model<'Subject'>, + private readonly loggingService: LoggingService, + private readonly storageService: StorageService ) {} /** @@ -122,34 +125,48 @@ export class SubjectsService { } async deleteById(id: string, { ability, force }: EntityOperationOptions & { force?: boolean } = {}) { - const subject = await this.findById(id); + const where = { AND: [accessibleQuery(ability, 'delete', 'Subject')], id }; + const subject = await this.subjectModel.findFirst({ where }); + if (!subject) { + throw new NotFoundException(`Failed to find subject with id: ${id}`); + } if (!force) { - await this.subjectModel.delete({ - where: { AND: [accessibleQuery(ability, 'delete', 'Subject')], id: subject.id } - }); + await this.subjectModel.delete({ where }); return { success: true }; } + const recordWhere = { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], subjectId: subject.id }; + const records = await this.prismaClient.instrumentRecord.findMany({ + select: { id: true }, + where: recordWhere + }); + const recordIds = records.map((record) => record.id); + const files = await this.prismaClient.instrumentRecordFile.findMany({ + where: { recordId: { in: recordIds } } + }); await this.prismaClient.$transaction([ + this.prismaClient.instrumentRecordFile.deleteMany({ where: { id: { in: files.map((file) => file.id) } } }), this.prismaClient.instrumentRecord.deleteMany({ - where: { - subject: { - id: subject.id - } - } + where: { ...recordWhere, id: { in: recordIds } } }), this.prismaClient.session.deleteMany({ - where: { - subject: { - id: subject.id - } - } + where: { AND: [accessibleQuery(ability, 'delete', 'Session')], subjectId: subject.id } }), - this.prismaClient.subject.deleteMany({ - where: { - id: subject.id - } - }) + this.prismaClient.subject.delete({ where }) ]); + const storageFiles = files.map(({ basename, groupId, index, recordId }) => ({ + groupId, + location: { basename, index }, + recordId + })); + try { + await this.storageService.deleteObjects(storageFiles); + } catch (error) { + this.loggingService.error({ + error, + files: storageFiles, + message: `Failed to delete storage objects after deleting subject '${id}'; orphaned objects require cleanup` + }); + } return { success: true }; } diff --git a/apps/api/test/suites/03-record-file-deletion.suite.ts b/apps/api/test/suites/03-record-file-deletion.suite.ts new file mode 100644 index 000000000..1a6cda61f --- /dev/null +++ b/apps/api/test/suites/03-record-file-deletion.suite.ts @@ -0,0 +1,246 @@ +import { randomUUID } from 'node:crypto'; + +import { LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; +import type { InstrumentRecord } from '@prisma/client'; +import { afterEach, beforeAll, beforeEach, expect, it, vi } from 'vitest'; +import type { MockInstance } from 'vitest'; + +import type { RuntimePrismaClient } from '@/core/prisma'; +import { StorageService } from '@/storage/storage.service'; + +import { defineSuite } from '../helpers'; + +export default defineSuite('record file deletion', function () { + let prisma: RuntimePrismaClient; + let adminHeaders: { authorization: string }; + let scopedHeaders: { authorization: string }; + let groupId: string; + let otherGroupId: string; + let instrumentId: string; + let cleanup: MockInstance; + + const seed = async (recordGroupIds: string[]) => { + const subject = await prisma.subject.create({ + data: { groupIds: [groupId], id: `issue1572-${randomUUID()}` } + }); + const records: InstrumentRecord[] = []; + for (const recordGroupId of recordGroupIds) { + const session = await prisma.session.create({ + data: { date: new Date(), groupId: recordGroupId, subjectId: subject.id, type: 'IN_PERSON' } + }); + records.push( + await prisma.instrumentRecord.create({ + data: { + date: new Date(), + files: { create: { basename: 'file', groupId: recordGroupId, index: 0, name: 'test.txt', size: 4 } }, + groupId: recordGroupId, + instrumentId, + pending: false, + sessionId: session.id, + subjectId: subject.id + } + }) + ); + } + return { records, subject }; + }; + + beforeAll(async () => { + prisma = this.app.get(PRISMA_CLIENT_TOKEN); + const password = 'DataCapture2025_Test'; + const login = await this.app.inject({ + method: 'POST', + payload: { password, username: 'admin' }, + url: '/v1/auth/login' + }); + expect(login.statusCode).toBe(200); + adminHeaders = { authorization: `Bearer ${login.json<{ accessToken: string }>().accessToken}` }; + const createGroup = async (name: string) => { + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { name, type: 'RESEARCH' }, + url: '/v1/groups' + }); + expect(response.statusCode).toBe(201); + return response.json<{ id: string }>().id; + }; + groupId = await createGroup('File Deletion'); + otherGroupId = await createGroup('Other File Deletion'); + instrumentId = ( + await prisma.instrument.create({ + data: { bundle: '{}', groupIds: [], id: `file-deletion-${randomUUID()}` } + }) + ).id; + const username = `file-deletion-${randomUUID()}`; + const user = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { + basePermissionLevel: 'STANDARD', + firstName: 'File', + groupIds: [groupId], + lastName: 'Deletion', + password, + username + }, + url: '/v1/users' + }); + expect(user.statusCode).toBe(201); + const permissions = await this.app.inject({ + headers: adminHeaders, + method: 'PUT', + payload: { + permissions: ['InstrumentRecord', 'Session', 'Subject'].map((subject) => ({ + action: 'delete', + groupId, + subject + })) + }, + url: `/v1/users/${user.json<{ id: string }>().id}/permissions` + }); + expect(permissions.statusCode).toBe(200); + const scopedLogin = await this.app.inject({ + method: 'POST', + payload: { password, username }, + url: '/v1/auth/login' + }); + expect(scopedLogin.statusCode).toBe(200); + scopedHeaders = { authorization: `Bearer ${scopedLogin.json<{ accessToken: string }>().accessToken}` }; + }); + + beforeEach(() => { + cleanup = vi.spyOn(this.app.get(StorageService), 'deleteObjects').mockResolvedValue(undefined); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should delete a record carrying a file and request cleanup after its rows are gone', async () => { + const { + records: [record] + } = await seed([groupId]); + cleanup.mockImplementationOnce(async () => { + expect(await prisma.instrumentRecord.findUnique({ where: { id: record!.id } })).toBeNull(); + expect(await prisma.instrumentRecordFile.count({ where: { recordId: record!.id } })).toBe(0); + }); + const response = await this.app.inject({ + headers: scopedHeaders, + method: 'DELETE', + url: `/v1/instrument-records/${record!.id}` + }); + expect(response.statusCode).toBe(204); + expect(cleanup).toHaveBeenCalledWith([{ groupId, location: { basename: 'file', index: 0 }, recordId: record!.id }]); + expect(await prisma.session.findUnique({ where: { id: record!.sessionId } })).not.toBeNull(); + }); + + it('should hide inaccessible records just like missing records and preserve their files', async () => { + const { + records: [record] + } = await seed([otherGroupId]); + for (const id of [record!.id, '000000000000000000000000']) { + const response = await this.app.inject({ + headers: scopedHeaders, + method: 'DELETE', + url: `/v1/instrument-records/${id}` + }); + expect(response.statusCode).toBe(404); + } + expect(await prisma.instrumentRecordFile.count({ where: { recordId: record!.id } })).toBe(1); + expect(cleanup).not.toHaveBeenCalled(); + }); + + it('should force-delete all records, file rows, sessions and the subject', async () => { + const { records, subject } = await seed([groupId, otherGroupId]); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'DELETE', + url: `/v1/subjects/${subject.id}?force=true` + }); + expect(response.statusCode).toBe(200); + expect(await prisma.subject.findUnique({ where: { id: subject.id } })).toBeNull(); + expect(await prisma.session.count({ where: { subjectId: subject.id } })).toBe(0); + expect(await prisma.instrumentRecord.count({ where: { subjectId: subject.id } })).toBe(0); + expect( + await prisma.instrumentRecordFile.count({ where: { recordId: { in: records.map((record) => record.id) } } }) + ).toBe(0); + expect(cleanup).toHaveBeenCalledWith( + records.map((record) => ({ + groupId: record.groupId, + location: { basename: 'file', index: 0 }, + recordId: record.id + })) + ); + }); + + it('should roll back force deletion when some records are outside the caller scope', async () => { + const { records, subject } = await seed([groupId, otherGroupId]); + const response = await this.app.inject({ + headers: scopedHeaders, + method: 'DELETE', + url: `/v1/subjects/${subject.id}?force=true` + }); + expect(response.statusCode).toBe(500); + expect(await prisma.instrumentRecord.count({ where: { subjectId: subject.id } })).toBe(2); + expect( + await prisma.instrumentRecordFile.count({ where: { recordId: { in: records.map((record) => record.id) } } }) + ).toBe(2); + expect(await prisma.session.count({ where: { subjectId: subject.id } })).toBe(2); + expect(await prisma.subject.findUnique({ where: { id: subject.id } })).not.toBeNull(); + expect(cleanup).not.toHaveBeenCalled(); + }); + + it('should refuse an inaccessible subject before deleting any dependent rows', async () => { + const { + records: [record], + subject + } = await seed([otherGroupId]); + await prisma.subject.update({ data: { groupIds: [otherGroupId] }, where: { id: subject.id } }); + const response = await this.app.inject({ + headers: scopedHeaders, + method: 'DELETE', + url: `/v1/subjects/${subject.id}?force=true` + }); + expect(response.statusCode).toBe(404); + expect(await prisma.instrumentRecordFile.count({ where: { recordId: record!.id } })).toBe(1); + expect(cleanup).not.toHaveBeenCalled(); + }); + + it('should keep non-force subject deletion restricted while files and records exist', async () => { + const { + records: [record], + subject + } = await seed([groupId]); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'DELETE', + url: `/v1/subjects/${subject.id}` + }); + expect(response.statusCode).toBe(500); + expect(await prisma.instrumentRecordFile.count({ where: { recordId: record!.id } })).toBe(1); + expect(cleanup).not.toHaveBeenCalled(); + }); + + it('should keep a committed record deletion successful when storage fails and log the orphan', async () => { + const { + records: [record] + } = await seed([groupId]); + const error = new Error('storage unavailable'); + cleanup.mockRejectedValueOnce(error); + const log = vi.spyOn(LoggingService.prototype, 'error').mockImplementation(() => undefined); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'DELETE', + url: `/v1/instrument-records/${record!.id}` + }); + expect(response.statusCode).toBe(204); + expect(await prisma.instrumentRecord.findUnique({ where: { id: record!.id } })).toBeNull(); + expect(await prisma.instrumentRecordFile.count({ where: { recordId: record!.id } })).toBe(0); + expect(log.mock.lastCall?.[0]).toMatchObject({ error }); + expect(log.mock.lastCall?.[0]).toHaveProperty( + 'message', + expect.stringContaining('orphaned objects require cleanup') + ); + }); +}); diff --git a/testing/src/specs/record-file-deletion.spec.ts b/testing/src/specs/record-file-deletion.spec.ts new file mode 100644 index 000000000..1925ba806 --- /dev/null +++ b/testing/src/specs/record-file-deletion.spec.ts @@ -0,0 +1,42 @@ +import { expect, test } from '../support/fixtures'; + +test('should delete a record carrying file metadata and make its files inaccessible', async ({ + adminToken, + api, + apiRequestContext, + uniqueId +}) => { + const headers = { Authorization: `Bearer ${adminToken}` }; + const session = await api.createSession(null, { id: `file-deletion-${uniqueId}` }); + const instrumentId = await api.findInstrumentIdByName('ARBITRARY_SINGLE_FILE'); + const created = await apiRequestContext.post('/api/v1/instrument-records', { + data: { + data: {}, + date: new Date(), + instrumentId, + sessionId: session.id, + subjectId: session.subjectId + }, + headers + }); + expect(created.status()).toBe(201); + const record = (await created.json()) as { id: string }; + const recordPath = `/api/v1/instrument-records/${record.id}`; + const name = `file-${uniqueId}.txt`; + + const completed = await apiRequestContext.post(`${recordPath}/files/upload-complete`, { + data: { uploads: { file: [{ location: { basename: 'file', index: 0 }, name, size: 4 }] } }, + headers + }); + expect(completed.status()).toBe(201); + const files = await apiRequestContext.get(`${recordPath}/files`, { headers }); + expect(files.status()).toBe(200); + expect(await files.json()).toMatchObject({ file: [{ name, size: 4 }] }); + + const deleted = await apiRequestContext.delete(recordPath, { headers }); + expect(deleted.status()).toBe(204); + const missingRecord = await apiRequestContext.get(recordPath, { headers }); + expect(missingRecord.status()).toBe(404); + const missingFiles = await apiRequestContext.get(`${recordPath}/files`, { headers }); + expect(missingFiles.status()).toBe(404); +});