Fix deletion of records and subjects with uploaded files

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<LoggingService>;
+  let storageService: MockedInstance<StorageService>;
+  let fileModel: MockedInstance<Model<'InstrumentRecordFile'>>;
+  const transaction = vi.fn();
   let instrumentRecordsService: InstrumentRecordsService;
   let instrumentRecordModel: MockedInstance<Model<'InstrumentRecord'>>;
   let sessionModel: MockedInstance<Model<'Session'>>;
@@ -31,10 +35,27 @@ describe('InstrumentRecordsService', () => {
   let usersService: MockedInstance<UsersService>;
 
   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<unknown[]>();
+      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);

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<boolean> {

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<ConfigService>;
+  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();
+  });
+});

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<void> {
+    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);