From bac6420998ef832968ef5b3337a00cc8605c8d3c Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 20 Jul 2026 15:29:12 -0400 Subject: [PATCH 1/3] 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 | 132 ++++++++++++- .../instrument-records.service.ts | 113 ++++++------ .../__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, 510 insertions(+), 148 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 6c39ceebe..d69283b56 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,19 +184,60 @@ 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); }); - 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 () => { @@ -209,7 +250,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 () => { @@ -222,7 +263,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 () => { @@ -234,18 +275,89 @@ 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 return only the records this upload created, not every record in the group', async () => { + await instrumentRecordsService.upload({ ...baseUploadData, groupId: 'group-1' }); + + expect(instrumentRecordModel.findMany).toHaveBeenCalledWith({ + where: { sessionId: { in: ['session-1'] } } + }); + }); + + 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 31b6a0597..f66a223b0 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,60 @@ 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 - }); - - return this.instrumentRecordModel.findMany({ - where: { + 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 - } + 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; } + + 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 1e242e0fc..c2d3ad1ec 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', () => { @@ -6,4 +7,32 @@ test.describe('dashboard', () => { await expect(dashboardPage.pageHeader).toBeVisible(); await expect(dashboardPage.pageHeader).toContainText('Dashboard'); }); + + // 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 6b96e4761..0ab06f1fd 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,5 +1,7 @@ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; 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, User } from '@opendatacapture/schemas/user'; import type { APIRequestContext } from '@playwright/test'; @@ -47,6 +49,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 806dbf83b27600c5ed1b9878821fa25ab839599e Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 14:25:56 -0400 Subject: [PATCH 2/3] 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 e6150dd7d8ce7109851d09c01d8dd2bedc2000f8 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 18:15:26 -0400 Subject: [PATCH 3/3] 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 | 31 +++++++++++++++++ testing/src/support/api-client.ts | 34 +++++++++++++++++++ 8 files changed, 125 insertions(+), 76 deletions(-) create mode 100644 testing/src/specs/sessions.spec.ts create mode 100644 testing/src/specs/upload.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 c2d3ad1ec..1e242e0fc 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', () => { @@ -7,32 +6,4 @@ test.describe('dashboard', () => { await expect(dashboardPage.pageHeader).toBeVisible(); await expect(dashboardPage.pageHeader).toContainText('Dashboard'); }); - - // 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 new file mode 100644 index 000000000..7df177e44 --- /dev/null +++ b/testing/src/specs/upload.spec.ts @@ -0,0 +1,31 @@ +import { expect, test } from '../support/fixtures'; + +/** A minimal payload satisfying the seeded happiness questionnaire's validation schema. */ +const HAPPINESS_RECORD = { + isSatisfiedOverall: true, + personalLifeSatisfaction: 8, + professionalLifeSatisfaction: 7 +}; + +test.describe('instrument record upload', () => { + // 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 0ab06f1fd..156c881e3 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,5 +1,6 @@ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; 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, User } from '@opendatacapture/schemas/user'; @@ -10,6 +11,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; @@ -80,6 +83,37 @@ export class ApiClient { return { credentials: { password, username }, user }; } + /** 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; + } + + /** + * 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,