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 28dd89a50..f3b9be0eb 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 @@ -184,7 +184,7 @@ describe('InstrumentRecordsService', () => { beforeEach(() => { instrumentsService.findById.mockResolvedValue(mockInstrument as any); subjectsService.createMany.mockResolvedValue([] as any); - sessionsService.create.mockResolvedValue(mockSession as any); + sessionsService.createMany.mockResolvedValue([mockSession] as any); sessionsService.deleteByIds.mockResolvedValue(undefined as any); instrumentRecordModel.createMany.mockResolvedValue([] as any); instrumentRecordModel.findMany.mockResolvedValue([] as any); @@ -208,13 +208,63 @@ describe('InstrumentRecordsService', () => { expect(result).toStrictEqual([foreignRecord]); }); - it('should call sessionsService.create with the provided username', async () => { + it('should create the sessions in one batched call carrying the provided username', async () => { usersService.findByUsername.mockResolvedValueOnce({ groups: [{ id: 'group-1' }], username: 'validuser' } as any); await instrumentRecordsService.upload({ ...baseUploadData, groupId: 'group-1', username: 'validuser' }); expect(usersService.findByUsername).toHaveBeenCalledWith('validuser', undefined); - expect(sessionsService.create).toHaveBeenCalledWith(expect.objectContaining({ username: 'validuser' })); + expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).toHaveBeenCalledTimes(1); + expect(sessionsService.createMany).toHaveBeenCalledWith( + expect.objectContaining({ groupId: 'group-1', type: 'RETROSPECTIVE', username: 'validuser' }), + undefined + ); + }); + + it('should create the sessions with the caller ability, so their group and user lookups are scoped', async () => { + const ability = createAppAbility([{ action: 'create', subject: 'InstrumentRecord' }]); + + await instrumentRecordsService.upload({ ...baseUploadData }, { ability }); + + expect(sessionsService.createMany).toHaveBeenCalledWith(expect.anything(), { ability }); + }); + + it('should batch every record into a single session creation call', async () => { + const records = Array.from({ length: 25 }, (_, i) => ({ + data: { answer: i }, + date: new Date(), + subjectId: `subject-${i}` + })); + sessionsService.createMany.mockResolvedValueOnce( + records.map((_, i) => ({ ...mockSession, id: `session-${i}` })) as any + ); + + await instrumentRecordsService.upload({ ...baseUploadData, records }); + + expect(sessionsService.createMany).toHaveBeenCalledTimes(1); + const [call] = sessionsService.createMany.mock.lastCall as [{ entries: unknown[] }]; + expect(call.entries).toHaveLength(25); + }); + + it('should pair each record with the session created for it, by position', async () => { + const records = [ + { data: { answer: 1 }, date: new Date(), subjectId: 'subject-a' }, + { data: { answer: 2 }, date: new Date(), subjectId: 'subject-b' } + ]; + sessionsService.createMany.mockResolvedValueOnce([ + { ...mockSession, id: 'session-a' }, + { ...mockSession, id: 'session-b' } + ] as any); + + await instrumentRecordsService.upload({ ...baseUploadData, records }); + + expect(instrumentRecordModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [ + { sessionId: 'session-a', subjectId: 'subject-a' }, + { sessionId: 'session-b', subjectId: 'subject-b' } + ] + }); }); it('should throw a ForbiddenException when a non-admin user uploads without a group', async () => { @@ -227,7 +277,7 @@ describe('InstrumentRecordsService', () => { instrumentRecordsService.upload({ ...baseUploadData, username: 'validuser' }) ).rejects.toBeInstanceOf(ForbiddenException); - expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).not.toHaveBeenCalled(); }); it('should throw a ForbiddenException when a user uploads to a group they are not a member of', async () => { @@ -240,7 +290,7 @@ describe('InstrumentRecordsService', () => { instrumentRecordsService.upload({ ...baseUploadData, groupId: 'group-1', username: 'validuser' }) ).rejects.toBeInstanceOf(ForbiddenException); - expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).not.toHaveBeenCalled(); }); it('should reject and not create any sessions when an unknown username is provided', async () => { @@ -252,18 +302,85 @@ describe('InstrumentRecordsService', () => { NotFoundException ); - expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).not.toHaveBeenCalled(); }); - it('should call sessionsService.create with username undefined when no username is provided', async () => { + it('should create the sessions with username undefined when no username is provided', async () => { await instrumentRecordsService.upload({ ...baseUploadData }); expect(usersService.findByUsername).not.toHaveBeenCalled(); - expect(sessionsService.create).toHaveBeenCalledWith(expect.objectContaining({ username: undefined })); + expect(sessionsService.createMany).toHaveBeenCalledWith( + expect.objectContaining({ username: undefined }), + undefined + ); + }); + + it('should reject an invalid record before creating any sessions', async () => { + instrumentsService.findById.mockResolvedValue({ + ...mockInstrument, + validationSchema: { safeParse: () => ({ error: { issues: [] }, success: false }) } + } as any); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toBeInstanceOf( + UnprocessableEntityException + ); + + expect(sessionsService.createMany).not.toHaveBeenCalled(); + expect(instrumentRecordModel.createMany).not.toHaveBeenCalled(); + }); + + it('should report which record failed and why, so a rejected batch can be corrected', async () => { + const issues = [{ message: 'Required', path: ['answer'] }]; + instrumentsService.findById.mockResolvedValue({ + ...mockInstrument, + validationSchema: { + safeParse: (data: any) => + data.answer === 2 ? { error: { issues }, success: false } : { data, success: true } + } + } as any); + + await expect( + instrumentRecordsService.upload({ + ...baseUploadData, + records: [ + { data: { answer: 1 }, date: new Date(), subjectId: 'subject-1' }, + { data: { answer: 2 }, date: new Date(), subjectId: 'subject-2' } + ] + }) + ).rejects.toMatchObject({ + response: { issues, message: expect.stringContaining('at index 1') } + }); + }); + + it('should roll back the sessions when the record insert fails, so none is left without records', async () => { + instrumentRecordModel.createMany.mockRejectedValueOnce(new Error('insert failed')); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toThrow('insert failed'); + + expect(sessionsService.deleteByIds).toHaveBeenCalledWith(['session-1']); + }); + + it('should keep the sessions when only the read-back fails, since the records already reference them', async () => { + instrumentRecordModel.findMany.mockRejectedValueOnce(new Error('read-back failed')); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toThrow('read-back failed'); + + expect(sessionsService.deleteByIds).not.toHaveBeenCalled(); + }); + + // The bulk payload cannot carry a file and this path never attaches one, so such a record could + // only ever be incomplete. Refusing it is the same call `create` makes for series instruments. + it('should reject a file instrument rather than write a record its files can never reach', async () => { + instrumentsService.findById.mockResolvedValue({ ...mockInstrument, kind: 'FILE' } as any); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toBeInstanceOf( + UnprocessableEntityException + ); + + expect(sessionsService.createMany).not.toHaveBeenCalled(); + expect(instrumentRecordModel.createMany).not.toHaveBeenCalled(); }); - // `pending` is intentionally not written on create; the find-side OR filter treats missing and - // false `pending` alike (see the 'find' describe block), so records stay query-visible without it. it('should create records via createMany with the processed record data', async () => { await instrumentRecordsService.upload({ ...baseUploadData }); diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index 8f35d9ee7..e76b64e51 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -24,7 +24,7 @@ import type { UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; import { Prisma } from '@prisma/client'; -import type { InstrumentRecord as PrismaInstrumentRecord, Session } from '@prisma/client'; +import type { InstrumentRecord as PrismaInstrumentRecord } from '@prisma/client'; import { isNumber, mergeWith, pickBy } from 'lodash-es'; import { accessibleQuery } from '@/auth/ability.utils'; @@ -34,7 +34,6 @@ import { GroupsService } from '@/groups/groups.service'; import { InstrumentsService } from '@/instruments/instruments.service'; import { SessionsService } from '@/sessions/sessions.service'; import { StorageService } from '@/storage/storage.service'; -import { CreateSubjectDto } from '@/subjects/dto/create-subject.dto'; import { SubjectsService } from '@/subjects/subjects.service'; import { UsersService } from '@/users/users.service'; @@ -398,6 +397,14 @@ export class InstrumentRecordsService { `Cannot create instrument record for series instrument '${instrument.id}'` ); } + // A bulk-uploaded file record could never be completed: the payload schema has nowhere to carry a + // file, this path never attaches one, and the response ids the client would need to attach one + // afterwards are discarded. Refuse it rather than write a record that can only ever be pending. + if (instrument.kind === 'FILE') { + throw new UnprocessableEntityException( + `Cannot create instrument record for file instrument '${instrument.id}': files cannot be attached to a bulk upload` + ); + } if (username) { const user = await this.usersService.findByUsername(username, options); @@ -409,70 +416,66 @@ export class InstrumentRecordsService { } } - const createdSessionsArray: Session[] = []; - - try { - const subjectIdList = records.map(({ subjectId }) => { - const subjectToAdd: CreateSubjectDto = { id: subjectId }; - - return subjectToAdd; - }); - - await this.subjectsService.createMany(subjectIdList); - - const preProcessedRecords = await Promise.all( - records.map(async (record) => { - const { data: rawData, date, subjectId } = record; + // Every record is validated before anything is written, so a malformed record in the middle of a + // batch rejects the request without first creating sessions that then have to be rolled back. + const validatedRecords = records.map((record, index) => { + const parseResult = instrument.validationSchema.safeParse(this.parseJson(record.data)); + if (!parseResult.success) { + throw new UnprocessableEntityException({ + error: 'Unprocessable Entity', + issues: parseResult.error.issues, + message: `Data received for record at index ${index} does not pass validation schema of instrument '${instrument.id}'`, + statusCode: 422 + }); + } + return { data: parseResult.data, date: record.date, subjectId: record.subjectId }; + }); - // Validate data - const parseResult = instrument.validationSchema.safeParse(this.parseJson(rawData)); - if (!parseResult.success) { - console.error(parseResult.error.issues); - throw new UnprocessableEntityException( - `Data received for record does not pass validation schema of instrument '${instrument.id}'` - ); - } + // One batched call rather than one session creation per record, which cost several queries each. + // Returned in input order, so each record can be paired with its session by index. + const sessions = await this.sessionsService.createMany( + { + entries: validatedRecords.map((record) => ({ + date: record.date, + subjectData: { id: record.subjectId } + })), + groupId: groupId ?? null, + type: 'RETROSPECTIVE', + username: username ?? undefined + }, + options + ); - const session = await this.sessionsService.create({ - date: date, - groupId: groupId ?? null, - subjectData: { id: subjectId }, - type: 'RETROSPECTIVE', - username: username ?? undefined - }); - - createdSessionsArray.push(session); - - const computedMeasures = instrument.measures - ? this.instrumentMeasuresService.computeMeasures(instrument.measures, parseResult.data) - : null; - - return { - computedMeasures, - data: this.serializeData(parseResult.data), - date, - groupId, - instrumentId, - pending: false, - sessionId: session.id, - subjectId - }; - }) - ); + // Only the insert is rolled back on failure. Deleting the sessions after it has succeeded would + // strand the records that now reference them, so the read-back below sits outside the catch. + try { await this.instrumentRecordModel.createMany({ - data: preProcessedRecords - }); - - // Only the rows this call wrote. Filtering by instrument alone returned every group's records - // when `groupId` was omitted, and `accessibleQuery` cannot scope this read: a STANDARD uploader - // holds no `read InstrumentRecord` rule, which makes CASL throw rather than filter. - return this.instrumentRecordModel.findMany({ - where: { sessionId: { in: createdSessionsArray.map((session) => session.id) } } + data: validatedRecords.map((record, index) => ({ + computedMeasures: instrument.measures + ? this.instrumentMeasuresService.computeMeasures(instrument.measures, record.data) + : null, + data: this.serializeData(record.data), + date: record.date, + groupId, + instrumentId, + pending: false, + sessionId: sessions[index]!.id, + subjectId: record.subjectId + })) }); } catch (err) { - await this.sessionsService.deleteByIds(createdSessionsArray.map((session) => session.id)); + await this.sessionsService.deleteByIds(sessions.map((session) => session.id)); throw err; } + + // Only the rows this call wrote. Filtering by instrument alone returned every group's records + // when `groupId` was omitted, and `accessibleQuery` cannot scope this read: a STANDARD uploader + // holds no `read InstrumentRecord` rule, which makes CASL throw rather than filter. + return this.instrumentRecordModel.findMany({ + where: { + sessionId: { in: sessions.map((session) => session.id) } + } + }); } private getInstrumentById(instrumentId: string) { diff --git a/apps/api/src/sessions/__tests__/sessions.service.spec.ts b/apps/api/src/sessions/__tests__/sessions.service.spec.ts new file mode 100644 index 000000000..74cff40bc --- /dev/null +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -0,0 +1,237 @@ +import { getModelToken, LoggingService } from '@douglasneuroinformatics/libnest'; +import type { Model } from '@douglasneuroinformatics/libnest'; +import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; +import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { InternalServerErrorException, NotFoundException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { createAppAbility } from '@/auth/ability.utils'; +import { GroupsService } from '@/groups/groups.service'; +import { SubjectsService } from '@/subjects/subjects.service'; +import { UsersService } from '@/users/users.service'; + +import { SessionsService } from '../sessions.service'; + +describe('SessionsService', () => { + let sessionsService: SessionsService; + let sessionModel: MockedInstance>; + let groupsService: MockedInstance; + let subjectsService: MockedInstance; + let usersService: MockedInstance; + + const entry = (id: string) => ({ date: new Date(), subjectData: { id } }); + + beforeEach(async () => { + const moduleRef = await Test.createTestingModule({ + providers: [ + SessionsService, + MockFactory.createForModelToken(getModelToken('Session')), + MockFactory.createForService(GroupsService), + MockFactory.createForService(LoggingService), + MockFactory.createForService(SubjectsService), + MockFactory.createForService(UsersService) + ] + }).compile(); + + sessionsService = moduleRef.get(SessionsService); + sessionModel = moduleRef.get(getModelToken('Session')); + groupsService = moduleRef.get(GroupsService); + subjectsService = moduleRef.get(SubjectsService); + usersService = moduleRef.get(UsersService); + + subjectsService.createMany.mockResolvedValue([] as any); + subjectsService.addGroupForSubjects.mockResolvedValue({ count: 0 } as any); + sessionModel.createMany.mockResolvedValue({ count: 1 } as any); + sessionModel.findMany.mockImplementation(({ where }: any) => + Promise.resolve(where.id.in.map((id: string) => ({ id }))) + ); + }); + + describe('createMany', () => { + it('should not touch the database when there are no entries', async () => { + await expect(sessionsService.createMany({ entries: [], groupId: null, type: 'RETROSPECTIVE' })).resolves.toEqual( + [] + ); + expect(sessionModel.createMany).not.toHaveBeenCalled(); + }); + + // A session whose groupId is unset is invisible to a group manager, whose Session rule is + // { groupId: { in: [...] } }, and uncounted by every group-scoped query. + it('should set the groupId on a session for a subject that is already a member of the group', async () => { + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + await sessionsService.createMany({ + entries: [entry('subject-1')], + groupId: 'group-1', + type: 'RETROSPECTIVE' + }); + + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ data: [{ groupId: 'group-1' }] }); + }); + + it('should associate the batch with the group in a single write', async () => { + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + await sessionsService.createMany({ + entries: [entry('subject-1'), entry('subject-2')], + groupId: 'group-1', + type: 'RETROSPECTIVE' + }); + + // Every subject is handed over; which of them actually need the write is decided by the query + // `SubjectsService` builds, not by a membership list read here. + expect(subjectsService.addGroupForSubjects).toHaveBeenCalledExactlyOnceWith( + ['subject-1', 'subject-2'], + 'group-1' + ); + }); + + it('should not associate anything when no group was supplied', async () => { + await sessionsService.createMany({ entries: [entry('subject-1')], groupId: null, type: 'RETROSPECTIVE' }); + + expect(groupsService.findById).not.toHaveBeenCalled(); + expect(subjectsService.addGroupForSubjects).not.toHaveBeenCalled(); + }); + + it('should return the sessions in the order the entries were given, so callers can pair by index', async () => { + // The read-back is a findMany, which is free to return documents in any order. + sessionModel.findMany.mockImplementationOnce(({ where }: any) => + Promise.resolve([...where.id.in].reverse().map((id: string) => ({ id }))) + ); + + const sessions = await sessionsService.createMany({ + entries: [entry('subject-a'), entry('subject-b')], + groupId: null, + type: 'RETROSPECTIVE' + }); + + const [call] = sessionModel.createMany.mock.lastCall as [{ data: { id: string; subjectId: string }[] }]; + expect(sessions.map((session) => session.id)).toStrictEqual(call.data.map((session) => session.id)); + expect(call.data.map((session) => session.subjectId)).toStrictEqual(['subject-a', 'subject-b']); + }); + + it('should create every session in one call rather than one call per entry', async () => { + const entries = Array.from({ length: 20 }, (_, i) => entry(`subject-${i}`)); + + await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE' }); + + expect(sessionModel.createMany).toHaveBeenCalledOnce(); + expect(sessionModel.createMany.mock.lastCall?.[0].data).toHaveLength(20); + }); + + it('should resolve the user once for the whole batch and stamp it on every session', async () => { + const entries = [entry('subject-a'), entry('subject-b')]; + usersService.findByUsername.mockResolvedValueOnce({ id: 'user-1', username: 'someone' } as any); + + await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE', username: 'someone' }); + + expect(usersService.findByUsername).toHaveBeenCalledOnce(); + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [{ userId: 'user-1' }, { userId: 'user-1' }] + }); + }); + + it('should look the group up with the caller ability, so a group they cannot read is refused', async () => { + const ability = createAppAbility([{ action: 'read', conditions: { id: { in: ['group-1'] } }, subject: 'Group' }]); + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + await sessionsService.createMany( + { entries: [entry('subject-1')], groupId: 'group-1', type: 'RETROSPECTIVE' }, + { ability } + ); + + expect(groupsService.findById).toHaveBeenCalledWith('group-1', { ability }); + }); + + it('should write nothing when the caller may not read the group', async () => { + groupsService.findById.mockRejectedValueOnce(new NotFoundException()); + + await expect( + sessionsService.createMany({ entries: [entry('subject-1')], groupId: 'group-2', type: 'RETROSPECTIVE' }) + ).rejects.toBeInstanceOf(NotFoundException); + + expect(subjectsService.createMany).not.toHaveBeenCalled(); + expect(sessionModel.createMany).not.toHaveBeenCalled(); + }); + + it('should look the user up among the users the caller may read, so a caller cannot name anyone', async () => { + const ability = createAppAbility([{ action: 'read', conditions: { id: 'user-1' }, subject: 'User' }]); + usersService.findByUsername.mockResolvedValueOnce({ id: 'user-1', username: 'someone' } as any); + + await sessionsService.createMany( + { entries: [entry('subject-1')], groupId: null, type: 'RETROSPECTIVE', username: 'someone' }, + { ability } + ); + + expect(usersService.findByUsername).toHaveBeenCalledWith('someone', { ability }); + }); + + it('should refuse a username it cannot find rather than create sessions with no user', async () => { + usersService.findByUsername.mockRejectedValueOnce(new NotFoundException()); + + await expect( + sessionsService.createMany({ + entries: [entry('subject-1')], + groupId: null, + type: 'RETROSPECTIVE', + username: 'nobody' + }) + ).rejects.toBeInstanceOf(NotFoundException); + + expect(subjectsService.createMany).not.toHaveBeenCalled(); + expect(sessionModel.createMany).not.toHaveBeenCalled(); + }); + + // Without this, a session missing from the read-back would be handed to the caller as + // `undefined` typed `Session`, and `POST /sessions` would answer 201 with an empty body. + it('should throw rather than return a gap when the read-back misses a created session', async () => { + sessionModel.findMany.mockImplementationOnce(({ where }: any) => + Promise.resolve(where.id.in.slice(1).map((id: string) => ({ id }))) + ); + + await expect( + sessionsService.createMany({ + entries: [entry('subject-a'), entry('subject-b')], + groupId: null, + type: 'RETROSPECTIVE' + }) + ).rejects.toBeInstanceOf(InternalServerErrorException); + }); + + it('should delete the sessions it inserted when the read-back fails, since the caller never receives them to roll back', async () => { + sessionModel.findMany.mockRejectedValueOnce(new Error('read-back failed')); + + await expect( + sessionsService.createMany({ + entries: [entry('subject-a'), entry('subject-b')], + groupId: null, + type: 'RETROSPECTIVE' + }) + ).rejects.toThrow('read-back failed'); + + const [{ data }] = sessionModel.createMany.mock.lastCall as [{ data: { id: string }[] }]; + expect(sessionModel.deleteMany).toHaveBeenCalledWith({ + where: { AND: [{}], id: { in: data.map((session) => session.id) } } + }); + }); + }); + + describe('create', () => { + it('should return the single session created for the entry', async () => { + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + const session = await sessionsService.create({ + date: new Date(), + groupId: 'group-1', + subjectData: { id: 'subject-1' }, + type: 'IN_PERSON' + }); + + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [{ groupId: 'group-1', subjectId: 'subject-1', type: 'IN_PERSON' }] + }); + expect(session.id).toBe(sessionModel.createMany.mock.lastCall?.[0].data[0].id); + }); + }); +}); diff --git a/apps/api/src/sessions/sessions.controller.ts b/apps/api/src/sessions/sessions.controller.ts index 1ff3e2806..e627658c8 100644 --- a/apps/api/src/sessions/sessions.controller.ts +++ b/apps/api/src/sessions/sessions.controller.ts @@ -17,8 +17,8 @@ export class SessionsController { @ApiOperation({ description: 'Create Session' }) @Post() @RouteAccess({ action: 'create', subject: 'Session' }) - create(@Body() data: CreateSessionDto): Promise { - return this.sessionsService.create(data); + create(@Body() data: CreateSessionDto, @CurrentUser('ability') ability: AppAbility): Promise { + return this.sessionsService.create(data, { ability }); } @ApiOperation({ description: 'Find all sessions and usernames attached to them' }) diff --git a/apps/api/src/sessions/sessions.module.ts b/apps/api/src/sessions/sessions.module.ts index 4a7ea450c..737809bfc 100644 --- a/apps/api/src/sessions/sessions.module.ts +++ b/apps/api/src/sessions/sessions.module.ts @@ -2,6 +2,7 @@ import { Module } from '@nestjs/common'; import { GroupsModule } from '@/groups/groups.module'; import { SubjectsModule } from '@/subjects/subjects.module'; +import { UsersModule } from '@/users/users.module'; import { SessionsController } from './sessions.controller'; import { SessionsService } from './sessions.service'; @@ -9,7 +10,7 @@ import { SessionsService } from './sessions.service'; @Module({ controllers: [SessionsController], exports: [SessionsService], - imports: [GroupsModule, SubjectsModule], + imports: [GroupsModule, SubjectsModule, UsersModule], providers: [SessionsService] }) export class SessionsModule {} diff --git a/apps/api/src/sessions/sessions.service.ts b/apps/api/src/sessions/sessions.service.ts index 2b0889424..88d8be72c 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -1,25 +1,30 @@ -import { InjectModel, InjectPrismaClient, LoggingService } from '@douglasneuroinformatics/libnest'; +import { InjectModel, LoggingService } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; import { Injectable, InternalServerErrorException, NotFoundException } from '@nestjs/common'; import type { Group } from '@opendatacapture/schemas/group'; import type { CreateSessionData } from '@opendatacapture/schemas/session'; -import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; -import type { Prisma, Session, Subject, User } from '@prisma/client'; +import type { Prisma, Session } from '@prisma/client'; +import { ObjectId } from 'mongodb'; import { accessibleQuery } from '@/auth/ability.utils'; -import type { RuntimePrismaClient } from '@/core/prisma'; import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; import { SubjectsService } from '@/subjects/subjects.service'; +import { UsersService } from '@/users/users.service'; + +/** The batched form of `CreateSessionData`: what varies per session, and what the batch shares. */ +type CreateManySessionsData = Pick & { + entries: Pick[]; +}; @Injectable() export class SessionsService { constructor( - @InjectPrismaClient() private readonly prismaClient: RuntimePrismaClient, @InjectModel('Session') private readonly sessionModel: Model<'Session'>, private readonly groupsService: GroupsService, private readonly loggingService: LoggingService, - private readonly subjectsService: SubjectsService + private readonly subjectsService: SubjectsService, + private readonly usersService: UsersService ) {} async count(where: Prisma.SessionWhereInput = {}, { ability }: EntityOperationOptions = {}) { @@ -28,55 +33,73 @@ export class SessionsService { }); } - async create({ date, groupId, subjectData, type, username }: CreateSessionData): Promise { - this.loggingService.debug({ message: 'Attempting to create session' }); - const subject = await this.resolveSubject(subjectData); - - let user: null | Omit = null; + async create( + { date, groupId, subjectData, type, username }: CreateSessionData, + options?: EntityOperationOptions + ): Promise { + const [session] = await this.createMany( + { + entries: [{ date, subjectData }], + groupId, + type, + username + }, + options + ); + return session!; + } - if (username) { - user = await this.prismaClient.user.findFirst({ - where: { - username: username - } - }); + /** + * Create one session per entry, in a fixed number of queries rather than a fixed number per entry. + * + * Returned in the same order as `entries`, so a caller can pair each session with the input that + * produced it without a second lookup. + */ + async createMany( + { entries, groupId, type, username }: CreateManySessionsData, + { ability }: EntityOperationOptions = {} + ): Promise { + if (entries.length === 0) { + return []; } + this.loggingService.debug({ message: `Attempting to create ${entries.length} session(s)` }); - // If the subject is not yet associated with the group, check it exists then append it - let group: Group | null = null; - if (groupId && !subject.groupIds.includes(groupId)) { - group = await this.groupsService.findById(groupId); - if (group) { - await this.subjectsService.addGroupForSubject(subject.id, group.id); - } + const user = username ? await this.usersService.findByUsername(username, { ability }) : null; + const group: Group | null = groupId ? await this.groupsService.findById(groupId, { ability }) : null; + + // The subject writes are left unscoped. A caller's `read Subject` rule covers only its own + // groups, so a scoped existence check would miss a subject from another group and try to create + // it again, and a STANDARD caller holds no `update Subject` rule for the group association. + await this.subjectsService.createMany(entries.map((entry) => entry.subjectData)); + if (group) { + await this.subjectsService.addGroupForSubjects( + Array.from(new Set(entries.map((entry) => entry.subjectData.id))), + group.id + ); } - const { id } = await this.sessionModel.create({ - data: { - date, - group: group - ? { - connect: { id: group.id } - } - : undefined, - subject: { - connect: { id: subject.id } - }, + // Generated up front so the sessions can be read back in the order they were requested; + // createMany does not return the documents it inserted. + const ids = entries.map(() => new ObjectId().toHexString()); + + await this.sessionModel.createMany({ + data: entries.map((entry, index) => ({ + date: entry.date, + groupId: group?.id ?? null, + id: ids[index]!, + subjectId: entry.subjectData.id, type, - user: user - ? { - connect: { id: user.id } - } - : undefined - } + userId: user?.id ?? null + })) }); - return (await this.sessionModel.findUnique({ - include: { - subject: true - }, - where: { id } - }))!; + // The caller never receives these sessions if the read-back fails, so it cannot roll them back. + try { + return await this.readBackInOrder(ids); + } catch (err) { + await this.deleteByIds(ids); + throw err; + } } async deleteById(id: string, { ability }: EntityOperationOptions = {}) { @@ -126,18 +149,18 @@ export class SessionsService { return session; } - /** Get the subject if they exist, otherwise create them */ - private async resolveSubject(subjectData: CreateSubjectData) { - this.loggingService.debug({ message: 'Attempting to resolve subject', subjectData }); - let subject: Subject; - try { - subject = await this.subjectsService.findById(subjectData.id); - } catch (err) { - if (!(err instanceof NotFoundException)) { - throw new InternalServerErrorException('Unexpected Error', { cause: err }); + private async readBackInOrder(ids: string[]) { + const created = await this.sessionModel.findMany({ + include: { subject: true }, + where: { id: { in: ids } } + }); + const byId = new Map(created.map((session) => [session.id, session])); + return ids.map((id) => { + const session = byId.get(id); + if (!session) { + throw new InternalServerErrorException(`Failed to read back created session with id: ${id}`); } - subject = await this.subjectsService.create(subjectData); - } - return subject; + return session; + }); } } diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index ea12d89d3..ce9734716 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -4,6 +4,7 @@ import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; import { ConflictException, NotFoundException } from '@nestjs/common'; import { Test } from '@nestjs/testing'; +import type { Subject } from '@prisma/client'; import { pick } from 'lodash-es'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -47,6 +48,29 @@ describe('SubjectsService', () => { prismaClient = moduleRef.get(PRISMA_CLIENT_TOKEN); }); + describe('addGroupForSubjects', () => { + // The exclusion belongs in the query, not the caller: mongodb arrays admit duplicates, so a + // caller filtering against a list it read earlier would push the id twice under concurrency. + it('should skip subjects already in the group from within the query itself', async () => { + await subjectsService.addGroupForSubjects(['subject-1', 'subject-2'], 'group-1'); + + expect(subjectModel.updateMany.mock.lastCall?.[0]).toMatchObject({ + data: { groupIds: { push: 'group-1' } }, + where: { + id: { in: ['subject-1', 'subject-2'] }, + NOT: { groupIds: { has: 'group-1' } } + } + }); + }); + + it('should associate every subject in one write rather than one per subject', async () => { + await subjectsService.addGroupForSubjects(['subject-1', 'subject-2', 'subject-3'], 'group-1'); + + expect(subjectModel.updateMany).toHaveBeenCalledOnce(); + expect(subjectModel.update).not.toHaveBeenCalled(); + }); + }); + describe('create', () => { it('should call the subject model', async () => { const subject = { @@ -74,6 +98,36 @@ describe('SubjectsService', () => { }); }); + describe('createMany', () => { + // `demo.service.ts` hands `sessionsService.create` a whole row, which reaches this method. + it('should keep the demographics of a new subject and drop the fields the caller may not set', async () => { + subjectModel.findMany.mockResolvedValueOnce([]); + const row: Subject = { + createdAt: new Date(0), + dateOfBirth: new Date(2000, 0, 1), + firstName: 'Ada', + groupIds: ['group-9'], + id: 'subject-1', + lastName: 'Lovelace', + sex: 'FEMALE', + updatedAt: new Date(0) + }; + + await subjectsService.createMany([row]); + + expect(subjectModel.createMany.mock.lastCall?.[0].data).toStrictEqual([ + { + dateOfBirth: row.dateOfBirth, + firstName: 'Ada', + groupIds: [], + id: 'subject-1', + lastName: 'Lovelace', + sex: 'FEMALE' + } + ]); + }); + }); + describe('find', () => { it('should return the array returned by the subject model', async () => { subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]); diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index bde50db32..9a9498137 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -16,14 +16,21 @@ export class SubjectsService { @InjectModel('Subject') private readonly subjectModel: Model<'Subject'> ) {} - async addGroupForSubject(subjectId: string, groupId: string, { ability }: EntityOperationOptions = {}) { - return this.subjectModel.update({ - data: { - groupIds: { - push: groupId - } - }, - where: { ...accessibleQuery(ability, 'update', 'Subject'), id: subjectId } + /** + * Associate each of the given subjects with a group, in one write rather than one per subject. + * + * The membership check is part of the query rather than done by the caller: mongodb arrays admit + * duplicates, so a caller filtering against a list it read earlier would push the same group id + * twice if a concurrent request associated the subject in between. + */ + async addGroupForSubjects(subjectIds: string[], groupId: string, { ability }: EntityOperationOptions = {}) { + return this.subjectModel.updateMany({ + data: { groupIds: { push: groupId } }, + where: { + AND: [accessibleQuery(ability, 'update', 'Subject')], + id: { in: subjectIds }, + NOT: { groupIds: { has: groupId } } + } }); } @@ -53,42 +60,45 @@ export class SubjectsService { } async createMany(data: CreateSubjectDto[], { ability }: EntityOperationOptions = {}) { - //filter out all duplicate ids that are planned to be created via a set - const noDuplicatesSet = new Set( - data.map((record) => { - return record.id; - }) - ); - - const subjectIds = Array.from(noDuplicatesSet); + // keyed by id so duplicates within the request collapse, keeping the first entry for each + const requested = new Map(); + for (const subject of data) { + if (!requested.has(subject.id)) { + requested.set(subject.id, subject); + } + } //find the list of subject ids that already exist const existingSubjects = await this.subjectModel.findMany({ select: { id: true }, where: { AND: [accessibleQuery(ability, 'read', 'Subject')], - id: { in: subjectIds } + id: { in: Array.from(requested.keys()) } } }); //create a set of existing ids in the database to filter our to-be-created ids with const existingIds = new Set(existingSubjects.map((subj) => subj.id)); - //Filter out records whose IDs already exist - const subjectsToCreateIds = subjectIds.filter((record) => !existingIds.has(record)); - - const subjectsToCreate: CreateSubjectDto[] = subjectsToCreateIds.map((record) => { - return { - id: record - }; - }); + // The whole entry is kept, not just the id: a subject identified by personal info carries + // demographics that would otherwise be dropped on creation. + const subjectsToCreate = Array.from(requested.values()).filter((subject) => !existingIds.has(subject.id)); //if there are none left to create do not follow through with the command if (subjectsToCreate.length < 1) { return subjectsToCreate; } return this.subjectModel.createMany({ - data: subjectsToCreate, + // Named explicitly rather than spread: callers hand this whole Prisma rows, and a spread would + // carry fields that are not the caller's to set. + data: subjectsToCreate.map(({ dateOfBirth, firstName, id, lastName, sex }) => ({ + dateOfBirth, + firstName, + groupIds: [], + id, + lastName, + sex + })), ...accessibleQuery(ability, 'create', 'Subject') }); } diff --git a/testing/src/specs/sessions.spec.ts b/testing/src/specs/sessions.spec.ts new file mode 100644 index 000000000..8d556548a --- /dev/null +++ b/testing/src/specs/sessions.spec.ts @@ -0,0 +1,96 @@ +import type { CreateSessionData } from '@opendatacapture/schemas/session'; +import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; + +import { ApiClient } from '../support/api-client'; +import { expect, test } from '../support/fixtures'; + +test.describe('sessions', () => { + // A session was previously given its groupId only when the subject was not already a member of + // that group, so every visit after a subject's first was created without one. A group manager's + // Session rule is `{ groupId: { in: [...] } }`, which made those sessions invisible to them and + // uncounted by every group-scoped query, the dashboard trends included. + // + // Seeded into a group of its own, so the count is exactly what this test created. + test("should keep a returning subject's later sessions visible to their group manager", async ({ + api, + apiRequestContext, + uniqueId + }) => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [group.id] }); + const accessToken = await ApiClient.login(apiRequestContext, credentials); + const subjectId = `revisit-${uniqueId}`; + + await api.createSession(group.id, { id: subjectId }); + await api.createSession(group.id, { id: subjectId }); + + const response = await apiRequestContext.get(`/api/v1/sessions?groupId=${group.id}`, { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + const sessions = (await response.json()) as { subjectId: string }[]; + expect(sessions.filter((session) => session.subjectId === subjectId)).toHaveLength(2); + }); + + // Session creation resolves its subjects in a batch, and the batched path once kept only their ids. + test('should create a new subject with the demographics its first session names', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const group = await api.createGroup(); + const subjectData: CreateSubjectData = { + dateOfBirth: new Date('1990-05-17T00:00:00.000Z'), + firstName: 'Ada', + id: `clinical-${uniqueId}`, + lastName: 'Lovelace', + sex: 'FEMALE' + }; + + await api.createSession(group.id, subjectData); + + const response = await apiRequestContext.get(`/api/v1/subjects/${subjectData.id}`, { + headers: { Authorization: `Bearer ${adminToken}` } + }); + + expect(response.status()).toBe(200); + expect(await response.json()).toMatchObject({ + dateOfBirth: '1990-05-17T00:00:00.000Z', + firstName: 'Ada', + lastName: 'Lovelace', + sex: 'FEMALE' + }); + }); + + test('should refuse to create a session in a group the caller does not belong to, and write nothing', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const ownGroup = await api.createGroup(); + const foreignGroup = await api.createGroup(); + const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [ownGroup.id] }); + const accessToken = await ApiClient.login(apiRequestContext, credentials); + const subjectId = `foreign-${uniqueId}`; + const data: CreateSessionData = { + date: new Date(), + groupId: foreignGroup.id, + subjectData: { id: subjectId }, + type: 'IN_PERSON' + }; + + const response = await apiRequestContext.post('/api/v1/sessions', { + data, + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(404); + const subjectResponse = await apiRequestContext.get(`/api/v1/subjects/${subjectId}`, { + headers: { Authorization: `Bearer ${adminToken}` } + }); + expect(subjectResponse.status()).toBe(404); + }); +}); diff --git a/testing/src/specs/upload.spec.ts b/testing/src/specs/upload.spec.ts index 1d4cf9273..4ac446bae 100644 --- a/testing/src/specs/upload.spec.ts +++ b/testing/src/specs/upload.spec.ts @@ -1,3 +1,5 @@ +import type { UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; + import { UploadInstrumentPage } from '../pages/_app/upload/$instrumentId.page'; import { expect, test } from '../support/fixtures'; @@ -5,6 +7,13 @@ import { expect, test } from '../support/fixtures'; // exists in the catalog but is kind SERIES, so it never appears in the upload instrument list. const INSTRUMENT_TITLE = 'Happiness Questionnaire'; +/** A minimal payload satisfying the seeded happiness questionnaire's validation schema. */ +const HAPPINESS_RECORD = { + isSatisfiedOverall: true, + personalLifeSatisfaction: 8, + professionalLifeSatisfaction: 7 +}; + test.describe('upload', () => { test('should upload a valid CSV and create a record for the subject it names @smoke', async ({ getPageModel, @@ -59,4 +68,52 @@ test.describe('upload', () => { await expect(uploadInstrumentPage.errorHeading).not.toBeVisible(); await expect(uploadInstrumentPage.submitButton).toBeVisible(); }); + + // The response was previously every record in the group for the instrument, so a second upload + // leaked the first one's records back to the caller. An earlier upload to the same group and + // instrument is what distinguishes the scoped response from the group-wide one. + test('should answer a batch upload with exactly the records it created', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const instrumentId = await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'); + await api.uploadRecords(group.id, instrumentId, [ + { data: HAPPINESS_RECORD, date: new Date(), subjectId: `upload-${uniqueId}-earlier` } + ]); + + const batch = ['a', 'b', 'c'].map((suffix) => `upload-${uniqueId}-${suffix}`); + const records = await api.uploadRecords( + group.id, + instrumentId, + batch.map((subjectId) => ({ data: HAPPINESS_RECORD, date: new Date(), subjectId })) + ); + + expect(records).toHaveLength(3); + expect(new Set(records.map((record) => record.subjectId))).toStrictEqual(new Set(batch)); + }); + + // The bulk payload has nowhere to carry a file, so a record for a file instrument could only ever + // be incomplete. The upload is refused before anything is written. + test('should refuse a batch upload for a file instrument without writing a session or record', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const group = await api.createGroup(); + const headers = { Authorization: `Bearer ${adminToken}` }; + const subjectId = `file-${uniqueId}`; + const data: UploadInstrumentRecordsData = { + groupId: group.id, + instrumentId: await api.findInstrumentIdByName('ARBITRARY_SINGLE_FILE'), + records: [{ data: {}, date: new Date(), subjectId }] + }; + + const response = await apiRequestContext.post('/api/v1/instrument-records/upload', { data, headers }); + + expect(response.status()).toBe(422); + // Sessions are created only after the subjects they name, so a missing subject means no session. + const subject = await apiRequestContext.get(`/api/v1/subjects/${subjectId}`, { headers }); + expect(subject.status()).toBe(404); + const records = await apiRequestContext.get(`/api/v1/instrument-records?groupId=${group.id}`, { headers }); + expect(await records.json()).toStrictEqual([]); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index aae5072bc..15a825a8e 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,6 +1,9 @@ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { Permissions } from '@opendatacapture/schemas/core'; import type { CreateGroupData, Group } from '@opendatacapture/schemas/group'; +import type { UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; +import type { CreateSessionData, Session } from '@opendatacapture/schemas/session'; +import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; import type { CreateUserData, UpdateUserData, User } from '@opendatacapture/schemas/user'; import type { APIRequestContext } from '@playwright/test'; @@ -9,6 +12,8 @@ import { randomId } from './unique'; const API = '/api/v1'; +type UploadRecord = UploadInstrumentRecordsData['records'][number]; + /** Typed helper for seeding preconditions (groups, users) and authenticating over the API. */ export class ApiClient { private readonly request: APIRequestContext; @@ -48,6 +53,16 @@ export class ApiClient { return group; } + /** Creates a session, and with it the subject it names. */ + async createSession(groupId: null | string, subjectData: CreateSubjectData): Promise { + const data: CreateSessionData = { date: new Date(), groupId, subjectData, type: 'IN_PERSON' }; + return this.expectJson( + this.request.post(`${API}/sessions`, { data, headers: this.authHeaders }), + 201, + 'create session' + ); + } + /** Creates a user (GROUP_MANAGER by default) and returns the login credentials for it. */ async createUser(overrides: Partial = {}): Promise<{ credentials: $LoginCredentials; user: User }> { const username = overrides.username ?? `user_${randomId()}`; @@ -78,6 +93,20 @@ export class ApiClient { ); } + /** The id of a seeded instrument, looked up by the internal name its source file declares. */ + async findInstrumentIdByName(name: string): Promise { + const instruments = await this.expectJson<{ id: string; internal?: { name: string } }[]>( + this.request.get(`${API}/instruments/info`, { headers: this.authHeaders }), + 200, + 'list instruments' + ); + const instrument = instruments.find((candidate) => candidate.internal?.name === name); + if (!instrument) { + throw new Error(`No instrument named '${name}' among ${instruments.length} returned`); + } + return instrument.id; + } + /** Reads a user back as admin, to check what a write actually stored. */ async findUserById(id: string): Promise { return this.expectJson( @@ -123,6 +152,23 @@ export class ApiClient { return (await response.json()) as User; } + /** + * Bulk-creates one record per entry, and with them the subjects and sessions they name. Returns + * the records the api reports created, which the upload contract scopes to this request alone. + */ + async uploadRecords( + groupId: string, + instrumentId: string, + records: UploadRecord[] + ): Promise<{ subjectId: string }[]> { + const data: UploadInstrumentRecordsData = { groupId, instrumentId, records }; + return this.expectJson<{ subjectId: string }[]>( + this.request.post(`${API}/instrument-records/upload`, { data, headers: this.authHeaders }), + 201, + 'upload instrument records' + ); + } + private async expectJson( pending: ReturnType, status: number,