perf(api): group the has-record subject ids in the database

apps/api/src/subjects/__tests__/subjects.service.spec.ts

@@ -1,4 +1,4 @@
-import { CryptoService, getModelToken, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest';
+import { CryptoService, 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';
@@ -7,6 +7,8 @@ import { Test } from '@nestjs/testing';
 import { pick } from 'lodash-es';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 
+import { AbilityFactory } from '@/auth/ability.factory';
+import { accessibleQuery, createAppAbility } from '@/auth/ability.utils';
 import type { RuntimePrismaClient } from '@/core/prisma';
 
 import { SubjectsService } from '../subjects.service';
@@ -30,7 +32,8 @@ describe('SubjectsService', () => {
             $transaction: vi.fn(),
             instrumentRecord: {
               deleteMany: vi.fn(),
-              findMany: vi.fn()
+              findMany: vi.fn(),
+              groupBy: vi.fn()
             },
             session: {
               deleteMany: vi.fn()
@@ -80,16 +83,9 @@ describe('SubjectsService', () => {
       await expect(subjectsService.find()).resolves.toMatchObject([{ id: '123' }]);
     });
     it('should return the array of subjects with records', async () => {
-      prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ subjectId: '123' }]);
+      prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]);
       subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]);
       await expect(subjectsService.find({ hasRecord: true })).resolves.toMatchObject([{ id: '123' }]);
-      expect(prismaClient.instrumentRecord.findMany).toHaveBeenCalledWith(
-        expect.objectContaining({
-          distinct: ['subjectId'],
-          select: { subjectId: true },
-          where: {}
-        })
-      );
       expect(subjectModel.findMany).toHaveBeenCalledWith(
         expect.objectContaining({
           where: expect.objectContaining({
@@ -98,18 +94,60 @@ describe('SubjectsService', () => {
         })
       );
     });
+    // `distinct` is applied by the prisma query engine, so it returns one row per record over the
+    // wire; grouping asks mongodb for one row per subject instead.
+    it('should group the ids in the database rather than deduplicating them after the fact', async () => {
+      prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]);
+      subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]);
+      await subjectsService.find({ hasRecord: true });
+      expect(prismaClient.instrumentRecord.groupBy).toHaveBeenCalledWith(
+        expect.objectContaining({ by: ['subjectId'] })
+      );
+    });
     it('should filter instrument records by groupId when provided', async () => {
-      prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ subjectId: '123' }]);
+      prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]);
       subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]);
       await subjectsService.find({ groupId: 'group-1', hasRecord: true });
-      expect(prismaClient.instrumentRecord.findMany).toHaveBeenCalledWith(
+      expect(prismaClient.instrumentRecord.groupBy).toHaveBeenCalledWith(
         expect.objectContaining({
-          where: { groupId: 'group-1' }
+          where: { AND: [{}, { groupId: 'group-1' }] }
         })
       );
     });
+    it('should constrain the record query to what the caller may read, so the filter cannot be resolved from other groups records', async () => {
+      prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([]);
+      subjectModel.findMany.mockResolvedValueOnce([]);
+      // Conditions are what make this meaningful: an unconditional read rule yields `{}`, which is
+      // indistinguishable from the ability never having been applied.
+      const ability = createAppAbility([
+        { action: 'read', conditions: { groupId: { in: ['group-1'] } }, subject: 'InstrumentRecord' },
+        { action: 'read', conditions: { groupIds: { hasSome: ['group-1'] } }, subject: 'Subject' }
+      ]);
+      await subjectsService.find({ hasRecord: true }, { ability });
+      const [call] = prismaClient.instrumentRecord.groupBy.mock.lastCall as [{ where: { AND: unknown[] } }];
+      expect(call.where.AND[0]).toStrictEqual(accessibleQuery(ability, 'read', 'InstrumentRecord'));
+    });
+
+    // A STANDARD user holds `create` but not `read` on InstrumentRecord, and this route's guard names
+    // `read Subject`, so they reach the service. `accessibleQuery` throws on an ability with no rule
+    // for the subject at all, which escapes as a 500 rather than the empty list they should see.
+    it('should return an empty list for a caller who may read no records, rather than throwing', async () => {
+      const abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService);
+      const ability = abilityFactory.createForPayload({
+        basePermissionLevel: 'STANDARD',
+        groups: [{ id: 'group-1' }],
+        id: 'user-1'
+      } as any);
+      subjectModel.findMany.mockResolvedValueOnce([]);
+
+      await expect(subjectsService.find({ hasRecord: true }, { ability })).resolves.toStrictEqual([]);
+
+      expect(prismaClient.instrumentRecord.groupBy).not.toHaveBeenCalled();
+      const [call] = subjectModel.findMany.mock.lastCall as [{ where: { AND: unknown[] } }];
+      expect(call.where.AND).toContainEqual({ id: { in: [] } });
+    });
     it('should pass all subject IDs returned by instrument records to the subject query', async () => {
-      prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ subjectId: '123' }, { subjectId: '456' }]);
+      prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }, { subjectId: '456' }]);
       subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }, { id: '456' }]);
       await subjectsService.find({ hasRecord: true });
       expect(subjectModel.findMany).toHaveBeenCalledWith(

apps/api/src/subjects/subjects.service.ts

@@ -4,6 +4,7 @@ import { ConflictException, Injectable, NotFoundException } from '@nestjs/common
 import type { Prisma } from '@prisma/client';
 
 import { accessibleQuery } from '@/auth/ability.utils';
+import type { AppAbility } from '@/auth/auth.types';
 import type { RuntimePrismaClient } from '@/core/prisma';
 import type { EntityOperationOptions } from '@/core/types';
 
@@ -136,7 +137,7 @@ export class SubjectsService {
           AND: [
             accessibleQuery(ability, 'read', 'Subject'),
             groupInput,
-            { id: { in: await this.querySubjectIdsWithRecords(groupId) } }
+            { id: { in: await this.querySubjectIdsWithRecords(groupId, ability) } }
           ]
         }
       });
