From 24e37759bc97f131b52b881faf7aca54bec9485a Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 20 Jul 2026 15:29:12 -0400 Subject: [PATCH 1/9] perf(api): batch session creation and scope the upload response `SessionsService` gains a `createMany` that resolves subjects, the user and the group once for the whole batch, associates subjects with the group in a single write, and inserts every session in one call. Session ids are generated up front so the results come back in input order and a caller can pair each session with the entry that produced it. `create` now delegates to it, so there is one implementation rather than two. `upload` uses it, and validates every record before anything is written so a malformed record in the middle of a batch no longer creates sessions that then have to be rolled back. Its response is scoped to the sessions this upload created rather than returning every record in the group for that instrument. Measured over HTTP against a live instance, uploading 25 records: main 85-110 records returned, 51-66 KB, ~242-276 mongo ops this branch 25 records returned, 14.8 KB, ~95 mongo ops The record count on main grows on every upload; here it stays at 25. Fixes a bug along the way. `create` set a session's groupId only when the subject was not already a member of that group, so every visit after a subject's first produced a session with no groupId. A GROUP_MANAGER's Session rule is { groupId: { in: [...] } }, which means those sessions were invisible to them, and uncounted by any group-scoped query including the dashboard summary. From review on this branch: - `upload` now rejects file instruments with UnprocessableEntityException, alongside the existing series rejection, rather than writing them pending. The bulk payload cannot carry a file, this path never attaches one, and the client discards the response ids, so such a record could only ever be incomplete. This supersedes #1422, which set `pending` on that record instead. - The batched group association moved behind `SubjectsService`, where the rest of the subject writes live, replacing `addGroupForSubject` -- which this change had left with no callers -- rather than reaching into `prismaClient.subject` from the sessions service. - `CreateManySessionsData` is derived from `CreateSessionData` instead of restating it, so the two cannot drift. - `SubjectsService.createMany` names the fields it writes rather than spreading the caller's object; `demo.service.ts` hands it a full Prisma row. - Dropped two comments that described what the code used to do. Every claim above is pinned by a test, each verified by mutation: `SessionsService` had no test file at all, so this adds one, and the end-to-end suite now covers the groupId fix -- reinstating the old behaviour returns one of the two sessions instead of both. Refs #1414 Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- .../instrument-records.service.spec.ts | 124 ++++++++++++- .../instrument-records.service.ts | 118 ++++++------ .../__tests__/sessions.service.spec.ts | 173 ++++++++++++++++++ apps/api/src/sessions/sessions.service.ts | 114 ++++++------ .../__tests__/subjects.service.spec.ts | 23 +++ apps/api/src/subjects/subjects.service.ts | 62 ++++--- testing/src/specs/dashboard.spec.ts | 29 +++ testing/src/support/api-client.ts | 12 ++ 8 files changed, 506 insertions(+), 149 deletions(-) create mode 100644 apps/api/src/sessions/__tests__/sessions.service.spec.ts 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..373aa99c3 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,54 @@ 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' }) + ); + }); + + 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 +268,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 +281,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 +293,81 @@ 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 })); + }); + + 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..9dd3fdafc 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,63 @@ 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 + }); - 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..c7bf5c6cd --- /dev/null +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -0,0 +1,173 @@ +import { getModelToken, LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; +import type { Model } from '@douglasneuroinformatics/libnest'; +import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; +import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { RuntimePrismaClient } from '@/core/prisma'; +import { GroupsService } from '@/groups/groups.service'; +import { SubjectsService } from '@/subjects/subjects.service'; + +import { SessionsService } from '../sessions.service'; + +describe('SessionsService', () => { + let sessionsService: SessionsService; + let sessionModel: MockedInstance>; + let groupsService: MockedInstance; + let subjectsService: MockedInstance; + let prismaClient: MockedInstance & { [key: string]: any }; + + 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), + { + provide: PRISMA_CLIENT_TOKEN, + useValue: { + subject: { findMany: vi.fn(), updateMany: vi.fn() }, + user: { findFirst: vi.fn() } + } + } + ] + }).compile(); + + sessionsService = moduleRef.get(SessionsService); + sessionModel = moduleRef.get(getModelToken('Session')); + groupsService = moduleRef.get(GroupsService); + subjectsService = moduleRef.get(SubjectsService); + prismaClient = moduleRef.get(PRISMA_CLIENT_TOKEN); + + subjectsService.createMany.mockResolvedValue([] as any); + subjectsService.addGroupForSubjects.mockResolvedValue({ count: 0 } as any); + prismaClient.subject.findMany.mockResolvedValue([{ groupIds: [], id: 'subject-1' }]); + prismaClient.user.findFirst.mockResolvedValue(null); + 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 () => { + prismaClient.subject.findMany.mockResolvedValueOnce([{ groupIds: ['group-1'], id: 'subject-1' }]); + 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 () => { + prismaClient.subject.findMany.mockResolvedValueOnce([ + { groupIds: ['group-1'], id: 'subject-1' }, + { groupIds: [], id: 'subject-2' } + ]); + 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 () => { + prismaClient.subject.findMany.mockResolvedValueOnce([ + { groupIds: [], id: 'subject-a' }, + { groupIds: [], id: 'subject-b' } + ]); + // 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}`)); + prismaClient.subject.findMany.mockResolvedValueOnce(entries.map((_, i) => ({ groupIds: [], id: `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')]; + prismaClient.subject.findMany.mockResolvedValueOnce([ + { groupIds: [], id: 'subject-a' }, + { groupIds: [], id: 'subject-b' } + ]); + prismaClient.user.findFirst.mockResolvedValueOnce({ id: 'user-1', username: 'someone' }); + + await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE', username: 'someone' }); + + expect(prismaClient.user.findFirst).toHaveBeenCalledOnce(); + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [{ userId: 'user-1' }, { userId: 'user-1' }] + }); + }); + }); + + 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.service.ts b/apps/api/src/sessions/sessions.service.ts index 2b0889424..dce799460 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -1,10 +1,11 @@ import { InjectModel, InjectPrismaClient, LoggingService } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; -import { Injectable, InternalServerErrorException, NotFoundException } from '@nestjs/common'; +import { Injectable, 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 { ObjectId } from 'mongodb'; import { accessibleQuery } from '@/auth/ability.utils'; import type { RuntimePrismaClient } from '@/core/prisma'; @@ -12,6 +13,11 @@ import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; import { SubjectsService } from '@/subjects/subjects.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( @@ -29,54 +35,62 @@ 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; + const [session] = await this.createMany({ + entries: [{ date, subjectData }], + groupId, + type, + username + }); + 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): 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 subjects = await this.resolveSubjects(entries.map((entry) => entry.subjectData)); + + const user: null | Omit = username + ? await this.prismaClient.user.findFirst({ where: { username } }) + : null; + + const group: Group | null = groupId ? await this.groupsService.findById(groupId) : null; + if (group) { + await this.subjectsService.addGroupForSubjects( + subjects.map((subject) => subject.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 } - }))!; + 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) => byId.get(id)!); } async deleteById(id: string, { ability }: EntityOperationOptions = {}) { @@ -126,18 +140,10 @@ 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 }); - } - subject = await this.subjectsService.create(subjectData); - } - return subject; + /** Get each subject if they exist, otherwise create them, in two queries regardless of count. */ + private async resolveSubjects(subjectData: CreateSubjectData[]): Promise { + const ids = Array.from(new Set(subjectData.map((subject) => subject.id))); + await this.subjectsService.createMany(subjectData); + return this.prismaClient.subject.findMany({ where: { id: { in: ids } } }); } } diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index ea12d89d3..058333570 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -47,6 +47,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 = { 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/dashboard.spec.ts b/testing/src/specs/dashboard.spec.ts index 5c41549fb..735675e88 100644 --- a/testing/src/specs/dashboard.spec.ts +++ b/testing/src/specs/dashboard.spec.ts @@ -1,3 +1,4 @@ +import { ApiClient } from '../support/api-client'; import { expect, test } from '../support/fixtures'; test.describe('dashboard', () => { @@ -42,4 +43,32 @@ test.describe('dashboard', () => { await expect(dashboardPage.welcomeHeading).toContainText('Summary of Application State'); }); }); + + // 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, this dashboard included. + // + // Seeded into a group of its own, so the count is exactly what this test created. + test('should keep a returning subject 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); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index aae5072bc..621298d3b 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,6 +1,8 @@ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { Permissions } from '@opendatacapture/schemas/core'; import type { CreateGroupData, Group } from '@opendatacapture/schemas/group'; +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'; @@ -48,6 +50,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()}`; From 4082350cd78b1a34f7204d26eb41f90584555be5 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 14:25:56 -0400 Subject: [PATCH 2/9] refactor(api): resolve batch subjects through the subjects service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveSubjects read subject rows through the prisma client directly, where main's single-subject path went through SubjectsService — Model<'Subject'> is the repository here, and the batched path should not cross that boundary just because it grew a batch. The read lands next to findById as findByIds, and the sessions spec loses its now-unused prisma subject mocks. Co-Authored-By: Claude Fable 5 --- .../__tests__/sessions.service.spec.ts | 21 ++++++++++--------- apps/api/src/sessions/sessions.service.ts | 2 +- .../__tests__/subjects.service.spec.ts | 10 +++++++++ apps/api/src/subjects/subjects.service.ts | 7 +++++++ 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/apps/api/src/sessions/__tests__/sessions.service.spec.ts b/apps/api/src/sessions/__tests__/sessions.service.spec.ts index c7bf5c6cd..a1de758b7 100644 --- a/apps/api/src/sessions/__tests__/sessions.service.spec.ts +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -31,7 +31,6 @@ describe('SessionsService', () => { { provide: PRISMA_CLIENT_TOKEN, useValue: { - subject: { findMany: vi.fn(), updateMany: vi.fn() }, user: { findFirst: vi.fn() } } } @@ -46,7 +45,7 @@ describe('SessionsService', () => { subjectsService.createMany.mockResolvedValue([] as any); subjectsService.addGroupForSubjects.mockResolvedValue({ count: 0 } as any); - prismaClient.subject.findMany.mockResolvedValue([{ groupIds: [], id: 'subject-1' }]); + subjectsService.findByIds.mockResolvedValue([{ groupIds: [], id: 'subject-1' }] as any); prismaClient.user.findFirst.mockResolvedValue(null); sessionModel.createMany.mockResolvedValue({ count: 1 } as any); sessionModel.findMany.mockImplementation(({ where }: any) => @@ -65,7 +64,7 @@ describe('SessionsService', () => { // 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 () => { - prismaClient.subject.findMany.mockResolvedValueOnce([{ groupIds: ['group-1'], id: 'subject-1' }]); + subjectsService.findByIds.mockResolvedValueOnce([{ groupIds: ['group-1'], id: 'subject-1' }] as any); groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); await sessionsService.createMany({ @@ -78,10 +77,10 @@ describe('SessionsService', () => { }); it('should associate the batch with the group in a single write', async () => { - prismaClient.subject.findMany.mockResolvedValueOnce([ + subjectsService.findByIds.mockResolvedValueOnce([ { groupIds: ['group-1'], id: 'subject-1' }, { groupIds: [], id: 'subject-2' } - ]); + ] as any); groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); await sessionsService.createMany({ @@ -106,10 +105,10 @@ describe('SessionsService', () => { }); it('should return the sessions in the order the entries were given, so callers can pair by index', async () => { - prismaClient.subject.findMany.mockResolvedValueOnce([ + subjectsService.findByIds.mockResolvedValueOnce([ { groupIds: [], id: 'subject-a' }, { groupIds: [], id: 'subject-b' } - ]); + ] as any); // 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 }))) @@ -128,7 +127,9 @@ describe('SessionsService', () => { 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}`)); - prismaClient.subject.findMany.mockResolvedValueOnce(entries.map((_, i) => ({ groupIds: [], id: `subject-${i}` }))); + subjectsService.findByIds.mockResolvedValueOnce( + entries.map((_, i) => ({ groupIds: [], id: `subject-${i}` })) as any + ); await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE' }); @@ -138,10 +139,10 @@ describe('SessionsService', () => { 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')]; - prismaClient.subject.findMany.mockResolvedValueOnce([ + subjectsService.findByIds.mockResolvedValueOnce([ { groupIds: [], id: 'subject-a' }, { groupIds: [], id: 'subject-b' } - ]); + ] as any); prismaClient.user.findFirst.mockResolvedValueOnce({ id: 'user-1', username: 'someone' }); await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE', username: 'someone' }); diff --git a/apps/api/src/sessions/sessions.service.ts b/apps/api/src/sessions/sessions.service.ts index dce799460..8331b3056 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -144,6 +144,6 @@ export class SessionsService { private async resolveSubjects(subjectData: CreateSubjectData[]): Promise { const ids = Array.from(new Set(subjectData.map((subject) => subject.id))); await this.subjectsService.createMany(subjectData); - return this.prismaClient.subject.findMany({ where: { id: { in: ids } } }); + return this.subjectsService.findByIds(ids); } } diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index 058333570..d226f16f8 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -185,4 +185,14 @@ describe('SubjectsService', () => { await expect(subjectsService.findById('123')).resolves.toMatchObject({ id: '123' }); }); }); + + describe('findByIds', () => { + it('should query the whole id set in one call, omitting missing ids rather than throwing', async () => { + subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]); + await expect(subjectsService.findByIds(['123', 'does-not-exist'])).resolves.toMatchObject([{ id: '123' }]); + expect(subjectModel.findMany).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ where: expect.objectContaining({ id: { in: ['123', 'does-not-exist'] } }) }) + ); + }); + }); }); diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index 9a9498137..81a8ca4fe 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -168,6 +168,13 @@ export class SubjectsService { return subject; } + /** The subjects among `ids` that exist, in one query; a missing id is omitted rather than an error. */ + async findByIds(ids: string[], { ability }: EntityOperationOptions = {}) { + return this.subjectModel.findMany({ + where: { AND: [accessibleQuery(ability, 'read', 'Subject')], id: { in: ids } } + }); + } + private async querySubjectIdsWithRecords(groupId?: string): Promise { const records = await this.prismaClient.instrumentRecord.findMany({ distinct: ['subjectId'], From 9a40e2cbdbf89f5a9c604c06efafba1d80123b59 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 18:15:26 -0400 Subject: [PATCH 3/9] fix(api): drop the subject read-back and fail loudly on a session read-back gap resolveSubjects read the subjects back only to extract ids the caller already held, so the round trip is gone along with findByIds, which it was the sole caller of. The session read-back now throws when a created id is missing rather than typing a gap as Session, which would have answered 201 with an empty body. The groupId regression test moves out of the dashboard spec into its own sessions spec, and a new upload spec pins the response contract end to end: a batch upload answers with exactly the records it created, not everything in the group. Co-Authored-By: Claude Fable 5 --- .../__tests__/sessions.service.spec.ts | 34 +++++++++---------- apps/api/src/sessions/sessions.service.ts | 24 ++++++------- .../__tests__/subjects.service.spec.ts | 10 ------ apps/api/src/subjects/subjects.service.ts | 7 ---- testing/src/specs/dashboard.spec.ts | 29 ---------------- testing/src/specs/sessions.spec.ts | 32 +++++++++++++++++ testing/src/specs/upload.spec.ts | 28 +++++++++++++++ testing/src/support/api-client.ts | 34 +++++++++++++++++++ 8 files changed, 122 insertions(+), 76 deletions(-) create mode 100644 testing/src/specs/sessions.spec.ts diff --git a/apps/api/src/sessions/__tests__/sessions.service.spec.ts b/apps/api/src/sessions/__tests__/sessions.service.spec.ts index a1de758b7..d5221f0e1 100644 --- a/apps/api/src/sessions/__tests__/sessions.service.spec.ts +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -2,6 +2,7 @@ import { getModelToken, LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneur import type { Model } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { InternalServerErrorException } from '@nestjs/common'; import { Test } from '@nestjs/testing'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -45,7 +46,6 @@ describe('SessionsService', () => { subjectsService.createMany.mockResolvedValue([] as any); subjectsService.addGroupForSubjects.mockResolvedValue({ count: 0 } as any); - subjectsService.findByIds.mockResolvedValue([{ groupIds: [], id: 'subject-1' }] as any); prismaClient.user.findFirst.mockResolvedValue(null); sessionModel.createMany.mockResolvedValue({ count: 1 } as any); sessionModel.findMany.mockImplementation(({ where }: any) => @@ -64,7 +64,6 @@ describe('SessionsService', () => { // 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 () => { - subjectsService.findByIds.mockResolvedValueOnce([{ groupIds: ['group-1'], id: 'subject-1' }] as any); groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); await sessionsService.createMany({ @@ -77,10 +76,6 @@ describe('SessionsService', () => { }); it('should associate the batch with the group in a single write', async () => { - subjectsService.findByIds.mockResolvedValueOnce([ - { groupIds: ['group-1'], id: 'subject-1' }, - { groupIds: [], id: 'subject-2' } - ] as any); groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); await sessionsService.createMany({ @@ -105,10 +100,6 @@ describe('SessionsService', () => { }); it('should return the sessions in the order the entries were given, so callers can pair by index', async () => { - subjectsService.findByIds.mockResolvedValueOnce([ - { groupIds: [], id: 'subject-a' }, - { groupIds: [], id: 'subject-b' } - ] as any); // 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 }))) @@ -127,9 +118,6 @@ describe('SessionsService', () => { 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}`)); - subjectsService.findByIds.mockResolvedValueOnce( - entries.map((_, i) => ({ groupIds: [], id: `subject-${i}` })) as any - ); await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE' }); @@ -139,10 +127,6 @@ describe('SessionsService', () => { 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')]; - subjectsService.findByIds.mockResolvedValueOnce([ - { groupIds: [], id: 'subject-a' }, - { groupIds: [], id: 'subject-b' } - ] as any); prismaClient.user.findFirst.mockResolvedValueOnce({ id: 'user-1', username: 'someone' }); await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE', username: 'someone' }); @@ -152,6 +136,22 @@ describe('SessionsService', () => { data: [{ userId: 'user-1' }, { userId: 'user-1' }] }); }); + + // 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); + }); }); describe('create', () => { diff --git a/apps/api/src/sessions/sessions.service.ts b/apps/api/src/sessions/sessions.service.ts index 8331b3056..62f42ec52 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -1,10 +1,9 @@ import { InjectModel, InjectPrismaClient, LoggingService } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; -import { Injectable, NotFoundException } from '@nestjs/common'; +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, User } from '@prisma/client'; import { ObjectId } from 'mongodb'; import { accessibleQuery } from '@/auth/ability.utils'; @@ -56,7 +55,7 @@ export class SessionsService { } this.loggingService.debug({ message: `Attempting to create ${entries.length} session(s)` }); - const subjects = await this.resolveSubjects(entries.map((entry) => entry.subjectData)); + await this.subjectsService.createMany(entries.map((entry) => entry.subjectData)); const user: null | Omit = username ? await this.prismaClient.user.findFirst({ where: { username } }) @@ -65,7 +64,7 @@ export class SessionsService { const group: Group | null = groupId ? await this.groupsService.findById(groupId) : null; if (group) { await this.subjectsService.addGroupForSubjects( - subjects.map((subject) => subject.id), + Array.from(new Set(entries.map((entry) => entry.subjectData.id))), group.id ); } @@ -90,7 +89,13 @@ export class SessionsService { where: { id: { in: ids } } }); const byId = new Map(created.map((session) => [session.id, session])); - return ids.map((id) => byId.get(id)!); + return ids.map((id) => { + const session = byId.get(id); + if (!session) { + throw new InternalServerErrorException(`Failed to read back created session with id: ${id}`); + } + return session; + }); } async deleteById(id: string, { ability }: EntityOperationOptions = {}) { @@ -139,11 +144,4 @@ export class SessionsService { } return session; } - - /** Get each subject if they exist, otherwise create them, in two queries regardless of count. */ - private async resolveSubjects(subjectData: CreateSubjectData[]): Promise { - const ids = Array.from(new Set(subjectData.map((subject) => subject.id))); - await this.subjectsService.createMany(subjectData); - return this.subjectsService.findByIds(ids); - } } diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index d226f16f8..058333570 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -185,14 +185,4 @@ describe('SubjectsService', () => { await expect(subjectsService.findById('123')).resolves.toMatchObject({ id: '123' }); }); }); - - describe('findByIds', () => { - it('should query the whole id set in one call, omitting missing ids rather than throwing', async () => { - subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]); - await expect(subjectsService.findByIds(['123', 'does-not-exist'])).resolves.toMatchObject([{ id: '123' }]); - expect(subjectModel.findMany).toHaveBeenCalledExactlyOnceWith( - expect.objectContaining({ where: expect.objectContaining({ id: { in: ['123', 'does-not-exist'] } }) }) - ); - }); - }); }); diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index 81a8ca4fe..9a9498137 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -168,13 +168,6 @@ export class SubjectsService { return subject; } - /** The subjects among `ids` that exist, in one query; a missing id is omitted rather than an error. */ - async findByIds(ids: string[], { ability }: EntityOperationOptions = {}) { - return this.subjectModel.findMany({ - where: { AND: [accessibleQuery(ability, 'read', 'Subject')], id: { in: ids } } - }); - } - private async querySubjectIdsWithRecords(groupId?: string): Promise { const records = await this.prismaClient.instrumentRecord.findMany({ distinct: ['subjectId'], diff --git a/testing/src/specs/dashboard.spec.ts b/testing/src/specs/dashboard.spec.ts index 735675e88..5c41549fb 100644 --- a/testing/src/specs/dashboard.spec.ts +++ b/testing/src/specs/dashboard.spec.ts @@ -1,4 +1,3 @@ -import { ApiClient } from '../support/api-client'; import { expect, test } from '../support/fixtures'; test.describe('dashboard', () => { @@ -43,32 +42,4 @@ test.describe('dashboard', () => { await expect(dashboardPage.welcomeHeading).toContainText('Summary of Application State'); }); }); - - // 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, this dashboard included. - // - // Seeded into a group of its own, so the count is exactly what this test created. - test('should keep a returning subject 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); - }); }); diff --git a/testing/src/specs/sessions.spec.ts b/testing/src/specs/sessions.spec.ts new file mode 100644 index 000000000..a9b88d9ca --- /dev/null +++ b/testing/src/specs/sessions.spec.ts @@ -0,0 +1,32 @@ +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 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); + }); +}); diff --git a/testing/src/specs/upload.spec.ts b/testing/src/specs/upload.spec.ts index 1d4cf9273..d02d34c6b 100644 --- a/testing/src/specs/upload.spec.ts +++ b/testing/src/specs/upload.spec.ts @@ -5,6 +5,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 +66,25 @@ 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)); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 621298d3b..15a825a8e 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,6 +1,7 @@ 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'; @@ -11,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; @@ -90,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( @@ -135,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, From fa1f4cb19c67157e428601cc08b654f680f2b5d2 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 11 Sep 2026 14:09:34 -0400 Subject: [PATCH 4/9] test(api): pin the demographics createMany keeps for a new subject Nothing asserted that createMany keeps a clinical subject's demographics, so going back to the id-only form would have stayed green. The fixture is a whole Prisma row, so the test also pins that fields the caller may not set are dropped rather than spread through. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLfKsdHT1arfrc2fdT1fWq --- .../__tests__/subjects.service.spec.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index 058333570..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'; @@ -97,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' }]); From fb39d261b8f3de85548bcbc5d4257f6446b77447 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 11 Sep 2026 14:09:35 -0400 Subject: [PATCH 5/9] test(testing): read back the demographics a first session creates Session creation now resolves its subjects through the batched SubjectsService.createMany. This creates a clinical subject over the API and reads it back, so a subject created without its demographics fails end to end. The returning-subject test title also gains its possessive. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLfKsdHT1arfrc2fdT1fWq --- testing/src/specs/sessions.spec.ts | 35 +++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/testing/src/specs/sessions.spec.ts b/testing/src/specs/sessions.spec.ts index a9b88d9ca..ab260abe1 100644 --- a/testing/src/specs/sessions.spec.ts +++ b/testing/src/specs/sessions.spec.ts @@ -1,3 +1,5 @@ +import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; + import { ApiClient } from '../support/api-client'; import { expect, test } from '../support/fixtures'; @@ -8,7 +10,7 @@ test.describe('sessions', () => { // 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 later sessions visible to their group manager', async ({ + test("should keep a returning subject's later sessions visible to their group manager", async ({ api, apiRequestContext, uniqueId @@ -29,4 +31,35 @@ test.describe('sessions', () => { 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' + }); + }); }); From 371c283959769865b55e9d7882dce01a675fba4c Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 11 Sep 2026 14:09:36 -0400 Subject: [PATCH 6/9] test(testing): refuse a file instrument upload end to end The 422 for a bulk upload of a file instrument was covered only by the mocked service test. This drives the real endpoint and asserts that the subject the record names was never created, so no session was either, and that the group holds no record. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLfKsdHT1arfrc2fdT1fWq --- testing/src/specs/upload.spec.ts | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/testing/src/specs/upload.spec.ts b/testing/src/specs/upload.spec.ts index d02d34c6b..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'; @@ -87,4 +89,31 @@ test.describe('upload', () => { 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([]); + }); }); From aeb0f0f021c00fcc2cdbfa8b34dd85c65e27b02e Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Wed, 23 Sep 2026 10:34:38 -0400 Subject: [PATCH 7/9] fix(api): delete the sessions a batch inserted when their read-back fails createMany inserts the sessions and then reads them back. If that read failed, it threw before the caller received the sessions, so neither the upload rollback nor POST /sessions could delete them, and they were left without records. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_015wLMGmHorLgMr7dzbbXjMF --- .../__tests__/sessions.service.spec.ts | 17 ++++++++++ apps/api/src/sessions/sessions.service.ts | 34 ++++++++++++------- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/apps/api/src/sessions/__tests__/sessions.service.spec.ts b/apps/api/src/sessions/__tests__/sessions.service.spec.ts index d5221f0e1..37a9189f9 100644 --- a/apps/api/src/sessions/__tests__/sessions.service.spec.ts +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -152,6 +152,23 @@ describe('SessionsService', () => { }) ).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', () => { diff --git a/apps/api/src/sessions/sessions.service.ts b/apps/api/src/sessions/sessions.service.ts index 62f42ec52..dc36eabc6 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -84,18 +84,13 @@ export class SessionsService { })) }); - 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}`); - } - return session; - }); + // 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 = {}) { @@ -144,4 +139,19 @@ export class SessionsService { } return session; } + + 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}`); + } + return session; + }); + } } From d6966cf6f670230ff42f5d74eac3a01c76242859 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Wed, 23 Sep 2026 10:39:32 -0400 Subject: [PATCH 8/9] fix(api): scope the group and user a new session names to the caller POST /v1/sessions passed no ability, so its group and user lookups read across every group. Any role holding create Session could create a session, and associate a subject, in a group it does not belong to, and stamp it with any user. Both lookups now take the caller's ability, as the upload path already does. A group or user the caller may not read answers 404, and nothing is written before those lookups succeed. An unknown username is refused rather than silently giving a session with no user. The subject writes stay unscoped, because a caller's read Subject rule covers only its own groups and a STANDARD caller holds no update Subject rule. The demo seed and the gateway synchronizer pass no ability and stay system-level. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_015wLMGmHorLgMr7dzbbXjMF --- .../instrument-records.service.spec.ts | 19 ++++- .../instrument-records.service.ts | 21 ++--- .../__tests__/sessions.service.spec.ts | 76 +++++++++++++++---- apps/api/src/sessions/sessions.controller.ts | 4 +- apps/api/src/sessions/sessions.module.ts | 3 +- apps/api/src/sessions/sessions.service.ts | 47 +++++++----- 6 files changed, 121 insertions(+), 49 deletions(-) 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 373aa99c3..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 @@ -217,10 +217,19 @@ describe('InstrumentRecordsService', () => { expect(sessionsService.create).not.toHaveBeenCalled(); expect(sessionsService.createMany).toHaveBeenCalledTimes(1); expect(sessionsService.createMany).toHaveBeenCalledWith( - expect.objectContaining({ groupId: 'group-1', type: 'RETROSPECTIVE', username: 'validuser' }) + 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 }, @@ -300,7 +309,10 @@ describe('InstrumentRecordsService', () => { await instrumentRecordsService.upload({ ...baseUploadData }); expect(usersService.findByUsername).not.toHaveBeenCalled(); - expect(sessionsService.createMany).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 () => { @@ -322,7 +334,8 @@ describe('InstrumentRecordsService', () => { instrumentsService.findById.mockResolvedValue({ ...mockInstrument, validationSchema: { - safeParse: (data: any) => (data.answer === 2 ? { error: { issues }, success: false } : { data, success: true }) + safeParse: (data: any) => + data.answer === 2 ? { error: { issues }, success: false } : { data, success: true } } } as any); diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index 9dd3fdafc..e76b64e51 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -433,15 +433,18 @@ export class InstrumentRecordsService { // 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 - }); + 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 + ); // 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. diff --git a/apps/api/src/sessions/__tests__/sessions.service.spec.ts b/apps/api/src/sessions/__tests__/sessions.service.spec.ts index 37a9189f9..74cff40bc 100644 --- a/apps/api/src/sessions/__tests__/sessions.service.spec.ts +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -1,14 +1,15 @@ -import { getModelToken, LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; +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 } from '@nestjs/common'; +import { InternalServerErrorException, NotFoundException } from '@nestjs/common'; import { Test } from '@nestjs/testing'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it } from 'vitest'; -import type { RuntimePrismaClient } from '@/core/prisma'; +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'; @@ -17,7 +18,7 @@ describe('SessionsService', () => { let sessionModel: MockedInstance>; let groupsService: MockedInstance; let subjectsService: MockedInstance; - let prismaClient: MockedInstance & { [key: string]: any }; + let usersService: MockedInstance; const entry = (id: string) => ({ date: new Date(), subjectData: { id } }); @@ -29,12 +30,7 @@ describe('SessionsService', () => { MockFactory.createForService(GroupsService), MockFactory.createForService(LoggingService), MockFactory.createForService(SubjectsService), - { - provide: PRISMA_CLIENT_TOKEN, - useValue: { - user: { findFirst: vi.fn() } - } - } + MockFactory.createForService(UsersService) ] }).compile(); @@ -42,11 +38,10 @@ describe('SessionsService', () => { sessionModel = moduleRef.get(getModelToken('Session')); groupsService = moduleRef.get(GroupsService); subjectsService = moduleRef.get(SubjectsService); - prismaClient = moduleRef.get(PRISMA_CLIENT_TOKEN); + usersService = moduleRef.get(UsersService); subjectsService.createMany.mockResolvedValue([] as any); subjectsService.addGroupForSubjects.mockResolvedValue({ count: 0 } as any); - prismaClient.user.findFirst.mockResolvedValue(null); sessionModel.createMany.mockResolvedValue({ count: 1 } as any); sessionModel.findMany.mockImplementation(({ where }: any) => Promise.resolve(where.id.in.map((id: string) => ({ id }))) @@ -127,16 +122,67 @@ describe('SessionsService', () => { 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')]; - prismaClient.user.findFirst.mockResolvedValueOnce({ id: 'user-1', username: 'someone' }); + usersService.findByUsername.mockResolvedValueOnce({ id: 'user-1', username: 'someone' } as any); await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE', username: 'someone' }); - expect(prismaClient.user.findFirst).toHaveBeenCalledOnce(); + 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 () => { 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 dc36eabc6..88d8be72c 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -1,16 +1,16 @@ -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 { Prisma, Session, 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 & { @@ -20,11 +20,11 @@ type CreateManySessionsData = Pick, 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 = {}) { @@ -33,13 +33,19 @@ export class SessionsService { }); } - async create({ date, groupId, subjectData, type, username }: CreateSessionData): Promise { - const [session] = await this.createMany({ - entries: [{ date, subjectData }], - groupId, - type, - username - }); + 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!; } @@ -49,19 +55,22 @@ export class SessionsService { * 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): Promise { + 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)` }); - await this.subjectsService.createMany(entries.map((entry) => entry.subjectData)); - - const user: null | Omit = username - ? await this.prismaClient.user.findFirst({ where: { username } }) - : null; + const user = username ? await this.usersService.findByUsername(username, { ability }) : null; + const group: Group | null = groupId ? await this.groupsService.findById(groupId, { ability }) : null; - const group: Group | null = groupId ? await this.groupsService.findById(groupId) : 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))), From 6720e700f8f42d6f3da3d4f61a967d433c179e84 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Wed, 23 Sep 2026 10:39:34 -0400 Subject: [PATCH 9/9] test(testing): refuse a session in a group the caller does not belong to Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_015wLMGmHorLgMr7dzbbXjMF --- testing/src/specs/sessions.spec.ts | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/testing/src/specs/sessions.spec.ts b/testing/src/specs/sessions.spec.ts index ab260abe1..8d556548a 100644 --- a/testing/src/specs/sessions.spec.ts +++ b/testing/src/specs/sessions.spec.ts @@ -1,3 +1,4 @@ +import type { CreateSessionData } from '@opendatacapture/schemas/session'; import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; import { ApiClient } from '../support/api-client'; @@ -62,4 +63,34 @@ test.describe('sessions', () => { 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); + }); });