@@ -158,12 +159,36 @@ export class SubjectsService {
     return subject;
   }
 
-  private async querySubjectIdsWithRecords(groupId?: string): Promise<string[]> {
-    const records = await this.prismaClient.instrumentRecord.findMany({
-      distinct: ['subjectId'],
-      select: { subjectId: true },
-      where: groupId ? { groupId } : {}
+  /**
+   * The ids of subjects having at least one record, for the "with records only" filter.
+   *
+   * Grouped by the database rather than deduplicated after the fact: `distinct` is applied by the
+   * prisma query engine, so it returns one row per *record* over the wire and collapses them only
+   * once they have arrived.
+   *
+   * Deliberately not expressed as a `instrumentRecords: { some: ... }` relation filter on Subject.
+   * On mongodb prisma compiles that into a $lookup over the whole record collection, which measured
+   * far slower than either form here and carries the 100 MiB per-document ceiling that made the same
+   * construct fail outright elsewhere.
+   */
+  private async querySubjectIdsWithRecords(groupId?: string, ability?: AppAbility): Promise<string[]> {
+    // `accessibleQuery` throws rather than returning a restrictive filter when the ability holds no
+    // rule for the subject at all, and a STANDARD user holds `create` but not `read` on
+    // InstrumentRecord. This route's guard names `read Subject`, so such a caller reaches here; no
+    // readable records means no subjects qualify.
+    if (ability && !ability.can('read', 'InstrumentRecord')) {
+      return [];
+    }
+    // The accessibleQuery clause also drops records whose groupId is null: a group manager's rule is
+    // `groupId: { in: [...] }`, and prisma's `in` never matches null. That is intended — an
+    // admin-created, ungrouped record should not make a subject count as "with records" for a
+    // manager — but it is invisible at this call site, so it is recorded here.
+    const groups = await this.prismaClient.instrumentRecord.groupBy({
+      by: ['subjectId'],
+      where: {
+        AND: [accessibleQuery(ability, 'read', 'InstrumentRecord'), groupId ? { groupId } : {}]
+      }
     });
-    return records.map((r) => r.subjectId);
+    return groups.map((group) => group.subjectId);
   }
 }

apps/web/src/routes/_app/datahub/index.tsx

@@ -58,7 +58,11 @@ const Filters: React.FC<{
   return (
     <DropdownMenu open={isOpen} onOpenChange={setIsOpen}>
       <DropdownMenu.Trigger asChild>
-        <Button className="flex items-center justify-between gap-2" variant="outline">
+        <Button
+          className="flex items-center justify-between gap-2"
+          data-testid="datahub-filters-trigger"
+          variant="outline"
+        >
           {t('common.filters')}
           <ChevronDownIcon className="opacity-50" />
         </Button>
@@ -167,6 +171,7 @@ const Filters: React.FC<{
           </DropdownMenu.Label>
           <DropdownMenu.CheckboxItem
             checked={hasRecords}
+            data-testid="datahub-filter-has-records"
             onCheckedChange={setHasRecords}
             onSelect={(e) => e.preventDefault()}
           >

testing/AGENTS.md

@@ -65,11 +65,16 @@ A page object is only reachable from a spec once it is registered in the `pageMo
 | `appState`             | test option | localStorage first-run gating; both flags default to accepted/complete     |
 | `uniqueId`             | test        | Short random suffix for seeded data                                        |
 | `api`                  | worker      | `ApiClient` as admin — `createGroup()` / `createUser()` for preconditions  |
+| `isolatedGroupManager` | test        | Authenticates into a group created for this test alone; returns the group  |
 | `roleAccount(role)`    | worker      | Seeds a group + user per role once, then caches its token and username     |
 
 Set up preconditions over the API with the `api` fixture rather than by clicking through the UI;
 only drive the UI for the behaviour actually under test.
 
+`roleAccount`'s group is cached per worker and shared by every spec running in it, so a test that
+asserts on **how much** a group contains must use `isolatedGroupManager` instead. A group manager
+reads only their own groups, so a fresh group bounds what the test can see to what it seeded.
+
 Auth is injected as `window.__PLAYWRIGHT_ACCESS_TOKEN__`, which `apps/web`'s
 `src/store/slices/auth.slice.ts` reads on boot. It is memory-only and never persisted.
 

testing/src/pages/_app/datahub/index.page.ts

@@ -3,11 +3,23 @@ import type { Locator, Page } from '@playwright/test';
 import { AppPage } from '../route.page';
 
 export class DatahubPage extends AppPage {
+  readonly filtersTrigger: Locator;
+  readonly hasRecordsFilter: Locator;
   readonly pageHeader: Locator;
   readonly rowActionsTrigger: Locator;
+  readonly rows: Locator;
   constructor(page: Page) {
     super(page);
+    this.filtersTrigger = page.getByTestId('datahub-filters-trigger');
+    this.hasRecordsFilter = page.getByTestId('datahub-filter-has-records');
     this.pageHeader = page.getByTestId('page-header');
     this.rowActionsTrigger = page.getByTestId('row-actions-trigger').first();
+    this.rows = page.getByTestId('data-table-body').getByTestId('data-table-row');
+  }
+
+  /** Opens the filter menu and toggles "With records only", which refetches with `hasRecord=true`. */
+  async toggleWithRecordsOnly() {
+    await this.filtersTrigger.click();
+    await this.hasRecordsFilter.click();
   }
 }

testing/src/specs/datahub.spec.ts

@@ -1,9 +1,63 @@
+import { DatahubPage } from '../pages/_app/datahub/index.page';
 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('data hub', () => {
   test('should display the data hub header', async ({ getPageModel }) => {
     const datahubPage = await getPageModel('/datahub');
     await expect(datahubPage.pageHeader).toBeVisible();
     await expect(datahubPage.pageHeader).toContainText('Data Hub');
   });
+
+  test('should list only subjects holding records once "with records only" is applied', async ({
+    api,
+    isolatedGroupManager,
+    page,
+    uniqueId
+  }) => {
+    // A group of its own, so the row count is exactly what this test seeds. Two subjects exist only
+    // through sessions while a third holds a record, so the filter must drop exactly the recordless
+    // pair — hiding every subject or filtering none would both fail.
+    const group = await isolatedGroupManager();
+    const withRecord = `hasrecord-${uniqueId}`;
+    for (const suffix of ['a', 'b']) {
+      await api.createSession(group.id, { id: `recordless-${uniqueId}-${suffix}` });
+    }
+    await api.uploadRecords(group.id, await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'), [
+      { data: HAPPINESS_RECORD, date: new Date(), subjectId: withRecord }
+    ]);
+
+    const datahubPage = new DatahubPage(page);
+    await datahubPage.goto('/datahub');
+    await expect(datahubPage.rows).toHaveCount(3);
+
+    await datahubPage.toggleWithRecordsOnly();
+
+    await expect(datahubPage.rows).toHaveCount(1);
+    // The cell renders at most the id's first nine characters (the subjectIdDisplayLength default),
+    // so the assertion matches the visible prefix rather than the full seeded id.
+    await expect(datahubPage.rows).toContainText(withRecord.slice(0, 9));
+  });
+
+  // `GET /v1/subjects` is gated on `read Subject`, which a standard user holds, but resolving
+  // `hasRecord` reads instrument records, which they do not. The honest answer is an empty list.
+  test('should answer the with-records filter for a caller who may read no records', async ({
+    apiRequestContext,
+    roleAccount
+  }) => {
+    const { accessToken } = await roleAccount('STANDARD');
+
+    const response = await apiRequestContext.get('/api/v1/subjects?hasRecord=true', {
+      headers: { Authorization: `Bearer ${accessToken}` }
+    });
+
+    expect(response.status()).toBe(200);
+    expect(await response.json()).toStrictEqual([]);
+  });
 });

testing/src/support/api-client.ts

@@ -1,5 +1,8 @@
 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';
 import type { APIRequestContext } from '@playwright/test';
 
@@ -8,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;
@@ -47,6 +52,19 @@ export class ApiClient {
     return group;
   }
 
+  /**
+   * Creates a session, and with it the subject it names. A subject seeded this way holds no
+   * instrument records, which is what distinguishes it under the "with records only" filter.
+   */
+  async createSession(groupId: null | string, subjectData: CreateSubjectData): Promise<Session> {
+    const data: CreateSessionData = { date: new Date(), groupId, subjectData, type: 'IN_PERSON' };
+    return this.expectJson<Session>(
+      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<CreateUserData> = {}): Promise<{ credentials: $LoginCredentials; user: User }> {
     const username = overrides.username ?? `user_${randomId()}`;
@@ -68,6 +86,34 @@ 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<string> {
+    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. This is the
+   * cheapest way to give a subject an instrument record: the export only carries subjects that have
+   * one.
+   */
+  async uploadRecords(groupId: string, instrumentId: string, records: UploadRecord[]): Promise<void> {
+    const data: UploadInstrumentRecordsData = { groupId, instrumentId, records };
+    await this.expectJson(
+      this.request.post(`${API}/instrument-records/upload`, { data, headers: this.authHeaders }),
+      201,
+      'upload instrument records'
+    );
+  }
+
   private async expectJson<T>(
     pending: ReturnType<APIRequestContext['post']>,
     status: number,